Add s3 canned ACL support #189
Conversation
|
Hi @ivanmp91, we had a small git problem which made us rewrite history in Thanks and sorry for the inconvenience! |
Sure! This is done @adejanovski |
|
Hi @ivanmp91, there are a lot of commits in this PR that would need squashing and cleaning up. |
| @@ -73,7 +74,8 @@ def cp_upload(self, *, srcs, bucket_name, dest, max_retries=5): | |||
| awscli_output = "/tmp/awscli_{0}.output".format(job_id) | |||
| objects = [] | |||
| for src in srcs: | |||
| cmd = [self._aws_cli_path, "s3", "cp", str(src), "s3://{}/{}".format(bucket_name, dest)] | |||
| cmd = [self._aws_cli_path, "s3", "cp", "--acl", self.canned_acl, | |||
There was a problem hiding this comment.
Would this be backwards compatible with the current behavior if set to private by default?
I seem to understand that it would, but want to make sure of it.
There was a problem hiding this comment.
Also, we're moving to a consolidated set of classes for all S3 compatible backends in #272. I'm wondering how this would affect other storage providers than AWS.
I'd be in favor of making this flag added to the aws command line only if canned_acl is set to anything else but private (or maybe we could leave it unset by default).
@burmanm, wdyt?
There was a problem hiding this comment.
Yeah, I only added parameters to the command line if they differ from the default.
There was a problem hiding this comment.
@adejanovski according to aws s3 documentation, private is the default ACL (https://docs.aws.amazon.com/AmazonS3/latest/dev/acl-overview.html#canned-acl) which keeping it as a default option if is not set shouldn't change the behavior.
ivanmp91
force-pushed
the
add-s3-canned-acl
branch
5 times, most recently
from
6a32f5d
to
a94d329
Compare
ivanmp91
force-pushed
the
add-s3-canned-acl
branch
from
a94d329
to
f6aaedd
Compare
There was a problem hiding this comment.
Checking this PR again after a while 😅
Thanks for your incredible patience @ivanmp91
I'll go ahead and merge.
|
ouch there are some conflicts we need to handle before it can be merged. |
|
Closing in favour of #779 |
Changing default canned ACL[1] would be useful for situations where the S3 bucket is located in a different account. For S3, by default the object owner is the source AWS account where the object comes from [2]. This is a big limitation for a disaster recovery scenario where the restore could be done in a different account and requires to previously change the permissions to all objects in the bucket.
The default ACL value will remain as private.
[1] https://docs.aws.amazon.com/AmazonS3/latest/dev/acl-overview.html#canned-acl
[2] https://aws.amazon.com/premiumsupport/knowledge-center/s3-bucket-owner-access/
┆Issue is synchronized with this Jira Task by Unito
┆friendlyId: K8SSAND-1406
┆priority: Medium