From b70c84fcf6adac09afded65af6441a33127a8c9a Mon Sep 17 00:00:00 2001 From: romi1495 Date: Mon, 6 Jul 2026 12:48:24 -0500 Subject: [PATCH 1/8] fix(s3_cors): add check_mode support, fix idempotency, and add integration tests This commit addresses multiple issues with the s3_cors module and adds comprehensive test coverage. Bug Fixes: - Add support for check_mode operation to both create and delete operations - Fix idempotency issue where state=absent always reported changes even when no CORS configuration existed - Replace compare_policies() (designed for IAM policies) with a CORS-specific normalize_cors_rules() function for accurate rule comparison - Use is_boto3_error_code() helper for proper NoSuchCORSConfiguration handling Tests: - Add comprehensive integration test suite with 45 test assertions - Test coverage includes: - Create CORS rules with check_mode and idempotency - Update CORS rules with check_mode and idempotency - Multiple CORS rules support - Wildcard origins handling - Remove CORS rules with check_mode and idempotency All tests pass successfully against live AWS S3. --- changelogs/fragments/s3_cors-improvements.yml | 8 + plugins/modules/s3_cors.py | 50 +- tests/integration/targets/s3_cors/aliases | 1 + .../targets/s3_cors/defaults/main.yml | 2 + .../integration/targets/s3_cors/meta/main.yml | 2 + .../targets/s3_cors/tasks/main.yml | 501 ++++++++++++++++++ 6 files changed, 556 insertions(+), 8 deletions(-) create mode 100644 changelogs/fragments/s3_cors-improvements.yml create mode 100644 tests/integration/targets/s3_cors/aliases create mode 100644 tests/integration/targets/s3_cors/defaults/main.yml create mode 100644 tests/integration/targets/s3_cors/meta/main.yml create mode 100644 tests/integration/targets/s3_cors/tasks/main.yml diff --git a/changelogs/fragments/s3_cors-improvements.yml b/changelogs/fragments/s3_cors-improvements.yml new file mode 100644 index 00000000000..723d2fcc0ad --- /dev/null +++ b/changelogs/fragments/s3_cors-improvements.yml @@ -0,0 +1,8 @@ +--- +bugfixes: + - s3_cors - Added support for check_mode operation (https://github.com/ansible-collections/community.aws/pull/XXXX). + - s3_cors - Fixed idempotency issue in ``state=absent`` where the module always reported changes even when no CORS configuration existed (https://github.com/ansible-collections/community.aws/pull/XXXX). + - s3_cors - Fixed CORS rule comparison logic by replacing IAM policy comparison with CORS-specific normalization to ensure accurate change detection (https://github.com/ansible-collections/community.aws/pull/XXXX). + +tests: + - s3_cors - Added comprehensive integration tests covering create, update, delete operations with check_mode and idempotency validation (https://github.com/ansible-collections/community.aws/pull/XXXX). diff --git a/plugins/modules/s3_cors.py b/plugins/modules/s3_cors.py index c9cf2d0546a..f3ccf5a3d76 100644 --- a/plugins/modules/s3_cors.py +++ b/plugins/modules/s3_cors.py @@ -103,11 +103,31 @@ from ansible.module_utils.common.dict_transformations import snake_dict_to_camel_dict -from ansible_collections.amazon.aws.plugins.module_utils.policy import compare_policies +from ansible_collections.amazon.aws.plugins.module_utils.botocore import is_boto3_error_code from ansible_collections.community.aws.plugins.module_utils.modules import AnsibleCommunityAWSModule as AnsibleAWSModule +def normalize_cors_rules(rules): + """Normalize CORS rules for comparison by sorting lists within each rule.""" + if not rules: + return [] + + normalized = [] + for rule in rules: + norm_rule = {} + for key, value in rule.items(): + # Sort list values for consistent comparison + if isinstance(value, list): + norm_rule[key] = sorted(value) + else: + norm_rule[key] = value + normalized.append(norm_rule) + + # Sort rules by a consistent key (e.g., allowed_origins as a string representation) + return sorted(normalized, key=lambda x: str(sorted(x.items()))) + + def create_or_update_bucket_cors(connection, module): name = module.params.get("name") rules = module.params.get("rules", []) @@ -119,11 +139,12 @@ def create_or_update_bucket_cors(connection, module): current_camel_rules = [] new_camel_rules = snake_dict_to_camel_dict(rules, capitalize_first=True) - # compare_policies() takes two dicts and makes them hashable for comparison - if compare_policies(new_camel_rules, current_camel_rules): + + # Compare normalized rules + if normalize_cors_rules(new_camel_rules) != normalize_cors_rules(current_camel_rules): changed = True - if changed: + if changed and not module.check_mode: try: connection.put_bucket_cors(Bucket=name, CORSConfiguration={"CORSRules": new_camel_rules}) except (BotoCoreError, ClientError) as e: @@ -136,11 +157,24 @@ def destroy_bucket_cors(connection, module): name = module.params.get("name") changed = False + # Check if CORS configuration exists try: - connection.delete_bucket_cors(Bucket=name) + connection.get_bucket_cors(Bucket=name) + # CORS exists, so we need to delete it changed = True - except (BotoCoreError, ClientError) as e: - module.fail_json_aws(e, msg=f"Unable to delete CORS for bucket {name}") + except is_boto3_error_code("NoSuchCORSConfiguration"): + # Bucket has no CORS configuration, nothing to delete + changed = False + except (BotoCoreError, ClientError) as e: # pylint: disable=duplicate-except + # Some other error occurred + module.fail_json_aws(e, msg=f"Unable to get CORS configuration for bucket {name}") + + # Only delete if CORS exists and not in check mode + if changed and not module.check_mode: + try: + connection.delete_bucket_cors(Bucket=name) + except (BotoCoreError, ClientError) as e: + module.fail_json_aws(e, msg=f"Unable to delete CORS for bucket {name}") module.exit_json(changed=changed) @@ -152,7 +186,7 @@ def main(): state=dict(type="str", choices=["present", "absent"], required=True), ) - module = AnsibleAWSModule(argument_spec=argument_spec) + module = AnsibleAWSModule(argument_spec=argument_spec, supports_check_mode=True) client = module.client("s3") diff --git a/tests/integration/targets/s3_cors/aliases b/tests/integration/targets/s3_cors/aliases new file mode 100644 index 00000000000..4ef4b2067d0 --- /dev/null +++ b/tests/integration/targets/s3_cors/aliases @@ -0,0 +1 @@ +cloud/aws diff --git a/tests/integration/targets/s3_cors/defaults/main.yml b/tests/integration/targets/s3_cors/defaults/main.yml new file mode 100644 index 00000000000..2446c01e3dd --- /dev/null +++ b/tests/integration/targets/s3_cors/defaults/main.yml @@ -0,0 +1,2 @@ +--- +test_bucket: '{{ tiny_prefix }}-s3-cors' diff --git a/tests/integration/targets/s3_cors/meta/main.yml b/tests/integration/targets/s3_cors/meta/main.yml new file mode 100644 index 00000000000..23d65c7ef45 --- /dev/null +++ b/tests/integration/targets/s3_cors/meta/main.yml @@ -0,0 +1,2 @@ +--- +dependencies: [] diff --git a/tests/integration/targets/s3_cors/tasks/main.yml b/tests/integration/targets/s3_cors/tasks/main.yml new file mode 100644 index 00000000000..1c086be0579 --- /dev/null +++ b/tests/integration/targets/s3_cors/tasks/main.yml @@ -0,0 +1,501 @@ +--- +# Integration tests for s3_cors +# +# Notes: +# - s3_cors manages CORS configuration for S3 buckets +# - Tests cover state=present (create/update) and state=absent (remove) +# - Check mode and idempotency are tested for all operations +# +- module_defaults: + group/aws: + access_key: '{{ aws_access_key }}' + secret_key: '{{ aws_secret_key }}' + session_token: '{{ security_token | default(omit) }}' + region: '{{ aws_region }}' + collections: + - amazon.aws + block: + + # ============================================================ + # Setup - Create test bucket + # ============================================================ + + - name: Create simple s3_bucket for CORS testing + s3_bucket: + state: present + name: '{{ test_bucket }}' + register: output + + - name: Assert bucket was created + assert: + that: + - output is changed + - output.name == test_bucket + + # ============================================================ + # Test state=present - Create CORS rules + # ============================================================ + + - name: Create CORS rules (check_mode) + s3_cors: + state: present + name: '{{ test_bucket }}' + rules: + - allowed_origins: + - http://www.example.com + allowed_methods: + - GET + - POST + allowed_headers: + - Authorization + expose_headers: + - x-amz-server-side-encryption + - x-amz-request-id + max_age_seconds: 30000 + register: result + check_mode: true + + - name: Assert check_mode shows changed + assert: + that: + - result is changed + + - name: Create CORS rules + s3_cors: + state: present + name: '{{ test_bucket }}' + rules: + - allowed_origins: + - http://www.example.com + allowed_methods: + - GET + - POST + allowed_headers: + - Authorization + expose_headers: + - x-amz-server-side-encryption + - x-amz-request-id + max_age_seconds: 30000 + register: result + + - name: Assert CORS rules were created + assert: + that: + - result is changed + - result.name == test_bucket + - result.rules | length == 1 + - result.rules[0].allowed_origins == ['http://www.example.com'] + - result.rules[0].allowed_methods == ['GET', 'POST'] + + - name: Create CORS rules idempotency (check_mode) + s3_cors: + state: present + name: '{{ test_bucket }}' + rules: + - allowed_origins: + - http://www.example.com + allowed_methods: + - GET + - POST + allowed_headers: + - Authorization + expose_headers: + - x-amz-server-side-encryption + - x-amz-request-id + max_age_seconds: 30000 + register: result + check_mode: true + + - name: Assert check_mode shows no change + assert: + that: + - result is not changed + + - name: Create CORS rules idempotency + s3_cors: + state: present + name: '{{ test_bucket }}' + rules: + - allowed_origins: + - http://www.example.com + allowed_methods: + - GET + - POST + allowed_headers: + - Authorization + expose_headers: + - x-amz-server-side-encryption + - x-amz-request-id + max_age_seconds: 30000 + register: result + + - name: Assert idempotency - no change + assert: + that: + - result is not changed + + # ============================================================ + # Test state=present - Update CORS rules + # ============================================================ + + - name: Update CORS rules - change max_age_seconds (check_mode) + s3_cors: + state: present + name: '{{ test_bucket }}' + rules: + - allowed_origins: + - http://www.example.com + allowed_methods: + - GET + - POST + allowed_headers: + - Authorization + expose_headers: + - x-amz-server-side-encryption + - x-amz-request-id + max_age_seconds: 60000 + register: result + check_mode: true + + - name: Assert check_mode shows changed + assert: + that: + - result is changed + + - name: Update CORS rules - change max_age_seconds + s3_cors: + state: present + name: '{{ test_bucket }}' + rules: + - allowed_origins: + - http://www.example.com + allowed_methods: + - GET + - POST + allowed_headers: + - Authorization + expose_headers: + - x-amz-server-side-encryption + - x-amz-request-id + max_age_seconds: 60000 + register: result + + - name: Assert CORS rules were updated + assert: + that: + - result is changed + - result.rules[0].max_age_seconds == 60000 + + - name: Update CORS rules idempotency (check_mode) + s3_cors: + state: present + name: '{{ test_bucket }}' + rules: + - allowed_origins: + - http://www.example.com + allowed_methods: + - GET + - POST + allowed_headers: + - Authorization + expose_headers: + - x-amz-server-side-encryption + - x-amz-request-id + max_age_seconds: 60000 + register: result + check_mode: true + + - name: Assert check_mode shows no change + assert: + that: + - result is not changed + + - name: Update CORS rules idempotency + s3_cors: + state: present + name: '{{ test_bucket }}' + rules: + - allowed_origins: + - http://www.example.com + allowed_methods: + - GET + - POST + allowed_headers: + - Authorization + expose_headers: + - x-amz-server-side-encryption + - x-amz-request-id + max_age_seconds: 60000 + register: result + + - name: Assert idempotency - no change + assert: + that: + - result is not changed + + # ============================================================ + # Test state=present - Multiple CORS rules + # ============================================================ + + - name: Add multiple CORS rules (check_mode) + s3_cors: + state: present + name: '{{ test_bucket }}' + rules: + - allowed_origins: + - http://www.example.com + allowed_methods: + - GET + - POST + allowed_headers: + - Authorization + max_age_seconds: 60000 + - allowed_origins: + - https://www.example.org + allowed_methods: + - GET + allowed_headers: + - '*' + max_age_seconds: 3000 + register: result + check_mode: true + + - name: Assert check_mode shows changed + assert: + that: + - result is changed + + - name: Add multiple CORS rules + s3_cors: + state: present + name: '{{ test_bucket }}' + rules: + - allowed_origins: + - http://www.example.com + allowed_methods: + - GET + - POST + allowed_headers: + - Authorization + max_age_seconds: 60000 + - allowed_origins: + - https://www.example.org + allowed_methods: + - GET + allowed_headers: + - '*' + max_age_seconds: 3000 + register: result + + - name: Assert multiple CORS rules were created + assert: + that: + - result is changed + - result.rules | length == 2 + + - name: Add multiple CORS rules idempotency (check_mode) + s3_cors: + state: present + name: '{{ test_bucket }}' + rules: + - allowed_origins: + - http://www.example.com + allowed_methods: + - GET + - POST + allowed_headers: + - Authorization + max_age_seconds: 60000 + - allowed_origins: + - https://www.example.org + allowed_methods: + - GET + allowed_headers: + - '*' + max_age_seconds: 3000 + register: result + check_mode: true + + - name: Assert check_mode shows no change + assert: + that: + - result is not changed + + - name: Add multiple CORS rules idempotency + s3_cors: + state: present + name: '{{ test_bucket }}' + rules: + - allowed_origins: + - http://www.example.com + allowed_methods: + - GET + - POST + allowed_headers: + - Authorization + max_age_seconds: 60000 + - allowed_origins: + - https://www.example.org + allowed_methods: + - GET + allowed_headers: + - '*' + max_age_seconds: 3000 + register: result + + - name: Assert idempotency - no change + assert: + that: + - result is not changed + + # ============================================================ + # Test state=present - Wildcard origins + # ============================================================ + + - name: Set CORS with wildcard origin (check_mode) + s3_cors: + state: present + name: '{{ test_bucket }}' + rules: + - allowed_origins: + - '*' + allowed_methods: + - GET + allowed_headers: + - '*' + max_age_seconds: 3000 + register: result + check_mode: true + + - name: Assert check_mode shows changed + assert: + that: + - result is changed + + - name: Set CORS with wildcard origin + s3_cors: + state: present + name: '{{ test_bucket }}' + rules: + - allowed_origins: + - '*' + allowed_methods: + - GET + allowed_headers: + - '*' + max_age_seconds: 3000 + register: result + + - name: Assert wildcard CORS rule was created + assert: + that: + - result is changed + - result.rules | length == 1 + - result.rules[0].allowed_origins == ['*'] + + - name: Set CORS with wildcard origin idempotency (check_mode) + s3_cors: + state: present + name: '{{ test_bucket }}' + rules: + - allowed_origins: + - '*' + allowed_methods: + - GET + allowed_headers: + - '*' + max_age_seconds: 3000 + register: result + check_mode: true + + - name: Assert check_mode shows no change + assert: + that: + - result is not changed + + - name: Set CORS with wildcard origin idempotency + s3_cors: + state: present + name: '{{ test_bucket }}' + rules: + - allowed_origins: + - '*' + allowed_methods: + - GET + allowed_headers: + - '*' + max_age_seconds: 3000 + register: result + + - name: Assert idempotency - no change + assert: + that: + - result is not changed + + # ============================================================ + # Test state=absent - Remove CORS rules + # ============================================================ + + - name: Remove CORS rules (check_mode) + s3_cors: + state: absent + name: '{{ test_bucket }}' + register: result + check_mode: true + + - name: Assert check_mode shows changed + assert: + that: + - result is changed + + - name: Remove CORS rules + s3_cors: + state: absent + name: '{{ test_bucket }}' + register: result + + - name: Assert CORS rules were removed + assert: + that: + - result is changed + + - name: Remove CORS rules idempotency (check_mode) + s3_cors: + state: absent + name: '{{ test_bucket }}' + register: result + check_mode: true + + - name: Assert check_mode shows no change + assert: + that: + - result is not changed + + - name: Remove CORS rules idempotency + s3_cors: + state: absent + name: '{{ test_bucket }}' + register: result + + - name: Assert idempotency - no change (bucket has no CORS) + assert: + that: + - result is not changed + + # ============================================================ + # Cleanup + # ============================================================ + + always: + - name: Remove CORS configuration + s3_cors: + name: '{{ test_bucket }}' + state: absent + ignore_errors: true + + - name: Delete test bucket + s3_bucket: + name: '{{ test_bucket }}' + state: absent + force: true + ignore_errors: true From 882c3858d07932525ea54120687ffb0d7a8b0bf5 Mon Sep 17 00:00:00 2001 From: romi1495 Date: Wed, 8 Jul 2026 15:35:25 -0500 Subject: [PATCH 2/8] small edits --- tests/integration/targets/s3_cors/tasks/main.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/integration/targets/s3_cors/tasks/main.yml b/tests/integration/targets/s3_cors/tasks/main.yml index 1c086be0579..ac403169408 100644 --- a/tests/integration/targets/s3_cors/tasks/main.yml +++ b/tests/integration/targets/s3_cors/tasks/main.yml @@ -26,14 +26,14 @@ name: '{{ test_bucket }}' register: output - - name: Assert bucket was created + - name: Assert bucket was created properly assert: that: - output is changed - output.name == test_bucket # ============================================================ - # Test state=present - Create CORS rules + # Test Create CORS rules (state = present) # ============================================================ - name: Create CORS rules (check_mode) From a9ab0ee6451bf9f1e6f3273c23af796e9d90876b Mon Sep 17 00:00:00 2001 From: romi1495 Date: Wed, 8 Jul 2026 15:39:34 -0500 Subject: [PATCH 3/8] Update changelog fragment with PR #2471 --- changelogs/fragments/s3_cors-improvements.yml | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/changelogs/fragments/s3_cors-improvements.yml b/changelogs/fragments/s3_cors-improvements.yml index 723d2fcc0ad..6aef0ee3a55 100644 --- a/changelogs/fragments/s3_cors-improvements.yml +++ b/changelogs/fragments/s3_cors-improvements.yml @@ -1,8 +1,8 @@ --- bugfixes: - - s3_cors - Added support for check_mode operation (https://github.com/ansible-collections/community.aws/pull/XXXX). - - s3_cors - Fixed idempotency issue in ``state=absent`` where the module always reported changes even when no CORS configuration existed (https://github.com/ansible-collections/community.aws/pull/XXXX). - - s3_cors - Fixed CORS rule comparison logic by replacing IAM policy comparison with CORS-specific normalization to ensure accurate change detection (https://github.com/ansible-collections/community.aws/pull/XXXX). + - s3_cors - Added support for check_mode operation (https://github.com/ansible-collections/community.aws/pull/2471). + - s3_cors - Fixed idempotency issue in ``state=absent`` where the module always reported changes even when no CORS configuration existed (https://github.com/ansible-collections/community.aws/pull/2471). + - s3_cors - Fixed CORS rule comparison logic by replacing IAM policy comparison with CORS-specific normalization to ensure accurate change detection (https://github.com/ansible-collections/community.aws/pull/2471). tests: - - s3_cors - Added comprehensive integration tests covering create, update, delete operations with check_mode and idempotency validation (https://github.com/ansible-collections/community.aws/pull/XXXX). + - s3_cors - Added comprehensive integration tests covering create, update, delete operations with check_mode and idempotency validation (https://github.com/ansible-collections/community.aws/pull/2471). From e435ab45681a2214890362819af4dd7d0ad4603d Mon Sep 17 00:00:00 2001 From: romi1495 Date: Wed, 8 Jul 2026 15:54:16 -0500 Subject: [PATCH 4/8] fixing 'tests' issue and changelog breaking --- changelogs/fragments/s3_cors-improvements.yml | 3 --- 1 file changed, 3 deletions(-) diff --git a/changelogs/fragments/s3_cors-improvements.yml b/changelogs/fragments/s3_cors-improvements.yml index 6aef0ee3a55..4d4dd8b9343 100644 --- a/changelogs/fragments/s3_cors-improvements.yml +++ b/changelogs/fragments/s3_cors-improvements.yml @@ -3,6 +3,3 @@ bugfixes: - s3_cors - Added support for check_mode operation (https://github.com/ansible-collections/community.aws/pull/2471). - s3_cors - Fixed idempotency issue in ``state=absent`` where the module always reported changes even when no CORS configuration existed (https://github.com/ansible-collections/community.aws/pull/2471). - s3_cors - Fixed CORS rule comparison logic by replacing IAM policy comparison with CORS-specific normalization to ensure accurate change detection (https://github.com/ansible-collections/community.aws/pull/2471). - -tests: - - s3_cors - Added comprehensive integration tests covering create, update, delete operations with check_mode and idempotency validation (https://github.com/ansible-collections/community.aws/pull/2471). From a734d8a7c3999651724bacf23c4bb43edf96b510 Mon Sep 17 00:00:00 2001 From: romi1495 Date: Tue, 14 Jul 2026 15:13:00 -0500 Subject: [PATCH 5/8] s3_cors: address review feedback - sort key, unit tests, docs, error handling - Replace string-based sorting with tuple-based sorting in normalize_cors_rules() per reviewer suggestion for robustness and performance - Add type hints and docstring to normalize_cors_rules(), add docstrings to all functions - Add unit tests for normalize_cors_rules() following s3_lifecycle test pattern - Fix RETURN block: name and rules returned only when state=present - Add suboptions documentation for the rules parameter - Add multi-rule example to EXAMPLES block - Improve error handling in create_or_update_bucket_cors() to specifically catch NoSuchCORSConfiguration instead of bare ClientError Co-Authored-By: Claude Opus 4.6 --- plugins/modules/s3_cors.py | 89 ++++++++++++++--- .../s3_cors/test_normalize_cors_rules.py | 95 +++++++++++++++++++ 2 files changed, 173 insertions(+), 11 deletions(-) create mode 100644 tests/unit/plugins/modules/s3_cors/test_normalize_cors_rules.py diff --git a/plugins/modules/s3_cors.py b/plugins/modules/s3_cors.py index f3ccf5a3d76..e69df419186 100644 --- a/plugins/modules/s3_cors.py +++ b/plugins/modules/s3_cors.py @@ -23,9 +23,37 @@ type: str rules: description: - - Cors rules to put on the S3 bucket. + - CORS rules to apply to the S3 bucket. type: list elements: dict + suboptions: + allowed_origins: + description: + - One or more origins you want customers to be able to access the bucket from. + type: list + elements: str + required: true + allowed_methods: + description: + - HTTP methods that are allowed from the origin specified in I(allowed_origins). + type: list + elements: str + required: true + choices: ['GET', 'PUT', 'HEAD', 'POST', 'DELETE'] + allowed_headers: + description: + - Headers that are specified in the C(Access-Control-Request-Headers) header. + type: list + elements: str + expose_headers: + description: + - One or more headers in the response that you want customers to be able to access from their applications. + type: list + elements: str + max_age_seconds: + description: + - Time in seconds that the browser should cache the preflight response for the specified resource. + type: int state: description: - Create or remove cors on the S3 bucket. @@ -62,6 +90,26 @@ - community.aws.s3_cors: name: mys3bucket state: absent + +# Create CORS rules with multiple origins and methods +- community.aws.s3_cors: + name: mys3bucket + state: present + rules: + - allowed_origins: + - http://www.example.com + - http://www.example.org + allowed_methods: + - GET + - PUT + - POST + allowed_headers: + - '*' + max_age_seconds: 3600 + - allowed_origins: + - '*' + allowed_methods: + - GET """ RETURN = r""" @@ -71,13 +119,13 @@ type: bool sample: true name: - description: name of bucket - returned: always + description: Name of the S3 bucket. + returned: when I(state=present) type: str sample: 'bucket-name' rules: - description: list of current rules - returned: always + description: CORS rules applied to the bucket. + returned: when I(state=present) type: list sample: [ { @@ -108,8 +156,20 @@ from ansible_collections.community.aws.plugins.module_utils.modules import AnsibleCommunityAWSModule as AnsibleAWSModule -def normalize_cors_rules(rules): - """Normalize CORS rules for comparison by sorting lists within each rule.""" +def normalize_cors_rules(rules: list[dict] | None) -> list[dict]: + """Normalize CORS rules for deterministic comparison. + + Sorts list values within each rule (e.g., AllowedOrigins, AllowedMethods) and + sorts the rules themselves by a stable tuple-based key so that two equivalent + rule sets always compare equal regardless of ordering. + + Args: + rules: A list of CORS rule dicts as returned by boto3 or passed by the user. + May be None or empty. + + Returns: + A new list of normalized rule dicts, sorted deterministically. + """ if not rules: return [] @@ -117,26 +177,31 @@ def normalize_cors_rules(rules): for rule in rules: norm_rule = {} for key, value in rule.items(): - # Sort list values for consistent comparison if isinstance(value, list): norm_rule[key] = sorted(value) else: norm_rule[key] = value normalized.append(norm_rule) - # Sort rules by a consistent key (e.g., allowed_origins as a string representation) - return sorted(normalized, key=lambda x: str(sorted(x.items()))) + def sort_key(rule): + """Create a hashable, sortable key from a rule dict.""" + return tuple(sorted((k, tuple(v) if isinstance(v, list) else v) for k, v in rule.items())) + + return sorted(normalized, key=sort_key) def create_or_update_bucket_cors(connection, module): + """Create or update CORS configuration on an S3 bucket.""" name = module.params.get("name") rules = module.params.get("rules", []) changed = False try: current_camel_rules = connection.get_bucket_cors(Bucket=name)["CORSRules"] - except ClientError: + except is_boto3_error_code("NoSuchCORSConfiguration"): current_camel_rules = [] + except (BotoCoreError, ClientError) as e: # pylint: disable=duplicate-except + module.fail_json_aws(e, msg=f"Unable to get CORS configuration for bucket {name}") new_camel_rules = snake_dict_to_camel_dict(rules, capitalize_first=True) @@ -154,6 +219,7 @@ def create_or_update_bucket_cors(connection, module): def destroy_bucket_cors(connection, module): + """Remove CORS configuration from an S3 bucket.""" name = module.params.get("name") changed = False @@ -180,6 +246,7 @@ def destroy_bucket_cors(connection, module): def main(): + """Entry point for the s3_cors Ansible module.""" argument_spec = dict( name=dict(required=True, type="str"), rules=dict(type="list", elements="dict"), diff --git a/tests/unit/plugins/modules/s3_cors/test_normalize_cors_rules.py b/tests/unit/plugins/modules/s3_cors/test_normalize_cors_rules.py new file mode 100644 index 00000000000..caeec97c2e1 --- /dev/null +++ b/tests/unit/plugins/modules/s3_cors/test_normalize_cors_rules.py @@ -0,0 +1,95 @@ +# -*- coding: utf-8 -*- + +# Copyright: (c) 2026, Ansible Project +# GNU General Public License v3.0+ (see COPYING or https://www.gnu.org/licenses/gpl-3.0.txt) + +import pytest + +from ansible_collections.community.aws.plugins.modules.s3_cors import normalize_cors_rules + + +@pytest.mark.parametrize( + "rules,expected", + [ + [None, []], + [[], []], + ], +) +def test_normalize_cors_rules_empty(rules, expected): + assert normalize_cors_rules(rules) == expected + + +@pytest.mark.parametrize( + "rules,expected", + [ + # List values within a rule should be sorted + [ + [{"AllowedMethods": ["POST", "GET"], "AllowedOrigins": ["http://b.com", "http://a.com"]}], + [{"AllowedMethods": ["GET", "POST"], "AllowedOrigins": ["http://a.com", "http://b.com"]}], + ], + # Non-list values (like MaxAgeSeconds) should pass through unchanged + [ + [{"AllowedMethods": ["GET"], "AllowedOrigins": ["*"], "MaxAgeSeconds": 3600}], + [{"AllowedMethods": ["GET"], "AllowedOrigins": ["*"], "MaxAgeSeconds": 3600}], + ], + # ExposeHeaders should also be sorted + [ + [{"AllowedMethods": ["GET"], "AllowedOrigins": ["*"], "ExposeHeaders": ["x-amz-request-id", "x-amz-id-2"]}], + [{"AllowedMethods": ["GET"], "AllowedOrigins": ["*"], "ExposeHeaders": ["x-amz-id-2", "x-amz-request-id"]}], + ], + ], +) +def test_normalize_cors_rules_single_rule(rules, expected): + assert normalize_cors_rules(rules) == expected + + +@pytest.mark.parametrize( + "rules_a,rules_b", + [ + # Same rules in different order should normalize identically + [ + [ + {"AllowedMethods": ["GET"], "AllowedOrigins": ["http://b.com"]}, + {"AllowedMethods": ["POST"], "AllowedOrigins": ["http://a.com"]}, + ], + [ + {"AllowedMethods": ["POST"], "AllowedOrigins": ["http://a.com"]}, + {"AllowedMethods": ["GET"], "AllowedOrigins": ["http://b.com"]}, + ], + ], + ], +) +def test_normalize_cors_rules_deterministic_sort(rules_a, rules_b): + assert normalize_cors_rules(rules_a) == normalize_cors_rules(rules_b) + + +@pytest.mark.parametrize( + "rules_a,rules_b", + [ + # Dict key ordering should not matter + [ + [{"AllowedOrigins": ["*"], "AllowedMethods": ["GET"]}], + [{"AllowedMethods": ["GET"], "AllowedOrigins": ["*"]}], + ], + # Multiple rules with different key orderings and list orderings + [ + [ + {"AllowedOrigins": ["*"], "AllowedMethods": ["GET"], "MaxAgeSeconds": 100}, + {"AllowedMethods": ["POST", "PUT"], "AllowedOrigins": ["http://example.com"]}, + ], + [ + {"AllowedOrigins": ["http://example.com"], "AllowedMethods": ["PUT", "POST"]}, + {"MaxAgeSeconds": 100, "AllowedMethods": ["GET"], "AllowedOrigins": ["*"]}, + ], + ], + ], +) +def test_normalize_cors_rules_key_order_independent(rules_a, rules_b): + assert normalize_cors_rules(rules_a) == normalize_cors_rules(rules_b) + + +def test_normalize_cors_rules_max_age_seconds_int(): + rules = [{"AllowedMethods": ["GET"], "AllowedOrigins": ["*"], "MaxAgeSeconds": 30000}] + result = normalize_cors_rules(rules) + assert result[0]["MaxAgeSeconds"] == 30000 + assert isinstance(result[0]["MaxAgeSeconds"], int) From cf0886f05fc1d150d90692a8706fbd127a36ff62 Mon Sep 17 00:00:00 2001 From: romi1495 Date: Fri, 17 Jul 2026 10:54:53 -0500 Subject: [PATCH 6/8] fix(s3_cors): remove type hint syntax incompatible with older Python The `list[dict] | None` union syntax requires Python 3.10+ and fails pylint sanity checks in ansible-test. Type information is documented in the docstring instead. Co-Authored-By: Claude Opus 4.6 --- plugins/modules/s3_cors.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugins/modules/s3_cors.py b/plugins/modules/s3_cors.py index e69df419186..f59b71adc17 100644 --- a/plugins/modules/s3_cors.py +++ b/plugins/modules/s3_cors.py @@ -156,7 +156,7 @@ from ansible_collections.community.aws.plugins.module_utils.modules import AnsibleCommunityAWSModule as AnsibleAWSModule -def normalize_cors_rules(rules: list[dict] | None) -> list[dict]: +def normalize_cors_rules(rules): """Normalize CORS rules for deterministic comparison. Sorts list values within each rule (e.g., AllowedOrigins, AllowedMethods) and From 3df70ed6d7403cee59f6901be760596a6ced7255 Mon Sep 17 00:00:00 2001 From: romi1495 Date: Mon, 20 Jul 2026 11:51:08 -0500 Subject: [PATCH 7/8] s3_cors: address review - use FQCN, fix docs markup, reclassify changelog - Use fully qualified collection names in integration tests (community.aws.s3_cors, amazon.aws.s3_bucket) - Fix RETURN block to use O() markup instead of I() for option references - Move check_mode support from bugfixes to minor_changes in changelog Co-Authored-By: Claude Opus 4.6 --- changelogs/fragments/s3_cors-improvements.yml | 3 +- plugins/modules/s3_cors.py | 4 +- .../targets/s3_cors/tasks/main.yml | 46 +++++++++---------- 3 files changed, 27 insertions(+), 26 deletions(-) diff --git a/changelogs/fragments/s3_cors-improvements.yml b/changelogs/fragments/s3_cors-improvements.yml index 4d4dd8b9343..a9693d779f3 100644 --- a/changelogs/fragments/s3_cors-improvements.yml +++ b/changelogs/fragments/s3_cors-improvements.yml @@ -1,5 +1,6 @@ --- -bugfixes: +minor_changes: - s3_cors - Added support for check_mode operation (https://github.com/ansible-collections/community.aws/pull/2471). +bugfixes: - s3_cors - Fixed idempotency issue in ``state=absent`` where the module always reported changes even when no CORS configuration existed (https://github.com/ansible-collections/community.aws/pull/2471). - s3_cors - Fixed CORS rule comparison logic by replacing IAM policy comparison with CORS-specific normalization to ensure accurate change detection (https://github.com/ansible-collections/community.aws/pull/2471). diff --git a/plugins/modules/s3_cors.py b/plugins/modules/s3_cors.py index f59b71adc17..34c7cfef498 100644 --- a/plugins/modules/s3_cors.py +++ b/plugins/modules/s3_cors.py @@ -120,12 +120,12 @@ sample: true name: description: Name of the S3 bucket. - returned: when I(state=present) + returned: when O(state=present) type: str sample: 'bucket-name' rules: description: CORS rules applied to the bucket. - returned: when I(state=present) + returned: when O(state=present) type: list sample: [ { diff --git a/tests/integration/targets/s3_cors/tasks/main.yml b/tests/integration/targets/s3_cors/tasks/main.yml index ac403169408..4c01380f9b4 100644 --- a/tests/integration/targets/s3_cors/tasks/main.yml +++ b/tests/integration/targets/s3_cors/tasks/main.yml @@ -21,7 +21,7 @@ # ============================================================ - name: Create simple s3_bucket for CORS testing - s3_bucket: + amazon.aws.s3_bucket: state: present name: '{{ test_bucket }}' register: output @@ -37,7 +37,7 @@ # ============================================================ - name: Create CORS rules (check_mode) - s3_cors: + community.aws.s3_cors: state: present name: '{{ test_bucket }}' rules: @@ -61,7 +61,7 @@ - result is changed - name: Create CORS rules - s3_cors: + community.aws.s3_cors: state: present name: '{{ test_bucket }}' rules: @@ -88,7 +88,7 @@ - result.rules[0].allowed_methods == ['GET', 'POST'] - name: Create CORS rules idempotency (check_mode) - s3_cors: + community.aws.s3_cors: state: present name: '{{ test_bucket }}' rules: @@ -112,7 +112,7 @@ - result is not changed - name: Create CORS rules idempotency - s3_cors: + community.aws.s3_cors: state: present name: '{{ test_bucket }}' rules: @@ -139,7 +139,7 @@ # ============================================================ - name: Update CORS rules - change max_age_seconds (check_mode) - s3_cors: + community.aws.s3_cors: state: present name: '{{ test_bucket }}' rules: @@ -163,7 +163,7 @@ - result is changed - name: Update CORS rules - change max_age_seconds - s3_cors: + community.aws.s3_cors: state: present name: '{{ test_bucket }}' rules: @@ -187,7 +187,7 @@ - result.rules[0].max_age_seconds == 60000 - name: Update CORS rules idempotency (check_mode) - s3_cors: + community.aws.s3_cors: state: present name: '{{ test_bucket }}' rules: @@ -211,7 +211,7 @@ - result is not changed - name: Update CORS rules idempotency - s3_cors: + community.aws.s3_cors: state: present name: '{{ test_bucket }}' rules: @@ -238,7 +238,7 @@ # ============================================================ - name: Add multiple CORS rules (check_mode) - s3_cors: + community.aws.s3_cors: state: present name: '{{ test_bucket }}' rules: @@ -266,7 +266,7 @@ - result is changed - name: Add multiple CORS rules - s3_cors: + community.aws.s3_cors: state: present name: '{{ test_bucket }}' rules: @@ -294,7 +294,7 @@ - result.rules | length == 2 - name: Add multiple CORS rules idempotency (check_mode) - s3_cors: + community.aws.s3_cors: state: present name: '{{ test_bucket }}' rules: @@ -322,7 +322,7 @@ - result is not changed - name: Add multiple CORS rules idempotency - s3_cors: + community.aws.s3_cors: state: present name: '{{ test_bucket }}' rules: @@ -353,7 +353,7 @@ # ============================================================ - name: Set CORS with wildcard origin (check_mode) - s3_cors: + community.aws.s3_cors: state: present name: '{{ test_bucket }}' rules: @@ -373,7 +373,7 @@ - result is changed - name: Set CORS with wildcard origin - s3_cors: + community.aws.s3_cors: state: present name: '{{ test_bucket }}' rules: @@ -394,7 +394,7 @@ - result.rules[0].allowed_origins == ['*'] - name: Set CORS with wildcard origin idempotency (check_mode) - s3_cors: + community.aws.s3_cors: state: present name: '{{ test_bucket }}' rules: @@ -414,7 +414,7 @@ - result is not changed - name: Set CORS with wildcard origin idempotency - s3_cors: + community.aws.s3_cors: state: present name: '{{ test_bucket }}' rules: @@ -437,7 +437,7 @@ # ============================================================ - name: Remove CORS rules (check_mode) - s3_cors: + community.aws.s3_cors: state: absent name: '{{ test_bucket }}' register: result @@ -449,7 +449,7 @@ - result is changed - name: Remove CORS rules - s3_cors: + community.aws.s3_cors: state: absent name: '{{ test_bucket }}' register: result @@ -460,7 +460,7 @@ - result is changed - name: Remove CORS rules idempotency (check_mode) - s3_cors: + community.aws.s3_cors: state: absent name: '{{ test_bucket }}' register: result @@ -472,7 +472,7 @@ - result is not changed - name: Remove CORS rules idempotency - s3_cors: + community.aws.s3_cors: state: absent name: '{{ test_bucket }}' register: result @@ -488,13 +488,13 @@ always: - name: Remove CORS configuration - s3_cors: + community.aws.s3_cors: name: '{{ test_bucket }}' state: absent ignore_errors: true - name: Delete test bucket - s3_bucket: + amazon.aws.s3_bucket: name: '{{ test_bucket }}' state: absent force: true From b814e32959721775f5f6946b21e677c31c17f586 Mon Sep 17 00:00:00 2001 From: romi1495 Date: Tue, 21 Jul 2026 11:27:17 -0500 Subject: [PATCH 8/8] =?UTF-8?q?s3=5Fcors:=20clean=20up=20unit=20tests=20?= =?UTF-8?q?=E2=80=94=20remove=20redundancy,=20improve=20naming?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Rename tests to describe behavior, not structure - Remove redundant ExposeHeaders and MaxAgeSeconds test cases - Remove misleading key_order_independent test (Python dicts are already order-independent for comparison) - Use reserved example domains (a.example.com) per RFC 2606 Co-Authored-By: Claude Opus 4.6 --- .../s3_cors/test_normalize_cors_rules.py | 65 ++++++------------- 1 file changed, 20 insertions(+), 45 deletions(-) diff --git a/tests/unit/plugins/modules/s3_cors/test_normalize_cors_rules.py b/tests/unit/plugins/modules/s3_cors/test_normalize_cors_rules.py index caeec97c2e1..ed32ac1f6ca 100644 --- a/tests/unit/plugins/modules/s3_cors/test_normalize_cors_rules.py +++ b/tests/unit/plugins/modules/s3_cors/test_normalize_cors_rules.py @@ -22,74 +22,49 @@ def test_normalize_cors_rules_empty(rules, expected): @pytest.mark.parametrize( "rules,expected", [ - # List values within a rule should be sorted [ - [{"AllowedMethods": ["POST", "GET"], "AllowedOrigins": ["http://b.com", "http://a.com"]}], - [{"AllowedMethods": ["GET", "POST"], "AllowedOrigins": ["http://a.com", "http://b.com"]}], - ], - # Non-list values (like MaxAgeSeconds) should pass through unchanged - [ - [{"AllowedMethods": ["GET"], "AllowedOrigins": ["*"], "MaxAgeSeconds": 3600}], - [{"AllowedMethods": ["GET"], "AllowedOrigins": ["*"], "MaxAgeSeconds": 3600}], - ], - # ExposeHeaders should also be sorted - [ - [{"AllowedMethods": ["GET"], "AllowedOrigins": ["*"], "ExposeHeaders": ["x-amz-request-id", "x-amz-id-2"]}], - [{"AllowedMethods": ["GET"], "AllowedOrigins": ["*"], "ExposeHeaders": ["x-amz-id-2", "x-amz-request-id"]}], + [{"AllowedMethods": ["POST", "GET"], "AllowedOrigins": ["http://b.example.com", "http://a.example.com"]}], + [{"AllowedMethods": ["GET", "POST"], "AllowedOrigins": ["http://a.example.com", "http://b.example.com"]}], ], ], ) -def test_normalize_cors_rules_single_rule(rules, expected): +def test_normalize_cors_rules_sorts_list_values(rules, expected): assert normalize_cors_rules(rules) == expected +def test_normalize_cors_rules_preserves_non_list_values(): + rules = [{"AllowedMethods": ["GET"], "AllowedOrigins": ["*"], "MaxAgeSeconds": 30000}] + result = normalize_cors_rules(rules) + assert result[0]["MaxAgeSeconds"] == 30000 + assert isinstance(result[0]["MaxAgeSeconds"], int) + + @pytest.mark.parametrize( "rules_a,rules_b", [ - # Same rules in different order should normalize identically + # Same rules in different list positions should normalize identically [ [ - {"AllowedMethods": ["GET"], "AllowedOrigins": ["http://b.com"]}, - {"AllowedMethods": ["POST"], "AllowedOrigins": ["http://a.com"]}, + {"AllowedMethods": ["GET"], "AllowedOrigins": ["http://b.example.com"]}, + {"AllowedMethods": ["POST"], "AllowedOrigins": ["http://a.example.com"]}, ], [ - {"AllowedMethods": ["POST"], "AllowedOrigins": ["http://a.com"]}, - {"AllowedMethods": ["GET"], "AllowedOrigins": ["http://b.com"]}, + {"AllowedMethods": ["POST"], "AllowedOrigins": ["http://a.example.com"]}, + {"AllowedMethods": ["GET"], "AllowedOrigins": ["http://b.example.com"]}, ], ], - ], -) -def test_normalize_cors_rules_deterministic_sort(rules_a, rules_b): - assert normalize_cors_rules(rules_a) == normalize_cors_rules(rules_b) - - -@pytest.mark.parametrize( - "rules_a,rules_b", - [ - # Dict key ordering should not matter - [ - [{"AllowedOrigins": ["*"], "AllowedMethods": ["GET"]}], - [{"AllowedMethods": ["GET"], "AllowedOrigins": ["*"]}], - ], - # Multiple rules with different key orderings and list orderings + # Different rule positions combined with unsorted list values [ [ {"AllowedOrigins": ["*"], "AllowedMethods": ["GET"], "MaxAgeSeconds": 100}, - {"AllowedMethods": ["POST", "PUT"], "AllowedOrigins": ["http://example.com"]}, + {"AllowedMethods": ["POST", "PUT"], "AllowedOrigins": ["http://a.example.com"]}, ], [ - {"AllowedOrigins": ["http://example.com"], "AllowedMethods": ["PUT", "POST"]}, - {"MaxAgeSeconds": 100, "AllowedMethods": ["GET"], "AllowedOrigins": ["*"]}, + {"AllowedOrigins": ["http://a.example.com"], "AllowedMethods": ["PUT", "POST"]}, + {"AllowedMethods": ["GET"], "AllowedOrigins": ["*"], "MaxAgeSeconds": 100}, ], ], ], ) -def test_normalize_cors_rules_key_order_independent(rules_a, rules_b): +def test_normalize_cors_rules_deterministic_order(rules_a, rules_b): assert normalize_cors_rules(rules_a) == normalize_cors_rules(rules_b) - - -def test_normalize_cors_rules_max_age_seconds_int(): - rules = [{"AllowedMethods": ["GET"], "AllowedOrigins": ["*"], "MaxAgeSeconds": 30000}] - result = normalize_cors_rules(rules) - assert result[0]["MaxAgeSeconds"] == 30000 - assert isinstance(result[0]["MaxAgeSeconds"], int)