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

S3_bucket : Handle setting of permissions while acl is disabled #1168

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions changelogs/fragments/1168-s3_bucket_acl_disabled.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
bugfixes:
- s3_bucket - Handle setting of permissions while acl is disabled.(https://github.com/ansible-collections/amazon.aws/pull/1168).
23 changes: 17 additions & 6 deletions plugins/modules/s3_object.py
Original file line number Diff line number Diff line change
Expand Up @@ -396,6 +396,7 @@
import base64
import time


try:
import botocore
except ImportError:
Expand Down Expand Up @@ -607,6 +608,8 @@ def create_dirkey(module, s3, bucket, obj, encrypt, expiry):
s3.put_object_acl(ACL=acl, Bucket=bucket, Key=obj)
except is_boto3_error_code(IGNORE_S3_DROP_IN_EXCEPTIONS):
module.warn("PutObjectAcl is not implemented by your storage provider. Set the permissions parameters to the empty list to avoid this warning")
except is_boto3_error_code('AccessControlListNotSupported'):
module.warn("PutObjectAcl operation : The bucket does not allow ACLs.")
except (botocore.exceptions.BotoCoreError, botocore.exceptions.ClientError) as e: # pylint: disable=duplicate-except
module.fail_json_aws(e, msg="Failed while creating object %s." % obj)

Expand Down Expand Up @@ -696,6 +699,8 @@ def upload_s3file(module, s3, bucket, obj, expiry, metadata, encrypt, headers, s
s3.put_object_acl(ACL=acl, Bucket=bucket, Key=obj)
except is_boto3_error_code(IGNORE_S3_DROP_IN_EXCEPTIONS):
module.warn("PutObjectAcl is not implemented by your storage provider. Set the permission parameters to the empty list to avoid this warning")
except is_boto3_error_code('AccessControlListNotSupported'):
module.warn("PutObjectAcl operation : The bucket does not allow ACLs.")
except (botocore.exceptions.BotoCoreError, botocore.exceptions.ClientError) as e: # pylint: disable=duplicate-except
module.fail_json_aws(e, msg="Unable to set object ACL")

Expand Down Expand Up @@ -831,6 +836,8 @@ def copy_object_to_bucket(module, s3, bucket, obj, encrypt, metadata, validate,
module.exit_json(msg="Object copied from bucket %s to bucket %s." % (bucketsrc['Bucket'], bucket), tags=tags, changed=True)
except is_boto3_error_code(IGNORE_S3_DROP_IN_EXCEPTIONS):
module.warn("PutObjectAcl is not implemented by your storage provider. Set the permissions parameters to the empty list to avoid this warning")
except is_boto3_error_code('AccessControlListNotSupported'):
module.warn("PutObjectAcl operation : The bucket does not allow ACLs.")
except (botocore.exceptions.BotoCoreError, botocore.exceptions.ClientError) as e: # pylint: disable=duplicate-except
module.fail_json_aws(e, msg="Failed while copying object %s from bucket %s." % (obj, module.params['copy_src'].get('Bucket')))

Expand Down Expand Up @@ -1081,6 +1088,7 @@ def main():
# check if bucket exists, if yes, check if ACL is disabled
acl_disabled = False
exists = bucket_check(module, s3, bucket)

if exists:
try:
ownership_controls = s3.get_bucket_ownership_controls(Bucket=bucket)['OwnershipControls']
Expand Down Expand Up @@ -1217,15 +1225,17 @@ def main():
if key_check(module, s3, bucket, dirobj):
module.exit_json(msg="Bucket %s and key %s already exists." % (bucket, obj), changed=False)
else:
# setting valid object acls for the create_dirkey function
module.params['permission'] = object_acl
if not acl_disabled:
# setting valid object acls for the create_dirkey function
module.params['permission'] = object_acl
create_dirkey(module, s3, bucket, dirobj, encrypt, expiry)
else:
# only use valid bucket acls for the create_bucket function
module.params['permission'] = bucket_acl
create_bucket(module, s3, bucket, location)
# only use valid object acls for the create_dirkey function
module.params['permission'] = object_acl
if not acl_disabled:
# only use valid object acls for the create_dirkey function
module.params['permission'] = object_acl
Copy link
Member

Choose a reason for hiding this comment

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

I don't think it's a good idea to modify module.params like this. The variable is supposed to be read-only. It becomes really hard to track the origin of the change.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@goneri I have not changed the code flow much from what existed. I am not modifying the module.params['permission'], but setting the value based on a condition.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I saw that the existing code is already doing that.

create_dirkey(module, s3, bucket, dirobj, encrypt, expiry)

# Support for grabbing the time-expired URL for an object in S3/Walrus.
Expand Down Expand Up @@ -1265,8 +1275,9 @@ def main():
# only use valid bucket acls for create_bucket function
module.params['permission'] = bucket_acl
create_bucket(module, s3, bucket, location)
# only use valid object acls for the copy operation
module.params['permission'] = object_acl
if not acl_disabled:
# only use valid object acls for the copy operation
module.params['permission'] = object_acl
copy_object_to_bucket(module, s3, bucket, obj, encrypt, metadata, validate, d_etag)

module.exit_json(failed=False)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -85,17 +85,39 @@
- upload_file_result.msg != "PUT operation complete"
- '"s3:PutObject" not in upload_file_result.resource_actions'

- name: Create an object in the bucket with permissions (permission not set)
amazon.aws.s3_object:
bucket: "{{ bucket_name }}-acl-disabled"
object: "/test_directory"
permission: bucket-owner-full-control
mode: create
register: permission_result

- assert:
that:
- permission_result is changed
- upload_file_result is not failed
- '"PutObjectAcl operation : The bucket does not allow ACLs." in permission_result.warnings'
- '"Virtual directory test_directory/ created" in permission_result.msg'

always:

- name: Delete the file in the bucket
amazon.aws.s3_object:
bucket: "{{ bucket_name }}-acl-disabled"
src: "{{ remote_tmp_dir }}/acl_disabled_upload_test.txt"
object: "acl_disabled_upload_test.txt"
object: "{{ item }}"
mode: delobj
retries: 3
delay: 3
ignore_errors: true
loop:
- "acl_disabled_upload_test.txt"
- "test_directory/"

- name: List keys simple
amazon.aws.s3_object:
bucket: "{{ bucket_name }}-acl-disabled"
mode: list

- name: Delete bucket created in this test
s3_bucket:
Expand Down