Push network spec validation down into network.Model

Also add a SetDefaults which avoids updating the spec in the validation function in the CNI case. With this all uses of libnetwork are in manager/allocator/cnmallocator and manager/network/cnm/cnm.go while all uses of CNI are in manager/network/cni/cni.go and agent/exec/containerd/adapter.go. I'd still like to abstract the containerd/adaptor.go case further. Signed-off-by: Ian Campbell <[email protected]>
moby · Jul 6, 2017 · 9abaffe · 9abaffe
1 parent a522f8d
commit 9abaffe
Show file tree

Hide file tree

Showing 4 changed files with 131 additions and 105 deletions.
diff --git a/manager/controlapi/network.go b/manager/controlapi/network.go
@@ -1,11 +1,6 @@
 package controlapi
 
 import (
-	"net"
-
-	"github.com/containernetworking/cni/libcni"
-	"github.com/docker/libnetwork/driverapi"
-	"github.com/docker/libnetwork/ipamapi"
 	"github.com/docker/swarmkit/api"
 	"github.com/docker/swarmkit/identity"
 	"github.com/docker/swarmkit/manager/allocator"
@@ -17,95 +12,13 @@ import (
 	"google.golang.org/grpc/codes"
 )
 
-func validateIPAMConfiguration(ipamConf *api.IPAMConfig) error {
-	if ipamConf == nil {
-		return grpc.Errorf(codes.InvalidArgument, "ipam configuration: cannot be empty")
-	}
-
-	_, subnet, err := net.ParseCIDR(ipamConf.Subnet)
-	if err != nil {
-		return grpc.Errorf(codes.InvalidArgument, "ipam configuration: invalid subnet %s", ipamConf.Subnet)
-	}
-
-	if ipamConf.Range != "" {
-		ip, _, err := net.ParseCIDR(ipamConf.Range)
-		if err != nil {
-			return grpc.Errorf(codes.InvalidArgument, "ipam configuration: invalid range %s", ipamConf.Range)
-		}
-
-		if !subnet.Contains(ip) {
-			return grpc.Errorf(codes.InvalidArgument, "ipam configuration: subnet %s does not contain range %s", ipamConf.Subnet, ipamConf.Range)
-		}
-	}
-
-	if ipamConf.Gateway != "" {
-		ip := net.ParseIP(ipamConf.Gateway)
-		if ip == nil {
-			return grpc.Errorf(codes.InvalidArgument, "ipam configuration: invalid gateway %s", ipamConf.Gateway)
-		}
-
-		if !subnet.Contains(ip) {
-			return grpc.Errorf(codes.InvalidArgument, "ipam configuration: subnet %s does not contain gateway %s", ipamConf.Subnet, ipamConf.Gateway)
-		}
-	}
-
-	return nil
-}
-
-func validateIPAM(ipam *api.IPAMOptions, nm network.Model) error {
-	if ipam == nil {
-		// It is ok to not specify any IPAM configurations. We
-		// will choose good defaults.
-		return nil
-	}
-
-	if err := nm.ValidateDriver(ipam.Driver, ipamapi.PluginEndpointType); err != nil {
-		return err
-	}
-
-	for _, ipamConf := range ipam.Configs {
-		if err := validateIPAMConfiguration(ipamConf); err != nil {
-			return err
-		}
-	}
-
-	return nil
-}
-
 func validateNetworkSpec(spec *api.NetworkSpec, nm network.Model) error {
 	if spec == nil {
 		return grpc.Errorf(codes.InvalidArgument, errInvalidArgument.Error())
 	}
 
-	if spec.Ingress && spec.DriverConfig != nil && spec.DriverConfig.Name != "overlay" {
-		return grpc.Errorf(codes.Unimplemented, "only overlay driver is currently supported for ingress network")
-	}
-
-	// XXX Abstractions
-	if spec.DriverConfig != nil && spec.DriverConfig.Name == "cni" {
-		if spec.IPAM != nil {
-			return grpc.Errorf(codes.InvalidArgument, "CNI networks cannot have IPAM")
-		}
-
-		// This is rather similar to cniConfig in the containerd executor...
-		cniConfig, ok := spec.DriverConfig.Options["config"]
-		if !ok {
-			return grpc.Errorf(codes.InvalidArgument, "CNI network has no config")
-		}
-
-		cni, err := libcni.ConfFromBytes([]byte(cniConfig))
-		if err != nil {
-			return grpc.Errorf(codes.InvalidArgument, "Failed to parse CNI config: %s", err)
-		}
-
-		if spec.Annotations.Name != "" && spec.Annotations.Name != cni.Network.Name {
-			return grpc.Errorf(codes.InvalidArgument,
-				"CNI Network name (%q) must match Spec annotations name (%q)",
-				cni.Network.Name, spec.Annotations.Name)
-		}
-
-		// XXX updating in a function called validate..., pretty bad form?
-		spec.Annotations.Name = cni.Network.Name
+	if err := nm.ValidateNetworkSpec(spec); err != nil {
+		return err
 	}
 
 	if spec.Attachable && spec.Ingress {
@@ -120,13 +33,6 @@ func validateNetworkSpec(spec *api.NetworkSpec, nm network.Model) error {
 		return grpc.Errorf(codes.PermissionDenied, "label %s is for internally created predefined networks and cannot be applied by users",
 			networkallocator.PredefinedLabel)
 	}
-	if err := nm.ValidateDriver(spec.DriverConfig, driverapi.NetworkPluginEndpointType); err != nil {
-		return err
-	}
-
-	if err := validateIPAM(spec.IPAM, nm); err != nil {
-		return err
-	}
 
 	return nil
 }
@@ -135,6 +41,9 @@ func validateNetworkSpec(spec *api.NetworkSpec, nm network.Model) error {
 // - Returns `InvalidArgument` if the NetworkSpec is malformed.
 // - Returns an error if the creation fails.
 func (s *Server) CreateNetwork(ctx context.Context, request *api.CreateNetworkRequest) (*api.CreateNetworkResponse, error) {
+	if err := s.nm.SetDefaults(request.Spec); err != nil {
+		return nil, err
+	}
 	if err := validateNetworkSpec(request.Spec, s.nm); err != nil {
 		return nil, err
 	}

diff --git a/manager/network/cni/cni.go b/manager/network/cni/cni.go
@@ -1,6 +1,7 @@
 package cni
 
 import (
+	"github.com/containernetworking/cni/libcni"
 	"github.com/docker/swarmkit/api"
 	"github.com/docker/swarmkit/manager/allocator/cniallocator"
 	"github.com/docker/swarmkit/manager/allocator/networkallocator"
@@ -24,16 +25,55 @@ func (nm *cni) SupportIngressNetwork() bool {
 	return false
 }
 
-func (nm *cni) ValidateDriver(driver *api.Driver, pluginType string) error {
-	if driver == nil {
-		// It is ok to not specify the driver. We will choose
-		// a default driver.
+func parseCNIspec(spec *api.NetworkSpec) (*libcni.NetworkConfig, error) {
+	// This is rather similar to cniConfig in the containerd executor...
+	cniConfig, ok := spec.DriverConfig.Options["config"]
+	if !ok {
+		return nil, grpc.Errorf(codes.InvalidArgument, "CNI network has no config")
+	}
+
+	cni, err := libcni.ConfFromBytes([]byte(cniConfig))
+	if err != nil {
+		return nil, grpc.Errorf(codes.InvalidArgument, "Failed to parse CNI config: %s", err)
+	}
+	return cni, nil
+}
+
+func (nm *cni) SetDefaults(spec *api.NetworkSpec) error {
+	if spec.DriverConfig == nil || spec.DriverConfig.Name != "cni" {
 		return nil
 	}
 
-	// All drivers in CNI mode are "cni".
-	if driver.Name != "cni" {
-		return grpc.Errorf(codes.InvalidArgument, "driver %s of type %s is not supported in CNI mode", driver.Name, pluginType)
+	cni, err := parseCNIspec(spec)
+	if err != nil {
+		return err
+	}
+
+	if spec.Annotations.Name == "" {
+		spec.Annotations.Name = cni.Network.Name
+	}
+
+	return nil
+}
+
+func (nm *cni) ValidateNetworkSpec(spec *api.NetworkSpec) error {
+	if spec.DriverConfig == nil || spec.DriverConfig.Name != "cni" {
+		return grpc.Errorf(codes.InvalidArgument, "spec is not for a CNI network")
+	}
+
+	cni, err := parseCNIspec(spec)
+	if err != nil {
+		return err
+	}
+
+	if spec.Annotations.Name != cni.Network.Name {
+		return grpc.Errorf(codes.InvalidArgument,
+			"CNI Network name (%q) must match Spec annotations name (%q)",
+			cni.Network.Name, spec.Annotations.Name)
+	}
+
+	if spec.IPAM != nil {
+		return grpc.Errorf(codes.InvalidArgument, "CNI networks cannot have IPAM")
 	}
 
 	return nil

diff --git a/manager/network/cnm/cnm.go b/manager/network/cnm/cnm.go
@@ -1,6 +1,7 @@
 package cnm
 
 import (
+	"net"
 	"strings"
 
 	"github.com/docker/docker/pkg/plugingetter"
@@ -33,7 +34,27 @@ func (nm *cnm) SupportIngressNetwork() bool {
 	return true
 }
 
-func (nm *cnm) ValidateDriver(driver *api.Driver, pluginType string) error {
+func (nm *cnm) ValidateNetworkSpec(spec *api.NetworkSpec) error {
+	if spec.Ingress && spec.DriverConfig != nil && spec.DriverConfig.Name != "overlay" {
+		return grpc.Errorf(codes.Unimplemented, "only overlay driver is currently supported for ingress network")
+	}
+
+	if err := nm.validateDriver(spec.DriverConfig, driverapi.NetworkPluginEndpointType); err != nil {
+		return err
+	}
+
+	if err := nm.validateIPAM(spec.IPAM); err != nil {
+		return err
+	}
+
+	return nil
+}
+
+func (nm *cnm) SetDefaults(spec *api.NetworkSpec) error {
+	return nil
+}
+
+func (nm *cnm) validateDriver(driver *api.Driver, pluginType string) error {
 	if driver == nil {
 		// It is ok to not specify the driver. We will choose
 		// a default driver.
@@ -72,3 +93,58 @@ func (nm *cnm) ValidateDriver(driver *api.Driver, pluginType string) error {
 
 	return nil
 }
+
+func validateIPAMConfiguration(ipamConf *api.IPAMConfig) error {
+	if ipamConf == nil {
+		return grpc.Errorf(codes.InvalidArgument, "ipam configuration: cannot be empty")
+	}
+
+	_, subnet, err := net.ParseCIDR(ipamConf.Subnet)
+	if err != nil {
+		return grpc.Errorf(codes.InvalidArgument, "ipam configuration: invalid subnet %s", ipamConf.Subnet)
+	}
+
+	if ipamConf.Range != "" {
+		ip, _, err := net.ParseCIDR(ipamConf.Range)
+		if err != nil {
+			return grpc.Errorf(codes.InvalidArgument, "ipam configuration: invalid range %s", ipamConf.Range)
+		}
+
+		if !subnet.Contains(ip) {
+			return grpc.Errorf(codes.InvalidArgument, "ipam configuration: subnet %s does not contain range %s", ipamConf.Subnet, ipamConf.Range)
+		}
+	}
+
+	if ipamConf.Gateway != "" {
+		ip := net.ParseIP(ipamConf.Gateway)
+		if ip == nil {
+			return grpc.Errorf(codes.InvalidArgument, "ipam configuration: invalid gateway %s", ipamConf.Gateway)
+		}
+
+		if !subnet.Contains(ip) {
+			return grpc.Errorf(codes.InvalidArgument, "ipam configuration: subnet %s does not contain gateway %s", ipamConf.Subnet, ipamConf.Gateway)
+		}
+	}
+
+	return nil
+}
+
+func (nm *cnm) validateIPAM(ipam *api.IPAMOptions) error {
+	if ipam == nil {
+		// It is ok to not specify any IPAM configurations. We
+		// will choose good defaults.
+		return nil
+	}
+
+	if err := nm.validateDriver(ipam.Driver, ipamapi.PluginEndpointType); err != nil {
+		return err
+	}
+
+	for _, ipamConf := range ipam.Configs {
+		if err := validateIPAMConfiguration(ipamConf); err != nil {
+			return err
+		}
+	}
+
+	return nil
+}
diff --git a/manager/network/model.go b/manager/network/model.go
@@ -8,7 +8,8 @@ import (
 // Model is an abstraction over the Network Model to be used.
 type Model interface {
 	Allocator() (networkallocator.NetworkAllocator, error)
-	ValidateDriver(driver *api.Driver, pluginType string) error
+	SetDefaults(spec *api.NetworkSpec) error
+	ValidateNetworkSpec(spec *api.NetworkSpec) error
 	PredefinedNetworks() []networkallocator.PredefinedNetworkData
 	SupportIngressNetwork() bool
 }