Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(s3): bucketKeyEnabled implemented #1158

Merged

Conversation

haarchri
Copy link
Member

Signed-off-by: haarchri chhaar30@googlemail.com

Description of your changes

implemented bucketKeyEnabled in sseConfig

Fixes #1154

I have:

  • Read and followed Crossplane's contribution process.
  • Run make reviewable test to ensure this PR is ready for review.

How has this code been tested

t.b.d

@haarchri haarchri marked this pull request as ready for review February 20, 2022 18:35
@chlunde
Copy link
Collaborator

chlunde commented Feb 23, 2022

diff --git a/pkg/controller/s3/bucket/sseConfig.go b/pkg/controller/s3/bucket/sseConfig.go
index 93772cca..616e4105 100644
--- a/pkg/controller/s3/bucket/sseConfig.go
+++ b/pkg/controller/s3/bucket/sseConfig.go
@@ -75,6 +75,9 @@ func (in *SSEConfigurationClient) Observe(ctx context.Context, bucket *v1beta1.B
                if string(outputRule.SSEAlgorithm) != Rule.ApplyServerSideEncryptionByDefault.SSEAlgorithm {
                        return NeedsUpdate, nil
                }
+               if external.ServerSideEncryptionConfiguration.Rules[i].BucketKeyEnabled != Rule.BucketKeyEnabled {
+                       return NeedsUpdate, nil
+               }
        }
 
        return Updated, nil

perhaps also a test for this:

+               "NeedsUpdateEnableBucketKey": {
+                       args: args{
+                               b: s3testing.Bucket(s3testing.WithSSEConfig(generateSSEConfigWithBucketEncryption())),
+                               cl: NewSSEConfigurationClient(fake.MockBucketClient{
+                                       MockGetBucketEncryption: func(ctx context.Context, input *s3.GetBucketEncryptionInput, opts []func(*s3.Options)) (*s3.GetBucketEncryptionOutput, error) {
+                                               return &s3.GetBucketEncryptionOutput{ServerSideEncryptionConfiguration: generateAWSSSE()}, nil
+                                       },
+                               }),
+                       },
+                       want: want{
+                               status: NeedsUpdate,
+                               err:    nil,
+                       },
+               },

@haarchri
Copy link
Member Author

haarchri commented Mar 7, 2022

e2e-tests are failing since we merged crossplane/crossplane#2932

using crossplane version 1.7.0-rc.0.125.gfff10011

kubectl get pkgrev
NAME                                                           HEALTHY   REVISION   IMAGE          STATE    DEP-FOUND   DEP-INSTALLED   AGE
providerrevision.pkg.crossplane.io/provider-aws-provider-aws             1          provider-aws   Active                               2m40s

Events:
  Type     Reason             Age                    From                                         Message
  ----     ------             ----                   ----                                         -------
  Normal   BindClusterRole    2m10s (x3 over 2m10s)  rbac/providerrevision.pkg.crossplane.io      Bound system ClusterRole to provider ServiceAccount(s)
  Normal   ApplyClusterRoles  2m10s (x2 over 2m10s)  rbac/providerrevision.pkg.crossplane.io      Applied RBAC ClusterRoles
  Warning  ParsePackage       9s (x8 over 2m10s)     packages/providerrevision.pkg.crossplane.io  failed to get pre-cached package with pull policy Never

tested locally with crossplane 1.6.3 without issues

Signed-off-by: haarchri <chhaar30@googlemail.com>
Signed-off-by: haarchri <chhaar30@googlemail.com>
@haarchri haarchri force-pushed the feature/s3-bucket-key-enabled branch from c36e6ab to 97b8168 Compare March 7, 2022 16:09
@haarchri
Copy link
Member Author

haarchri commented Mar 7, 2022

@chlunde rebased after sdk bump

@haarchri haarchri requested a review from chlunde March 7, 2022 16:29
Copy link
Collaborator

@chlunde chlunde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tested toggling on/off, reviewed, thanks!

@chlunde chlunde merged commit ecf5c1d into crossplane-contrib:master Mar 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

S3 Bucket - bucketKeyEnabled
2 participants