From 423f4d425b6628fd5d7f1acf0a6f97f5e99f5780 Mon Sep 17 00:00:00 2001 From: Simon Wakenhut Date: Tue, 22 Oct 2024 12:52:55 +0200 Subject: [PATCH] fix(ec2): Fix tags compare Signed-off-by: Simon Wakenhut (cherry picked from commit 138b8cf25debbe75a98f5af919cb8e05b2850fd3) --- pkg/clients/ec2/address.go | 29 -- pkg/clients/ec2/internetgateway.go | 2 +- pkg/clients/ec2/routetable.go | 2 +- pkg/clients/ec2/subnet.go | 2 +- pkg/clients/ec2/tags_test.go | 28 ++ pkg/clients/ec2/tags_v1beta1.go | 31 +-- pkg/clients/ec2/vpc.go | 2 +- pkg/controller/ec2/natgateway/controller.go | 2 +- .../ec2/securitygroup/controller_test.go | 249 ++++++++++++++++++ 9 files changed, 287 insertions(+), 60 deletions(-) diff --git a/pkg/clients/ec2/address.go b/pkg/clients/ec2/address.go index f97cb8b329..d42108c9c8 100644 --- a/pkg/clients/ec2/address.go +++ b/pkg/clients/ec2/address.go @@ -3,7 +3,6 @@ package ec2 import ( "context" "errors" - "sort" "github.com/aws/aws-sdk-go-v2/aws" "github.com/aws/aws-sdk-go-v2/service/ec2" @@ -101,31 +100,3 @@ func BuildFromEC2Tags(tags []ec2types.Tag) []v1beta1.Tag { return res } - -// CompareTags compares arrays of v1beta1.Tag and ec2types.Tag -func CompareTags(tags []v1beta1.Tag, ec2Tags []ec2types.Tag) bool { - if len(tags) != len(ec2Tags) { - return false - } - - SortTags(tags, ec2Tags) - - for i, t := range tags { - if t.Key != *ec2Tags[i].Key || t.Value != *ec2Tags[i].Value { - return false - } - } - - return true -} - -// SortTags sorts array of v1beta1.Tag and ec2types.Tag on 'Key' -func SortTags(tags []v1beta1.Tag, ec2Tags []ec2types.Tag) { - sort.Slice(tags, func(i, j int) bool { - return tags[i].Key < tags[j].Key - }) - - sort.Slice(ec2Tags, func(i, j int) bool { - return *ec2Tags[i].Key < *ec2Tags[j].Key - }) -} diff --git a/pkg/clients/ec2/internetgateway.go b/pkg/clients/ec2/internetgateway.go index c73ec5899e..8e4153bdce 100644 --- a/pkg/clients/ec2/internetgateway.go +++ b/pkg/clients/ec2/internetgateway.go @@ -96,7 +96,7 @@ func IsIgUpToDate(p v1beta1.InternetGatewayParameters, ig ec2types.InternetGatew // if the attachment in spec exists in ig.Attachments, compare the tags and return for _, a := range ig.Attachments { if aws.ToString(p.VPCID) == aws.ToString(a.VpcId) { - return CompareTagsV1Beta1(p.Tags, ig.Tags) + return CompareTags(p.Tags, ig.Tags) } } diff --git a/pkg/clients/ec2/routetable.go b/pkg/clients/ec2/routetable.go index 02dc2e8bba..ff5ddf437d 100644 --- a/pkg/clients/ec2/routetable.go +++ b/pkg/clients/ec2/routetable.go @@ -160,7 +160,7 @@ func CreateRTPatch(in ec2types.RouteTable, target v1beta1.RouteTableParameters) targetCopy := target.DeepCopy() currentParams := &v1beta1.RouteTableParameters{} - SortTagsV1Beta1(target.Tags, in.Tags) + CompareTags(target.Tags, in.Tags) if !pointer.BoolValue(target.IgnoreRoutes) { // Add the default route for fair comparison. diff --git a/pkg/clients/ec2/subnet.go b/pkg/clients/ec2/subnet.go index b7e2806388..bd2c2a34a5 100644 --- a/pkg/clients/ec2/subnet.go +++ b/pkg/clients/ec2/subnet.go @@ -85,5 +85,5 @@ func IsSubnetUpToDate(p v1beta1.SubnetParameters, s ec2types.Subnet) bool { if aws.ToBool(p.AssignIPv6AddressOnCreation) != aws.ToBool(s.AssignIpv6AddressOnCreation) { return false } - return CompareTagsV1Beta1(p.Tags, s.Tags) + return CompareTags(p.Tags, s.Tags) } diff --git a/pkg/clients/ec2/tags_test.go b/pkg/clients/ec2/tags_test.go index 98faacaf17..7af6a0e396 100644 --- a/pkg/clients/ec2/tags_test.go +++ b/pkg/clients/ec2/tags_test.go @@ -70,6 +70,34 @@ func TestDiffEC2Tags(t *testing.T) { remove: []ec2types.Tag{}, }, }, + "TagsWithSameKeyValuesButDifferentOrder": { + args: args{ + local: []ec2types.Tag{ + { + Key: aws.String("val"), + Value: aws.String("key"), + }, + { + Key: aws.String("name"), + Value: aws.String("somename"), + }, + }, + remote: []ec2types.Tag{ + { + Key: aws.String("name"), + Value: aws.String("somename"), + }, + { + Key: aws.String("val"), + Value: aws.String("key"), + }, + }, + }, + want: want{ + add: []ec2types.Tag{}, + remove: []ec2types.Tag{}, + }, + }, "TagsWithSameKeyDifferentValuesAndSameLength": { args: args{ local: []ec2types.Tag{ diff --git a/pkg/clients/ec2/tags_v1beta1.go b/pkg/clients/ec2/tags_v1beta1.go index 35260db007..46b34a9a25 100644 --- a/pkg/clients/ec2/tags_v1beta1.go +++ b/pkg/clients/ec2/tags_v1beta1.go @@ -17,8 +17,6 @@ limitations under the License. package ec2 import ( - "sort" - "github.com/aws/aws-sdk-go-v2/aws" ec2types "github.com/aws/aws-sdk-go-v2/service/ec2/types" @@ -47,30 +45,11 @@ func GenerateEC2TagsV1Beta1(tags []svcapitypes.Tag) []ec2types.Tag { return res } -// CompareTags compares arrays of v1beta1.Tag and ec2type.Tag -func CompareTagsV1Beta1(tags []svcapitypes.Tag, ec2Tags []ec2types.Tag) bool { - if len(tags) != len(ec2Tags) { +// CompareTags compares arrays of v1beta1.Tag and ec2types.Tag +func CompareTags(spec []svcapitypes.Tag, current []ec2types.Tag) bool { + if len(spec) != len(current) { return false } - - SortTagsV1Beta1(tags, ec2Tags) - - for i, t := range tags { - if t.Key != *ec2Tags[i].Key || t.Value != *ec2Tags[i].Value { - return false - } - } - - return true -} - -// SortTags sorts array of v1beta1.Tag and ec2type.Tag on 'Key' -func SortTagsV1Beta1(tags []svcapitypes.Tag, ec2Tags []ec2types.Tag) { - sort.Slice(tags, func(i, j int) bool { - return tags[i].Key < tags[j].Key - }) - - sort.Slice(ec2Tags, func(i, j int) bool { - return *ec2Tags[i].Key < *ec2Tags[j].Key - }) + toAdd, toRemove := DiffEC2Tags(GenerateEC2TagsV1Beta1(spec), current) + return len(toAdd) == 0 && len(toRemove) == 0 } diff --git a/pkg/clients/ec2/vpc.go b/pkg/clients/ec2/vpc.go index 51c9bd2a73..a89227cc27 100644 --- a/pkg/clients/ec2/vpc.go +++ b/pkg/clients/ec2/vpc.go @@ -53,7 +53,7 @@ func IsVpcUpToDate(spec v1beta1.VPCParameters, vpc ec2types.Vpc, attributes ec2. return false } - return CompareTagsV1Beta1(spec.Tags, vpc.Tags) + return CompareTags(spec.Tags, vpc.Tags) } // GenerateVpcObservation is used to produce v1beta1.VPCObservation from diff --git a/pkg/controller/ec2/natgateway/controller.go b/pkg/controller/ec2/natgateway/controller.go index 4a189d828f..e570cde6ff 100644 --- a/pkg/controller/ec2/natgateway/controller.go +++ b/pkg/controller/ec2/natgateway/controller.go @@ -142,7 +142,7 @@ func (e *external) Observe(ctx context.Context, mgd resource.Managed) (managed.E return managed.ExternalObservation{ ResourceExists: true, - ResourceUpToDate: ec2.CompareTagsV1Beta1(cr.Spec.ForProvider.Tags, observed.Tags), + ResourceUpToDate: ec2.CompareTags(cr.Spec.ForProvider.Tags, observed.Tags), }, nil } diff --git a/pkg/controller/ec2/securitygroup/controller_test.go b/pkg/controller/ec2/securitygroup/controller_test.go index ca2f71b721..6d635c1a51 100644 --- a/pkg/controller/ec2/securitygroup/controller_test.go +++ b/pkg/controller/ec2/securitygroup/controller_test.go @@ -31,6 +31,7 @@ import ( "github.com/google/go-cmp/cmp" "github.com/google/go-cmp/cmp/cmpopts" "github.com/pkg/errors" + "k8s.io/utils/ptr" "sigs.k8s.io/controller-runtime/pkg/client" "github.com/crossplane-contrib/provider-aws/apis/ec2/v1beta1" @@ -160,6 +161,254 @@ func TestObserve(t *testing.T) { }, }, }, + "SameTags": { + args: args{ + sg: &fake.MockSecurityGroupClient{ + MockDescribe: func(ctx context.Context, input *awsec2.DescribeSecurityGroupsInput, opts []func(*awsec2.Options)) (*awsec2.DescribeSecurityGroupsOutput, error) { + return &awsec2.DescribeSecurityGroupsOutput{ + SecurityGroups: []awsec2types.SecurityGroup{{ + Tags: []awsec2types.Tag{ + { + Key: ptr.To("key1"), + Value: ptr.To("value1"), + }, + { + Key: ptr.To("key2"), + Value: ptr.To("value2"), + }, + { + Key: ptr.To("key3"), + Value: ptr.To("value3"), + }, + }, + }}, + }, nil + }, + MockDescribeRules: func(ctx context.Context, input *awsec2.DescribeSecurityGroupRulesInput, opts []func(*awsec2.Options)) (*awsec2.DescribeSecurityGroupRulesOutput, error) { + return &awsec2.DescribeSecurityGroupRulesOutput{ + SecurityGroupRules: []awsec2types.SecurityGroupRule{}, + }, nil + }, + }, + cr: sg( + withExternalName(sgID), + withSpec(v1beta1.SecurityGroupParameters{ + Tags: []v1beta1.Tag{ + { + Key: "key1", + Value: "value1", + }, + { + Key: "key2", + Value: "value2", + }, + { + Key: "key3", + Value: "value3", + }, + }, + }), + withStatus(v1beta1.SecurityGroupObservation{ + SecurityGroupID: sgID, + }), + ), + }, + want: want{ + cr: sg( + withExternalName(sgID), + withSpec(v1beta1.SecurityGroupParameters{ + Tags: []v1beta1.Tag{ + { + Key: "key1", + Value: "value1", + }, + { + Key: "key2", + Value: "value2", + }, + { + Key: "key3", + Value: "value3", + }, + }, + }), + withStatus(v1beta1.SecurityGroupObservation{ + IngressRules: []v1beta1.SecurityGroupRuleObservation{}, + EgressRules: []v1beta1.SecurityGroupRuleObservation{}, + }), + withConditions(xpv1.Available()), + ), + result: managed.ExternalObservation{ + ResourceExists: true, + ResourceUpToDate: true, + }, + }, + }, + "SameTagsDifferentOrder": { + args: args{ + sg: &fake.MockSecurityGroupClient{ + MockDescribe: func(ctx context.Context, input *awsec2.DescribeSecurityGroupsInput, opts []func(*awsec2.Options)) (*awsec2.DescribeSecurityGroupsOutput, error) { + return &awsec2.DescribeSecurityGroupsOutput{ + SecurityGroups: []awsec2types.SecurityGroup{{ + Tags: []awsec2types.Tag{ + { + Key: ptr.To("key1"), + Value: ptr.To("value1"), + }, + { + Key: ptr.To("key3"), + Value: ptr.To("value3"), + }, + { + Key: ptr.To("key2"), + Value: ptr.To("value2"), + }, + }, + }}, + }, nil + }, + MockDescribeRules: func(ctx context.Context, input *awsec2.DescribeSecurityGroupRulesInput, opts []func(*awsec2.Options)) (*awsec2.DescribeSecurityGroupRulesOutput, error) { + return &awsec2.DescribeSecurityGroupRulesOutput{ + SecurityGroupRules: []awsec2types.SecurityGroupRule{}, + }, nil + }, + }, + cr: sg( + withExternalName(sgID), + withSpec(v1beta1.SecurityGroupParameters{ + Tags: []v1beta1.Tag{ + { + Key: "key3", + Value: "value3", + }, + { + Key: "key2", + Value: "value2", + }, + { + Key: "key1", + Value: "value1", + }, + }, + }), + withStatus(v1beta1.SecurityGroupObservation{ + SecurityGroupID: sgID, + }), + ), + }, + want: want{ + cr: sg( + withExternalName(sgID), + withSpec(v1beta1.SecurityGroupParameters{ + Tags: []v1beta1.Tag{ + { + Key: "key3", + Value: "value3", + }, + { + Key: "key2", + Value: "value2", + }, + { + Key: "key1", + Value: "value1", + }, + }, + }), + withStatus(v1beta1.SecurityGroupObservation{ + IngressRules: []v1beta1.SecurityGroupRuleObservation{}, + EgressRules: []v1beta1.SecurityGroupRuleObservation{}, + }), + withConditions(xpv1.Available()), + ), + result: managed.ExternalObservation{ + ResourceExists: true, + ResourceUpToDate: true, + }, + }, + }, + "DifferentTagValues": { + args: args{ + sg: &fake.MockSecurityGroupClient{ + MockDescribe: func(ctx context.Context, input *awsec2.DescribeSecurityGroupsInput, opts []func(*awsec2.Options)) (*awsec2.DescribeSecurityGroupsOutput, error) { + return &awsec2.DescribeSecurityGroupsOutput{ + SecurityGroups: []awsec2types.SecurityGroup{{ + Tags: []awsec2types.Tag{ + { + Key: ptr.To("key1"), + Value: ptr.To("othervalue"), + }, + { + Key: ptr.To("key2"), + Value: ptr.To("value2"), + }, + { + Key: ptr.To("key3"), + Value: ptr.To("value3"), + }, + }, + }}, + }, nil + }, + MockDescribeRules: func(ctx context.Context, input *awsec2.DescribeSecurityGroupRulesInput, opts []func(*awsec2.Options)) (*awsec2.DescribeSecurityGroupRulesOutput, error) { + return &awsec2.DescribeSecurityGroupRulesOutput{ + SecurityGroupRules: []awsec2types.SecurityGroupRule{}, + }, nil + }, + }, + cr: sg( + withExternalName(sgID), + withSpec(v1beta1.SecurityGroupParameters{ + Tags: []v1beta1.Tag{ + { + Key: "key1", + Value: "value1", + }, + { + Key: "key2", + Value: "value2", + }, + { + Key: "key3", + Value: "value3", + }, + }, + }), + withStatus(v1beta1.SecurityGroupObservation{ + SecurityGroupID: sgID, + }), + ), + }, + want: want{ + cr: sg( + withExternalName(sgID), + withSpec(v1beta1.SecurityGroupParameters{ + Tags: []v1beta1.Tag{ + { + Key: "key1", + Value: "value1", + }, + { + Key: "key2", + Value: "value2", + }, + { + Key: "key3", + Value: "value3", + }, + }, + }), + withStatus(v1beta1.SecurityGroupObservation{ + IngressRules: []v1beta1.SecurityGroupRuleObservation{}, + EgressRules: []v1beta1.SecurityGroupRuleObservation{}, + }), + ), + result: managed.ExternalObservation{ + ResourceExists: true, + ResourceUpToDate: false, + }, + }, + }, "EmptyExternalNameExistingSG": { args: args{ kube: &test.MockClient{