Skip to content

Commit b185781

Browse files
author
Cecile Robert-Michon
committed
🐛 don't return nil in for loops
Update inboundnatrules.go
1 parent b2514b1 commit b185781

File tree

9 files changed

+275
-27
lines changed

9 files changed

+275
-27
lines changed

‎cloud/services/bastionhosts/bastionhosts.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -110,7 +110,7 @@ func (s *Service) Delete(ctx context.Context) error {
110110
err := s.Client.Delete(ctx, s.Scope.ResourceGroup(), bastionSpec.Name)
111111
if err != nil && azure.ResourceNotFound(err) {
112112
// already deleted
113-
return nil
113+
continue
114114
}
115115
if err != nil {
116116
return errors.Wrapf(err, "failed to delete Bastion Host %s in resource group %s", bastionSpec.Name, s.Scope.ResourceGroup())

‎cloud/services/bastionhosts/bastionhosts_test.go‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -309,10 +309,17 @@ func TestDeleteBastionHost(t *testing.T) {
309309
SubnetName: "my-subnet",
310310
PublicIPName: "my-publicip",
311311
},
312+
{
313+
Name: "my-bastionhost1",
314+
VNetName: "my-vnet",
315+
SubnetName: "my-subnet",
316+
PublicIPName: "my-publicip",
317+
},
312318
})
313319
s.ResourceGroup().AnyTimes().Return("my-rg")
314320
m.Delete(context.TODO(), "my-rg", "my-bastionhost").
315321
Return(autorest.NewErrorWithResponse("", "", &http.Response{StatusCode: 404}, "Not found"))
322+
m.Delete(context.TODO(), "my-rg", "my-bastionhost1")
316323
},
317324
},
318325
{

‎cloud/services/disks/disks.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,7 @@ func (s *Service) Delete(ctx context.Context) error {
3636
err := s.Client.Delete(ctx, s.Scope.ResourceGroup(), diskSpec.Name)
3737
if err != nil && azure.ResourceNotFound(err) {
3838
// already deleted
39-
return nil
39+
continue
4040
}
4141
if err != nil {
4242
return errors.Wrapf(err, "failed to delete disk %s in resource group %s", diskSpec.Name, s.Scope.ResourceGroup())

‎cloud/services/disks/disks_test.go‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -64,9 +64,13 @@ func TestDeleteDisk(t *testing.T) {
6464
{
6565
Name: "my-disk-1",
6666
},
67+
{
68+
Name: "my-disk-2",
69+
},
6770
})
6871
s.ResourceGroup().AnyTimes().Return("my-rg")
6972
m.Delete(context.TODO(), "my-rg", "my-disk-1").Return(autorest.NewErrorWithResponse("", "", &http.Response{StatusCode: 404}, "Not Found"))
73+
m.Delete(context.TODO(), "my-rg", "my-disk-2").Return(autorest.NewErrorWithResponse("", "", &http.Response{StatusCode: 404}, "Not Found"))
7074
},
7175
},
7276
{
@@ -78,6 +82,9 @@ func TestDeleteDisk(t *testing.T) {
7882
{
7983
Name: "my-disk-1",
8084
},
85+
{
86+
Name: "my-disk-2",
87+
},
8188
})
8289
s.ResourceGroup().AnyTimes().Return("my-rg")
8390
m.Delete(context.TODO(), "my-rg", "my-disk-1").Return(autorest.NewErrorWithResponse("", "", &http.Response{StatusCode: 500}, "Internal Server Error"))

‎cloud/services/inboundnatrules/inboundnatrules.go‎

