Skip to content

Commit

Permalink
Merge pull request #13508 from danwinship/cidr-validation
Browse files Browse the repository at this point in the history 
Merged by openshift-bot
  • Loading branch information
OpenShift Bot authored Apr 7, 2017
2 parents 42c3bfa + a0822dd commit 8125090
Show file tree
Hide file tree
Showing 6 changed files with 128 additions and 19 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
32 changes: 32 additions & 0 deletions pkg/sdn/api/validation/validation_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand All @@ -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{
Expand Down Expand Up @@ -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 {
Expand Down
17 changes: 13 additions & 4 deletions pkg/sdn/plugin/common.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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{
Expand Down
19 changes: 19 additions & 0 deletions pkg/util/netutils/common.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
48 changes: 48 additions & 0 deletions pkg/util/netutils/common_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package netutils

import (
"net"
"strings"
"testing"
)

Expand Down Expand Up @@ -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)
}
}
}
}
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 8125090

Please sign in to comment.