diff --git a/changelogs/fragments/s3_cors-improvements.yml b/changelogs/fragments/s3_cors-improvements.yml new file mode 100644 index 00000000000..a9693d779f3 --- /dev/null +++ b/changelogs/fragments/s3_cors-improvements.yml @@ -0,0 +1,6 @@ +--- +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 c9cf2d0546a..34c7cfef498 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 O(state=present) type: str sample: 'bucket-name' rules: - description: list of current rules - returned: always + description: CORS rules applied to the bucket. + returned: when O(state=present) type: list sample: [ { @@ -103,27 +151,65 @@ 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 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 [] + + normalized = [] + for rule in rules: + norm_rule = {} + for key, value in rule.items(): + if isinstance(value, list): + norm_rule[key] = sorted(value) + else: + norm_rule[key] = value + normalized.append(norm_rule) + + 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) - # 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: @@ -133,26 +219,41 @@ 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 + # 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) 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"), 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..4c01380f9b4 --- /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 + amazon.aws.s3_bucket: + state: present + name: '{{ test_bucket }}' + register: output + + - name: Assert bucket was created properly + assert: + that: + - output is changed + - output.name == test_bucket + + # ============================================================ + # Test Create CORS rules (state = present) + # ============================================================ + + - name: Create CORS rules (check_mode) + community.aws.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 + community.aws.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) + community.aws.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 + community.aws.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) + community.aws.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 + community.aws.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) + community.aws.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 + community.aws.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) + community.aws.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 + community.aws.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) + community.aws.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 + community.aws.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) + community.aws.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 + community.aws.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) + community.aws.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 + community.aws.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) + community.aws.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 + community.aws.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) + community.aws.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 + community.aws.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 + community.aws.s3_cors: + name: '{{ test_bucket }}' + state: absent + ignore_errors: true + + - name: Delete test bucket + amazon.aws.s3_bucket: + name: '{{ test_bucket }}' + state: absent + force: true + ignore_errors: true 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..ed32ac1f6ca --- /dev/null +++ b/tests/unit/plugins/modules/s3_cors/test_normalize_cors_rules.py @@ -0,0 +1,70 @@ +# -*- 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", + [ + [ + [{"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_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 list positions should normalize identically + [ + [ + {"AllowedMethods": ["GET"], "AllowedOrigins": ["http://b.example.com"]}, + {"AllowedMethods": ["POST"], "AllowedOrigins": ["http://a.example.com"]}, + ], + [ + {"AllowedMethods": ["POST"], "AllowedOrigins": ["http://a.example.com"]}, + {"AllowedMethods": ["GET"], "AllowedOrigins": ["http://b.example.com"]}, + ], + ], + # Different rule positions combined with unsorted list values + [ + [ + {"AllowedOrigins": ["*"], "AllowedMethods": ["GET"], "MaxAgeSeconds": 100}, + {"AllowedMethods": ["POST", "PUT"], "AllowedOrigins": ["http://a.example.com"]}, + ], + [ + {"AllowedOrigins": ["http://a.example.com"], "AllowedMethods": ["PUT", "POST"]}, + {"AllowedMethods": ["GET"], "AllowedOrigins": ["*"], "MaxAgeSeconds": 100}, + ], + ], + ], +) +def test_normalize_cors_rules_deterministic_order(rules_a, rules_b): + assert normalize_cors_rules(rules_a) == normalize_cors_rules(rules_b)