Lines changed: 38 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -30,9 +30,6 @@ func (s *Service) Reconcile(ctx context.Context) error {
3030
for _, inboundNatSpec := range s.Scope.InboundNatSpecs() {
3131
s.Scope.V(2).Info("creating inbound NAT rule", "NAT rule", inboundNatSpec.Name)
3232

33-
var sshFrontendPort int32 = 22
34-
ports := make(map[int32]struct{})
35-
3633
lb, err := s.LoadBalancersClient.Get(ctx, s.Scope.ResourceGroup(), inboundNatSpec.LoadBalancerName)
3734
if err != nil {
3835
return errors.Wrapf(err, "failed to get Load Balancer %s", inboundNatSpec.LoadBalancerName)
@@ -41,28 +38,18 @@ func (s *Service) Reconcile(ctx context.Context) error {
4138
if lb.LoadBalancerPropertiesFormat == nil || lb.FrontendIPConfigurations == nil || lb.InboundNatRules == nil {
4239
return errors.Errorf("Could not get existing inbound NAT rules from load balancer %s properties", to.String(lb.Name))
4340
}
44-
for _, v := range *lb.InboundNatRules {
45-
if to.String(v.Name) == inboundNatSpec.Name {
46-
// Inbound NAT Rule already exists, nothing to do here.
47-
s.Scope.V(2).Info("NAT rule already exists", "NAT rule", inboundNatSpec.Name)
48-
return nil
49-
}
50-
ports[*v.InboundNatRulePropertiesFormat.FrontendPort] = struct{}{}
41+
42+
ports := make(map[int32]struct{})
43+
if s.natRuleExists(ports)(*lb.InboundNatRules, inboundNatSpec.Name) {
44+
// Inbound NAT Rule already exists, nothing to do here.
45+
continue
5146
}
52-
if _, ok := ports[22]; ok {
53-
var i int32
54-
found := false
55-
for i = 2201; i < 2220; i++ {
56-
if _, ok := ports[i]; !ok {
57-
sshFrontendPort = i
58-
found = true
59-
break
60-
}
61-
}
62-
if !found {
63-
return errors.Errorf("Failed to find available SSH Frontend port for NAT Rule %s in load balancer %s", inboundNatSpec.Name, to.String(lb.Name))
64-
}
47+
48+
sshFrontendPort, err := s.getAvailablePort(ports)
49+
if err != nil {
50+
return errors.Wrapf(err, "failed to find available SSH Frontend port for NAT Rule %s in load balancer %s", inboundNatSpec.Name, to.String(lb.Name))
6551
}
52+
6653
rule := network.InboundNatRule{
6754
Name: to.StringPtr(inboundNatSpec.Name),
6855
InboundNatRulePropertiesFormat: &network.InboundNatRulePropertiesFormat{
@@ -101,3 +88,31 @@ func (s *Service) Delete(ctx context.Context) error {
10188
}
10289
return nil
10390
}
91+
92+
func (s *Service) natRuleExists(ports map[int32]struct{}) func([]network.InboundNatRule, string) bool {
93+
return func(rules []network.InboundNatRule, name string) bool {
94+
for _, v := range rules {
95+
if to.String(v.Name) == name {
96+
s.Scope.V(2).Info("NAT rule already exists", "NAT rule", name)
97+
return true
98+
}
99+
ports[*v.InboundNatRulePropertiesFormat.FrontendPort] = struct{}{}
100+
}
101+
return false
102+
}
103+
}
104+
105+
func (s *Service) getAvailablePort(ports map[int32]struct{}) (int32, error) {
106+
var i int32 = 22
107+
if _, ok := ports[22]; ok {
108+
for i = 2201; i < 2220; i++ {
109+
if _, ok := ports[i]; !ok {
110+
s.Scope.V(2).Info("Found available port", "port", i)
111+
return i, nil
112+
}
113+
}
114+
return i, errors.Errorf("No available SSH Frontend ports")
115+
}
116+
s.Scope.V(2).Info("Found available port", "port", i)
117+
return i, nil
118+
}

‎cloud/services/inboundnatrules/inboundnatrules_test.go‎

Lines changed: 215 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -172,6 +172,10 @@ func TestReconcileInboundNATRule(t *testing.T) {
172172
Name: "my-machine-nat-rule",
173173
LoadBalancerName: "my-public-lb",
174174
},
175+
{
176+
Name: "my-other-nat-rule",
177+
LoadBalancerName: "my-other-public-lb",
178+
},
175179
})
176180
s.ResourceGroup().AnyTimes().Return("my-rg")
177181
s.Location().AnyTimes().Return("fake-location")
@@ -202,7 +206,19 @@ func TestReconcileInboundNATRule(t *testing.T) {
202206
},
203207
},
204208
},
205-
}}, nil))
209+
}}, nil),
210+
mLoadBalancer.Get(context.TODO(), "my-rg", "my-other-public-lb").Return(network.LoadBalancer{
211+
Name: to.StringPtr("my-other-public-lb"),
212+
ID: pointer.StringPtr("my-public-lb-id"),
213+
LoadBalancerPropertiesFormat: &network.LoadBalancerPropertiesFormat{
214+
FrontendIPConfigurations: &[]network.FrontendIPConfiguration{
215+
{
216+
ID: to.StringPtr("frontend-ip-config-id"),
217+
},
218+
},
219+
InboundNatRules: &[]network.InboundNatRule{},
220+
}}, nil),
221+
m.CreateOrUpdate(context.TODO(), "my-rg", "my-other-public-lb", "my-other-nat-rule", gomock.AssignableToTypeOf(network.InboundNatRule{})))
206222
},
207223
},
208224
}
@@ -325,3 +341,201 @@ func TestDeleteNetworkInterface(t *testing.T) {
325341
})
326342
}
327343
}
344+
345+
func TestNatRuleExists(t *testing.T) {
346+
testcases := []struct {
347+
name string
348+
ruleName string
349+
existingRules []network.InboundNatRule
350+
expectedResult bool
351+
expectedPorts map[int32]struct{}
352+
expect func(s *mock_inboundnatrules.MockInboundNatScopeMockRecorder,
353+
m *mock_inboundnatrules.MockClientMockRecorder, mLoadBalancer *mock_loadbalancers.MockClientMockRecorder)
354+
}{
355+
{
356+
name: "Rule exists",
357+
ruleName: "my-rule",
358+
existingRules: []network.InboundNatRule{
359+
{
360+
InboundNatRulePropertiesFormat: &network.InboundNatRulePropertiesFormat{
361+
FrontendPort: to.Int32Ptr(2201),
362+
},
363+
Name: to.StringPtr("some-rule"),
364+
},
365+
{
366+
InboundNatRulePropertiesFormat: &network.InboundNatRulePropertiesFormat{
367+
FrontendPort: to.Int32Ptr(22),
368+
},
369+
Name: to.StringPtr("my-rule"),
370+
},
371+
},
372+
expectedResult: true,
373+
expect: func(s *mock_inboundnatrules.MockInboundNatScopeMockRecorder,
374+
m *mock_inboundnatrules.MockClientMockRecorder, mLoadBalancer *mock_loadbalancers.MockClientMockRecorder) {
375+
s.V(gomock.AssignableToTypeOf(2)).Return(klogr.New())
376+
},
377+
},
378+
{
379+
name: "Rule doesn't exist",
380+
ruleName: "my-rule",
381+
existingRules: []network.InboundNatRule{
382+
{
383+
InboundNatRulePropertiesFormat: &network.InboundNatRulePropertiesFormat{
384+
FrontendPort: to.Int32Ptr(22),
385+
},
386+
Name: to.StringPtr("other-rule"),
387+
},
388+
{
389+
InboundNatRulePropertiesFormat: &network.InboundNatRulePropertiesFormat{
390+
FrontendPort: to.Int32Ptr(2205),
391+
},
392+
Name: to.StringPtr("other-rule-2"),
393+
},
394+
},
395+
expectedResult: false,
396+
expectedPorts: map[int32]struct{}{
397+
22: {},
398+
2205: {},
399+
},
400+
expect: func(s *mock_inboundnatrules.MockInboundNatScopeMockRecorder,
401+
m *mock_inboundnatrules.MockClientMockRecorder, mLoadBalancer *mock_loadbalancers.MockClientMockRecorder) {
402+
},
403+
},
404+
{
405+
name: "No rules exist",
406+
ruleName: "my-rule",
407+
existingRules: []network.InboundNatRule{},
408+
expectedResult: false,
409+
expectedPorts: map[int32]struct{}{},
410+
expect: func(s *mock_inboundnatrules.MockInboundNatScopeMockRecorder,
411+
m *mock_inboundnatrules.MockClientMockRecorder, mLoadBalancer *mock_loadbalancers.MockClientMockRecorder) {
412+
},
413+
},
414+
}
415+
416+
for _, tc := range testcases {
417+
tc := tc
418+
t.Run(tc.name, func(t *testing.T) {
419+
g := NewWithT(t)
420+
t.Parallel()
421+
mockCtrl := gomock.NewController(t)
422+
defer mockCtrl.Finish()
423+
scopeMock := mock_inboundnatrules.NewMockInboundNatScope(mockCtrl)
424+
clientMock := mock_inboundnatrules.NewMockClient(mockCtrl)
425+
loadBalancerMock := mock_loadbalancers.NewMockClient(mockCtrl)
426+
427+
tc.expect(scopeMock.EXPECT(), clientMock.EXPECT(), loadBalancerMock.EXPECT())
428+
429+
s := &Service{
430+
Scope: scopeMock,
431+
Client: clientMock,
432+
LoadBalancersClient: loadBalancerMock,
433+
}
434+
435+
ports := make(map[int32]struct{})
436+
exists := s.natRuleExists(ports)(tc.existingRules, tc.ruleName)
437+
g.Expect(exists).To(Equal(tc.expectedResult))
438+
if !exists {
439+
g.Expect(ports).To(Equal(tc.expectedPorts))
440+
}
441+
})
442+
}
443+
}
444+
445+
func TestGetAvailablePort(t *testing.T) {
446+
testcases := []struct {
447+
name string
448+
portsInput map[int32]struct{}
449+
expectedError string
450+
expectedPortResult int32
451+
expect func(s *mock_inboundnatrules.MockInboundNatScopeMockRecorder,
452+
m *mock_inboundnatrules.MockClientMockRecorder, mLoadBalancer *mock_loadbalancers.MockClientMockRecorder)
453+
}{
454+
{
455+
name: "Empty ports",
456+
portsInput: map[int32]struct{}{},
457+
expectedError: "",
458+
expectedPortResult: 22,
459+
expect: func(s *mock_inboundnatrules.MockInboundNatScopeMockRecorder,
460+
m *mock_inboundnatrules.MockClientMockRecorder, mLoadBalancer *mock_loadbalancers.MockClientMockRecorder) {
461+
s.V(gomock.AssignableToTypeOf(2)).Return(klogr.New())
462+
},
463+
},
464+
{
465+
name: "22 taken",
466+
portsInput: map[int32]struct{}{
467+
22: {},
468+
},
469+
expectedError: "",
470+
expectedPortResult: 2201,
471+
expect: func(s *mock_inboundnatrules.MockInboundNatScopeMockRecorder,
472+
m *mock_inboundnatrules.MockClientMockRecorder, mLoadBalancer *mock_loadbalancers.MockClientMockRecorder) {
473+
s.V(gomock.AssignableToTypeOf(2)).Return(klogr.New())
474+
},
475+
},
476+
{
477+
name: "Existing ports",
478+
portsInput: map[int32]struct{}{
479+
22: {},
480+
2201: {},
481+
2202: {},
482+
2204: {},
483+
},
484+
expectedError: "",
485+
expectedPortResult: 2203,
486+
expect: func(s *mock_inboundnatrules.MockInboundNatScopeMockRecorder,
487+
m *mock_inboundnatrules.MockClientMockRecorder, mLoadBalancer *mock_loadbalancers.MockClientMockRecorder) {
488+
s.V(gomock.AssignableToTypeOf(2)).Return(klogr.New())
489+
},
490+
},
491+
{
492+
name: "No ports available",
493+
portsInput: getFullPortsMap(),
494+
expectedError: "No available SSH Frontend ports",
495+
expectedPortResult: 0,
496+
expect: func(s *mock_inboundnatrules.MockInboundNatScopeMockRecorder,
497+
m *mock_inboundnatrules.MockClientMockRecorder, mLoadBalancer *mock_loadbalancers.MockClientMockRecorder) {
498+
},
499+
},
500+
}
501+
502+
for _, tc := range testcases {
503+
tc := tc
504+
t.Run(tc.name, func(t *testing.T) {
505+
g := NewWithT(t)
506+
t.Parallel()
507+
mockCtrl := gomock.NewController(t)
508+
defer mockCtrl.Finish()
509+
scopeMock := mock_inboundnatrules.NewMockInboundNatScope(mockCtrl)
510+
clientMock := mock_inboundnatrules.NewMockClient(mockCtrl)
511+
loadBalancerMock := mock_loadbalancers.NewMockClient(mockCtrl)
512+
513+
tc.expect(scopeMock.EXPECT(), clientMock.EXPECT(), loadBalancerMock.EXPECT())
514+
515+
s := &Service{
516+
Scope: scopeMock,
517+
Client: clientMock,
518+
LoadBalancersClient: loadBalancerMock,
519+
}
520+
521+
res, err := s.getAvailablePort(tc.portsInput)
522+
if tc.expectedError != "" {
523+
g.Expect(err).To(HaveOccurred())
524+
g.Expect(err).To(MatchError(tc.expectedError))
525+
} else {
526+
g.Expect(err).NotTo(HaveOccurred())
527+
g.Expect(res).To(Equal(tc.expectedPortResult))
528+
}
529+
})
530+
}
531+
}
532+
533+
func getFullPortsMap() map[int32]struct{} {
534+
res := map[int32]struct{}{
535+
22: {},
536+
}
537+
for i := 2201; i < 2220; i++ {
538+
res[int32(i)] = struct{}{}
539+
}
540+
return res
541+
}

‎cloud/services/networkinterfaces/networkinterfaces.go‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ package networkinterfaces
1818

1919
import (
2020
"context"
21+
2122
"github.com/Azure/azure-sdk-for-go/services/network/mgmt/2019-06-01/network"
2223
"github.com/Azure/go-autorest/autorest/to"
2324
"github.com/pkg/errors"

‎cloud/services/publicips/publicips.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -66,7 +66,7 @@ func (s *Service) Delete(ctx context.Context) error {
6666
err := s.Client.Delete(ctx, s.Scope.ResourceGroup(), ip.Name)
6767
if err != nil && azure.ResourceNotFound(err) {
6868
// already deleted
69-
return nil
69+
continue
7070
}
7171
if err != nil {
7272
return errors.Wrapf(err, "failed to delete public IP %s in resource group %s", ip.Name, s.Scope.ResourceGroup())

0 commit comments

Comments
 (0)