Skip to content

Commit 7716ca6

Browse files
JacobTanenbaumJacobTanenbaum
committed
changes the location of intersect tests and additions to testing
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
1 parent 85f5a3f commit 7716ca6

File tree

7 files changed

+102
-92
lines changed

7 files changed

+102
-92
lines changed

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

+1-20
Original file line numberDiff line numberDiff line change
@@ -20,19 +20,6 @@ func SetDefaultClusterNetwork(cn sdnapi.ClusterNetwork) {
2020
defaultClusterNetwork = &cn
2121
}
2222

23-
//intersect tests two CIDR addresses to see if the first contains the second
24-
func intersect(cidr1, cidr2 string) bool {
25-
_, net1, err := net.ParseCIDR(cidr1)
26-
if err != nil {
27-
return false
28-
}
29-
_, net2, err := net.ParseCIDR(cidr2)
30-
if err != nil {
31-
//cidr2 was not a cidr address which will be caught later
32-
return false
33-
}
34-
return net1.Contains(net2.IP)
35-
}
3623

3724
// ValidateClusterNetwork tests if required fields in the ClusterNetwork are set, and ensures that the "default" ClusterNetwork can only be set to the correct values
3825
func ValidateClusterNetwork(clusterNet *sdnapi.ClusterNetwork) field.ErrorList {
@@ -41,7 +28,7 @@ func ValidateClusterNetwork(clusterNet *sdnapi.ClusterNetwork) field.ErrorList {
4128
if len(clusterNet.ClusterNetworks) == 0 {
4229
allErrs = append(allErrs, field.Invalid(field.NewPath("clusterNetworks"), clusterNet.ClusterNetworks, "empty"))
4330
}
44-
for index, cidr := range clusterNet.ClusterNetworks {
31+
for _, cidr := range clusterNet.ClusterNetworks {
4532
clusterIPNet, err := netutils.ParseCIDRMask(cidr.CIDR)
4633
if err != nil {
4734
allErrs = append(allErrs, field.Invalid(field.NewPath("network"), cidr.CIDR, err.Error()))
@@ -58,12 +45,6 @@ func ValidateClusterNetwork(clusterNet *sdnapi.ClusterNetwork) field.ErrorList {
5845
if err != nil {
5946
allErrs = append(allErrs, field.Invalid(field.NewPath("serviceNetwork"), clusterNet.ServiceNetwork, err.Error()))
6047
}
61-
for _, cidr_next := range clusterNet.ClusterNetworks[index+1:] {
62-
if intersect(cidr.CIDR, cidr_next.CIDR) {
63-
allErrs = append(allErrs, field.Invalid(field.NewPath("clusterNetworkCIDR"), clusterNet.ClusterNetworks, "two of the specified cidr addresses overlap"))
64-
65-
}
66-
}
6748
if (clusterIPNet != nil) && (serviceIPNet != nil) && clusterIPNet.Contains(serviceIPNet.IP) {
6849
allErrs = append(allErrs, field.Invalid(field.NewPath("serviceNetwork"), clusterNet.ServiceNetwork, "service network overlaps with cluster network"))
6950
}

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

+16-26
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@ func TestValidateClusterNetwork(t *testing.T) {
2626
name: "Good one",
2727
cn: &api.ClusterNetwork{
2828
ObjectMeta: metav1.ObjectMeta{Name: "any"},
29-
ClusterDef: []api.ClusterNetworkEntry{{ClusterNetworkCIDR: "10.20.0.0/16"}},
29+
ClusterNetworks: []api.ClusterNetworkEntry{{CIDR: "10.20.0.0/16"}},
3030
HostSubnetLength: 8,
3131
ServiceNetwork: "172.30.0.0/16",
3232
},
@@ -36,7 +36,7 @@ func TestValidateClusterNetwork(t *testing.T) {
3636
name: "Good one multiple addresses",
3737
cn: &api.ClusterNetwork{
3838
ObjectMeta: metav1.ObjectMeta{Name: "any"},
39-
ClusterDef: []api.ClusterNetworkEntry{{ClusterNetworkCIDR: "10.20.0.0/16"}, {ClusterNetworkCIDR: "10.128.0.0/16"}},
39+
ClusterNetworks: []api.ClusterNetworkEntry{{CIDR: "10.20.0.0/16"}, {CIDR: "10.128.0.0/16"}},
4040
HostSubnetLength: 8,
4141
ServiceNetwork: "172.30.0.0/16",
4242
},
@@ -46,7 +46,7 @@ func TestValidateClusterNetwork(t *testing.T) {
4646
name: "Bad network",
4747
cn: &api.ClusterNetwork{
4848
ObjectMeta: metav1.ObjectMeta{Name: "any"},
49-
ClusterDef: []api.ClusterNetworkEntry{{ClusterNetworkCIDR: "10.20.0.0.0/16"}},
49+
ClusterNetworks: []api.ClusterNetworkEntry{{CIDR: "10.20.0.0.0/16"}},
5050
HostSubnetLength: 8,
5151
ServiceNetwork: "172.30.0.0/16",
5252
},
@@ -56,7 +56,7 @@ func TestValidateClusterNetwork(t *testing.T) {
5656
name: "Bad network CIDR",
5757
cn: &api.ClusterNetwork{
5858
ObjectMeta: metav1.ObjectMeta{Name: "any"},
59-
ClusterDef: []api.ClusterNetworkEntry{{ClusterNetworkCIDR: "10.20.0.1/16"}},
59+
ClusterNetworks: []api.ClusterNetworkEntry{{CIDR: "10.20.0.1/16"}},
6060
HostSubnetLength: 8,
6161
ServiceNetwork: "172.30.0.0/16",
6262
},
@@ -66,7 +66,7 @@ func TestValidateClusterNetwork(t *testing.T) {
6666
name: "Subnet length too large for network",
6767
cn: &api.ClusterNetwork{
6868
ObjectMeta: metav1.ObjectMeta{Name: "any"},
69-
ClusterDef: []api.ClusterNetworkEntry{{ClusterNetworkCIDR: "10.20.30.0/24"}},
69+
ClusterNetworks: []api.ClusterNetworkEntry{{CIDR: "10.20.30.0/24"}},
7070
HostSubnetLength: 16,
7171
ServiceNetwork: "172.30.0.0/16",
7272
},
@@ -76,7 +76,7 @@ func TestValidateClusterNetwork(t *testing.T) {
7676
name: "Subnet length too small",
7777
cn: &api.ClusterNetwork{
7878
ObjectMeta: metav1.ObjectMeta{Name: "any"},
79-
ClusterDef: []api.ClusterNetworkEntry{{ClusterNetworkCIDR: "10.20.0.0/16"}},
79+
ClusterNetworks: []api.ClusterNetworkEntry{{CIDR: "10.20.0.0/16"}},
8080
HostSubnetLength: 1,
8181
ServiceNetwork: "172.30.0.0/16",
8282
},
@@ -86,7 +86,7 @@ func TestValidateClusterNetwork(t *testing.T) {
8686
name: "Bad service network",
8787
cn: &api.ClusterNetwork{
8888
ObjectMeta: metav1.ObjectMeta{Name: "any"},
89-
ClusterDef: []api.ClusterNetworkEntry{{ClusterNetworkCIDR: "10.20.0.0/16"}},
89+
ClusterNetworks: []api.ClusterNetworkEntry{{CIDR: "10.20.0.0/16"}},
9090
HostSubnetLength: 8,
9191
ServiceNetwork: "1172.30.0.0/16",
9292
},
@@ -96,27 +96,17 @@ func TestValidateClusterNetwork(t *testing.T) {
9696
name: "Bad service network CIDR",
9797
cn: &api.ClusterNetwork{
9898
ObjectMeta: metav1.ObjectMeta{Name: "any"},
99-
ClusterDef: []api.ClusterNetworkEntry{{ClusterNetworkCIDR: "10.20.0.0/16"}},
99+
ClusterNetworks: []api.ClusterNetworkEntry{{CIDR: "10.20.0.0/16"}},
100100
HostSubnetLength: 8,
101101
ServiceNetwork: "172.30.1.0/16",
102102
},
103103
expectedErrors: 1,
104104
},
105-
{
106-
name: "Two cluster network addresses overlap",
107-
cn: &api.ClusterNetwork{
108-
ObjectMeta: metav1.ObjectMeta{Name: "any"},
109-
ClusterDef: []api.ClusterNetworkEntry{{ClusterNetworkCIDR: "10.20.0.0/16"}, {ClusterNetworkCIDR: "10.20.4.0/16"}},
110-
HostSubnetLength: 8,
111-
ServiceNetwork: "172.30.0.0/16",
112-
},
113-
expectedErrors: 2,
114-
},
115105
{
116106
name: "Service network overlaps with cluster network",
117107
cn: &api.ClusterNetwork{
118108
ObjectMeta: metav1.ObjectMeta{Name: "any"},
119-
ClusterDef: []api.ClusterNetworkEntry{{ClusterNetworkCIDR: "10.20.0.0/16"}},
109+
ClusterNetworks: []api.ClusterNetworkEntry{{CIDR: "10.20.0.0/16"}},
120110
HostSubnetLength: 8,
121111
ServiceNetwork: "10.20.1.0/24",
122112
},
@@ -126,7 +116,7 @@ func TestValidateClusterNetwork(t *testing.T) {
126116
name: "Cluster network overlaps with service network",
127117
cn: &api.ClusterNetwork{
128118
ObjectMeta: metav1.ObjectMeta{Name: "any"},
129-
ClusterDef: []api.ClusterNetworkEntry{{ClusterNetworkCIDR: "10.20.0.0/16"}},
119+
ClusterNetworks: []api.ClusterNetworkEntry{{CIDR: "10.20.0.0/16"}},
130120
HostSubnetLength: 8,
131121
ServiceNetwork: "10.0.0.0/8",
132122
},
@@ -146,7 +136,7 @@ func TestValidateClusterNetwork(t *testing.T) {
146136
func TestSetDefaultClusterNetwork(t *testing.T) {
147137
defaultClusterNetwork := api.ClusterNetwork{
148138
ObjectMeta: metav1.ObjectMeta{Name: api.ClusterNetworkDefault},
149-
ClusterDef: []api.ClusterNetworkEntry{{ClusterNetworkCIDR: "10.20.0.0/16"}},
139+
ClusterNetworks: []api.ClusterNetworkEntry{{CIDR: "10.20.0.0/16"}},
150140
HostSubnetLength: 8,
151141
ServiceNetwork: "172.30.0.0/16",
152142
PluginName: "redhat/openshift-ovs-multitenant",
@@ -167,7 +157,7 @@ func TestSetDefaultClusterNetwork(t *testing.T) {
167157
name: "Wrong Network",
168158
cn: &api.ClusterNetwork{
169159
ObjectMeta: metav1.ObjectMeta{Name: api.ClusterNetworkDefault},
170-
ClusterDef: []api.ClusterNetworkEntry{{ClusterNetworkCIDR: "10.30.0.0/16"}},
160+
ClusterNetworks: []api.ClusterNetworkEntry{{CIDR: "10.30.0.0/16"}},
171161
HostSubnetLength: 8,
172162
ServiceNetwork: "172.30.0.0/16",
173163
PluginName: "redhat/openshift-ovs-multitenant",
@@ -178,7 +168,7 @@ func TestSetDefaultClusterNetwork(t *testing.T) {
178168
name: "Additional Network",
179169
cn: &api.ClusterNetwork{
180170
ObjectMeta: metav1.ObjectMeta{Name: api.ClusterNetworkDefault},
181-
ClusterDef: []api.ClusterNetworkEntry{{ClusterNetworkCIDR: "10.20.0.0/16"}, {ClusterNetworkCIDR: "10.30.0.0/16"}},
171+
ClusterNetworks: []api.ClusterNetworkEntry{{CIDR: "10.20.0.0/16"}, {CIDR: "10.30.0.0/16"}},
182172
HostSubnetLength: 8,
183173
ServiceNetwork: "172.30.0.0/16",
184174
PluginName: "redhat/openshift-ovs-multitenant",
@@ -189,7 +179,7 @@ func TestSetDefaultClusterNetwork(t *testing.T) {
189179
name: "Wrong HostSubnetLength",
190180
cn: &api.ClusterNetwork{
191181
ObjectMeta: metav1.ObjectMeta{Name: api.ClusterNetworkDefault},
192-
ClusterDef: []api.ClusterNetworkEntry{{ClusterNetworkCIDR: "10.20.0.0/16"}},
182+
ClusterNetworks: []api.ClusterNetworkEntry{{CIDR: "10.20.0.0/16"}},
193183
HostSubnetLength: 9,
194184
ServiceNetwork: "172.30.0.0/16",
195185
PluginName: "redhat/openshift-ovs-multitenant",
@@ -200,7 +190,7 @@ func TestSetDefaultClusterNetwork(t *testing.T) {
200190
name: "Wrong ServiceNetwork",
201191
cn: &api.ClusterNetwork{
202192
ObjectMeta: metav1.ObjectMeta{Name: api.ClusterNetworkDefault},
203-
ClusterDef: []api.ClusterNetworkEntry{{ClusterNetworkCIDR: "10.20.0.0/16"}},
193+
ClusterNetworks: []api.ClusterNetworkEntry{{CIDR: "10.20.0.0/16"}},
204194
HostSubnetLength: 8,
205195
ServiceNetwork: "172.20.0.0/16",
206196
PluginName: "redhat/openshift-ovs-multitenant",
@@ -211,7 +201,7 @@ func TestSetDefaultClusterNetwork(t *testing.T) {
211201
name: "Wrong PluginName",
212202
cn: &api.ClusterNetwork{
213203
ObjectMeta: metav1.ObjectMeta{Name: api.ClusterNetworkDefault},
214-
ClusterDef: []api.ClusterNetworkEntry{{ClusterNetworkCIDR: "10.20.0.0/16"}},
204+
ClusterNetworks: []api.ClusterNetworkEntry{{CIDR: "10.20.0.0/16"}},
215205
HostSubnetLength: 8,
216206
ServiceNetwork: "172.30.0.0/16",
217207
PluginName: "redhat/openshift-ovs-subnet",

‎pkg/sdn/plugin/common.go

+10
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,11 @@ type NetworkInfo struct {
5454
ServiceNetwork *net.IPNet
5555
}
5656

57+
//determine if two cidr addresses intersect
58+
func intersect(cidr1, cidr2 *net.IPNet) bool {
59+
return cidr2.Contains(cidr1.IP) || cidr1.Contains(cidr2.IP)
60+
}
61+
5762
func parseNetworkInfo(clusterNetwork []osapi.ClusterNetworkEntry, serviceNetwork string) (*NetworkInfo, error) {
5863
var cn []*net.IPNet
5964

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

‎pkg/sdn/plugin/common_test.go

+54
Original file line numberDiff line numberDiff line change
@@ -188,3 +188,57 @@ func Test_checkClusterObjects(t *testing.T) {
188188
}
189189
}
190190
}
191+
192+
193+
func Test_parseNetworkInfo(t *testing.T){
194+
tests := []struct {
195+
name string
196+
cidrs []osapi.ClusterNetworkEntry
197+
serviceNetwork string
198+
err string
199+
}{
200+
{
201+
name: "valid single cidr",
202+
cidrs: []osapi.ClusterNetworkEntry{{CIDR: "10.0.0.0/16"}},
203+
serviceNetwork: "172.30.0.0/16",
204+
err: "",
205+
},
206+
{
207+
name: "valid multiple cidr",
208+
cidrs: []osapi.ClusterNetworkEntry{{CIDR: "10.0.0.0/16"},{CIDR: "10.4.0.0/16"}},
209+
serviceNetwork: "172.30.0.0/16",
210+
err: "",
211+
},
212+
{
213+
name: "invalid CIDR address",
214+
cidrs: []osapi.ClusterNetworkEntry{{CIDR: "Invalid"}},
215+
serviceNetwork: "172.30.0.0/16",
216+
err: "Invalid",
217+
},
218+
{
219+
name: "overlapping cidr addresses",
220+
cidrs: []osapi.ClusterNetworkEntry{{CIDR: "10.0.0.0/16"},{CIDR: "10.0.0.0/8"}},
221+
serviceNetwork: "172.30.0.0/16",
222+
err: "overlap",
223+
},
224+
{
225+
name: "invalid serviceNetwork",
226+
cidrs: []osapi.ClusterNetworkEntry{{CIDR: "10.0.0.0/16"}},
227+
serviceNetwork: "172.30.0.0i/16",
228+
err: "172.30.0.0i/16",
229+
},
230+
231+
}
232+
for _, test := range tests {
233+
_, err := parseNetworkInfo(test.cidrs, test.serviceNetwork)
234+
if err == nil {
235+
if len(test.err) > 0 {
236+
t.Fatalf("test %q unexpectedly did not get an error", test.name)
237+
}
238+
} else {
239+
if !strings.Contains(err.Error(), test.err) {
240+
t.Fatalf("test %q: error did not match %q: %v", test.name, test.err, err)
241+
}
242+
}
243+
}
244+
}

‎pkg/sdn/plugin/master.go

+13-16
Original file line numberDiff line numberDiff line change
@@ -168,28 +168,25 @@ func (master *OsdnMaster) checkClusterNetworkAgainstClusterObjects() error {
168168
func clusterNetworkChanged(obj *osapi.ClusterNetwork, old *osapi.ClusterNetwork) (bool, error) {
169169
changed := false
170170

171-
172-
if old.Network != obj.Network {
171+
if len(old.ClusterNetworks) != len(obj.ClusterNetworks) {
173172
changed = true
173+
}
174174

175-
_, newNet, err := net.ParseCIDR(obj.Network)
176-
if err != nil {
177-
return true, err
178-
}
179-
newSize, _ := newNet.Mask.Size()
180-
oldBase, oldNet, err := net.ParseCIDR(old.Network)
181-
if err != nil {
182-
// Shouldn't happen, but if the existing value is invalid, then any change should be an improvement...
183-
} else {
184-
oldSize, _ := oldNet.Mask.Size()
185175

186-
// oldSize and newSize are, eg the "16" in "10.1.0.0/16", so
187-
// "newSize < oldSize" means the new network is larger
188-
if !(newSize < oldSize && newNet.Contains(oldBase)) {
189-
return true, fmt.Errorf("cannot change clusterNetworkCIDR to a value that does not include the existing network.")
176+
for _, oldCIDR := range old.ClusterNetworks{
177+
found := false
178+
for _, newCIDR := range obj.ClusterNetworks {
179+
if newCIDR == oldCIDR {
180+
found = true
190181
}
191182
}
183+
if found == false {
184+
changed = true
185+
break
186+
}
192187
}
188+
189+
193190
if old.HostSubnetLength != obj.HostSubnetLength {
194191
return true, fmt.Errorf("cannot change the hostSubnetLength of an already-deployed cluster")
195192
}

‎pkg/sdn/plugin/master_test.go

+4-26
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,6 @@
11
package plugin
22

33
import (
4-
"fmt"
54
"testing"
65

76
osapi "github.com/openshift/origin/pkg/sdn/api"
@@ -10,7 +9,7 @@ import (
109
func Test_clusterNetworkChanged(t *testing.T) {
1110
origCN := osapi.ClusterNetwork{
1211
Network: "10.128.0.0/14",
13-
ClusterDef: []osapi.ClusterNetworkEntry{{ClusterNetworkCIDR: "10.128.0.0/14"}},
12+
ClusterNetworks: []osapi.ClusterNetworkEntry{{CIDR: "10.128.0.0/14"}},
1413
HostSubnetLength: 10,
1514
ServiceNetwork: "172.30.0.0/16",
1615
PluginName: "redhat/openshift-ovs-subnet",
@@ -29,31 +28,10 @@ func Test_clusterNetworkChanged(t *testing.T) {
2928
{
3029
name: "larger Network",
3130
changes: &osapi.ClusterNetwork{
32-
ClusterDef: []osapi.ClusterNetworkEntry{{ClusterNetworkCIDR: "10.128.0.0/12"}},
31+
ClusterNetworks: []osapi.ClusterNetworkEntry{{CIDR: "10.128.0.0/12"}},
3332
},
3433
expectError: false,
3534
},
36-
{
37-
name: "larger Network",
38-
changes: &osapi.ClusterNetwork{
39-
Network: "10.0.0.0/8",
40-
},
41-
expectError: false,
42-
},
43-
{
44-
name: "smaller Network",
45-
changes: &osapi.ClusterNetwork{
46-
Network: "10.128.0.0/15",
47-
},
48-
expectError: true,
49-
},
50-
{
51-
name: "moved Network",
52-
changes: &osapi.ClusterNetwork{
53-
Network: "10.1.0.0/16",
54-
},
55-
expectError: true,
56-
},
5735
{
5836
name: "larger HostSubnetLength",
5937
changes: &osapi.ClusterNetwork{
@@ -101,8 +79,8 @@ func Test_clusterNetworkChanged(t *testing.T) {
10179
for _, test := range tests {
10280
newCN := origCN
10381
expectChanged := false
104-
if test.changes.ClusterDef != nil {
105-
newCN.ClusterDef = test.changes.ClusterDef
82+
if test.changes.ClusterNetworks != nil {
83+
newCN.ClusterNetworks = test.changes.ClusterNetworks
10684
expectChanged = true
10785
}
10886
if test.changes.Network != "" {

0 commit comments

Comments
 (0)