From 8a281b95344dbd752f84423c5634e5101ba8bd34 Mon Sep 17 00:00:00 2001 From: Kirill Sushkov Date: Thu, 21 Mar 2024 15:59:33 +0100 Subject: [PATCH] fix(s3 bucket): Ignore AWS system tags Signed-off-by: Kirill Sushkov (teeverr) --- pkg/controller/s3/bucket/taggingConfig.go | 59 +++++++++++++++++-- .../s3/bucket/taggingConfig_test.go | 41 +++++++++++++ 2 files changed, 94 insertions(+), 6 deletions(-) diff --git a/pkg/controller/s3/bucket/taggingConfig.go b/pkg/controller/s3/bucket/taggingConfig.go index 85b8dd4992..87a3a8888f 100644 --- a/pkg/controller/s3/bucket/taggingConfig.go +++ b/pkg/controller/s3/bucket/taggingConfig.go @@ -18,6 +18,7 @@ package bucket import ( "context" + "strings" awss3 "github.com/aws/aws-sdk-go-v2/service/s3" "github.com/aws/aws-sdk-go-v2/service/s3/types" @@ -26,6 +27,7 @@ import ( "github.com/crossplane/crossplane-runtime/pkg/resource" "github.com/google/go-cmp/cmp" "github.com/google/go-cmp/cmp/cmpopts" + "k8s.io/utils/ptr" "github.com/crossplane-contrib/provider-aws/apis/s3/v1beta1" "github.com/crossplane-contrib/provider-aws/pkg/clients/s3" @@ -39,9 +41,14 @@ const ( taggingDeleteFailed = "cannot delete Bucket tagging set" ) +type cache struct { + getBucketTaggingOutput *awss3.GetBucketTaggingOutput +} + // TaggingConfigurationClient is the client for API methods and reconciling the CORSConfiguration type TaggingConfigurationClient struct { client s3.BucketClient + cache cache } // NewTaggingConfigurationClient creates the client for CORS Configuration @@ -49,17 +56,31 @@ func NewTaggingConfigurationClient(client s3.BucketClient) *TaggingConfiguration return &TaggingConfigurationClient{client: client} } +// CacheBucketTaggingOutput returns cached *awss3.GetBucketTaggingOutput` if it exists, otherwise adds +// `TaggingConfigurationClient.GetBucketTagging` output to cache and then returns it +func (in *TaggingConfigurationClient) CacheBucketTaggingOutput(ctx context.Context, bucketName *string) (*awss3.GetBucketTaggingOutput, error) { + if in.cache.getBucketTaggingOutput == nil { + external, err := in.client.GetBucketTagging(ctx, &awss3.GetBucketTaggingInput{Bucket: bucketName}) + if err != nil { + return external, err + } + in.cache.getBucketTaggingOutput = external + return external, nil + } + return in.cache.getBucketTaggingOutput, nil +} + // Observe checks if the resource exists and if it matches the local configuration func (in *TaggingConfigurationClient) Observe(ctx context.Context, bucket *v1beta1.Bucket) (ResourceStatus, error) { - external, err := in.client.GetBucketTagging(ctx, &awss3.GetBucketTaggingInput{Bucket: pointer.ToOrNilIfZeroValue(meta.GetExternalName(bucket))}) - config := bucket.Spec.ForProvider.BucketTagging + config := bucket.Spec.ForProvider.BucketTagging.DeepCopy() + external, err := in.CacheBucketTaggingOutput(ctx, pointer.ToOrNilIfZeroValue(meta.GetExternalName(bucket))) if err != nil { if s3.TaggingNotFound(err) && config == nil { return Updated, nil } return NeedsUpdate, errorutils.Wrap(resource.Ignore(s3.TaggingNotFound, err), taggingGetFailed) } - + config = addExistingSystemTags(config, external) switch { case config == nil && len(external.TagSet) == 0: return Updated, nil @@ -77,8 +98,12 @@ func (in *TaggingConfigurationClient) CreateOrUpdate(ctx context.Context, bucket if bucket.Spec.ForProvider.BucketTagging == nil { return nil } - input := GeneratePutBucketTagging(meta.GetExternalName(bucket), bucket.Spec.ForProvider.BucketTagging) - _, err := in.client.PutBucketTagging(ctx, input) + external, err := in.CacheBucketTaggingOutput(ctx, pointer.ToOrNilIfZeroValue(meta.GetExternalName(bucket))) + if err != nil { + return err + } + input := GeneratePutBucketTagging(meta.GetExternalName(bucket), addExistingSystemTags(bucket.Spec.ForProvider.BucketTagging, external)) + _, err = in.client.PutBucketTagging(ctx, input) return errorutils.Wrap(err, taggingPutFailed) } @@ -95,7 +120,7 @@ func (in *TaggingConfigurationClient) Delete(ctx context.Context, bucket *v1beta // LateInitialize does nothing because the resource might have been deleted by // the user. func (in *TaggingConfigurationClient) LateInitialize(ctx context.Context, bucket *v1beta1.Bucket) error { - external, err := in.client.GetBucketTagging(ctx, &awss3.GetBucketTaggingInput{Bucket: pointer.ToOrNilIfZeroValue(meta.GetExternalName(bucket))}) + external, err := in.CacheBucketTaggingOutput(ctx, pointer.ToOrNilIfZeroValue(meta.GetExternalName(bucket))) if err != nil { return errorutils.Wrap(resource.Ignore(s3.TaggingNotFound, err), taggingGetFailed) } @@ -145,3 +170,25 @@ func GeneratePutBucketTagging(name string, config *v1beta1.Tagging) *awss3.PutBu Tagging: GenerateTagging(config), } } + +// addExistingSystemTags returns `*v1beta1.Tagging` which contains tags from desired state and system tags from observed resource if these tags exist +// AWS API provides only put/delete/get operations for tags, so there is only one way to change - override the whole tag set, +// It is impossible in case if observed bucket already has systemd tags(they are not settable), in this case combining tags from desired tagSet +// with system tags from observed bucket is equal to ignoring of them +func addExistingSystemTags(desiredTags *v1beta1.Tagging, observedTags *awss3.GetBucketTaggingOutput) *v1beta1.Tagging { + var systemTags []v1beta1.Tag + tagSet := desiredTags.DeepCopy() + for _, t := range observedTags.TagSet { + key := pointer.StringValue(t.Key) + if strings.HasPrefix(key, "aws:") { + systemTags = append(systemTags, v1beta1.Tag{Key: ptr.Deref(t.Key, ""), Value: ptr.Deref(t.Value, "")}) + } + } + if systemTags != nil { + if tagSet == nil { + tagSet = &v1beta1.Tagging{TagSet: make([]v1beta1.Tag, 0)} + } + tagSet.TagSet = append(tagSet.TagSet, systemTags...) + } + return tagSet +} diff --git a/pkg/controller/s3/bucket/taggingConfig_test.go b/pkg/controller/s3/bucket/taggingConfig_test.go index d6c7cce03f..d124ab5c11 100644 --- a/pkg/controller/s3/bucket/taggingConfig_test.go +++ b/pkg/controller/s3/bucket/taggingConfig_test.go @@ -60,6 +60,10 @@ var ( Key: aws.String("abc"), Value: aws.String("abc"), } + awsSystemTag1 = types.Tag{ + Key: aws.String("aws:tag1"), + Value: aws.String("1"), + } awsTags = []types.Tag{awsTag, awsTag1, awsTag2} _ SubresourceClient = &TaggingConfigurationClient{} ) @@ -133,6 +137,34 @@ func TestTaggingObserve(t *testing.T) { err: nil, }, }, + "DeleteNoNeededIgnoreSystemTags": { + args: args{ + b: s3testing.Bucket(s3testing.WithTaggingConfig(nil)), + cl: NewTaggingConfigurationClient(fake.MockBucketClient{ + MockGetBucketTagging: func(ctx context.Context, input *s3.GetBucketTaggingInput, opts []func(*s3.Options)) (*s3.GetBucketTaggingOutput, error) { + return &s3.GetBucketTaggingOutput{TagSet: []types.Tag{awsSystemTag1}}, nil + }, + }), + }, + want: want{ + status: Updated, + err: nil, + }, + }, + "NoUpdateIgnoreSystemTags": { + args: args{ + b: s3testing.Bucket(s3testing.WithTaggingConfig(generateTaggingConfig())), + cl: NewTaggingConfigurationClient(fake.MockBucketClient{ + MockGetBucketTagging: func(ctx context.Context, input *s3.GetBucketTaggingInput, opts []func(*s3.Options)) (*s3.GetBucketTaggingOutput, error) { + return &s3.GetBucketTaggingOutput{TagSet: []types.Tag{awsTag2, awsTag, awsTag1, awsSystemTag1}}, nil + }, + }), + }, + want: want{ + status: Updated, + err: nil, + }, + }, "NoUpdateNotExists": { args: args{ b: s3testing.Bucket(s3testing.WithTaggingConfig(nil)), @@ -225,6 +257,9 @@ func TestTaggingCreateOrUpdate(t *testing.T) { MockPutBucketTagging: func(ctx context.Context, input *s3.PutBucketTaggingInput, opts []func(*s3.Options)) (*s3.PutBucketTaggingOutput, error) { return nil, errBoom }, + MockGetBucketTagging: func(ctx context.Context, input *s3.GetBucketTaggingInput, opts []func(*s3.Options)) (*s3.GetBucketTaggingOutput, error) { + return &s3.GetBucketTaggingOutput{TagSet: nil}, nil + }, }), }, want: want{ @@ -238,6 +273,9 @@ func TestTaggingCreateOrUpdate(t *testing.T) { MockPutBucketTagging: func(ctx context.Context, input *s3.PutBucketTaggingInput, opts []func(*s3.Options)) (*s3.PutBucketTaggingOutput, error) { return &s3.PutBucketTaggingOutput{}, nil }, + MockGetBucketTagging: func(ctx context.Context, input *s3.GetBucketTaggingInput, opts []func(*s3.Options)) (*s3.GetBucketTaggingOutput, error) { + return &s3.GetBucketTaggingOutput{TagSet: nil}, nil + }, }), }, want: want{ @@ -251,6 +289,9 @@ func TestTaggingCreateOrUpdate(t *testing.T) { MockPutBucketTagging: func(ctx context.Context, input *s3.PutBucketTaggingInput, opts []func(*s3.Options)) (*s3.PutBucketTaggingOutput, error) { return &s3.PutBucketTaggingOutput{}, nil }, + MockGetBucketTagging: func(ctx context.Context, input *s3.GetBucketTaggingInput, opts []func(*s3.Options)) (*s3.GetBucketTaggingOutput, error) { + return &s3.GetBucketTaggingOutput{TagSet: nil}, nil + }, }), }, want: want{