Merge pull request moby#46850 from robmry/46829-allow_ipv6_subnet_change

Allow overlapping change in bridge's IPv6 network.

daemon/config/config_linux.go

@@ -9,6 +9,7 @@ import (
 	"github.com/containerd/cgroups/v3"
 	"github.com/docker/docker/api/types/container"
 	"github.com/docker/docker/api/types/system"
+	"github.com/docker/docker/libnetwork/drivers/bridge"
 	"github.com/docker/docker/opts"
 	"github.com/docker/docker/pkg/homedir"
 	"github.com/docker/docker/pkg/rootless"
@@ -178,6 +179,10 @@ func (conf *Config) ValidatePlatformConfig() error {
 		return err
 	}
 
+	if err := bridge.ValidateFixedCIDRV6(conf.FixedCIDRv6); err != nil {
+		return errors.Wrap(err, "invalid fixed-cidr-v6")
+	}
+
 	return verifyDefaultCgroupNsMode(conf.CgroupNamespaceMode)
 }
 

integration/networking/bridge_test.go

@@ -86,6 +86,32 @@ func TestBridgeICC(t *testing.T) {
 			},
 			linkLocal: true,
 		},
+		{
+			// As for 'LL non-internal', but ping the container by name instead of by address
+			// - the busybox test containers only have one interface with a link local
+			// address, so the zone index is not required:
+			//   RFC-4007, section 6: "[...] for nodes with only a single non-loopback
+			//   interface (e.g., a single Ethernet interface), the common case, link-local
+			//   addresses need not be qualified with a zone index."
+			// So, for this common case, LL addresses should be included in DNS config.
+			name: "IPv6 link-local address on non-internal network ping by name",
+			bridgeOpts: []func(*types.NetworkCreate){
+				network.WithIPv6(),
+				network.WithIPAM("fe80::/64", "fe80::1"),
+			},
+		},
+		{
+			name: "IPv6 nonstandard link-local subnet on non-internal network ping by name",
+			// No interfaces apart from the one on the bridge network with this non-default
+			// subnet will be on this link local subnet (it's not currently possible to
+			// configure two networks with the same LL subnet, although perhaps it should
+			// be). So, again, no zone index is required and the LL address should be
+			// included in DNS config.
+			bridgeOpts: []func(*types.NetworkCreate){
+				network.WithIPv6(),
+				network.WithIPAM("fe80:1234::/64", "fe80:1234::1"),
+			},
+		},
 		{
 			name: "IPv6 non-internal network with SLAAC LL address",
 			bridgeOpts: []func(*types.NetworkCreate){
@@ -292,3 +318,162 @@ func TestBridgeINC(t *testing.T) {
 		})
 	}
 }
