Skip to content

Commit

Permalink
refactor: cleanup functions filter resource
Browse files Browse the repository at this point in the history 
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 <richard.case@outlook.com>
  • Loading branch information
@richardcase
richardcase committed Aug 1, 2022
1 parent 587b8bc commit a65ccb2
Show file tree
Hide file tree
Showing 5 changed files with 191 additions and 136 deletions.
50 changes: 25 additions & 25 deletions pkg/cloud/services/gc/cleanup.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
"context"
"fmt"
"strconv"
"strings"

"github.com/aws/aws-sdk-go/aws"
"github.com/aws/aws-sdk-go/aws/arn"
Expand Down Expand Up @@ -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 {
Expand All @@ -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]
Expand All @@ -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
}
60 changes: 59 additions & 1 deletion pkg/cloud/services/gc/cleanup_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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
})
Expand All @@ -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,
},
{
Expand Down Expand Up @@ -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 {
Expand Down
49 changes: 23 additions & 26 deletions pkg/cloud/services/gc/ec2.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}

Expand Down
Loading

0 comments on commit a65ccb2

Please sign in to comment.