From 9184741e646084c680d188a8dfe22b2885fef00a Mon Sep 17 00:00:00 2001 From: Mark Chappell Date: Tue, 16 May 2023 09:58:03 +0200 Subject: [PATCH 1/8] Integration test --- .../targets/s3_object/tasks/main.yml | 21 +++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/tests/integration/targets/s3_object/tasks/main.yml b/tests/integration/targets/s3_object/tasks/main.yml index f3ca74856ed..0178a98e3c1 100644 --- a/tests/integration/targets/s3_object/tasks/main.yml +++ b/tests/integration/targets/s3_object/tasks/main.yml @@ -269,6 +269,27 @@ that: - upload_file.stat.checksum == download_file.stat.checksum + - name: test get object (absolute path) + s3_object: + bucket: "{{ bucket_name }}" + mode: get + dest: "{{ remote_tmp_dir }}/download-2.txt" + object: /delete.txt + retries: 3 + delay: 3 + register: result + until: "result.msg == 'GET operation complete'" + + - name: stat the file so we can compare the checksums + stat: + path: "{{ remote_tmp_dir }}/download-2.txt" + get_checksum: yes + register: download_file + + - assert: + that: + - upload_file.stat.checksum == download_file.stat.checksum + - name: test get with overwrite=different and identical files s3_object: bucket: "{{ bucket_name }}" From e3fe0f01624f1db33ff0ba2ba2fbcf95c5b8c054 Mon Sep 17 00:00:00 2001 From: Mark Chappell Date: Tue, 16 May 2023 11:19:42 +0200 Subject: [PATCH 2/8] s3_object - fix regression related to leading / in object key names --- .../1548-s3_object-leading-slash.yml | 4 ++ plugins/modules/s3_object.py | 37 +++++++++++++------ 2 files changed, 30 insertions(+), 11 deletions(-) create mode 100644 changelogs/fragments/1548-s3_object-leading-slash.yml diff --git a/changelogs/fragments/1548-s3_object-leading-slash.yml b/changelogs/fragments/1548-s3_object-leading-slash.yml new file mode 100644 index 00000000000..ffc4c1aa515 --- /dev/null +++ b/changelogs/fragments/1548-s3_object-leading-slash.yml @@ -0,0 +1,4 @@ +bugfixes: +- s3_object - fixes regression related to objects with a leading ``/`` (https://github.com/ansible-collections/amazon.aws/issues/1548). +deprecated_features: +- s3_object - support for passing object keys with a leading ``/`` has been deprecated and will be removed in a release after 2025-12-01 (https://github.com/ansible-collections/amazon.aws/pull/1549). diff --git a/plugins/modules/s3_object.py b/plugins/modules/s3_object.py index 6570bccd8c2..68e7de8fae1 100644 --- a/plugins/modules/s3_object.py +++ b/plugins/modules/s3_object.py @@ -89,8 +89,12 @@ type: str object: description: - - Keyname of the object inside the bucket. + - Key name of the object inside the bucket. - Can be used to create "virtual directories", see examples. + - Object key names should not include the leading C(/), see + U(https://docs.aws.amazon.com/AmazonS3/latest/userguide/object-keys.html) for more + information. Support for passing the leading C(/) has been deprecated and will be removed + in a release after 2025-12-01. type: str sig_v4: description: @@ -399,13 +403,13 @@ - prefix1/key2 """ +import base64 +import copy +import io import mimetypes import os -import io -from ssl import SSLError -import base64 import time - +from ssl import SSLError try: import botocore @@ -1325,9 +1329,9 @@ def s3_object_do_copy(module, connection, connection_v4, s3_vars): ) -def populate_facts(module, **variable_dict): - for k, v in module.params.items(): - variable_dict[k] = v +def populate_params(module): + # Copy the parameters dict, we shouldn't be directly modifying it. + variable_dict = copy.deepcopy(module.params) if variable_dict["validate_bucket_name"]: validate_bucket_name(variable_dict["bucket"]) @@ -1347,8 +1351,19 @@ def populate_facts(module, **variable_dict): variable_dict["overwrite"] = "never" # Bucket deletion does not require obj. Prevents ambiguity with delobj. - if variable_dict["object"] and variable_dict.get("mode") == "delete": - module.fail_json(msg="Parameter obj cannot be used with mode=delete") + if variable_dict["object"]: + if variable_dict.get("mode") == "delete": + module.fail_json(msg="Parameter object cannot be used with mode=delete") + obj = variable_dict["object"] + # If the object starts with / remove the leading character + if obj.startswith("/"): + obj = obj[1:] + variable_dict["object"] = obj + module.deprecate( + "Support for passing object key names with a leading '/' has been deprecated.", + date="2025-12-01", + collection_name="amazon.aws", + ) variable_dict["validate"] = not variable_dict["ignore_nonexistent_bucket"] variable_dict["acl_disabled"] = False @@ -1483,7 +1498,7 @@ def main(): except (botocore.exceptions.ClientError, botocore.exceptions.BotoCoreError) as e: module.fail_json_aws(e, msg="Failed to connect to AWS") - s3_object_params = populate_facts(module, **module.params) + s3_object_params = populate_params(module) s3_object_params.update(validate_bucket(module, s3, s3_object_params)) func_mapping = { From 1071e9c517d8593ac4204c3ab839ee7b45c2b5ec Mon Sep 17 00:00:00 2001 From: Mark Chappell Date: Tue, 16 May 2023 11:47:46 +0200 Subject: [PATCH 3/8] s3_object unit tests --- tests/unit/plugins/modules/test_s3_object.py | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/tests/unit/plugins/modules/test_s3_object.py b/tests/unit/plugins/modules/test_s3_object.py index 630efff4575..c8d3fc4fcf3 100644 --- a/tests/unit/plugins/modules/test_s3_object.py +++ b/tests/unit/plugins/modules/test_s3_object.py @@ -95,7 +95,7 @@ def test_s3_object_do_list_success(m_paginated_list, m_list_keys): @patch(utils + ".get_aws_connection_info") -def test_populate_facts(m_get_aws_connection_info): +def test_populate_params(m_get_aws_connection_info): module = MagicMock() m_get_aws_connection_info.return_value = ( "us-east-1", @@ -143,10 +143,18 @@ def test_populate_facts(m_get_aws_connection_info): "validate_certs": True, "version": None, } - result = s3_object.populate_facts(module) + result = s3_object.populate_params(module) for k, v in module.params.items(): assert result[k] == v + module.params.update({"object": "example.txt", "mode": "get"}) + result = s3_object.populate_params(module) + assert result["object"] == "example.txt" + + module.params.update({"object": "/example.txt", "mode": "get"}) + result = s3_object.populate_params(module) + assert result["object"] == "example.txt" + module.params.update({"object": "example.txt", "mode": "delete"}) - result = s3_object.populate_facts(module) - module.fail_json.assert_called_with(msg="Parameter obj cannot be used with mode=delete") + result = s3_object.populate_params(module) + module.fail_json.assert_called_with(msg="Parameter object cannot be used with mode=delete") From d21b3a1f17f611b87cce8c14a14b15bae5d150f4 Mon Sep 17 00:00:00 2001 From: Mark Chappell Date: Tue, 16 May 2023 13:10:00 +0200 Subject: [PATCH 4/8] Fixups --- .../targets/s3_object/tasks/copy_object_acl_disabled_bucket.yml | 2 +- tests/integration/targets/s3_object/tasks/main.yml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/integration/targets/s3_object/tasks/copy_object_acl_disabled_bucket.yml b/tests/integration/targets/s3_object/tasks/copy_object_acl_disabled_bucket.yml index 4f9bdb6df6f..9eba65f4fb1 100644 --- a/tests/integration/targets/s3_object/tasks/copy_object_acl_disabled_bucket.yml +++ b/tests/integration/targets/s3_object/tasks/copy_object_acl_disabled_bucket.yml @@ -98,7 +98,7 @@ - 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' + - '"Virtual directory test_directory/ created" in permission_result.msg' always: diff --git a/tests/integration/targets/s3_object/tasks/main.yml b/tests/integration/targets/s3_object/tasks/main.yml index 0178a98e3c1..5b36fc7c444 100644 --- a/tests/integration/targets/s3_object/tasks/main.yml +++ b/tests/integration/targets/s3_object/tasks/main.yml @@ -649,7 +649,7 @@ s3_object: bucket: "{{ bucket_name_acl }}" mode: put - src: "{{ tmpdir.path }}/upload.txt" + src: "{{ remote_tmp_dir }}/upload.txt" object: file-with-permissions.txt permission: private ignore_nonexistent_bucket: True From ee3183b5f9825c9fe6f74874152f3f11a56b4b58 Mon Sep 17 00:00:00 2001 From: Mark Chappell Date: Tue, 16 May 2023 13:59:29 +0200 Subject: [PATCH 5/8] bad ACL --- tests/integration/targets/s3_object/tasks/main.yml | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/integration/targets/s3_object/tasks/main.yml b/tests/integration/targets/s3_object/tasks/main.yml index 5b36fc7c444..5603ddc1ec1 100644 --- a/tests/integration/targets/s3_object/tasks/main.yml +++ b/tests/integration/targets/s3_object/tasks/main.yml @@ -645,13 +645,14 @@ that: - result is not changed + # Public objects aren't allowed by default - name: fail to upload the file to the bucket with an ACL s3_object: bucket: "{{ bucket_name_acl }}" mode: put src: "{{ remote_tmp_dir }}/upload.txt" object: file-with-permissions.txt - permission: private + permission: public-read ignore_nonexistent_bucket: True register: upload_private ignore_errors: True From 162ea7ffa72274a8a5a9dfbbcd7112294b49aea4 Mon Sep 17 00:00:00 2001 From: Mark Chappell Date: Tue, 16 May 2023 14:00:26 +0200 Subject: [PATCH 6/8] Catch boto3.exceptions.Boto3Error, boto3 s3 client is sometimes re-raising exceptions as its own --- plugins/modules/s3_object.py | 25 ++++++++++++++++++++++++- 1 file changed, 24 insertions(+), 1 deletion(-) diff --git a/plugins/modules/s3_object.py b/plugins/modules/s3_object.py index 68e7de8fae1..872356c697b 100644 --- a/plugins/modules/s3_object.py +++ b/plugins/modules/s3_object.py @@ -412,6 +412,9 @@ from ssl import SSLError try: + # Beware, S3 is a "special" case, it sometimes catches botocore exceptions and + # re-raises them as boto3 exceptions. + import boto3 import botocore except ImportError: pass # Handled by AnsibleAWSModule @@ -461,6 +464,7 @@ def key_check(module, s3, bucket, obj, version=None, validate=True): except ( botocore.exceptions.BotoCoreError, botocore.exceptions.ClientError, + boto3.exceptions.Boto3Error, ) as e: # pylint: disable=duplicate-except raise S3ObjectFailure(f"Failed while looking up object (during key check) {obj}.", e) @@ -529,6 +533,7 @@ def bucket_check(module, s3, bucket, validate=True): except ( botocore.exceptions.BotoCoreError, botocore.exceptions.ClientError, + boto3.exceptions.Boto3Error, ) as e: # pylint: disable=duplicate-except raise S3ObjectFailure( f"Failed while looking up bucket '{bucket}' (during bucket_check).", @@ -575,6 +580,7 @@ def list_keys(module, s3, bucket, prefix, marker, max_keys): except ( botocore.exceptions.ClientError, botocore.exceptions.BotoCoreError, + boto3.exceptions.Boto3Error, ) as e: raise S3ObjectFailure(f"Failed while listing the keys in the bucket {bucket}", e) @@ -591,6 +597,7 @@ def delete_key(module, s3, bucket, obj): except ( botocore.exceptions.ClientError, botocore.exceptions.BotoCoreError, + boto3.exceptions.Boto3Error, ) as e: raise S3ObjectFailure(f"Failed while trying to delete {obj}.", e) @@ -611,6 +618,7 @@ def put_object_acl(module, s3, bucket, obj, params=None): except ( botocore.exceptions.BotoCoreError, botocore.exceptions.ClientError, + boto3.exceptions.Boto3Error, ) as e: # pylint: disable=duplicate-except raise S3ObjectFailure(f"Failed while creating object {obj}.", e) @@ -751,6 +759,7 @@ def upload_s3file( except ( botocore.exceptions.ClientError, botocore.exceptions.BotoCoreError, + boto3.exceptions.Boto3Error, ) as e: raise S3ObjectFailure("Unable to complete PUT operation.", e) @@ -790,6 +799,7 @@ def download_s3file(module, s3, bucket, obj, dest, retries, version=None): except ( botocore.exceptions.BotoCoreError, botocore.exceptions.ClientError, + boto3.exceptions.Boto3Error, ) as e: # pylint: disable=duplicate-except raise S3ObjectFailure(f"Could not find the key {obj}.", e) @@ -801,6 +811,7 @@ def download_s3file(module, s3, bucket, obj, dest, retries, version=None): except ( botocore.exceptions.ClientError, botocore.exceptions.BotoCoreError, + boto3.exceptions.Boto3Error, ) as e: # actually fail on last pass through the loop. if x >= retries: @@ -834,6 +845,7 @@ def download_s3str(module, s3, bucket, obj, version=None): except ( botocore.exceptions.BotoCoreError, botocore.exceptions.ClientError, + boto3.exceptions.Boto3Error, ) as e: # pylint: disable=duplicate-except raise S3ObjectFailure(f"Failed while getting contents of object {obj} as a string.", e) @@ -856,6 +868,7 @@ def get_download_url(module, s3, bucket, obj, expiry, tags=None, changed=True): except ( botocore.exceptions.ClientError, botocore.exceptions.BotoCoreError, + boto3.exceptions.Boto3Error, ) as e: raise S3ObjectFailure("Failed while getting download url.", e) @@ -871,6 +884,7 @@ def put_download_url(s3, bucket, obj, expiry): except ( botocore.exceptions.ClientError, botocore.exceptions.BotoCoreError, + boto3.exceptions.Boto3Error, ) as e: raise S3ObjectFailure("Unable to generate presigned URL", e) @@ -941,6 +955,7 @@ def copy_object_to_bucket(module, s3, bucket, obj, encrypt, metadata, validate, except ( botocore.exceptions.BotoCoreError, botocore.exceptions.ClientError, + boto3.exceptions.Boto3Error, ) as e: # pylint: disable=duplicate-except raise S3ObjectFailure( f"Failed while copying object {obj} from bucket {module.params['copy_src'].get('Bucket')}.", @@ -985,6 +1000,7 @@ def wait_tags_are_applied(module, s3, bucket, obj, expected_tags_dict, version=N except ( botocore.exceptions.ClientError, botocore.exceptions.BotoCoreError, + boto3.exceptions.Boto3Error, ) as e: raise S3ObjectFailure("Failed to get object tags.", e) @@ -1010,6 +1026,7 @@ def ensure_tags(client, module, bucket, obj): except ( botocore.exceptions.BotoCoreError, botocore.exceptions.ClientError, + boto3.exceptions.Boto3Error, ) as e: # pylint: disable=duplicate-except raise S3ObjectFailure("Failed to get object tags.", e) @@ -1033,6 +1050,7 @@ def ensure_tags(client, module, bucket, obj): except ( botocore.exceptions.BotoCoreError, botocore.exceptions.ClientError, + boto3.exceptions.Boto3Error, ) as e: raise S3ObjectFailure("Failed to update object tags.", e) else: @@ -1041,6 +1059,7 @@ def ensure_tags(client, module, bucket, obj): except ( botocore.exceptions.BotoCoreError, botocore.exceptions.ClientError, + boto3.exceptions.Boto3Error, ) as e: raise S3ObjectFailure("Failed to delete object tags.", e) @@ -1495,7 +1514,11 @@ def main(): try: s3 = module.client("s3", retry_decorator=retry_decorator, **extra_params) s3_v4 = module.client("s3", retry_decorator=retry_decorator, **extra_params_v4) - except (botocore.exceptions.ClientError, botocore.exceptions.BotoCoreError) as e: + except ( + botocore.exceptions.ClientError, + botocore.exceptions.BotoCoreError, + boto3.exceptions.Boto3Error, + ) as e: module.fail_json_aws(e, msg="Failed to connect to AWS") s3_object_params = populate_params(module) From 228c43df7fdebc02769371d35473a97acd1d593e Mon Sep 17 00:00:00 2001 From: Mark Chappell Date: Tue, 16 May 2023 15:14:43 +0200 Subject: [PATCH 7/8] Use s3_object_info instead of s3_object: { mode: list } --- tests/integration/targets/s3_object/tasks/delete_bucket.yml | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/tests/integration/targets/s3_object/tasks/delete_bucket.yml b/tests/integration/targets/s3_object/tasks/delete_bucket.yml index 1c6f4892743..69bc6143088 100644 --- a/tests/integration/targets/s3_object/tasks/delete_bucket.yml +++ b/tests/integration/targets/s3_object/tasks/delete_bucket.yml @@ -1,9 +1,8 @@ - name: delete bucket at the end of Integration tests block: - name: list bucket object - s3_object: - bucket: "{{ item }}" - mode: list + s3_object_info: + bucket_name: "{{ item }}" register: objects ignore_errors: true From 1450b6d630305a08da82531f22daf51526e23435 Mon Sep 17 00:00:00 2001 From: Mark Chappell Date: Tue, 16 May 2023 16:45:10 +0200 Subject: [PATCH 8/8] Remove deprecation --- .../fragments/1548-s3_object-leading-slash.yml | 4 ++-- plugins/modules/s3_object.py | 15 ++++++++------- 2 files changed, 10 insertions(+), 9 deletions(-) diff --git a/changelogs/fragments/1548-s3_object-leading-slash.yml b/changelogs/fragments/1548-s3_object-leading-slash.yml index ffc4c1aa515..17219af7057 100644 --- a/changelogs/fragments/1548-s3_object-leading-slash.yml +++ b/changelogs/fragments/1548-s3_object-leading-slash.yml @@ -1,4 +1,4 @@ bugfixes: - s3_object - fixes regression related to objects with a leading ``/`` (https://github.com/ansible-collections/amazon.aws/issues/1548). -deprecated_features: -- s3_object - support for passing object keys with a leading ``/`` has been deprecated and will be removed in a release after 2025-12-01 (https://github.com/ansible-collections/amazon.aws/pull/1549). +# deprecated_features: +# - s3_object - support for passing object keys with a leading ``/`` has been deprecated and will be removed in a release after 2025-12-01 (https://github.com/ansible-collections/amazon.aws/pull/1549). diff --git a/plugins/modules/s3_object.py b/plugins/modules/s3_object.py index 872356c697b..88c75447b33 100644 --- a/plugins/modules/s3_object.py +++ b/plugins/modules/s3_object.py @@ -93,8 +93,9 @@ - Can be used to create "virtual directories", see examples. - Object key names should not include the leading C(/), see U(https://docs.aws.amazon.com/AmazonS3/latest/userguide/object-keys.html) for more - information. Support for passing the leading C(/) has been deprecated and will be removed - in a release after 2025-12-01. + information. +# - Support for passing the leading C(/) has been deprecated and will be removed +# in a release after 2025-12-01. type: str sig_v4: description: @@ -1378,11 +1379,11 @@ def populate_params(module): if obj.startswith("/"): obj = obj[1:] variable_dict["object"] = obj - module.deprecate( - "Support for passing object key names with a leading '/' has been deprecated.", - date="2025-12-01", - collection_name="amazon.aws", - ) + # module.deprecate( + # "Support for passing object key names with a leading '/' has been deprecated.", + # date="2025-12-01", + # collection_name="amazon.aws", + # ) variable_dict["validate"] = not variable_dict["ignore_nonexistent_bucket"] variable_dict["acl_disabled"] = False