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".
(net.ParseCIDR() accepts the latter two, because they are valid ways of
referring to hosts within that network, but they aren't valid ways of
referring to the network itself, which is what we want here).

(All OpenShift-generated objects already do this correctly, but this
might cause previously-considered-valid EgressNetworkPolicy objects to
start failing to validate.)
  • Loading branch information
@danwinship
danwinship committed Mar 22, 2017
1 parent e61510a commit ead561e
Show file tree
Hide file tree
Showing 3 changed files with 105 additions and 12 deletions.
25 changes: 19 additions & 6 deletions pkg/sdn/api/validation/validation.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,24 @@ import (
sdnapi "github.com/openshift/origin/pkg/sdn/api"
)

// parseCIDRMask parses a CIDR string and verifies that it does not have any bits set beyond the mask length
func parseCIDRMask(cidr string) (net.IP, *net.IPNet, error) {
ip, net, err := net.ParseCIDR(cidr)
if err != nil {
return nil, nil, err
}
if !ip.Equal(ip.Mask(net.Mask)) {
maskLen, addrLen := net.Mask.Size()
return nil, nil, fmt.Errorf("CIDR address %q is not in canonical form (should be %s/%d or %s/%d?)", cidr, ip.Mask(net.Mask).String(), maskLen, ip.String(), addrLen)
}
return ip, net, nil
}

// 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)
clusterIP, clusterIPNet, err := parseCIDRMask(clusterNet.Network)
if err != nil {
allErrs = append(allErrs, field.Invalid(field.NewPath("network"), clusterNet.Network, err.Error()))
} else {
Expand All @@ -25,7 +38,7 @@ func ValidateClusterNetwork(clusterNet *sdnapi.ClusterNetwork) field.ErrorList {
}
}

serviceIP, serviceIPNet, err := net.ParseCIDR(clusterNet.ServiceNetwork)
serviceIP, serviceIPNet, err := parseCIDRMask(clusterNet.ServiceNetwork)
if err != nil {
allErrs = append(allErrs, field.Invalid(field.NewPath("serviceNetwork"), clusterNet.ServiceNetwork, err.Error()))
}
Expand All @@ -41,13 +54,13 @@ func ValidateClusterNetwork(clusterNet *sdnapi.ClusterNetwork) field.ErrorList {
}

func validateNewNetwork(obj *sdnapi.ClusterNetwork, old *sdnapi.ClusterNetwork) *field.Error {
oldBase, oldNet, err := net.ParseCIDR(old.Network)
oldBase, oldNet, err := 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 := parseCIDRMask(obj.Network)
if err != nil {
return field.Invalid(field.NewPath("network"), obj.Network, err.Error())
}
Expand Down Expand Up @@ -96,7 +109,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 := parseCIDRMask(hs.Subnet)
if err != nil {
allErrs = append(allErrs, field.Invalid(field.NewPath("subnet"), hs.Subnet, err.Error()))
}
Expand Down Expand Up @@ -147,7 +160,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 := 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
80 changes: 80 additions & 0 deletions pkg/sdn/api/validation/validation_test.go
View file
Original file line number Diff line number Diff line change
@@ -1,13 +1,61 @@
package validation

import (
"strings"
"testing"

kapi "k8s.io/kubernetes/pkg/api"

"github.com/openshift/origin/pkg/sdn/api"
)

func Test_parseCIDRMask(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)
}
}
}
}

// TestValidateClusterNetwork ensures not specifying a required field results in error and a fully specified
// sdn passes successfully
func TestValidateClusterNetwork(t *testing.T) {
Expand Down Expand Up @@ -36,6 +84,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 +114,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 +197,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 ead561e

Please sign in to comment.