From f69e34c034a2ec409935466479d041da3bae2173 Mon Sep 17 00:00:00 2001
From: Erik Fuller <16261515+erikfuller@users.noreply.github.com>
Date: Fri, 17 Nov 2023 17:02:59 -0800
Subject: [PATCH] Fix for leaking target groups on service delete (#525)

* Fix for leaking target groups on service delete due to listener delete race condition
---
 docs/contributing/developer.md             |  2 +-
 pkg/aws/services/vpclattice.go             | 18 ++++++++++++++++
 pkg/aws/services/vpclattice_mocks.go       | 15 +++++++++++++
 pkg/deploy/lattice/service_manager.go      | 25 ++++++++++++++++++++++
 pkg/deploy/lattice/service_manager_test.go | 17 +++++++++++++++
 5 files changed, 76 insertions(+), 1 deletion(-)

diff --git a/docs/contributing/developer.md b/docs/contributing/developer.md
index ed8b0670..0791b6a5 100644
--- a/docs/contributing/developer.md
+++ b/docs/contributing/developer.md
@@ -93,7 +93,7 @@ make e2e-clean
 
 ## Local Development
 
-A minimal sanity check on changes can be done with make presubmit. This command will also run on PR.
+A minimal test of changes can be done with ```make presubmit```. This command will also run on PR.
 
 ```
 make presubmit
diff --git a/pkg/aws/services/vpclattice.go b/pkg/aws/services/vpclattice.go
index 84216ec6..97a1cdc4 100644
--- a/pkg/aws/services/vpclattice.go
+++ b/pkg/aws/services/vpclattice.go
@@ -94,6 +94,7 @@ func IsInvalidError(err error) bool {
 
 type Lattice interface {
 	vpclatticeiface.VPCLatticeAPI
+	ListListenersAsList(ctx context.Context, input *vpclattice.ListListenersInput) ([]*vpclattice.ListenerSummary, error)
 	GetRulesAsList(ctx context.Context, input *vpclattice.ListRulesInput) ([]*vpclattice.GetRuleOutput, error)
 	ListRulesAsList(ctx context.Context, input *vpclattice.ListRulesInput) ([]*vpclattice.RuleSummary, error)
 	ListServiceNetworksAsList(ctx context.Context, input *vpclattice.ListServiceNetworksInput) ([]*vpclattice.ServiceNetworkSummary, error)
@@ -128,6 +129,23 @@ func NewDefaultLattice(sess *session.Session, region string) *defaultLattice {
 	return &defaultLattice{VPCLatticeAPI: latticeSess, cache: cache}
 }
 
+func (d *defaultLattice) ListListenersAsList(ctx context.Context, input *vpclattice.ListListenersInput) ([]*vpclattice.ListenerSummary, error) {
+	var result []*vpclattice.ListenerSummary
+
+	err := d.ListListenersPagesWithContext(ctx, input, func(page *vpclattice.ListListenersOutput, lastPage bool) bool {
+		for _, l := range page.Items {
+			result = append(result, l)
+		}
+		return true
+	})
+
+	if err != nil {
+		return nil, err
+	}
+
+	return result, nil
+}
+
 func (d *defaultLattice) GetRulesAsList(ctx context.Context, input *vpclattice.ListRulesInput) ([]*vpclattice.GetRuleOutput, error) {
 	var result []*vpclattice.GetRuleOutput
 
diff --git a/pkg/aws/services/vpclattice_mocks.go b/pkg/aws/services/vpclattice_mocks.go
index febbc415..4635889e 100644
--- a/pkg/aws/services/vpclattice_mocks.go
+++ b/pkg/aws/services/vpclattice_mocks.go
@@ -1679,6 +1679,21 @@ func (mr *MockLatticeMockRecorder) ListListeners(arg0 interface{}) *gomock.Call
 	return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ListListeners", reflect.TypeOf((*MockLattice)(nil).ListListeners), arg0)
 }
 
+// ListListenersAsList mocks base method.
+func (m *MockLattice) ListListenersAsList(arg0 context.Context, arg1 *vpclattice.ListListenersInput) ([]*vpclattice.ListenerSummary, error) {
+	m.ctrl.T.Helper()
+	ret := m.ctrl.Call(m, "ListListenersAsList", arg0, arg1)
+	ret0, _ := ret[0].([]*vpclattice.ListenerSummary)
+	ret1, _ := ret[1].(error)
+	return ret0, ret1
+}
+
+// ListListenersAsList indicates an expected call of ListListenersAsList.
+func (mr *MockLatticeMockRecorder) ListListenersAsList(arg0, arg1 interface{}) *gomock.Call {
+	mr.mock.ctrl.T.Helper()
+	return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ListListenersAsList", reflect.TypeOf((*MockLattice)(nil).ListListenersAsList), arg0, arg1)
+}
+
 // ListListenersPages mocks base method.
 func (m *MockLattice) ListListenersPages(arg0 *vpclattice.ListListenersInput, arg1 func(*vpclattice.ListListenersOutput, bool) bool) error {
 	m.ctrl.T.Helper()
diff --git a/pkg/deploy/lattice/service_manager.go b/pkg/deploy/lattice/service_manager.go
index dd015c9f..37b00554 100644
--- a/pkg/deploy/lattice/service_manager.go
+++ b/pkg/deploy/lattice/service_manager.go
@@ -302,6 +302,25 @@ func (m *defaultServiceManager) deleteAllAssociations(ctx context.Context, svc *
 	return nil
 }
 
+func (m *defaultServiceManager) deleteAllListeners(ctx context.Context, svc *SvcSummary) error {
+	listeners, err := m.cloud.Lattice().ListListenersAsList(ctx, &vpclattice.ListListenersInput{
+		ServiceIdentifier: svc.Id,
+	})
+	if err != nil {
+		return err
+	}
+	for _, listener := range listeners {
+		_, err = m.cloud.Lattice().DeleteListenerWithContext(ctx, &vpclattice.DeleteListenerInput{
+			ServiceIdentifier:  svc.Id,
+			ListenerIdentifier: listener.Id,
+		})
+		if err != nil {
+			return err
+		}
+	}
+	return nil
+}
+
 func (m *defaultServiceManager) deleteAssociation(ctx context.Context, assocArn *string) error {
 	delReq := &DelSnSvcAssocReq{ServiceNetworkServiceAssociationIdentifier: assocArn}
 	_, err := m.cloud.Lattice().DeleteServiceNetworkServiceAssociationWithContext(ctx, delReq)
@@ -371,6 +390,12 @@ func (m *defaultServiceManager) Delete(ctx context.Context, svc *Service) error
 		return err
 	}
 
+	// deleting listeners explicitly helps ensure target groups are free to delete
+	err = m.deleteAllListeners(ctx, svcSum)
+	if err != nil {
+		return err
+	}
+
 	err = m.deleteService(ctx, svcSum)
 	if err != nil {
 		return err
diff --git a/pkg/deploy/lattice/service_manager_test.go b/pkg/deploy/lattice/service_manager_test.go
index ac8595ff..b46f64a1 100644
--- a/pkg/deploy/lattice/service_manager_test.go
+++ b/pkg/deploy/lattice/service_manager_test.go
@@ -311,6 +311,23 @@ func TestServiceManagerInteg(t *testing.T) {
 			DeleteServiceWithContext(gomock.Any(), gomock.Any()).Return(nil, nil).
 			Times(1)
 
+		// assert we delete listeners
+		mockLattice.EXPECT().
+			ListListenersAsList(gomock.Any(), gomock.Any()).
+			Return([]*vpclattice.ListenerSummary{
+				{
+					Id: aws.String("L1"),
+				},
+				{
+					Id: aws.String("L2"),
+				},
+			}, nil)
+
+		mockLattice.EXPECT().
+			DeleteListenerWithContext(gomock.Any(), gomock.Any()).
+			Return(nil, nil).
+			Times(2)
+
 		err := m.Delete(ctx, svc)
 		assert.Nil(t, err)
 	})