Skip to content

Commit 4ba8b59

Browse files
FedomnFedomn
committed
Revert "rewrite TargetGroup filter using ELBV2 DescribeTargetGroups API"
1 parent 0a80fea commit 4ba8b59

File tree

6 files changed

+104
-181
lines changed

6 files changed

+104
-181
lines changed

‎internal/alb/lb/loadbalancer.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -144,7 +144,7 @@ func (controller *defaultController) Reconcile(ctx context.Context, ingress *ext
144144
if err := controller.lsGroupController.Reconcile(ctx, lbArn, ingress, tgGroup); err != nil {
145145
return nil, fmt.Errorf("failed to reconcile listeners due to %v", err)
146146
}
147-
if err := controller.tgGroupController.GC(ctx, lbArn, tgGroup); err != nil {
147+
if err := controller.tgGroupController.GC(ctx, tgGroup); err != nil {
148148
return nil, fmt.Errorf("failed to GC targetGroups due to %v", err)
149149
}
150150

@@ -167,7 +167,7 @@ func (controller *defaultController) Delete(ctx context.Context, ingressKey type
167167
if err = controller.lsGroupController.Delete(ctx, aws.StringValue(instance.LoadBalancerArn)); err != nil {
168168
return fmt.Errorf("failed to delete listeners due to %v", err)
169169
}
170-
if err = controller.tgGroupController.Delete(ctx, aws.StringValue(instance.LoadBalancerArn)); err != nil {
170+
if err = controller.tgGroupController.Delete(ctx, ingressKey); err != nil {
171171
return fmt.Errorf("failed to GC targetGroups due to %v", err)
172172
}
173173

‎internal/alb/tg/targetgroup_group.go

Lines changed: 16 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ import (
1414
"github.com/kubernetes-sigs/aws-alb-ingress-controller/internal/ingress/backend"
1515
"github.com/kubernetes-sigs/aws-alb-ingress-controller/internal/ingress/controller/store"
1616
extensions "k8s.io/api/extensions/v1beta1"
17+
"k8s.io/apimachinery/pkg/types"
1718
"k8s.io/apimachinery/pkg/util/intstr"
1819
"k8s.io/apimachinery/pkg/util/sets"
1920
"sigs.k8s.io/controller-runtime/pkg/client"
@@ -25,10 +26,10 @@ type GroupController interface {
2526
Reconcile(ctx context.Context, ingress *extensions.Ingress) (TargetGroupGroup, error)
2627

2728
// GC will delete unused targetGroups matched by tag selector
28-
GC(ctx context.Context, lbArn string, tgGroup TargetGroupGroup) error
29+
GC(ctx context.Context, tgGroup TargetGroupGroup) error
2930

3031
// Delete will delete all targetGroups created for ingress
31-
Delete(ctx context.Context, lbArn string) error
32+
Delete(ctx context.Context, ingressKey types.NamespacedName) error
3233
}
3334

3435
// NewGroupController creates an GroupController
@@ -81,21 +82,21 @@ func (controller *defaultGroupController) Reconcile(ctx context.Context, ingress
8182
}, nil
8283
}
8384

84-
func (controller *defaultGroupController) GC(ctx context.Context, lbArn string, tgGroup TargetGroupGroup) error {
85+
func (controller *defaultGroupController) GC(ctx context.Context, tgGroup TargetGroupGroup) error {
86+
tagFilters := make(map[string][]string)
87+
for k, v := range tgGroup.selector {
88+
tagFilters[k] = []string{v}
89+
}
90+
8591
usedExternalTGARNs := sets.NewString(tgGroup.externalTGARNs...)
8692
usedServiceTGARNs := sets.NewString()
8793
for _, tg := range tgGroup.TGByBackend {
8894
usedServiceTGARNs.Insert(tg.Arn)
8995
}
90-
targetGroups, err := controller.cloud.GetTargetGroupsByLbArn(ctx, lbArn)
96+
arns, err := controller.cloud.GetResourcesByFilters(tagFilters, aws.ResourceTypeEnumELBTargetGroup)
9197
if err != nil {
9298
return fmt.Errorf("failed to get targetGroups due to %v", err)
9399
}
94-
arns := make([]string, 0, len(targetGroups))
95-
for _, tg := range targetGroups {
96-
arns = append(arns, aws.StringValue(tg.TargetGroupArn))
97-
}
98-
99100
currentServiceTGARNs := sets.NewString(arns...)
100101
unusedServiceTGARNs := currentServiceTGARNs.Difference(usedServiceTGARNs)
101102
for arn := range unusedServiceTGARNs {
@@ -113,8 +114,12 @@ func (controller *defaultGroupController) GC(ctx context.Context, lbArn string,
113114
return nil
114115
}
115116

116-
func (controller *defaultGroupController) Delete(ctx context.Context, lbArn string) error {
117-
return controller.GC(ctx, lbArn, TargetGroupGroup{})
117+
func (controller *defaultGroupController) Delete(ctx context.Context, ingressKey types.NamespacedName) error {
118+
selector := controller.nameTagGen.TagTGGroup(ingressKey.Namespace, ingressKey.Name)
119+
tgGroup := TargetGroupGroup{
120+
selector: selector,
121+
}
122+
return controller.GC(ctx, tgGroup)
118123
}
119124

120125
// ExtractTargetGroupBackends returns backends for Ingress.

‎internal/alb/tg/targetgroup_group_test.go

Lines changed: 86 additions & 64 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ import (
1414
"github.com/stretchr/testify/mock"
1515
extensions "k8s.io/api/extensions/v1beta1"
1616
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
17+
"k8s.io/apimachinery/pkg/types"
1718
"k8s.io/apimachinery/pkg/util/intstr"
1819
)
1920

@@ -24,9 +25,10 @@ type TGReconcileCall struct {
2425
Err error
2526
}
2627

27-
type GetTargetGroupsByLoadBalancerCall struct {
28-
LbArn string
29-
TargetGroups []*elbv2.TargetGroup
28+
type GetResourcesByFiltersCall struct {
29+
TagFilters map[string][]string
30+
ResourceType string
31+
Arns []string
3032
Err error
3133
}
3234

@@ -533,13 +535,12 @@ func TestDefaultGroupController_Reconcile(t *testing.T) {
533535
}
534536

535537
func TestDefaultGroupController_GC(t *testing.T) {
536-
lbArn := "lbArn"
537538
for _, tc := range []struct {
538-
Name string
539-
TGGroup TargetGroupGroup
540-
GetTargetGroupsByLoadBalancerCall *GetTargetGroupsByLoadBalancerCall
541-
DeleteTargetGroupByArnCalls []DeleteTargetGroupByArnCall
542-
ExpectedError error
539+
Name string
540+
TGGroup TargetGroupGroup
541+
GetResourcesByFiltersCall *GetResourcesByFiltersCall
542+
DeleteTargetGroupByArnCalls []DeleteTargetGroupByArnCall
543+
ExpectedError error
543544
}{
544545
{
545546
Name: "GC succeeds",
@@ -550,14 +551,12 @@ func TestDefaultGroupController_GC(t *testing.T) {
550551
ServicePort: intstr.FromInt(80),
551552
}: {Arn: "arn1"},
552553
},
554+
selector: map[string]string{"key1": "value1", "key2": "value2"},
553555
},
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-
},
556+
GetResourcesByFiltersCall: &GetResourcesByFiltersCall{
557+
TagFilters: map[string][]string{"key1": {"value1"}, "key2": {"value2"}},
558+
ResourceType: aws.ResourceTypeEnumELBTargetGroup,
559+
Arns: []string{"arn1", "arn2", "arn3"},
561560
},
562561
DeleteTargetGroupByArnCalls: []DeleteTargetGroupByArnCall{
563562
{
@@ -578,14 +577,12 @@ func TestDefaultGroupController_GC(t *testing.T) {
578577
}: {Arn: "arn1"},
579578
},
580579
externalTGARNs: []string{"arn3"},
580+
selector: map[string]string{"key1": "value1", "key2": "value2"},
581581
},
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-
},
582+
GetResourcesByFiltersCall: &GetResourcesByFiltersCall{
583+
TagFilters: map[string][]string{"key1": {"value1"}, "key2": {"value2"}},
584+
ResourceType: aws.ResourceTypeEnumELBTargetGroup,
585+
Arns: []string{"arn1", "arn2", "arn3"},
589586
},
590587
DeleteTargetGroupByArnCalls: []DeleteTargetGroupByArnCall{
591588
{
@@ -602,12 +599,14 @@ func TestDefaultGroupController_GC(t *testing.T) {
602599
ServicePort: intstr.FromInt(80),
603600
}: {Arn: "arn1"},
604601
},
602+
selector: map[string]string{"key1": "value1", "key2": "value2"},
605603
},
606-
GetTargetGroupsByLoadBalancerCall: &GetTargetGroupsByLoadBalancerCall{
607-
LbArn: lbArn,
608-
Err: errors.New("GetTargetGroupsByLbArn"),
604+
GetResourcesByFiltersCall: &GetResourcesByFiltersCall{
605+
TagFilters: map[string][]string{"key1": {"value1"}, "key2": {"value2"}},
606+
ResourceType: aws.ResourceTypeEnumELBTargetGroup,
607+
Err: errors.New("GetResourcesByFiltersCall"),
609608
},
610-
ExpectedError: errors.New("failed to get targetGroups due to GetTargetGroupsByLbArn"),
609+
ExpectedError: errors.New("failed to get targetGroups due to GetResourcesByFiltersCall"),
611610
},
612611
{
613612
Name: "GC failed when deleting targetGroup",
@@ -618,14 +617,12 @@ func TestDefaultGroupController_GC(t *testing.T) {
618617
ServicePort: intstr.FromInt(80),
619618
}: {Arn: "arn1"},
620619
},
620+
selector: map[string]string{"key1": "value1", "key2": "value2"},
621621
},
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-
},
622+
GetResourcesByFiltersCall: &GetResourcesByFiltersCall{
623+
TagFilters: map[string][]string{"key1": {"value1"}, "key2": {"value2"}},
624+
ResourceType: aws.ResourceTypeEnumELBTargetGroup,
625+
Arns: []string{"arn1", "arn2", "arn3"},
629626
},
630627
DeleteTargetGroupByArnCalls: []DeleteTargetGroupByArnCall{
631628
{
@@ -638,10 +635,9 @@ func TestDefaultGroupController_GC(t *testing.T) {
638635
} {
639636
ctx := context.Background()
640637
cloud := &mocks.CloudAPI{}
641-
if tc.GetTargetGroupsByLoadBalancerCall != nil {
642-
cloud.On("GetTargetGroupsByLbArn", ctx, lbArn).Return(tc.GetTargetGroupsByLoadBalancerCall.TargetGroups, tc.GetTargetGroupsByLoadBalancerCall.Err)
638+
if tc.GetResourcesByFiltersCall != nil {
639+
cloud.On("GetResourcesByFilters", tc.GetResourcesByFiltersCall.TagFilters, tc.GetResourcesByFiltersCall.ResourceType).Return(tc.GetResourcesByFiltersCall.Arns, tc.GetResourcesByFiltersCall.Err)
643640
}
644-
645641
for _, call := range tc.DeleteTargetGroupByArnCalls {
646642
cloud.On("DeleteTargetGroupByArn", ctx, call.Arn).Return(call.Err)
647643
}
@@ -657,7 +653,7 @@ func TestDefaultGroupController_GC(t *testing.T) {
657653
tgController: mockTGController,
658654
}
659655

660-
err := controller.GC(context.Background(), lbArn, tc.TGGroup)
656+
err := controller.GC(context.Background(), tc.TGGroup)
661657
assert.Equal(t, tc.ExpectedError, err)
662658
cloud.AssertExpectations(t)
663659
mockNameTagGen.AssertExpectations(t)
@@ -666,22 +662,29 @@ func TestDefaultGroupController_GC(t *testing.T) {
666662
}
667663

668664
func TestDefaultGroupController_Delete(t *testing.T) {
669-
lbArn := "lbArn"
670665
for _, tc := range []struct {
671-
Name string
672-
GetTargetGroupsByLoadBalancerCall *GetTargetGroupsByLoadBalancerCall
673-
DeleteTargetGroupByArnCalls []DeleteTargetGroupByArnCall
674-
ExpectedError error
666+
Name string
667+
IngressKey types.NamespacedName
668+
TagTGGroupCall *TagTGGroupCall
669+
GetResourcesByFiltersCall *GetResourcesByFiltersCall
670+
DeleteTargetGroupByArnCalls []DeleteTargetGroupByArnCall
671+
ExpectedError error
675672
}{
676673
{
677674
Name: "DELETE succeeds",
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-
},
675+
IngressKey: types.NamespacedName{
676+
Namespace: "namespace",
677+
Name: "ingress",
678+
},
679+
TagTGGroupCall: &TagTGGroupCall{
680+
Namespace: "namespace",
681+
IngressName: "ingress",
682+
Tags: map[string]string{"key1": "value1", "key2": "value2"},
683+
},
684+
GetResourcesByFiltersCall: &GetResourcesByFiltersCall{
685+
TagFilters: map[string][]string{"key1": {"value1"}, "key2": {"value2"}},
686+
ResourceType: aws.ResourceTypeEnumELBTargetGroup,
687+
Arns: []string{"arn1", "arn2", "arn3"},
685688
},
686689
DeleteTargetGroupByArnCalls: []DeleteTargetGroupByArnCall{
687690
{
@@ -697,21 +700,37 @@ func TestDefaultGroupController_Delete(t *testing.T) {
697700
},
698701
{
699702
Name: "DELETE failed when fetch current targetGroups",
700-
GetTargetGroupsByLoadBalancerCall: &GetTargetGroupsByLoadBalancerCall{
701-
LbArn: lbArn,
702-
Err: errors.New("GetTargetGroupsByLbArn"),
703+
IngressKey: types.NamespacedName{
704+
Namespace: "namespace",
705+
Name: "ingress",
703706
},
704-
ExpectedError: errors.New("failed to get targetGroups due to GetTargetGroupsByLbArn"),
707+
TagTGGroupCall: &TagTGGroupCall{
708+
Namespace: "namespace",
709+
IngressName: "ingress",
710+
Tags: map[string]string{"key1": "value1", "key2": "value2"},
711+
},
712+
GetResourcesByFiltersCall: &GetResourcesByFiltersCall{
713+
TagFilters: map[string][]string{"key1": {"value1"}, "key2": {"value2"}},
714+
ResourceType: aws.ResourceTypeEnumELBTargetGroup,
715+
Err: errors.New("GetResourcesByFiltersCall"),
716+
},
717+
ExpectedError: errors.New("failed to get targetGroups due to GetResourcesByFiltersCall"),
705718
},
706719
{
707720
Name: "DELETE failed when deleting targetGroup",
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-
},
721+
IngressKey: types.NamespacedName{
722+
Namespace: "namespace",
723+
Name: "ingress",
724+
},
725+
TagTGGroupCall: &TagTGGroupCall{
726+
Namespace: "namespace",
727+
IngressName: "ingress",
728+
Tags: map[string]string{"key1": "value1", "key2": "value2"},
729+
},
730+
GetResourcesByFiltersCall: &GetResourcesByFiltersCall{
731+
TagFilters: map[string][]string{"key1": {"value1"}, "key2": {"value2"}},
732+
ResourceType: aws.ResourceTypeEnumELBTargetGroup,
733+
Arns: []string{"arn1", "arn2", "arn3"},
715734
},
716735
DeleteTargetGroupByArnCalls: []DeleteTargetGroupByArnCall{
717736
{
@@ -724,13 +743,16 @@ func TestDefaultGroupController_Delete(t *testing.T) {
724743
} {
725744
ctx := context.Background()
726745
cloud := &mocks.CloudAPI{}
727-
if tc.GetTargetGroupsByLoadBalancerCall != nil {
728-
cloud.On("GetTargetGroupsByLbArn", ctx, lbArn).Return(tc.GetTargetGroupsByLoadBalancerCall.TargetGroups, tc.GetTargetGroupsByLoadBalancerCall.Err)
746+
if tc.GetResourcesByFiltersCall != nil {
747+
cloud.On("GetResourcesByFilters", tc.GetResourcesByFiltersCall.TagFilters, tc.GetResourcesByFiltersCall.ResourceType).Return(tc.GetResourcesByFiltersCall.Arns, tc.GetResourcesByFiltersCall.Err)
729748
}
730749
for _, call := range tc.DeleteTargetGroupByArnCalls {
731750
cloud.On("DeleteTargetGroupByArn", ctx, call.Arn).Return(call.Err)
732751
}
733752
mockNameTagGen := &MockNameTagGenerator{}
753+
if tc.TagTGGroupCall != nil {
754+
mockNameTagGen.On("TagTGGroup", tc.TagTGGroupCall.Namespace, tc.TagTGGroupCall.IngressName).Return(tc.TagTGGroupCall.Tags)
755+
}
734756
mockTGController := &MockController{}
735757
for _, call := range tc.DeleteTargetGroupByArnCalls {
736758
mockTGController.On("StopReconcilingPodConditionStatus", call.Arn).Return()
@@ -742,7 +764,7 @@ func TestDefaultGroupController_Delete(t *testing.T) {
742764
tgController: mockTGController,
743765
}
744766

745-
err := controller.Delete(context.Background(), lbArn)
767+
err := controller.Delete(context.Background(), tc.IngressKey)
746768
assert.Equal(t, tc.ExpectedError, err)
747769
cloud.AssertExpectations(t)
748770
mockNameTagGen.AssertExpectations(t)

‎internal/aws/elbv2.go

Lines changed: 0 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -36,9 +36,6 @@ type ELBV2API interface {
3636
// GetTargetGroupByName retrieve TargetGroup instance by name
3737
GetTargetGroupByName(context.Context, string) (*elbv2.TargetGroup, error)
3838

39-
// GetTargetGroupsByLbArn retrieve TargetGroup Instances by LoadBalancer arn
40-
GetTargetGroupsByLbArn(ctx context.Context, lbArn string) ([]*elbv2.TargetGroup, error)
41-
4239
// DeleteTargetGroupByArn deletes TargetGroup instance by arn
4340
DeleteTargetGroupByArn(context.Context, string) error
4441

@@ -294,17 +291,6 @@ func (c *Cloud) GetTargetGroupByName(ctx context.Context, name string) (*elbv2.T
294291
return targetGroups[0], nil
295292
}
296293

297-
// GetTargetGroupsByLbArn retrieve TargetGroup Instances by LoadBalancer arn
298-
func (c *Cloud) GetTargetGroupsByLbArn(ctx context.Context, lbArn string) ([]*elbv2.TargetGroup, error) {
299-
targetGroups, err := c.describeTargetGroupsHelper(&elbv2.DescribeTargetGroupsInput{
300-
LoadBalancerArn: aws.String(lbArn),
301-
})
302-
if err != nil {
303-
return nil, err
304-
}
305-
return targetGroups, nil
306-
}
307-
308294
// DeleteTargetGroupByArn deletes TargetGroup instance by arn
309295
func (c *Cloud) DeleteTargetGroupByArn(ctx context.Context, arn string) error {
310296
_, err := c.elbv2.DeleteTargetGroupWithContext(ctx, &elbv2.DeleteTargetGroupInput{

0 commit comments

Comments
 (0)