refactor: cleanup functions filter resource

The the cleanup functions now accepted the full list of AWS resources
and then they filter which resources they want to delete themselves.

Signed-off-by: Richard Case <[email protected]>

Author: richardcase

pkg/cloud/services/gc/cleanup.go

@@ -20,6 +20,7 @@ import (
 	"context"
 	"fmt"
 	"strconv"
+	"strings"
 
 	"github.com/aws/aws-sdk-go/aws"
 	"github.com/aws/aws-sdk-go/aws/arn"
@@ -51,13 +52,13 @@ func (s *Service) ReconcileDelete(ctx context.Context) error {
 		return fmt.Errorf("converting value %s of annotation %s to bool: %w", val, expinfrav1.ExternalResourceGCAnnotation, err)
 	}
 
-	if shouldGC {
-		if err := s.deleteResources(ctx); err != nil {
-			return fmt.Errorf("deleting workload services of type load balancer: %w", err)
-		}
+	if !shouldGC {
+		s.scope.Info("cluster opted-out of garbage collection")
+
+		return nil
 	}
 
-	return nil
+	return s.deleteResources(ctx)
 }
 
 func (s *Service) deleteResources(ctx context.Context) error {
@@ -79,7 +80,7 @@ func (s *Service) deleteResources(ctx context.Context) error {
 		return fmt.Errorf("getting tagged resources: %w", err)
 	}
 
-	resources := map[string][]*rgapi.ResourceTagMapping{}
+	resources := []*AWSResource{}
 
 	for i := range awsOutput.ResourceTagMappingList {
 		mapping := awsOutput.ResourceTagMappingList[i]
@@ -88,34 +89,33 @@ func (s *Service) deleteResources(ctx context.Context) error {
 			return fmt.Errorf("parsing resource arn %s: %w", *mapping.ResourceARN, err)
 		}
 
-		_, found := s.cleanupFuncs[parsedArn.Service]
-		if !found {
-			s.scope.V(2).Info("skipping clean-up of tagged resource for service", "service", parsedArn.Service, "arn", mapping.ResourceARN)
-
-			continue
+		tags := map[string]string{}
+		for _, rgTag := range mapping.Tags {
+			tags[*rgTag.Key] = *rgTag.Value
 		}
 
-		resources[parsedArn.Service] = append(resources[parsedArn.Service], mapping)
+		resources = append(resources, &AWSResource{
+			ARN:  &parsedArn,
+			Tags: tags,
+		})
 	}
 
-	for svcName, svcResources := range resources {
-		cleanupFunc := s.cleanupFuncs[svcName]
-
-		s.scope.V(2).Info("Calling clean-up function for service", "service_name", svcName)
-		if deleteErr := cleanupFunc(ctx, svcResources); deleteErr != nil {
-			return fmt.Errorf("deleting resources for service %s: %w", svcName, deleteErr)
-		}
+	if deleteErr := s.cleanupFuncs.Execute(ctx, resources); deleteErr != nil {
+		return fmt.Errorf("deleting resources: %w", deleteErr)
 	}
 
 	return nil
 }
 
-func getTagValue(tagName string, mapping *rgapi.ResourceTagMapping) string {
-	for _, tag := range mapping.Tags {
-		if *tag.Key == tagName {
-			return *tag.Value
-		}
+func (s *Service) isMatchingResource(resource *AWSResource, serviceName, resourceName string) bool {
+	if resource.ARN.Service != serviceName {
+		s.scope.V(5).Info("Resource not for service", "arn", resource.ARN.String(), "service_name", serviceName, "resource_name", resourceName)
+		return false
+	}
+	if !strings.HasPrefix(resource.ARN.Resource, resourceName+"/") {
+		s.scope.V(5).Info("Resource type does not match", "arn", resource.ARN.String(), "service_name", serviceName, "resource_name", resourceName)
+		return false
 	}
 
-	return ""
+	return true
 }

pkg/cloud/services/gc/cleanup_test.go

@@ -22,6 +22,7 @@ import (
 
 	"github.com/aws/aws-sdk-go/aws"
 	"github.com/aws/aws-sdk-go/aws/request"
+	"github.com/aws/aws-sdk-go/service/ec2"
 	"github.com/aws/aws-sdk-go/service/elb"
 	"github.com/aws/aws-sdk-go/service/elbv2"
 	rgapi "github.com/aws/aws-sdk-go/service/resourcegroupstaggingapi"
@@ -471,6 +472,19 @@ func TestReconcileDelete(t *testing.T) {
 									},
 								},
 							},
+							{
+								ResourceARN: aws.String("arn:aws:ec2:eu-west-2:1234567890:security-group/sg-123456"),
+								Tags: []*rgapi.Tag{
+									{
+										Key:   aws.String("kubernetes.io/cluster/cluster1"),
+										Value: aws.String("owned"),
+									},
+									{
+										Key:   aws.String(serviceNameTag),
+										Value: aws.String("default/svc1"),
+									},
+								},
+							},
 						},
 					}, nil
 				})
@@ -485,7 +499,11 @@ func TestReconcileDelete(t *testing.T) {
 					TargetGroupArn: aws.String("arn:aws:elasticloadbalancing:eu-west-2:1234567890:targetgroup/k8s-default-podinfo-2c868b281a/e979fe9bd6825433"),
 				})
 			},
