Merge pull request #46976 from robmry/bridge_todos

Validate IPv6 address in libnetwork's bridge driver, remove unused error types.

libnetwork/drivers/bridge/bridge_linux.go

@@ -236,12 +236,21 @@ func (c *networkConfiguration) Validate() error {
 		}
 	}
 
-	// If default v6 gw is specified, AddressIPv6 must be specified and gw must belong to AddressIPv6 subnet
-	if c.EnableIPv6 && c.DefaultGatewayIPv6 != nil {
-		if c.AddressIPv6 == nil || !c.AddressIPv6.Contains(c.DefaultGatewayIPv6) {
+	if c.EnableIPv6 {
+		// If IPv6 is enabled, AddressIPv6 must have been configured.
+		if c.AddressIPv6 == nil {
+			return errdefs.System(errors.New("no IPv6 address was allocated for the bridge"))
+		}
+		// AddressIPv6 must be IPv6, and not overlap with the LL subnet prefix.
+		if err := validateIPv6Subnet(c.AddressIPv6); err != nil {
+			return err
+		}
+		// If a default gw is specified, it must belong to AddressIPv6's subnet
+		if c.DefaultGatewayIPv6 != nil && !c.AddressIPv6.Contains(c.DefaultGatewayIPv6) {
 			return &ErrInvalidGateway{}
 		}
 	}
+
 	return nil
 }
 
@@ -556,13 +565,6 @@ 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
 }
 
@@ -590,11 +592,6 @@ func parseNetworkOptions(id string, option options.Generic) (*networkConfigurati
 		}
 	}
 
-	// Finally validate the configuration
-	if err = config.Validate(); err != nil {
-		return nil, err
-	}
-
 	if config.BridgeName == "" && !config.DefaultBridge {
 		config.BridgeName = "br-" + id[:12]
 	}
@@ -654,16 +651,22 @@ func (d *driver) CreateNetwork(id string, option map[string]interface{}, nInfo d
 	}
 	d.Unlock()
 
-	// Parse and validate the config. It should not be conflict with existing networks' config
+	// Parse the config.
 	config, err := parseNetworkOptions(id, option)
 	if err != nil {
 		return err
 	}
 
+	// Add IP addresses/gateways to the configuration.
 	if err = config.processIPAM(id, ipV4Data, ipV6Data); err != nil {
 		return err
 	}
 
+	// Validate the configuration
+	if err = config.Validate(); err != nil {
+		return err
+	}
+
 	// start the critical section, from this point onward we are dealing with the list of networks
 	// so to be consistent we cannot allow that the list changes
 	d.configNetwork.Lock()
