From 9d9b05446c2f5e2f9d7f1f2e0cc9bfb82ae243c2 Mon Sep 17 00:00:00 2001 From: Albin Kerouanton Date: Fri, 8 Aug 2025 17:36:14 +0200 Subject: [PATCH] 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).