From 268e636b2bcd4cda879d59b14f9f7ff24dbfd9e9 Mon Sep 17 00:00:00 2001 From: Albin Kerouanton Date: Wed, 6 Aug 2025 11:02:32 +0200 Subject: [PATCH 1/4] libnet/pmapi: let portmappers specify NAT/fwding rules Add two new fields to portmapperapi.PortBinding: NAT and Forwarding. These can be used by portmappers to specify how they want their callers (e.g. bridge driver) to reconfigure the host firewall to NAT a host port, or allow forwarding to the container port. If portmappers don't want to opt-in to these, they can implement their own firewall rules, and not fill these fields. Signed-off-by: Albin Kerouanton --- daemon/libnetwork/portmapperapi/api.go | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/daemon/libnetwork/portmapperapi/api.go b/daemon/libnetwork/portmapperapi/api.go index 932a70feac..d161a068da 100644 --- a/daemon/libnetwork/portmapperapi/api.go +++ b/daemon/libnetwork/portmapperapi/api.go @@ -92,9 +92,25 @@ func (pbReq PortBindingReq) Compare(other PortBindingReq) int { } type PortBinding struct { + // PortBinding contains the port binding information reported through the + // Engine API. types.PortBinding // Mapper is the name of the port mapper used to process this PortBinding. Mapper string + + // NAT represents the host IP and port that should be NATed to the + // container IP and port specified in types.PortBinding. When set, callers + // of the port mapper should reconfigure the host firewall. When it's not + // set, callers won't reconfigure the host firewall. + // + // If the address is invalid, or a non-unicast address, or the port is 0, + // it's treated as an error. If both Forwarding and NAT are specified, NAT + // takes precedence. + NAT netip.AddrPort + // Forwarding indicates whether callers of the port mapper should update + // the host firewall to allow traffic forwarding to IP:Port. + Forwarding bool + // BoundSocket is used to reserve a host port for the binding. If the // userland proxy is in-use, it's passed to the proxy when the proxy is // started, then it's closed and set to nil here. From 9d9b05446c2f5e2f9d7f1f2e0cc9bfb82ae243c2 Mon Sep 17 00:00:00 2001 From: Albin Kerouanton Date: Fri, 8 Aug 2025 17:36:14 +0200 Subject: [PATCH 2/4] libnet/pm/nat: move back fw / proxy steps into the bridge driver Since commit 4e246efcd, individual portmappers are responsible for setting up firewall rules and proxying according to their needs. This change moves the responsibility back to the bridge driver, removing unnecessary code duplication across portmappers. For now, only the nat portmapper takes advantage of this. This partially reverts commit 4e246efcd. Signed-off-by: Albin Kerouanton --- daemon/daemon_unix.go | 3 +- .../libnetwork/drivers/bridge/bridge_linux.go | 28 +-- .../drivers/bridge/port_mapping_linux.go | 188 +++++++++++++++--- .../drivers/bridge/port_mapping_linux_test.go | 14 +- daemon/libnetwork/drivers_linux.go | 11 +- .../portmappers/nat/mapper_linux.go | 72 +------ 6 files changed, 184 insertions(+), 132 deletions(-) diff --git a/daemon/daemon_unix.go b/daemon/daemon_unix.go index eb74c88b55..9ee29059cb 100644 --- a/daemon/daemon_unix.go +++ b/daemon/daemon_unix.go @@ -933,7 +933,8 @@ func networkPlatformOptions(conf *config.Config) []nwconfig.Option { "DisableFilterForwardDrop": conf.BridgeConfig.DisableFilterForwardDrop, "EnableIPTables": conf.BridgeConfig.EnableIPTables, "EnableIP6Tables": conf.BridgeConfig.EnableIP6Tables, - "Hairpin": !conf.EnableUserlandProxy || conf.UserlandProxyPath == "", + "EnableProxy": conf.EnableUserlandProxy && conf.UserlandProxyPath != "", + "ProxyPath": conf.UserlandProxyPath, "AllowDirectRouting": conf.BridgeConfig.AllowDirectRouting, "AcceptFwMark": conf.BridgeConfig.BridgeAcceptFwMark, }, diff --git a/daemon/libnetwork/drivers/bridge/bridge_linux.go b/daemon/libnetwork/drivers/bridge/bridge_linux.go index 32f002e20d..7e193e41a8 100644 --- a/daemon/libnetwork/drivers/bridge/bridge_linux.go +++ b/daemon/libnetwork/drivers/bridge/bridge_linux.go @@ -69,10 +69,11 @@ type configuration struct { DisableFilterForwardDrop bool EnableIPTables bool EnableIP6Tables bool - // Hairpin indicates whether packets sent from a container to a host port - // published by another container on the same bridge network should be - // hairpinned. - Hairpin bool + // EnableProxy indicates whether the userland proxy should be used for NAT + // port-mappings that can't be fulfilled with firewall rules alone. This + // must not be true if ProxyPath is empty. + EnableProxy bool + ProxyPath string AllowDirectRouting bool AcceptFwMark string } @@ -472,15 +473,6 @@ func (n *bridgeNetwork) gwMode(v firewaller.IPVersion) gwMode { return n.config.GwModeIPv6 } -func (n *bridgeNetwork) hairpin() bool { - n.Lock() - defer n.Unlock() - if n.driver == nil { - return false - } - return n.driver.config.Hairpin -} - func (n *bridgeNetwork) portMappers() *drvregistry.PortMappers { n.Lock() defer n.Unlock() @@ -525,7 +517,7 @@ func (d *driver) configure(option map[string]any) error { d.firewaller, err = newFirewaller(context.Background(), firewaller.Config{ IPv4: config.EnableIPTables, IPv6: config.EnableIP6Tables, - Hairpin: config.Hairpin, + Hairpin: !config.EnableProxy, AllowDirectRouting: config.AllowDirectRouting, WSL2Mirrored: isRunningUnderWSL2MirroredMode(context.Background()), }) @@ -853,8 +845,8 @@ func (d *driver) createNetwork(ctx context.Context, config *networkConfiguration } // Module br_netfilter needs to be loaded with net.bridge.bridge-nf-call-ip[6]tables - // enabled to implement icc=false, or DNAT when hairpin mode is enabled. - enableBrNfCallIptables := !config.EnableICC || d.config.Hairpin + // enabled to implement icc=false, or DNAT when the userland-proxy is disabled. + enableBrNfCallIptables := !config.EnableICC || !d.config.EnableProxy // Conditionally queue setup steps depending on configuration values. for _, step := range []struct { @@ -906,7 +898,7 @@ func (d *driver) createNetwork(ctx context.Context, config *networkConfiguration }, // Setup Loopback Addresses Routing - {d.config.Hairpin, "setupLoopbackAddressesRouting", setupLoopbackAddressesRouting}, + {!d.config.EnableProxy, "setupLoopbackAddressesRouting", setupLoopbackAddressesRouting}, // Setup DefaultGatewayIPv4 {config.DefaultGatewayIPv4 != nil, "setupGatewayIPv4", setupGatewayIPv4}, @@ -1185,7 +1177,7 @@ func (d *driver) CreateEndpoint(ctx context.Context, nid, eid string, ifInfo dri return fmt.Errorf("adding interface %s to bridge %s failed: %v", hostIfName, config.BridgeName, err) } - if dconfig.Hairpin { + if !dconfig.EnableProxy { err = setHairpinMode(d.nlh, host, true) if err != nil { return err diff --git a/daemon/libnetwork/drivers/bridge/port_mapping_linux.go b/daemon/libnetwork/drivers/bridge/port_mapping_linux.go index f0cb7a117f..5b52540d3e 100644 --- a/daemon/libnetwork/drivers/bridge/port_mapping_linux.go +++ b/daemon/libnetwork/drivers/bridge/port_mapping_linux.go @@ -5,15 +5,22 @@ import ( "errors" "fmt" "net" + "net/netip" + "os" "slices" "github.com/containerd/log" - "github.com/moby/moby/v2/daemon/internal/sliceutil" + "github.com/moby/moby/v2/daemon/libnetwork/drvregistry" "github.com/moby/moby/v2/daemon/libnetwork/netutils" + "github.com/moby/moby/v2/daemon/libnetwork/portallocator" + "github.com/moby/moby/v2/daemon/libnetwork/portmapper" "github.com/moby/moby/v2/daemon/libnetwork/portmapperapi" "github.com/moby/moby/v2/daemon/libnetwork/types" ) +// Allow unit tests to supply a dummy StartProxy. +var startProxy = portmapper.StartProxy + // addPortMappings takes cfg, the configuration for port mappings, selects host // ports when ranges are given, binds host ports to check they're available and // reserve them, starts docker-proxy if required, and sets up iptables @@ -72,19 +79,11 @@ func (n *bridgeNetwork) addPortMappings( continue } - pm, err := pms.Get(c.Mapper) + newB, err := n.mapPorts(ctx, pms, toBind) if err != nil { return nil, err } - - newB, err := pm.MapPorts(ctx, toBind, n.firewallerNetwork) - if err != nil { - return nil, err - } - bindings = append(bindings, sliceutil.Map(newB, func(b portmapperapi.PortBinding) portmapperapi.PortBinding { - b.Mapper = c.Mapper - return b - })...) + bindings = append(bindings, newB...) // Reset toBind now the ports are bound. toBind = toBind[:0] @@ -93,6 +92,96 @@ func (n *bridgeNetwork) addPortMappings( return bindings, nil } +// mapPorts calls the port mapper used to map the ports in reqs, applies the firewall rules requested by that portmapper, +// and starts userland proxies if needed. It returns an error if it fails on any of these steps, and rolls back any +// changes it made. Caller must ensure that reqs is non-empty and all requests have the same Mapper set. +func (n *bridgeNetwork) mapPorts(ctx context.Context, pms *drvregistry.PortMappers, reqs []portmapperapi.PortBindingReq) (_ []portmapperapi.PortBinding, retErr error) { + mapper := reqs[0].Mapper + pm, err := pms.Get(mapper) + if err != nil { + return nil, err + } + + bindings, err := pm.MapPorts(ctx, reqs, n.firewallerNetwork) + if err != nil { + return nil, err + } + defer func() { + if retErr != nil { + if err := pm.UnmapPorts(ctx, bindings, n.firewallerNetwork); err != nil { + log.G(ctx).WithFields(log.Fields{ + "bindings": bindings, + "error": err, + "origErr": retErr, + }).Warn("Failed to unmap port bindings after error") + } + return + } + }() + + for i := range bindings { + // Make sure that Mapper is correctly set such that UnmapPorts call the right portmapper. + bindings[i].Mapper = mapper + } + + fwPorts := collectFirewallPorts(bindings) + if err := n.firewallerNetwork.AddPorts(ctx, fwPorts); err != nil { + return nil, err + } + defer func() { + if retErr != nil { + if err := n.firewallerNetwork.DelPorts(ctx, fwPorts); err != nil { + log.G(ctx).WithFields(log.Fields{ + "bindings": bindings, + "error": err, + "origErr": retErr, + }).Warn("Failed to remove firewall rules after error") + } + } + }() + + // Start userland proxy processes. + defer func() { + if retErr != nil { + for _, pb := range bindings { + if pb.StopProxy == nil { + continue + } + if err := pb.StopProxy(); err != nil { + log.G(ctx).WithFields(log.Fields{ + "binding": pb.PortBinding, + "error": err, + }).Warnf("failed to stop userland proxy for port mapping") + } + } + } + }() + if n.driver.config.EnableProxy { + for i, pb := range bindings { + if pb.BoundSocket == nil || pb.RootlesskitUnsupported || pb.StopProxy != nil { + continue + } + if err := portallocator.DetachSocketFilter(bindings[i].BoundSocket); err != nil { + return nil, fmt.Errorf("failed to detach socket filter for port mapping %s: %w", bindings[i].PortBinding, err) + } + var err error + bindings[i].StopProxy, err = startProxy(pb.ChildPortBinding(), n.driver.config.ProxyPath, pb.BoundSocket) + if err != nil { + return nil, fmt.Errorf("failed to start userland proxy for port mapping %s: %w", pb.PortBinding, err) + } + if err := bindings[i].BoundSocket.Close(); err != nil { + log.G(ctx).WithFields(log.Fields{ + "error": err, + "mapping": pb.PortBinding, + }).Warnf("failed to close proxy socket") + } + bindings[i].BoundSocket = nil + } + } + + return bindings, nil +} + // sortAndNormPBs transforms cfg into a list of portBindingReq, with all fields // normalized: // @@ -123,7 +212,6 @@ func (n *bridgeNetwork) sortAndNormPBs( containerIPv6 = ep.addrv6.IP } - hairpin := n.hairpin() disableNAT4, disableNAT6 := n.getNATDisabled() add4 := !ep.portBindingState.ipv4 && pbmReq.ipv4 || (disableNAT4 && !ep.portBindingState.routed && pbmReq.routed) @@ -146,7 +234,7 @@ func (n *bridgeNetwork) sortAndNormPBs( // This change was added to keep backward compatibility containerIP := containerIPv6 if containerIPv6 == nil && pbmReq.ipv4 && add6 { - if hairpin { + if !n.driver.config.EnableProxy { // There's no way to map from host-IPv6 to container-IPv4 with the userland proxy // disabled. // If that is required, don't treat it as an error because, as networks are @@ -189,21 +277,6 @@ func needSamePort(a, b portmapperapi.PortBindingReq) bool { a.HostPortEnd == b.HostPortEnd } -// mergeChildHostIPs take a slice of PortBinding and returns a slice of -// types.PortBinding, where the HostIP in each of the results has the -// value of ChildHostIP from the input (if present). -func mergeChildHostIPs(pbs []portmapperapi.PortBinding) []types.PortBinding { - res := make([]types.PortBinding, 0, len(pbs)) - for _, b := range pbs { - pb := b.PortBinding - if b.ChildHostIP != nil { - pb.HostIP = b.ChildHostIP - } - res = append(res, pb) - } - return res -} - // configurePortBindingIPv4 returns a new port binding with the HostIP field // populated and true, if a binding is required. Else, false and an empty // binding. @@ -343,6 +416,15 @@ func (n *bridgeNetwork) unmapPBs(ctx context.Context, bindings []portmapperapi.P if err := pm.UnmapPorts(ctx, []portmapperapi.PortBinding{b}, n.firewallerNetwork); err != nil { errs = append(errs, fmt.Errorf("unmapping port binding %s: %w", b.PortBinding, err)) } + if b.StopProxy != nil { + if err := b.StopProxy(); err != nil && !errors.Is(err, os.ErrProcessDone) { + errs = append(errs, fmt.Errorf("unmapping port binding %s: failed to stop userland proxy: %w", b.PortBinding, err)) + } + } + } + + if err := n.firewallerNetwork.DelPorts(ctx, collectFirewallPorts(bindings)); err != nil { + return err } return errors.Join(errs...) @@ -365,7 +447,55 @@ func (n *bridgeNetwork) reapplyPerPortIptables() { } } - if err := n.firewallerNetwork.AddPorts(context.Background(), mergeChildHostIPs(allPBs)); err != nil { + if err := n.firewallerNetwork.AddPorts(context.Background(), collectFirewallPorts(allPBs)); err != nil { log.G(context.TODO()).Warnf("Failed to reconfigure NAT: %s", err) } } + +// collectFirewallPorts collects all the types.PortBinding needed to +// reconfigure the host firewall for a given list of port bindings. If one of +// the pbs is NATed, but has an invalid NAT field (i.e. multicast address, or a +// port 0), an error is returned. +func collectFirewallPorts(pbs []portmapperapi.PortBinding) []types.PortBinding { + var fwPBs []types.PortBinding + for _, pb := range pbs { + if pb.NAT.IsValid() { + if pb.NAT.Addr().IsMulticast() || pb.NAT.Port() == 0 { + log.G(context.Background()).WithFields(log.Fields{"pb": pb}).Error("invalid NAT address") + continue + } + fwPBs = append(fwPBs, toNATBinding(pb)) + } else if pb.Forwarding { + fwPBs = append(fwPBs, toFwdBinding(pb)) + } + } + return fwPBs +} + +// toNATBinding converts a portmapperapi.PortBinding to a types.PortBinding +// that can be passed to firewaller.Network for setting up a NAT rule. +func toNATBinding(pb portmapperapi.PortBinding) types.PortBinding { + return types.PortBinding{ + IP: pb.IP, + Port: pb.Port, + Proto: pb.Proto, + HostIP: pb.NAT.Addr().AsSlice(), + HostPort: pb.NAT.Port(), + HostPortEnd: pb.NAT.Port(), + } +} + +// toFwdBinding converts a portmapperapi.PortBinding to a types.PortBinding +// that can be passed to firewaller.Network for setting up forwarding. +func toFwdBinding(pb portmapperapi.PortBinding) types.PortBinding { + unspecAddr := netip.IPv4Unspecified() + if pb.IP.To4() == nil { + unspecAddr = netip.IPv6Unspecified() + } + return types.PortBinding{ + IP: pb.IP, + Port: pb.Port, + Proto: pb.Proto, + HostIP: unspecAddr.AsSlice(), + } +} diff --git a/daemon/libnetwork/drivers/bridge/port_mapping_linux_test.go b/daemon/libnetwork/drivers/bridge/port_mapping_linux_test.go index 3b33186b49..3bd5725adf 100644 --- a/daemon/libnetwork/drivers/bridge/port_mapping_linux_test.go +++ b/daemon/libnetwork/drivers/bridge/port_mapping_linux_test.go @@ -563,7 +563,6 @@ func TestAddPortMappings(t *testing.T) { cfg: []portmapperapi.PortBindingReq{ {PortBinding: types.PortBinding{Proto: types.TCP, Port: 22, HostIP: net.IPv6loopback}}, }, - hairpin: true, expLogs: []string{"Cannot map from IPv6 to an IPv4-only container because the userland proxy is disabled"}, }, { @@ -573,7 +572,6 @@ func TestAddPortMappings(t *testing.T) { cfg: []portmapperapi.PortBindingReq{ {PortBinding: types.PortBinding{Proto: types.TCP, Port: 22}}, }, - hairpin: true, expLogs: []string{"Cannot map from default host binding address to an IPv4-only container because the userland proxy is disabled"}, }, { @@ -718,7 +716,7 @@ func TestAddPortMappings(t *testing.T) { // Mock the startProxy function used by the code under test. proxies := map[proxyCall]bool{} // proxy -> is not stopped - startProxy := func(pb types.PortBinding, listenSock *os.File) (stop func() error, retErr error) { + startProxy = func(pb types.PortBinding, _ string, listenSock *os.File) (stop func() error, retErr error) { if tc.busyPortIPv4 > 0 && tc.busyPortIPv4 == int(pb.HostPort) && pb.HostIP.To4() != nil { return nil, errors.New("busy port") } @@ -768,9 +766,7 @@ func TestAddPortMappings(t *testing.T) { pms := &drvregistry.PortMappers{} err := nat.Register(pms, nat.Config{ - RlkClient: pdc, - EnableProxy: tc.enableProxy, - StartProxy: startProxy, + RlkClient: pdc, }) assert.NilError(t, err) err = routed.Register(pms) @@ -791,7 +787,7 @@ func TestAddPortMappings(t *testing.T) { netlabel.GenericData: &configuration{ EnableIPTables: true, EnableIP6Tables: true, - Hairpin: tc.hairpin, + EnableProxy: tc.enableProxy, }, } err = n.driver.configure(genericOption) @@ -892,8 +888,8 @@ func TestAddPortMappings(t *testing.T) { } // Check a docker-proxy was started and stopped for each expected port binding. + expProxies := map[proxyCall]bool{} if tc.enableProxy { - expProxies := map[proxyCall]bool{} for _, expPB := range tc.expPBs { hip := expChildIP(expPB.HostIP) is4 := hip.To4() != nil @@ -905,8 +901,8 @@ func TestAddPortMappings(t *testing.T) { expPB.IP, int(expPB.Port)) expProxies[p] = tc.expReleaseErr != "" } - assert.Check(t, is.DeepEqual(expProxies, proxies)) } + assert.Check(t, is.DeepEqual(expProxies, proxies)) // Check the port driver has seen the expected port mappings and no others, // and that they have all been closed. diff --git a/daemon/libnetwork/drivers_linux.go b/daemon/libnetwork/drivers_linux.go index d5037b86a0..a4be36ed51 100644 --- a/daemon/libnetwork/drivers_linux.go +++ b/daemon/libnetwork/drivers_linux.go @@ -3,7 +3,6 @@ package libnetwork import ( "context" "fmt" - "os" "github.com/moby/moby/v2/daemon/libnetwork/config" "github.com/moby/moby/v2/daemon/libnetwork/datastore" @@ -16,10 +15,8 @@ import ( "github.com/moby/moby/v2/daemon/libnetwork/drivers/overlay" "github.com/moby/moby/v2/daemon/libnetwork/drvregistry" "github.com/moby/moby/v2/daemon/libnetwork/internal/rlkclient" - "github.com/moby/moby/v2/daemon/libnetwork/portmapper" "github.com/moby/moby/v2/daemon/libnetwork/portmappers/nat" "github.com/moby/moby/v2/daemon/libnetwork/portmappers/routed" - "github.com/moby/moby/v2/daemon/libnetwork/types" ) func registerNetworkDrivers(r driverapi.Registerer, store *datastore.Store, pms *drvregistry.PortMappers, driverConfig func(string) map[string]any) error { @@ -60,13 +57,7 @@ func registerPortMappers(ctx context.Context, r *drvregistry.PortMappers, cfg *c } } - if err := nat.Register(r, nat.Config{ - RlkClient: pdc, - StartProxy: func(pb types.PortBinding, file *os.File) (func() error, error) { - return portmapper.StartProxy(pb, cfg.UserlandProxyPath, file) - }, - EnableProxy: cfg.EnableUserlandProxy && cfg.UserlandProxyPath != "", - }); err != nil { + if err := nat.Register(r, nat.Config{RlkClient: pdc}); err != nil { return fmt.Errorf("registering nat portmapper: %w", err) } diff --git a/daemon/libnetwork/portmappers/nat/mapper_linux.go b/daemon/libnetwork/portmappers/nat/mapper_linux.go index e411e68f45..002ba4c8ee 100644 --- a/daemon/libnetwork/portmappers/nat/mapper_linux.go +++ b/daemon/libnetwork/portmappers/nat/mapper_linux.go @@ -6,7 +6,6 @@ import ( "fmt" "net" "net/netip" - "os" "strconv" "github.com/containerd/log" @@ -23,8 +22,6 @@ type PortDriverClient interface { AddPort(ctx context.Context, proto string, hostIP, childIP netip.Addr, hostPort int) (func() error, error) } -type proxyStarter func(types.PortBinding, *os.File) (func() error, error) - // Register the "nat" port-mapper with libnetwork. func Register(r portmapperapi.Registerer, cfg Config) error { return r.Register(driverName, NewPortMapper(cfg)) @@ -32,24 +29,18 @@ func Register(r portmapperapi.Registerer, cfg Config) error { type PortMapper struct { // pdc is used to interact with rootlesskit port driver. - pdc PortDriverClient - startProxy proxyStarter - enableProxy bool + pdc PortDriverClient } type Config struct { // RlkClient is called by MapPorts to determine the ChildHostIP and ask // rootlesskit to map ports in its netns. - RlkClient PortDriverClient - StartProxy proxyStarter - EnableProxy bool + RlkClient PortDriverClient } func NewPortMapper(cfg Config) PortMapper { return PortMapper{ - pdc: cfg.RlkClient, - startProxy: cfg.StartProxy, - enableProxy: cfg.EnableProxy, + pdc: cfg.RlkClient, } } @@ -102,42 +93,16 @@ func (pm PortMapper) MapPorts(ctx context.Context, cfg []portmapperapi.PortBindi } pb.PortBinding.HostPort = uint16(allocatedPort) pb.PortBinding.HostPortEnd = pb.HostPort + + childHIP, _ := netip.AddrFromSlice(cfg[i].ChildHostIP) + pb.NAT = netip.AddrPortFrom(childHIP, pb.PortBinding.HostPort) + bindings = append(bindings, pb) } if err := configPortDriver(ctx, bindings, pm.pdc); err != nil { return nil, err } - if err := fwn.AddPorts(ctx, mergeChildHostIPs(bindings)); err != nil { - return nil, err - } - - // Start userland proxy processes. - if pm.enableProxy { - for i := range bindings { - if bindings[i].BoundSocket == nil || bindings[i].RootlesskitUnsupported || bindings[i].StopProxy != nil { - continue - } - if err := portallocator.DetachSocketFilter(bindings[i].BoundSocket); err != nil { - return nil, fmt.Errorf("failed to detach socket filter for port mapping %s: %w", bindings[i].PortBinding, err) - } - var err error - bindings[i].StopProxy, err = pm.startProxy( - bindings[i].ChildPortBinding(), bindings[i].BoundSocket, - ) - if err != nil { - return nil, fmt.Errorf("failed to start userland proxy for port mapping %s: %w", - bindings[i].PortBinding, err) - } - if err := bindings[i].BoundSocket.Close(); err != nil { - log.G(ctx).WithFields(log.Fields{ - "error": err, - "mapping": bindings[i].PortBinding, - }).Warnf("failed to close proxy socket") - } - bindings[i].BoundSocket = nil - } - } return bindings, nil } @@ -155,14 +120,6 @@ func (pm PortMapper) UnmapPorts(ctx context.Context, pbs []portmapperapi.PortBin errs = append(errs, err) } } - if pb.StopProxy != nil { - if err := pb.StopProxy(); err != nil && !errors.Is(err, os.ErrProcessDone) { - errs = append(errs, fmt.Errorf("failed to stop userland proxy: %w", err)) - } - } - } - if err := fwn.DelPorts(ctx, mergeChildHostIPs(pbs)); err != nil { - errs = append(errs, err) } for _, pb := range pbs { portallocator.Get().ReleasePort(pb.ChildHostIP, pb.Proto.String(), int(pb.HostPort)) @@ -180,21 +137,6 @@ func setChildHostIP(pdc PortDriverClient, req portmapperapi.PortBindingReq) port return req } -// mergeChildHostIPs take a slice of PortBinding and returns a slice of -// types.PortBinding, where the HostIP in each of the results has the -// value of ChildHostIP from the input (if present). -func mergeChildHostIPs(pbs []portmapperapi.PortBinding) []types.PortBinding { - res := make([]types.PortBinding, 0, len(pbs)) - for _, b := range pbs { - pb := b.PortBinding - if b.ChildHostIP != nil { - pb.HostIP = b.ChildHostIP - } - res = append(res, pb) - } - return res -} - // configPortDriver passes the port binding's details to rootlesskit, and updates the // port binding with callbacks to remove the rootlesskit config (or marks the binding as // unsupported by rootlesskit). From 9b1c4ad3b19321fdb2976dea07a853c8cff39c3a Mon Sep 17 00:00:00 2001 From: Albin Kerouanton Date: Mon, 11 Aug 2025 02:21:34 +0200 Subject: [PATCH 3/4] libnet/pm/routed: don't set up firewall rules directly Instead of setting up firewall rules directly in the routed port mapper, we now rely on the bridge driver to handle firewall reconfiguration. Signed-off-by: Albin Kerouanton --- .../drivers/bridge/port_mapping_linux_test.go | 1 - .../portmappers/routed/mapper_linux.go | 23 +++++++------------ 2 files changed, 8 insertions(+), 16 deletions(-) diff --git a/daemon/libnetwork/drivers/bridge/port_mapping_linux_test.go b/daemon/libnetwork/drivers/bridge/port_mapping_linux_test.go index 3bd5725adf..7286523ebf 100644 --- a/daemon/libnetwork/drivers/bridge/port_mapping_linux_test.go +++ b/daemon/libnetwork/drivers/bridge/port_mapping_linux_test.go @@ -45,7 +45,6 @@ func TestPortMappingConfig(t *testing.T) { config := &configuration{ EnableIPTables: true, - Hairpin: true, } genericOption := make(map[string]any) genericOption[netlabel.GenericData] = config diff --git a/daemon/libnetwork/portmappers/routed/mapper_linux.go b/daemon/libnetwork/portmappers/routed/mapper_linux.go index c3ed62d886..d27f51b1b2 100644 --- a/daemon/libnetwork/portmappers/routed/mapper_linux.go +++ b/daemon/libnetwork/portmappers/routed/mapper_linux.go @@ -4,9 +4,7 @@ import ( "context" "github.com/containerd/log" - "github.com/moby/moby/v2/daemon/internal/sliceutil" "github.com/moby/moby/v2/daemon/libnetwork/portmapperapi" - "github.com/moby/moby/v2/daemon/libnetwork/types" ) const driverName = "routed" @@ -22,16 +20,19 @@ func NewPortMapper() PortMapper { return PortMapper{} } -// MapPorts sets up firewall rules to allow direct remote access to pbs. +// MapPorts returns a PortBinding for every PortBindingReq received, with Forwarding enabled for each. If a HostPort is +// specified, it's logged and ignored. func (pm PortMapper) MapPorts(ctx context.Context, reqs []portmapperapi.PortBindingReq, fwn portmapperapi.Firewaller) ([]portmapperapi.PortBinding, error) { if len(reqs) == 0 { return nil, nil } res := make([]portmapperapi.PortBinding, 0, len(reqs)) - bindings := make([]types.PortBinding, 0, len(reqs)) for _, c := range reqs { - pb := portmapperapi.PortBinding{PortBinding: c.Copy()} + pb := portmapperapi.PortBinding{ + PortBinding: c.Copy(), + Forwarding: true, + } if pb.HostPort != 0 || pb.HostPortEnd != 0 { log.G(ctx).WithFields(log.Fields{"mapping": pb}).Infof( "Host port ignored, because NAT is disabled") @@ -39,19 +40,11 @@ func (pm PortMapper) MapPorts(ctx context.Context, reqs []portmapperapi.PortBind pb.HostPortEnd = 0 } res = append(res, pb) - bindings = append(bindings, pb.PortBinding) - } - - if err := fwn.AddPorts(ctx, bindings); err != nil { - return nil, err } return res, nil } -// UnmapPorts removes firewall rules allowing direct remote access to the pbs. -func (pm PortMapper) UnmapPorts(ctx context.Context, pbs []portmapperapi.PortBinding, fwn portmapperapi.Firewaller) error { - return fwn.DelPorts(ctx, sliceutil.Map(pbs, func(pb portmapperapi.PortBinding) types.PortBinding { - return pb.PortBinding - })) +func (pm PortMapper) UnmapPorts(_ context.Context, _ []portmapperapi.PortBinding, _ portmapperapi.Firewaller) error { + return nil } From fc045ad1396dfd78a38f2aa88edd62acf68015d5 Mon Sep 17 00:00:00 2001 From: Albin Kerouanton Date: Mon, 11 Aug 2025 02:23:44 +0200 Subject: [PATCH 4/4] libnet/pmapi: remove firewaller arg from Map/UnmapPorts Signed-off-by: Albin Kerouanton --- .../drivers/bridge/port_mapping_linux.go | 6 +++--- .../drivers/bridge/port_mapping_linux_test.go | 4 ++-- daemon/libnetwork/drvregistry/portmappers_test.go | 4 ++-- daemon/libnetwork/portmapperapi/api.go | 4 ++-- daemon/libnetwork/portmapperapi/firewaller.go | 14 -------------- daemon/libnetwork/portmappers/nat/mapper_linux.go | 6 +++--- .../portmappers/nat/mapper_linux_test.go | 2 +- .../libnetwork/portmappers/routed/mapper_linux.go | 4 ++-- 8 files changed, 15 insertions(+), 29 deletions(-) delete mode 100644 daemon/libnetwork/portmapperapi/firewaller.go diff --git a/daemon/libnetwork/drivers/bridge/port_mapping_linux.go b/daemon/libnetwork/drivers/bridge/port_mapping_linux.go index 5b52540d3e..81307cb84a 100644 --- a/daemon/libnetwork/drivers/bridge/port_mapping_linux.go +++ b/daemon/libnetwork/drivers/bridge/port_mapping_linux.go @@ -102,13 +102,13 @@ func (n *bridgeNetwork) mapPorts(ctx context.Context, pms *drvregistry.PortMappe return nil, err } - bindings, err := pm.MapPorts(ctx, reqs, n.firewallerNetwork) + bindings, err := pm.MapPorts(ctx, reqs) if err != nil { return nil, err } defer func() { if retErr != nil { - if err := pm.UnmapPorts(ctx, bindings, n.firewallerNetwork); err != nil { + if err := pm.UnmapPorts(ctx, bindings); err != nil { log.G(ctx).WithFields(log.Fields{ "bindings": bindings, "error": err, @@ -413,7 +413,7 @@ func (n *bridgeNetwork) unmapPBs(ctx context.Context, bindings []portmapperapi.P continue } - if err := pm.UnmapPorts(ctx, []portmapperapi.PortBinding{b}, n.firewallerNetwork); err != nil { + if err := pm.UnmapPorts(ctx, []portmapperapi.PortBinding{b}); err != nil { errs = append(errs, fmt.Errorf("unmapping port binding %s: %w", b.PortBinding, err)) } if b.StopProxy != nil { diff --git a/daemon/libnetwork/drivers/bridge/port_mapping_linux_test.go b/daemon/libnetwork/drivers/bridge/port_mapping_linux_test.go index 7286523ebf..fa7e77071d 100644 --- a/daemon/libnetwork/drivers/bridge/port_mapping_linux_test.go +++ b/daemon/libnetwork/drivers/bridge/port_mapping_linux_test.go @@ -990,7 +990,7 @@ type stubPortMapper struct { mapped []portmapperapi.PortBinding } -func (pm *stubPortMapper) MapPorts(_ context.Context, reqs []portmapperapi.PortBindingReq, _ portmapperapi.Firewaller) ([]portmapperapi.PortBinding, error) { +func (pm *stubPortMapper) MapPorts(_ context.Context, reqs []portmapperapi.PortBindingReq) ([]portmapperapi.PortBinding, error) { if len(reqs) == 0 { return []portmapperapi.PortBinding{}, nil } @@ -1002,7 +1002,7 @@ func (pm *stubPortMapper) MapPorts(_ context.Context, reqs []portmapperapi.PortB return pbs, nil } -func (pm *stubPortMapper) UnmapPorts(_ context.Context, reqs []portmapperapi.PortBinding, _ portmapperapi.Firewaller) error { +func (pm *stubPortMapper) UnmapPorts(_ context.Context, reqs []portmapperapi.PortBinding) error { for _, req := range reqs { // We're only checking for the PortBinding here, not any other // property of [portmapperapi.PortBinding]. diff --git a/daemon/libnetwork/drvregistry/portmappers_test.go b/daemon/libnetwork/drvregistry/portmappers_test.go index 30c3a8a183..ad05f952c7 100644 --- a/daemon/libnetwork/drvregistry/portmappers_test.go +++ b/daemon/libnetwork/drvregistry/portmappers_test.go @@ -10,11 +10,11 @@ import ( type fakePortMapper struct{} -func (f fakePortMapper) MapPorts(_ context.Context, _ []portmapperapi.PortBindingReq, _ portmapperapi.Firewaller) ([]portmapperapi.PortBinding, error) { +func (f fakePortMapper) MapPorts(_ context.Context, _ []portmapperapi.PortBindingReq) ([]portmapperapi.PortBinding, error) { return nil, nil } -func (f fakePortMapper) UnmapPorts(_ context.Context, _ []portmapperapi.PortBinding, _ portmapperapi.Firewaller) error { +func (f fakePortMapper) UnmapPorts(_ context.Context, _ []portmapperapi.PortBinding) error { return nil } diff --git a/daemon/libnetwork/portmapperapi/api.go b/daemon/libnetwork/portmapperapi/api.go index d161a068da..40531248b0 100644 --- a/daemon/libnetwork/portmapperapi/api.go +++ b/daemon/libnetwork/portmapperapi/api.go @@ -29,10 +29,10 @@ type PortMapper interface { // When an ephemeral port, or a single port from a range is requested // MapPorts should attempt a few times to find a free port available // across all IP addresses. - MapPorts(ctx context.Context, reqs []PortBindingReq, fwn Firewaller) ([]PortBinding, error) + MapPorts(ctx context.Context, reqs []PortBindingReq) ([]PortBinding, error) // UnmapPorts takes a list of port bindings to unmap. - UnmapPorts(ctx context.Context, pbs []PortBinding, fwn Firewaller) error + UnmapPorts(ctx context.Context, pbs []PortBinding) error } type PortBindingReq struct { diff --git a/daemon/libnetwork/portmapperapi/firewaller.go b/daemon/libnetwork/portmapperapi/firewaller.go deleted file mode 100644 index e09484147e..0000000000 --- a/daemon/libnetwork/portmapperapi/firewaller.go +++ /dev/null @@ -1,14 +0,0 @@ -package portmapperapi - -import ( - "context" - - "github.com/moby/moby/v2/daemon/libnetwork/types" -) - -type Firewaller interface { - // AddPorts adds the configuration needed for NATing ports. - AddPorts(ctx context.Context, pbs []types.PortBinding) error - // DelPorts deletes the configuration needed for NATing ports. - DelPorts(ctx context.Context, pbs []types.PortBinding) error -} diff --git a/daemon/libnetwork/portmappers/nat/mapper_linux.go b/daemon/libnetwork/portmappers/nat/mapper_linux.go index 002ba4c8ee..aa175e018c 100644 --- a/daemon/libnetwork/portmappers/nat/mapper_linux.go +++ b/daemon/libnetwork/portmappers/nat/mapper_linux.go @@ -47,7 +47,7 @@ func NewPortMapper(cfg Config) PortMapper { // MapPorts allocates and binds host ports for the given cfg. The caller is // responsible for ensuring that all entries in cfg have the same proto, // container port, and host port range (their host addresses must differ). -func (pm PortMapper) MapPorts(ctx context.Context, cfg []portmapperapi.PortBindingReq, fwn portmapperapi.Firewaller) (_ []portmapperapi.PortBinding, retErr error) { +func (pm PortMapper) MapPorts(ctx context.Context, cfg []portmapperapi.PortBindingReq) (_ []portmapperapi.PortBinding, retErr error) { if len(cfg) == 0 { return nil, nil } @@ -64,7 +64,7 @@ func (pm PortMapper) MapPorts(ctx context.Context, cfg []portmapperapi.PortBindi bindings := make([]portmapperapi.PortBinding, 0, len(cfg)) defer func() { if retErr != nil { - if err := pm.UnmapPorts(ctx, bindings, fwn); err != nil { + if err := pm.UnmapPorts(ctx, bindings); err != nil { log.G(ctx).WithFields(log.Fields{ "pbs": bindings, "error": err, @@ -107,7 +107,7 @@ func (pm PortMapper) MapPorts(ctx context.Context, cfg []portmapperapi.PortBindi return bindings, nil } -func (pm PortMapper) UnmapPorts(ctx context.Context, pbs []portmapperapi.PortBinding, fwn portmapperapi.Firewaller) error { +func (pm PortMapper) UnmapPorts(ctx context.Context, pbs []portmapperapi.PortBinding) error { var errs []error for _, pb := range pbs { if pb.BoundSocket != nil { diff --git a/daemon/libnetwork/portmappers/nat/mapper_linux_test.go b/daemon/libnetwork/portmappers/nat/mapper_linux_test.go index 2edf6c0489..e5b1882841 100644 --- a/daemon/libnetwork/portmappers/nat/mapper_linux_test.go +++ b/daemon/libnetwork/portmappers/nat/mapper_linux_test.go @@ -30,7 +30,7 @@ func TestBindHostPortsError(t *testing.T) { }, } pm := &PortMapper{} - pbs, err := pm.MapPorts(context.Background(), cfg, nil) + pbs, err := pm.MapPorts(context.Background(), cfg) assert.Check(t, is.Error(err, "port binding mismatch 80/tcp:8080-8080, 80/tcp:8080-8081")) assert.Check(t, is.Nil(pbs)) } diff --git a/daemon/libnetwork/portmappers/routed/mapper_linux.go b/daemon/libnetwork/portmappers/routed/mapper_linux.go index d27f51b1b2..5c4ad5d1d5 100644 --- a/daemon/libnetwork/portmappers/routed/mapper_linux.go +++ b/daemon/libnetwork/portmappers/routed/mapper_linux.go @@ -22,7 +22,7 @@ func NewPortMapper() PortMapper { // MapPorts returns a PortBinding for every PortBindingReq received, with Forwarding enabled for each. If a HostPort is // specified, it's logged and ignored. -func (pm PortMapper) MapPorts(ctx context.Context, reqs []portmapperapi.PortBindingReq, fwn portmapperapi.Firewaller) ([]portmapperapi.PortBinding, error) { +func (pm PortMapper) MapPorts(ctx context.Context, reqs []portmapperapi.PortBindingReq) ([]portmapperapi.PortBinding, error) { if len(reqs) == 0 { return nil, nil } @@ -45,6 +45,6 @@ func (pm PortMapper) MapPorts(ctx context.Context, reqs []portmapperapi.PortBind return res, nil } -func (pm PortMapper) UnmapPorts(_ context.Context, _ []portmapperapi.PortBinding, _ portmapperapi.Firewaller) error { +func (pm PortMapper) UnmapPorts(_ context.Context, _ []portmapperapi.PortBinding) error { return nil }