Feature/s3 bucket encryption - Implements PR #4235 #5194
Conversation
k8s-ci-robot
added
cncf-cla: yes
|
/assign @justinsb |
|
I was actually thinking about this the other day, great job @chrisz100! This looks good to me! /ok-to-test |
k8s-ci-robot
added
lgtm
|
@justinsb any idea when this is likely to get integrated or if at all? Our client is having this on the timeline and just moving the issue without a real idea when and if at all doesn't satisfy stakeholders I'm afraid. |
|
@chrisz100 Looks like @justinsb has slatted this for the 1.10 milestone so we can hopefully get this in for you soon. |
| @@ -134,7 +142,7 @@ func (s *S3Context) getRegionForBucket(bucket string) (string, error) { | |||
| } | |||
|
|
|||
| if err := validateRegion(awsRegion); err != nil { | |||
| return "", err | |||
| return bucketDetails, err | |||
There was a problem hiding this comment.
Usually better to return nil, err here
| } | ||
| return region, nil | ||
| return bucketDetails, nil |
There was a problem hiding this comment.
I don't think we set defaultEncryption if S3_REGION is set? Also we aren't caching it here...
| // the following cases might lead to the operation failing: | ||
| // 1. A deny policy on s3:GetEncryptionConfiguration | ||
| // 2. No default encryption policy set | ||
| glog.Warningf("Unable to read bucket encryption policy: will encrypt using AES256") |
There was a problem hiding this comment.
As a style thing, IMO it's often clearer to have the function return the value & error, and then have the caller decide whether to ignore the error.
| if p.bucketDetails == nil || p.bucketDetails.region == "" { | ||
| bucketDetails, err := p.s3Context.getDetailsForBucket(p.bucket) | ||
|
|
||
| p.bucketDetails = bucketDetails |
There was a problem hiding this comment.
Ah I see what you're doing now returning the bucketDetails always. It's a slightly hard pattern to reason about though - because a retry will succeed I think.
|
Some nits / style ideas for the next PR :-) Thanks @gekart and @chrisz100 /approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: chrisz100, justinsb, mikesplain The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
As @gekart postponed working on the bucket encryption I took this forward and made his implementaion mergable again.
Original PR #4235