Create S3_cors tests and fix check_mode, other bugs - #2471
Conversation
…ation 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.
|
Build succeeded. ✔️ ansible-galaxy-importer SUCCESS in 6m 05s (non-voting) |
|
Build succeeded. ✔️ ansible-galaxy-importer SUCCESS in 4m 15s (non-voting) |
alinabuzachis
left a comment
There was a problem hiding this comment.
@Romi1495 Great work! I only have a few minor suggestions. One more thing, could you verify that the module documentation is accurate? Please also check the RETURN block and the examples to make sure they're correct and up to date.
| 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()))) |
There was a problem hiding this comment.
Tuple-based sorting is more robust and handles edge cases better than string representation. It's also more performant.
| 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()))) | |
| # Sort rules by a deterministic key using tuples instead of string representation | |
| 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) |
| from ansible_collections.community.aws.plugins.module_utils.modules import AnsibleCommunityAWSModule as AnsibleAWSModule | ||
|
|
||
|
|
||
| def normalize_cors_rules(rules): |
There was a problem hiding this comment.
I suggest adding unit tests for normalize_cors_rules(). Also, can you please add type hints and docstrings consistently?
…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 <noreply@anthropic.com>
Docs Build 📝Thank you for contribution!✨ The docsite for this PR is available for download as an artifact from this run: You can compare to the docs for the File changes:
Click to see the diff comparison.NOTE: only file modifications are shown here. New and deleted files are excluded. diff --git a/home/runner/work/community.aws/community.aws/docsbuild/base/collections/community/aws/s3_cors_module.html b/home/runner/work/community.aws/community.aws/docsbuild/head/collections/community/aws/s3_cors_module.html
index f652bcd..0719b81 100644
--- a/home/runner/work/community.aws/community.aws/docsbuild/base/collections/community/aws/s3_cors_module.html
+++ b/home/runner/work/community.aws/community.aws/docsbuild/head/collections/community/aws/s3_cors_module.html
@@ -267,10 +267,53 @@ see <a class="reference internal" href="#ansible-collections-community-aws-s3-co
<div class="ansibleOptionAnchor" id="parameter-rules"></div><p class="ansible-option-title" id="ansible-collections-community-aws-s3-cors-module-parameter-rules"><strong>rules</strong></p>
<a class="ansibleOptionLink" href="#parameter-rules" title="Permalink to this option"></a><p class="ansible-option-type-line"><span class="ansible-option-type">list</span> / <span class="ansible-option-elements">elements=dictionary</span></p>
</div></td>
-<td><div class="ansible-option-cell"><p>Cors rules to put on the S3 bucket.</p>
+<td><div class="ansible-option-cell"><p>CORS rules to apply to the S3 bucket.</p>
</div></td>
</tr>
-<tr class="row-odd"><td><div class="ansible-option-cell">
+<tr class="row-odd"><td><div class="ansible-option-indent"></div><div class="ansible-option-cell">
+<div class="ansibleOptionAnchor" id="parameter-rules/allowed_headers"></div><p class="ansible-option-title" id="ansible-collections-community-aws-s3-cors-module-parameter-rules-allowed-headers"><strong>allowed_headers</strong></p>
+<a class="ansibleOptionLink" href="#parameter-rules/allowed_headers" title="Permalink to this option"></a><p class="ansible-option-type-line"><span class="ansible-option-type">list</span> / <span class="ansible-option-elements">elements=string</span></p>
+</div></td>
+<td><div class="ansible-option-indent-desc"></div><div class="ansible-option-cell"><p>Headers that are specified in the <code class="docutils literal notranslate"><span class="pre">Access-Control-Request-Headers</span></code> header.</p>
+</div></td>
+</tr>
+<tr class="row-even"><td><div class="ansible-option-indent"></div><div class="ansible-option-cell">
+<div class="ansibleOptionAnchor" id="parameter-rules/allowed_methods"></div><p class="ansible-option-title" id="ansible-collections-community-aws-s3-cors-module-parameter-rules-allowed-methods"><strong>allowed_methods</strong></p>
+<a class="ansibleOptionLink" href="#parameter-rules/allowed_methods" title="Permalink to this option"></a><p class="ansible-option-type-line"><span class="ansible-option-type">list</span> / <span class="ansible-option-elements">elements=string</span> / <span class="ansible-option-required">required</span></p>
+</div></td>
+<td><div class="ansible-option-indent-desc"></div><div class="ansible-option-cell"><p>HTTP methods that are allowed from the origin specified in <em>allowed_origins</em>.</p>
+<p class="ansible-option-line"><strong class="ansible-option-choices">Choices:</strong></p>
+<ul class="simple">
+<li><p><code class="ansible-option-choices-entry docutils literal notranslate"><span class="pre">"GET"</span></code></p></li>
+<li><p><code class="ansible-option-choices-entry docutils literal notranslate"><span class="pre">"PUT"</span></code></p></li>
+<li><p><code class="ansible-option-choices-entry docutils literal notranslate"><span class="pre">"HEAD"</span></code></p></li>
+<li><p><code class="ansible-option-choices-entry docutils literal notranslate"><span class="pre">"POST"</span></code></p></li>
+<li><p><code class="ansible-option-choices-entry docutils literal notranslate"><span class="pre">"DELETE"</span></code></p></li>
+</ul>
+</div></td>
+</tr>
+<tr class="row-odd"><td><div class="ansible-option-indent"></div><div class="ansible-option-cell">
+<div class="ansibleOptionAnchor" id="parameter-rules/allowed_origins"></div><p class="ansible-option-title" id="ansible-collections-community-aws-s3-cors-module-parameter-rules-allowed-origins"><strong>allowed_origins</strong></p>
+<a class="ansibleOptionLink" href="#parameter-rules/allowed_origins" title="Permalink to this option"></a><p class="ansible-option-type-line"><span class="ansible-option-type">list</span> / <span class="ansible-option-elements">elements=string</span> / <span class="ansible-option-required">required</span></p>
+</div></td>
+<td><div class="ansible-option-indent-desc"></div><div class="ansible-option-cell"><p>One or more origins you want customers to be able to access the bucket from.</p>
+</div></td>
+</tr>
+<tr class="row-even"><td><div class="ansible-option-indent"></div><div class="ansible-option-cell">
+<div class="ansibleOptionAnchor" id="parameter-rules/expose_headers"></div><p class="ansible-option-title" id="ansible-collections-community-aws-s3-cors-module-parameter-rules-expose-headers"><strong>expose_headers</strong></p>
+<a class="ansibleOptionLink" href="#parameter-rules/expose_headers" title="Permalink to this option"></a><p class="ansible-option-type-line"><span class="ansible-option-type">list</span> / <span class="ansible-option-elements">elements=string</span></p>
+</div></td>
+<td><div class="ansible-option-indent-desc"></div><div class="ansible-option-cell"><p>One or more headers in the response that you want customers to be able to access from their applications.</p>
+</div></td>
+</tr>
+<tr class="row-odd"><td><div class="ansible-option-indent"></div><div class="ansible-option-cell">
+<div class="ansibleOptionAnchor" id="parameter-rules/max_age_seconds"></div><p class="ansible-option-title" id="ansible-collections-community-aws-s3-cors-module-parameter-rules-max-age-seconds"><strong>max_age_seconds</strong></p>
+<a class="ansibleOptionLink" href="#parameter-rules/max_age_seconds" title="Permalink to this option"></a><p class="ansible-option-type-line"><span class="ansible-option-type">integer</span></p>
+</div></td>
+<td><div class="ansible-option-indent-desc"></div><div class="ansible-option-cell"><p>Time in seconds that the browser should cache the preflight response for the specified resource.</p>
+</div></td>
+</tr>
+<tr class="row-even"><td><div class="ansible-option-cell">
<div class="ansibleOptionAnchor" id="parameter-secret_key"></div>
<div class="ansibleOptionAnchor" id="parameter-aws_secret_access_key"></div>
<div class="ansibleOptionAnchor" id="parameter-aws_secret_key"></div><p class="ansible-option-title" id="ansible-collections-community-aws-s3-cors-module-parameter-secret-key"><span id="ansible-collections-community-aws-s3-cors-module-parameter-aws-secret-key"></span><span id="ansible-collections-community-aws-s3-cors-module-parameter-aws-secret-access-key"></span><strong>secret_key</strong></p>
@@ -284,7 +327,7 @@ see <a class="reference internal" href="#ansible-collections-community-aws-s3-co
<p>The <em>aws_secret_access_key</em> alias was added in release 5.1.0 for consistency with the AWS botocore SDK.</p>
</div></td>
</tr>
-<tr class="row-even"><td><div class="ansible-option-cell">
+<tr class="row-odd"><td><div class="ansible-option-cell">
<div class="ansibleOptionAnchor" id="parameter-session_token"></div>
<div class="ansibleOptionAnchor" id="parameter-aws_session_token"></div><p class="ansible-option-title" id="ansible-collections-community-aws-s3-cors-module-parameter-session-token"><span id="ansible-collections-community-aws-s3-cors-module-parameter-aws-session-token"></span><strong>session_token</strong></p>
<a class="ansibleOptionLink" href="#parameter-session_token" title="Permalink to this option"></a><p class="ansible-option-type-line"><span class="ansible-option-aliases">aliases: aws_session_token</span></p>
@@ -296,7 +339,7 @@ see <a class="reference internal" href="#ansible-collections-community-aws-s3-co
<p>The <em>session_token</em> and <em>profile</em> options are mutually exclusive.</p>
</div></td>
</tr>
-<tr class="row-odd"><td><div class="ansible-option-cell">
+<tr class="row-even"><td><div class="ansible-option-cell">
<div class="ansibleOptionAnchor" id="parameter-state"></div><p class="ansible-option-title" id="ansible-collections-community-aws-s3-cors-module-parameter-state"><strong>state</strong></p>
<a class="ansibleOptionLink" href="#parameter-state" title="Permalink to this option"></a><p class="ansible-option-type-line"><span class="ansible-option-type">string</span> / <span class="ansible-option-required">required</span></p>
</div></td>
@@ -308,7 +351,7 @@ see <a class="reference internal" href="#ansible-collections-community-aws-s3-co
</ul>
</div></td>
</tr>
-<tr class="row-even"><td><div class="ansible-option-cell">
+<tr class="row-odd"><td><div class="ansible-option-cell">
<div class="ansibleOptionAnchor" id="parameter-validate_certs"></div><p class="ansible-option-title" id="ansible-collections-community-aws-s3-cors-module-parameter-validate-certs"><strong>validate_certs</strong></p>
<a class="ansibleOptionLink" href="#parameter-validate_certs" title="Permalink to this option"></a><p class="ansible-option-type-line"><span class="ansible-option-type">boolean</span></p>
</div></td>
@@ -359,6 +402,26 @@ see <a class="reference internal" href="#ansible-collections-community-aws-s3-co
<span class="p p-Indicator">-</span><span class="w"> </span><span class="nt">community.aws.s3_cors</span><span class="p">:</span>
<span class="w"> </span><span class="nt">name</span><span class="p">:</span><span class="w"> </span><span class="l l-Scalar l-Scalar-Plain">mys3bucket</span>
<span class="w"> </span><span class="nt">state</span><span class="p">:</span><span class="w"> </span><span class="l l-Scalar l-Scalar-Plain">absent</span>
+
+<span class="c1"># Create CORS rules with multiple origins and methods</span>
+<span class="p p-Indicator">-</span><span class="w"> </span><span class="nt">community.aws.s3_cors</span><span class="p">:</span>
+<span class="w"> </span><span class="nt">name</span><span class="p">:</span><span class="w"> </span><span class="l l-Scalar l-Scalar-Plain">mys3bucket</span>
+<span class="w"> </span><span class="nt">state</span><span class="p">:</span><span class="w"> </span><span class="l l-Scalar l-Scalar-Plain">present</span>
+<span class="w"> </span><span class="nt">rules</span><span class="p">:</span>
+<span class="w"> </span><span class="p p-Indicator">-</span><span class="w"> </span><span class="nt">allowed_origins</span><span class="p">:</span>
+<span class="w"> </span><span class="p p-Indicator">-</span><span class="w"> </span><span class="l l-Scalar l-Scalar-Plain">http://www.example.com</span>
+<span class="w"> </span><span class="p p-Indicator">-</span><span class="w"> </span><span class="l l-Scalar l-Scalar-Plain">http://www.example.org</span>
+<span class="w"> </span><span class="nt">allowed_methods</span><span class="p">:</span>
+<span class="w"> </span><span class="p p-Indicator">-</span><span class="w"> </span><span class="l l-Scalar l-Scalar-Plain">GET</span>
+<span class="w"> </span><span class="p p-Indicator">-</span><span class="w"> </span><span class="l l-Scalar l-Scalar-Plain">PUT</span>
+<span class="w"> </span><span class="p p-Indicator">-</span><span class="w"> </span><span class="l l-Scalar l-Scalar-Plain">POST</span>
+<span class="w"> </span><span class="nt">allowed_headers</span><span class="p">:</span>
+<span class="w"> </span><span class="p p-Indicator">-</span><span class="w"> </span><span class="s">'*'</span>
+<span class="w"> </span><span class="nt">max_age_seconds</span><span class="p">:</span><span class="w"> </span><span class="l l-Scalar l-Scalar-Plain">3600</span>
+<span class="w"> </span><span class="p p-Indicator">-</span><span class="w"> </span><span class="nt">allowed_origins</span><span class="p">:</span>
+<span class="w"> </span><span class="p p-Indicator">-</span><span class="w"> </span><span class="s">'*'</span>
+<span class="w"> </span><span class="nt">allowed_methods</span><span class="p">:</span>
+<span class="w"> </span><span class="p p-Indicator">-</span><span class="w"> </span><span class="l l-Scalar l-Scalar-Plain">GET</span>
</pre></div>
</div>
</section>
@@ -385,8 +448,8 @@ see <a class="reference internal" href="#ansible-collections-community-aws-s3-co
<div class="ansibleOptionAnchor" id="return-name"></div><p class="ansible-option-title" id="ansible-collections-community-aws-s3-cors-module-return-name"><strong>name</strong></p>
<a class="ansibleOptionLink" href="#return-name" title="Permalink to this return value"></a><p class="ansible-option-type-line"><span class="ansible-option-type">string</span></p>
</div></td>
-<td><div class="ansible-option-cell"><p>name of bucket</p>
-<p class="ansible-option-line"><strong class="ansible-option-returned-bold">Returned:</strong> always</p>
+<td><div class="ansible-option-cell"><p>Name of the S3 bucket.</p>
+<p class="ansible-option-line"><strong class="ansible-option-returned-bold">Returned:</strong> when <code class="ansible-option-value docutils literal notranslate"><a class="reference internal" href="#ansible-collections-community-aws-s3-cors-module-parameter-state"><span class="std std-ref"><span class="pre">state=present</span></span></a></code></p>
<p class="ansible-option-line ansible-option-sample"><strong class="ansible-option-sample-bold">Sample:</strong> <code class="ansible-option-sample docutils literal notranslate"><span class="pre">"bucket-name"</span></code></p>
</div></td>
</tr>
@@ -394,8 +457,8 @@ see <a class="reference internal" href="#ansible-collections-community-aws-s3-co
<div class="ansibleOptionAnchor" id="return-rules"></div><p class="ansible-option-title" id="ansible-collections-community-aws-s3-cors-module-return-rules"><strong>rules</strong></p>
<a class="ansibleOptionLink" href="#return-rules" title="Permalink to this return value"></a><p class="ansible-option-type-line"><span class="ansible-option-type">list</span> / <span class="ansible-option-elements">elements=string</span></p>
</div></td>
-<td><div class="ansible-option-cell"><p>list of current rules</p>
-<p class="ansible-option-line"><strong class="ansible-option-returned-bold">Returned:</strong> always</p>
+<td><div class="ansible-option-cell"><p>CORS rules applied to the bucket.</p>
+<p class="ansible-option-line"><strong class="ansible-option-returned-bold">Returned:</strong> when <code class="ansible-option-value docutils literal notranslate"><a class="reference internal" href="#ansible-collections-community-aws-s3-cors-module-parameter-state"><span class="std std-ref"><span class="pre">state=present</span></span></a></code></p>
<p class="ansible-option-line ansible-option-sample"><strong class="ansible-option-sample-bold">Sample:</strong> <code class="ansible-option-sample docutils literal notranslate"><span class="pre">[{"allowed_headers":</span> <span class="pre">["Authorization"],</span> <span class="pre">"allowed_methods":</span> <span class="pre">["GET"],</span> <span class="pre">"allowed_origins":</span> <span class="pre">["*"],</span> <span class="pre">"max_age_seconds":</span> <span class="pre">30000}]</span></code></p>
</div></td>
</tr>
|
|
Build succeeded. ✔️ ansible-galaxy-importer SUCCESS in 3m 15s (non-voting) |
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 <noreply@anthropic.com>
|
Build succeeded. ✔️ ansible-galaxy-importer SUCCESS in 3m 37s (non-voting) |
| # Create CORS rules with multiple origins and methods | ||
| - community.aws.s3_cors: |
There was a problem hiding this comment.
| # Create CORS rules with multiple origins and methods | |
| - community.aws.s3_cors: | |
| - name: Create CORS rules with multiple origins and methods | |
| community.aws.s3_cors: |
| description: name of bucket | ||
| returned: always | ||
| description: Name of the S3 bucket. | ||
| returned: when I(state=present) |
There was a problem hiding this comment.
| returned: when I(state=present) | |
| returned: when O(state=present) |
| description: list of current rules | ||
| returned: always | ||
| description: CORS rules applied to the bucket. | ||
| returned: when I(state=present) |
There was a problem hiding this comment.
| returned: when I(state=present) | |
| returned: when O(state=present) |
| # ============================================================ | ||
|
|
||
| - name: Create simple s3_bucket for CORS testing | ||
| s3_bucket: |
There was a problem hiding this comment.
| s3_bucket: | |
| amazon.aws.s3_bucket: |
| - result is changed | ||
|
|
||
| - name: Remove CORS rules | ||
| s3_cors: |
There was a problem hiding this comment.
| s3_cors: | |
| community.aws.s3_cors: |
| - result is changed | ||
|
|
||
| - name: Remove CORS rules idempotency (check_mode) | ||
| s3_cors: |
There was a problem hiding this comment.
| s3_cors: | |
| community.aws.s3_cors: |
| - result is not changed | ||
|
|
||
| - name: Remove CORS rules idempotency | ||
| s3_cors: |
There was a problem hiding this comment.
| s3_cors: | |
| community.aws.s3_cors: |
|
|
||
| always: | ||
| - name: Remove CORS configuration | ||
| s3_cors: |
There was a problem hiding this comment.
| s3_cors: | |
| community.aws.s3_cors: |
| ignore_errors: true | ||
|
|
||
| - name: Delete test bucket | ||
| s3_bucket: |
There was a problem hiding this comment.
| s3_bucket: | |
| amazon.aws.s3_bucket: |
…elog - 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 <noreply@anthropic.com>
|
Build succeeded. ✔️ ansible-galaxy-importer SUCCESS in 4m 27s (non-voting) |
|
@Romi1495 would you add the missing permissions to https://github.com/mattclay/aws-terminator? Thank you. |
| # -*- coding: utf-8 -*- | ||
|
|
||
| # Copyright (c) 2017 Ansible Project | ||
| # GNU General Public License v3.0+ (see COPYING or https://www.gnu.org/licenses/gpl-3.0.txt) |
There was a problem hiding this comment.
| # GNU General Public License v3.0+ (see COPYING or https://www.gnu.org/licenses/gpl-3.0.txt) | |
| from __future__ import annotations |
Rather than removing the type hints, we can use the future import.
| ], | ||
| ], | ||
| ) | ||
| def test_normalize_cors_rules_key_order_independent(rules_a, rules_b): |
There was a problem hiding this comment.
I'm trying to understand what this test is verifying... The Dicts don't really have a sort order.
| [ | ||
| # List values within a rule should be sorted | ||
| [ | ||
| [{"AllowedMethods": ["POST", "GET"], "AllowedOrigins": ["http://b.com", "http://a.com"]}], |
There was a problem hiding this comment.
Rather than "a.com" I'd suggest "a.example.com"
While single letter .coms are unusual, technically someone could buy a.com or b.com
If you'd like a little variation, there's also ".test"
- 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 <noreply@anthropic.com>
|
Build succeeded. ✔️ ansible-galaxy-importer SUCCESS in 4m 40s (non-voting) |
SUMMARY
This PR adds comprehensive integration tests for the
s3_corsmodule and fixes three critical bugsdiscovered during test development.
The
s3_corsmodule had no integration tests and contained several bugs:--checkstate=absent- always reported changes even when no CORS configurationexisted
positive change detection
Bug Fixes:
supports_check_mode=Trueand proper check_mode handling in both create and delete operationsdestroy_bucket_cors()to check if CORS exists before deletion usingis_boto3_error_code("NoSuchCORSConfiguration")compare_policies()(designed for IAM policies) with customnormalize_cors_rules()function for accurate CORS rule comparison
Tests:
This PR improves module reliability in preparation for potential promotion to amazon.aws (ref: #2458).
Changelog fragment included:
changelogs/fragments/s3_cors-improvements.ymlISSUE TYPE
COMPONENT NAME
s3_cors
ADDITIONAL INFORMATION
Testing:
ansible-test integration s3_cors -v Result: ✅ All 45 tests passed Test Coverage: - 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 Files Changed: - plugins/modules/s3_cors.py - Bug fixes and check_mode support (+54 lines, -8 lines) - tests/integration/targets/s3_cors/ - New integration test suite (4 files, 501 lines) - changelogs/fragments/s3_cors-improvements.yml - Changelog fragment Related Issues: No currently open issues for s3_cors. This PR addresses the testing gap and improves module quality. ---Assisted-by: Claude Sonnet 4.5 (Anthropic) ---