From 1f54820037e58765ad2b79aa70d4e735b9daa82b Mon Sep 17 00:00:00 2001 From: Mark Chappell Date: Mon, 21 Jun 2021 09:44:35 +0200 Subject: [PATCH 1/5] Cleanly fail if bucket encryption is 'NotImplemented' --- plugins/modules/s3_bucket.py | 41 +++++++++++++++++++----------------- 1 file changed, 22 insertions(+), 19 deletions(-) diff --git a/plugins/modules/s3_bucket.py b/plugins/modules/s3_bucket.py index 82950fbe6ba..af71882160e 100644 --- a/plugins/modules/s3_bucket.py +++ b/plugins/modules/s3_bucket.py @@ -420,25 +420,28 @@ def create_or_update_bucket(s3_client, module, location): # Encryption try: current_encryption = get_bucket_encryption(s3_client, name) - except (ClientError, BotoCoreError) as e: - module.fail_json_aws(e, msg="Failed to get bucket encryption") - - if encryption is not None: - current_encryption_algorithm = current_encryption.get('SSEAlgorithm') if current_encryption else None - current_encryption_key = current_encryption.get('KMSMasterKeyID') if current_encryption else None - if encryption == 'none' and current_encryption_algorithm is not None: - try: - delete_bucket_encryption(s3_client, name) - except (BotoCoreError, ClientError) as e: - module.fail_json_aws(e, msg="Failed to delete bucket encryption") - current_encryption = wait_encryption_is_applied(module, s3_client, name, None) - changed = True - elif encryption != 'none' and (encryption != current_encryption_algorithm) or (encryption == 'aws:kms' and current_encryption_key != encryption_key_id): - expected_encryption = {'SSEAlgorithm': encryption} - if encryption == 'aws:kms' and encryption_key_id is not None: - expected_encryption.update({'KMSMasterKeyID': encryption_key_id}) - current_encryption = put_bucket_encryption_with_retry(module, s3_client, name, expected_encryption) - changed = True + except is_boto3_error_code(['NotImplemented', 'XNotImplemented']): + if encryption is not None: + module.fail_json_aws(exp, msg="Failed to get bucket encryption settings") + except (BotoCoreError, ClientError) as exp: # pylint: disable=duplicate-except + module.fail_json_aws(exp, msg="Failed to get bucket encryption settings") + else: + if encryption is not None: + current_encryption_algorithm = current_encryption.get('SSEAlgorithm') if current_encryption else None + current_encryption_key = current_encryption.get('KMSMasterKeyID') if current_encryption else None + if encryption == 'none' and current_encryption_algorithm is not None: + try: + delete_bucket_encryption(s3_client, name) + except (BotoCoreError, ClientError) as e: + module.fail_json_aws(e, msg="Failed to delete bucket encryption") + current_encryption = wait_encryption_is_applied(module, s3_client, name, None) + changed = True + elif encryption != 'none' and (encryption != current_encryption_algorithm) or (encryption == 'aws:kms' and current_encryption_key != encryption_key_id): + expected_encryption = {'SSEAlgorithm': encryption} + if encryption == 'aws:kms' and encryption_key_id is not None: + expected_encryption.update({'KMSMasterKeyID': encryption_key_id}) + current_encryption = put_bucket_encryption_with_retry(module, s3_client, name, expected_encryption) + changed = True result['encryption'] = current_encryption From 7735e2b202789d9d5a47994df833209c1c8ee8f2 Mon Sep 17 00:00:00 2001 From: Mark Chappell Date: Mon, 21 Jun 2021 09:58:10 +0200 Subject: [PATCH 2/5] Be more consistent with exception handling --- plugins/modules/s3_bucket.py | 84 ++++++++++++++++++------------------ 1 file changed, 42 insertions(+), 42 deletions(-) diff --git a/plugins/modules/s3_bucket.py b/plugins/modules/s3_bucket.py index af71882160e..5029185afe5 100644 --- a/plugins/modules/s3_bucket.py +++ b/plugins/modules/s3_bucket.py @@ -240,7 +240,7 @@ import time try: - from botocore.exceptions import BotoCoreError, ClientError, EndpointConnectionError, WaiterError + import botocore except ImportError: pass # Handled by AnsibleAWSModule @@ -279,9 +279,9 @@ def create_or_update_bucket(s3_client, module, location): try: bucket_is_present = bucket_exists(s3_client, name) - except EndpointConnectionError as e: + except botocore.exceptions.EndpointConnectionError as e: module.fail_json_aws(e, msg="Invalid endpoint provided: %s" % to_text(e)) - except (BotoCoreError, ClientError) as e: + except (botocore.exceptions.BotoCoreError, botocore.exceptions.ClientError) as e: module.fail_json_aws(e, msg="Failed to check bucket presence") if not bucket_is_present: @@ -289,19 +289,19 @@ def create_or_update_bucket(s3_client, module, location): bucket_changed = create_bucket(s3_client, name, location) s3_client.get_waiter('bucket_exists').wait(Bucket=name) changed = changed or bucket_changed - except WaiterError as e: + except botocore.exceptions.WaiterError as e: module.fail_json_aws(e, msg='An error occurred waiting for the bucket to become available') - except (BotoCoreError, ClientError) as e: + except (botocore.exceptions.BotoCoreError, botocore.exceptions.ClientError) as e: module.fail_json_aws(e, msg="Failed while creating bucket") # Versioning try: versioning_status = get_bucket_versioning(s3_client, name) - except is_boto3_error_code(['NotImplemented', 'XNotImplemented']) as exp: + except is_boto3_error_code(['NotImplemented', 'XNotImplemented']) as e: if versioning is not None: - module.fail_json_aws(exp, msg="Failed to get bucket versioning") - except (BotoCoreError, ClientError) as exp: # pylint: disable=duplicate-except - module.fail_json_aws(exp, msg="Failed to get bucket versioning") + module.fail_json_aws(e, msg="Failed to get bucket versioning") + except (botocore.exceptions.BotoCoreError, botocore.exceptions.ClientError) as e: # pylint: disable=duplicate-except + module.fail_json_aws(e, msg="Failed to get bucket versioning") else: if versioning is not None: required_versioning = None @@ -314,7 +314,7 @@ def create_or_update_bucket(s3_client, module, location): try: put_bucket_versioning(s3_client, name, required_versioning) changed = True - except (BotoCoreError, ClientError) as e: + except (botocore.exceptions.BotoCoreError, botocore.exceptions.ClientError) as e: module.fail_json_aws(e, msg="Failed to update bucket versioning") versioning_status = wait_versioning_is_applied(module, s3_client, name, required_versioning) @@ -330,9 +330,9 @@ def create_or_update_bucket(s3_client, module, location): requester_pays_status = get_bucket_request_payment(s3_client, name) except is_boto3_error_code(['NotImplemented', 'XNotImplemented']): if requester_pays is not None: - module.fail_json_aws(exp, msg="Failed to get bucket request payment") - except (BotoCoreError, ClientError) as exp: # pylint: disable=duplicate-except - module.fail_json_aws(exp, msg="Failed to get bucket request payment") + module.fail_json_aws(e, msg="Failed to get bucket request payment") + except (botocore.exceptions.BotoCoreError, botocore.exceptions.ClientError) as e: # pylint: disable=duplicate-except + module.fail_json_aws(e, msg="Failed to get bucket request payment") else: if requester_pays is not None: payer = 'Requester' if requester_pays else 'BucketOwner' @@ -353,9 +353,9 @@ def create_or_update_bucket(s3_client, module, location): current_policy = get_bucket_policy(s3_client, name) except is_boto3_error_code(['NotImplemented', 'XNotImplemented']): if policy is not None: - module.fail_json_aws(exp, msg="Failed to get bucket policy") - except (BotoCoreError, ClientError) as exp: # pylint: disable=duplicate-except - module.fail_json_aws(exp, msg="Failed to get bucket policy") + module.fail_json_aws(e, msg="Failed to get bucket policy") + except (botocore.exceptions.BotoCoreError, botocore.exceptions.ClientError) as e: # pylint: disable=duplicate-except + module.fail_json_aws(e, msg="Failed to get bucket policy") else: if policy is not None: if isinstance(policy, string_types): @@ -364,14 +364,14 @@ def create_or_update_bucket(s3_client, module, location): if not policy and current_policy: try: delete_bucket_policy(s3_client, name) - except (BotoCoreError, ClientError) as e: + except (botocore.exceptions.BotoCoreError, botocore.exceptions.ClientError) as e: module.fail_json_aws(e, msg="Failed to delete bucket policy") current_policy = wait_policy_is_applied(module, s3_client, name, policy) changed = True elif compare_policies(current_policy, policy): try: put_bucket_policy(s3_client, name, policy) - except (BotoCoreError, ClientError) as e: + except (botocore.exceptions.BotoCoreError, botocore.exceptions.ClientError) as e: module.fail_json_aws(e, msg="Failed to update bucket policy") current_policy = wait_policy_is_applied(module, s3_client, name, policy, should_fail=False) if current_policy is None: @@ -388,9 +388,9 @@ def create_or_update_bucket(s3_client, module, location): current_tags_dict = get_current_bucket_tags_dict(s3_client, name) except is_boto3_error_code(['NotImplemented', 'XNotImplemented']): if tags is not None: - module.fail_json_aws(exp, msg="Failed to get bucket tags") - except (ClientError, BotoCoreError) as exp: # pylint: disable=duplicate-except - module.fail_json_aws(exp, msg="Failed to get bucket tags") + module.fail_json_aws(e, msg="Failed to get bucket tags") + except (botocore.exceptions.BotoCoreError, botocore.exceptions.ClientError) as e: # pylint: disable=duplicate-except + module.fail_json_aws(e, msg="Failed to get bucket tags") else: if tags is not None: # Tags are always returned as text @@ -404,13 +404,13 @@ def create_or_update_bucket(s3_client, module, location): if tags: try: put_bucket_tagging(s3_client, name, tags) - except (BotoCoreError, ClientError) as e: + except (botocore.exceptions.BotoCoreError, botocore.exceptions.ClientError) as e: module.fail_json_aws(e, msg="Failed to update bucket tags") else: if purge_tags: try: delete_bucket_tagging(s3_client, name) - except (BotoCoreError, ClientError) as e: + except (botocore.exceptions.BotoCoreError, botocore.exceptions.ClientError) as e: module.fail_json_aws(e, msg="Failed to delete bucket tags") current_tags_dict = wait_tags_are_applied(module, s3_client, name, tags) changed = True @@ -422,9 +422,9 @@ def create_or_update_bucket(s3_client, module, location): current_encryption = get_bucket_encryption(s3_client, name) except is_boto3_error_code(['NotImplemented', 'XNotImplemented']): if encryption is not None: - module.fail_json_aws(exp, msg="Failed to get bucket encryption settings") - except (BotoCoreError, ClientError) as exp: # pylint: disable=duplicate-except - module.fail_json_aws(exp, msg="Failed to get bucket encryption settings") + module.fail_json_aws(e, msg="Failed to get bucket encryption settings") + except (botocore.exceptions.BotoCoreError, botocore.exceptions.ClientError) as e: # pylint: disable=duplicate-except + module.fail_json_aws(e, msg="Failed to get bucket encryption settings") else: if encryption is not None: current_encryption_algorithm = current_encryption.get('SSEAlgorithm') if current_encryption else None @@ -432,7 +432,7 @@ def create_or_update_bucket(s3_client, module, location): if encryption == 'none' and current_encryption_algorithm is not None: try: delete_bucket_encryption(s3_client, name) - except (BotoCoreError, ClientError) as e: + except (botocore.exceptions.BotoCoreError, botocore.exceptions.ClientError) as e: module.fail_json_aws(e, msg="Failed to delete bucket encryption") current_encryption = wait_encryption_is_applied(module, s3_client, name, None) changed = True @@ -452,8 +452,8 @@ def create_or_update_bucket(s3_client, module, location): if public_access is not None: try: current_public_access = get_bucket_public_access(s3_client, name) - except (ClientError, BotoCoreError) as err_public_access: - module.fail_json_aws(err_public_access, msg="Failed to get bucket public access configuration") + except (botocore.exceptions.BotoCoreError, botocore.exceptions.ClientError) as e: # pylint: disable=duplicate-except + module.fail_json_aws(e, msg="Failed to get bucket public access configuration") camel_public_block = snake_dict_to_camel_dict(public_access, capitalize_first=True) if current_public_access == camel_public_block: @@ -467,8 +467,8 @@ def create_or_update_bucket(s3_client, module, location): if delete_public_access: try: current_public_access = get_bucket_public_access(s3_client, name) - except (ClientError, BotoCoreError) as err_public_access: - module.fail_json_aws(err_public_access, msg="Failed to get bucket public access configuration") + except (botocore.exceptions.BotoCoreError, botocore.exceptions.ClientError) as e: # pylint: disable=duplicate-except + module.fail_json_aws(e, msg="Failed to get bucket public access configuration") if current_public_access == {}: result['public_access_block'] = current_public_access @@ -586,7 +586,7 @@ def put_bucket_encryption_with_retry(module, s3_client, name, expected_encryptio for retries in range(1, max_retries + 1): try: put_bucket_encryption(s3_client, name, expected_encryption) - except (BotoCoreError, ClientError) as e: + except (botocore.exceptions.BotoCoreError, botocore.exceptions.ClientError) as e: # pylint: disable=duplicate-except module.fail_json_aws(e, msg="Failed to set bucket encryption") current_encryption = wait_encryption_is_applied(module, s3_client, name, expected_encryption, should_fail=(retries == max_retries), retries=5) @@ -666,7 +666,7 @@ def wait_policy_is_applied(module, s3_client, bucket_name, expected_policy, shou for dummy in range(0, 12): try: current_policy = get_bucket_policy(s3_client, bucket_name) - except (ClientError, BotoCoreError) as e: + except (botocore.exceptions.BotoCoreError, botocore.exceptions.ClientError) as e: module.fail_json_aws(e, msg="Failed to get bucket policy") if compare_policies(current_policy, expected_policy): @@ -684,7 +684,7 @@ def wait_payer_is_applied(module, s3_client, bucket_name, expected_payer, should for dummy in range(0, 12): try: requester_pays_status = get_bucket_request_payment(s3_client, bucket_name) - except (BotoCoreError, ClientError) as e: + except (botocore.exceptions.BotoCoreError, botocore.exceptions.ClientError) as e: module.fail_json_aws(e, msg="Failed to get bucket request payment") if requester_pays_status != expected_payer: time.sleep(5) @@ -701,7 +701,7 @@ def wait_encryption_is_applied(module, s3_client, bucket_name, expected_encrypti for dummy in range(0, retries): try: encryption = get_bucket_encryption(s3_client, bucket_name) - except (BotoCoreError, ClientError) as e: + except (botocore.exceptions.BotoCoreError, botocore.exceptions.ClientError) as e: module.fail_json_aws(e, msg="Failed to get updated encryption for bucket") if encryption != expected_encryption: time.sleep(5) @@ -719,7 +719,7 @@ def wait_versioning_is_applied(module, s3_client, bucket_name, required_versioni for dummy in range(0, 24): try: versioning_status = get_bucket_versioning(s3_client, bucket_name) - except (BotoCoreError, ClientError) as e: + except (botocore.exceptions.BotoCoreError, botocore.exceptions.ClientError) as e: module.fail_json_aws(e, msg="Failed to get updated versioning for bucket") if versioning_status.get('Status') != required_versioning: time.sleep(8) @@ -733,7 +733,7 @@ def wait_tags_are_applied(module, s3_client, bucket_name, expected_tags_dict): for dummy in range(0, 12): try: current_tags_dict = get_current_bucket_tags_dict(s3_client, bucket_name) - except (ClientError, BotoCoreError) as e: + except (botocore.exceptions.BotoCoreError, botocore.exceptions.ClientError) as e: module.fail_json_aws(e, msg="Failed to get bucket policy") if current_tags_dict != expected_tags_dict: time.sleep(5) @@ -801,9 +801,9 @@ def destroy_bucket(s3_client, module): name = module.params.get("name") try: bucket_is_present = bucket_exists(s3_client, name) - except EndpointConnectionError as e: + except botocore.exceptions.EndpointConnectionError as e: module.fail_json_aws(e, msg="Invalid endpoint provided: %s" % to_text(e)) - except (BotoCoreError, ClientError) as e: + except (botocore.exceptions.BotoCoreError, botocore.exceptions.ClientError) as e: module.fail_json_aws(e, msg="Failed to check bucket presence") if not bucket_is_present: @@ -831,15 +831,15 @@ def destroy_bucket(s3_client, module): ), errors=resp['Errors'], response=resp ) - except (BotoCoreError, ClientError) as e: + except (botocore.exceptions.BotoCoreError, botocore.exceptions.ClientError) as e: module.fail_json_aws(e, msg="Failed while deleting bucket") try: delete_bucket(s3_client, name) s3_client.get_waiter('bucket_not_exists').wait(Bucket=name, WaiterConfig=dict(Delay=5, MaxAttempts=60)) - except WaiterError as e: + except botocore.exceptions.WaiterError as e: module.fail_json_aws(e, msg='An error occurred waiting for the bucket to be deleted.') - except (BotoCoreError, ClientError) as e: + except (botocore.exceptions.BotoCoreError, botocore.exceptions.ClientError) as e: module.fail_json_aws(e, msg="Failed to delete bucket") module.exit_json(changed=True) From 36fefdd6f58744dd868b97972467e7337ecbbe03 Mon Sep 17 00:00:00 2001 From: Mark Chappell Date: Mon, 21 Jun 2021 10:11:31 +0200 Subject: [PATCH 3/5] Rework the logic a little to bring down the line length (we're not more deeply indented) --- plugins/modules/s3_bucket.py | 28 +++++++++++++++------------- 1 file changed, 15 insertions(+), 13 deletions(-) diff --git a/plugins/modules/s3_bucket.py b/plugins/modules/s3_bucket.py index 5029185afe5..def31456d86 100644 --- a/plugins/modules/s3_bucket.py +++ b/plugins/modules/s3_bucket.py @@ -429,19 +429,21 @@ def create_or_update_bucket(s3_client, module, location): if encryption is not None: current_encryption_algorithm = current_encryption.get('SSEAlgorithm') if current_encryption else None current_encryption_key = current_encryption.get('KMSMasterKeyID') if current_encryption else None - if encryption == 'none' and current_encryption_algorithm is not None: - try: - delete_bucket_encryption(s3_client, name) - except (botocore.exceptions.BotoCoreError, botocore.exceptions.ClientError) as e: - module.fail_json_aws(e, msg="Failed to delete bucket encryption") - current_encryption = wait_encryption_is_applied(module, s3_client, name, None) - changed = True - elif encryption != 'none' and (encryption != current_encryption_algorithm) or (encryption == 'aws:kms' and current_encryption_key != encryption_key_id): - expected_encryption = {'SSEAlgorithm': encryption} - if encryption == 'aws:kms' and encryption_key_id is not None: - expected_encryption.update({'KMSMasterKeyID': encryption_key_id}) - current_encryption = put_bucket_encryption_with_retry(module, s3_client, name, expected_encryption) - changed = True + if encryption == 'none': + if current_encryption_algorithm is not None: + try: + delete_bucket_encryption(s3_client, name) + except (botocore.exceptions.BotoCoreError, botocore.exceptions.ClientError) as e: + module.fail_json_aws(e, msg="Failed to delete bucket encryption") + current_encryption = wait_encryption_is_applied(module, s3_client, name, None) + changed = True + else: + if (encryption != current_encryption_algorithm) or (encryption == 'aws:kms' and current_encryption_key != encryption_key_id): + expected_encryption = {'SSEAlgorithm': encryption} + if encryption == 'aws:kms' and encryption_key_id is not None: + expected_encryption.update({'KMSMasterKeyID': encryption_key_id}) + current_encryption = put_bucket_encryption_with_retry(module, s3_client, name, expected_encryption) + changed = True result['encryption'] = current_encryption From c8d33d4a62f31726ea24445eafb9e197552635c0 Mon Sep 17 00:00:00 2001 From: Mark Chappell Date: Mon, 21 Jun 2021 10:20:30 +0200 Subject: [PATCH 4/5] changelog --- changelogs/fragments/391-s3_bucket-enc_notimplemented.yml | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 changelogs/fragments/391-s3_bucket-enc_notimplemented.yml diff --git a/changelogs/fragments/391-s3_bucket-enc_notimplemented.yml b/changelogs/fragments/391-s3_bucket-enc_notimplemented.yml new file mode 100644 index 00000000000..238cd8ebbc8 --- /dev/null +++ b/changelogs/fragments/391-s3_bucket-enc_notimplemented.yml @@ -0,0 +1,2 @@ +bugfixes: +- s3_bucket - Gracefully handle ``NotImplemented`` exceptions when fetching encryption settings (https://github.com/ansible-collections/amazon.aws/issues/390). From 19a8b7cb5bfdcdd6bf3ab6bfc540f1876cb4db03 Mon Sep 17 00:00:00 2001 From: Mark Chappell Date: Mon, 21 Jun 2021 10:26:40 +0200 Subject: [PATCH 5/5] Fix error handling (not collecting the exception) when setting a NotImplemented function --- changelogs/fragments/391-s3_bucket-enc_notimplemented.yml | 1 + plugins/modules/s3_bucket.py | 8 ++++---- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/changelogs/fragments/391-s3_bucket-enc_notimplemented.yml b/changelogs/fragments/391-s3_bucket-enc_notimplemented.yml index 238cd8ebbc8..917c6d97363 100644 --- a/changelogs/fragments/391-s3_bucket-enc_notimplemented.yml +++ b/changelogs/fragments/391-s3_bucket-enc_notimplemented.yml @@ -1,2 +1,3 @@ bugfixes: - s3_bucket - Gracefully handle ``NotImplemented`` exceptions when fetching encryption settings (https://github.com/ansible-collections/amazon.aws/issues/390). +- s3_bucket - Fix error handling when attempting to set a feature that is not implemented (https://github.com/ansible-collections/amazon.aws/pull/391). diff --git a/plugins/modules/s3_bucket.py b/plugins/modules/s3_bucket.py index def31456d86..f2c6f525f24 100644 --- a/plugins/modules/s3_bucket.py +++ b/plugins/modules/s3_bucket.py @@ -328,7 +328,7 @@ def create_or_update_bucket(s3_client, module, location): # Requester pays try: requester_pays_status = get_bucket_request_payment(s3_client, name) - except is_boto3_error_code(['NotImplemented', 'XNotImplemented']): + except is_boto3_error_code(['NotImplemented', 'XNotImplemented']) as e: if requester_pays is not None: module.fail_json_aws(e, msg="Failed to get bucket request payment") except (botocore.exceptions.BotoCoreError, botocore.exceptions.ClientError) as e: # pylint: disable=duplicate-except @@ -351,7 +351,7 @@ def create_or_update_bucket(s3_client, module, location): # Policy try: current_policy = get_bucket_policy(s3_client, name) - except is_boto3_error_code(['NotImplemented', 'XNotImplemented']): + except is_boto3_error_code(['NotImplemented', 'XNotImplemented']) as e: if policy is not None: module.fail_json_aws(e, msg="Failed to get bucket policy") except (botocore.exceptions.BotoCoreError, botocore.exceptions.ClientError) as e: # pylint: disable=duplicate-except @@ -386,7 +386,7 @@ def create_or_update_bucket(s3_client, module, location): # Tags try: current_tags_dict = get_current_bucket_tags_dict(s3_client, name) - except is_boto3_error_code(['NotImplemented', 'XNotImplemented']): + except is_boto3_error_code(['NotImplemented', 'XNotImplemented']) as e: if tags is not None: module.fail_json_aws(e, msg="Failed to get bucket tags") except (botocore.exceptions.BotoCoreError, botocore.exceptions.ClientError) as e: # pylint: disable=duplicate-except @@ -420,7 +420,7 @@ def create_or_update_bucket(s3_client, module, location): # Encryption try: current_encryption = get_bucket_encryption(s3_client, name) - except is_boto3_error_code(['NotImplemented', 'XNotImplemented']): + except is_boto3_error_code(['NotImplemented', 'XNotImplemented']) as e: if encryption is not None: module.fail_json_aws(e, msg="Failed to get bucket encryption settings") except (botocore.exceptions.BotoCoreError, botocore.exceptions.ClientError) as e: # pylint: disable=duplicate-except