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(bucket): Merge BucketPolicy with Bucket #1685

Merged

Conversation

MisterMX
Copy link
Collaborator

@MisterMX MisterMX commented Mar 1, 2023

Description of your changes

This adds a new field spec.forProvider.policy to Bucket to merge the functionality of BucketPolicy into the former.

To avoid breaking changes with existing setups, policies are never deleted by the bucket controller by default. If spec.forProvider.policy is nil, the controller will do nothing.

To change this behaviour the user can set the new field spec.forProvider.policyUpdatePolicy.deletionPolicy to IfNull (default is Never). This way, the controller will delete the external AWS policy.

This also deprecates BucketPolicy with a warning to use the policy field in Bucket instead.

Fixes #1684

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

See the provided example.

@MisterMX MisterMX requested a review from chlunde March 1, 2023 15:38
@MisterMX MisterMX force-pushed the feat/s3-bucket-policy-merge branch 2 times, most recently from 8cce0d7 to d42fc9d Compare March 2, 2023 11:18
Deprecate BucketPolicy.

Signed-off-by: Maximilian Blatt <maximilian.blatt-extern@deutschebahn.com>
(external expert on behalf of DB Netz AG)
@MisterMX MisterMX force-pushed the feat/s3-bucket-policy-merge branch 2 times, most recently from d1a601b to 6b6e213 Compare March 7, 2023 15:20
Copy link
Member

@haarchri haarchri left a comment

Choose a reason for hiding this comment

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

LGTM

@haarchri haarchri merged commit 005c245 into crossplane-contrib:master Mar 8, 2023
@tcd156
Copy link

tcd156 commented Apr 18, 2023

The deprecation warning log is firing in the provider-aws pod constantly, even though I have no BucketPolicies specified.

Also, given that we can't infer the ARN of the bucket, is there a suggested method to easily interpolate that value into the policy?

@tcd156
Copy link

tcd156 commented Apr 18, 2023

Without BucketPolicy, this creates a situation that I'm in where I want to create an S3 bucket, and a CloudFront distribution.

The Distribution needs the Bucket, but the Bucket needs the Distribution ARN in order to set the policy so that only CloudFront can communicate to the Bucket.

So neither can be created without the other.

@MisterMX
Copy link
Collaborator Author

The deprecation warning log is firing in the provider-aws pod constantly, even though I have no BucketPolicies specified.

That sounds more like an issue with the underlying k8s.io/client-go because its spams every log directly to stderr.

Also, given that we can't infer the ARN of the bucket, is there a suggested method to easily interpolate that value into the policy?

When you are creating a bucket MR you are also know the external name of the bucket. You can derive the ARN of the bucket as long as you know the ID of the AWS account it is created in.

The Distribution needs the Bucket, but the Bucket needs the Distribution ARN in order to set the policy so that only CloudFront can communicate to the Bucket.

Wouldn't it work if you create the bucket without a policy and add it later, once cloudfront has been initialized?

@tcd156
Copy link

tcd156 commented Apr 24, 2023

Yes, and it does - however, the intent here is to deprecate bucket policy, no?

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.

Merge Bucket and BucketPolicy
3 participants