Skip to content

Commit

Permalink
changes the location of intersect tests and additions to testing
Browse files Browse the repository at this point in the history 
move the intersection code from validation to common

add testing for parseNetworkInfo to comm_test.go

fix a few other testing things that came from changing ClusterDef to ClusterNetworks
  • Loading branch information
@JacobTanenbaum
JacobTanenbaum committed Jun 22, 2017
1 parent a1cc6bf commit 6010d34
Show file tree
Hide file tree
Showing 7 changed files with 102 additions and 93 deletions.
21 changes: 1 addition & 20 deletions pkg/sdn/api/validation/validation.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,19 +20,6 @@ func SetDefaultClusterNetwork(cn sdnapi.ClusterNetwork) {
defaultClusterNetwork = &cn
}

//intersect tests two CIDR addresses to see if the first contains the second
func intersect(cidr1, cidr2 string) bool {
_, net1, err := net.ParseCIDR(cidr1)
if err != nil {
return false
}
_, net2, err := net.ParseCIDR(cidr2)
if err != nil {
//cidr2 was not a cidr address which will be caught later
return false
}
return net1.Contains(net2.IP)
}

// ValidateClusterNetwork tests if required fields in the ClusterNetwork are set, and ensures that the "default" ClusterNetwork can only be set to the correct values
func ValidateClusterNetwork(clusterNet *sdnapi.ClusterNetwork) field.ErrorList {
Expand All @@ -41,7 +28,7 @@ func ValidateClusterNetwork(clusterNet *sdnapi.ClusterNetwork) field.ErrorList {
if len(clusterNet.ClusterNetworks) == 0 {
allErrs = append(allErrs, field.Invalid(field.NewPath("clusterNetworks"), clusterNet.ClusterNetworks, "empty"))
}
for index, cidr := range clusterNet.ClusterNetworks {
for _, cidr := range clusterNet.ClusterNetworks {
clusterIPNet, err := netutils.ParseCIDRMask(cidr.CIDR)
if err != nil {
allErrs = append(allErrs, field.Invalid(field.NewPath("network"), cidr.CIDR, err.Error()))
Expand All @@ -58,12 +45,6 @@ func ValidateClusterNetwork(clusterNet *sdnapi.ClusterNetwork) field.ErrorList {
if err != nil {
allErrs = append(allErrs, field.Invalid(field.NewPath("serviceNetwork"), clusterNet.ServiceNetwork, err.Error()))
}
for _, cidr_next := range clusterNet.ClusterNetworks[index+1:] {
if intersect(cidr.CIDR, cidr_next.CIDR) {
allErrs = append(allErrs, field.Invalid(field.NewPath("clusterNetworkCIDR"), clusterNet.ClusterNetworks, "two of the specified cidr addresses overlap"))

}
}
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"))
}
Expand Down
42 changes: 16 additions & 26 deletions pkg/sdn/api/validation/validation_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ func TestValidateClusterNetwork(t *testing.T) {
name: "Good one",
cn: &api.ClusterNetwork{
ObjectMeta: metav1.ObjectMeta{Name: "any"},
ClusterDef: []api.ClusterNetworkEntry{{ClusterNetworkCIDR: "10.20.0.0/16"}},
ClusterNetworks: []api.ClusterNetworkEntry{{CIDR: "10.20.0.0/16"}},
HostSubnetLength: 8,
ServiceNetwork: "172.30.0.0/16",
},
Expand All @@ -36,7 +36,7 @@ func TestValidateClusterNetwork(t *testing.T) {
name: "Good one multiple addresses",
cn: &api.ClusterNetwork{
ObjectMeta: metav1.ObjectMeta{Name: "any"},
ClusterDef: []api.ClusterNetworkEntry{{ClusterNetworkCIDR: "10.20.0.0/16"}, {ClusterNetworkCIDR: "10.128.0.0/16"}},
ClusterNetworks: []api.ClusterNetworkEntry{{CIDR: "10.20.0.0/16"}, {CIDR: "10.128.0.0/16"}},
HostSubnetLength: 8,
ServiceNetwork: "172.30.0.0/16",
},
Expand All @@ -46,7 +46,7 @@ func TestValidateClusterNetwork(t *testing.T) {
name: "Bad network",
cn: &api.ClusterNetwork{
ObjectMeta: metav1.ObjectMeta{Name: "any"},
ClusterDef: []api.ClusterNetworkEntry{{ClusterNetworkCIDR: "10.20.0.0.0/16"}},
ClusterNetworks: []api.ClusterNetworkEntry{{CIDR: "10.20.0.0.0/16"}},
HostSubnetLength: 8,
ServiceNetwork: "172.30.0.0/16",
},
Expand All @@ -56,7 +56,7 @@ func TestValidateClusterNetwork(t *testing.T) {
name: "Bad network CIDR",
cn: &api.ClusterNetwork{
ObjectMeta: metav1.ObjectMeta{Name: "any"},
ClusterDef: []api.ClusterNetworkEntry{{ClusterNetworkCIDR: "10.20.0.1/16"}},
ClusterNetworks: []api.ClusterNetworkEntry{{CIDR: "10.20.0.1/16"}},
HostSubnetLength: 8,
ServiceNetwork: "172.30.0.0/16",
},
Expand All @@ -66,7 +66,7 @@ func TestValidateClusterNetwork(t *testing.T) {
name: "Subnet length too large for network",
cn: &api.ClusterNetwork{
ObjectMeta: metav1.ObjectMeta{Name: "any"},
ClusterDef: []api.ClusterNetworkEntry{{ClusterNetworkCIDR: "10.20.30.0/24"}},
ClusterNetworks: []api.ClusterNetworkEntry{{CIDR: "10.20.30.0/24"}},
HostSubnetLength: 16,
ServiceNetwork: "172.30.0.0/16",
},
Expand All @@ -76,7 +76,7 @@ func TestValidateClusterNetwork(t *testing.T) {
name: "Subnet length too small",
cn: &api.ClusterNetwork{
ObjectMeta: metav1.ObjectMeta{Name: "any"},
ClusterDef: []api.ClusterNetworkEntry{{ClusterNetworkCIDR: "10.20.0.0/16"}},
ClusterNetworks: []api.ClusterNetworkEntry{{CIDR: "10.20.0.0/16"}},
HostSubnetLength: 1,
ServiceNetwork: "172.30.0.0/16",
},
Expand All @@ -86,7 +86,7 @@ func TestValidateClusterNetwork(t *testing.T) {
name: "Bad service network",
cn: &api.ClusterNetwork{
ObjectMeta: metav1.ObjectMeta{Name: "any"},
ClusterDef: []api.ClusterNetworkEntry{{ClusterNetworkCIDR: "10.20.0.0/16"}},
ClusterNetworks: []api.ClusterNetworkEntry{{CIDR: "10.20.0.0/16"}},
HostSubnetLength: 8,
ServiceNetwork: "1172.30.0.0/16",
},
Expand All @@ -96,27 +96,17 @@ func TestValidateClusterNetwork(t *testing.T) {
name: "Bad service network CIDR",
cn: &api.ClusterNetwork{
ObjectMeta: metav1.ObjectMeta{Name: "any"},
ClusterDef: []api.ClusterNetworkEntry{{ClusterNetworkCIDR: "10.20.0.0/16"}},
ClusterNetworks: []api.ClusterNetworkEntry{{CIDR: "10.20.0.0/16"}},
HostSubnetLength: 8,
ServiceNetwork: "172.30.1.0/16",
},
expectedErrors: 1,
},
{
name: "Two cluster network addresses overlap",
cn: &api.ClusterNetwork{
ObjectMeta: metav1.ObjectMeta{Name: "any"},
ClusterDef: []api.ClusterNetworkEntry{{ClusterNetworkCIDR: "10.20.0.0/16"}, {ClusterNetworkCIDR: "10.20.4.0/16"}},
HostSubnetLength: 8,
ServiceNetwork: "172.30.0.0/16",
},
expectedErrors: 2,
},
{
name: "Service network overlaps with cluster network",
cn: &api.ClusterNetwork{
ObjectMeta: metav1.ObjectMeta{Name: "any"},
ClusterDef: []api.ClusterNetworkEntry{{ClusterNetworkCIDR: "10.20.0.0/16"}},
ClusterNetworks: []api.ClusterNetworkEntry{{CIDR: "10.20.0.0/16"}},
HostSubnetLength: 8,
ServiceNetwork: "10.20.1.0/24",
},
Expand All @@ -126,7 +116,7 @@ func TestValidateClusterNetwork(t *testing.T) {
name: "Cluster network overlaps with service network",
cn: &api.ClusterNetwork{
ObjectMeta: metav1.ObjectMeta{Name: "any"},
ClusterDef: []api.ClusterNetworkEntry{{ClusterNetworkCIDR: "10.20.0.0/16"}},
ClusterNetworks: []api.ClusterNetworkEntry{{CIDR: "10.20.0.0/16"}},
HostSubnetLength: 8,
ServiceNetwork: "10.0.0.0/8",
},
Expand All @@ -146,7 +136,7 @@ func TestValidateClusterNetwork(t *testing.T) {
func TestSetDefaultClusterNetwork(t *testing.T) {
defaultClusterNetwork := api.ClusterNetwork{
ObjectMeta: metav1.ObjectMeta{Name: api.ClusterNetworkDefault},
ClusterDef: []api.ClusterNetworkEntry{{ClusterNetworkCIDR: "10.20.0.0/16"}},
ClusterNetworks: []api.ClusterNetworkEntry{{CIDR: "10.20.0.0/16"}},
HostSubnetLength: 8,
ServiceNetwork: "172.30.0.0/16",
PluginName: "redhat/openshift-ovs-multitenant",
Expand All @@ -167,7 +157,7 @@ func TestSetDefaultClusterNetwork(t *testing.T) {
name: "Wrong Network",
cn: &api.ClusterNetwork{
ObjectMeta: metav1.ObjectMeta{Name: api.ClusterNetworkDefault},
ClusterDef: []api.ClusterNetworkEntry{{ClusterNetworkCIDR: "10.30.0.0/16"}},
ClusterNetworks: []api.ClusterNetworkEntry{{CIDR: "10.30.0.0/16"}},
HostSubnetLength: 8,
ServiceNetwork: "172.30.0.0/16",
PluginName: "redhat/openshift-ovs-multitenant",
Expand All @@ -178,7 +168,7 @@ func TestSetDefaultClusterNetwork(t *testing.T) {
name: "Additional Network",
cn: &api.ClusterNetwork{
ObjectMeta: metav1.ObjectMeta{Name: api.ClusterNetworkDefault},
ClusterDef: []api.ClusterNetworkEntry{{ClusterNetworkCIDR: "10.20.0.0/16"}, {ClusterNetworkCIDR: "10.30.0.0/16"}},
ClusterNetworks: []api.ClusterNetworkEntry{{CIDR: "10.20.0.0/16"}, {CIDR: "10.30.0.0/16"}},
HostSubnetLength: 8,
ServiceNetwork: "172.30.0.0/16",
PluginName: "redhat/openshift-ovs-multitenant",
Expand All @@ -189,7 +179,7 @@ func TestSetDefaultClusterNetwork(t *testing.T) {
name: "Wrong HostSubnetLength",
cn: &api.ClusterNetwork{
ObjectMeta: metav1.ObjectMeta{Name: api.ClusterNetworkDefault},
ClusterDef: []api.ClusterNetworkEntry{{ClusterNetworkCIDR: "10.20.0.0/16"}},
ClusterNetworks: []api.ClusterNetworkEntry{{CIDR: "10.20.0.0/16"}},
HostSubnetLength: 9,
ServiceNetwork: "172.30.0.0/16",
PluginName: "redhat/openshift-ovs-multitenant",
Expand All @@ -200,7 +190,7 @@ func TestSetDefaultClusterNetwork(t *testing.T) {
name: "Wrong ServiceNetwork",
cn: &api.ClusterNetwork{
ObjectMeta: metav1.ObjectMeta{Name: api.ClusterNetworkDefault},
ClusterDef: []api.ClusterNetworkEntry{{ClusterNetworkCIDR: "10.20.0.0/16"}},
ClusterNetworks: []api.ClusterNetworkEntry{{CIDR: "10.20.0.0/16"}},
HostSubnetLength: 8,
ServiceNetwork: "172.20.0.0/16",
PluginName: "redhat/openshift-ovs-multitenant",
Expand All @@ -211,7 +201,7 @@ func TestSetDefaultClusterNetwork(t *testing.T) {
name: "Wrong PluginName",
cn: &api.ClusterNetwork{
ObjectMeta: metav1.ObjectMeta{Name: api.ClusterNetworkDefault},
ClusterDef: []api.ClusterNetworkEntry{{ClusterNetworkCIDR: "10.20.0.0/16"}},
ClusterNetworks: []api.ClusterNetworkEntry{{CIDR: "10.20.0.0/16"}},
HostSubnetLength: 8,
ServiceNetwork: "172.30.0.0/16",
PluginName: "redhat/openshift-ovs-subnet",
Expand Down
10 changes: 10 additions & 0 deletions pkg/sdn/plugin/common.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,11 @@ type NetworkInfo struct {
ServiceNetwork *net.IPNet
}

//determine if two cidr addresses intersect
func intersect(cidr1, cidr2 *net.IPNet) bool {
return cidr2.Contains(cidr1.IP) || cidr1.Contains(cidr2.IP)
}

func parseNetworkInfo(clusterNetwork []osapi.ClusterNetworkEntry, serviceNetwork string) (*NetworkInfo, error) {
var cn []*net.IPNet

Expand All @@ -66,6 +71,11 @@ func parseNetworkInfo(clusterNetwork []osapi.ClusterNetworkEntry, serviceNetwork
}
glog.Errorf("Configured clusterNetworkCIDR value %q is invalid; treating it as %q", entry.CIDR, clusterAddress.String())
}
for _, cidr := range cn {
if intersect(cidr, clusterAddress) {
return nil, fmt.Errorf("Two of the cidr addresses overlap: %s, %s", cidr.String(), clusterAddress.String())
}
}
cn = append(cn, clusterAddress)
}

Expand Down
54 changes: 54 additions & 0 deletions pkg/sdn/plugin/common_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -188,3 +188,57 @@ func Test_checkClusterObjects(t *testing.T) {
}
}
}


func Test_parseNetworkInfo(t *testing.T){
tests := []struct {
name string
cidrs []osapi.ClusterNetworkEntry
serviceNetwork string
err string
}{
{
name: "valid single cidr",
cidrs: []osapi.ClusterNetworkEntry{{CIDR: "10.0.0.0/16"}},
serviceNetwork: "172.30.0.0/16",
err: "",
},
{
name: "valid multiple cidr",
cidrs: []osapi.ClusterNetworkEntry{{CIDR: "10.0.0.0/16"},{CIDR: "10.4.0.0/16"}},
serviceNetwork: "172.30.0.0/16",
err: "",
},
{
name: "invalid CIDR address",
cidrs: []osapi.ClusterNetworkEntry{{CIDR: "Invalid"}},
serviceNetwork: "172.30.0.0/16",
err: "Invalid",
},
{
name: "overlapping cidr addresses",
cidrs: []osapi.ClusterNetworkEntry{{CIDR: "10.0.0.0/16"},{CIDR: "10.0.0.0/8"}},
serviceNetwork: "172.30.0.0/16",
err: "overlap",
},
{
name: "invalid serviceNetwork",
cidrs: []osapi.ClusterNetworkEntry{{CIDR: "10.0.0.0/16"}},
serviceNetwork: "172.30.0.0i/16",
err: "172.30.0.0i/16",
},

}
for _, test := range tests {
_, err := parseNetworkInfo(test.cidrs, test.serviceNetwork)
if err == nil {
if len(test.err) > 0 {
t.Fatalf("test %q unexpectedly did not get an error", test.name)
}
} else {
if !strings.Contains(err.Error(), test.err) {
t.Fatalf("test %q: error did not match %q: %v", test.name, test.err, err)
}
}
}
}
30 changes: 13 additions & 17 deletions pkg/sdn/plugin/master.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ package plugin

import (
"fmt"
"net"

log "github.com/golang/glog"

Expand Down Expand Up @@ -150,28 +149,25 @@ func (master *OsdnMaster) checkClusterNetworkAgainstClusterObjects() error {
func clusterNetworkChanged(obj *osapi.ClusterNetwork, old *osapi.ClusterNetwork) (bool, error) {
changed := false


if old.Network != obj.Network {
if len(old.ClusterNetworks) != len(obj.ClusterNetworks) {
changed = true
}

_, newNet, err := net.ParseCIDR(obj.Network)
if err != nil {
return true, err
}
newSize, _ := newNet.Mask.Size()
oldBase, oldNet, err := net.ParseCIDR(old.Network)
if err != nil {
// Shouldn't happen, but if the existing value is invalid, then any change should be an improvement...
} else {
oldSize, _ := oldNet.Mask.Size()

// oldSize and newSize are, eg the "16" in "10.1.0.0/16", so
// "newSize < oldSize" means the new network is larger
if !(newSize < oldSize && newNet.Contains(oldBase)) {
return true, fmt.Errorf("cannot change clusterNetworkCIDR to a value that does not include the existing network.")
for _, oldCIDR := range old.ClusterNetworks{
found := false
for _, newCIDR := range obj.ClusterNetworks {
if newCIDR == oldCIDR {
found = true
}
}
if found == false {
changed = true
break
}
}


if old.HostSubnetLength != obj.HostSubnetLength {
return true, fmt.Errorf("cannot change the hostSubnetLength of an already-deployed cluster")
}
Expand Down
30 changes: 4 additions & 26 deletions pkg/sdn/plugin/master_test.go
View file
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
package plugin

import (
"fmt"
"testing"

osapi "github.com/openshift/origin/pkg/sdn/api"
Expand All @@ -10,7 +9,7 @@ import (
func Test_clusterNetworkChanged(t *testing.T) {
origCN := osapi.ClusterNetwork{
Network: "10.128.0.0/14",
ClusterDef: []osapi.ClusterNetworkEntry{{ClusterNetworkCIDR: "10.128.0.0/14"}},
ClusterNetworks: []osapi.ClusterNetworkEntry{{CIDR: "10.128.0.0/14"}},
HostSubnetLength: 10,
ServiceNetwork: "172.30.0.0/16",
PluginName: "redhat/openshift-ovs-subnet",
Expand All @@ -29,31 +28,10 @@ func Test_clusterNetworkChanged(t *testing.T) {
{
name: "larger Network",
changes: &osapi.ClusterNetwork{
ClusterDef: []osapi.ClusterNetworkEntry{{ClusterNetworkCIDR: "10.128.0.0/12"}},
ClusterNetworks: []osapi.ClusterNetworkEntry{{CIDR: "10.128.0.0/12"}},
},
expectError: false,
},
{
name: "larger Network",
changes: &osapi.ClusterNetwork{
Network: "10.0.0.0/8",
},
expectError: false,
},
{
name: "smaller Network",
changes: &osapi.ClusterNetwork{
Network: "10.128.0.0/15",
},
expectError: true,
},
{
name: "moved Network",
changes: &osapi.ClusterNetwork{
Network: "10.1.0.0/16",
},
expectError: true,
},
{
name: "larger HostSubnetLength",
changes: &osapi.ClusterNetwork{
Expand Down Expand Up @@ -101,8 +79,8 @@ func Test_clusterNetworkChanged(t *testing.T) {
for _, test := range tests {
newCN := origCN
expectChanged := false
if test.changes.ClusterDef != nil {
newCN.ClusterDef = test.changes.ClusterDef
if test.changes.ClusterNetworks != nil {
newCN.ClusterNetworks = test.changes.ClusterNetworks
expectChanged = true
}
if test.changes.Network != "" {
Expand Down
Loading

0 comments on commit 6010d34

Please sign in to comment.