diff --git a/pkg/sdn/api/validation/validation.go b/pkg/sdn/api/validation/validation.go index db3bb0599273..c836aee6d262 100644 --- a/pkg/sdn/api/validation/validation.go +++ b/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,15 +26,15 @@ 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")) } @@ -41,20 +42,20 @@ func ValidateClusterNetwork(clusterNet *sdnapi.ClusterNetwork) field.ErrorList { } 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())) } diff --git a/pkg/sdn/api/validation/validation_test.go b/pkg/sdn/api/validation/validation_test.go index 56f3aeacfa9a..7b1e36ba306c 100644 --- a/pkg/sdn/api/validation/validation_test.go +++ b/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 { diff --git a/test/integration/etcd_storage_path_test.go b/test/integration/etcd_storage_path_test.go index 2a65e27cf3c5..d88f0bb331b0 100644 --- a/test/integration/etcd_storage_path_test.go +++ b/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 },