Filter out AWS internal tags when reconciling

kubernetes-sigs · Oct 29, 2024 · 4c61f2b · 4c61f2b
1 parent 1416be7
commit 4c61f2b
Show file tree

Hide file tree

Showing 2 changed files with 80 additions and 25 deletions.
diff --git a/pkg/cloud/tags/tags.go b/pkg/cloud/tags/tags.go
@@ -21,6 +21,7 @@ import (
 	"context"
 	"fmt"
 	"sort"
+	"strings"
 
 	"github.com/aws/aws-sdk-go/aws"
 	"github.com/aws/aws-sdk-go/service/ec2"
@@ -32,6 +33,10 @@ import (
 	infrav1 "sigs.k8s.io/cluster-api-provider-aws/v2/api/v1beta2"
 )
 
+const (
+	AwsInternalTagPrefix = "aws:" // AwsInternalTagPrefix is the prefix for AWS internal tags, which are reserved for internal AWS use.
+)
+
 var (
 	// ErrBuildParamsRequired defines an error for when no build params are supplied.
 	ErrBuildParamsRequired = errors.New("no build params supplied")
@@ -99,7 +104,10 @@ func WithEC2(ec2client ec2iface.EC2API) BuilderOption {
 			// For testing, we need sorted keys
 			sortedKeys := make([]string, 0, len(tags))
 			for k := range tags {
-				sortedKeys = append(sortedKeys, k)
+				// We want to filter out the tag keys that start with `aws:` as they are reserved for internal AWS use.
+				if !strings.HasPrefix(k, AwsInternalTagPrefix) {
+					sortedKeys = append(sortedKeys, k)
+				}
 			}
 			sort.Strings(sortedKeys)
 
@@ -127,10 +135,12 @@ func WithEKS(eksclient eksiface.EKSAPI) BuilderOption {
 	return func(b *Builder) {
 		b.applyFunc = func(params *infrav1.BuildParams) error {
 			tags := infrav1.Build(*params)
-
 			eksTags := make(map[string]*string, len(tags))
 			for k, v := range tags {
-				eksTags[k] = aws.String(v)
+				// We want to filter out the tag keys that start with `aws:` as they are reserved for internal AWS use.
+				if !strings.HasPrefix(k, AwsInternalTagPrefix) {
+					eksTags[k] = aws.String(v)
+				}
 			}
 
 			tagResourcesInput := &eks.TagResourceInput{

diff --git a/pkg/cloud/tags/tags_test.go b/pkg/cloud/tags/tags_test.go
@@ -34,14 +34,7 @@ import (
 )
 
 var (
-	bp = infrav1.BuildParams{
-		Lifecycle:   infrav1.ResourceLifecycleOwned,
-		ClusterName: "testcluster",
-		Name:        aws.String("test"),
-		Role:        aws.String("testrole"),
-		Additional:  map[string]string{"k1": "v1"},
-	}
-	tags = []*ec2.Tag{
+	expectedTags = []*ec2.Tag{
 		{
 			Key:   aws.String("Name"),
 			Value: aws.String("test"),
@@ -140,30 +133,64 @@ func TestTagsEnsureWithEC2(t *testing.T) {
 		expect  func(m *mocks.MockEC2APIMockRecorder)
 	}{
 		{
-			name:    "Should return error when create tag fails",
-			builder: Builder{params: &bp},
+			name: "Should return error when create tag fails",
+			builder: Builder{params: &infrav1.BuildParams{
+				Lifecycle:   infrav1.ResourceLifecycleOwned,
+				ClusterName: "testcluster",
+				Name:        aws.String("test"),
+				Role:        aws.String("testrole"),
+				Additional:  map[string]string{"k1": "v1"},
+			}},
 			expect: func(m *mocks.MockEC2APIMockRecorder) {
 				m.CreateTagsWithContext(context.TODO(), gomock.Eq(&ec2.CreateTagsInput{
 					Resources: aws.StringSlice([]string{""}),
-					Tags:      tags,
+					Tags:      expectedTags,
 				})).Return(nil, errors.New("failed to create tag"))
 			},
 		},
 		{
-			name:    "Should return error when optional configuration for builder is nil",
-			builder: Builder{params: &bp, applyFunc: nil},
+			name: "Should return error when optional configuration for builder is nil",
+			builder: Builder{params: &infrav1.BuildParams{
+				Lifecycle:   infrav1.ResourceLifecycleOwned,
+				ClusterName: "testcluster",
+				Name:        aws.String("test"),
+				Role:        aws.String("testrole"),
+				Additional:  map[string]string{"k1": "v1"},
+			}, applyFunc: nil},
 		},
 		{
 			name:    "Should return error when build params is nil",
 			builder: Builder{params: nil},
 		},
 		{
-			name:    "Should ensure tags successfully",
-			builder: Builder{params: &bp},
+			name: "Should ensure tags successfully",
+			builder: Builder{params: &infrav1.BuildParams{
+				Lifecycle:   infrav1.ResourceLifecycleOwned,
+				ClusterName: "testcluster",
+				Name:        aws.String("test"),
+				Role:        aws.String("testrole"),
+				Additional:  map[string]string{"k1": "v1"},
+			}},
+			expect: func(m *mocks.MockEC2APIMockRecorder) {
+				m.CreateTagsWithContext(context.TODO(), gomock.Eq(&ec2.CreateTagsInput{
+					Resources: aws.StringSlice([]string{""}),
+					Tags:      expectedTags,
+				})).Return(nil, nil)
+			},
+		},
+		{
+			name: "Should filter internal aws tags",
+			builder: Builder{params: &infrav1.BuildParams{
+				Lifecycle:   infrav1.ResourceLifecycleOwned,
+				ClusterName: "testcluster",
+				Name:        aws.String("test"),
+				Role:        aws.String("testrole"),
+				Additional:  map[string]string{"k1": "v1", "aws:cloudformation:stack-name": "cloudformation-stack-name"},
+			}},
 			expect: func(m *mocks.MockEC2APIMockRecorder) {
 				m.CreateTagsWithContext(context.TODO(), gomock.Eq(&ec2.CreateTagsInput{
 					Resources: aws.StringSlice([]string{""}),
-					Tags:      tags,
+					Tags:      expectedTags,
 				})).Return(nil, nil)
 			},
 		},
@@ -198,8 +225,14 @@ func TestTagsEnsureWithEKS(t *testing.T) {
 		expect  func(m *mock_eksiface.MockEKSAPIMockRecorder)
 	}{
 		{
-			name:    "Should return error when tag resources fails",
-			builder: Builder{params: &bp},
+			name: "Should return error when tag resources fails",
+			builder: Builder{params: &infrav1.BuildParams{
+				Lifecycle:   infrav1.ResourceLifecycleOwned,
+				ClusterName: "testcluster",
+				Name:        aws.String("test"),
+				Role:        aws.String("testrole"),
+				Additional:  map[string]string{"k1": "v1"},
+			}},
 			expect: func(m *mock_eksiface.MockEKSAPIMockRecorder) {
 				m.TagResource(gomock.Eq(&eks.TagResourceInput{
 					ResourceArn: aws.String(""),
@@ -208,8 +241,14 @@ func TestTagsEnsureWithEKS(t *testing.T) {
 			},
 		},
 		{
-			name:    "Should ensure tags successfully",
-			builder: Builder{params: &bp},
+			name: "Should ensure tags successfully",
+			builder: Builder{params: &infrav1.BuildParams{
+				Lifecycle:   infrav1.ResourceLifecycleOwned,
+				ClusterName: "testcluster",
+				Name:        aws.String("test"),
+				Role:        aws.String("testrole"),
+				Additional:  map[string]string{"k1": "v1"},
+			}},
 			expect: func(m *mock_eksiface.MockEKSAPIMockRecorder) {
 				m.TagResource(gomock.Eq(&eks.TagResourceInput{
 					ResourceArn: aws.String(""),
@@ -243,10 +282,16 @@ func TestTagsEnsureWithEKS(t *testing.T) {
 
 func TestTagsBuildParamsToTagSpecification(t *testing.T) {
 	g := NewWithT(t)
-	tagSpec := BuildParamsToTagSpecification("test-resource", bp)
+	tagSpec := BuildParamsToTagSpecification("test-resource", infrav1.BuildParams{
+		Lifecycle:   infrav1.ResourceLifecycleOwned,
+		ClusterName: "testcluster",
+		Name:        aws.String("test"),
+		Role:        aws.String("testrole"),
+		Additional:  map[string]string{"k1": "v1"},
+	})
 	expectedTagSpec := &ec2.TagSpecification{
 		ResourceType: aws.String("test-resource"),
-		Tags:         tags,
+		Tags:         expectedTags,
 	}
 	g.Expect(expectedTagSpec).To(Equal(tagSpec))
 }