Skip to content

Commit b4a4a1b

Browse files
k8s-ci-robotk8s-ci-robot
authored
Merge pull request #594 from mskrocki/reduce
Removed unnecessary cidr validation.
2 parents 64c5860 + 0091a37 commit b4a4a1b

File tree

3 files changed

+48
-180
lines changed

3 files changed

+48
-180
lines changed

‎pkg/controller/nodeipam/ipam/BUILD

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,6 @@ go_library(
2323
"//pkg/util/node",
2424
"//pkg/util/taints",
2525
"//providers/gce",
26-
"//vendor/github.com/google/go-cmp/cmp",
2726
"//vendor/google.golang.org/api/compute/v1:compute",
2827
"//vendor/k8s.io/api/core/v1:core",
2928
"//vendor/k8s.io/apimachinery/pkg/api/errors",
@@ -87,6 +86,5 @@ go_test(
8786
"//vendor/k8s.io/cloud-provider-gcp/crd/client/network/clientset/versioned/fake",
8887
"//vendor/k8s.io/cloud-provider-gcp/crd/client/network/informers/externalversions",
8988
"//vendor/k8s.io/component-base/metrics/testutil",
90-
"//vendor/k8s.io/utils/net",
9189
],
9290
)

‎pkg/controller/nodeipam/ipam/cloud_cidr_allocator.go