+
+func TestDefaultBridgeIPv6(t *testing.T) {
+	skip.If(t, testEnv.DaemonInfo.OSType == "windows")
+
+	ctx := setupTest(t)
+
+	testcases := []struct {
+		name          string
+		fixed_cidr_v6 string
+	}{
+		{
+			name:          "IPv6 ULA",
+			fixed_cidr_v6: "fd00:1234::/64",
+		},
+		{
+			name:          "IPv6 LLA only",
+			fixed_cidr_v6: "fe80::/64",
+		},
+		{
+			name:          "IPv6 nonstandard LLA only",
+			fixed_cidr_v6: "fe80:1234::/64",
+		},
+	}
+
+	for _, tc := range testcases {
+		t.Run(tc.name, func(t *testing.T) {
+			ctx := testutil.StartSpan(ctx, t)
+
+			d := daemon.New(t)
+			d.StartWithBusybox(ctx, t,
+				"--experimental",
+				"--ip6tables",
+				"--ipv6",
+				"--fixed-cidr-v6", tc.fixed_cidr_v6,
+			)
+			defer d.Stop(t)
+
+			c := d.NewClientT(t)
+			defer c.Close()
+
+			cID := container.Run(ctx, t, c,
+				container.WithImage("busybox:latest"),
+				container.WithCmd("top"),
+			)
+			defer c.ContainerRemove(ctx, cID, containertypes.RemoveOptions{
+				Force: true,
+			})
+
+			networkName := "bridge"
+			inspect := container.Inspect(ctx, t, c, cID)
+			pingHost := inspect.NetworkSettings.Networks[networkName].GlobalIPv6Address
+
+			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),
+			)
+			defer c.ContainerRemove(ctx, res.ContainerID, containertypes.RemoveOptions{
+				Force: true,
+			})
+
+			assert.Check(t, is.Equal(res.ExitCode, 0))
+			assert.Check(t, is.Equal(res.Stderr.String(), ""))
+			assert.Check(t, is.Contains(res.Stdout.String(), "1 packets transmitted, 1 packets received"))
+		})
+	}
+}
+
+// Check that it's possible to change 'fixed-cidr-v6' and restart the daemon.
+func TestDefaultBridgeAddresses(t *testing.T) {
+	skip.If(t, testEnv.DaemonInfo.OSType == "windows")
+
+	ctx := setupTest(t)
+	d := daemon.New(t)
+
+	type testStep struct {
+		stepName    string
+		fixedCIDRV6 string
+		expAddrs    []string
+	}
+
+	testcases := []struct {
+		name  string
+		steps []testStep
+	}{
+		{
+			name: "Unique-Local Subnet Changes",
+			steps: []testStep{
+				{
+					stepName:    "Set up initial UL prefix",
+					fixedCIDRV6: "fd1c:f1a0:5d8d:aaaa::/64",
+					expAddrs:    []string{"fd1c:f1a0:5d8d:aaaa::1/64", "fe80::1/64"},
+				},
+				{
+					// Modify that prefix, the default bridge's address must be deleted and re-added.
+					stepName:    "Modify UL prefix - address change",
+					fixedCIDRV6: "fd1c:f1a0:5d8d:bbbb::/64",
+					expAddrs:    []string{"fd1c:f1a0:5d8d:bbbb::1/64", "fe80::1/64"},
+				},
+				{
+					// Modify the prefix length, the default bridge's address should not change.
+					stepName:    "Modify UL prefix - no address change",
+					fixedCIDRV6: "fd1c:f1a0:5d8d:bbbb::/80",
+					// The prefix length displayed by 'ip a' is not updated - it's informational, and
+					// can't be changed without unnecessarily deleting and re-adding the address.
+					expAddrs: []string{"fd1c:f1a0:5d8d:bbbb::1/64", "fe80::1/64"},
+				},
+			},
+		},
+		{
+			name: "Link-Local Subnet Changes",
+			steps: []testStep{
+				{
+					stepName:    "Standard LL subnet prefix",
+					fixedCIDRV6: "fe80::/64",
+					expAddrs:    []string{"fe80::1/64"},
+				},
+				{
+					// Modify that prefix, the default bridge's address must be deleted and re-added.
+					// The bridge must still have an address in the required (standard) LL subnet.
+					stepName:    "Nonstandard LL prefix - address change",
+					fixedCIDRV6: "fe80:1234::/32",
+					expAddrs:    []string{"fe80:1234::1/32", "fe80::1/64"},
+				},
+				{
+					// Modify the prefix length, the addresses should not change.
+					stepName:    "Modify LL prefix - no address change",
+					fixedCIDRV6: "fe80:1234::/64",
+					// The prefix length displayed by 'ip a' is not updated - it's informational, and
+					// can't be changed without unnecessarily deleting and re-adding the address.
+					expAddrs: []string{"fe80:1234::1/", "fe80::1/64"},
+				},
+			},
+		},
+	}
+
+	for _, tc := range testcases {
+		t.Run(tc.name, func(t *testing.T) {
+			for _, step := range tc.steps {
+				// Check that the daemon starts - regression test for:
+				//   https://github.com/moby/moby/issues/46829
+				d.Start(t, "--experimental", "--ipv6", "--ip6tables", "--fixed-cidr-v6="+step.fixedCIDRV6)
+				d.Stop(t)
+
+				// Check that the expected addresses have been applied to the bridge. (Skip in
+				// rootless mode, because the bridge is in a different network namespace.)
+				if !testEnv.IsRootless() {
+					res := testutil.RunCommand(ctx, "ip", "-6", "addr", "show", "docker0")
+					assert.Equal(t, res.ExitCode, 0, step.stepName)
+					stdout := res.Stdout()
+					for _, expAddr := range step.expAddrs {
+						assert.Check(t, is.Contains(stdout, expAddr))
+					}
+				}
+			}
+		})
+	}
+}

libnetwork/drivers/bridge/bridge_linux.go

