daemon: Improve NetworkingConfig & EndpointSettings validation

So far, only a subset of NetworkingConfig was validated when calling
ContainerCreate. Other parameters would be validated when the container
was started. And the same goes for EndpointSettings on NetworkConnect.

This commit adds two validation steps:

1. Check if the IP addresses set in endpoint's IPAMConfig are valid,
   when ContainerCreate and ConnectToNetwork is called ;
2. Check if the network allows static IP addresses, only on
   ConnectToNetwork as we need the libnetwork's Network for that and it
   might not exist until NetworkAttachment requests are sent to the
   Swarm leader (which happens only when starting the container) ;

Signed-off-by: Albin Kerouanton <albinker@gmail.com>

api/swagger.yaml

@@ -10038,6 +10038,10 @@ paths:
       responses:
         200:
           description: "No error"
+        400:
+          description: "bad parameter"
+          schema:
+            $ref: "#/definitions/ErrorResponse"
         403:
           description: "Operation not supported for swarm scoped networks"
           schema:

daemon/container_operations.go

@@ -562,39 +562,56 @@ func (daemon *Daemon) allocateNetwork(cfg *config.Config, container *container.C
 	return nil
 }
 
-// hasUserDefinedIPAddress returns whether the passed IPAM configuration contains IP address configuration
-func hasUserDefinedIPAddress(ipamConfig *networktypes.EndpointIPAMConfig) bool {
-	return ipamConfig != nil && (len(ipamConfig.IPv4Address) > 0 || len(ipamConfig.IPv6Address) > 0)
-}
-
-// User specified ip address is acceptable only for networks with user specified subnets.
-func validateNetworkingConfig(n *libnetwork.Network, epConfig *networktypes.EndpointSettings) error {
-	if n == nil || epConfig == nil {
+// validateEndpointSettings checks whether the given epConfig is valid. The nw parameter might be nil as a container
+// can be created with a reference to a network that don't exist yet. In that case, only partial validation will be
+// done.
+func validateEndpointSettings(nw *libnetwork.Network, nwName string, epConfig *networktypes.EndpointSettings) error {
+	if epConfig == nil {
 		return nil
 	}
-	if !containertypes.NetworkMode(n.Name()).IsUserDefined() {
-		if hasUserDefinedIPAddress(epConfig.IPAMConfig) && !enableIPOnPredefinedNetwork() {
+
+	ipamConfig := &networktypes.EndpointIPAMConfig{}
+	if epConfig.IPAMConfig != nil {
+		ipamConfig = epConfig.IPAMConfig
+	}
+
+	if !containertypes.NetworkMode(nwName).IsUserDefined() {
+		hasStaticAddresses := ipamConfig.IPv4Address != "" || ipamConfig.IPv6Address != ""
+		// On Linux, user specified IP address is accepted only by networks with user specified subnets.
+		if hasStaticAddresses && !enableIPOnPredefinedNetwork() {
 			return runconfig.ErrUnsupportedNetworkAndIP
 		}
 		if len(epConfig.Aliases) > 0 && !serviceDiscoveryOnDefaultNetwork() {
 			return runconfig.ErrUnsupportedNetworkAndAlias
 		}
 	}
-	if !hasUserDefinedIPAddress(epConfig.IPAMConfig) {
+
+	if ipamConfig.IPv4Address != "" {
+		if addr := net.ParseIP(ipamConfig.IPv4Address); addr == nil || addr.To4() == nil || addr.IsUnspecified() {
+			return fmt.Errorf("invalid IPv4 address: %s", ipamConfig.IPv4Address)
+		}
+	}
+	if ipamConfig.IPv6Address != "" {
+		if addr := net.ParseIP(ipamConfig.IPv6Address); addr == nil || addr.To4() != nil || addr.IsUnspecified() {
+			return fmt.Errorf("invalid IPv6 address: %s", ipamConfig.IPv6Address)
+		}
+	}
+
+	if nw == nil {
 		return nil
 	}
 
-	_, _, nwIPv4Configs, nwIPv6Configs := n.IpamConfig()
+	_, _, nwIPv4Configs, nwIPv6Configs := nw.IpamConfig()
 	for _, s := range []struct {
 		ipConfigured  bool
 		subnetConfigs []*libnetwork.IpamConf
 	}{
 		{
-			ipConfigured:  len(epConfig.IPAMConfig.IPv4Address) > 0,
+			ipConfigured:  len(ipamConfig.IPv4Address) > 0,
 			subnetConfigs: nwIPv4Configs,
 		},
 		{
-			ipConfigured:  len(epConfig.IPAMConfig.IPv6Address) > 0,
+			ipConfigured:  len(ipamConfig.IPv6Address) > 0,
 			subnetConfigs: nwIPv6Configs,
 		},
 	} {
@@ -657,7 +674,7 @@ func (daemon *Daemon) updateNetworkConfig(container *container.Container, n *lib
 		}
 	}
 
-	if err := validateNetworkingConfig(n, endpointConfig); err != nil {
+	if err := validateEndpointSettings(n, n.Name(), endpointConfig); err != nil {
 		return err
 	}
 

daemon/container_operations_unix.go

@@ -367,11 +367,13 @@ func isLinkable(child *container.Container) bool {
 	return ok
 }
 
+// TODO(aker): remove when we make the default bridge network behave like any other network
 func enableIPOnPredefinedNetwork() bool {
 	return false
 }
 
 // serviceDiscoveryOnDefaultNetwork indicates if service discovery is supported on the default network
+// TODO(aker): remove when we make the default bridge network behave like any other network
 func serviceDiscoveryOnDefaultNetwork() bool {
 	return false
 }

daemon/create.go

@@ -4,7 +4,6 @@ import (
 	"context"
 	"errors"
 	"fmt"
-	"net"
 	"runtime"
 	"strings"
 	"time"
@@ -98,7 +97,7 @@ func (daemon *Daemon) containerCreate(ctx context.Context, daemonCfg *configStor
 		}
 	}
 
-	err = verifyNetworkingConfig(opts.params.NetworkingConfig)
+	err = daemon.validateNetworkingConfig(opts.params.NetworkingConfig)
 	if err != nil {
 		return containertypes.CreateResponse{Warnings: warnings}, errdefs.InvalidParameter(err)
 	}
@@ -321,8 +320,8 @@ func (daemon *Daemon) mergeAndVerifyConfig(config *containertypes.Config, img *i
 	return nil
 }
 
-// verifyNetworkingConfig validates if the nwConfig is valid.
-func verifyNetworkingConfig(nwConfig *networktypes.NetworkingConfig) error {
+// validateNetworkingConfig checks whether a container's NetworkingConfig is valid.
+func (daemon *Daemon) validateNetworkingConfig(nwConfig *networktypes.NetworkingConfig) error {
 	if nwConfig == nil {
 		return nil
 	}
@@ -331,20 +330,14 @@ func verifyNetworkingConfig(nwConfig *networktypes.NetworkingConfig) error {
 		if v == nil {
 			return fmt.Errorf("no EndpointSettings for %s", k)
 		}
-		if v.IPAMConfig != nil {
-			if v.IPAMConfig.IPv4Address != "" && net.ParseIP(v.IPAMConfig.IPv4Address).To4() == nil {
-				return fmt.Errorf("invalid IPv4 address: %s", v.IPAMConfig.IPv4Address)
-			}
-			if v.IPAMConfig.IPv6Address != "" {
-				n := net.ParseIP(v.IPAMConfig.IPv6Address)
-				// if the address is an invalid network address (ParseIP == nil) or if it is
-				// an IPv4 address (To4() != nil), then it is an invalid IPv6 address
-				if n == nil || n.To4() != nil {
-					return fmt.Errorf("invalid IPv6 address: %s", v.IPAMConfig.IPv6Address)
-				}
-			}
+
+		// The referenced network k might not exist when the container is created, so just ignore the error in that case.
+		nw, _ := daemon.FindNetwork(k)
+		if err := validateEndpointSettings(nw, k, v); err != nil {
+			return err
 		}
 	}
+
 	return nil
 }
 

daemon/create_test.go

@@ -1,21 +0,0 @@
-package daemon // import "github.com/docker/docker/daemon"
-
-import (
-	"testing"
-
-	"github.com/docker/docker/api/types/network"
-	"gotest.tools/v3/assert"
-	is "gotest.tools/v3/assert/cmp"
-)
-
-// Test case for 35752
-func TestVerifyNetworkingConfig(t *testing.T) {
-	name := "mynet"
-	endpoints := make(map[string]*network.EndpointSettings, 1)
-	endpoints[name] = nil
-	nwConfig := &network.NetworkingConfig{
-		EndpointsConfig: endpoints,
-	}
-	err := verifyNetworkingConfig(nwConfig)
-	assert.Check(t, is.Error(err, "no EndpointSettings for mynet"), "should produce an error because no EndpointSettings were passed")
-}

docs/api/version-history.md

@@ -47,6 +47,9 @@ keywords: "API, Docker, rcli, REST, documentation"
   1.44.
 * `POST /containers/create` now accepts multiple `EndpointSettings` in
   `NetworkingConfig.EndpointSettings`.
+* `POST /containers/create` and `POST /networks/{id}/connect` will now catch
+  validation errors that were previously only returned during `POST /containers/{id}/start`.
+  Note that this change is _unversioned_ and applies to all API versions.
 
 ## v1.43 API changes