@@ -793,7 +796,7 @@ func (d *driver) createNetwork(config *networkConfiguration) (err error) {
 	// Even if a bridge exists try to setup IPv4.
 	bridgeSetup.queueStep(setupBridgeIPv4)
 
-	enableIPv6Forwarding := d.config.EnableIPForwarding && config.AddressIPv6 != nil
+	enableIPv6Forwarding := config.EnableIPv6 && d.config.EnableIPForwarding
 
 	// Conditionally queue setup steps depending on configuration values.
 	for _, step := range []struct {

libnetwork/drivers/bridge/bridge_linux_test.go

@@ -219,6 +219,15 @@ func getIPv4Data(t *testing.T) []driverapi.IPAMData {
 	return []driverapi.IPAMData{ipd}
 }
 
+func getIPv6Data(t *testing.T) []driverapi.IPAMData {
+	ipd := driverapi.IPAMData{AddressSpace: "full"}
+	// There's no default IPv6 address pool, so use an arbitrary unique-local prefix.
+	addr, nw, _ := net.ParseCIDR("fdcd:d1b1:99d2:abcd::1/64")
+	ipd.Pool = nw
+	ipd.Gateway = &net.IPNet{IP: addr, Mask: nw.Mask}
+	return []driverapi.IPAMData{ipd}
+}
+
 func TestCreateFullOptions(t *testing.T) {
 	defer netnsutils.SetupTestOSContext(t)()
 	d := newDriver()
@@ -255,7 +264,7 @@ func TestCreateFullOptions(t *testing.T) {
 			AuxAddresses: map[string]*net.IPNet{DefaultGatewayV4AuxKey: defgw},
 		},
 	}
-	err := d.CreateNetwork("dummy", netOption, nil, ipdList, nil)
+	err := d.CreateNetwork("dummy", netOption, nil, ipdList, getIPv6Data(t))
 	if err != nil {
 		t.Fatalf("Failed to create bridge: %v", err)
 	}

libnetwork/drivers/bridge/errors.go

@@ -84,16 +84,6 @@ func (eig *ErrInvalidGateway) Error() string {
 // InvalidParameter denotes the type of this error
 func (eig *ErrInvalidGateway) InvalidParameter() {}
 
-// ErrInvalidContainerSubnet is returned when the container subnet (FixedCIDR) is not valid.
-type ErrInvalidContainerSubnet struct{}
-
-func (eis *ErrInvalidContainerSubnet) Error() string {
-	return "container subnet must be a subset of bridge network"
-}
-
-// InvalidParameter denotes the type of this error
-func (eis *ErrInvalidContainerSubnet) InvalidParameter() {}
-
 // ErrInvalidMtu is returned when the user provided MTU is not valid.
 type ErrInvalidMtu int
 
@@ -180,35 +170,6 @@ func (ndbee NonDefaultBridgeNeedsIPError) Error() string {
 // Forbidden denotes the type of this error
 func (ndbee NonDefaultBridgeNeedsIPError) Forbidden() {}
 
-// FixedCIDRv4Error is returned when fixed-cidrv4 configuration
-// failed.
-type FixedCIDRv4Error struct {
-	Net    *net.IPNet
-	Subnet *net.IPNet
-	Err    error
-}
-
-func (fcv4 *FixedCIDRv4Error) Error() string {
-	return fmt.Sprintf("setup FixedCIDRv4 failed for subnet %s in %s: %v", fcv4.Subnet, fcv4.Net, fcv4.Err)
-}
-
-// InternalError denotes the type of this error
-func (fcv4 *FixedCIDRv4Error) InternalError() {}
-
-// FixedCIDRv6Error is returned when fixed-cidrv6 configuration
-// failed.
-type FixedCIDRv6Error struct {
-	Net *net.IPNet
-	Err error
-}
-
-func (fcv6 *FixedCIDRv6Error) Error() string {
-	return fmt.Sprintf("setup FixedCIDRv6 failed for subnet %s in %s: %v", fcv6.Net, fcv6.Net, fcv6.Err)
-}
-
-// InternalError denotes the type of this error
-func (fcv6 *FixedCIDRv6Error) InternalError() {}
-
 // IPv4AddrAddError is returned when IPv4 address could not be added to the bridge.
 type IPv4AddrAddError struct {
 	IP  *net.IPNet

libnetwork/drivers/bridge/interface_linux.go

@@ -70,39 +70,28 @@ func (i *bridgeInterface) addresses(family int) ([]netlink.Addr, error) {
 	return addrs, nil
 }
 
-func getRequiredIPv6Addrs(config *networkConfiguration) (requiredAddrs map[netip.Addr]netip.Prefix, addr *net.IPNet, gateway net.IP, err error) {
+func getRequiredIPv6Addrs(config *networkConfiguration) (requiredAddrs map[netip.Addr]netip.Prefix, err error) {
 	requiredAddrs = make(map[netip.Addr]netip.Prefix)
 
-	// TODO(robmry) - is config.AddressIPv6 always set at this point?
-	//   The logic here is preserved from the original setupBridgeIPv6(), but
-	//   can probably be simplified.
-
 	// Always give the bridge 'fe80::1' - every interface is required to have an
 	// address in 'fe80::/64'. Linux may assign an address, but we'll replace it with
 	// 'fe80::1'. Then, if the configured prefix is 'fe80::/64', the IPAM pool
 	// assigned address will not be a second address in the LL subnet.
-	addr = bridgeIPv6
-	gateway = bridgeIPv6.IP
 	ra, ok := netiputil.ToPrefix(bridgeIPv6)
 	if !ok {
 		err = fmt.Errorf("Failed to convert Link-Local IPv6 address to netip.Prefix")
-		return nil, nil, net.IP{}, err
+		return nil, err
 	}
 	requiredAddrs[ra.Addr()] = ra
 
-	// Set up the user-specified bridge address and gateway.
-	if config.AddressIPv6 != nil {
-		addr = config.AddressIPv6
-		gateway = config.AddressIPv6.IP
-		ra, ok := netiputil.ToPrefix(config.AddressIPv6)
-		if !ok {
-			err = fmt.Errorf("failed to convert bridge IPv6 address '%s' to netip.Prefix", config.AddressIPv6.String())
-			return nil, nil, net.IP{}, err
-		}
-		requiredAddrs[ra.Addr()] = ra
+	ra, ok = netiputil.ToPrefix(config.AddressIPv6)
+	if !ok {
+		err = fmt.Errorf("failed to convert bridge IPv6 address '%s' to netip.Prefix", config.AddressIPv6.String())
+		return nil, err
 	}
+	requiredAddrs[ra.Addr()] = ra
 
-	return requiredAddrs, addr, gateway, nil
+	return requiredAddrs, nil
 }
 
 func (i *bridgeInterface) programIPv6Addresses(config *networkConfiguration) error {
@@ -114,10 +103,12 @@ func (i *bridgeInterface) programIPv6Addresses(config *networkConfiguration) err
 
 	// Get the list of required IPv6 addresses for this bridge.
 	var requiredAddrs map[netip.Addr]netip.Prefix
-	requiredAddrs, i.bridgeIPv6, i.gatewayIPv6, err = getRequiredIPv6Addrs(config)
+	requiredAddrs, err = getRequiredIPv6Addrs(config)
 	if err != nil {
 		return errdefs.System(err)
 	}
+	i.bridgeIPv6 = config.AddressIPv6
+	i.gatewayIPv6 = config.AddressIPv6.IP
 
 	// Remove addresses that aren't required.
 	for _, existingAddr := range existingAddrs {

libnetwork/drivers/bridge/interface_linux_test.go

@@ -130,10 +130,8 @@ func TestGetRequiredIPv6Addrs(t *testing.T) {
 				expResult[netip.MustParseAddr(strings.Split(addr, "/")[0])] = netip.MustParsePrefix(addr)
 			}
 
-			reqd, addr, gw, err := getRequiredIPv6Addrs(config)
+			reqd, err := getRequiredIPv6Addrs(config)
 			assert.Check(t, is.Nil(err))
-			assert.Check(t, is.DeepEqual(addr, config.AddressIPv6))
-			assert.Check(t, is.DeepEqual(gw, config.AddressIPv6.IP))
 			assert.Check(t, is.DeepEqual(reqd, expResult,
 				cmp.Comparer(func(a, b netip.Prefix) bool { return a == b })))
 		})
@@ -143,7 +141,7 @@ func TestGetRequiredIPv6Addrs(t *testing.T) {
 func TestProgramIPv6Addresses(t *testing.T) {
 	defer netnsutils.SetupTestOSContext(t)()
 
-	checkAddrs := func(i *bridgeInterface, expAddrs []string) {
+	checkAddrs := func(i *bridgeInterface, nc *networkConfiguration, expAddrs []string) {
 		t.Helper()
 		exp := []netlink.Addr{}
 		for _, a := range expAddrs {
@@ -153,6 +151,8 @@ func TestProgramIPv6Addresses(t *testing.T) {
 		actual, err := i.addresses(netlink.FAMILY_V6)
 		assert.NilError(t, err)
 		assert.DeepEqual(t, exp, actual)
+		assert.Check(t, is.DeepEqual(i.bridgeIPv6, nc.AddressIPv6))
+		assert.Check(t, is.DeepEqual(i.gatewayIPv6, nc.AddressIPv6.IP))
 	}
 
 	nc := &networkConfiguration{}
@@ -163,7 +163,7 @@ func TestProgramIPv6Addresses(t *testing.T) {
 	nc.AddressIPv6 = cidrToIPNet(t, "2000:3000::1/64")
 	err := i.programIPv6Addresses(nc)
 	assert.NilError(t, err)
-	checkAddrs(i, []string{"2000:3000::1/64", "fe80::1/64"})
+	checkAddrs(i, nc, []string{"2000:3000::1/64", "fe80::1/64"})
 
 	// Shrink the subnet of that regular address, the prefix length of the address
 	// will not be modified - but it's informational-only, the address itself has
@@ -171,18 +171,18 @@ func TestProgramIPv6Addresses(t *testing.T) {
 	nc.AddressIPv6 = cidrToIPNet(t, "2000:3000::1/80")
 	err = i.programIPv6Addresses(nc)
 	assert.NilError(t, err)
-	checkAddrs(i, []string{"2000:3000::1/64", "fe80::1/64"})
+	checkAddrs(i, nc, []string{"2000:3000::1/64", "fe80::1/64"})
 
 	// Ask for link-local only, by specifying an address with the Link Local prefix.
 	// The regular address should be removed.
 	nc.AddressIPv6 = cidrToIPNet(t, "fe80::1/64")
 	err = i.programIPv6Addresses(nc)
 	assert.NilError(t, err)
-	checkAddrs(i, []string{"fe80::1/64"})
+	checkAddrs(i, nc, []string{"fe80::1/64"})
 
 	// Swap the standard link local address for a nonstandard one.
 	nc.AddressIPv6 = cidrToIPNet(t, "fe80:5555::1/55")
 	err = i.programIPv6Addresses(nc)
 	assert.NilError(t, err)
-	checkAddrs(i, []string{"fe80:5555::1/55", "fe80::1/64"})
+	checkAddrs(i, nc, []string{"fe80:5555::1/55", "fe80::1/64"})
 }

libnetwork/drivers/bridge/network_linux_test.go

@@ -27,7 +27,7 @@ func TestLinkCreate(t *testing.T) {
 	genericOption[netlabel.GenericData] = config
 
 	ipdList := getIPv4Data(t)
-	err := d.CreateNetwork("dummy", genericOption, nil, ipdList, nil)
+	err := d.CreateNetwork("dummy", genericOption, nil, ipdList, getIPv6Data(t))
 	if err != nil {
 		t.Fatalf("Failed to create bridge: %v", err)
 	}
@@ -120,7 +120,7 @@ func TestLinkCreateTwo(t *testing.T) {
 	genericOption[netlabel.GenericData] = config
 
 	ipdList := getIPv4Data(t)
-	err := d.CreateNetwork("dummy", genericOption, nil, ipdList, nil)
+	err := d.CreateNetwork("dummy", genericOption, nil, ipdList, getIPv6Data(t))
 	if err != nil {
 		t.Fatalf("Failed to create bridge: %v", err)
 	}
@@ -157,7 +157,7 @@ func TestLinkCreateNoEnableIPv6(t *testing.T) {
 	genericOption[netlabel.GenericData] = config
 
 	ipdList := getIPv4Data(t)
-	err := d.CreateNetwork("dummy", genericOption, nil, ipdList, nil)
+	err := d.CreateNetwork("dummy", genericOption, nil, ipdList, getIPv6Data(t))
 	if err != nil {
 		t.Fatalf("Failed to create bridge: %v", err)
 	}
@@ -193,7 +193,7 @@ func TestLinkDelete(t *testing.T) {
 	genericOption[netlabel.GenericData] = config
 
 	ipdList := getIPv4Data(t)
-	err := d.CreateNetwork("dummy", genericOption, nil, ipdList, nil)
+	err := d.CreateNetwork("dummy", genericOption, nil, ipdList, getIPv6Data(t))
 	if err != nil {
 		t.Fatalf("Failed to create bridge: %v", err)
 	}

libnetwork/drivers/bridge/port_mapping_linux_test.go

@@ -37,13 +37,13 @@ func TestPortMappingConfig(t *testing.T) {
 	netOptions := make(map[string]interface{})
 	netOptions[netlabel.GenericData] = netConfig
 
-	ipdList := getIPv4Data(t)
-	err := d.CreateNetwork("dummy", netOptions, nil, ipdList, nil)
+	ipdList4 := getIPv4Data(t)
+	err := d.CreateNetwork("dummy", netOptions, nil, ipdList4, getIPv6Data(t))
 	if err != nil {
 		t.Fatalf("Failed to create bridge: %v", err)
 	}
 
-	te := newTestEndpoint(ipdList[0].Pool, 11)
+	te := newTestEndpoint(ipdList4[0].Pool, 11)
 	err = d.CreateEndpoint("dummy", "ep1", te.Interface(), nil)
 	if err != nil {
 		t.Fatalf("Failed to create the endpoint: %s", err.Error())
@@ -122,13 +122,13 @@ func TestPortMappingV6Config(t *testing.T) {
 	netOptions := make(map[string]interface{})
 	netOptions[netlabel.GenericData] = netConfig
 
-	ipdList := getIPv4Data(t)
-	err := d.CreateNetwork("dummy", netOptions, nil, ipdList, nil)
+	ipdList4 := getIPv4Data(t)
+	err := d.CreateNetwork("dummy", netOptions, nil, ipdList4, getIPv6Data(t))
 	if err != nil {
 		t.Fatalf("Failed to create bridge: %v", err)
 	}
 
-	te := newTestEndpoint(ipdList[0].Pool, 11)
+	te := newTestEndpoint(ipdList4[0].Pool, 11)
 	err = d.CreateEndpoint("dummy", "ep1", te.Interface(), nil)
 	if err != nil {
 		t.Fatalf("Failed to create the endpoint: %s", err.Error())

libnetwork/drivers/bridge/setup_ipv6_linux.go

@@ -53,9 +53,6 @@ func setupBridgeIPv6(config *networkConfiguration, i *bridgeInterface) error {
 }
 
 func setupGatewayIPv6(config *networkConfiguration, i *bridgeInterface) error {
-	if config.AddressIPv6 == nil {
-		return &ErrInvalidContainerSubnet{}
-	}
 	if !config.AddressIPv6.Contains(config.DefaultGatewayIPv6) {
 		return &ErrInvalidGateway{}
 	}

libnetwork/drivers/bridge/setup_ipv6_linux_test.go

@@ -21,6 +21,8 @@ func TestSetupIPv6(t *testing.T) {
 	defer nh.Close()
 
 	config, br := setupTestInterface(t, nh)
+	addr, nw, _ := net.ParseCIDR("fdcc:949:6399:1234::1/64")
+	config.AddressIPv6 = &net.IPNet{IP: addr, Mask: nw.Mask}
 	if err := setupBridgeIPv6(config, br); err != nil {
 		t.Fatalf("Failed to setup bridge IPv6: %v", err)
 	}