@@ -1,8 +1,8 @@
 package bridge
 
 import (
+	"bytes"
 	"context"
-	"errors"
 	"fmt"
 	"net"
 	"os"
@@ -11,6 +11,7 @@ import (
 	"sync"
 
 	"github.com/containerd/log"
+	"github.com/docker/docker/errdefs"
 	"github.com/docker/docker/libnetwork/datastore"
 	"github.com/docker/docker/libnetwork/driverapi"
 	"github.com/docker/docker/libnetwork/iptables"
@@ -22,6 +23,7 @@ import (
 	"github.com/docker/docker/libnetwork/portmapper"
 	"github.com/docker/docker/libnetwork/scope"
 	"github.com/docker/docker/libnetwork/types"
+	"github.com/pkg/errors"
 	"github.com/vishvananda/netlink"
 )
 
@@ -179,6 +181,44 @@ func Register(r driverapi.Registerer, config map[string]interface{}) error {
 	})
 }
 
+// The behaviour of previous implementations of bridge subnet prefix assignment
+// is preserved here...
+//
+// The LL prefix, 'fe80::/64' can be used as an IPAM pool. Linux always assigns
+// link-local addresses with this prefix. But, pool-assigned addresses are very
+// unlikely to conflict.
+//
+// Don't allow a nonstandard LL subnet to overlap with 'fe80::/64'. For example,
+// if the config asked for subnet prefix 'fe80::/80', the bridge and its
+// containers would each end up with two LL addresses, Linux's '/64' and one from
+// the IPAM pool claiming '/80'. Although the specified prefix length must not
+// affect the host's determination of whether the address is on-link and to be
+// added to the interface's Prefix List (RFC-5942), differing prefix lengths
+// would be confusing and have been disallowed by earlier implementations of
+// bridge address assignment.
+func validateIPv6Subnet(addr *net.IPNet) error {
+	if addr != nil && bridgeIPv6.Contains(addr.IP) && !bytes.Equal(bridgeIPv6.Mask, addr.Mask) {
+		return errdefs.InvalidParameter(errors.New("clash with the Link-Local prefix 'fe80::/64'"))
+	}
+	return nil
+}
+
+// ValidateFixedCIDRV6 checks that val is an IPv6 address and prefix length that
+// does not overlap with the link local subnet prefix 'fe80::/64'.
+func ValidateFixedCIDRV6(val string) error {
+	if val == "" {
+		return nil
+	}
+	ip, ipNet, err := net.ParseCIDR(val)
+	if err != nil {
+		return errdefs.InvalidParameter(err)
+	}
+	if ip.To4() != nil {
+		return errdefs.InvalidParameter(errors.New("fixed-cidr-v6 is not an IPv6 subnet"))
+	}
+	return validateIPv6Subnet(ipNet)
+}
+
 // Validate performs a static validation on the network configuration parameters.
 // Whatever can be assessed a priori before attempting any programming.
 func (c *networkConfiguration) Validate() error {
@@ -516,6 +556,13 @@ func (c *networkConfiguration) processIPAM(id string, ipamV4Data, ipamV6Data []d
 		}
 	}
 
+	// TODO(robmry) - move this to networkConfiguration.Validate()
+	//   - but that can't happen until Validate() is called after processIPAM() has set
+	//     up the IP addresses, instead of during parseNetworkOptions().
+	if err := validateIPv6Subnet(c.AddressIPv6); err != nil {
+		return err
+	}
+
 	return nil
 }
 
@@ -759,9 +806,9 @@ func (d *driver) createNetwork(config *networkConfiguration) (err error) {
 		// assigned an IPv6 link-local address.
 		{config.EnableIPv6, setupBridgeIPv6},
 
-		// We ensure that the bridge has the expectedIPv4 and IPv6 addresses in
-		// the case of a previously existing device.
-		{bridgeAlreadyExists && !config.InhibitIPv4, setupVerifyAndReconcile},
+		// Ensure the bridge has the expected IPv4 addresses in the case of a previously
+		// existing device.
+		{bridgeAlreadyExists && !config.InhibitIPv4, setupVerifyAndReconcileIPv4},
 
 		// Enable IPv6 Forwarding
 		{enableIPv6Forwarding, setupIPv6Forwarding},
@@ -1059,7 +1106,6 @@ func (d *driver) CreateEndpoint(nid, eid string, ifInfo driverapi.InterfaceInfo,
 		if config.AddressIPv6 != nil {
 			network = config.AddressIPv6
 		}
-
 		ones, _ := network.Mask.Size()
 		if ones > 80 {
 			err = types.ForbiddenErrorf("Cannot self generate an IPv6 address on network %v: At least 48 host bits are needed.", network)