Skip to content

Commit 6a6c8b5

Browse files
author
Doyoon Kim
authored
Fix target group leaking and e2etest (#510)
* Fix target group leaking and e2etest * Fix grpcurl runner * Fix unit test * Fix more flaky/buggy e2e test cases * Rollback conditional * Address PR comments
1 parent a25205c commit 6a6c8b5

20 files changed

+223
-211
lines changed

‎cmd/aws-application-networking-k8s/main.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -193,7 +193,7 @@ func main() {
193193
setupLog.Fatalf("iam auth policy controller setup failed: %s", err)
194194
}
195195

196-
err = controllers.RegisterVpcAssociationPolicyController(ctrlLog.Named("vpc-association-policy"), mgr, cloud)
196+
err = controllers.RegisterVpcAssociationPolicyController(ctrlLog.Named("vpc-association-policy"), cloud, finalizerManager, mgr)
197197
if err != nil {
198198
setupLog.Fatalf("vpc association policy controller setup failed: %s", err)
199199
}

‎controllers/vpcassociationpolicy_controller.go‎

Lines changed: 27 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -11,31 +11,37 @@ import (
1111
"github.com/aws/aws-application-networking-k8s/pkg/utils/gwlog"
1212

1313
"github.com/aws/aws-application-networking-k8s/controllers/eventhandlers"
14+
"github.com/aws/aws-application-networking-k8s/pkg/k8s"
1415
"github.com/aws/aws-application-networking-k8s/pkg/utils"
1516
"k8s.io/apimachinery/pkg/api/meta"
1617
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
1718
ctrl "sigs.k8s.io/controller-runtime"
1819
"sigs.k8s.io/controller-runtime/pkg/builder"
1920
"sigs.k8s.io/controller-runtime/pkg/client"
20-
"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
2121
"sigs.k8s.io/controller-runtime/pkg/predicate"
2222
gwv1alpha2 "sigs.k8s.io/gateway-api/apis/v1alpha2"
2323
gwv1beta1 "sigs.k8s.io/gateway-api/apis/v1beta1"
2424
)
2525

26+
const (
27+
finalizer = "vpcassociationpolicies.application-networking.k8s.aws/resources"
28+
)
29+
2630
type vpcAssociationPolicyReconciler struct {
27-
log gwlog.Logger
28-
client client.Client
29-
cloud pkg_aws.Cloud
30-
manager deploy.ServiceNetworkManager
31+
log gwlog.Logger
32+
client client.Client
33+
cloud pkg_aws.Cloud
34+
finalizerManager k8s.FinalizerManager
35+
manager deploy.ServiceNetworkManager
3136
}
3237

33-
func RegisterVpcAssociationPolicyController(log gwlog.Logger, mgr ctrl.Manager, cloud pkg_aws.Cloud) error {
38+
func RegisterVpcAssociationPolicyController(log gwlog.Logger, cloud pkg_aws.Cloud, finalizerManager k8s.FinalizerManager, mgr ctrl.Manager) error {
3439
controller := &vpcAssociationPolicyReconciler{
35-
log: log,
36-
client: mgr.GetClient(),
37-
cloud: cloud,
38-
manager: deploy.NewDefaultServiceNetworkManager(log, cloud),
40+
log: log,
41+
client: mgr.GetClient(),
42+
cloud: cloud,
43+
finalizerManager: finalizerManager,
44+
manager: deploy.NewDefaultServiceNetworkManager(log, cloud),
3945
}
4046

4147
eh := eventhandlers.NewPolicyEventHandler(log, mgr.GetClient(), &anv1alpha1.VpcAssociationPolicy{})
@@ -77,11 +83,6 @@ func (c *vpcAssociationPolicyReconciler) Reconcile(ctx context.Context, req ctrl
7783
return ctrl.Result{RequeueAfter: time.Second * 30}, nil
7884
}
7985

80-
err = c.handleFinalizer(ctx, k8sPolicy)
81-
if err != nil {
82-
return ctrl.Result{}, err
83-
}
84-
8586
c.log.Infow("reconciled vpc association policy",
8687
"req", req,
8788
"targetRef", k8sPolicy.Spec.TargetRef,
@@ -90,21 +91,11 @@ func (c *vpcAssociationPolicyReconciler) Reconcile(ctx context.Context, req ctrl
9091
return ctrl.Result{}, nil
9192
}
9293

93-
func (c *vpcAssociationPolicyReconciler) handleFinalizer(ctx context.Context, k8sPolicy *anv1alpha1.VpcAssociationPolicy) error {
94-
finalizer := "vpcassociationpolicies.application-networking.k8s.aws/resources"
95-
if k8sPolicy.DeletionTimestamp.IsZero() {
96-
if !controllerutil.ContainsFinalizer(k8sPolicy, finalizer) {
97-
controllerutil.AddFinalizer(k8sPolicy, finalizer)
98-
}
99-
} else {
100-
if controllerutil.ContainsFinalizer(k8sPolicy, finalizer) {
101-
controllerutil.RemoveFinalizer(k8sPolicy, finalizer)
102-
}
103-
}
104-
return c.client.Update(ctx, k8sPolicy)
105-
}
106-
10794
func (c *vpcAssociationPolicyReconciler) upsert(ctx context.Context, k8sPolicy *anv1alpha1.VpcAssociationPolicy) error {
95+
err := c.finalizerManager.AddFinalizers(ctx, k8sPolicy, finalizer)
96+
if err != nil {
97+
return err
98+
}
10899
snName := string(k8sPolicy.Spec.TargetRef.Name)
109100
sgIds := utils.SliceMap(k8sPolicy.Spec.SecurityGroupIds, func(sg anv1alpha1.SecurityGroupId) *string {
110101
str := string(sg)
@@ -132,6 +123,10 @@ func (c *vpcAssociationPolicyReconciler) delete(ctx context.Context, k8sPolicy *
132123
if err != nil {
133124
return c.handleDeleteError(err)
134125
}
126+
err = c.finalizerManager.RemoveFinalizers(ctx, k8sPolicy, finalizer)
127+
if err != nil {
128+
return err
129+
}
135130
return nil
136131
}
137132

@@ -169,6 +164,9 @@ func (c *vpcAssociationPolicyReconciler) handleDeleteError(err error) error {
169164
}
170165

171166
func (c *vpcAssociationPolicyReconciler) updateLatticeAnnotation(ctx context.Context, k8sPolicy *anv1alpha1.VpcAssociationPolicy, resArn string) error {
167+
if k8sPolicy.Annotations == nil {
168+
k8sPolicy.Annotations = make(map[string]string)
169+
}
172170
k8sPolicy.Annotations["application-networking.k8s.aws/resourceArn"] = resArn
173171
err := c.client.Update(ctx, k8sPolicy)
174172
return err

‎pkg/deploy/lattice/service_network_manager.go‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -177,7 +177,7 @@ func (m *defaultServiceNetworkManager) CreateOrUpdate(ctx context.Context, servi
177177
}
178178
resp, err := vpcLatticeSess.CreateServiceNetworkWithContext(ctx, &serviceNetworkInput)
179179
if err != nil {
180-
return model.ServiceNetworkStatus{ServiceNetworkARN: "", ServiceNetworkID: ""}, err
180+
return model.ServiceNetworkStatus{}, err
181181
}
182182

183183
serviceNetworkId = aws.StringValue(resp.Id)
@@ -201,6 +201,7 @@ func (m *defaultServiceNetworkManager) CreateOrUpdate(ctx context.Context, servi
201201
createServiceNetworkVpcAssociationInput := vpclattice.CreateServiceNetworkVpcAssociationInput{
202202
ServiceNetworkIdentifier: &serviceNetworkId,
203203
VpcIdentifier: &config.VpcID,
204+
Tags: m.cloud.DefaultTags(),
204205
}
205206
_, err = vpcLatticeSess.CreateServiceNetworkVpcAssociationWithContext(ctx, &createServiceNetworkVpcAssociationInput)
206207
if err != nil {

‎pkg/deploy/lattice/service_network_manager_test.go‎

Lines changed: 14 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,7 @@ func Test_CreateOrUpdateServiceNetwork_SnNotExist_NeedToAssociate(t *testing.T)
5252
createServiceNetworkVpcAssociationInput := &vpclattice.CreateServiceNetworkVpcAssociationInput{
5353
ServiceNetworkIdentifier: &snId,
5454
VpcIdentifier: &config.VpcID,
55+
Tags: cloud.DefaultTags(),
5556
}
5657
associationStatus := vpclattice.ServiceNetworkVpcAssociationStatusActive
5758
createServiceNetworkVPCAssociationOutput := &vpclattice.CreateServiceNetworkVpcAssociationOutput{
@@ -286,10 +287,6 @@ func Test_CreateOrUpdateServiceNetwork_SnAlreadyExist_ServiceNetworkVpcAssociati
286287
createServiceNetworkVPCAssociationOutput := &vpclattice.CreateServiceNetworkVpcAssociationOutput{
287288
Status: &associationStatus,
288289
}
289-
createServiceNetworkVpcAssociationInput := &vpclattice.CreateServiceNetworkVpcAssociationInput{
290-
ServiceNetworkIdentifier: &snId,
291-
VpcIdentifier: &config.VpcID,
292-
}
293290

294291
c := gomock.NewController(t)
295292
defer c.Finish()
@@ -305,6 +302,12 @@ func Test_CreateOrUpdateServiceNetwork_SnAlreadyExist_ServiceNetworkVpcAssociati
305302
SvcNetwork: item,
306303
Tags: snTagsOuput.Tags,
307304
}, nil)
305+
306+
createServiceNetworkVpcAssociationInput := &vpclattice.CreateServiceNetworkVpcAssociationInput{
307+
ServiceNetworkIdentifier: &snId,
308+
VpcIdentifier: &config.VpcID,
309+
Tags: cloud.DefaultTags(),
310+
}
308311
mockLattice.EXPECT().CreateServiceNetworkVpcAssociationWithContext(ctx, createServiceNetworkVpcAssociationInput).Return(createServiceNetworkVPCAssociationOutput, nil)
309312

310313
snMgr := NewDefaultServiceNetworkManager(gwlog.FallbackLogger, cloud)
@@ -349,10 +352,6 @@ func Test_CreateOrUpdateServiceNetwork_SnAlreadyExist_SnAssociatedWithOtherVPC(t
349352
createServiceNetworkVPCAssociationOutput := &vpclattice.CreateServiceNetworkVpcAssociationOutput{
350353
Status: &associationStatus,
351354
}
352-
createServiceNetworkVpcAssociationInput := &vpclattice.CreateServiceNetworkVpcAssociationInput{
353-
ServiceNetworkIdentifier: &snId,
354-
VpcIdentifier: &config.VpcID,
355-
}
356355

357356
c := gomock.NewController(t)
358357
defer c.Finish()
@@ -368,6 +367,12 @@ func Test_CreateOrUpdateServiceNetwork_SnAlreadyExist_SnAssociatedWithOtherVPC(t
368367
SvcNetwork: item,
369368
Tags: snTagsOuput.Tags,
370369
}, nil)
370+
371+
createServiceNetworkVpcAssociationInput := &vpclattice.CreateServiceNetworkVpcAssociationInput{
372+
ServiceNetworkIdentifier: &snId,
373+
VpcIdentifier: &config.VpcID,
374+
Tags: cloud.DefaultTags(),
375+
}
371376
mockLattice.EXPECT().CreateServiceNetworkVpcAssociationWithContext(ctx, createServiceNetworkVpcAssociationInput).Return(createServiceNetworkVPCAssociationOutput, nil)
372377

373378
snMgr := NewDefaultServiceNetworkManager(gwlog.FallbackLogger, cloud)
@@ -410,6 +415,7 @@ func Test_CreateOrUpdateServiceNetwork_SnNotExist_ServiceNetworkVpcAssociationRe
410415
createServiceNetworkVpcAssociationInput := &vpclattice.CreateServiceNetworkVpcAssociationInput{
411416
ServiceNetworkIdentifier: &snId,
412417
VpcIdentifier: &config.VpcID,
418+
Tags: cloud.DefaultTags(),
413419
}
414420

415421
mockLattice.EXPECT().FindServiceNetwork(ctx, gomock.Any(), gomock.Any()).Return(nil, nil)

‎pkg/deploy/lattice/target_group_manager.go‎

Lines changed: 41 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -248,26 +248,27 @@ type tgListOutput struct {
248248
func (s *defaultTargetGroupManager) List(ctx context.Context) ([]tgListOutput, error) {
249249
lattice := s.cloud.Lattice()
250250
var tgList []tgListOutput
251-
targetGroupListInput := vpclattice.ListTargetGroupsInput{
252-
VpcIdentifier: aws.String(config.VpcID),
253-
TargetGroupType: aws.String(vpclattice.TargetGroupTypeIp),
254-
}
251+
targetGroupListInput := vpclattice.ListTargetGroupsInput{}
255252
resp, err := lattice.ListTargetGroupsAsList(ctx, &targetGroupListInput)
256253
if err != nil {
257254
return nil, err
258255
}
259256
if len(resp) == 0 {
260257
return nil, nil
261258
}
262-
tgArns := utils.SliceMap(resp, func(tg *vpclattice.TargetGroupSummary) string {
259+
validTgs := utils.SliceFilter(resp, func(tg *vpclattice.TargetGroupSummary) bool {
260+
return aws.StringValue(tg.VpcIdentifier) == config.VpcID &&
261+
aws.StringValue(tg.Type) == vpclattice.TargetGroupTypeIp
262+
})
263+
tgArns := utils.SliceMap(validTgs, func(tg *vpclattice.TargetGroupSummary) string {
263264
return aws.StringValue(tg.Arn)
264265
})
265266
tgArnToTagsMap, err := s.cloud.Tagging().GetTagsForArns(ctx, tgArns)
266267

267268
if err != nil {
268269
return nil, err
269270
}
270-
for _, tg := range resp {
271+
for _, tg := range validTgs {
271272
tgList = append(tgList, tgListOutput{
272273
tgSummary: tg,
273274
tags: tgArnToTagsMap[*tg.Arn],
@@ -288,44 +289,43 @@ func (s *defaultTargetGroupManager) findTargetGroup(
288289
if len(arns) == 0 {
289290
return nil, nil
290291
}
291-
// Tag fields guarantee one result, as there can be only one target group for one service/route combination.
292-
// We move forward but log this situation to help troubleshooting
293-
if len(arns) > 1 {
294-
s.log.Warnw("Target groups with conflicting tags found", "arns", arns)
295-
}
296-
arn := arns[0]
297292

298-
latticeTg, err := s.cloud.Lattice().GetTargetGroupWithContext(ctx, &vpclattice.GetTargetGroupInput{
299-
TargetGroupIdentifier: &arn,
300-
})
301-
if err != nil {
302-
return nil, services.IgnoreNotFound(err)
303-
}
293+
for _, arn := range arns {
294+
latticeTg, err := s.cloud.Lattice().GetTargetGroupWithContext(ctx, &vpclattice.GetTargetGroupInput{
295+
TargetGroupIdentifier: &arn,
296+
})
297+
if err != nil {
298+
if services.IsNotFoundError(err) {
299+
continue
300+
}
301+
return nil, err
302+
}
304303

305-
// we ignore create failed status, so may as well check for it first
306-
status := aws.StringValue(latticeTg.Status)
307-
if status == vpclattice.TargetGroupStatusCreateFailed {
308-
return nil, nil
309-
}
304+
// we ignore create failed status, so may as well check for it first
305+
status := aws.StringValue(latticeTg.Status)
306+
if status == vpclattice.TargetGroupStatusCreateFailed {
307+
continue
308+
}
310309

311-
// Double-check the immutable fields to ensure TG is valid
312-
match, err := s.IsTargetGroupMatch(ctx, modelTargetGroup, &vpclattice.TargetGroupSummary{
313-
Arn: latticeTg.Arn,
314-
Port: latticeTg.Config.Port,
315-
Protocol: latticeTg.Config.Protocol,
316-
IpAddressType: latticeTg.Config.IpAddressType,
317-
Type: latticeTg.Type,
318-
VpcIdentifier: latticeTg.Config.VpcIdentifier,
319-
}, nil) // we already know that tags match
320-
if err != nil {
321-
return nil, err
322-
}
323-
if match {
324-
switch status {
325-
case vpclattice.TargetGroupStatusCreateInProgress, vpclattice.TargetGroupStatusDeleteInProgress:
326-
return nil, errors.New(LATTICE_RETRY)
327-
case vpclattice.TargetGroupStatusDeleteFailed, vpclattice.TargetGroupStatusActive:
328-
return latticeTg, nil
310+
// Check the immutable fields to ensure TG is valid
311+
match, err := s.IsTargetGroupMatch(ctx, modelTargetGroup, &vpclattice.TargetGroupSummary{
312+
Arn: latticeTg.Arn,
313+
Port: latticeTg.Config.Port,
314+
Protocol: latticeTg.Config.Protocol,
315+
IpAddressType: latticeTg.Config.IpAddressType,
316+
Type: latticeTg.Type,
317+
VpcIdentifier: latticeTg.Config.VpcIdentifier,
318+
}, nil) // we already know that tags match
319+
if err != nil {
320+
return nil, err
321+
}
322+
if match {
323+
switch status {
324+
case vpclattice.TargetGroupStatusCreateInProgress, vpclattice.TargetGroupStatusDeleteInProgress:
325+
return nil, errors.New(LATTICE_RETRY)
326+
case vpclattice.TargetGroupStatusDeleteFailed, vpclattice.TargetGroupStatusActive:
327+
return latticeTg, nil
328+
}
329329
}
330330
}
331331

‎pkg/deploy/lattice/target_group_manager_test.go‎

Lines changed: 13 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -777,16 +777,23 @@ func Test_ListTG_TGsExist(t *testing.T) {
777777
arn := "123456789"
778778
id := "123456789"
779779
name1 := "test1"
780+
config.VpcID = "vpc-id"
781+
config.ClusterName = "cluster-name"
782+
tgType := vpclattice.TargetGroupTypeIp
780783
tg1 := &vpclattice.TargetGroupSummary{
781-
Arn: &arn,
782-
Id: &id,
783-
Name: &name1,
784+
Arn: &arn,
785+
Id: &id,
786+
Name: &name1,
787+
VpcIdentifier: &config.VpcID,
788+
Type: &tgType,
784789
}
785790
name2 := "test2"
786791
tg2 := &vpclattice.TargetGroupSummary{
787-
Arn: &arn,
788-
Id: &id,
789-
Name: &name2,
792+
Arn: &arn,
793+
Id: &id,
794+
Name: &name2,
795+
VpcIdentifier: &config.VpcID,
796+
Type: &tgType,
790797
}
791798
listTGOutput := []*vpclattice.TargetGroupSummary{tg1, tg2}
792799

0 commit comments

Comments
 (0)