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/pkg/sdn/plugin/common.go b/pkg/sdn/plugin/common.go index 664209321157..478db30842a3 100644 --- a/pkg/sdn/plugin/common.go +++ b/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{ diff --git a/pkg/util/netutils/common.go b/pkg/util/netutils/common.go index 3508e0a30ac5..0bb39d43ca8d 100644 --- a/pkg/util/netutils/common.go +++ b/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 +} diff --git a/pkg/util/netutils/common_test.go b/pkg/util/netutils/common_test.go index 20bb86f7c04f..495651dd3d81 100644 --- a/pkg/util/netutils/common_test.go +++ b/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) + } + } + } +} 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 },