Lines changed: 8 additions & 64 deletions
Original file line numberDiff line numberDiff line change
@@ -22,11 +22,9 @@ package ipam
2222
import (
2323
"context"
2424
"fmt"
25-
"net"
2625
"reflect"
2726
"time"
2827

29-
"github.com/google/go-cmp/cmp"
3028
"k8s.io/apimachinery/pkg/api/meta"
3129
"k8s.io/apimachinery/pkg/util/wait"
3230
"k8s.io/client-go/util/workqueue"
@@ -350,26 +348,15 @@ func (ca *cloudCIDRAllocator) updateCIDRAllocation(nodeName string) error {
350348
if err != nil {
351349
return fmt.Errorf("failed to parse strings %v as CIDRs: %v", cidrStrings, err)
352350
}
353-
354-
// TODO: revisit need of needPodCIDRsUpdate with current code base
355-
// additionally: spec.podCIDRs: Forbidden: node updates may not change podCIDR except from "" to valid
356-
needUpdate, err := needPodCIDRsUpdate(node, cidrs)
357-
if err != nil {
358-
return fmt.Errorf("err: %v, CIDRS: %v", err, cidrStrings)
359-
}
360-
if needUpdate {
361-
if node.Spec.PodCIDR != "" {
362-
klog.ErrorS(nil, "PodCIDR being reassigned!", "nodeName", node.Name, "node.Spec.PodCIDRs", node.Spec.PodCIDRs, "cidrStrings", cidrStrings)
363-
// We fall through and set the CIDR despite this error. This
364-
// implements the same logic as implemented in the
365-
// rangeAllocator.
366-
//
367-
// See https://github.com/kubernetes/kubernetes/pull/42147#discussion_r103357248
351+
if len(cidrs) > 1 {
352+
if dualStack, _ := netutils.IsDualStackCIDRs(cidrs); !dualStack {
353+
return fmt.Errorf("err: IPs are not dual stack, CIDRS: %v", cidrStrings)
368354
}
369-
node.Spec.PodCIDR = cidrStrings[0]
370-
node.Spec.PodCIDRs = cidrStrings
371355
}
372356

357+
node.Spec.PodCIDR = cidrStrings[0]
358+
node.Spec.PodCIDRs = cidrStrings
359+
373360
err = ca.updateNodeCIDR(node, oldNode)
374361
if err != nil {
375362
return err
@@ -434,18 +421,13 @@ func (ca *cloudCIDRAllocator) updateNodeCIDR(node, oldNode *v1.Node) error {
434421

435422
// update Spec.podCIDR
436423
if !reflect.DeepEqual(node.Spec, oldNode.Spec) {
437-
// TODO: remove the retry since it is handled by the reconciliation loop
438-
for i := 0; i < cidrUpdateRetries; i++ {
439-
if err = utilnode.PatchNodeCIDRs(ca.client, types.NodeName(node.Name), node.Spec.PodCIDRs); err == nil {
440-
klog.InfoS("Set the node PodCIDRs", "nodeName", node.Name, "cidrStrings", node.Spec.PodCIDRs)
441-
break
442-
}
443-
}
424+
err = utilnode.PatchNodeCIDRs(ca.client, types.NodeName(node.Name), node.Spec.PodCIDRs)
444425
if err != nil {
445426
nodeutil.RecordNodeStatusChange(ca.recorder, node, "CIDRAssignmentFailed")
446427
klog.ErrorS(err, "Failed to update the node PodCIDR after multiple attempts", "nodeName", node.Name, "cidrStrings", node.Spec.PodCIDRs)
447428
return err
448429
}
430+
klog.InfoS("Set the node PodCIDRs", "nodeName", node.Name, "cidrStrings", node.Spec.PodCIDRs)
449431
}
450432

451433
// Update Conditions
@@ -463,44 +445,6 @@ func (ca *cloudCIDRAllocator) updateNodeCIDR(node, oldNode *v1.Node) error {
463445
return err
464446
}
465447

466-
func needPodCIDRsUpdate(node *v1.Node, podCIDRs []*net.IPNet) (bool, error) {
467-
if node.Spec.PodCIDR == "" {
468-
return true, nil
469-
}
470-
_, nodePodCIDR, err := net.ParseCIDR(node.Spec.PodCIDR)
471-
if err != nil {
472-
klog.ErrorS(err, "Found invalid node.Spec.PodCIDR", "node.Spec.PodCIDR", node.Spec.PodCIDR)
473-
// We will try to overwrite with new CIDR(s)
474-
return true, nil
475-
}
476-
nodePodCIDRs, err := netutils.ParseCIDRs(node.Spec.PodCIDRs)
477-
if err != nil {
478-
klog.ErrorS(err, "Found invalid node.Spec.PodCIDRs", "node.Spec.PodCIDRs", node.Spec.PodCIDRs)
479-
// We will try to overwrite with new CIDR(s)
480-
return true, nil
481-
}
482-
483-
if len(podCIDRs) == 1 {
484-
if cmp.Equal(nodePodCIDR, podCIDRs[0]) {
485-
klog.V(4).InfoS("Node already has allocated CIDR. It matches the proposed one.", "nodeName", node.Name, "podCIDRs[0]", podCIDRs[0])
486-
return false, nil
487-
}
488-
} else if len(nodePodCIDRs) == len(podCIDRs) {
489-
if dualStack, _ := netutils.IsDualStackCIDRs(podCIDRs); !dualStack {
490-
return false, fmt.Errorf("IPs are not dual stack")
491-
}
492-
for idx, cidr := range podCIDRs {
493-
if !cmp.Equal(nodePodCIDRs[idx], cidr) {
494-
return true, nil
495-
}
496-
}
497-
klog.V(4).InfoS("Node already has allocated CIDRs. It matches the proposed one.", "nodeName", node.Name, "podCIDRs", podCIDRs)
498-
return false, nil
499-
}
500-
501-
return true, nil
502-
}
503-
504448
func (ca *cloudCIDRAllocator) ReleaseCIDR(node *v1.Node) error {
505449
klog.V(2).Infof("Node %v PodCIDR (%v) will be released by external cloud provider (not managed by controller)",
506450
node.Name, node.Spec.PodCIDR)

‎pkg/controller/nodeipam/ipam/cloud_cidr_allocator_test.go

Lines changed: 40 additions & 114 deletions
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,6 @@ import (
4242
"k8s.io/cloud-provider-gcp/pkg/controller/testutil"
4343
"k8s.io/cloud-provider-gcp/providers/gce"
4444
metricsUtil "k8s.io/component-base/metrics/testutil"
45-
netutils "k8s.io/utils/net"
4645
)
4746

4847
const (
@@ -98,10 +97,6 @@ func TestBoundedRetries(t *testing.T) {
9897
}
9998
}
10099

101-
func withinExpectedRange(got time.Duration, expected time.Duration) bool {
102-
return got >= expected/2 && got <= 3*expected/2
103-
}
104-
105100
func TestUpdateCIDRAllocation(t *testing.T) {
106101
tests := []struct {
107102
name string
@@ -131,7 +126,7 @@ func TestUpdateCIDRAllocation(t *testing.T) {
131126
nodeChanges: func(node *v1.Node) {},
132127
},
133128
{
134-
name: "provider not set",
129+
name: "want error - provider not set",
135130
fakeNodeHandler: &testutil.FakeNodeHandler{
136131
Existing: []*v1.Node{
137132
{
@@ -152,7 +147,7 @@ func TestUpdateCIDRAllocation(t *testing.T) {
152147
expectErrMsg: "doesn't have providerID",
153148
},
154149
{
155-
name: "node not found in gce by provider",
150+
name: "want error - node not found in gce by provider",
156151
fakeNodeHandler: &testutil.FakeNodeHandler{
157152
Existing: []*v1.Node{
158153
{
@@ -176,7 +171,7 @@ func TestUpdateCIDRAllocation(t *testing.T) {
176171
expectErrMsg: "failed to get instance from provider",
177172
},
178173
{
179-
name: "gce node has no networks",
174+
name: "want error - gce node has no networks",
180175
fakeNodeHandler: &testutil.FakeNodeHandler{
181176
Existing: []*v1.Node{
182177
{
@@ -287,7 +282,7 @@ func TestUpdateCIDRAllocation(t *testing.T) {
287282
expectedUpdate: true,
288283
},
289284
{
290-
name: "incorrect cidr",
285+
name: "want error - incorrect cidr",
291286
fakeNodeHandler: &testutil.FakeNodeHandler{
292287
Existing: []*v1.Node{
293288
{
@@ -320,6 +315,40 @@ func TestUpdateCIDRAllocation(t *testing.T) {
320315
expectErr: true,
321316
expectErrMsg: "failed to parse strings",
322317
},
318+
{
319+
name: "want error - incorrect dualstack cidr",
320+
fakeNodeHandler: &testutil.FakeNodeHandler{
321+
Existing: []*v1.Node{
322+
{
323+
ObjectMeta: metav1.ObjectMeta{
324+
Name: "test",
325+
},
326+
Spec: v1.NodeSpec{
327+
ProviderID: "gce://test-project/us-central1-b/test",
328+
},
329+
},
330+
},
331+
Clientset: fake.NewSimpleClientset(),
332+
},
333+
gceInstance: []*compute.Instance{
334+
{
335+
Name: "test",
336+
NetworkInterfaces: []*compute.NetworkInterface{
337+
{
338+
Ipv6Address: "10.10.1.0",
339+
AliasIpRanges: []*compute.AliasIpRange{
340+
{
341+
IpCidrRange: "192.168.1.0/24",
342+
},
343+
},
344+
},
345+
},
346+
},
347+
},
348+
nodeChanges: func(node *v1.Node) {},
349+
expectErr: true,
350+
expectErrMsg: "err: IPs are not dual stack",
351+
},
323352
{
324353
name: "node already configured",
325354
fakeNodeHandler: &testutil.FakeNodeHandler{
@@ -804,7 +833,7 @@ func TestUpdateCIDRAllocation(t *testing.T) {
804833
},
805834
},
806835
{
807-
name: "[mn] [invalid] - node with cidrs in incorrect format",
836+
name: "[mn] want error - node with cidrs in incorrect format",
808837
networks: []*networkv1.Network{
809838
network(networkv1.DefaultPodNetworkName, defaultGKENetworkParamsName, true),
810839
network(redNetworkName, redGKENetworkParamsName, true),
@@ -860,7 +889,7 @@ func TestUpdateCIDRAllocation(t *testing.T) {
860889
expectErrMsg: "invalid CIDR address",
861890
},
862891
{
863-
name: "[mn] one additional network with /32 cidr",
892+
name: "[mn] want error - one additional network with /32 cidr",
864893
networks: []*networkv1.Network{
865894
network(networkv1.DefaultPodNetworkName, defaultGKENetworkParamsName, true),
866895
network(redNetworkName, redGKENetworkParamsName, true),
@@ -1321,106 +1350,3 @@ func interfaces(network, subnetwork, networkIP string, aliasIPRanges []*compute.
13211350
NetworkIP: networkIP,
13221351
}
13231352
}
1324-
1325-
func TestNeedPodCIDRsUpdate(t *testing.T) {
1326-
for _, tc := range []struct {
1327-
desc string
1328-
cidrs []string
1329-
nodePodCIDR string
1330-
nodePodCIDRs []string
1331-
want bool
1332-
wantErr bool
1333-
}{
1334-
{
1335-
desc: "want error - invalid cidr",
1336-
cidrs: []string{"10.10.10.0/24"},
1337-
nodePodCIDR: "10.10..0/24",
1338-
nodePodCIDRs: []string{"10.10..0/24"},
1339-
want: true,
1340-
},
1341-
{
1342-
desc: "want error - cidr len 2 but not dual stack",
1343-
cidrs: []string{"10.10.10.0/24", "10.10.11.0/24"},
1344-
nodePodCIDR: "10.10.10.0/24",
1345-
nodePodCIDRs: []string{"10.10.10.0/24", "2001:db8::/64"},
1346-
wantErr: true,
1347-
},
1348-
{
1349-
desc: "want false - matching v4 only cidr",
1350-
cidrs: []string{"10.10.10.0/24"},
1351-
nodePodCIDR: "10.10.10.0/24",
1352-
nodePodCIDRs: []string{"10.10.10.0/24"},
1353-
want: false,
1354-
},
1355-
{
1356-
desc: "want false - nil node.Spec.PodCIDR",
1357-
cidrs: []string{"10.10.10.0/24"},
1358-
want: true,
1359-
},
1360-
{
1361-
desc: "want true - non matching v4 only cidr",
1362-
cidrs: []string{"10.10.10.0/24"},
1363-
nodePodCIDR: "10.10.11.0/24",
1364-
nodePodCIDRs: []string{"10.10.11.0/24"},
1365-
want: true,
1366-
},
1367-
{
1368-
desc: "want false - matching v4 and v6 cidrs",
1369-
cidrs: []string{"10.10.10.0/24", "2001:db8::/64"},
1370-
nodePodCIDR: "10.10.10.0/24",
1371-
nodePodCIDRs: []string{"10.10.10.0/24", "2001:db8::/64"},
1372-
want: false,
1373-
},
1374-
{
1375-
desc: "want false - matching v4 and v6 cidrs, different strings but same CIDRs",
1376-
cidrs: []string{"10.10.10.0/24", "2001:db8::/64"},
1377-
nodePodCIDR: "10.10.10.0/24",
1378-
nodePodCIDRs: []string{"10.10.10.0/24", "2001:db8:0::/64"},
1379-
want: false,
1380-
},
1381-
{
1382-
desc: "want true - matching v4 and non matching v6 cidrs",
1383-
cidrs: []string{"10.10.10.0/24", "2001:db8::/64"},
1384-
nodePodCIDR: "10.10.10.0/24",
1385-
nodePodCIDRs: []string{"10.10.10.0/24", "2001:dba::/64"},
1386-
want: true,
1387-
},
1388-
{
1389-
desc: "want true - nil node.Spec.PodCIDRs",
1390-
cidrs: []string{"10.10.10.0/24", "2001:db8::/64"},
1391-
want: true,
1392-
},
1393-
{
1394-
desc: "want true - matching v6 and non matching v4 cidrs",
1395-
cidrs: []string{"10.10.10.0/24", "2001:db8::/64"},
1396-
nodePodCIDR: "10.10.1.0/24",
1397-
nodePodCIDRs: []string{"10.10.1.0/24", "2001:db8::/64"},
1398-
want: true,
1399-
},
1400-
{
1401-
desc: "want true - missing v6",
1402-
cidrs: []string{"10.10.10.0/24", "2001:db8::/64"},
1403-
nodePodCIDR: "10.10.10.0/24",
1404-
nodePodCIDRs: []string{"10.10.10.0/24"},
1405-
want: true,
1406-
},
1407-
} {
1408-
var node v1.Node
1409-
node.Spec.PodCIDR = tc.nodePodCIDR
1410-
node.Spec.PodCIDRs = tc.nodePodCIDRs
1411-
netCIDRs, err := netutils.ParseCIDRs(tc.cidrs)
1412-
if err != nil {
1413-
t.Errorf("failed to parse %v as CIDRs: %v", tc.cidrs, err)
1414-
}
1415-
1416-
t.Run(tc.desc, func(t *testing.T) {
1417-
got, err := needPodCIDRsUpdate(&node, netCIDRs)
1418-
if tc.wantErr == (err == nil) {
1419-
t.Errorf("err: %v, wantErr: %v", err, tc.wantErr)
1420-
}
1421-
if err == nil && got != tc.want {
1422-
t.Errorf("got: %v, want: %v", got, tc.want)
1423-
}
1424-
})
1425-
}
1426-
}

0 commit comments

Comments
 (0)