Enable canned ACL for S3 #9042
Enable canned ACL for S3 #9042
Conversation
| if h.Config.ACL != "" { | ||
| input.ACL = aws.String(h.Config.ACL) | ||
| } |
There was a problem hiding this comment.
Can you check if this is needed? I think you're setting it on the AWS session, so it shouldn't be needed right?
There was a problem hiding this comment.
Looks like it might be needed, but please verify.
https://docs.aws.amazon.com/AmazonS3/latest/API/API_CreateMultipartUpload.html
There was a problem hiding this comment.
This is needed. The session doesn't handle the ACL.
constants.go
Outdated
| @@ -582,11 +585,25 @@ const ( | |||
| // MinClientVersion is the minimum client version required by the server. | |||
| var MinClientVersion string | |||
|
|
|||
| // S3AllowedACL is the set of canned ACLs that S3 accepts | |||
| var S3AllowedACL map[string]struct{} | |||
There was a problem hiding this comment.
It looks like you only need this in one package, so I would define it in the package where it is used and unexport it.
Less things can go wrong this way 🙂
| } { | ||
| t.Run(tc.desc, func(t *testing.T) { | ||
| url, err := url.Parse(fmt.Sprintf("%s?acl=%s", baseUrl, tc.acl)) | ||
| require.Nil(t, err) |
|
@atburke I think one of the requirements in the ticket was to transfer ownership to a different account right? How will that be handled? |
|
Ownership is transferred when the ACL is set to |
|
@atburke So you have to be able to write to a bucket in another account and ownership goes to account owner? Do you have an example of how to setup permissions like that? I think it would be useful for our documentation. |
|
Correct. The AWS docs have an example for doing exactly this. |
This change allows admins to specify a canned ACL when using S3.
This change allows admins to specify a canned ACL when using S3.
This change allows admins to specify a canned ACL when using S3.
This change allows admins to specify a canned ACL when using S3.
This PR allows admins to specify a canned ACL when using S3 to store session recordings. The
aclflag has been added as a query param for the audit session uri.Resolves #7869.