-			ec2Mocks:  func(m *mocks.MockEC2APIMockRecorder) {},
+			ec2Mocks: func(m *mocks.MockEC2APIMockRecorder) {
+				m.DeleteSecurityGroupWithContext(gomock.Any(), &ec2.DeleteSecurityGroupInput{
+					GroupId: aws.String("sg-123456"),
+				})
+			},
 			expectErr: false,
 		},
 		{
@@ -528,6 +546,46 @@ func TestReconcileDelete(t *testing.T) {
 			ec2Mocks:   func(m *mocks.MockEC2APIMockRecorder) {},
 			expectErr:  false,
 		},
+		{
+			name:         "eks with security group created by EKS",
+			clusterScope: createManageScope(t, ""),
+			rgAPIMocks: func(m *mocks.MockResourceGroupsTaggingAPIAPIMockRecorder) {
+				m.GetResourcesWithContext(gomock.Any(), &rgapi.GetResourcesInput{
+					TagFilters: []*rgapi.TagFilter{
+						{
+							Key:    aws.String("kubernetes.io/cluster/eks-test-cluster"),
+							Values: []*string{aws.String("owned")},
+						},
+					},
+				}).DoAndReturn(func(awsCtx context.Context, input *rgapi.GetResourcesInput, opts ...request.Option) (*rgapi.GetResourcesOutput, error) {
+					return &rgapi.GetResourcesOutput{
+						ResourceTagMappingList: []*rgapi.ResourceTagMapping{
+							{
+								ResourceARN: aws.String("arn:aws:ec2:eu-west-2:1234567890:security-group/sg-123456"),
+								Tags: []*rgapi.Tag{
+									{
+										Key:   aws.String("kubernetes.io/cluster/cluster1"),
+										Value: aws.String("owned"),
+									},
+									{
+										Key:   aws.String(serviceNameTag),
+										Value: aws.String("default/svc1"),
+									},
+									{
+										Key:   aws.String(eksClusterNameTag),
+										Value: aws.String("default_eks_test_cluster"),
+									},
+								},
+							},
+						},
+					}, nil
+				})
+			},
+			elbMocks:   func(m *mocks.MockELBAPIMockRecorder) {},
+			elbv2Mocks: func(m *mocks.MockELBV2APIMockRecorder) {},
+			ec2Mocks:   func(m *mocks.MockEC2APIMockRecorder) {},
+			expectErr:  false,
+		},
 	}
 
 	for _, tc := range testCases {

pkg/cloud/services/gc/ec2.go

@@ -22,49 +22,46 @@ import (
 	"strings"
 
 	"github.com/aws/aws-sdk-go/aws"
-	"github.com/aws/aws-sdk-go/aws/arn"
 	"github.com/aws/aws-sdk-go/service/ec2"
-	rgapi "github.com/aws/aws-sdk-go/service/resourcegroupstaggingapi"
 )
 
-func (s *Service) deleteEC2Resources(ctx context.Context, resources []*rgapi.ResourceTagMapping) error {
-	for i := range resources {
-		res := resources[i]
-
-		parsedARN, err := arn.Parse(*res.ResourceARN)
-		if err != nil {
-			return fmt.Errorf("parsing arn %s: %w", *res.ResourceARN, err)
+func (s *Service) deleteSecurityGroups(ctx context.Context, resources []*AWSResource) error {
+	for _, resource := range resources {
+		if !s.isSecurityGroupToDelete(resource) {
+			s.scope.V(5).Info("Resource not a security group for deletion", "arn", resource.ARN.String())
+			continue
 		}
 
-		if strings.HasPrefix(parsedARN.Resource, "security-group/") {
-			s.scope.V(2).Info("Deleting Security group", "arn", parsedARN.String())
-			return s.deleteSecurityGroup(ctx, &parsedARN, res)
+		groupID := strings.ReplaceAll(resource.ARN.Resource, "security-group/", "")
+		if err := s.deleteSecurityGroup(ctx, groupID); err != nil {
+			return fmt.Errorf("deleting security group %s: %w", groupID, err)
 		}
 	}
-
-	s.scope.V(2).Info("Finished deleting ec2 resources")
+	s.scope.V(2).Info("Finished processing resources for security group deletion")
 
 	return nil
 }
 
-func (s *Service) deleteSecurityGroup(ctx context.Context, lbARN *arn.ARN, mapping *rgapi.ResourceTagMapping) error {
-	eksClusterName := getTagValue(eksClusterNameTag, mapping)
-	if eksClusterName != "" {
-		s.scope.V(2).Info("Security group created by EKS directly, skipping deletion", "cluster_name", eksClusterName)
-
-		return nil
+func (s *Service) isSecurityGroupToDelete(resource *AWSResource) bool {
+	if !s.isMatchingResource(resource, ec2.ServiceName, "security-group") {
+		return false
 	}
+	if eksClusterName := resource.Tags[eksClusterNameTag]; eksClusterName != "" {
+		s.scope.V(5).Info("Security group was created by EKS directly", "arn", resource.ARN.String(), "check", "securitygroup", "cluster_name", eksClusterName)
+		return false
+	}
+	s.scope.V(5).Info("Resource is a security group to delete", "arn", resource.ARN.String(), "check", "securitygroup")
 
-	//TODO: should we check for the security group name start with k8s-elb-
+	return true
+}
 
-	groupID := strings.ReplaceAll(lbARN.Resource, "security-group/", "")
+func (s *Service) deleteSecurityGroup(ctx context.Context, securityGroupID string) error {
 	input := ec2.DeleteSecurityGroupInput{
-		GroupId: aws.String(groupID),
+		GroupId: aws.String(securityGroupID),
 	}
 
-	s.scope.V(2).Info("Deleting security group", "group_id", groupID, "arn", lbARN.String())
-	_, err := s.ec2Client.DeleteSecurityGroupWithContext(ctx, &input)
-	if err != nil {
+	s.scope.V(2).Info("Deleting security group", "group_id", securityGroupID)
+	if _, err := s.ec2Client.DeleteSecurityGroupWithContext(ctx, &input); err != nil {
 		return fmt.Errorf("deleting security group: %w", err)
 	}