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.

Sign up for GitHub

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

Jump to bottom

s3 bucket policy to v1alpha2 #391

Merged
merged 5 commits into from
Nov 6, 2020
Merged

s3 bucket policy to v1alpha2 #391

merged 5 commits into from
Nov 6, 2020

Conversation

krishchow
Copy link
Collaborator

@krishchow krishchow commented Oct 20, 2020 •
edited
Loading

Description of your changes

Fixes #374

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

Additional unit tests, and manual testing
[contribution process]: https://git.io/fj2m9

@krishchow
Copy link
Collaborator Author

krishchow commented Oct 20, 2020 •
edited
Loading

The main reason I opened this draft PR early on in the process is to get feedback from the community. There are a few points of discussion I want to try and get a dialog on:

  • I haven't changed the API group yet, because I am not sure if this qualifies as ready for v1beta1, or if it should be v1alpha2.
  • Currently, I am templating the Resource field of the bucket policy with the ARN prefix, and then the rest of the bucket path. This isn't required, but it is more of a QOL feature, since it is generally less to write for users, and I have not been able to find a case where the resource is not an S3 bucket. However, I can see a clear reason for removing it, since it may be unclear to new users of the provider-aws since the input syntax is different from a regular AWS s3 bucket policy.

@krishchow krishchow force-pushed the BucketV1beta1 branch 6 times, most recently from 968d6d5 to d160929 Compare October 26, 2020 18:27
@krishchow
Copy link
Collaborator Author

  • Ended up moving the bucket to v1alpha2 to stay on the safe side.
  • I am no longer templating the resource ARN, I don't think it was necessary, and it could make it harder to understand the bucket policy initially.

@krishchow krishchow marked this pull request as ready for review October 26, 2020 18:29
@prasek prasek mentioned this pull request Oct 28, 2020
Closed
26 tasks
hasheddan
hasheddan reviewed Nov 4, 2020
View reviewed changes
Copy link
Member

@hasheddan hasheddan left a comment

Choose a reason for hiding this comment

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

@krishchow apologies for the delay in review here! This looks pretty good to me :) It looks like the main change is that we reference the IAM user directly instead of extracting it from the Bucket, are there any other major differences here?

@muvaf could you take a peek at this as well?

@krishchow
Copy link
Collaborator Author

@hasheddan that's one of the main changes yes :). Instead of referencing a single IAMUser for the entire policy, each statement can refer to an IAMUser.

I have also added support for a few other identity types supported by the bucket policy, also the other big thing support for Condition in the policy. This was one key feature that wasn't previously supported.

muvaf
muvaf reviewed Nov 5, 2020
View reviewed changes
Copy link
Member

@muvaf muvaf left a comment

Choose a reason for hiding this comment

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

Sorry for the delayed review. There are a few things to address and I think it's the right call to make it v1alpha2 for now. Thanks @krishchow !

apis/s3/v1alpha2/bucketpolicy_types.go Outdated Show resolved Hide resolved
apis/s3/v1alpha2/bucketpolicy_types.go
@@ -48,147 +47,133 @@ type BucketPolicyParameters struct {
// BucketNameSelector selects a reference to an S3Bucket to retrieve its bucketName
Copy link
Member

Choose a reason for hiding this comment

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

I think some of the parameters are immutable like BucketName

apis/s3/v1alpha2/bucketpolicy_types.go Outdated Show resolved Hide resolved
apis/s3/v1alpha2/bucketpolicy_types.go Outdated Show resolved Hide resolved
apis/s3/v1alpha2/bucketpolicy_types.go Show resolved Hide resolved
apis/s3/v1alpha2/referencers.go Outdated Show resolved Hide resolved
examples/s3/bucket.yaml Outdated Show resolved Hide resolved
examples/s3/bucket.yaml Outdated Show resolved Hide resolved
apis/s3/v1alpha2/bucketpolicy_types.go Show resolved Hide resolved
apis/s3/v1alpha2/bucketpolicy_types.go Outdated Show resolved Hide resolved
@negz negz added this to PR Open in v0.14 via automation Nov 5, 2020
@krishchow krishchow force-pushed the BucketV1beta1 branch 2 times, most recently from e47061f to d8e1194 Compare November 5, 2020 20:39
muvaf
muvaf approved these changes Nov 6, 2020
View reviewed changes
Copy link
Member

@muvaf muvaf left a comment

Choose a reason for hiding this comment

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

thanks @krishchow !

@muvaf
Copy link
Member

muvaf commented Nov 6, 2020

@krishchow could you resolve the conflict so that we can merge it?

@muvaf muvaf changed the title s3 bucket policy to v1beta1 s3 bucket policy to v1alpha2 Nov 6, 2020
@krishchow
bucket policy: bucket policy changes for condition, updates IAM user …
5967ebb 
…to fit into bucket principal

Signed-off-by: Krish Chowdhary <krish@redhat.com>
@krishchow
bucket policy: added more unit tests, updated examples
12c2461 
Signed-off-by: Krish Chowdhary <krish@redhat.com>
@krishchow
bucket policy: moved bucketpolicy to v1alpha2
74bf6d0 
Signed-off-by: Krish Chowdhary <krish@redhat.com>
@krishchow
bucket policy: renamed bucket fields, matching json tags
412aba1 
Signed-off-by: Krish Chowdhary <krish@redhat.com>
@krishchow
s3 bucket: regenerated value
8b0a70c 
Signed-off-by: Krish Chowdhary <krish@redhat.com>
@krishchow
Copy link
Collaborator Author

@muvaf Rebased and fixed all the conflicts

@muvaf muvaf merged commit f4c4e3c into crossplane-contrib:master Nov 6, 2020
v0.14 automation moved this from PR Open to Done Nov 6, 2020
wolffbe pushed a commit to wolffbe/provider-aws that referenced this pull request Feb 12, 2021
@muvaf
Merge pull request crossplane-contrib#391 from krishchow/BucketV1beta1
bafc56b 
s3 bucket policy to v1alpha2
namku pushed a commit to namku/provider-aws that referenced this pull request Mar 9, 2021
@muvaf
Merge pull request crossplane-contrib#391 from krishchow/BucketV1beta1
d1e6d34 
s3 bucket policy to v1alpha2
@negz negz removed this from Done in v0.14 May 26, 2022
tektondeploy pushed a commit to gtn3010/provider-aws that referenced this pull request Mar 12, 2024
@ezgidemirel
Merge pull request crossplane-contrib#391 from ezgidemirel/fix-sec-group
92806ea 
Remove ingress/egress references from securitygroup resource
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hasheddan hasheddan hasheddan left review comments

@muvaf muvaf muvaf approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

S3 Bucket Policy to v1beta
3 participants
@krishchow @muvaf @hasheddan