Skip to content

Commit 07049d4

Browse files
danwinshipdanwinship
committed
Fix defaulting of legacy ClusterNetwork fields
1 parent dea5d49 commit 07049d4

File tree

3 files changed

+127
-44
lines changed

3 files changed

+127
-44
lines changed

‎pkg/network/apis/network/validation/validation.go

+35-15
Original file line numberDiff line numberDiff line change
@@ -50,29 +50,49 @@ func ValidateClusterNetwork(clusterNet *networkapi.ClusterNetwork) field.ErrorLi
5050
allErrs := validation.ValidateObjectMeta(&clusterNet.ObjectMeta, false, path.ValidatePathSegmentName, field.NewPath("metadata"))
5151
var testedCIDRS []*net.IPNet
5252

53-
// This check is mainly for ensuring backward compatibility
54-
if len(clusterNet.Network) != 0 || clusterNet.HostSubnetLength != 0 {
55-
//In the case that a user manually makes a clusterNetwork object with clusterNet.Network and clusterNet.HostubnetLength at least make sure they are valid values
56-
clusterIPNet, err := validateCIDRv4(clusterNet.Network)
57-
if err != nil {
58-
allErrs = append(allErrs, field.Invalid(field.NewPath("network"), clusterNet.Network, err.Error()))
53+
serviceIPNet, err := validateCIDRv4(clusterNet.ServiceNetwork)
54+
if err != nil {
55+
allErrs = append(allErrs, field.Invalid(field.NewPath("serviceNetwork"), clusterNet.ServiceNetwork, err.Error()))
56+
}
57+
58+
if len(clusterNet.ClusterNetworks) == 0 {
59+
// legacy ClusterNetwork; old fields must be set
60+
if clusterNet.Network == "" {
61+
allErrs = append(allErrs, field.Required(field.NewPath("network"), "network must be set (if clusterNetworks is empty)"))
62+
} else if clusterNet.HostSubnetLength == 0 {
63+
allErrs = append(allErrs, field.Required(field.NewPath("hostsubnetlength"), "hostsubnetlength must be set (if clusterNetworks is empty)"))
5964
} else {
65+
clusterIPNet, err := validateCIDRv4(clusterNet.Network)
66+
if err != nil {
67+
allErrs = append(allErrs, field.Invalid(field.NewPath("network"), clusterNet.Network, err.Error()))
68+
}
6069
maskLen, addrLen := clusterIPNet.Mask.Size()
6170
if clusterNet.HostSubnetLength > uint32(addrLen-maskLen) {
62-
allErrs = append(allErrs, field.Invalid(field.NewPath("hostsubnetlength"), clusterNet.HostSubnetLength, "subnet length is too large for clusterNetwork"))
71+
allErrs = append(allErrs, field.Invalid(field.NewPath("hostsubnetlength"), clusterNet.HostSubnetLength, "subnet length is too large for cidr"))
6372
} else if clusterNet.HostSubnetLength < 2 {
6473
allErrs = append(allErrs, field.Invalid(field.NewPath("hostsubnetlength"), clusterNet.HostSubnetLength, "subnet length must be at least 2"))
6574
}
75+
76+
if (clusterIPNet != nil) && (serviceIPNet != nil) && configapi.CIDRsOverlap(clusterIPNet.String(), serviceIPNet.String()) {
77+
allErrs = append(allErrs, field.Invalid(field.NewPath("serviceNetwork"), clusterNet.ServiceNetwork, "service network overlaps with cluster network"))
78+
}
79+
}
80+
} else {
81+
// "new" ClusterNetwork
82+
if clusterNet.Name == networkapi.ClusterNetworkDefault {
83+
if clusterNet.Network != clusterNet.ClusterNetworks[0].CIDR {
84+
allErrs = append(allErrs, field.Invalid(field.NewPath("network"), clusterNet.Network, "network must be identical to clusterNetworks[0].cidr"))
85+
}
86+
if clusterNet.HostSubnetLength != clusterNet.ClusterNetworks[0].HostSubnetLength {
87+
allErrs = append(allErrs, field.Invalid(field.NewPath("hostsubnetlength"), clusterNet.HostSubnetLength, "hostsubnetlength must be identical to clusterNetworks[0].hostSubnetLength"))
88+
}
89+
} else if clusterNet.Network != "" || clusterNet.HostSubnetLength != 0 {
90+
if clusterNet.Network != clusterNet.ClusterNetworks[0].CIDR || clusterNet.HostSubnetLength != clusterNet.ClusterNetworks[0].HostSubnetLength {
91+
allErrs = append(allErrs, field.Invalid(field.NewPath("clusterNetworks").Index(0), clusterNet.ClusterNetworks[0], "network and hostsubnetlength must be unset or identical to clusterNetworks[0]"))
92+
}
6693
}
6794
}
6895

69-
if len(clusterNet.ClusterNetworks) == 0 && len(clusterNet.Network) == 0 {
70-
allErrs = append(allErrs, field.Invalid(field.NewPath("clusterNetworks"), clusterNet.ClusterNetworks, "must have at least one cluster network CIDR"))
71-
}
72-
serviceIPNet, err := validateCIDRv4(clusterNet.ServiceNetwork)
73-
if err != nil {
74-
allErrs = append(allErrs, field.Invalid(field.NewPath("serviceNetwork"), clusterNet.ServiceNetwork, err.Error()))
75-
}
7696
for i, cn := range clusterNet.ClusterNetworks {
7797
clusterIPNet, err := validateCIDRv4(cn.CIDR)
7898
if err != nil {
@@ -103,7 +123,7 @@ func ValidateClusterNetwork(clusterNet *networkapi.ClusterNetwork) field.ErrorLi
103123
allErrs = append(allErrs, field.Invalid(field.NewPath("network"), clusterNet.Network, "cannot change the default ClusterNetwork record via API."))
104124
}
105125
if clusterNet.HostSubnetLength != defaultClusterNetwork.HostSubnetLength {
106-
allErrs = append(allErrs, field.Invalid(field.NewPath("hostSubnetLength"), clusterNet.HostSubnetLength, "cannot change the default ClusterNetwork record via API."))
126+
allErrs = append(allErrs, field.Invalid(field.NewPath("hostsubnetlength"), clusterNet.HostSubnetLength, "cannot change the default ClusterNetwork record via API."))
107127
}
108128
if !reflect.DeepEqual(clusterNet.ClusterNetworks, defaultClusterNetwork.ClusterNetworks) {
109129
allErrs = append(allErrs, field.Invalid(field.NewPath("ClusterNetworks"), clusterNet.ClusterNetworks, "cannot change the default ClusterNetwork record via API"))

‎pkg/network/apis/network/validation/validation_test.go

+83-27
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@ func TestValidateClusterNetwork(t *testing.T) {
2626
expectedErrors: 0,
2727
},
2828
{
29-
name: "Good one old network and hostsubnetlength set ",
29+
name: "Good one old network and hostsubnetlength set",
3030
cn: &networkapi.ClusterNetwork{
3131
ObjectMeta: metav1.ObjectMeta{Name: "any"},
3232
Network: "10.20.0.0/16",
@@ -37,7 +37,29 @@ func TestValidateClusterNetwork(t *testing.T) {
3737
expectedErrors: 0,
3838
},
3939
{
40-
name: "only old network set ",
40+
name: "old network set incorrectly",
41+
cn: &networkapi.ClusterNetwork{
42+
ObjectMeta: metav1.ObjectMeta{Name: "any"},
43+
Network: "10.30.0.0/16",
44+
HostSubnetLength: 8,
45+
ClusterNetworks: []networkapi.ClusterNetworkEntry{{CIDR: "10.20.0.0/16", HostSubnetLength: 8}},
46+
ServiceNetwork: "172.30.0.0/16",
47+
},
48+
expectedErrors: 1,
49+
},
50+
{
51+
name: "old hostsubnetlength set incorrectly",
52+
cn: &networkapi.ClusterNetwork{
53+
ObjectMeta: metav1.ObjectMeta{Name: "any"},
54+
Network: "10.20.0.0/16",
55+
HostSubnetLength: 9,
56+
ClusterNetworks: []networkapi.ClusterNetworkEntry{{CIDR: "10.20.0.0/16", HostSubnetLength: 8}},
57+
ServiceNetwork: "172.30.0.0/16",
58+
},
59+
expectedErrors: 1,
60+
},
61+
{
62+
name: "only old network set",
4163
cn: &networkapi.ClusterNetwork{
4264
ObjectMeta: metav1.ObjectMeta{Name: "any"},
4365
Network: "10.20.0.0/16",
@@ -47,7 +69,7 @@ func TestValidateClusterNetwork(t *testing.T) {
4769
expectedErrors: 1,
4870
},
4971
{
50-
name: "only old hostsubnetlength set ",
72+
name: "only old hostsubnetlength set",
5173
cn: &networkapi.ClusterNetwork{
5274
ObjectMeta: metav1.ObjectMeta{Name: "any"},
5375
HostSubnetLength: 8,
@@ -185,10 +207,12 @@ func TestValidateClusterNetwork(t *testing.T) {
185207

186208
func TestSetDefaultClusterNetwork(t *testing.T) {
187209
defaultClusterNetwork := networkapi.ClusterNetwork{
188-
ObjectMeta: metav1.ObjectMeta{Name: networkapi.ClusterNetworkDefault},
189-
ClusterNetworks: []networkapi.ClusterNetworkEntry{{CIDR: "10.20.0.0/16", HostSubnetLength: 8}},
190-
ServiceNetwork: "172.30.0.0/16",
191-
PluginName: "redhat/openshift-ovs-multitenant",
210+
ObjectMeta: metav1.ObjectMeta{Name: networkapi.ClusterNetworkDefault},
211+
ClusterNetworks: []networkapi.ClusterNetworkEntry{{CIDR: "10.20.0.0/16", HostSubnetLength: 8}},
212+
ServiceNetwork: "172.30.0.0/16",
213+
PluginName: "redhat/openshift-ovs-multitenant",
214+
Network: "10.20.0.0/16",
215+
HostSubnetLength: 8,
192216
}
193217
SetDefaultClusterNetwork(defaultClusterNetwork)
194218

@@ -205,52 +229,84 @@ func TestSetDefaultClusterNetwork(t *testing.T) {
205229
{
206230
name: "Wrong Network",
207231
cn: &networkapi.ClusterNetwork{
208-
ObjectMeta: metav1.ObjectMeta{Name: networkapi.ClusterNetworkDefault},
209-
ClusterNetworks: []networkapi.ClusterNetworkEntry{{CIDR: "10.30.0.0/16", HostSubnetLength: 8}},
210-
ServiceNetwork: "172.30.0.0/16",
211-
PluginName: "redhat/openshift-ovs-multitenant",
232+
ObjectMeta: metav1.ObjectMeta{Name: networkapi.ClusterNetworkDefault},
233+
ClusterNetworks: []networkapi.ClusterNetworkEntry{{CIDR: "10.30.0.0/16", HostSubnetLength: 8}},
234+
ServiceNetwork: "172.30.0.0/16",
235+
PluginName: "redhat/openshift-ovs-multitenant",
236+
Network: "10.30.0.0/16",
237+
HostSubnetLength: 8,
212238
},
213-
expectedErrors: 1,
239+
expectedErrors: 2,
214240
},
215241
{
216242
name: "Additional Network",
217243
cn: &networkapi.ClusterNetwork{
218-
ObjectMeta: metav1.ObjectMeta{Name: networkapi.ClusterNetworkDefault},
219-
ClusterNetworks: []networkapi.ClusterNetworkEntry{{CIDR: "10.20.0.0/16", HostSubnetLength: 8}, {CIDR: "10.30.0.0/16", HostSubnetLength: 8}},
220-
ServiceNetwork: "172.30.0.0/16",
221-
PluginName: "redhat/openshift-ovs-multitenant",
244+
ObjectMeta: metav1.ObjectMeta{Name: networkapi.ClusterNetworkDefault},
245+
ClusterNetworks: []networkapi.ClusterNetworkEntry{{CIDR: "10.20.0.0/16", HostSubnetLength: 8}, {CIDR: "10.30.0.0/16", HostSubnetLength: 8}},
246+
ServiceNetwork: "172.30.0.0/16",
247+
PluginName: "redhat/openshift-ovs-multitenant",
248+
Network: "10.20.0.0/16",
249+
HostSubnetLength: 8,
222250
},
223251
expectedErrors: 1,
224252
},
225253
{
226254
name: "Wrong HostSubnetLength",
227255
cn: &networkapi.ClusterNetwork{
228-
ObjectMeta: metav1.ObjectMeta{Name: networkapi.ClusterNetworkDefault},
229-
ClusterNetworks: []networkapi.ClusterNetworkEntry{{CIDR: "10.20.0.0/16", HostSubnetLength: 9}},
230-
ServiceNetwork: "172.30.0.0/16",
231-
PluginName: "redhat/openshift-ovs-multitenant",
256+
ObjectMeta: metav1.ObjectMeta{Name: networkapi.ClusterNetworkDefault},
257+
ClusterNetworks: []networkapi.ClusterNetworkEntry{{CIDR: "10.20.0.0/16", HostSubnetLength: 9}},
258+
ServiceNetwork: "172.30.0.0/16",
259+
PluginName: "redhat/openshift-ovs-multitenant",
260+
Network: "10.20.0.0/16",
261+
HostSubnetLength: 9,
232262
},
233-
expectedErrors: 1,
263+
expectedErrors: 2,
234264
},
235265
{
236266
name: "Wrong ServiceNetwork",
237267
cn: &networkapi.ClusterNetwork{
238-
ObjectMeta: metav1.ObjectMeta{Name: networkapi.ClusterNetworkDefault},
239-
ClusterNetworks: []networkapi.ClusterNetworkEntry{{CIDR: "10.20.0.0/16", HostSubnetLength: 8}},
240-
ServiceNetwork: "172.20.0.0/16",
241-
PluginName: "redhat/openshift-ovs-multitenant",
268+
ObjectMeta: metav1.ObjectMeta{Name: networkapi.ClusterNetworkDefault},
269+
ClusterNetworks: []networkapi.ClusterNetworkEntry{{CIDR: "10.20.0.0/16", HostSubnetLength: 8}},
270+
ServiceNetwork: "172.20.0.0/16",
271+
PluginName: "redhat/openshift-ovs-multitenant",
272+
Network: "10.20.0.0/16",
273+
HostSubnetLength: 8,
242274
},
243275
expectedErrors: 1,
244276
},
245277
{
246278
name: "Wrong PluginName",
279+
cn: &networkapi.ClusterNetwork{
280+
ObjectMeta: metav1.ObjectMeta{Name: networkapi.ClusterNetworkDefault},
281+
ClusterNetworks: []networkapi.ClusterNetworkEntry{{CIDR: "10.20.0.0/16", HostSubnetLength: 8}},
282+
ServiceNetwork: "172.30.0.0/16",
283+
PluginName: "redhat/openshift-ovs-subnet",
284+
Network: "10.20.0.0/16",
285+
HostSubnetLength: 8,
286+
},
287+
expectedErrors: 1,
288+
},
289+
{
290+
name: "Wrong legacy Network",
291+
cn: &networkapi.ClusterNetwork{
292+
ObjectMeta: metav1.ObjectMeta{Name: networkapi.ClusterNetworkDefault},
293+
ClusterNetworks: []networkapi.ClusterNetworkEntry{{CIDR: "10.20.0.0/16", HostSubnetLength: 8}},
294+
ServiceNetwork: "172.30.0.0/16",
295+
PluginName: "redhat/openshift-ovs-multitenant",
296+
Network: "10.30.0.0/16",
297+
HostSubnetLength: 8,
298+
},
299+
expectedErrors: 2,
300+
},
301+
{
302+
name: "Missing legacy fields",
247303
cn: &networkapi.ClusterNetwork{
248304
ObjectMeta: metav1.ObjectMeta{Name: networkapi.ClusterNetworkDefault},
249305
ClusterNetworks: []networkapi.ClusterNetworkEntry{{CIDR: "10.20.0.0/16", HostSubnetLength: 8}},
250306
ServiceNetwork: "172.30.0.0/16",
251-
PluginName: "redhat/openshift-ovs-subnet",
307+
PluginName: "redhat/openshift-ovs-multitenant",
252308
},
253-
expectedErrors: 1,
309+
expectedErrors: 4,
254310
},
255311
}
256312

‎pkg/network/master/master.go

+9-2
Original file line numberDiff line numberDiff line change
@@ -55,13 +55,16 @@ func Start(networkConfig osconfigapi.MasterNetworkConfig, networkClient networkc
5555

5656
var err error
5757
var clusterNetworkEntries []networkapi.ClusterNetworkEntry
58-
for _, cidr := range networkConfig.ClusterNetworks {
59-
clusterNetworkEntries = append(clusterNetworkEntries, networkapi.ClusterNetworkEntry{CIDR: cidr.CIDR, HostSubnetLength: cidr.HostSubnetLength})
58+
for _, entry := range networkConfig.ClusterNetworks {
59+
clusterNetworkEntries = append(clusterNetworkEntries, networkapi.ClusterNetworkEntry{CIDR: entry.CIDR, HostSubnetLength: entry.HostSubnetLength})
6060
}
6161
master.networkInfo, err = common.ParseNetworkInfo(clusterNetworkEntries, networkConfig.ServiceNetworkCIDR)
6262
if err != nil {
6363
return err
6464
}
65+
if len(clusterNetworkEntries) == 0 {
66+
panic("No ClusterNetworks set in networkConfig; should have been defaulted in if not configured")
67+
}
6568

6669
configCN := &networkapi.ClusterNetwork{
6770
TypeMeta: metav1.TypeMeta{Kind: "ClusterNetwork"},
@@ -70,6 +73,10 @@ func Start(networkConfig osconfigapi.MasterNetworkConfig, networkClient networkc
7073
ClusterNetworks: clusterNetworkEntries,
7174
ServiceNetwork: networkConfig.ServiceNetworkCIDR,
7275
PluginName: networkConfig.NetworkPluginName,
76+
77+
// Need to set these for backward compat
78+
Network: clusterNetworkEntries[0].CIDR,
79+
HostSubnetLength: clusterNetworkEntries[0].HostSubnetLength,
7380
}
7481
osapivalidation.SetDefaultClusterNetwork(*configCN)
7582

0 commit comments

Comments
 (0)