diff --git a/controllers/awscluster_controller_test.go b/controllers/awscluster_controller_test.go index 491a90cde5..c01b7944a6 100644 --- a/controllers/awscluster_controller_test.go +++ b/controllers/awscluster_controller_test.go @@ -823,6 +823,7 @@ func mockedVPCCallsForExistingVPCAndSubnets(m *mocks.MockEC2APIMockRecorder) { }, }, }, nil) + // subnet-1 has no existing tags, so both new tags are applied m.CreateTags(context.TODO(), gomock.Eq(&ec2.CreateTagsInput{ Resources: []string{"subnet-1"}, Tags: []ec2types.Tag{ @@ -836,13 +837,10 @@ func mockedVPCCallsForExistingVPCAndSubnets(m *mocks.MockEC2APIMockRecorder) { }, }, })).Return(&ec2.CreateTagsOutput{}, nil) + // subnet-2 already has `kubernetes.io/cluster/test-cluster=shared`, so only the ELB role tag is new m.CreateTags(context.TODO(), gomock.Eq(&ec2.CreateTagsInput{ Resources: []string{"subnet-2"}, Tags: []ec2types.Tag{ - { - Key: aws.String("kubernetes.io/cluster/test-cluster"), - Value: aws.String("shared"), - }, { Key: aws.String("kubernetes.io/role/elb"), Value: aws.String("1"), diff --git a/pkg/cloud/services/network/subnets_test.go b/pkg/cloud/services/network/subnets_test.go index b87080bf60..65f0f3c763 100644 --- a/pkg/cloud/services/network/subnets_test.go +++ b/pkg/cloud/services/network/subnets_test.go @@ -2779,16 +2779,12 @@ func TestReconcileSubnets(t *testing.T) { }, }), gomock.Any()).Return(&ec2.DescribeNatGatewaysOutput{}, nil) - // Public subnet + // Public subnet - only new or changed tags expectedAppliedAwsTagsForSubnet1 := []types.Tag{ { Key: aws.String("Name"), Value: aws.String("test-cluster-subnet-public-us-east-1a"), }, - { - Key: aws.String("kubernetes.io/cluster/test-cluster"), - Value: aws.String("owned"), - }, { Key: aws.String("kubernetes.io/role/elb"), Value: aws.String("1"), @@ -2797,10 +2793,6 @@ func TestReconcileSubnets(t *testing.T) { Key: aws.String("sigs.k8s.io/cluster-api-provider-aws/cluster/test-cluster"), Value: aws.String("owned"), }, - { - Key: aws.String("sigs.k8s.io/cluster-api-provider-aws/role"), - Value: aws.String("public"), - }, { Key: aws.String("this-tag-is-in-the-spec"), Value: aws.String("but-its-not-on-aws"), @@ -2811,16 +2803,12 @@ func TestReconcileSubnets(t *testing.T) { })). Return(nil, nil) - // Private subnet + // Private subnet - only new or changed tags expectedAppliedAwsTagsForSubnet2 := []types.Tag{ { Key: aws.String("Name"), Value: aws.String("test-cluster-subnet-private-us-east-1a"), }, - { - Key: aws.String("kubernetes.io/cluster/test-cluster"), - Value: aws.String("owned"), - }, { Key: aws.String("kubernetes.io/role/internal-elb"), Value: aws.String("1"), @@ -2829,10 +2817,6 @@ func TestReconcileSubnets(t *testing.T) { Key: aws.String("sigs.k8s.io/cluster-api-provider-aws/cluster/test-cluster"), Value: aws.String("owned"), }, - { - Key: aws.String("sigs.k8s.io/cluster-api-provider-aws/role"), - Value: aws.String("private"), - }, { Key: aws.String("subnet-2-this-tag-is-in-the-spec"), Value: aws.String("subnet-2-but-its-not-on-aws"), diff --git a/pkg/cloud/services/network/vpc_test.go b/pkg/cloud/services/network/vpc_test.go index 9b9f3a1e4d..029ce313a7 100644 --- a/pkg/cloud/services/network/vpc_test.go +++ b/pkg/cloud/services/network/vpc_test.go @@ -173,22 +173,10 @@ func TestReconcileVPC(t *testing.T) { m.CreateTags(context.TODO(), &ec2.CreateTagsInput{ Resources: []string{"vpc-exists"}, Tags: []types.Tag{ - { - Key: aws.String("Name"), - Value: aws.String("test-cluster-vpc"), - }, { Key: aws.String("additional"), Value: aws.String("tags"), }, - { - Key: aws.String("sigs.k8s.io/cluster-api-provider-aws/cluster/test-cluster"), - Value: aws.String("owned"), - }, - { - Key: aws.String("sigs.k8s.io/cluster-api-provider-aws/role"), - Value: aws.String("common"), - }, }, }) m.DescribeVpcAttribute(context.TODO(), gomock.AssignableToTypeOf(&ec2.DescribeVpcAttributeInput{})). diff --git a/pkg/cloud/tags/tags.go b/pkg/cloud/tags/tags.go index 0b8b362d41..bd7e7b3501 100644 --- a/pkg/cloud/tags/tags.go +++ b/pkg/cloud/tags/tags.go @@ -57,7 +57,7 @@ type BuilderOption func(*Builder) // Builder is the interface for a tags builder. type Builder struct { params *infrav1.BuildParams - applyFunc func(params *infrav1.BuildParams) error + applyFunc func(params *infrav1.BuildParams, diff infrav1.Tags) error } // New creates a new TagsBuilder with the specified build parameters @@ -74,28 +74,19 @@ func New(params *infrav1.BuildParams, opts ...BuilderOption) *Builder { return builder } -// Apply tags a resource with tags including the cluster tag. -func (b *Builder) Apply() error { +// Ensure applies the tags if the current tags differ from the params. +// Only new or changed tags are passed to the apply function. +func (b *Builder) Ensure(current infrav1.Tags) error { if b.params == nil { return ErrBuildParamsRequired } if b.applyFunc == nil { return ErrApplyFuncRequired } - - if err := b.applyFunc(b.params); err != nil { - return fmt.Errorf("failed applying tags: %w", err) - } - return nil -} - -// Ensure applies the tags if the current tags differ from the params. -func (b *Builder) Ensure(current infrav1.Tags) error { - if b.params == nil { - return ErrBuildParamsRequired - } if diff := computeDiff(current, *b.params); len(diff) > 0 { - return b.Apply() + if err := b.applyFunc(b.params, diff); err != nil { + return fmt.Errorf("failed applying tags: %w", err) + } } return nil } @@ -103,13 +94,12 @@ func (b *Builder) Ensure(current infrav1.Tags) error { // WithEC2 is used to denote that the tags builder will be using EC2. func WithEC2(ec2client common.EC2API) BuilderOption { return func(b *Builder) { - b.applyFunc = func(params *infrav1.BuildParams) error { - tags := infrav1.Build(*params) - awsTags := make([]ec2types.Tag, 0, len(tags)) + b.applyFunc = func(params *infrav1.BuildParams, diff infrav1.Tags) error { + awsTags := make([]ec2types.Tag, 0, len(diff)) // For testing, we need sorted keys - sortedKeys := make([]string, 0, len(tags)) - for k := range tags { + sortedKeys := make([]string, 0, len(diff)) + for k := range diff { // 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) @@ -120,7 +110,7 @@ func WithEC2(ec2client common.EC2API) BuilderOption { for _, key := range sortedKeys { tag := ec2types.Tag{ Key: aws.String(key), - Value: aws.String(tags[key]), + Value: aws.String(diff[key]), } awsTags = append(awsTags, tag) } @@ -139,10 +129,9 @@ func WithEC2(ec2client common.EC2API) BuilderOption { // WithEKS is used to specify that the tags builder will be targeting EKS. func WithEKS(ctx context.Context, eksclient eksClient) 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 { + b.applyFunc = func(params *infrav1.BuildParams, diff infrav1.Tags) error { + eksTags := make(map[string]*string, len(diff)) + for k, v := range diff { // 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) diff --git a/pkg/cloud/tags/tags_test.go b/pkg/cloud/tags/tags_test.go index 3b84e79a95..3b53cb6da0 100644 --- a/pkg/cloud/tags/tags_test.go +++ b/pkg/cloud/tags/tags_test.go @@ -281,6 +281,270 @@ func TestTagsEnsureWithEKS(t *testing.T) { } } +func TestEnsureOnlyAppliesDiffTagsEC2(t *testing.T) { + pName := "test" + pRole := "testrole" + params := &infrav1.BuildParams{ + Lifecycle: infrav1.ResourceLifecycleOwned, + ClusterName: "testcluster", + ResourceID: "res-123", + Name: &pName, + Role: &pRole, + Additional: map[string]string{"k1": "v1"}, + } + + tests := []struct { + name string + current infrav1.Tags + expectedTags []ec2types.Tag + expect func(m *mocks.MockEC2APIMockRecorder, expectedTags []ec2types.Tag) + }{ + { + name: "no current tags, all tags are new", + current: nil, + expectedTags: []ec2types.Tag{ + {Key: aws.String("Name"), Value: aws.String(pName)}, + {Key: aws.String("k1"), Value: aws.String("v1")}, + {Key: aws.String(infrav1.ClusterTagKey("testcluster")), Value: aws.String(string(infrav1.ResourceLifecycleOwned))}, + {Key: aws.String(infrav1.NameAWSClusterAPIRole), Value: aws.String(pRole)}, + }, + expect: func(m *mocks.MockEC2APIMockRecorder, expectedTags []ec2types.Tag) { + m.CreateTags(context.TODO(), gomock.Eq(&ec2.CreateTagsInput{ + Resources: []string{"res-123"}, + Tags: expectedTags, + })).Return(nil, nil) + }, + }, + { + name: "all tags match, no apply call", + current: infrav1.Tags{ + "Name": pName, + "k1": "v1", + infrav1.ClusterTagKey("testcluster"): string(infrav1.ResourceLifecycleOwned), + infrav1.NameAWSClusterAPIRole: pRole, + }, + }, + { + name: "only changed tags are passed to apply", + current: infrav1.Tags{ + "Name": pName, + "k1": "old-value", + infrav1.ClusterTagKey("testcluster"): string(infrav1.ResourceLifecycleOwned), + infrav1.NameAWSClusterAPIRole: pRole, + }, + expectedTags: []ec2types.Tag{ + {Key: aws.String("k1"), Value: aws.String("v1")}, + }, + expect: func(m *mocks.MockEC2APIMockRecorder, expectedTags []ec2types.Tag) { + m.CreateTags(context.TODO(), gomock.Eq(&ec2.CreateTagsInput{ + Resources: []string{"res-123"}, + Tags: expectedTags, + })).Return(nil, nil) + }, + }, + { + name: "only missing tags are passed to apply", + current: infrav1.Tags{ + "Name": pName, + infrav1.ClusterTagKey("testcluster"): string(infrav1.ResourceLifecycleOwned), + infrav1.NameAWSClusterAPIRole: pRole, + }, + expectedTags: []ec2types.Tag{ + {Key: aws.String("k1"), Value: aws.String("v1")}, + }, + expect: func(m *mocks.MockEC2APIMockRecorder, expectedTags []ec2types.Tag) { + m.CreateTags(context.TODO(), gomock.Eq(&ec2.CreateTagsInput{ + Resources: []string{"res-123"}, + Tags: expectedTags, + })).Return(nil, nil) + }, + }, + { + name: "external tags do not cause apply", + current: infrav1.Tags{ + "Name": pName, + "k1": "v1", + infrav1.ClusterTagKey("testcluster"): string(infrav1.ResourceLifecycleOwned), + infrav1.NameAWSClusterAPIRole: pRole, + "external-tag": "external-value", + }, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + g := NewWithT(t) + + // The `diff` parameter should equal the test case's `expectedTags`. + // This is to show better error messages than with mocking. + var actualDiff infrav1.Tags + builder := New(params, func(b *Builder) { + b.applyFunc = func(_ *infrav1.BuildParams, diff infrav1.Tags) error { + actualDiff = diff + return nil + } + }) + err := builder.Ensure(tc.current) + g.Expect(err).To(BeNil()) + var expectedTagsAsInfraType infrav1.Tags + for _, tag := range tc.expectedTags { + if expectedTagsAsInfraType == nil { + expectedTagsAsInfraType = infrav1.Tags{} + } + expectedTagsAsInfraType[*tag.Key] = *tag.Value + } + g.Expect(actualDiff).To(Equal(expectedTagsAsInfraType), "Diff calculation is wrong, or the test case has a wrong expectedTags") + + // Now check with mocking since the actual AWS API calls are most important. + // This ensures that the `diff` is used, and not all tags (incl. unchanged ones) + // get sent in the AWS request. + mockCtrl := gomock.NewController(t) + ec2Mock := mocks.NewMockEC2API(mockCtrl) + + if tc.expect != nil { + tc.expect(ec2Mock.EXPECT(), tc.expectedTags) + } + + builder = New(params, WithEC2(ec2Mock)) + + err = builder.Ensure(tc.current) + g.Expect(err).To(BeNil()) + }) + } +} + +func TestEnsureOnlyAppliesDiffTagsEKS(t *testing.T) { + pName := "test" + pRole := "testrole" + params := &infrav1.BuildParams{ + Lifecycle: infrav1.ResourceLifecycleOwned, + ClusterName: "testcluster", + ResourceID: "res-123", + Name: &pName, + Role: &pRole, + Additional: map[string]string{"k1": "v1"}, + } + + tests := []struct { + name string + current infrav1.Tags + expectedTags map[string]string + expect func(m *mock_eksiface.MockEKSAPIMockRecorder, expectedTags map[string]string) + }{ + { + name: "no current tags, all tags are new", + current: nil, + expectedTags: map[string]string{ + "Name": pName, + "k1": "v1", + infrav1.ClusterTagKey("testcluster"): string(infrav1.ResourceLifecycleOwned), + infrav1.NameAWSClusterAPIRole: pRole, + }, + expect: func(m *mock_eksiface.MockEKSAPIMockRecorder, expectedTags map[string]string) { + m.TagResource(gomock.Eq(context.TODO()), gomock.Eq(&eks.TagResourceInput{ + ResourceArn: aws.String("res-123"), + Tags: expectedTags, + })).Return(nil, nil) + }, + }, + { + name: "all tags match, no apply call", + current: infrav1.Tags{ + "Name": pName, + "k1": "v1", + infrav1.ClusterTagKey("testcluster"): string(infrav1.ResourceLifecycleOwned), + infrav1.NameAWSClusterAPIRole: pRole, + }, + }, + { + name: "only changed tags are passed to apply", + current: infrav1.Tags{ + "Name": pName, + "k1": "old-value", + infrav1.ClusterTagKey("testcluster"): string(infrav1.ResourceLifecycleOwned), + infrav1.NameAWSClusterAPIRole: pRole, + }, + expectedTags: map[string]string{ + "k1": "v1", + }, + expect: func(m *mock_eksiface.MockEKSAPIMockRecorder, expectedTags map[string]string) { + m.TagResource(gomock.Eq(context.TODO()), gomock.Eq(&eks.TagResourceInput{ + ResourceArn: aws.String("res-123"), + Tags: expectedTags, + })).Return(nil, nil) + }, + }, + { + name: "only missing tags are passed to apply", + current: infrav1.Tags{ + "Name": pName, + infrav1.ClusterTagKey("testcluster"): string(infrav1.ResourceLifecycleOwned), + infrav1.NameAWSClusterAPIRole: pRole, + }, + expectedTags: map[string]string{ + "k1": "v1", + }, + expect: func(m *mock_eksiface.MockEKSAPIMockRecorder, expectedTags map[string]string) { + m.TagResource(gomock.Eq(context.TODO()), gomock.Eq(&eks.TagResourceInput{ + ResourceArn: aws.String("res-123"), + Tags: expectedTags, + })).Return(nil, nil) + }, + }, + { + name: "external tags do not cause apply", + current: infrav1.Tags{ + "Name": pName, + "k1": "v1", + infrav1.ClusterTagKey("testcluster"): string(infrav1.ResourceLifecycleOwned), + infrav1.NameAWSClusterAPIRole: pRole, + "external-tag": "external-value", + }, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + g := NewWithT(t) + + // The `diff` parameter should equal the test case's `expectedTags`. + // This is to show better error messages than with mocking. + var actualDiff infrav1.Tags + builder := New(params, func(b *Builder) { + b.applyFunc = func(_ *infrav1.BuildParams, diff infrav1.Tags) error { + actualDiff = diff + return nil + } + }) + err := builder.Ensure(tc.current) + g.Expect(err).To(BeNil()) + var expectedTagsAsInfraType infrav1.Tags + for k, v := range tc.expectedTags { + if expectedTagsAsInfraType == nil { + expectedTagsAsInfraType = infrav1.Tags{} + } + expectedTagsAsInfraType[k] = v + } + g.Expect(actualDiff).To(Equal(expectedTagsAsInfraType), "Diff calculation is wrong, or the test case has a wrong expectedTags") + + // Now check with mocking since the actual AWS API calls are most important. + // This ensures that the `diff` is used, and not all tags (incl. unchanged ones) + // get sent in the AWS request. + mockCtrl := gomock.NewController(t) + eksMock := mock_eksiface.NewMockEKSAPI(mockCtrl) + + if tc.expect != nil { + tc.expect(eksMock.EXPECT(), tc.expectedTags) + } + + builder = New(params, WithEKS(context.TODO(), eksMock)) + + err = builder.Ensure(tc.current) + g.Expect(err).To(BeNil()) + }) + } +} + func TestTagsBuildParamsToTagSpecification(t *testing.T) { g := NewWithT(t) tagSpec := BuildParamsToTagSpecification("test-resource", infrav1.BuildParams{