Dangling fix AWS Shield Protections after Ingress are deleted #4042

go.mod

@@ -6,6 +6,7 @@ require (
 	github.com/aws/aws-sdk-go v1.55.5
 	github.com/aws/aws-sdk-go-v2 v1.32.6
 	github.com/aws/aws-sdk-go-v2/config v1.27.27
+	github.com/aws/aws-sdk-go-v2/credentials v1.17.27
 	github.com/aws/aws-sdk-go-v2/feature/ec2/imds v1.16.11
 	github.com/aws/aws-sdk-go-v2/service/acm v1.28.4
 	github.com/aws/aws-sdk-go-v2/service/appmesh v1.27.7
@@ -14,6 +15,7 @@ require (
 	github.com/aws/aws-sdk-go-v2/service/resourcegroupstaggingapi v1.23.3
 	github.com/aws/aws-sdk-go-v2/service/servicediscovery v1.31.7
 	github.com/aws/aws-sdk-go-v2/service/shield v1.27.3
+	github.com/aws/aws-sdk-go-v2/service/sts v1.30.3
 	github.com/aws/aws-sdk-go-v2/service/wafregional v1.23.3
 	github.com/aws/aws-sdk-go-v2/service/wafv2 v1.51.4
 	github.com/aws/smithy-go v1.22.1
@@ -56,16 +58,13 @@ require (
 	github.com/ajg/form v1.5.1 // indirect
 	github.com/andybalholm/brotli v1.0.4 // indirect
 	github.com/asaskevich/govalidator v0.0.0-20200428143746-21a406dcc535 // indirect
-	github.com/aws/aws-sdk-go-v2/credentials v1.17.27 // indirect
 	github.com/aws/aws-sdk-go-v2/internal/configsources v1.3.25 // indirect
 	github.com/aws/aws-sdk-go-v2/internal/endpoints/v2 v2.6.25 // indirect
 	github.com/aws/aws-sdk-go-v2/internal/ini v1.8.0 // indirect
-	github.com/aws/aws-sdk-go-v2/service/iam v1.36.3 // indirect
 	github.com/aws/aws-sdk-go-v2/service/internal/accept-encoding v1.11.3 // indirect
 	github.com/aws/aws-sdk-go-v2/service/internal/presigned-url v1.11.17 // indirect
 	github.com/aws/aws-sdk-go-v2/service/sso v1.22.4 // indirect
 	github.com/aws/aws-sdk-go-v2/service/ssooidc v1.26.4 // indirect
-	github.com/aws/aws-sdk-go-v2/service/sts v1.30.3 // indirect
 	github.com/beorn7/perks v1.0.1 // indirect
 	github.com/cespare/xxhash/v2 v2.3.0 // indirect
 	github.com/chai2010/gettext-go v1.0.2 // indirect

go.sum

@@ -60,7 +60,6 @@ github.com/aws/aws-sdk-go-v2/service/ec2 v1.173.0 h1:ta62lid9JkIpKZtZZXSj6rP2AqY
 github.com/aws/aws-sdk-go-v2/service/ec2 v1.173.0/go.mod h1:o6QDjdVKpP5EF0dp/VlvqckzuSDATr1rLdHt3A5m0YY=
 github.com/aws/aws-sdk-go-v2/service/elasticloadbalancingv2 v1.43.1 h1:L9Wt9zgtoYKIlaeFTy+EztGjL4oaXBBGtVXA+jaeYko=
 github.com/aws/aws-sdk-go-v2/service/elasticloadbalancingv2 v1.43.1/go.mod h1:yxzLdxt7bVGvIOPYIKFtiaJCJnx2ChlIIvlhW4QgI6M=
-github.com/aws/aws-sdk-go-v2/service/iam v1.36.3/go.mod h1:HSvujsK8xeEHMIB18oMXjSfqaN9cVqpo/MtHJIksQRk=
 github.com/aws/aws-sdk-go-v2/service/internal/accept-encoding v1.11.3 h1:dT3MqvGhSoaIhRseqw2I0yH81l7wiR2vjs57O51EAm8=
 github.com/aws/aws-sdk-go-v2/service/internal/accept-encoding v1.11.3/go.mod h1:GlAeCkHwugxdHaueRr4nhPuY+WW+gR8UjlcqzPr1SPI=
 github.com/aws/aws-sdk-go-v2/service/internal/presigned-url v1.11.17 h1:HGErhhrxZlQ044RiM+WdoZxp0p+EGM62y3L6pwA4olE=

pkg/deploy/shield/protection_manager.go

@@ -88,22 +88,46 @@ func (m *defaultProtectionManager) CreateProtection(ctx context.Context, resourc
 }
 
 func (m *defaultProtectionManager) DeleteProtection(ctx context.Context, resourceARN string, protectionID string) error {
-	req := &shieldsdk.DeleteProtectionInput{
-		ProtectionId: awssdk.String(protectionID),
-	}
-	m.logger.Info("disabling shield protection",
-		"resourceARN", resourceARN,
-		"protectionID", protectionID)
-	_, err := m.shieldClient.DeleteProtectionWithContext(ctx, req)
-	if err != nil {
-		return err
-	}
-	m.logger.Info("disabled shield protection",
-		"resourceARN", resourceARN)
+    req := &shieldsdk.DeleteProtectionInput{
+        ProtectionId: awssdk.String(protectionID),
+    }
+    m.logger.Info("disabling shield protection",
+        "resourceARN", resourceARN,
+        "protectionID", protectionID)
+    _, err := m.shieldClient.DeleteProtectionWithContext(ctx, req)
+    if err != nil {
+        return err
+    }
+    m.logger.Info("disabled shield protection",
+        "resourceARN", resourceARN)
+
+    // Remove the protection info from the cache
+    m.protectionInfoByResourceARNCache.Delete(resourceARN)
+
+    // Verify that the protection resource is deleted
+    err = m.verifyProtectionDeleted(ctx, protectionID)
+    if err != nil {
+        return err
+    }
+
+    return nil
+}
 
-	var protectionInfo *ProtectionInfo
-	m.protectionInfoByResourceARNCache.Set(resourceARN, protectionInfo, m.protectionInfoByResourceARNCacheTTL)
-	return nil
+func (m *defaultProtectionManager) verifyProtectionDeleted(ctx context.Context, protectionID string) error {
+    req := &shieldsdk.DescribeProtectionInput{
+        ProtectionId: awssdk.String(protectionID),
+    }
+    _, err := m.shieldClient.DescribeProtectionWithContext(ctx, req)
+    if err != nil {
+        var resourceNotFoundException *shieldtypes.ResourceNotFoundException
+        if errors.As(err, &resourceNotFoundException) {
+            // Protection resource is successfully deleted
+            return nil
+        }
+        return err
+    }
+    // Protection resource still exists
+    return errors.New("protection resource still exists")
 }
 
 func (m *defaultProtectionManager) GetProtection(ctx context.Context, resourceARN string) (*ProtectionInfo, error) {

pkg/deploy/shield/protection_manager_test.go

@@ -2,18 +2,19 @@ package shield
 
 import (
 	"context"
-	shieldtypes "github.com/aws/aws-sdk-go-v2/service/shield/types"
 	"testing"
 	"time"
 
-	shieldsdk "github.com/aws/aws-sdk-go-v2/service/shield"
+	shieldtypes "github.com/aws/aws-sdk-go-v2/service/shield/types"
+	"github.com/aws/aws-sdk-go-v2/aws"
 	"github.com/go-logr/logr"
 	"github.com/golang/mock/gomock"
 	"github.com/pkg/errors"
 	"github.com/stretchr/testify/assert"
 	"k8s.io/apimachinery/pkg/util/cache"
 	"sigs.k8s.io/aws-load-balancer-controller/pkg/aws/services"
 	"sigs.k8s.io/controller-runtime/pkg/log"
+	shieldsdk "github.com/aws/aws-sdk-go-v2/service/shield"
 )
 
 func Test_defaultProtectionManager_IsSubscribed(t *testing.T) {
@@ -169,3 +170,129 @@ func Test_defaultProtectionManager_IsSubscribed(t *testing.T) {
 		})
 	}
 }
+
+func Test_defaultProtectionManager_DeleteProtection(t *testing.T) {
+	type deleteProtectionCall struct {
+		req  *shieldsdk.DeleteProtectionInput
+		resp *shieldsdk.DeleteProtectionOutput
+		err  error
+	}
+	type describeProtectionCall struct {
+		req  *shieldsdk.DescribeProtectionInput
+		resp *shieldsdk.DescribeProtectionOutput
+		err  error
+	}
+	type fields struct {
+		deleteProtectionCalls      []deleteProtectionCall
+		describeProtectionCalls     []describeProtectionCall
+		protectionInfoByResourceARNCacheTTL time.Duration
+	}
+	type testCase struct {
+		resourceARN string
+		protectionID string
+		wantErr      error
+	}
+	tests := []struct {
+		name                string
+		fields              fields
+		testCases           []testCase
+	}{
+		{
+			name: "delete protection successfully",
+			fields: fields{
+				deleteProtectionCalls: []deleteProtectionCall{
+					{
+						req:  &shieldsdk.DeleteProtectionInput{ProtectionId: aws.String("protection-id")},
+						resp: &shieldsdk.DeleteProtectionOutput{},
+					},
+				},
+				describeProtectionCalls: []describeProtectionCall{
+					{
+						req:  &shieldsdk.DescribeProtectionInput{ProtectionId: aws.String("protection-id")},
+						err:  &shieldtypes.ResourceNotFoundException{},
+					},
+				},
+				protectionInfoByResourceARNCacheTTL: 10 * time.Minute,
+			},
+			testCases: []testCase{
+				{
+					resourceARN: "resource-arn",
+					protectionID: "protection-id",
+				},
+			},
+		},
+		{
+			name: "delete protection fails",
+			fields: fields{
+				deleteProtectionCalls: []deleteProtectionCall{
+					{
+						req: &shieldsdk.DeleteProtectionInput{ProtectionId: aws.String("protection-id")},
+						err: errors.New("some aws api error"),
+					},
+				},
+				protectionInfoByResourceARNCacheTTL: 10 * time.Minute,
+			},
+			testCases: []testCase{
+				{
+					resourceARN: "resource-arn",
+					protectionID: "protection-id",
+					wantErr:      errors.New("some aws api error"),
+				},
+			},
+		},
+		{
+			name: "protection still exists after deletion",
+			fields: fields{
+				deleteProtectionCalls: []deleteProtectionCall{
+					{
+						req:  &shieldsdk.DeleteProtectionInput{ProtectionId: aws.String("protection-id")},
+						resp: &shieldsdk.DeleteProtectionOutput{},
+					},
+				},
+				describeProtectionCalls: []describeProtectionCall{
+					{
+						req:  &shieldsdk.DescribeProtectionInput{ProtectionId: aws.String("protection-id")},
+						resp: &shieldsdk.DescribeProtectionOutput{Protection: &shieldtypes.Protection{}},
+					},
+				},
+				protectionInfoByResourceARNCacheTTL: 10 * time.Minute,
+			},
+			testCases: []testCase{
+				{
+					resourceARN: "resource-arn",
+					protectionID: "protection-id",
+					wantErr:      errors.New("protection resource still exists"),
+				},
+			},
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			ctrl := gomock.NewController(t)
+			defer ctrl.Finish()
+
+			shieldClient := services.NewMockShield(ctrl)
+			for _, call := range tt.fields.deleteProtectionCalls {
+				shieldClient.EXPECT().DeleteProtectionWithContext(gomock.Any(), call.req).Return(call.resp, call.err)
+			}
+			for _, call := range tt.fields.describeProtectionCalls {
+				shieldClient.EXPECT().DescribeProtectionWithContext(gomock.Any(), call.req).Return(call.resp, call.err)
+			}
+
+			m := &defaultProtectionManager{
+				shieldClient:                    shieldClient,
+				logger:                          logr.New(&log.NullLogSink{}),
+				protectionInfoByResourceARNCache: cache.NewExpiring(),
+				protectionInfoByResourceARNCacheTTL: tt.fields.protectionInfoByResourceARNCacheTTL,
+			}
+			for _, testCase := range tt.testCases {
+				err := m.DeleteProtection(context.Background(), testCase.resourceARN, testCase.protectionID)
+				if testCase.wantErr != nil {
+					assert.EqualError(t, err, testCase.wantErr.Error())
+				} else {
+					assert.NoError(t, err)
+				}
+			}
+		})
+	}
+}