Skip to content

Commit a0822dd

Browse files
danwinshipdanwinship
committed
Validate that SDN API object CIDRs are in canonical form
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.
1 parent 3a10959 commit a0822dd

File tree

3 files changed

+48
-15
lines changed

3 files changed

+48
-15
lines changed

‎pkg/sdn/api/validation/validation.go

+10-9
Original file line numberDiff line numberDiff line change
@@ -9,13 +9,14 @@ import (
99
"k8s.io/kubernetes/pkg/util/validation/field"
1010

1111
sdnapi "github.com/openshift/origin/pkg/sdn/api"
12+
"github.com/openshift/origin/pkg/util/netutils"
1213
)
1314

1415
// ValidateClusterNetwork tests if required fields in the ClusterNetwork are set.
1516
func ValidateClusterNetwork(clusterNet *sdnapi.ClusterNetwork) field.ErrorList {
1617
allErrs := validation.ValidateObjectMeta(&clusterNet.ObjectMeta, false, path.ValidatePathSegmentName, field.NewPath("metadata"))
1718

18-
clusterIP, clusterIPNet, err := net.ParseCIDR(clusterNet.Network)
19+
clusterIPNet, err := netutils.ParseCIDRMask(clusterNet.Network)
1920
if err != nil {
2021
allErrs = append(allErrs, field.Invalid(field.NewPath("network"), clusterNet.Network, err.Error()))
2122
} else {
@@ -25,36 +26,36 @@ func ValidateClusterNetwork(clusterNet *sdnapi.ClusterNetwork) field.ErrorList {
2526
}
2627
}
2728

28-
serviceIP, serviceIPNet, err := net.ParseCIDR(clusterNet.ServiceNetwork)
29+
serviceIPNet, err := netutils.ParseCIDRMask(clusterNet.ServiceNetwork)
2930
if err != nil {
3031
allErrs = append(allErrs, field.Invalid(field.NewPath("serviceNetwork"), clusterNet.ServiceNetwork, err.Error()))
3132
}
3233

33-
if (clusterIPNet != nil) && (serviceIP != nil) && clusterIPNet.Contains(serviceIP) {
34+
if (clusterIPNet != nil) && (serviceIPNet != nil) && clusterIPNet.Contains(serviceIPNet.IP) {
3435
allErrs = append(allErrs, field.Invalid(field.NewPath("serviceNetwork"), clusterNet.ServiceNetwork, "service network overlaps with cluster network"))
3536
}
36-
if (serviceIPNet != nil) && (clusterIP != nil) && serviceIPNet.Contains(clusterIP) {
37+
if (serviceIPNet != nil) && (clusterIPNet != nil) && serviceIPNet.Contains(clusterIPNet.IP) {
3738
allErrs = append(allErrs, field.Invalid(field.NewPath("network"), clusterNet.Network, "cluster network overlaps with service network"))
3839
}
3940

4041
return allErrs
4142
}
4243

4344
func validateNewNetwork(obj *sdnapi.ClusterNetwork, old *sdnapi.ClusterNetwork) *field.Error {
44-
oldBase, oldNet, err := net.ParseCIDR(old.Network)
45+
oldNet, err := netutils.ParseCIDRMask(old.Network)
4546
if err != nil {
4647
// Shouldn't happen, but if the existing value is invalid, then any change should be an improvement...
4748
return nil
4849
}
4950
oldSize, _ := oldNet.Mask.Size()
50-
_, newNet, err := net.ParseCIDR(obj.Network)
51+
newNet, err := netutils.ParseCIDRMask(obj.Network)
5152
if err != nil {
5253
return field.Invalid(field.NewPath("network"), obj.Network, err.Error())
5354
}
5455
newSize, _ := newNet.Mask.Size()
5556
// oldSize/newSize is, eg the "16" in "10.1.0.0/16", so "newSize < oldSize" means
5657
// the new network is larger
57-
if newSize < oldSize && newNet.Contains(oldBase) {
58+
if newSize < oldSize && newNet.Contains(oldNet.IP) {
5859
return nil
5960
} else {
6061
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 {
9697
allErrs = append(allErrs, field.Invalid(field.NewPath("subnet"), hs.Subnet, "field cannot be empty"))
9798
}
9899
} else {
99-
_, _, err := net.ParseCIDR(hs.Subnet)
100+
_, err := netutils.ParseCIDRMask(hs.Subnet)
100101
if err != nil {
101102
allErrs = append(allErrs, field.Invalid(field.NewPath("subnet"), hs.Subnet, err.Error()))
102103
}
@@ -147,7 +148,7 @@ func ValidateEgressNetworkPolicy(policy *sdnapi.EgressNetworkPolicy) field.Error
147148
allErrs = append(allErrs, field.Invalid(field.NewPath("spec").Child("egress").Index(i).Child("type"), rule.Type, "invalid policy type"))
148149
}
149150

150-
_, _, err := net.ParseCIDR(rule.To.CIDRSelector)
151+
_, err := netutils.ParseCIDRMask(rule.To.CIDRSelector)
151152
if err != nil {
152153
allErrs = append(allErrs, field.Invalid(field.NewPath("spec").Child("egress").Index(i).Child("to"), rule.To.CIDRSelector, err.Error()))
153154
}

‎pkg/sdn/api/validation/validation_test.go

+32
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,16 @@ func TestValidateClusterNetwork(t *testing.T) {
3636
},
3737
expectedErrors: 1,
3838
},
39+
{
40+
name: "Bad network CIDR",
41+
cn: &api.ClusterNetwork{
42+
ObjectMeta: kapi.ObjectMeta{Name: "any"},
43+
Network: "10.20.0.1/16",
44+
HostSubnetLength: 8,
45+
ServiceNetwork: "172.30.0.0/16",
46+
},
47+
expectedErrors: 1,
48+
},
3949
{
4050
name: "Invalid subnet length",
4151
cn: &api.ClusterNetwork{
@@ -56,6 +66,16 @@ func TestValidateClusterNetwork(t *testing.T) {
5666
},
5767
expectedErrors: 1,
5868
},
69+
{
70+
name: "Bad service network CIDR",
71+
cn: &api.ClusterNetwork{
72+
ObjectMeta: kapi.ObjectMeta{Name: "any"},
73+
Network: "10.20.0.0/16",
74+
HostSubnetLength: 8,
75+
ServiceNetwork: "172.30.1.0/16",
76+
},
77+
expectedErrors: 1,
78+
},
5979
{
6080
name: "Service network overlaps with cluster network",
6181
cn: &api.ClusterNetwork{
@@ -129,6 +149,18 @@ func TestValidateHostSubnet(t *testing.T) {
129149
},
130150
expectedErrors: 1,
131151
},
152+
{
153+
name: "Malformed subnet CIDR",
154+
hs: &api.HostSubnet{
155+
ObjectMeta: kapi.ObjectMeta{
156+
Name: "abc.def.com",
157+
},
158+
Host: "abc.def.com",
159+
HostIP: "10.20.30.40",
160+
Subnet: "8.8.0.1/24",
161+
},
162+
expectedErrors: 1,
163+
},
132164
}
133165

134166
for _, tc := range tests {

‎test/integration/etcd_storage_path_test.go

+6-6
Original file line numberDiff line numberDiff line change
@@ -268,29 +268,29 @@ var etcdStorageData = map[unversioned.GroupVersionResource]struct {
268268
expectedGVK: gvkP("", "v1", "NetNamespace"), // expect the legacy group to be persisted
269269
},
270270
gvr("", "v1", "hostsubnets"): {
271-
stub: `{"host": "hostname", "hostIP": "192.168.1.1", "metadata": {"name": "hostname"}, "subnet": "192.168.1.1/24"}`,
271+
stub: `{"host": "hostname", "hostIP": "192.168.1.1", "metadata": {"name": "hostname"}, "subnet": "192.168.1.0/24"}`,
272272
expectedEtcdPath: "openshift.io/registry/sdnsubnets/hostname",
273273
},
274274
gvr("network.openshift.io", "v1", "hostsubnets"): {
275-
stub: `{"host": "hostnameg", "hostIP": "192.168.1.1", "metadata": {"name": "hostnameg"}, "subnet": "192.168.1.1/24"}`,
275+
stub: `{"host": "hostnameg", "hostIP": "192.168.1.1", "metadata": {"name": "hostnameg"}, "subnet": "192.168.1.0/24"}`,
276276
expectedEtcdPath: "openshift.io/registry/sdnsubnets/hostnameg",
277277
expectedGVK: gvkP("", "v1", "HostSubnet"), // expect the legacy group to be persisted
278278
},
279279
gvr("", "v1", "clusternetworks"): {
280-
stub: `{"metadata": {"name": "cn1"}, "network": "192.168.0.1/24", "serviceNetwork": "192.168.1.1/24"}`,
280+
stub: `{"metadata": {"name": "cn1"}, "network": "192.168.0.0/24", "serviceNetwork": "192.168.1.0/24"}`,
281281
expectedEtcdPath: "openshift.io/registry/sdnnetworks/cn1",
282282
},
283283
gvr("network.openshift.io", "v1", "clusternetworks"): {
284-
stub: `{"metadata": {"name": "cn1g"}, "network": "192.168.0.1/24", "serviceNetwork": "192.168.1.1/24"}`,
284+
stub: `{"metadata": {"name": "cn1g"}, "network": "192.168.0.0/24", "serviceNetwork": "192.168.1.0/24"}`,
285285
expectedEtcdPath: "openshift.io/registry/sdnnetworks/cn1g",
286286
expectedGVK: gvkP("", "v1", "ClusterNetwork"), // expect the legacy group to be persisted
287287
},
288288
gvr("", "v1", "egressnetworkpolicies"): {
289-
stub: `{"metadata": {"name": "enp1"}, "spec": {"egress": [{"to": {"cidrSelector": "192.168.1.1/24"}, "type": "Allow"}]}}`,
289+
stub: `{"metadata": {"name": "enp1"}, "spec": {"egress": [{"to": {"cidrSelector": "192.168.1.0/24"}, "type": "Allow"}]}}`,
290290
expectedEtcdPath: "openshift.io/registry/egressnetworkpolicy/etcdstoragepathtestnamespace/enp1",
291291
},
292292
gvr("network.openshift.io", "v1", "egressnetworkpolicies"): {
293-
stub: `{"metadata": {"name": "enp1g"}, "spec": {"egress": [{"to": {"cidrSelector": "192.168.1.1/24"}, "type": "Allow"}]}}`,
293+
stub: `{"metadata": {"name": "enp1g"}, "spec": {"egress": [{"to": {"cidrSelector": "192.168.1.0/24"}, "type": "Allow"}]}}`,
294294
expectedEtcdPath: "openshift.io/registry/egressnetworkpolicy/etcdstoragepathtestnamespace/enp1g",
295295
expectedGVK: gvkP("", "v1", "EgressNetworkPolicy"), // expect the legacy group to be persisted
296296
},

0 commit comments

Comments
 (0)