From 4a2bd1085ec965cefca965a2ecf396b4813f6b0c Mon Sep 17 00:00:00 2001 From: Rob Murray Date: Sun, 11 Aug 2024 15:46:59 +0000 Subject: [PATCH 01/10] Move default bridge test into linux-only file Becuase I'm about to add tests that use netlink, and the netlink package breaks compilation under Windows. Signed-off-by: Rob Murray --- integration/daemon/daemon_linux_test.go | 36 +++++++++++++++++++++++++ integration/daemon/daemon_test.go | 29 -------------------- 2 files changed, 36 insertions(+), 29 deletions(-) create mode 100644 integration/daemon/daemon_linux_test.go diff --git a/integration/daemon/daemon_linux_test.go b/integration/daemon/daemon_linux_test.go new file mode 100644 index 0000000000..591cba531d --- /dev/null +++ b/integration/daemon/daemon_linux_test.go @@ -0,0 +1,36 @@ +package daemon // import "github.com/docker/docker/integration/daemon" + +import ( + "testing" + + "github.com/docker/docker/testutil" + "github.com/docker/docker/testutil/daemon" + "gotest.tools/v3/icmd" +) + +func TestDaemonDefaultBridgeWithFixedCidrButNoBip(t *testing.T) { + ctx := testutil.StartSpan(baseContext, t) + + bridgeName := "ext-bridge1" + d := daemon.New(t, daemon.WithEnvVars("DOCKER_TEST_CREATE_DEFAULT_BRIDGE="+bridgeName)) + defer func() { + d.Stop(t) + d.Cleanup(t) + }() + + defer func() { + // No need to clean up when running this test in rootless mode, as the + // interface is deleted when the daemon is stopped and the netns + // reclaimed by the kernel. + if !testEnv.IsRootless() { + deleteInterface(t, bridgeName) + } + }() + d.StartWithBusybox(ctx, t, "--bridge", bridgeName, "--fixed-cidr", "192.168.130.0/24") +} + +func deleteInterface(t *testing.T, ifName string) { + icmd.RunCommand("ip", "link", "delete", ifName).Assert(t, icmd.Success) + icmd.RunCommand("iptables", "-t", "nat", "--flush").Assert(t, icmd.Success) + icmd.RunCommand("iptables", "--flush").Assert(t, icmd.Success) +} diff --git a/integration/daemon/daemon_test.go b/integration/daemon/daemon_test.go index 608f8176ad..8f15776fa4 100644 --- a/integration/daemon/daemon_test.go +++ b/integration/daemon/daemon_test.go @@ -699,32 +699,3 @@ func testLiveRestoreUserChainsSetup(t *testing.T) { assert.Check(t, is.Equal(strings.TrimSpace(result.Stdout()), "-A FORWARD -j DOCKER-USER"), "the jump to DOCKER-USER should be the first rule in the FORWARD chain") }) } - -func TestDaemonDefaultBridgeWithFixedCidrButNoBip(t *testing.T) { - skip.If(t, runtime.GOOS == "windows") - - ctx := testutil.StartSpan(baseContext, t) - - bridgeName := "ext-bridge1" - d := daemon.New(t, daemon.WithEnvVars("DOCKER_TEST_CREATE_DEFAULT_BRIDGE="+bridgeName)) - defer func() { - d.Stop(t) - d.Cleanup(t) - }() - - defer func() { - // No need to clean up when running this test in rootless mode, as the - // interface is deleted when the daemon is stopped and the netns - // reclaimed by the kernel. - if !testEnv.IsRootless() { - deleteInterface(t, bridgeName) - } - }() - d.StartWithBusybox(ctx, t, "--bridge", bridgeName, "--fixed-cidr", "192.168.130.0/24") -} - -func deleteInterface(t *testing.T, ifName string) { - icmd.RunCommand("ip", "link", "delete", ifName).Assert(t, icmd.Success) - icmd.RunCommand("iptables", "-t", "nat", "--flush").Assert(t, icmd.Success) - icmd.RunCommand("iptables", "--flush").Assert(t, icmd.Success) -} From facb2323a014e30891fe554ab033452c69a6b86b Mon Sep 17 00:00:00 2001 From: Rob Murray Date: Sun, 11 Aug 2024 16:53:39 +0100 Subject: [PATCH 02/10] Add tests for IPAM Config of default bridge Signed-off-by: Rob Murray --- daemon/daemon_unix.go | 30 ++- integration/daemon/daemon_linux_test.go | 290 ++++++++++++++++++++++++ 2 files changed, 314 insertions(+), 6 deletions(-) diff --git a/daemon/daemon_unix.go b/daemon/daemon_unix.go index 31a1d3f47d..011739f6d9 100644 --- a/daemon/daemon_unix.go +++ b/daemon/daemon_unix.go @@ -927,10 +927,7 @@ func driverOptions(config *config.Config) nwconfig.Option { } func initBridgeDriver(controller *libnetwork.Controller, cfg config.BridgeConfig) error { - bridgeName := bridge.DefaultBridgeName - if cfg.Iface != "" { - bridgeName = cfg.Iface - } + bridgeName, userManagedBridge := getDefaultBridgeName(cfg) netOption := map[string]string{ bridge.BridgeName: bridgeName, bridge.DefaultBridge: strconv.FormatBool(true), @@ -938,7 +935,6 @@ func initBridgeDriver(controller *libnetwork.Controller, cfg config.BridgeConfig bridge.EnableIPMasquerade: strconv.FormatBool(cfg.EnableIPMasq), bridge.EnableICC: strconv.FormatBool(cfg.InterContainerCommunication), } - // --ip processing if cfg.DefaultIP != nil { netOption[bridge.DefaultBindingIP] = cfg.DefaultIP.String() @@ -986,7 +982,7 @@ func initBridgeDriver(controller *libnetwork.Controller, cfg config.BridgeConfig } ipamV4Conf.PreferredPool = ipNet.String() ipamV4Conf.Gateway = ip.String() - } else if bridgeName == bridge.DefaultBridgeName && ipamV4Conf.PreferredPool != "" { + } else if !userManagedBridge && ipamV4Conf.PreferredPool != "" { log.G(context.TODO()).Infof("Default bridge (%s) is assigned with an IP address %s. Daemon option --bip can be used to set a preferred IP address", bridgeName, ipamV4Conf.PreferredPool) } @@ -1069,6 +1065,28 @@ func initBridgeDriver(controller *libnetwork.Controller, cfg config.BridgeConfig return nil } +func getDefaultBridgeName(cfg config.BridgeConfig) (bridgeName string, userManagedBridge bool) { + // cfg.Iface is --bridge, the option to supply a user-managed bridge. + if cfg.Iface != "" { + // The default network will use a user-managed bridge, the daemon will not + // create it, and it is not possible to supply an address using --bip. + return cfg.Iface, true + } + // Without a --bridge, the bridge is "docker0", created and managed by the + // daemon. A --bip (cidr) can be supplied to define the bridge's IP address + // and the network's subnet. + // + // Env var DOCKER_TEST_CREATE_DEFAULT_BRIDGE env var modifies the default + // bridge name. Unlike '--bridge', the bridge does not need to be created + // outside the daemon, and it's still possible to use '--bip'. It is + // intended only for use in moby tests; it may be removed, its behaviour + // may be modified, and it may not do what you want anyway! + if bn := os.Getenv("DOCKER_TEST_CREATE_DEFAULT_BRIDGE"); bn != "" { + return bn, false + } + return bridge.DefaultBridgeName, false +} + // Remove default bridge interface if present (--bridge=none use case) func removeDefaultBridgeInterface() { if lnk, err := nlwrap.LinkByName(bridge.DefaultBridgeName); err == nil { diff --git a/integration/daemon/daemon_linux_test.go b/integration/daemon/daemon_linux_test.go index 591cba531d..6406cfc1d9 100644 --- a/integration/daemon/daemon_linux_test.go +++ b/integration/daemon/daemon_linux_test.go @@ -1,11 +1,18 @@ package daemon // import "github.com/docker/docker/integration/daemon" import ( + "context" + "net" "testing" + "github.com/docker/docker/api/types/network" "github.com/docker/docker/testutil" "github.com/docker/docker/testutil/daemon" + "github.com/vishvananda/netlink" + "gotest.tools/v3/assert" + is "gotest.tools/v3/assert/cmp" "gotest.tools/v3/icmd" + "gotest.tools/v3/skip" ) func TestDaemonDefaultBridgeWithFixedCidrButNoBip(t *testing.T) { @@ -29,6 +36,289 @@ func TestDaemonDefaultBridgeWithFixedCidrButNoBip(t *testing.T) { d.StartWithBusybox(ctx, t, "--bridge", bridgeName, "--fixed-cidr", "192.168.130.0/24") } +// Test fixed-cidr and bip options, with various addresses on the bridge +// before the daemon starts. +func TestDaemonDefaultBridgeIPAM_Docker0(t *testing.T) { + skip.If(t, testEnv.IsRootless, "can't create test bridge in rootless namespace") + ctx := testutil.StartSpan(baseContext, t) + + testcases := []defaultBridgeIPAMTestCase{ + { + name: "fixed-cidr only", + daemonArgs: []string{"--fixed-cidr", "192.168.176.0/24"}, + expIPAMConfig: []network.IPAMConfig{ + { + Subnet: "192.168.176.0/24", + IPRange: "192.168.176.0/24", + }, + }, + }, + { + name: "bip only", + daemonArgs: []string{"--bip", "192.168.176.88/24"}, + expIPAMConfig: []network.IPAMConfig{ + { + Subnet: "192.168.176.0/24", + Gateway: "192.168.176.88", + }, + }, + }, + { + name: "existing bridge address only", + initialBridgeAddrs: []string{"192.168.176.88/24"}, + expIPAMConfig: []network.IPAMConfig{ + { + Subnet: "192.168.176.0/24", + Gateway: "192.168.176.88", + }, + }, + }, + { + name: "fixed-cidr within old bridge subnet", + initialBridgeAddrs: []string{"192.168.176.88/20"}, + daemonArgs: []string{"--fixed-cidr", "192.168.176.0/24"}, + // There's no --bip to dictate the subnet, so it's derived from an + // existing bridge address. If fixed-cidr's subnet is made smaller + // following a daemon restart, a user might reasonably expect the + // default bridge network's subnet to shrink to match. However, + // that has not been the behaviour - instead, only the allocatable + // range is reduced (as would happen with a user-managed bridge). + // In this case, if the user wants a smaller subnet, their options + // are to delete docker0, or supply a --bip. A change in this subtle + // behaviour might be best. But, it's probably not causing problems, + // and it'd be a breaking change for anyone relying on the existing + // behaviour. + expIPAMConfig: []network.IPAMConfig{ + { + Subnet: "192.168.176.0/20", + IPRange: "192.168.176.0/24", + Gateway: "192.168.176.88", + }, + }, + }, + { + name: "fixed-cidr within old bridge subnet with new bip", + initialBridgeAddrs: []string{"192.168.176.88/20"}, + daemonArgs: []string{"--fixed-cidr", "192.168.176.0/24", "--bip", "192.168.176.99/24"}, + expIPAMConfig: []network.IPAMConfig{ + { + Subnet: "192.168.176.0/24", + IPRange: "192.168.176.0/24", + Gateway: "192.168.176.99", + }, + }, + }, + { + name: "old bridge subnet within fixed-cidr", + initialBridgeAddrs: []string{"192.168.176.88/24"}, + daemonArgs: []string{"--fixed-cidr", "192.168.176.0/20"}, + expIPAMConfig: []network.IPAMConfig{ + { + // FIXME(robmry) - subnet didn't change, allocatable range + // is bigger than the subnet. + Subnet: "192.168.176.0/24", + IPRange: "192.168.176.0/20", + Gateway: "192.168.176.88", + }, + }, + }, + { + name: "old bridge subnet outside fixed-cidr", + initialBridgeAddrs: []string{"192.168.176.88/24"}, + daemonArgs: []string{"--fixed-cidr", "192.168.177.0/24"}, + expIPAMConfig: []network.IPAMConfig{ + { + // FIXME(robmry) - subnet and bridge address haven't changed, + // and the allocatable range is outside the subnet. + Subnet: "192.168.176.0/24", + IPRange: "192.168.177.0/24", + Gateway: "192.168.176.88", + }, + }, + }, + { + name: "old bridge subnet outside fixed-cidr with bip", + initialBridgeAddrs: []string{"192.168.176.88/24"}, + daemonArgs: []string{"--fixed-cidr", "192.168.177.0/24", "--bip", "192.168.177.99/24"}, + expIPAMConfig: []network.IPAMConfig{ + { + Subnet: "192.168.177.0/24", + IPRange: "192.168.177.0/24", + Gateway: "192.168.177.99", + }, + }, + }, + } + for _, tc := range testcases { + testDefaultBridgeIPAM(ctx, t, tc) + } +} + +// Like TestDaemonUserDefaultBridgeIPAMDocker0, but with a user-defined/supplied +// bridge, instead of docker0. +func TestDaemonDefaultBridgeIPAM_UserBr(t *testing.T) { + skip.If(t, testEnv.IsRootless, "can't create test bridge in rootless namespace") + ctx := testutil.StartSpan(baseContext, t) + + testcases := []defaultBridgeIPAMTestCase{ + { + name: "bridge only", + initialBridgeAddrs: []string{"192.168.176.88/20"}, + expIPAMConfig: []network.IPAMConfig{ + { + Subnet: "192.168.176.0/20", + Gateway: "192.168.176.88", + }, + }, + }, + { + name: "fixed-cidr only", + daemonArgs: []string{"--fixed-cidr", "192.168.176.0/24"}, + expIPAMConfig: []network.IPAMConfig{ + { + Subnet: "192.168.176.0/24", + IPRange: "192.168.176.0/24", + }, + }, + }, + { + name: "fcidr in bridge subnet and bridge ip in fcidr", + initialBridgeAddrs: []string{"192.168.160.88/20", "192.168.176.88/20", "192.168.192.88/20"}, + daemonArgs: []string{"--fixed-cidr", "192.168.176.0/24"}, + // Selected bip should be the one within fixed-cidr + expIPAMConfig: []network.IPAMConfig{ + { + Subnet: "192.168.176.0/20", + IPRange: "192.168.176.0/24", + Gateway: "192.168.176.88", + }, + }, + }, + { + name: "fcidr in bridge subnet and bridge ip not in fcidr", + initialBridgeAddrs: []string{"192.168.160.88/20", "192.168.176.88/20", "192.168.192.88/20"}, + daemonArgs: []string{"--fixed-cidr", "192.168.177.0/24"}, + expIPAMConfig: []network.IPAMConfig{ + { + // FIXME(robmry) - selected subnet should be the one that encompasses + // fixed-cidr, allocatable range is outside the subnet. + Subnet: "192.168.160.0/20", + IPRange: "192.168.177.0/24", + Gateway: "192.168.160.88", + }, + }, + }, + { + name: "fixed-cidr bigger than bridge subnet", + initialBridgeAddrs: []string{"192.168.176.88/24"}, + daemonArgs: []string{"--fixed-cidr", "192.168.176.0/20"}, + expIPAMConfig: []network.IPAMConfig{ + { + Subnet: "192.168.176.0/24", + // FIXME(robmry) - allocatable range is bigger than the subnet. + IPRange: "192.168.176.0/20", + Gateway: "192.168.176.88", + }, + }, + }, + { + name: "no bridge ip within fixed-cidr", + initialBridgeAddrs: []string{"192.168.160.88/20", "192.168.192.88/20"}, + daemonArgs: []string{"--fixed-cidr", "192.168.176.0/24"}, + // Selected bip should be the one within fixed-cidr + expIPAMConfig: []network.IPAMConfig{ + { + // FIXME(robmry) - allocatable range outside subnet. + Subnet: "192.168.160.0/20", + IPRange: "192.168.176.0/24", + Gateway: "192.168.160.88", + }, + }, + }, + { + name: "fixed-cidr contains bridge subnet", + initialBridgeAddrs: []string{"192.168.177.1/24"}, + daemonArgs: []string{"--fixed-cidr", "192.168.176.0/20"}, + expIPAMConfig: []network.IPAMConfig{ + { + Subnet: "192.168.177.0/24", + // FIXME(robmry) - allocatable range is bigger than subnet + IPRange: "192.168.176.0/20", + Gateway: "192.168.177.1", + }, + }, + }, + } + for _, tc := range testcases { + tc.userDefinedBridge = true + testDefaultBridgeIPAM(ctx, t, tc) + } +} + +type defaultBridgeIPAMTestCase struct { + name string + userDefinedBridge bool + initialBridgeAddrs []string + daemonArgs []string + expIPAMConfig []network.IPAMConfig +} + +func testDefaultBridgeIPAM(ctx context.Context, t *testing.T, tc defaultBridgeIPAMTestCase) { + t.Run(tc.name, func(t *testing.T) { + ctx := testutil.StartSpan(ctx, t) + const bridgeName = "br-dbi" + + createBridge(t, bridgeName, tc.initialBridgeAddrs) + defer deleteInterface(t, bridgeName) + + var dOpts []daemon.Option + dArgs := tc.daemonArgs + if tc.userDefinedBridge { + // If a bridge is supplied by the user, the daemon should use its addresses + // to infer --bip (which cannot be specified). + dArgs = append(dArgs, []string{"--bridge", bridgeName}...) + } else { + // The bridge is created and managed by docker, it's always called "docker0", + // unless this test-only env var is set - to avoid conflict with the docker0 + // belonging to the daemon started in CI runs. + dOpts = append(dOpts, daemon.WithEnvVars("DOCKER_TEST_CREATE_DEFAULT_BRIDGE="+bridgeName)) + } + + d := daemon.New(t, dOpts...) + defer func() { + d.Stop(t) + d.Cleanup(t) + }() + d.StartWithBusybox(ctx, t, dArgs...) + c := d.NewClientT(t) + defer c.Close() + + insp, err := c.NetworkInspect(ctx, network.NetworkBridge, network.InspectOptions{}) + assert.NilError(t, err) + assert.Check(t, is.DeepEqual(insp.IPAM.Config, tc.expIPAMConfig)) + }) +} + +func createBridge(t *testing.T, ifName string, addrs []string) { + t.Helper() + + link := &netlink.Bridge{ + LinkAttrs: netlink.LinkAttrs{ + Name: ifName, + }, + } + + err := netlink.LinkAdd(link) + assert.NilError(t, err) + for _, addr := range addrs { + ip, ipNet, err := net.ParseCIDR(addr) + assert.NilError(t, err) + ipNet.IP = ip + err = netlink.AddrAdd(link, &netlink.Addr{IPNet: ipNet}) + assert.NilError(t, err) + } +} + func deleteInterface(t *testing.T, ifName string) { icmd.RunCommand("ip", "link", "delete", ifName).Assert(t, icmd.Success) icmd.RunCommand("iptables", "-t", "nat", "--flush").Assert(t, icmd.Success) From 63b558af77f8a9d65f1d3e5d3a0f257f858a4b12 Mon Sep 17 00:00:00 2001 From: Rob Murray Date: Sat, 10 Aug 2024 12:40:24 +0100 Subject: [PATCH 03/10] Don't clear top bits of IP then check if it's global unicast The intention may have been to only accept a globally routable address as the gateway address on a user-supplied default bridge. But, the test didn't do anything, so it's been allowing LL subnets. It's too late to change that now so, remove the broken check. Signed-off-by: Rob Murray --- daemon/daemon_unix.go | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/daemon/daemon_unix.go b/daemon/daemon_unix.go index 011739f6d9..2fe35c5ef5 100644 --- a/daemon/daemon_unix.go +++ b/daemon/daemon_unix.go @@ -969,10 +969,7 @@ func initBridgeDriver(controller *libnetwork.Controller, cfg config.BridgeConfig } ipamV4Conf.PreferredPool = lntypes.GetIPNetCanonical(nw).String() - hip, _ := lntypes.GetHostPartIP(nw.IP, nw.Mask) - if hip.IsGlobalUnicast() { - ipamV4Conf.Gateway = nw.IP.String() - } + ipamV4Conf.Gateway = nw.IP.String() } if cfg.IP != "" { From 90baa2bc36141e99d0652ee59a4bd8fd52b147f2 Mon Sep 17 00:00:00 2001 From: Rob Murray Date: Sat, 10 Aug 2024 14:45:55 +0100 Subject: [PATCH 04/10] Fix selection of subnet from user-managed default bridge When a user-managed bridge is used instead of docker0 (--bridge), with a fixed-cidr - the bridge should have an IP address/subnet that encompasses fixed-cidr ... the bridge address's subnet then defines the network's subnet, and fixed-cidr defines the allocatable range within that. But, selection of the correct subnet/address from the bridge depended on the address being within fixed-cidr (within the allocatable range). This change removes that assumption. So, a bridge address with a subnet that includes fixed-cidr is selected. Signed-off-by: Rob Murray --- daemon/daemon_unix.go | 16 ++++++++++++---- integration/daemon/daemon_linux_test.go | 7 +++---- 2 files changed, 15 insertions(+), 8 deletions(-) diff --git a/daemon/daemon_unix.go b/daemon/daemon_unix.go index 2fe35c5ef5..426672a191 100644 --- a/daemon/daemon_unix.go +++ b/daemon/daemon_unix.go @@ -953,18 +953,26 @@ func initBridgeDriver(controller *libnetwork.Controller, cfg config.BridgeConfig } if len(nwList) > 0 { + // Pick any address from the bridge as a starting point. nw := nwList[0] if len(nwList) > 1 && cfg.FixedCIDR != "" { - _, fCIDR, err := net.ParseCIDR(cfg.FixedCIDR) + fCidrIP, fCidrNet, err := net.ParseCIDR(cfg.FixedCIDR) if err != nil { - return errors.Wrap(err, "parse CIDR failed") + return errors.Wrap(err, "parse fixed-cidr failed") } - // Iterate through in case there are multiple addresses for the bridge + // If there's an address with a subnet that contains fixed-cidr, use it. for _, entry := range nwList { - if fCIDR.Contains(entry.IP) { + if entry.Contains(fCidrIP) { nw = entry break } + // For backwards compatibility - prefer the first bridge address within + // fixed-cidr. If fixed-cidr has a bigger subnet than nw.IP, this doesn't really + // make sense - the allocatable range (fixed-cidr) will be bigger than the subnet + // (entry.IPNet). + if fCidrNet.Contains(entry.IP) && !fCidrNet.Contains(nw.IP) { + nw = entry + } } } diff --git a/integration/daemon/daemon_linux_test.go b/integration/daemon/daemon_linux_test.go index 6406cfc1d9..9c874561cc 100644 --- a/integration/daemon/daemon_linux_test.go +++ b/integration/daemon/daemon_linux_test.go @@ -198,13 +198,12 @@ func TestDaemonDefaultBridgeIPAM_UserBr(t *testing.T) { name: "fcidr in bridge subnet and bridge ip not in fcidr", initialBridgeAddrs: []string{"192.168.160.88/20", "192.168.176.88/20", "192.168.192.88/20"}, daemonArgs: []string{"--fixed-cidr", "192.168.177.0/24"}, + // Selected bridge subnet should be the one that encompasses fixed-cidr. expIPAMConfig: []network.IPAMConfig{ { - // FIXME(robmry) - selected subnet should be the one that encompasses - // fixed-cidr, allocatable range is outside the subnet. - Subnet: "192.168.160.0/20", + Subnet: "192.168.176.0/20", IPRange: "192.168.177.0/24", - Gateway: "192.168.160.88", + Gateway: "192.168.176.88", }, }, }, From 237654a5536080c288b03782f2e8b58afbbc459a Mon Sep 17 00:00:00 2001 From: Rob Murray Date: Sat, 10 Aug 2024 15:41:54 +0100 Subject: [PATCH 05/10] Refactor IPAM config for default bridge Factor out selection of addresses from an existing bridge from the code that uses the selected address to set up IPAM for the default bridge. Signed-off-by: Rob Murray --- daemon/daemon_unix.go | 101 ++++++++++++++++++++++++------------------ 1 file changed, 57 insertions(+), 44 deletions(-) diff --git a/daemon/daemon_unix.go b/daemon/daemon_unix.go index 426672a191..db8002024c 100644 --- a/daemon/daemon_unix.go +++ b/daemon/daemon_unix.go @@ -940,66 +940,42 @@ func initBridgeDriver(controller *libnetwork.Controller, cfg config.BridgeConfig netOption[bridge.DefaultBindingIP] = cfg.DefaultIP.String() } - ipamV4Conf := &libnetwork.IpamConf{AuxAddresses: make(map[string]string)} - - // By default, libnetwork will request an arbitrary available address - // pool for the network from the configured IPAM allocator. - // Configure it to use the IPv4 network ranges of the existing bridge - // interface if one exists with IPv4 addresses assigned to it. - nwList, nw6List, err := ifaceAddrs(bridgeName) if err != nil { return errors.Wrap(err, "list bridge addresses failed") } - if len(nwList) > 0 { - // Pick any address from the bridge as a starting point. - nw := nwList[0] - if len(nwList) > 1 && cfg.FixedCIDR != "" { - fCidrIP, fCidrNet, err := net.ParseCIDR(cfg.FixedCIDR) - if err != nil { - return errors.Wrap(err, "parse fixed-cidr failed") - } - // If there's an address with a subnet that contains fixed-cidr, use it. - for _, entry := range nwList { - if entry.Contains(fCidrIP) { - nw = entry - break - } - // For backwards compatibility - prefer the first bridge address within - // fixed-cidr. If fixed-cidr has a bigger subnet than nw.IP, this doesn't really - // make sense - the allocatable range (fixed-cidr) will be bigger than the subnet - // (entry.IPNet). - if fCidrNet.Contains(entry.IP) && !fCidrNet.Contains(nw.IP) { - nw = entry - } - } - } + var ( + fCidrIP, bIP net.IP + fCidrIPNet, bIPNet *net.IPNet + ) - ipamV4Conf.PreferredPool = lntypes.GetIPNetCanonical(nw).String() - ipamV4Conf.Gateway = nw.IP.String() + if cfg.FixedCIDR != "" { + if fCidrIP, fCidrIPNet, err = net.ParseCIDR(cfg.FixedCIDR); err != nil { + return errors.Wrap(err, "parse fixed-cidr failed") + } } if cfg.IP != "" { - ip, ipNet, err := net.ParseCIDR(cfg.IP) - if err != nil { + if bIP, bIPNet, err = net.ParseCIDR(cfg.IP); err != nil { return err } - ipamV4Conf.PreferredPool = ipNet.String() - ipamV4Conf.Gateway = ip.String() + } else { + bIP, bIPNet = selectBIP(nwList, fCidrIP, fCidrIPNet) + } + + ipamV4Conf := &libnetwork.IpamConf{AuxAddresses: make(map[string]string)} + if bIP != nil { + ipamV4Conf.PreferredPool = bIPNet.String() + ipamV4Conf.Gateway = bIP.String() } else if !userManagedBridge && ipamV4Conf.PreferredPool != "" { log.G(context.TODO()).Infof("Default bridge (%s) is assigned with an IP address %s. Daemon option --bip can be used to set a preferred IP address", bridgeName, ipamV4Conf.PreferredPool) } - if cfg.FixedCIDR != "" { - _, fCIDR, err := net.ParseCIDR(cfg.FixedCIDR) - if err != nil { - return err - } - - ipamV4Conf.SubPool = fCIDR.String() + if fCidrIP != nil { + ipamV4Conf.SubPool = fCidrIPNet.String() if ipamV4Conf.PreferredPool == "" { - ipamV4Conf.PreferredPool = fCIDR.String() + ipamV4Conf.PreferredPool = fCidrIPNet.String() } } @@ -1092,6 +1068,43 @@ func getDefaultBridgeName(cfg config.BridgeConfig) (bridgeName string, userManag return bridge.DefaultBridgeName, false } +// selectBIP searches bridgeNws for: +// - An address that encompasses fCidrNet if there is one. +// - Else, an address that is within fCidrNet if there is one. +// - Else, any address, if there is one. +// If an address is found, it's returned as bIP, with its subnet in canonical +// form in bIPNet. +func selectBIP( + bridgeNws []*net.IPNet, + fCidrIP net.IP, + fCidrNet *net.IPNet, +) (bIP net.IP, bIPNet *net.IPNet) { + if len(bridgeNws) > 0 { + // Pick any address from the bridge as a starting point. + nw := bridgeNws[0] + if len(bridgeNws) > 1 && fCidrNet != nil { + // If there's an address with a subnet that contains fixed-cidr, use it. + for _, entry := range bridgeNws { + if entry.Contains(fCidrIP) { + nw = entry + break + } + // For backwards compatibility - prefer the first bridge address within + // fixed-cidr. If fixed-cidr has a bigger subnet than nw.IP, this doesn't really + // make sense - the allocatable range (fixed-cidr) will be bigger than the subnet + // (entry.IPNet). + if fCidrNet.Contains(entry.IP) && !fCidrNet.Contains(nw.IP) { + nw = entry + } + } + } + + bIP = nw.IP + bIPNet = lntypes.GetIPNetCanonical(nw) + } + return bIP, bIPNet +} + // Remove default bridge interface if present (--bridge=none use case) func removeDefaultBridgeInterface() { if lnk, err := nlwrap.LinkByName(bridge.DefaultBridgeName); err == nil { From 85159ce09f70881f95aab8611b1ab45d1cafeca4 Mon Sep 17 00:00:00 2001 From: Rob Murray Date: Sat, 10 Aug 2024 16:09:16 +0100 Subject: [PATCH 06/10] Allow non-overlapping change of fixed-cidr When a docker-managed default bridge (docker0) already has an address, and the fixed-cidr subnet fits within the subnet defined by that address, the existing address should be used as the gateway and to define the subnet. But, when fixed-cidr is changed, no --bip is supplied, and no existing bridge network includes fixed-cidr ... the existing bridge address needs to be updated. Signed-off-by: Rob Murray --- daemon/daemon_unix.go | 7 +++++++ integration/daemon/daemon_linux_test.go | 14 ++++++++++---- 2 files changed, 17 insertions(+), 4 deletions(-) diff --git a/daemon/daemon_unix.go b/daemon/daemon_unix.go index db8002024c..7fbdeb5e67 100644 --- a/daemon/daemon_unix.go +++ b/daemon/daemon_unix.go @@ -962,6 +962,13 @@ func initBridgeDriver(controller *libnetwork.Controller, cfg config.BridgeConfig } } else { bIP, bIPNet = selectBIP(nwList, fCidrIP, fCidrIPNet) + // If the bridge is docker-managed (docker0) and fixed-cidr is not + // inside a subnet belonging to any existing bridge IP, ignore the + // bridge IP. + if !userManagedBridge && fCidrIP != nil && bIPNet != nil && !bIPNet.Contains(fCidrIP) { + bIP = nil + bIPNet = nil + } } ipamV4Conf := &libnetwork.IpamConf{AuxAddresses: make(map[string]string)} diff --git a/integration/daemon/daemon_linux_test.go b/integration/daemon/daemon_linux_test.go index 9c874561cc..efefce9378 100644 --- a/integration/daemon/daemon_linux_test.go +++ b/integration/daemon/daemon_linux_test.go @@ -126,13 +126,19 @@ func TestDaemonDefaultBridgeIPAM_Docker0(t *testing.T) { name: "old bridge subnet outside fixed-cidr", initialBridgeAddrs: []string{"192.168.176.88/24"}, daemonArgs: []string{"--fixed-cidr", "192.168.177.0/24"}, + // The bridge's address/subnet should be ignored, this is a change + // of fixed-cidr. expIPAMConfig: []network.IPAMConfig{ { - // FIXME(robmry) - subnet and bridge address haven't changed, - // and the allocatable range is outside the subnet. - Subnet: "192.168.176.0/24", + Subnet: "192.168.177.0/24", IPRange: "192.168.177.0/24", - Gateway: "192.168.176.88", + // No Gateway is configured, because the address could not be learnt from the + // bridge. An address will have been allocated but, because there's config (the + // fixed-cidr), inspect shows just the config. (Surprisingly, when there's no + // config at all, the inspect output still says its showing config but actually + // shows the running state.) When the daemon is restarted, after a gateway + // address has been assigned to the bridge, that address will become config - so + // a Gateway address will show up in the inspect output. }, }, }, From 311a8bc89980a55a32be249d0c13927a370c667d Mon Sep 17 00:00:00 2001 From: Rob Murray Date: Sat, 10 Aug 2024 16:46:39 +0100 Subject: [PATCH 07/10] Allow increase of fixed-cidr subnet size For a docker-managed default bridge (docker0), when no --bip is supplied, the gateway address and subnet size can be inferred from existing bridge addresses. But, if fixed-cidr's subnet size is increased so that it's biggger than the subnet of the bridge's existing address - the bridge's subnet needs to be incresed to match. (fixed-cidr determines the range of addresses that can be automatically allocated, and these should not fall outside the default bridge's subnet.) Signed-off-by: Rob Murray --- daemon/daemon_unix.go | 25 +++++++++++++++++++------ integration/daemon/daemon_linux_test.go | 4 +--- 2 files changed, 20 insertions(+), 9 deletions(-) diff --git a/daemon/daemon_unix.go b/daemon/daemon_unix.go index 7fbdeb5e67..46b4bb2309 100644 --- a/daemon/daemon_unix.go +++ b/daemon/daemon_unix.go @@ -962,12 +962,25 @@ func initBridgeDriver(controller *libnetwork.Controller, cfg config.BridgeConfig } } else { bIP, bIPNet = selectBIP(nwList, fCidrIP, fCidrIPNet) - // If the bridge is docker-managed (docker0) and fixed-cidr is not - // inside a subnet belonging to any existing bridge IP, ignore the - // bridge IP. - if !userManagedBridge && fCidrIP != nil && bIPNet != nil && !bIPNet.Contains(fCidrIP) { - bIP = nil - bIPNet = nil + if !userManagedBridge && fCidrIP != nil && bIPNet != nil { + if !bIPNet.Contains(fCidrIP) { + // The bridge is docker-managed (docker0) and fixed-cidr is not + // inside a subnet belonging to any existing bridge IP. (fixed-cidr + // has changed.) So, ignore the existing bridge IP. + bIP = nil + bIPNet = nil + } else { + fCidrOnes, _ := fCidrIPNet.Mask.Size() + bIPOnes, _ := bIPNet.Mask.Size() + if fCidrOnes < bIPOnes { + // The bridge is docker-managed (docker0) and fixed-cidr (the + // allocatable address range) is bigger than the subnet implied + // by the bridge's current address. (fixed-cidr has changed.) + // The bridge's address is ok, but its subnet needs to be updated. + bIPNet.IP = bIPNet.IP.Mask(fCidrIPNet.Mask) + bIPNet.Mask = fCidrIPNet.Mask + } + } } } diff --git a/integration/daemon/daemon_linux_test.go b/integration/daemon/daemon_linux_test.go index efefce9378..d0d898f919 100644 --- a/integration/daemon/daemon_linux_test.go +++ b/integration/daemon/daemon_linux_test.go @@ -114,9 +114,7 @@ func TestDaemonDefaultBridgeIPAM_Docker0(t *testing.T) { daemonArgs: []string{"--fixed-cidr", "192.168.176.0/20"}, expIPAMConfig: []network.IPAMConfig{ { - // FIXME(robmry) - subnet didn't change, allocatable range - // is bigger than the subnet. - Subnet: "192.168.176.0/24", + Subnet: "192.168.176.0/20", IPRange: "192.168.176.0/20", Gateway: "192.168.176.88", }, From cc538b2bf05a575d61f84360f632fa7085f12154 Mon Sep 17 00:00:00 2001 From: Rob Murray Date: Sat, 10 Aug 2024 17:17:58 +0100 Subject: [PATCH 08/10] Drop fixed-cidr if not within user-managed bridge subnet When a user-managed bridge is used for the default network (--bridge), an address from the bridge determines the subnet for the network. If a fixed-cidr is supplied, it should fall within that subnet. If it doesn't, it's a misconfiguration - fixed-cidr is the range of allocatable addresses, and they need to be in the network. (Either the user's bridge is missing an address that matches their fixed-cidr or the fixed-cidr is wrong.) When this happens, because it's been allowed in the past (and, because the address-pool implementation treats fixed-cidr/SubPool as an offset into the network rather than an actual address range, so working IP addresses would normally still be assigned to containers) ... don't reject the config and cause daemon startup to fail. Just log a warning and ignore fixed-cidr. Signed-off-by: Rob Murray --- daemon/daemon_unix.go | 17 ++++++++++++++ integration/daemon/daemon_linux_test.go | 30 +++++++++++++++++-------- 2 files changed, 38 insertions(+), 9 deletions(-) diff --git a/daemon/daemon_unix.go b/daemon/daemon_unix.go index 46b4bb2309..ff312a791e 100644 --- a/daemon/daemon_unix.go +++ b/daemon/daemon_unix.go @@ -996,6 +996,23 @@ func initBridgeDriver(controller *libnetwork.Controller, cfg config.BridgeConfig ipamV4Conf.SubPool = fCidrIPNet.String() if ipamV4Conf.PreferredPool == "" { ipamV4Conf.PreferredPool = fCidrIPNet.String() + } else if userManagedBridge && bIPNet != nil { + fCidrOnes, _ := fCidrIPNet.Mask.Size() + bIPOnes, _ := bIPNet.Mask.Size() + if !bIPNet.Contains(fCidrIP) || (fCidrOnes < bIPOnes) { + // Don't allow SubPool (the range of allocatable addresses) to be outside, or + // bigger than, the network itself. This is a configuration error, either the + // user-managed bridge is missing an address to match fixed-cidr, or fixed-cidr + // is wrong. But, just log rather than raise an error that would cause daemon + // startup to fail, because this has been allowed by earlier versions. Remove + // the SubPool, so that addresses are allocated from the whole of PreferredPool. + log.G(context.TODO()).WithFields(log.Fields{ + "bridge": bridgeName, + "fixed-cidr": cfg.FixedCIDR, + "bridge-network": bIPNet.String(), + }).Warn("fixed-cidr is outside the subnet implied by addresses on the user-managed default bridge, this may be treated as an error in a future release") + ipamV4Conf.SubPool = "" + } } } diff --git a/integration/daemon/daemon_linux_test.go b/integration/daemon/daemon_linux_test.go index d0d898f919..43226cf491 100644 --- a/integration/daemon/daemon_linux_test.go +++ b/integration/daemon/daemon_linux_test.go @@ -215,11 +215,15 @@ func TestDaemonDefaultBridgeIPAM_UserBr(t *testing.T) { name: "fixed-cidr bigger than bridge subnet", initialBridgeAddrs: []string{"192.168.176.88/24"}, daemonArgs: []string{"--fixed-cidr", "192.168.176.0/20"}, + // fixed-cidr (the range of allocatable addresses) is bigger than the + // bridge subnet - this is a configuration error, but has historically + // been allowed. Because IPRange is treated as an offset into Subnet, it + // would normally result in a docker network that allocated addresses + // within the selected subnet. So, fixed-cidr is dropped, making the + // whole subnet allocatable. expIPAMConfig: []network.IPAMConfig{ { - Subnet: "192.168.176.0/24", - // FIXME(robmry) - allocatable range is bigger than the subnet. - IPRange: "192.168.176.0/20", + Subnet: "192.168.176.0/24", Gateway: "192.168.176.88", }, }, @@ -228,12 +232,15 @@ func TestDaemonDefaultBridgeIPAM_UserBr(t *testing.T) { name: "no bridge ip within fixed-cidr", initialBridgeAddrs: []string{"192.168.160.88/20", "192.168.192.88/20"}, daemonArgs: []string{"--fixed-cidr", "192.168.176.0/24"}, - // Selected bip should be the one within fixed-cidr + // fixed-cidr (the range of allocatable addresses) is outside the bridge + // subnet - this is a configuration error, but has historically been + // allowed. Because IPRange is treated as an offset into Subnet, it + // would normally result in a docker network that allocated addresses + // within the selected subnet. So, fixed-cidr is dropped, making the + // whole subnet allocatable. expIPAMConfig: []network.IPAMConfig{ { - // FIXME(robmry) - allocatable range outside subnet. Subnet: "192.168.160.0/20", - IPRange: "192.168.176.0/24", Gateway: "192.168.160.88", }, }, @@ -242,11 +249,16 @@ func TestDaemonDefaultBridgeIPAM_UserBr(t *testing.T) { name: "fixed-cidr contains bridge subnet", initialBridgeAddrs: []string{"192.168.177.1/24"}, daemonArgs: []string{"--fixed-cidr", "192.168.176.0/20"}, + // fixed-cidr (the range of allocatable addresses) is bigger than the + // bridge subnet, and the bridge's address is not within fixed-cidr. + // This is a configuration error, but has historically been allowed. + // Because IPRange is treated as an offset into Subnet, it would + // normally result in a docker network that allocated addresses + // within the selected subnet. So, fixed-cidr is dropped, making the + // whole subnet allocatable. expIPAMConfig: []network.IPAMConfig{ { - Subnet: "192.168.177.0/24", - // FIXME(robmry) - allocatable range is bigger than subnet - IPRange: "192.168.176.0/20", + Subnet: "192.168.177.0/24", Gateway: "192.168.177.1", }, }, From fdd2591cbe8137823ce6d47a21b7ee751ddbdb45 Mon Sep 17 00:00:00 2001 From: Rob Murray Date: Sat, 10 Aug 2024 18:38:15 +0100 Subject: [PATCH 09/10] Separate IPv4 IPAM conf from the rest of default bridge conf Signed-off-by: Rob Murray --- daemon/daemon_linux.go | 31 +---- daemon/daemon_linux_test.go | 12 +- daemon/daemon_unix.go | 251 +++++++++++++++++++++++------------- 3 files changed, 178 insertions(+), 116 deletions(-) diff --git a/daemon/daemon_linux.go b/daemon/daemon_linux.go index fa1b90fe0f..2127bf05d8 100644 --- a/daemon/daemon_linux.go +++ b/daemon/daemon_linux.go @@ -5,7 +5,6 @@ import ( "context" "fmt" "io" - "net" "os" "regexp" "strings" @@ -148,42 +147,20 @@ func setupResolvConf(config *config.Config) { config.ResolvConf = resolvconf.Path() } -// ifaceAddrs returns the IPv4 and IPv6 addresses assigned to the network +// ifaceAddrs returns the addresses from family assigned to the network // interface with name linkName. // // No error is returned if the named interface does not exist. -func ifaceAddrs(linkName string) (v4, v6 []*net.IPNet, err error) { +func ifaceAddrs(linkName string, family int) ([]netlink.Addr, error) { nl := ns.NlHandle() link, err := nl.LinkByName(linkName) if err != nil { if !errors.As(err, new(netlink.LinkNotFoundError)) { - return nil, nil, err - } - return nil, nil, nil - } - - get := func(family int) ([]*net.IPNet, error) { - addrs, err := nl.AddrList(link, family) - if err != nil { return nil, err } - - ipnets := make([]*net.IPNet, len(addrs)) - for i := range addrs { - ipnets[i] = addrs[i].IPNet - } - return ipnets, nil + return nil, nil } - - v4, err = get(netlink.FAMILY_V4) - if err != nil { - return nil, nil, err - } - v6, err = get(netlink.FAMILY_V6) - if err != nil { - return nil, nil, err - } - return v4, v6, nil + return nl.AddrList(link, family) } var ( diff --git a/daemon/daemon_linux_test.go b/daemon/daemon_linux_test.go index b6e3125eb2..eaf496d8f0 100644 --- a/daemon/daemon_linux_test.go +++ b/daemon/daemon_linux_test.go @@ -363,12 +363,20 @@ func TestIfaceAddrs(t *testing.T) { createBridge(t, "test", tt.nws...) - ipv4Nw, ipv6Nw, err := ifaceAddrs("test") + ipv4Nw, err := ifaceAddrs("test", netlink.FAMILY_V4) + if err != nil { + t.Fatal(err) + } + ipv6Nw, err := ifaceAddrs("test", netlink.FAMILY_V6) if err != nil { t.Fatal(err) } - assert.Check(t, is.DeepEqual(tt.nws, ipv4Nw, + ipnets := make([]*net.IPNet, len(ipv4Nw)) + for i := range ipv4Nw { + ipnets[i] = ipv4Nw[i].IPNet + } + assert.Check(t, is.DeepEqual(ipnets, tt.nws, cmpopts.SortSlices(func(a, b *net.IPNet) bool { return a.String() < b.String() }))) // IPv6 link-local address assert.Check(t, is.Len(ipv6Nw, 1)) diff --git a/daemon/daemon_unix.go b/daemon/daemon_unix.go index ff312a791e..fe658b5bf9 100644 --- a/daemon/daemon_unix.go +++ b/daemon/daemon_unix.go @@ -940,84 +940,9 @@ func initBridgeDriver(controller *libnetwork.Controller, cfg config.BridgeConfig netOption[bridge.DefaultBindingIP] = cfg.DefaultIP.String() } - nwList, nw6List, err := ifaceAddrs(bridgeName) + ipamV4Conf, err := getDefaultBridgeIPAMConf(bridgeName, userManagedBridge, cfg, netlink.FAMILY_V4) if err != nil { - return errors.Wrap(err, "list bridge addresses failed") - } - - var ( - fCidrIP, bIP net.IP - fCidrIPNet, bIPNet *net.IPNet - ) - - if cfg.FixedCIDR != "" { - if fCidrIP, fCidrIPNet, err = net.ParseCIDR(cfg.FixedCIDR); err != nil { - return errors.Wrap(err, "parse fixed-cidr failed") - } - } - - if cfg.IP != "" { - if bIP, bIPNet, err = net.ParseCIDR(cfg.IP); err != nil { - return err - } - } else { - bIP, bIPNet = selectBIP(nwList, fCidrIP, fCidrIPNet) - if !userManagedBridge && fCidrIP != nil && bIPNet != nil { - if !bIPNet.Contains(fCidrIP) { - // The bridge is docker-managed (docker0) and fixed-cidr is not - // inside a subnet belonging to any existing bridge IP. (fixed-cidr - // has changed.) So, ignore the existing bridge IP. - bIP = nil - bIPNet = nil - } else { - fCidrOnes, _ := fCidrIPNet.Mask.Size() - bIPOnes, _ := bIPNet.Mask.Size() - if fCidrOnes < bIPOnes { - // The bridge is docker-managed (docker0) and fixed-cidr (the - // allocatable address range) is bigger than the subnet implied - // by the bridge's current address. (fixed-cidr has changed.) - // The bridge's address is ok, but its subnet needs to be updated. - bIPNet.IP = bIPNet.IP.Mask(fCidrIPNet.Mask) - bIPNet.Mask = fCidrIPNet.Mask - } - } - } - } - - ipamV4Conf := &libnetwork.IpamConf{AuxAddresses: make(map[string]string)} - if bIP != nil { - ipamV4Conf.PreferredPool = bIPNet.String() - ipamV4Conf.Gateway = bIP.String() - } else if !userManagedBridge && ipamV4Conf.PreferredPool != "" { - log.G(context.TODO()).Infof("Default bridge (%s) is assigned with an IP address %s. Daemon option --bip can be used to set a preferred IP address", bridgeName, ipamV4Conf.PreferredPool) - } - - if fCidrIP != nil { - ipamV4Conf.SubPool = fCidrIPNet.String() - if ipamV4Conf.PreferredPool == "" { - ipamV4Conf.PreferredPool = fCidrIPNet.String() - } else if userManagedBridge && bIPNet != nil { - fCidrOnes, _ := fCidrIPNet.Mask.Size() - bIPOnes, _ := bIPNet.Mask.Size() - if !bIPNet.Contains(fCidrIP) || (fCidrOnes < bIPOnes) { - // Don't allow SubPool (the range of allocatable addresses) to be outside, or - // bigger than, the network itself. This is a configuration error, either the - // user-managed bridge is missing an address to match fixed-cidr, or fixed-cidr - // is wrong. But, just log rather than raise an error that would cause daemon - // startup to fail, because this has been allowed by earlier versions. Remove - // the SubPool, so that addresses are allocated from the whole of PreferredPool. - log.G(context.TODO()).WithFields(log.Fields{ - "bridge": bridgeName, - "fixed-cidr": cfg.FixedCIDR, - "bridge-network": bIPNet.String(), - }).Warn("fixed-cidr is outside the subnet implied by addresses on the user-managed default bridge, this may be treated as an error in a future release") - ipamV4Conf.SubPool = "" - } - } - } - - if cfg.DefaultGatewayIPv4 != nil { - ipamV4Conf.AuxAddresses["DefaultGatewayIPv4"] = cfg.DefaultGatewayIPv4.String() + return err } var ( @@ -1050,6 +975,10 @@ func initBridgeDriver(controller *libnetwork.Controller, cfg config.BridgeConfig // In case the --fixed-cidr-v6 is specified and the current docker0 bridge IPv6 // address belongs to the same network, we need to inform libnetwork about it, so // that it can be reserved with IPAM and it will not be given away to somebody else + nw6List, err := ifaceAddrs(bridgeName, netlink.FAMILY_V6) + if err != nil { + return errors.Wrap(err, "list bridge IPv6 addresses failed") + } for _, nw6 := range nw6List { if fCIDRv6.Contains(nw6.IP) { ipamV6Conf.Gateway = nw6.IP.String() @@ -1065,7 +994,6 @@ func initBridgeDriver(controller *libnetwork.Controller, cfg config.BridgeConfig ipamV6Conf.AuxAddresses["DefaultGatewayIPv6"] = cfg.DefaultGatewayIPv6.String() } - v4Conf := []*libnetwork.IpamConf{ipamV4Conf} v6Conf := []*libnetwork.IpamConf{} if ipamV6Conf != nil { v6Conf = append(v6Conf, ipamV6Conf) @@ -1075,7 +1003,7 @@ func initBridgeDriver(controller *libnetwork.Controller, cfg config.BridgeConfig libnetwork.NetworkOptionEnableIPv4(true), libnetwork.NetworkOptionEnableIPv6(cfg.EnableIPv6), libnetwork.NetworkOptionDriverOpts(netOption), - libnetwork.NetworkOptionIpam("default", "", v4Conf, v6Conf, nil), + libnetwork.NetworkOptionIpam("default", "", ipamV4Conf, v6Conf, nil), libnetwork.NetworkOptionDeferIPv6Alloc(deferIPv6Alloc)) if err != nil { return fmt.Errorf(`error creating default %q network: %v`, network.NetworkBridge, err) @@ -1105,25 +1033,152 @@ func getDefaultBridgeName(cfg config.BridgeConfig) (bridgeName string, userManag return bridge.DefaultBridgeName, false } -// selectBIP searches bridgeNws for: +// getDefaultBridgeIPAMConf works out IPAM configuration for the +// default bridge, for the given address family (netlink.FAMILY_V4 or +// netlink.FAMILY_V6). +// +// Inputs are: +// - bip +// - CIDR, not a plain IP address. +// - docker-managed bridge only (docker0), not allowed with a user-managed +// bridge (--bridge) where the user is responsible for creating the bridge +// and configuring its addresses. +// - determines the network subnet (ipamConf.PreferredPool) and becomes the +// bridge/gateway address (ipamConf.Gateway). +// +// - existing bridge addresses +// - for a user-managed bridge +// - an address is selected from the bridge to perform the same role as +// bip for a daemon-managed bridge. +// - for docker0 +// - if there's an address on the bridge that's compatible with the other +// options, it's used as the gateway address and - because it's always +// worked this way, to determine the subnet if it's bigger than the +// sub-pool configured through fixed-cidr[-v6]. For example, if the +// bridge has address 10.11.12.13/16 and fixed-cidr=10.11.22.0/24, the +// default bridge network's subnet is 10.11.0.0/16, sub-pool for +// automatic address allocation 10.11.22.0/24, gateway 10.11.12.13. +// +// - fixed-cidr/fixed-cidr-v6 +// - ipamConf.SubPool, the pool for automatic address allocation (somewhat +// equivalent to --ip-range for a user-defined network), must be contained +// within the subnet. Used as ipamConf.PreferredPool if it's not given a +// value by other rules. +// +// So, for example, with this config (taken from docs): +// +// "bip": "192.168.1.1/24", +// "fixed-cidr": "192.168.1.0/25", +// +// - the bridge's address is "192.168.1.1/24" +// - the subnet is "192.168.1.0/24" +// - the bridge driver can allocate addresses from "192.168.1.0/25" +// +// The result is the same if "bip" is unset (including for a user-managed +// bridge), when the bridge already has address "192.168.1.1/24". +// +// Note that this function logs-then-ignores invalid configuration, because it +// has to tolerate existing configuration - raising an error prevents daemon +// startup. Earlier versions of the daemon didn't spot bad config, but generally +// did something unsurprising with it. +func getDefaultBridgeIPAMConf( + bridgeName string, + userManagedBridge bool, + cfg config.BridgeConfig, + family int, +) ([]*libnetwork.IpamConf, error) { + var ( + fCidrIP, bIP net.IP + fCidrIPNet, bIPNet *net.IPNet + err error + ) + + if cfg.FixedCIDR != "" { + if fCidrIP, fCidrIPNet, err = net.ParseCIDR(cfg.FixedCIDR); err != nil { + return nil, errors.Wrap(err, "parse fixed-cidr failed") + } + } + + if cfg.IP != "" { + if bIP, bIPNet, err = net.ParseCIDR(cfg.IP); err != nil { + return nil, err + } + } else { + if bIP, bIPNet, err = selectBIP(userManagedBridge, bridgeName, family, fCidrIP, fCidrIPNet); err != nil { + return nil, err + } + } + + ipamConf := &libnetwork.IpamConf{AuxAddresses: make(map[string]string)} + if bIP != nil { + ipamConf.PreferredPool = bIPNet.String() + ipamConf.Gateway = bIP.String() + } else if !userManagedBridge && ipamConf.PreferredPool != "" { + log.G(context.TODO()).Infof("Default bridge (%s) is assigned with an IP address %s. Daemon option --bip can be used to set a preferred IP address", bridgeName, ipamConf.PreferredPool) + } + + if fCidrIP != nil && fCidrIPNet != nil { + ipamConf.SubPool = fCidrIPNet.String() + if ipamConf.PreferredPool == "" { + ipamConf.PreferredPool = fCidrIPNet.String() + } else if userManagedBridge && bIPNet != nil { + fCidrOnes, _ := fCidrIPNet.Mask.Size() + bIPOnes, _ := bIPNet.Mask.Size() + if !bIPNet.Contains(fCidrIP) || (fCidrOnes < bIPOnes) { + // Don't allow SubPool (the range of allocatable addresses) to be outside, or + // bigger than, the network itself. This is a configuration error, either the + // user-managed bridge is missing an address to match fixed-cidr, or fixed-cidr + // is wrong. But, just log rather than raise an error that would cause daemon + // startup to fail, because this has been allowed by earlier versions. Remove + // the SubPool, so that addresses are allocated from the whole of PreferredPool. + log.G(context.TODO()).WithFields(log.Fields{ + "bridge": bridgeName, + "fixed-cidr": cfg.FixedCIDR, + "bridge-network": bIPNet.String(), + }).Warn("fixed-cidr is outside the subnet implied by addresses on the user-managed default bridge, this may be treated as an error in a future release") + ipamConf.SubPool = "" + } + } + } + + if cfg.DefaultGatewayIPv4 != nil { + ipamConf.AuxAddresses["DefaultGatewayIPv4"] = cfg.DefaultGatewayIPv4.String() + } + + return []*libnetwork.IpamConf{ipamConf}, nil +} + +// selectBIP searches the addresses from family on bridge bridgeName for: // - An address that encompasses fCidrNet if there is one. // - Else, an address that is within fCidrNet if there is one. // - Else, any address, if there is one. -// If an address is found, it's returned as bIP, with its subnet in canonical +// +// If an address is found, the bridge is docker managed (docker0), and the +// bridge address is not compatible with current fixed-cidr/bip configuration, +// the address is ignored or modified accordingly, so that the current config +// can take effect. +// +// If there is an address, it's returned as bIP with its subnet in canonical // form in bIPNet. func selectBIP( - bridgeNws []*net.IPNet, + userManagedBridge bool, + bridgeName string, + family int, fCidrIP net.IP, fCidrNet *net.IPNet, -) (bIP net.IP, bIPNet *net.IPNet) { +) (bIP net.IP, bIPNet *net.IPNet, err error) { + bridgeNws, err := ifaceAddrs(bridgeName, family) + if err != nil { + return nil, nil, errors.Wrap(err, "list bridge addresses failed") + } if len(bridgeNws) > 0 { // Pick any address from the bridge as a starting point. - nw := bridgeNws[0] + nw := bridgeNws[0].IPNet if len(bridgeNws) > 1 && fCidrNet != nil { // If there's an address with a subnet that contains fixed-cidr, use it. for _, entry := range bridgeNws { if entry.Contains(fCidrIP) { - nw = entry + nw = entry.IPNet break } // For backwards compatibility - prefer the first bridge address within @@ -1131,7 +1186,7 @@ func selectBIP( // make sense - the allocatable range (fixed-cidr) will be bigger than the subnet // (entry.IPNet). if fCidrNet.Contains(entry.IP) && !fCidrNet.Contains(nw.IP) { - nw = entry + nw = entry.IPNet } } } @@ -1139,7 +1194,29 @@ func selectBIP( bIP = nw.IP bIPNet = lntypes.GetIPNetCanonical(nw) } - return bIP, bIPNet + + if !userManagedBridge && fCidrIP != nil && bIPNet != nil { + if !bIPNet.Contains(fCidrIP) { + // The bridge is docker-managed (docker0) and fixed-cidr is not + // inside a subnet belonging to any existing bridge IP. (fixed-cidr + // has changed.) So, ignore the existing bridge IP. + bIP = nil + bIPNet = nil + } else { + fCidrOnes, _ := fCidrNet.Mask.Size() + bIPOnes, _ := bIPNet.Mask.Size() + if fCidrOnes < bIPOnes { + // The bridge is docker-managed (docker0) and fixed-cidr (the + // allocatable address range) is bigger than the subnet implied + // by the bridge's current address. (fixed-cidr has changed.) + // The bridge's address is ok, but its subnet needs to be updated. + bIPNet.IP = bIPNet.IP.Mask(fCidrNet.Mask) + bIPNet.Mask = fCidrNet.Mask + } + } + } + + return bIP, bIPNet, nil } // Remove default bridge interface if present (--bridge=none use case) From 0b5b1db1c1206b9196bfe8c7b4f1d9da7e3db407 Mon Sep 17 00:00:00 2001 From: Rob Murray Date: Sat, 10 Aug 2024 20:03:21 +0100 Subject: [PATCH 10/10] Use default ULA prefix if fixed-cidr-v6 is not specified Use the same logic to generate IPAMConf for IPv6 as for IPv4. - When no fixed-cidr-v6 is specified, rather than error out, use the default address pools (as for an IPv4 default bridge with no fixed-cidr, and as for user-defined networks). - Add daemon option --bip6, similar to --bip. - Necessary because it's the only way to override an old address on docker0 (daemon-managed default bridge), as illustrated by test cases. - For a user-managed default bridge (--bridge), use IPv6 addresses on the user's bridge to determine the pool, sub-pool and gateway. Following the same rules as IPv4. - Don't set up IPv6 IPAMConf if IPv6 is not enabled. Signed-off-by: Rob Murray --- cmd/dockerd/config_unix.go | 3 +- cmd/dockerd/daemon_unix_test.go | 7 +- daemon/config/config_linux.go | 1 + daemon/daemon_unix.go | 149 ++++++----- integration/daemon/daemon_linux_test.go | 262 ++++++++++++-------- integration/networking/bridge_linux_test.go | 30 ++- man/dockerd.8.md | 7 +- 7 files changed, 277 insertions(+), 182 deletions(-) diff --git a/cmd/dockerd/config_unix.go b/cmd/dockerd/config_unix.go index 6074f2c995..91ee008ce2 100644 --- a/cmd/dockerd/config_unix.go +++ b/cmd/dockerd/config_unix.go @@ -31,7 +31,8 @@ func installConfigFlags(conf *config.Config, flags *pflag.FlagSet) { flags.BoolVar(&conf.BridgeConfig.DisableFilterForwardDrop, "ip-forward-no-drop", false, "Do not set the filter-FORWARD policy to DROP when enabling IP forwarding") flags.BoolVar(&conf.BridgeConfig.EnableIPMasq, "ip-masq", true, "Enable IP masquerading") flags.BoolVar(&conf.BridgeConfig.EnableIPv6, "ipv6", false, "Enable IPv6 networking") - flags.StringVar(&conf.BridgeConfig.IP, "bip", "", "Specify network bridge IP") + flags.StringVar(&conf.BridgeConfig.IP, "bip", "", "Specify default-bridge IPv4 network") + flags.StringVar(&conf.BridgeConfig.IP6, "bip6", "", "Specify default-bridge IPv6 network") flags.StringVarP(&conf.BridgeConfig.Iface, "bridge", "b", "", "Attach containers to a network bridge") flags.StringVar(&conf.BridgeConfig.FixedCIDR, "fixed-cidr", "", "IPv4 subnet for fixed IPs") flags.StringVar(&conf.BridgeConfig.FixedCIDRv6, "fixed-cidr-v6", "", "IPv6 subnet for fixed IPs") diff --git a/cmd/dockerd/daemon_unix_test.go b/cmd/dockerd/daemon_unix_test.go index df61485986..669be65850 100644 --- a/cmd/dockerd/daemon_unix_test.go +++ b/cmd/dockerd/daemon_unix_test.go @@ -33,7 +33,7 @@ func TestLoadDaemonCliConfigWithDaemonFlags(t *testing.T) { } func TestLoadDaemonConfigWithNetwork(t *testing.T) { - content := `{"bip": "127.0.0.2", "ip": "127.0.0.1"}` + content := `{"bip": "127.0.0.2/8", "bip6": "fd98:e5f2:e637::1/64", "ip": "127.0.0.1"}` tempFile := fs.NewFile(t, "config", fs.WithContent(content)) defer tempFile.Remove() @@ -42,8 +42,9 @@ func TestLoadDaemonConfigWithNetwork(t *testing.T) { assert.NilError(t, err) assert.Assert(t, loadedConfig != nil) - assert.Check(t, is.Equal("127.0.0.2", loadedConfig.IP)) - assert.Check(t, is.Equal("127.0.0.1", loadedConfig.DefaultIP.String())) + assert.Check(t, is.Equal(loadedConfig.IP, "127.0.0.2/8")) + assert.Check(t, is.Equal(loadedConfig.IP6, "fd98:e5f2:e637::1/64")) + assert.Check(t, is.Equal(loadedConfig.DefaultIP.String(), "127.0.0.1")) } func TestLoadDaemonConfigWithMapOptions(t *testing.T) { diff --git a/daemon/config/config_linux.go b/daemon/config/config_linux.go index 18ca1007bd..9668a99eee 100644 --- a/daemon/config/config_linux.go +++ b/daemon/config/config_linux.go @@ -60,6 +60,7 @@ type DefaultBridgeConfig struct { MTU int `json:"mtu,omitempty"` DefaultIP net.IP `json:"ip,omitempty"` IP string `json:"bip,omitempty"` + IP6 string `json:"bip6,omitempty"` DefaultGatewayIPv4 net.IP `json:"default-gateway,omitempty"` DefaultGatewayIPv6 net.IP `json:"default-gateway-v6,omitempty"` InterContainerCommunication bool `json:"icc,omitempty"` diff --git a/daemon/daemon_unix.go b/daemon/daemon_unix.go index fe658b5bf9..0040fa48de 100644 --- a/daemon/daemon_unix.go +++ b/daemon/daemon_unix.go @@ -737,6 +737,9 @@ func verifyDaemonSettings(conf *config.Config) error { if conf.BridgeConfig.Iface != "" && conf.BridgeConfig.IP != "" { return fmt.Errorf("You specified -b & --bip, mutually exclusive options. Please specify only one") } + if conf.BridgeConfig.Iface != "" && conf.BridgeConfig.IP6 != "" { + return fmt.Errorf("You specified -b & --bip6, mutually exclusive options. Please specify only one") + } if !conf.BridgeConfig.InterContainerCommunication { if !conf.BridgeConfig.EnableIPTables { return fmt.Errorf("You specified --iptables=false with --icc=false. ICC=false uses iptables to function. Please set --icc or --iptables to true") @@ -926,6 +929,47 @@ func driverOptions(config *config.Config) nwconfig.Option { }) } +type defBrOptsV4 struct { + cfg config.BridgeConfig +} + +func (o defBrOptsV4) nlFamily() int { + return netlink.FAMILY_V4 +} +func (o defBrOptsV4) fixedCIDR() (fCIDR, optName string) { + return o.cfg.FixedCIDR, "fixed-cidr" +} +func (o defBrOptsV4) bip() (bip, optName string) { + return o.cfg.IP, "bip" +} +func (o defBrOptsV4) defGw() (gw net.IP, optName, auxAddrLabel string) { + return o.cfg.DefaultGatewayIPv4, "default-gateway", "DefaultGatewayIPv4" +} + +type defBrOptsV6 struct { + cfg config.BridgeConfig +} + +func (o defBrOptsV6) nlFamily() int { + return netlink.FAMILY_V6 +} +func (o defBrOptsV6) fixedCIDR() (fCIDR, optName string) { + return o.cfg.FixedCIDRv6, "fixed-cidr-v6" +} +func (o defBrOptsV6) bip() (bip, optName string) { + return o.cfg.IP6, "bip6" +} +func (o defBrOptsV6) defGw() (gw net.IP, optName, auxAddrLabel string) { + return o.cfg.DefaultGatewayIPv6, "default-gateway-v6", "DefaultGatewayIPv6" +} + +type defBrOpts interface { + nlFamily() int + fixedCIDR() (fCIDR, optName string) + bip() (bip, optName string) + defGw() (gw net.IP, optName, auxAddrLabel string) +} + func initBridgeDriver(controller *libnetwork.Controller, cfg config.BridgeConfig) error { bridgeName, userManagedBridge := getDefaultBridgeName(cfg) netOption := map[string]string{ @@ -940,70 +984,43 @@ func initBridgeDriver(controller *libnetwork.Controller, cfg config.BridgeConfig netOption[bridge.DefaultBindingIP] = cfg.DefaultIP.String() } - ipamV4Conf, err := getDefaultBridgeIPAMConf(bridgeName, userManagedBridge, cfg, netlink.FAMILY_V4) + ipamV4Conf, err := getDefaultBridgeIPAMConf(bridgeName, userManagedBridge, defBrOptsV4{cfg}) if err != nil { return err } - var ( - deferIPv6Alloc bool - ipamV6Conf *libnetwork.IpamConf - ) - - if cfg.EnableIPv6 && cfg.FixedCIDRv6 == "" { - return errdefs.InvalidParameter(errors.New("IPv6 is enabled for the default bridge, but no subnet is configured. Specify an IPv6 subnet using --fixed-cidr-v6")) - } else if cfg.FixedCIDRv6 != "" { - _, fCIDRv6, err := net.ParseCIDR(cfg.FixedCIDRv6) + var deferIPv6Alloc bool + var ipamV6Conf []*libnetwork.IpamConf + if cfg.EnableIPv6 { + ipamV6Conf, err = getDefaultBridgeIPAMConf(bridgeName, userManagedBridge, defBrOptsV6{cfg}) if err != nil { return err } - - // In case user has specified the daemon flag --fixed-cidr-v6 and the passed network has - // at least 48 host bits, we need to guarantee the current behavior where the containers' - // IPv6 addresses will be constructed based on the containers' interface MAC address. - // We do so by telling libnetwork to defer the IPv6 address allocation for the endpoints - // on this network until after the driver has created the endpoint and returned the - // constructed address. Libnetwork will then reserve this address with the ipam driver. - ones, _ := fCIDRv6.Mask.Size() - deferIPv6Alloc = ones <= 80 - - ipamV6Conf = &libnetwork.IpamConf{ - AuxAddresses: make(map[string]string), - PreferredPool: fCIDRv6.String(), - } - - // In case the --fixed-cidr-v6 is specified and the current docker0 bridge IPv6 - // address belongs to the same network, we need to inform libnetwork about it, so - // that it can be reserved with IPAM and it will not be given away to somebody else - nw6List, err := ifaceAddrs(bridgeName, netlink.FAMILY_V6) - if err != nil { - return errors.Wrap(err, "list bridge IPv6 addresses failed") - } - for _, nw6 := range nw6List { - if fCIDRv6.Contains(nw6.IP) { - ipamV6Conf.Gateway = nw6.IP.String() - break + // If the subnet has at least 48 host bits, preserve the legacy default bridge + // behaviour of constructing a MAC address from the IPv4 address, then + // constructing an IPv6 addresses based on that MAC address. Tell libnetwork to + // defer the IPv6 address allocation for endpoints on this network until after + // the driver has created the endpoint and proposed an IPv4 address. Libnetwork + // will then reserve this address with the ipam driver. If no preferred pool has + // been set the built-in ULA prefix will be used, assume it has at-least 48-bits. + if len(ipamV6Conf) == 0 || ipamV6Conf[0].PreferredPool == "" { + deferIPv6Alloc = true + } else { + _, ppNet, err := net.ParseCIDR(ipamV6Conf[0].PreferredPool) + if err != nil { + return err } + ones, _ := ppNet.Mask.Size() + deferIPv6Alloc = ones <= 80 } } - if cfg.DefaultGatewayIPv6 != nil { - if ipamV6Conf == nil { - ipamV6Conf = &libnetwork.IpamConf{AuxAddresses: make(map[string]string)} - } - ipamV6Conf.AuxAddresses["DefaultGatewayIPv6"] = cfg.DefaultGatewayIPv6.String() - } - - v6Conf := []*libnetwork.IpamConf{} - if ipamV6Conf != nil { - v6Conf = append(v6Conf, ipamV6Conf) - } // Initialize default network on "bridge" with the same name _, err = controller.NewNetwork("bridge", network.NetworkBridge, "", libnetwork.NetworkOptionEnableIPv4(true), libnetwork.NetworkOptionEnableIPv6(cfg.EnableIPv6), libnetwork.NetworkOptionDriverOpts(netOption), - libnetwork.NetworkOptionIpam("default", "", ipamV4Conf, v6Conf, nil), + libnetwork.NetworkOptionIpam("default", "", ipamV4Conf, ipamV6Conf, nil), libnetwork.NetworkOptionDeferIPv6Alloc(deferIPv6Alloc)) if err != nil { return fmt.Errorf(`error creating default %q network: %v`, network.NetworkBridge, err) @@ -1084,8 +1101,7 @@ func getDefaultBridgeName(cfg config.BridgeConfig) (bridgeName string, userManag func getDefaultBridgeIPAMConf( bridgeName string, userManagedBridge bool, - cfg config.BridgeConfig, - family int, + opts defBrOpts, ) ([]*libnetwork.IpamConf, error) { var ( fCidrIP, bIP net.IP @@ -1093,18 +1109,18 @@ func getDefaultBridgeIPAMConf( err error ) - if cfg.FixedCIDR != "" { - if fCidrIP, fCidrIPNet, err = net.ParseCIDR(cfg.FixedCIDR); err != nil { - return nil, errors.Wrap(err, "parse fixed-cidr failed") + if fixedCIDR, fixedCIDROpt := opts.fixedCIDR(); fixedCIDR != "" { + if fCidrIP, fCidrIPNet, err = net.ParseCIDR(fixedCIDR); err != nil { + return nil, errors.Wrap(err, "parse "+fixedCIDROpt+" failed") } } - if cfg.IP != "" { - if bIP, bIPNet, err = net.ParseCIDR(cfg.IP); err != nil { - return nil, err + if cfgBIP, cfgBIPOpt := opts.bip(); cfgBIP != "" { + if bIP, bIPNet, err = net.ParseCIDR(cfgBIP); err != nil { + return nil, errors.Wrap(err, "parse "+cfgBIPOpt+" failed") } } else { - if bIP, bIPNet, err = selectBIP(userManagedBridge, bridgeName, family, fCidrIP, fCidrIPNet); err != nil { + if bIP, bIPNet, err = selectBIP(userManagedBridge, bridgeName, opts.nlFamily(), fCidrIP, fCidrIPNet); err != nil { return nil, err } } @@ -1114,7 +1130,8 @@ func getDefaultBridgeIPAMConf( ipamConf.PreferredPool = bIPNet.String() ipamConf.Gateway = bIP.String() } else if !userManagedBridge && ipamConf.PreferredPool != "" { - log.G(context.TODO()).Infof("Default bridge (%s) is assigned with an IP address %s. Daemon option --bip can be used to set a preferred IP address", bridgeName, ipamConf.PreferredPool) + _, bipOptName := opts.bip() + log.G(context.TODO()).Infof("Default bridge (%s) is assigned with an IP address %s. Daemon option --"+bipOptName+" can be used to set a preferred IP address", bridgeName, ipamConf.PreferredPool) } if fCidrIP != nil && fCidrIPNet != nil { @@ -1128,21 +1145,27 @@ func getDefaultBridgeIPAMConf( // Don't allow SubPool (the range of allocatable addresses) to be outside, or // bigger than, the network itself. This is a configuration error, either the // user-managed bridge is missing an address to match fixed-cidr, or fixed-cidr - // is wrong. But, just log rather than raise an error that would cause daemon + // is wrong. + fixedCIDR, fixedCIDROpt := opts.fixedCIDR() + if opts.nlFamily() == netlink.FAMILY_V6 { + return nil, fmt.Errorf("%s=%s is outside any subnet implied by addresses on the user-managed default bridge", + fixedCIDROpt, fixedCIDR) + } + // For IPv4, just log rather than raise an error that would cause daemon // startup to fail, because this has been allowed by earlier versions. Remove // the SubPool, so that addresses are allocated from the whole of PreferredPool. log.G(context.TODO()).WithFields(log.Fields{ "bridge": bridgeName, - "fixed-cidr": cfg.FixedCIDR, + fixedCIDROpt: fixedCIDR, "bridge-network": bIPNet.String(), - }).Warn("fixed-cidr is outside the subnet implied by addresses on the user-managed default bridge, this may be treated as an error in a future release") + }).Warn(fixedCIDROpt + " is outside any subnet implied by addresses on the user-managed default bridge, this may be treated as an error in a future release") ipamConf.SubPool = "" } } } - if cfg.DefaultGatewayIPv4 != nil { - ipamConf.AuxAddresses["DefaultGatewayIPv4"] = cfg.DefaultGatewayIPv4.String() + if defGw, _, auxAddrLabel := opts.defGw(); defGw != nil { + ipamConf.AuxAddresses[auxAddrLabel] = defGw.String() } return []*libnetwork.IpamConf{ipamConf}, nil diff --git a/integration/daemon/daemon_linux_test.go b/integration/daemon/daemon_linux_test.go index 43226cf491..a89c9f8929 100644 --- a/integration/daemon/daemon_linux_test.go +++ b/integration/daemon/daemon_linux_test.go @@ -44,39 +44,55 @@ func TestDaemonDefaultBridgeIPAM_Docker0(t *testing.T) { testcases := []defaultBridgeIPAMTestCase{ { - name: "fixed-cidr only", - daemonArgs: []string{"--fixed-cidr", "192.168.176.0/24"}, + name: "no config", + // No config for the bridge, but override default-address-pools to + // get a predictable result for IPv6 (rather than the daemon's ULA). + daemonArgs: []string{ + "--default-address-pool", `base=192.168.176.0/20,size=24`, + "--default-address-pool", `base=fdd1:8161:2d2c::/56,size=64`, + }, expIPAMConfig: []network.IPAMConfig{ - { - Subnet: "192.168.176.0/24", - IPRange: "192.168.176.0/24", - }, + {Subnet: "192.168.176.0/24", Gateway: "192.168.176.1"}, + {Subnet: "fdd1:8161:2d2c::/64", Gateway: "fdd1:8161:2d2c::1/64"}, }, }, { - name: "bip only", - daemonArgs: []string{"--bip", "192.168.176.88/24"}, + name: "fixed-cidr only", + daemonArgs: []string{ + "--fixed-cidr", "192.168.176.0/24", + "--fixed-cidr-v6", "fdd1:8161:2d2c::/64", + }, expIPAMConfig: []network.IPAMConfig{ - { - Subnet: "192.168.176.0/24", - Gateway: "192.168.176.88", - }, + {Subnet: "192.168.176.0/24", IPRange: "192.168.176.0/24"}, + {Subnet: "fdd1:8161:2d2c::/64", IPRange: "fdd1:8161:2d2c::/64"}, + }, + }, + { + name: "bip only", + daemonArgs: []string{ + "--bip", "192.168.176.88/24", + "--bip6", "fdd1:8161:2d2c::8888/64", + }, + expIPAMConfig: []network.IPAMConfig{ + {Subnet: "192.168.176.0/24", Gateway: "192.168.176.88"}, + {Subnet: "fdd1:8161:2d2c::/64", Gateway: "fdd1:8161:2d2c::8888"}, }, }, { name: "existing bridge address only", - initialBridgeAddrs: []string{"192.168.176.88/24"}, + initialBridgeAddrs: []string{"192.168.176.88/24", "fdd1:8161:2d2c::8888/64"}, expIPAMConfig: []network.IPAMConfig{ - { - Subnet: "192.168.176.0/24", - Gateway: "192.168.176.88", - }, + {Subnet: "192.168.176.0/24", Gateway: "192.168.176.88"}, + {Subnet: "fdd1:8161:2d2c::/64", Gateway: "fdd1:8161:2d2c::8888"}, }, }, { name: "fixed-cidr within old bridge subnet", - initialBridgeAddrs: []string{"192.168.176.88/20"}, - daemonArgs: []string{"--fixed-cidr", "192.168.176.0/24"}, + initialBridgeAddrs: []string{"192.168.176.88/20", "fdd1:8161:2d2c::8888/56"}, + daemonArgs: []string{ + "--fixed-cidr", "192.168.176.0/24", + "--fixed-cidr-v6", "fdd1:8161:2d2c::/64", + }, // There's no --bip to dictate the subnet, so it's derived from an // existing bridge address. If fixed-cidr's subnet is made smaller // following a daemon restart, a user might reasonably expect the @@ -89,67 +105,65 @@ func TestDaemonDefaultBridgeIPAM_Docker0(t *testing.T) { // and it'd be a breaking change for anyone relying on the existing // behaviour. expIPAMConfig: []network.IPAMConfig{ - { - Subnet: "192.168.176.0/20", - IPRange: "192.168.176.0/24", - Gateway: "192.168.176.88", - }, + {Subnet: "192.168.176.0/20", IPRange: "192.168.176.0/24", Gateway: "192.168.176.88"}, + {Subnet: "fdd1:8161:2d2c::/56", IPRange: "fdd1:8161:2d2c::/64", Gateway: "fdd1:8161:2d2c::8888"}, }, }, { name: "fixed-cidr within old bridge subnet with new bip", - initialBridgeAddrs: []string{"192.168.176.88/20"}, - daemonArgs: []string{"--fixed-cidr", "192.168.176.0/24", "--bip", "192.168.176.99/24"}, + initialBridgeAddrs: []string{"192.168.176.88/20", "fdd1:8161:2d2c::/56"}, + daemonArgs: []string{ + "--fixed-cidr", "192.168.176.0/24", "--bip", "192.168.176.99/24", + "--fixed-cidr-v6", "fdd1:8161:2d2c::/64", "--bip6", "fdd1:8161:2d2c::9999/64", + }, expIPAMConfig: []network.IPAMConfig{ - { - Subnet: "192.168.176.0/24", - IPRange: "192.168.176.0/24", - Gateway: "192.168.176.99", - }, + {Subnet: "192.168.176.0/24", IPRange: "192.168.176.0/24", Gateway: "192.168.176.99"}, + {Subnet: "fdd1:8161:2d2c::/64", IPRange: "fdd1:8161:2d2c::/64", Gateway: "fdd1:8161:2d2c::9999"}, }, }, { name: "old bridge subnet within fixed-cidr", - initialBridgeAddrs: []string{"192.168.176.88/24"}, - daemonArgs: []string{"--fixed-cidr", "192.168.176.0/20"}, + initialBridgeAddrs: []string{"192.168.176.88/24", "fdd1:8161:2d2c::8888/64"}, + daemonArgs: []string{ + "--fixed-cidr", "192.168.176.0/20", + "--fixed-cidr-v6", "fdd1:8161:2d2c::/56", + }, expIPAMConfig: []network.IPAMConfig{ - { - Subnet: "192.168.176.0/20", - IPRange: "192.168.176.0/20", - Gateway: "192.168.176.88", - }, + {Subnet: "192.168.176.0/20", IPRange: "192.168.176.0/20", Gateway: "192.168.176.88"}, + {Subnet: "fdd1:8161:2d2c::/56", IPRange: "fdd1:8161:2d2c::/56", Gateway: "fdd1:8161:2d2c::8888"}, }, }, { name: "old bridge subnet outside fixed-cidr", - initialBridgeAddrs: []string{"192.168.176.88/24"}, - daemonArgs: []string{"--fixed-cidr", "192.168.177.0/24"}, + initialBridgeAddrs: []string{"192.168.176.88/24", "fdd1:8161:2d2c::8888/64"}, + daemonArgs: []string{ + "--fixed-cidr", "192.168.177.0/24", + "--fixed-cidr-v6", "fdd1:8161:2d2c:1::/64", + }, // The bridge's address/subnet should be ignored, this is a change // of fixed-cidr. expIPAMConfig: []network.IPAMConfig{ - { - Subnet: "192.168.177.0/24", - IPRange: "192.168.177.0/24", - // No Gateway is configured, because the address could not be learnt from the - // bridge. An address will have been allocated but, because there's config (the - // fixed-cidr), inspect shows just the config. (Surprisingly, when there's no - // config at all, the inspect output still says its showing config but actually - // shows the running state.) When the daemon is restarted, after a gateway - // address has been assigned to the bridge, that address will become config - so - // a Gateway address will show up in the inspect output. - }, + {Subnet: "192.168.177.0/24", IPRange: "192.168.177.0/24"}, + {Subnet: "fdd1:8161:2d2c:1::/64", IPRange: "fdd1:8161:2d2c:1::/64"}, + // No Gateway is configured, because the address could not be learnt from the + // bridge. An address will have been allocated but, because there's config (the + // fixed-cidr), inspect shows just the config. (Surprisingly, when there's no + // config at all, the inspect output still says its showing config but actually + // shows the running state.) When the daemon is restarted, after a gateway + // address has been assigned to the bridge, that address will become config - so + // a Gateway address will show up in the inspect output. }, }, { name: "old bridge subnet outside fixed-cidr with bip", - initialBridgeAddrs: []string{"192.168.176.88/24"}, - daemonArgs: []string{"--fixed-cidr", "192.168.177.0/24", "--bip", "192.168.177.99/24"}, + initialBridgeAddrs: []string{"192.168.176.88/24", "fdd1:8161:2d2c::8888/64"}, + daemonArgs: []string{ + "--fixed-cidr", "192.168.177.0/24", "--bip", "192.168.177.99/24", + "--fixed-cidr-v6", "fdd1:8161:2d2c:1::/64", "--bip6", "fdd1:8161:2d2c:1::9999/64", + }, expIPAMConfig: []network.IPAMConfig{ - { - Subnet: "192.168.177.0/24", - IPRange: "192.168.177.0/24", - Gateway: "192.168.177.99", - }, + {Subnet: "192.168.177.0/24", IPRange: "192.168.177.0/24", Gateway: "192.168.177.99"}, + {Subnet: "fdd1:8161:2d2c:1::/64", IPRange: "fdd1:8161:2d2c:1::/64", Gateway: "fdd1:8161:2d2c:1::9999"}, }, }, } @@ -167,83 +181,80 @@ func TestDaemonDefaultBridgeIPAM_UserBr(t *testing.T) { testcases := []defaultBridgeIPAMTestCase{ { name: "bridge only", - initialBridgeAddrs: []string{"192.168.176.88/20"}, + initialBridgeAddrs: []string{"192.168.176.88/20", "fdd1:8161:2d2c::8888/64"}, expIPAMConfig: []network.IPAMConfig{ - { - Subnet: "192.168.176.0/20", - Gateway: "192.168.176.88", - }, + {Subnet: "192.168.176.0/20", Gateway: "192.168.176.88"}, + {Subnet: "fdd1:8161:2d2c::/64", Gateway: "fdd1:8161:2d2c::8888"}, }, }, { - name: "fixed-cidr only", - daemonArgs: []string{"--fixed-cidr", "192.168.176.0/24"}, + name: "fixed-cidr only", + daemonArgs: []string{ + "--fixed-cidr", "192.168.176.0/24", + "--fixed-cidr-v6", "fdd1:8161:2d2c::/64", + }, expIPAMConfig: []network.IPAMConfig{ - { - Subnet: "192.168.176.0/24", - IPRange: "192.168.176.0/24", - }, + {Subnet: "192.168.176.0/24", IPRange: "192.168.176.0/24"}, + {Subnet: "fdd1:8161:2d2c::/64", IPRange: "fdd1:8161:2d2c::/64"}, }, }, { - name: "fcidr in bridge subnet and bridge ip in fcidr", - initialBridgeAddrs: []string{"192.168.160.88/20", "192.168.176.88/20", "192.168.192.88/20"}, - daemonArgs: []string{"--fixed-cidr", "192.168.176.0/24"}, + name: "fcidr in bridge subnet and bridge ip in fcidr", + initialBridgeAddrs: []string{ + "192.168.160.88/20", "192.168.176.88/20", "192.168.192.88/20", + "fdd1:8161:2d2c::8888/60", "fdd1:8161:2d2c:10::8888/60", "fdd1:8161:2d2c:20::8888/60", + }, + daemonArgs: []string{ + "--fixed-cidr", "192.168.176.0/24", + "--fixed-cidr-v6", "fdd1:8161:2d2c:10::/64", + }, // Selected bip should be the one within fixed-cidr expIPAMConfig: []network.IPAMConfig{ - { - Subnet: "192.168.176.0/20", - IPRange: "192.168.176.0/24", - Gateway: "192.168.176.88", - }, + {Subnet: "192.168.176.0/20", IPRange: "192.168.176.0/24", Gateway: "192.168.176.88"}, + {Subnet: "fdd1:8161:2d2c:10::/60", IPRange: "fdd1:8161:2d2c:10::/64", Gateway: "fdd1:8161:2d2c:10::8888"}, }, }, { - name: "fcidr in bridge subnet and bridge ip not in fcidr", - initialBridgeAddrs: []string{"192.168.160.88/20", "192.168.176.88/20", "192.168.192.88/20"}, - daemonArgs: []string{"--fixed-cidr", "192.168.177.0/24"}, + name: "fcidr in bridge subnet and bridge ip not in fcidr", + initialBridgeAddrs: []string{ + "192.168.160.88/20", "192.168.176.88/20", "192.168.192.88/20", + "fdd1:8161:2d2c::8888/60", "fdd1:8161:2d2c:10::8888/60", "fdd1:8161:2d2c:20::8888/60", + }, + daemonArgs: []string{ + "--fixed-cidr", "192.168.177.0/24", + "--fixed-cidr-v6", "fdd1:8161:2d2c:11::8888/64", + }, // Selected bridge subnet should be the one that encompasses fixed-cidr. expIPAMConfig: []network.IPAMConfig{ - { - Subnet: "192.168.176.0/20", - IPRange: "192.168.177.0/24", - Gateway: "192.168.176.88", - }, + {Subnet: "192.168.176.0/20", IPRange: "192.168.177.0/24", Gateway: "192.168.176.88"}, + {Subnet: "fdd1:8161:2d2c:10::/60", IPRange: "fdd1:8161:2d2c:11::/64", Gateway: "fdd1:8161:2d2c:10::8888"}, }, }, { name: "fixed-cidr bigger than bridge subnet", initialBridgeAddrs: []string{"192.168.176.88/24"}, daemonArgs: []string{"--fixed-cidr", "192.168.176.0/20"}, + ipv4Only: true, // fixed-cidr (the range of allocatable addresses) is bigger than the // bridge subnet - this is a configuration error, but has historically // been allowed. Because IPRange is treated as an offset into Subnet, it // would normally result in a docker network that allocated addresses // within the selected subnet. So, fixed-cidr is dropped, making the // whole subnet allocatable. - expIPAMConfig: []network.IPAMConfig{ - { - Subnet: "192.168.176.0/24", - Gateway: "192.168.176.88", - }, - }, + expIPAMConfig: []network.IPAMConfig{{Subnet: "192.168.176.0/24", Gateway: "192.168.176.88"}}, }, { name: "no bridge ip within fixed-cidr", - initialBridgeAddrs: []string{"192.168.160.88/20", "192.168.192.88/20"}, + initialBridgeAddrs: []string{"192.168.160.88/20"}, daemonArgs: []string{"--fixed-cidr", "192.168.176.0/24"}, + ipv4Only: true, // fixed-cidr (the range of allocatable addresses) is outside the bridge // subnet - this is a configuration error, but has historically been // allowed. Because IPRange is treated as an offset into Subnet, it // would normally result in a docker network that allocated addresses // within the selected subnet. So, fixed-cidr is dropped, making the // whole subnet allocatable. - expIPAMConfig: []network.IPAMConfig{ - { - Subnet: "192.168.160.0/20", - Gateway: "192.168.160.88", - }, - }, + expIPAMConfig: []network.IPAMConfig{{Subnet: "192.168.160.0/20", Gateway: "192.168.160.88"}}, }, { name: "fixed-cidr contains bridge subnet", @@ -256,12 +267,37 @@ func TestDaemonDefaultBridgeIPAM_UserBr(t *testing.T) { // normally result in a docker network that allocated addresses // within the selected subnet. So, fixed-cidr is dropped, making the // whole subnet allocatable. - expIPAMConfig: []network.IPAMConfig{ - { - Subnet: "192.168.177.0/24", - Gateway: "192.168.177.1", - }, - }, + ipv4Only: true, + expIPAMConfig: []network.IPAMConfig{{Subnet: "192.168.177.0/24", Gateway: "192.168.177.1"}}, + }, + + { + name: "fixed-cidr-v6 bigger than bridge subnet", + initialBridgeAddrs: []string{"fdd1:8161:2d2c::8888/64"}, + daemonArgs: []string{"--fixed-cidr-v6", "fdd1:8161:2d2c::/60"}, + // fixed-cidr-v6 (the range of allocatable addresses) is bigger than the bridge + // subnet - this is a configuration error. Unlike IPv4, it has not historically + // been allowed, so it will prevent daemon startup. + expStartErr: true, + }, + { + name: "no bridge ip within fixed-cidr-v6", + initialBridgeAddrs: []string{"fdd1:8161:2d2c::8888/60"}, + daemonArgs: []string{"--fixed-cidr-v6", "fdd1:8161:2d2c:10::/64"}, + // fixed-cidr-v6 (the range of allocatable addresses) is outside the bridge subnet - + // this is a configuration error. Unlike IPv4, it has not historically been + // allowed, so it will prevent daemon startup. + expStartErr: true, + }, + { + name: "fixed-cidr-v6 contains bridge subnet", + initialBridgeAddrs: []string{"fdd1:8161:2d2c:10::1/64"}, + daemonArgs: []string{"--fixed-cidr-v6", "fdd1:8161:2d2c:10::/60"}, + // fixed-cidr-v6 (the range of allocatable addresses) is bigger than the + // bridge subnet, and the bridge's address is not within fixed-cidr. + // This is a configuration error, Unlike IPv4, it has not historically been + // allowed, so it will prevent daemon startup. + expStartErr: true, }, } for _, tc := range testcases { @@ -275,6 +311,8 @@ type defaultBridgeIPAMTestCase struct { userDefinedBridge bool initialBridgeAddrs []string daemonArgs []string + ipv4Only bool + expStartErr bool expIPAMConfig []network.IPAMConfig } @@ -287,11 +325,14 @@ func testDefaultBridgeIPAM(ctx context.Context, t *testing.T, tc defaultBridgeIP defer deleteInterface(t, bridgeName) var dOpts []daemon.Option - dArgs := tc.daemonArgs + var dArgs []string + if !tc.ipv4Only { + dArgs = append(tc.daemonArgs, "--ipv6") + } if tc.userDefinedBridge { // If a bridge is supplied by the user, the daemon should use its addresses // to infer --bip (which cannot be specified). - dArgs = append(dArgs, []string{"--bridge", bridgeName}...) + dArgs = append(dArgs, "--bridge", bridgeName) } else { // The bridge is created and managed by docker, it's always called "docker0", // unless this test-only env var is set - to avoid conflict with the docker0 @@ -304,7 +345,14 @@ func testDefaultBridgeIPAM(ctx context.Context, t *testing.T, tc defaultBridgeIP d.Stop(t) d.Cleanup(t) }() - d.StartWithBusybox(ctx, t, dArgs...) + + if tc.expStartErr { + err := d.StartWithError(dArgs...) + assert.Check(t, is.ErrorContains(err, "daemon exited during startup")) + return + } + + d.Start(t, dArgs...) c := d.NewClientT(t) defer c.Close() diff --git a/integration/networking/bridge_linux_test.go b/integration/networking/bridge_linux_test.go index da7e748191..0b6d57a265 100644 --- a/integration/networking/bridge_linux_test.go +++ b/integration/networking/bridge_linux_test.go @@ -5,6 +5,7 @@ import ( "flag" "fmt" "net" + "net/netip" "os/exec" "regexp" "strconv" @@ -496,6 +497,9 @@ func TestDefaultBridgeIPv6(t *testing.T) { name string fixed_cidr_v6 string }{ + { + name: "built in ULA prefix", + }, { name: "IPv6 ULA", fixed_cidr_v6: "fd00:1234::/64", @@ -515,10 +519,11 @@ func TestDefaultBridgeIPv6(t *testing.T) { ctx := testutil.StartSpan(ctx, t) d := daemon.New(t) - d.StartWithBusybox(ctx, t, - "--ipv6", - "--fixed-cidr-v6", tc.fixed_cidr_v6, - ) + if tc.fixed_cidr_v6 == "" { + d.StartWithBusybox(ctx, t, "--ipv6") + } else { + d.StartWithBusybox(ctx, t, "--ipv6", "--fixed-cidr-v6", tc.fixed_cidr_v6) + } defer d.Stop(t) c := d.NewClientT(t) @@ -532,15 +537,26 @@ func TestDefaultBridgeIPv6(t *testing.T) { Force: true, }) - networkName := "bridge" + const networkName = "bridge" inspect := container.Inspect(ctx, t, c, cID) - pingHost := inspect.NetworkSettings.Networks[networkName].GlobalIPv6Address + gIPv6 := inspect.NetworkSettings.Networks[networkName].GlobalIPv6Address + + // The container's MAC and IPv6 addresses should be derived from the + // IPAM-allocated IPv4 address. + addr4, err := netip.ParseAddr(inspect.NetworkSettings.Networks[networkName].IPAddress) + assert.NilError(t, err) + mac, err := net.ParseMAC(inspect.NetworkSettings.Networks[networkName].MacAddress) + assert.NilError(t, err) + assert.Check(t, is.DeepEqual(addr4.AsSlice(), []byte(mac)[2:])) + addr6, err := netip.ParseAddr(gIPv6) + assert.NilError(t, err) + assert.Check(t, is.DeepEqual(addr4.AsSlice(), addr6.AsSlice()[12:])) attachCtx, cancel := context.WithTimeout(ctx, 5*time.Second) defer cancel() res := container.RunAttach(attachCtx, t, c, container.WithImage("busybox:latest"), - container.WithCmd("ping", "-c1", "-W3", pingHost), + container.WithCmd("ping", "-c1", "-W3", gIPv6), ) defer c.ContainerRemove(ctx, res.ContainerID, containertypes.RemoveOptions{ Force: true, diff --git a/man/dockerd.8.md b/man/dockerd.8.md index 96c0c9033c..ac48918161 100644 --- a/man/dockerd.8.md +++ b/man/dockerd.8.md @@ -10,6 +10,7 @@ dockerd - Enable daemon mode [**--authorization-plugin**[=*[]*]] [**-b**|**--bridge**[=*BRIDGE*]] [**--bip**[=*BIP*]] +[**--bip6**[=*BIP*]] [**--cgroup-parent**[=*[]*]] [**--config-file**[=*path*]] [**--containerd**[=*SOCKET-PATH*]] @@ -146,7 +147,11 @@ $ sudo dockerd --add-runtime runc=runc --add-runtime custom=/usr/local/bin/my-ru container networking **--bip**="" - Use the provided CIDR notation address for the dynamically created bridge + Use the provided CIDR notation IPv4 address for the dynamically created bridge + (docker0); Mutually exclusive of \-b + +**--bip6**="" + Use the provided CIDR notation IPv6 address for the dynamically created bridge (docker0); Mutually exclusive of \-b **--cgroup-parent**=""