Skip to content

Commit

Permalink
Validate that SDN API object CIDRs are in canonical form
Browse files Browse the repository at this point in the history 
Eg, if you want ClusterNetwork to be "10.128.0.0/14", you have to say
"10.128.0.0/14", not "10.128.0.1/14" or "10.128.32.99/14".

All OpenShift-generated objects already did this correctly, but this
might cause previously-considered-valid EgressNetworkPolicy objects to
start failing to validate.
  • Loading branch information
@danwinship
danwinship committed Apr 3, 2017
1 parent 0ffd292 commit d51561a
Show file tree
Hide file tree
Showing 3 changed files with 49 additions and 15 deletions.
19 changes: 10 additions & 9 deletions pkg/sdn/api/validation/validation.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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.")
Expand Down Expand Up @@ -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()))
}
Expand Down Expand Up @@ -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()))
}
Expand Down
33 changes: 33 additions & 0 deletions pkg/sdn/api/validation/validation_test.go
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package validation

import (
"strings"
"testing"

kapi "k8s.io/kubernetes/pkg/api"
Expand Down Expand Up @@ -36,6 +37,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{
Expand All @@ -56,6 +67,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{
Expand Down Expand Up @@ -129,6 +150,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 {
Expand Down
12 changes: 6 additions & 6 deletions test/integration/etcd_storage_path_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
},
Expand Down

0 comments on commit d51561a

Please sign in to comment.