Skip to content

Commit 0a80fea

Browse files
FedomnFedomn
committed
Revert "fix dangling TargetGroups by fetching TargetGroups before reconcile and delete"
1 parent 78594e6 commit 0a80fea

File tree

3 files changed

+97
-49
lines changed

3 files changed

+97
-49
lines changed

‎internal/alb/lb/loadbalancer.go

Lines changed: 2 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -137,18 +137,14 @@ func (controller *defaultController) Reconcile(ctx context.Context, ingress *ext
137137
}
138138
}
139139

140-
currentTargetGroups, err := controller.cloud.GetTargetGroupsByLbArn(ctx, lbArn)
141-
if err != nil {
142-
return nil, fmt.Errorf("failed to get current targetGroups due to %v", err)
143-
}
144140
tgGroup, err := controller.tgGroupController.Reconcile(ctx, ingress)
145141
if err != nil {
146142
return nil, fmt.Errorf("failed to reconcile targetGroups due to %v", err)
147143
}
148144
if err := controller.lsGroupController.Reconcile(ctx, lbArn, ingress, tgGroup); err != nil {
149145
return nil, fmt.Errorf("failed to reconcile listeners due to %v", err)
150146
}
151-
if err := controller.tgGroupController.GC(ctx, currentTargetGroups, tgGroup); err != nil {
147+
if err := controller.tgGroupController.GC(ctx, lbArn, tgGroup); err != nil {
152148
return nil, fmt.Errorf("failed to GC targetGroups due to %v", err)
153149
}
154150

@@ -168,14 +164,10 @@ func (controller *defaultController) Delete(ctx context.Context, ingressKey type
168164
return fmt.Errorf("failed to find existing LoadBalancer due to %v", err)
169165
}
170166
if instance != nil {
171-
currentTargetGroups, err := controller.cloud.GetTargetGroupsByLbArn(ctx, aws.StringValue(instance.LoadBalancerArn))
172-
if err != nil {
173-
return fmt.Errorf("failed to get current targetGroups due to %v", err)
174-
}
175167
if err = controller.lsGroupController.Delete(ctx, aws.StringValue(instance.LoadBalancerArn)); err != nil {
176168
return fmt.Errorf("failed to delete listeners due to %v", err)
177169
}
178-
if err = controller.tgGroupController.Delete(ctx, currentTargetGroups); err != nil {
170+
if err = controller.tgGroupController.Delete(ctx, aws.StringValue(instance.LoadBalancerArn)); err != nil {
179171
return fmt.Errorf("failed to GC targetGroups due to %v", err)
180172
}
181173

‎internal/alb/tg/targetgroup_group.go

Lines changed: 11 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -25,10 +25,10 @@ type GroupController interface {
2525
Reconcile(ctx context.Context, ingress *extensions.Ingress) (TargetGroupGroup, error)
2626

2727
// GC will delete unused targetGroups matched by tag selector
28-
GC(ctx context.Context, currentTargetGroups []*elbv2.TargetGroup, tgGroup TargetGroupGroup) error
28+
GC(ctx context.Context, lbArn string, tgGroup TargetGroupGroup) error
2929

3030
// Delete will delete all targetGroups created for ingress
31-
Delete(ctx context.Context, currentTargetGroups []*elbv2.TargetGroup) error
31+
Delete(ctx context.Context, lbArn string) error
3232
}
3333

3434
// NewGroupController creates an GroupController
@@ -81,15 +81,18 @@ func (controller *defaultGroupController) Reconcile(ctx context.Context, ingress
8181
}, nil
8282
}
8383

84-
func (controller *defaultGroupController) GC(ctx context.Context, currentTargetGroups []*elbv2.TargetGroup, tgGroup TargetGroupGroup) error {
84+
func (controller *defaultGroupController) GC(ctx context.Context, lbArn string, tgGroup TargetGroupGroup) error {
8585
usedExternalTGARNs := sets.NewString(tgGroup.externalTGARNs...)
8686
usedServiceTGARNs := sets.NewString()
8787
for _, tg := range tgGroup.TGByBackend {
8888
usedServiceTGARNs.Insert(tg.Arn)
8989
}
90-
91-
arns := make([]string, 0, len(currentTargetGroups))
92-
for _, tg := range currentTargetGroups {
90+
targetGroups, err := controller.cloud.GetTargetGroupsByLbArn(ctx, lbArn)
91+
if err != nil {
92+
return fmt.Errorf("failed to get targetGroups due to %v", err)
93+
}
94+
arns := make([]string, 0, len(targetGroups))
95+
for _, tg := range targetGroups {
9396
arns = append(arns, aws.StringValue(tg.TargetGroupArn))
9497
}
9598

@@ -110,8 +113,8 @@ func (controller *defaultGroupController) GC(ctx context.Context, currentTargetG
110113
return nil
111114
}
112115

113-
func (controller *defaultGroupController) Delete(ctx context.Context, currentTargetGroups []*elbv2.TargetGroup) error {
114-
return controller.GC(ctx, currentTargetGroups, TargetGroupGroup{})
116+
func (controller *defaultGroupController) Delete(ctx context.Context, lbArn string) error {
117+
return controller.GC(ctx, lbArn, TargetGroupGroup{})
115118
}
116119

117120
// ExtractTargetGroupBackends returns backends for Ingress.

‎internal/alb/tg/targetgroup_group_test.go

Lines changed: 84 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,12 @@ type TGReconcileCall struct {
2424
Err error
2525
}
2626

27+
type GetTargetGroupsByLoadBalancerCall struct {
28+
LbArn string
29+
TargetGroups []*elbv2.TargetGroup
30+
Err error
31+
}
32+
2733
type DeleteTargetGroupByArnCall struct {
2834
Arn string
2935
Err error
@@ -527,12 +533,13 @@ func TestDefaultGroupController_Reconcile(t *testing.T) {
527533
}
528534

529535
func TestDefaultGroupController_GC(t *testing.T) {
536+
lbArn := "lbArn"
530537
for _, tc := range []struct {
531-
Name string
532-
TGGroup TargetGroupGroup
533-
CurrentTargetGroups []*elbv2.TargetGroup
534-
DeleteTargetGroupByArnCalls []DeleteTargetGroupByArnCall
535-
ExpectedError error
538+
Name string
539+
TGGroup TargetGroupGroup
540+
GetTargetGroupsByLoadBalancerCall *GetTargetGroupsByLoadBalancerCall
541+
DeleteTargetGroupByArnCalls []DeleteTargetGroupByArnCall
542+
ExpectedError error
536543
}{
537544
{
538545
Name: "GC succeeds",
@@ -544,10 +551,13 @@ func TestDefaultGroupController_GC(t *testing.T) {
544551
}: {Arn: "arn1"},
545552
},
546553
},
547-
CurrentTargetGroups: []*elbv2.TargetGroup{
548-
{TargetGroupArn: aws.String("arn1")},
549-
{TargetGroupArn: aws.String("arn2")},
550-
{TargetGroupArn: aws.String("arn3")},
554+
GetTargetGroupsByLoadBalancerCall: &GetTargetGroupsByLoadBalancerCall{
555+
LbArn: lbArn,
556+
TargetGroups: []*elbv2.TargetGroup{
557+
{TargetGroupArn: aws.String("arn1")},
558+
{TargetGroupArn: aws.String("arn2")},
559+
{TargetGroupArn: aws.String("arn3")},
560+
},
551561
},
552562
DeleteTargetGroupByArnCalls: []DeleteTargetGroupByArnCall{
553563
{
@@ -569,17 +579,36 @@ func TestDefaultGroupController_GC(t *testing.T) {
569579
},
570580
externalTGARNs: []string{"arn3"},
571581
},
572-
CurrentTargetGroups: []*elbv2.TargetGroup{
573-
{TargetGroupArn: aws.String("arn1")},
574-
{TargetGroupArn: aws.String("arn2")},
575-
{TargetGroupArn: aws.String("arn3")},
582+
GetTargetGroupsByLoadBalancerCall: &GetTargetGroupsByLoadBalancerCall{
583+
LbArn: lbArn,
584+
TargetGroups: []*elbv2.TargetGroup{
585+
{TargetGroupArn: aws.String("arn1")},
586+
{TargetGroupArn: aws.String("arn2")},
587+
{TargetGroupArn: aws.String("arn3")},
588+
},
576589
},
577590
DeleteTargetGroupByArnCalls: []DeleteTargetGroupByArnCall{
578591
{
579592
Arn: "arn2",
580593
},
581594
},
582595
},
596+
{
597+
Name: "GC failed when fetch current targetGroups",
598+
TGGroup: TargetGroupGroup{
599+
TGByBackend: map[extensions.IngressBackend]TargetGroup{
600+
{
601+
ServiceName: "service1",
602+
ServicePort: intstr.FromInt(80),
603+
}: {Arn: "arn1"},
604+
},
605+
},
606+
GetTargetGroupsByLoadBalancerCall: &GetTargetGroupsByLoadBalancerCall{
607+
LbArn: lbArn,
608+
Err: errors.New("GetTargetGroupsByLbArn"),
609+
},
610+
ExpectedError: errors.New("failed to get targetGroups due to GetTargetGroupsByLbArn"),
611+
},
583612
{
584613
Name: "GC failed when deleting targetGroup",
585614
TGGroup: TargetGroupGroup{
@@ -590,10 +619,13 @@ func TestDefaultGroupController_GC(t *testing.T) {
590619
}: {Arn: "arn1"},
591620
},
592621
},
593-
CurrentTargetGroups: []*elbv2.TargetGroup{
594-
{TargetGroupArn: aws.String("arn1")},
595-
{TargetGroupArn: aws.String("arn2")},
596-
{TargetGroupArn: aws.String("arn3")},
622+
GetTargetGroupsByLoadBalancerCall: &GetTargetGroupsByLoadBalancerCall{
623+
LbArn: lbArn,
624+
TargetGroups: []*elbv2.TargetGroup{
625+
{TargetGroupArn: aws.String("arn1")},
626+
{TargetGroupArn: aws.String("arn2")},
627+
{TargetGroupArn: aws.String("arn3")},
628+
},
597629
},
598630
DeleteTargetGroupByArnCalls: []DeleteTargetGroupByArnCall{
599631
{
@@ -606,6 +638,9 @@ func TestDefaultGroupController_GC(t *testing.T) {
606638
} {
607639
ctx := context.Background()
608640
cloud := &mocks.CloudAPI{}
641+
if tc.GetTargetGroupsByLoadBalancerCall != nil {
642+
cloud.On("GetTargetGroupsByLbArn", ctx, lbArn).Return(tc.GetTargetGroupsByLoadBalancerCall.TargetGroups, tc.GetTargetGroupsByLoadBalancerCall.Err)
643+
}
609644

610645
for _, call := range tc.DeleteTargetGroupByArnCalls {
611646
cloud.On("DeleteTargetGroupByArn", ctx, call.Arn).Return(call.Err)
@@ -622,7 +657,7 @@ func TestDefaultGroupController_GC(t *testing.T) {
622657
tgController: mockTGController,
623658
}
624659

625-
err := controller.GC(context.Background(), tc.CurrentTargetGroups, tc.TGGroup)
660+
err := controller.GC(context.Background(), lbArn, tc.TGGroup)
626661
assert.Equal(t, tc.ExpectedError, err)
627662
cloud.AssertExpectations(t)
628663
mockNameTagGen.AssertExpectations(t)
@@ -631,18 +666,22 @@ func TestDefaultGroupController_GC(t *testing.T) {
631666
}
632667

633668
func TestDefaultGroupController_Delete(t *testing.T) {
669+
lbArn := "lbArn"
634670
for _, tc := range []struct {
635-
Name string
636-
CurrentTargetGroups []*elbv2.TargetGroup
637-
DeleteTargetGroupByArnCalls []DeleteTargetGroupByArnCall
638-
ExpectedError error
671+
Name string
672+
GetTargetGroupsByLoadBalancerCall *GetTargetGroupsByLoadBalancerCall
673+
DeleteTargetGroupByArnCalls []DeleteTargetGroupByArnCall
674+
ExpectedError error
639675
}{
640676
{
641677
Name: "DELETE succeeds",
642-
CurrentTargetGroups: []*elbv2.TargetGroup{
643-
{TargetGroupArn: aws.String("arn1")},
644-
{TargetGroupArn: aws.String("arn2")},
645-
{TargetGroupArn: aws.String("arn3")},
678+
GetTargetGroupsByLoadBalancerCall: &GetTargetGroupsByLoadBalancerCall{
679+
LbArn: lbArn,
680+
TargetGroups: []*elbv2.TargetGroup{
681+
{TargetGroupArn: aws.String("arn1")},
682+
{TargetGroupArn: aws.String("arn2")},
683+
{TargetGroupArn: aws.String("arn3")},
684+
},
646685
},
647686
DeleteTargetGroupByArnCalls: []DeleteTargetGroupByArnCall{
648687
{
@@ -656,12 +695,23 @@ func TestDefaultGroupController_Delete(t *testing.T) {
656695
},
657696
},
658697
},
698+
{
699+
Name: "DELETE failed when fetch current targetGroups",
700+
GetTargetGroupsByLoadBalancerCall: &GetTargetGroupsByLoadBalancerCall{
701+
LbArn: lbArn,
702+
Err: errors.New("GetTargetGroupsByLbArn"),
703+
},
704+
ExpectedError: errors.New("failed to get targetGroups due to GetTargetGroupsByLbArn"),
705+
},
659706
{
660707
Name: "DELETE failed when deleting targetGroup",
661-
CurrentTargetGroups: []*elbv2.TargetGroup{
662-
{TargetGroupArn: aws.String("arn1")},
663-
{TargetGroupArn: aws.String("arn2")},
664-
{TargetGroupArn: aws.String("arn3")},
708+
GetTargetGroupsByLoadBalancerCall: &GetTargetGroupsByLoadBalancerCall{
709+
LbArn: lbArn,
710+
TargetGroups: []*elbv2.TargetGroup{
711+
{TargetGroupArn: aws.String("arn1")},
712+
{TargetGroupArn: aws.String("arn2")},
713+
{TargetGroupArn: aws.String("arn3")},
714+
},
665715
},
666716
DeleteTargetGroupByArnCalls: []DeleteTargetGroupByArnCall{
667717
{
@@ -674,6 +724,9 @@ func TestDefaultGroupController_Delete(t *testing.T) {
674724
} {
675725
ctx := context.Background()
676726
cloud := &mocks.CloudAPI{}
727+
if tc.GetTargetGroupsByLoadBalancerCall != nil {
728+
cloud.On("GetTargetGroupsByLbArn", ctx, lbArn).Return(tc.GetTargetGroupsByLoadBalancerCall.TargetGroups, tc.GetTargetGroupsByLoadBalancerCall.Err)
729+
}
677730
for _, call := range tc.DeleteTargetGroupByArnCalls {
678731
cloud.On("DeleteTargetGroupByArn", ctx, call.Arn).Return(call.Err)
679732
}
@@ -689,7 +742,7 @@ func TestDefaultGroupController_Delete(t *testing.T) {
689742
tgController: mockTGController,
690743
}
691744

692-
err := controller.Delete(context.Background(), tc.CurrentTargetGroups)
745+
err := controller.Delete(context.Background(), lbArn)
693746
assert.Equal(t, tc.ExpectedError, err)
694747
cloud.AssertExpectations(t)
695748
mockNameTagGen.AssertExpectations(t)

0 commit comments

Comments
 (0)