Validate that SDN API object CIDRs are in canonical form

pkg/sdn/api/validation/validation.go

@@ -9,13 +9,14 @@ import (
 	"k8s.io/kubernetes/pkg/util/validation/field"
 
 	sdnapi "github.com/openshift/origin/pkg/sdn/api"
+	"github.com/openshift/origin/pkg/util/netutils"
 )
 
 // ValidateClusterNetwork tests if required fields in the ClusterNetwork are set.
 func ValidateClusterNetwork(clusterNet *sdnapi.ClusterNetwork) field.ErrorList {
 	allErrs := validation.ValidateObjectMeta(&clusterNet.ObjectMeta, false, path.ValidatePathSegmentName, field.NewPath("metadata"))
 
-	clusterIP, clusterIPNet, err := net.ParseCIDR(clusterNet.Network)
+	clusterIPNet, err := netutils.ParseCIDRMask(clusterNet.Network)
 	if err != nil {
 		allErrs = append(allErrs, field.Invalid(field.NewPath("network"), clusterNet.Network, err.Error()))
 	} else {
@@ -25,36 +26,36 @@ func ValidateClusterNetwork(clusterNet *sdnapi.ClusterNetwork) field.ErrorList {
 		}
 	}
 
-	serviceIP, serviceIPNet, err := net.ParseCIDR(clusterNet.ServiceNetwork)
+	serviceIPNet, err := netutils.ParseCIDRMask(clusterNet.ServiceNetwork)
 	if err != nil {
 		allErrs = append(allErrs, field.Invalid(field.NewPath("serviceNetwork"), clusterNet.ServiceNetwork, err.Error()))
 	}
 
-	if (clusterIPNet != nil) && (serviceIP != nil) && clusterIPNet.Contains(serviceIP) {
+	if (clusterIPNet != nil) && (serviceIPNet != nil) && clusterIPNet.Contains(serviceIPNet.IP) {
 		allErrs = append(allErrs, field.Invalid(field.NewPath("serviceNetwork"), clusterNet.ServiceNetwork, "service network overlaps with cluster network"))
 	}
-	if (serviceIPNet != nil) && (clusterIP != nil) && serviceIPNet.Contains(clusterIP) {
+	if (serviceIPNet != nil) && (clusterIPNet != nil) && serviceIPNet.Contains(clusterIPNet.IP) {
 		allErrs = append(allErrs, field.Invalid(field.NewPath("network"), clusterNet.Network, "cluster network overlaps with service network"))
 	}
 
 	return allErrs
 }
 
 func validateNewNetwork(obj *sdnapi.ClusterNetwork, old *sdnapi.ClusterNetwork) *field.Error {
-	oldBase, oldNet, err := net.ParseCIDR(old.Network)
+	oldNet, err := netutils.ParseCIDRMask(old.Network)
 	if err != nil {
 		// Shouldn't happen, but if the existing value is invalid, then any change should be an improvement...
 		return nil
 	}
 	oldSize, _ := oldNet.Mask.Size()
-	_, newNet, err := net.ParseCIDR(obj.Network)
+	newNet, err := netutils.ParseCIDRMask(obj.Network)
 	if err != nil {
 		return field.Invalid(field.NewPath("network"), obj.Network, err.Error())
 	}
 	newSize, _ := newNet.Mask.Size()
 	// oldSize/newSize is, eg the "16" in "10.1.0.0/16", so "newSize < oldSize" means
 	// the new network is larger
-	if newSize < oldSize && newNet.Contains(oldBase) {
+	if newSize < oldSize && newNet.Contains(oldNet.IP) {
 		return nil
 	} else {
 		return field.Invalid(field.NewPath("network"), obj.Network, "cannot change the cluster's network CIDR to a value that does not include the existing network.")
@@ -96,7 +97,7 @@ func ValidateHostSubnet(hs *sdnapi.HostSubnet) field.ErrorList {
 			allErrs = append(allErrs, field.Invalid(field.NewPath("subnet"), hs.Subnet, "field cannot be empty"))
 		}
 	} else {
-		_, _, err := net.ParseCIDR(hs.Subnet)
+		_, err := netutils.ParseCIDRMask(hs.Subnet)
 		if err != nil {
 			allErrs = append(allErrs, field.Invalid(field.NewPath("subnet"), hs.Subnet, err.Error()))
 		}
@@ -147,7 +148,7 @@ func ValidateEgressNetworkPolicy(policy *sdnapi.EgressNetworkPolicy) field.Error
 			allErrs = append(allErrs, field.Invalid(field.NewPath("spec").Child("egress").Index(i).Child("type"), rule.Type, "invalid policy type"))
 		}
 
-		_, _, err := net.ParseCIDR(rule.To.CIDRSelector)
+		_, err := netutils.ParseCIDRMask(rule.To.CIDRSelector)
 		if err != nil {
 			allErrs = append(allErrs, field.Invalid(field.NewPath("spec").Child("egress").Index(i).Child("to"), rule.To.CIDRSelector, err.Error()))
 		}

pkg/sdn/api/validation/validation_test.go

@@ -36,6 +36,16 @@ func TestValidateClusterNetwork(t *testing.T) {
 			},
 			expectedErrors: 1,
 		},
+		{
+			name: "Bad network CIDR",
+			cn: &api.ClusterNetwork{
+				ObjectMeta:       kapi.ObjectMeta{Name: "any"},
+				Network:          "10.20.0.1/16",
+				HostSubnetLength: 8,
+				ServiceNetwork:   "172.30.0.0/16",
+			},
+			expectedErrors: 1,
+		},
 		{
 			name: "Invalid subnet length",
 			cn: &api.ClusterNetwork{
@@ -56,6 +66,16 @@ func TestValidateClusterNetwork(t *testing.T) {
 			},
 			expectedErrors: 1,
 		},
+		{
+			name: "Bad service network CIDR",
+			cn: &api.ClusterNetwork{
+				ObjectMeta:       kapi.ObjectMeta{Name: "any"},
+				Network:          "10.20.0.0/16",
+				HostSubnetLength: 8,
+				ServiceNetwork:   "172.30.1.0/16",
+			},
+			expectedErrors: 1,
+		},
 		{
 			name: "Service network overlaps with cluster network",
 			cn: &api.ClusterNetwork{
@@ -129,6 +149,18 @@ func TestValidateHostSubnet(t *testing.T) {
 			},
 			expectedErrors: 1,
 		},
+		{
+			name: "Malformed subnet CIDR",
+			hs: &api.HostSubnet{
+				ObjectMeta: kapi.ObjectMeta{
+					Name: "abc.def.com",
+				},
+				Host:   "abc.def.com",
+				HostIP: "10.20.30.40",
+				Subnet: "8.8.0.1/24",
+			},
+			expectedErrors: 1,
+		},
 	}
 
 	for _, tc := range tests {

pkg/sdn/plugin/common.go

@@ -10,6 +10,7 @@ import (
 
 	osclient "github.com/openshift/origin/pkg/client"
 	osapi "github.com/openshift/origin/pkg/sdn/api"
+	"github.com/openshift/origin/pkg/util/netutils"
 
 	kapi "k8s.io/kubernetes/pkg/api"
 	"k8s.io/kubernetes/pkg/apis/extensions"
@@ -39,13 +40,21 @@ type NetworkInfo struct {
 }
 
 func parseNetworkInfo(clusterNetwork string, serviceNetwork string) (*NetworkInfo, error) {
-	_, cn, err := net.ParseCIDR(clusterNetwork)
+	cn, err := netutils.ParseCIDRMask(clusterNetwork)
 	if err != nil {
-		return nil, fmt.Errorf("failed to parse ClusterNetwork CIDR %s: %v", clusterNetwork, err)
+		_, cn, err := net.ParseCIDR(clusterNetwork)
+		if err != nil {
+			return nil, fmt.Errorf("failed to parse ClusterNetwork CIDR %s: %v", clusterNetwork, err)
+		}
+		glog.Errorf("Configured clusterNetworkCIDR value %q is invalid; treating it as %q", clusterNetwork, cn.String())
 	}
-	_, sn, err := net.ParseCIDR(serviceNetwork)
+	sn, err := netutils.ParseCIDRMask(serviceNetwork)
 	if err != nil {
-		return nil, fmt.Errorf("failed to parse ServiceNetwork CIDR %s: %v", serviceNetwork, err)
+		_, sn, err := net.ParseCIDR(serviceNetwork)
+		if err != nil {
+			return nil, fmt.Errorf("failed to parse ServiceNetwork CIDR %s: %v", serviceNetwork, err)
+		}
+		glog.Errorf("Configured serviceNetworkCIDR value %q is invalid; treating it as %q", serviceNetwork, sn.String())
 	}
 
 	return &NetworkInfo{

pkg/util/netutils/common.go

@@ -94,3 +94,22 @@ func GetNodeIP(nodeName string) (string, error) {
 	}
 	return ip.String(), nil
 }
+
+// ParseCIDRMask parses a CIDR string and ensures that it has no bits set beyond the
+// network mask length. Use this when the input is supposed to be either a description of
+// a subnet (eg, "192.168.1.0/24", meaning "192.168.1.0 to 192.168.1.255"), or a mask for
+// matching against (eg, "192.168.1.15/32", meaning "must match all 32 bits of the address
+// "192.168.1.15"). Use net.ParseCIDR() when the input is a host address that also
+// describes the subnet that it is on (eg, "192.168.1.15/24", meaning "the address
+// 192.168.1.15 on the network 192.168.1.0/24").
+func ParseCIDRMask(cidr string) (*net.IPNet, error) {
+	ip, net, err := net.ParseCIDR(cidr)
+	if err != nil {
+		return nil, err
+	}
+	if !ip.Equal(net.IP) {
+		maskLen, addrLen := net.Mask.Size()
+		return nil, fmt.Errorf("CIDR network specification %q is not in canonical form (should be %s/%d or %s/%d?)", cidr, ip.Mask(net.Mask).String(), maskLen, ip.String(), addrLen)
+	}
+	return net, nil
+}

pkg/util/netutils/common_test.go

@@ -2,6 +2,7 @@ package netutils
 
 import (
 	"net"
+	"strings"
 	"testing"
 )
 
@@ -40,3 +41,50 @@ func TestGenerateGateway(t *testing.T) {
 		t.Fatalf("Did not get expected gateway IP Address (gatewayIP=%s)", gatewayIP.String())
 	}
 }
+
+func TestParseCIDRMask(t *testing.T) {
+	tests := []struct {
+		cidr       string
+		fixedShort string
+		fixedLong  string
+	}{
+		{
+			cidr: "192.168.0.0/16",
+		},
+		{
+			cidr: "192.168.1.0/24",
+		},
+		{
+			cidr: "192.168.1.1/32",
+		},
+		{
+			cidr:       "192.168.1.0/16",
+			fixedShort: "192.168.0.0/16",
+			fixedLong:  "192.168.1.0/32",
+		},
+		{
+			cidr:       "192.168.1.1/24",
+			fixedShort: "192.168.1.0/24",
+			fixedLong:  "192.168.1.1/32",
+		},
+	}
+
+	for _, test := range tests {
+		_, err := ParseCIDRMask(test.cidr)
+		if test.fixedShort == "" && test.fixedLong == "" {
+			if err != nil {
+				t.Fatalf("unexpected error parsing CIDR mask %q: %v", test.cidr, err)
+			}
+		} else {
+			if err == nil {
+				t.Fatalf("unexpected lack of error parsing CIDR mask %q", test.cidr)
+			}
+			if !strings.Contains(err.Error(), test.fixedShort) {
+				t.Fatalf("error does not contain expected string %q: %v", test.fixedShort, err)
+			}
+			if !strings.Contains(err.Error(), test.fixedLong) {
+				t.Fatalf("error does not contain expected string %q: %v", test.fixedLong, err)
+			}
+		}
+	}
+}

test/integration/etcd_storage_path_test.go

@@ -268,29 +268,29 @@ var etcdStorageData = map[unversioned.GroupVersionResource]struct {
 		expectedGVK:      gvkP("", "v1", "NetNamespace"), // expect the legacy group to be persisted
 	},
 	gvr("", "v1", "hostsubnets"): {
-		stub:             `{"host": "hostname", "hostIP": "192.168.1.1", "metadata": {"name": "hostname"}, "subnet": "192.168.1.1/24"}`,
+		stub:             `{"host": "hostname", "hostIP": "192.168.1.1", "metadata": {"name": "hostname"}, "subnet": "192.168.1.0/24"}`,
 		expectedEtcdPath: "openshift.io/registry/sdnsubnets/hostname",
 	},
 	gvr("network.openshift.io", "v1", "hostsubnets"): {
-		stub:             `{"host": "hostnameg", "hostIP": "192.168.1.1", "metadata": {"name": "hostnameg"}, "subnet": "192.168.1.1/24"}`,
+		stub:             `{"host": "hostnameg", "hostIP": "192.168.1.1", "metadata": {"name": "hostnameg"}, "subnet": "192.168.1.0/24"}`,
 		expectedEtcdPath: "openshift.io/registry/sdnsubnets/hostnameg",
 		expectedGVK:      gvkP("", "v1", "HostSubnet"), // expect the legacy group to be persisted
 	},
 	gvr("", "v1", "clusternetworks"): {
-		stub:             `{"metadata": {"name": "cn1"}, "network": "192.168.0.1/24", "serviceNetwork": "192.168.1.1/24"}`,
+		stub:             `{"metadata": {"name": "cn1"}, "network": "192.168.0.0/24", "serviceNetwork": "192.168.1.0/24"}`,
 		expectedEtcdPath: "openshift.io/registry/sdnnetworks/cn1",
 	},
 	gvr("network.openshift.io", "v1", "clusternetworks"): {
-		stub:             `{"metadata": {"name": "cn1g"}, "network": "192.168.0.1/24", "serviceNetwork": "192.168.1.1/24"}`,
+		stub:             `{"metadata": {"name": "cn1g"}, "network": "192.168.0.0/24", "serviceNetwork": "192.168.1.0/24"}`,
 		expectedEtcdPath: "openshift.io/registry/sdnnetworks/cn1g",
 		expectedGVK:      gvkP("", "v1", "ClusterNetwork"), // expect the legacy group to be persisted
 	},
 	gvr("", "v1", "egressnetworkpolicies"): {
-		stub:             `{"metadata": {"name": "enp1"}, "spec": {"egress": [{"to": {"cidrSelector": "192.168.1.1/24"}, "type": "Allow"}]}}`,
+		stub:             `{"metadata": {"name": "enp1"}, "spec": {"egress": [{"to": {"cidrSelector": "192.168.1.0/24"}, "type": "Allow"}]}}`,
 		expectedEtcdPath: "openshift.io/registry/egressnetworkpolicy/etcdstoragepathtestnamespace/enp1",
 	},
 	gvr("network.openshift.io", "v1", "egressnetworkpolicies"): {
-		stub:             `{"metadata": {"name": "enp1g"}, "spec": {"egress": [{"to": {"cidrSelector": "192.168.1.1/24"}, "type": "Allow"}]}}`,
+		stub:             `{"metadata": {"name": "enp1g"}, "spec": {"egress": [{"to": {"cidrSelector": "192.168.1.0/24"}, "type": "Allow"}]}}`,
 		expectedEtcdPath: "openshift.io/registry/egressnetworkpolicy/etcdstoragepathtestnamespace/enp1g",
 		expectedGVK:      gvkP("", "v1", "EgressNetworkPolicy"), // expect the legacy group to be persisted
 	},