Extended the wafv2_web_acl module with custom_response_bodies argument #721

stefanhorning · 2021-09-17T12:39:48Z

SUMMARY

Extended the wafv2_web_acl module to also take the custom_response_bodies argument, improved docs and extended tests

ISSUE TYPE

Feature Pull Request

COMPONENT NAME

wafv2_web_acl

ADDITIONAL INFORMATION

Also touched docs of aws_waf_web_acl to make it easier to find the WAF v2 modules as I had trouble finding that at first.

ansibullbot · 2021-09-17T12:46:34Z

cc @jillr @markuman @mmochan @s-hertel @tremble @wimnat
click here for bot help

plugins/modules/wafv2_web_acl.py

changelogs/fragments/721-wafv2_web_acl.yml

plugins/modules/wafv2_web_acl.py

tests/integration/targets/wafv2/tasks/create_webacl.yml

stefanhorning · 2021-09-17T15:14:13Z

I believe I addressed all issues now. Let's see if build goes through now. Once green this should be ready to be merged.

markuman · 2021-09-17T16:17:58Z

Sanity is failing

ERROR: Found 1 validate-modules issue(s) which need to be resolved:
ERROR: plugins/modules/wafv2_web_acl.py:0:0: return-syntax-error: RETURN.custom_response_bodies.returned: required key not provided @ data['custom_response_bodies']['returned']. Got None

stefanhorning · 2021-09-20T08:01:13Z

Fixed RETURN docs now.

However I also noticed other CI errors in integration tests:

# ...
botocore.exceptions.ParamValidationError: Parameter validation failed:
Unknown parameter in input: "CustomResponseBodies", must be one of: Name, Scope, DefaultAction, Description, Rules, VisibilityConfig, Tags
Missing required parameter in Rules[2]: "VisibilityConfig"
Unknown parameter in Rules[2].Action.Block: "CustomResponse", must be one of: 
Unknown parameter in Rules[2].Statement: "VisibilityConfig", must be one of: ByteMatchStatement, SqliMatchStatement, XssMatchStatement, SizeConstraintStatement, GeoMatchStatement, RuleGroupReferenceStatement, IPSetReferenceStatement, RegexPatternSetReferenceStatement, RateBasedStatement, AndStatement, OrStatement, NotStatement, ManagedRuleGroupStatement
fatal: [testhost]: FAILED! => {
    "boto3_version": "1.15.0",
    "botocore_version": "1.18.0",
    "changed": false,
    "invocation": {
#    ...

This probably happens because of old boto or s.th. But what is weird that it also fails on params that were already there before my changes, such as VisibilityConfig.

markuman · 2021-09-20T08:06:20Z

When newer botocore/boto3 versions are necessary, we can install them like https://github.com/mediafellows/community.aws/blob/wafv2_web_acl_improvements/tests/integration/targets/aws_msk_config/tasks/main.yml#L20
Can we determine somehow when custom_response_bodies was introduce to name the version in the documentation?

But what is weird that it also fails on params that were already there before my changes, such as VisibilityConfig.

Indeed, I have no idea atm.

tremble · 2021-09-20T08:15:21Z

@stefanhorning That may be a result of #644 landing and forcing the tests to run against the oldest version of botocore that we support, to highlight issues like this. Unfortunately if the code wasn't exercised before the tests wouldn't highlight the issue...

We actually have an even easier way to setup the boto3 pip installation (I still need to refactor community.aws)

https://github.com/ansible-collections/amazon.aws/blob/main/tests/integration/targets/ec2_vol/meta/main.yml
https://github.com/ansible-collections/amazon.aws/blob/main/tests/integration/targets/ec2_vol/tasks/main.yml#L387

stefanhorning · 2021-09-20T08:23:38Z

I will try to use the pip role then. I am unfortunately not yet sure which botocore version introduced this, just for the boto3 lib I know which version, it seems to be the 1.18.0. See https://boto3.amazonaws.com/v1/documentation/api/1.18.0/reference/services/wafv2.html#WAFV2.Client.create_web_acl

Should I also add a note to the module docs and/or a check for correct boto version in the code?

tremble · 2021-09-20T08:26:38Z

try botocore 1.20.40

That seems to be to 'blame' for the CustomResponseBodies definition:
git blame botocore/data/wafv2/2019-07-29/service-2.json

071e4df826 (aws-sdk-python-automation 2021-03-29 18:09:32 +0000 1520)     "CustomResponseBodies":{
071e4df826 (aws-sdk-python-automation 2021-03-29 18:09:32 +0000 1521)       "type":"map",
071e4df826 (aws-sdk-python-automation 2021-03-29 18:09:32 +0000 1522)       "key":{"shape":"EntityName"},
071e4df826 (aws-sdk-python-automation 2021-03-29 18:09:32 +0000 1523)       "value":{"shape":"CustomResponseBody"},
071e4df826 (aws-sdk-python-automation 2021-03-29 18:09:32 +0000 1524)       "min":1
071e4df826 (aws-sdk-python-automation 2021-03-29 18:09:32 +0000 1525)     },

stefanhorning · 2021-09-20T08:26:50Z

Ok, found out through github project that boto3 1.18.0 depends on botocore 1.21.0, see https://github.com/boto/boto3/blob/1.18.0/setup.cfg

So I will try with that

tremble · 2021-09-20T08:30:22Z

Once you determine the minimum requirements, please do update the docs and code to cleanly fail:

See for example:
https://github.com/ansible-collections/amazon.aws/blob/main/plugins/modules/ec2_vol.py#L719

    if module.params.get('throughput'):
        module.require_botocore_at_least('1.19.27', reason='to set the throughput for a volume')

tests/integration/targets/wafv2/meta/main.yml

stefanhorning · 2021-09-20T15:21:13Z

Addressed all comments now I hope. Let's see what CI thinks.

plugins/modules/wafv2_web_acl.py

stefanhorning · 2021-12-10T14:26:18Z

Sorry, completely forgot about this PR. Have applied @tremble s suggestion now to check botocore version directly in main. Hope we are good to merge then (might still need a rebase though).

…estsuite. Bumped version_added for new feature.

…h newever botocore)

…as its only depedency setup in the wafv2 target Syntax fixing in create_webacl.yml test play

…s feature

…onse_bodies

stefanhorning · 2022-02-09T08:38:56Z

Got the extra test working now (for custom_response_bodies deletion). Also rebased on latest main to squash all irrelevant commits and get rid of merge commits.

So in my opinion this can finally get merged.

softwarefactory-project-zuul · 2022-02-09T08:58:23Z

Build succeeded.

ansible-galaxy-importer : FAILURE in 4m 17s (non-voting)
build-ansible-collection : SUCCESS in 2m 54s
ansible-test-sanity-docker-devel : SUCCESS in 9m 14s (non-voting)
ansible-test-sanity-docker-milestone : SUCCESS in 14m 45s
ansible-test-sanity-docker-stable-2.9 : SUCCESS in 11m 48s
ansible-test-sanity-docker-stable-2.11 : SUCCESS in 16m 40s
ansible-test-sanity-docker-stable-2.12 : SUCCESS in 10m 57s
ansible-test-units-community-aws-python38 : SUCCESS in 6m 53s
ansible-test-splitter : SUCCESS in 2m 57s
ansible-test-cloud-integration-aws-py38_0 : SUCCESS in 14m 47s
ansible-test-cloud-integration-aws-py38_1 : SKIPPED
ansible-test-cloud-integration-aws-py38_2 : SKIPPED
ansible-test-cloud-integration-aws-py38_3 : SKIPPED
ansible-test-cloud-integration-aws-py38_4 : SKIPPED
ansible-test-cloud-integration-aws-py38_5 : SKIPPED

markuman · 2022-02-09T11:57:20Z

Got the extra test working now (for custom_response_bodies deletion). Also rebased on latest main to squash all irrelevant commits and get rid of merge commits.

So in my opinion this can finally get merged.

Ah one last change request.

You created an addition webacl. name: "{{ resource_prefix }}-acl-with-response-body"
This should also be removed in tests/integration/targets/wafv2/tasks/main.yml after line 68.

And you can verfiy the removal when you read-out the web acl again with its '_info` module

    - name: Web acl with custom response bodies verify removal of custom response
      assert:
        that:
          - acl_without_response_bodies is changed

alinabuzachis

@stefanhorning Thank you for taking time to work on this. I agree with @markuman suggested. Other than that, LGTM!

softwarefactory-project-zuul · 2022-02-10T10:01:53Z

Build succeeded.

ansible-galaxy-importer : FAILURE in 4m 18s (non-voting)
build-ansible-collection : SUCCESS in 2m 56s
ansible-test-sanity-docker-devel : SUCCESS in 14m 32s (non-voting)
ansible-test-sanity-docker-milestone : SUCCESS in 16m 07s
ansible-test-sanity-docker-stable-2.9 : SUCCESS in 15m 29s
ansible-test-sanity-docker-stable-2.11 : SUCCESS in 17m 22s
ansible-test-sanity-docker-stable-2.12 : SUCCESS in 14m 06s
ansible-test-units-community-aws-python38 : SUCCESS in 10m 44s
ansible-test-splitter : SUCCESS in 5m 00s
ansible-test-cloud-integration-aws-py38_0 : SUCCESS in 14m 37s
ansible-test-cloud-integration-aws-py38_1 : SKIPPED
ansible-test-cloud-integration-aws-py38_2 : SKIPPED
ansible-test-cloud-integration-aws-py38_3 : SKIPPED
ansible-test-cloud-integration-aws-py38_4 : SKIPPED
ansible-test-cloud-integration-aws-py38_5 : SKIPPED

stefanhorning · 2022-02-10T10:09:07Z

@markuman @alinabuzachis added the extra test and cleanup task now without breking the build again. 🎉
So I guess we are good now.

markuman · 2022-02-10T10:20:42Z

111 conversations, that's a number :)
Thanks for your work @stefanhorning.
Thanks for reviewing @alinabuzachis

stefanhorning · 2022-02-10T10:26:25Z

Yeah, it's probably the longest PR of all Ansible PRs that only add one param to a module. :)

softwarefactory-project-zuul · 2022-02-10T10:36:13Z

Build succeeded (gate pipeline).

ansible-galaxy-importer : FAILURE in 6m 26s (non-voting)
build-ansible-collection : SUCCESS in 3m 04s
ansible-test-sanity-docker-devel : SUCCESS in 10m 22s (non-voting)
ansible-test-sanity-docker-milestone : SUCCESS in 15m 55s
ansible-test-sanity-docker-stable-2.9 : SUCCESS in 12m 36s
ansible-test-sanity-docker-stable-2.11 : SUCCESS in 13m 02s
ansible-test-sanity-docker-stable-2.12 : SUCCESS in 10m 41s
ansible-test-units-community-aws-python38 : SUCCESS in 9m 15s
ansible-test-splitter : SUCCESS in 4m 00s
ansible-test-cloud-integration-aws-py38_0 : SUCCESS in 14m 15s
ansible-test-cloud-integration-aws-py38_1 : SKIPPED
ansible-test-cloud-integration-aws-py38_2 : SKIPPED
ansible-test-cloud-integration-aws-py38_3 : SKIPPED
ansible-test-cloud-integration-aws-py38_4 : SKIPPED
ansible-test-cloud-integration-aws-py38_5 : SKIPPED

patchback · 2022-02-10T10:36:18Z

Backport to stable-3: 💚 backport PR created

✅ Backport PR branch: patchback/backports/stable-3/c91acf6a14c0aa69974241ead13223a84dbd5334/pr-721

Backported as #934

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

#721) Extended the wafv2_web_acl module with custom_response_bodies argument SUMMARY Extended the wafv2_web_acl module to also take the custom_response_bodies argument, improved docs and extended tests ISSUE TYPE Feature Pull Request COMPONENT NAME wafv2_web_acl ADDITIONAL INFORMATION Also touched docs of aws_waf_web_acl to make it easier to find the WAF v2 modules as I had trouble finding that at first. Reviewed-by: Markus Bergholz <git@osuv.de> Reviewed-by: Stefan Horning <None> Reviewed-by: Mark Chappell <None> Reviewed-by: Alina Buzachis <None> (cherry picked from commit c91acf6)

#721) (#934) [PR #721/c91acf6a backport][stable-3] Extended the wafv2_web_acl module with custom_response_bodies argument This is a backport of PR #721 as merged into main (c91acf6). SUMMARY Extended the wafv2_web_acl module to also take the custom_response_bodies argument, improved docs and extended tests ISSUE TYPE Feature Pull Request COMPONENT NAME wafv2_web_acl ADDITIONAL INFORMATION Also touched docs of aws_waf_web_acl to make it easier to find the WAF v2 modules as I had trouble finding that at first.

ec2_instance incr version added to 3.3.0 for added parameters SUMMARY CI problems in backport-3 ansible-collections#721 And we don't won't to stop 3.2.0 release. Let's try to put/backport it for the next release. ISSUE TYPE Docs Pull Request COMPONENT NAME ec2_instance Reviewed-by: Gonéri Le Bouder <goneri@lebouder.net> Reviewed-by: Mark Chappell <None>

…ctions#852) ec2_instance: metadata_options version_added increased SUMMARY After CI troubles in the past, we've forget to backport this feature. incr from 3.2.0 to 3.3.0 ansible-collections#763 failed backport 3 PR for 3.2.0 release ansible-collections#721 initial implementation for 3.2.0 ansible-collections#715 therefore no changelog fragment is necessary. ISSUE TYPE Feature Pull Request COMPONENT NAME ec2_instance ADDITIONAL INFORMATION Reviewed-by: Jill R <None> Reviewed-by: Alina Buzachis <None> Reviewed-by: Mark Chappell <None>

ansibullbot added community_review feature This issue/PR relates to a feature request integration tests/integration module module needs_triage plugins plugin (any type) tests tests labels Sep 17, 2021

markuman reviewed Sep 17, 2021

View reviewed changes

plugins/modules/wafv2_web_acl.py Show resolved Hide resolved

markuman reviewed Sep 17, 2021

View reviewed changes

changelogs/fragments/721-wafv2_web_acl.yml Outdated Show resolved Hide resolved

markuman reviewed Sep 17, 2021

View reviewed changes

plugins/modules/wafv2_web_acl.py Outdated Show resolved Hide resolved

markuman reviewed Sep 17, 2021

View reviewed changes

tests/integration/targets/wafv2/tasks/create_webacl.yml Outdated Show resolved Hide resolved

ansibullbot removed the needs_triage label Sep 17, 2021

stefanhorning closed this Sep 20, 2021

stefanhorning reopened this Sep 20, 2021

tremble reviewed Sep 20, 2021

View reviewed changes

tests/integration/targets/wafv2/meta/main.yml Show resolved Hide resolved

markuman reviewed Sep 21, 2021

View reviewed changes

plugins/modules/wafv2_web_acl.py Outdated Show resolved Hide resolved

tremble reviewed Sep 22, 2021

View reviewed changes

plugins/modules/wafv2_web_acl.py Outdated Show resolved Hide resolved

stefanhorning force-pushed the wafv2_web_acl_improvements branch from 2b7b13a to 4b20420 Compare December 10, 2021 15:18

stefanhorning added 6 commits February 9, 2022 09:35

Add setup_remote_tmp_dir role dependency to wafv2 tests

d0dad9a

Actually make use of virtualenv python with newer botocore in wafv2 t…

71a06aa

…estsuite. Bumped version_added for new feature.

Actually put the test that needs virtualenv in a block to use it (wit…

98e90a2

…h newever botocore)

Removing failing test for ALB setup, does not need to be tested here …

ce88edc

…as its only depedency setup in the wafv2 target Syntax fixing in create_webacl.yml test play

Improvements from code review for wafv2_web_acl custom_response_bodie…

a7b3882

…s feature

Additional test for wafv2_web_acl to also test removal of custom_resp…

a113218

…onse_bodies

stefanhorning force-pushed the wafv2_web_acl_improvements branch from a804714 to a113218 Compare February 9, 2022 08:37

ansibullbot added community_review and removed merge_commit This PR contains at least one merge commit. Please resolve! needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html labels Feb 9, 2022

markuman approved these changes Feb 9, 2022

View reviewed changes

markuman requested a review from alinabuzachis February 9, 2022 09:20

alinabuzachis approved these changes Feb 9, 2022

View reviewed changes

More test improvements for wafv2_web_acl

81b1941

markuman added the mergeit Merge the PR (SoftwareFactory) label Feb 10, 2022

softwarefactory-project-zuul bot merged commit c91acf6 into ansible-collections:main Feb 10, 2022

patchback bot mentioned this pull request Feb 10, 2022

[PR #721/c91acf6a backport][stable-3] Extended the wafv2_web_acl module with custom_response_bodies argument #934

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extended the wafv2_web_acl module with custom_response_bodies argument #721

Extended the wafv2_web_acl module with custom_response_bodies argument #721

stefanhorning commented Sep 17, 2021 •

edited

Loading

ansibullbot commented Sep 17, 2021

stefanhorning commented Sep 17, 2021

markuman commented Sep 17, 2021

stefanhorning commented Sep 20, 2021 •

edited

Loading

markuman commented Sep 20, 2021

tremble commented Sep 20, 2021 •

edited

Loading

stefanhorning commented Sep 20, 2021

tremble commented Sep 20, 2021

stefanhorning commented Sep 20, 2021

tremble commented Sep 20, 2021

stefanhorning commented Sep 20, 2021

stefanhorning commented Dec 10, 2021

stefanhorning commented Feb 9, 2022

softwarefactory-project-zuul bot commented Feb 9, 2022

markuman commented Feb 9, 2022

alinabuzachis left a comment •

edited

Loading

softwarefactory-project-zuul bot commented Feb 10, 2022

stefanhorning commented Feb 10, 2022

markuman commented Feb 10, 2022

stefanhorning commented Feb 10, 2022

softwarefactory-project-zuul bot commented Feb 10, 2022

patchback bot commented Feb 10, 2022 •

edited

Loading

Extended the wafv2_web_acl module with custom_response_bodies argument #721

Extended the wafv2_web_acl module with custom_response_bodies argument #721

Conversation

stefanhorning commented Sep 17, 2021 • edited Loading

SUMMARY

ISSUE TYPE

COMPONENT NAME

ADDITIONAL INFORMATION

ansibullbot commented Sep 17, 2021

stefanhorning commented Sep 17, 2021

markuman commented Sep 17, 2021

stefanhorning commented Sep 20, 2021 • edited Loading

markuman commented Sep 20, 2021

tremble commented Sep 20, 2021 • edited Loading

stefanhorning commented Sep 20, 2021

tremble commented Sep 20, 2021

stefanhorning commented Sep 20, 2021

tremble commented Sep 20, 2021

stefanhorning commented Sep 20, 2021

stefanhorning commented Dec 10, 2021

stefanhorning commented Feb 9, 2022

softwarefactory-project-zuul bot commented Feb 9, 2022

markuman commented Feb 9, 2022

alinabuzachis left a comment • edited Loading

Choose a reason for hiding this comment

softwarefactory-project-zuul bot commented Feb 10, 2022

stefanhorning commented Feb 10, 2022

markuman commented Feb 10, 2022

stefanhorning commented Feb 10, 2022

softwarefactory-project-zuul bot commented Feb 10, 2022

patchback bot commented Feb 10, 2022 • edited Loading

Backport to stable-3: 💚 backport PR created

stefanhorning commented Sep 17, 2021 •

edited

Loading

stefanhorning commented Sep 20, 2021 •

edited

Loading

tremble commented Sep 20, 2021 •

edited

Loading

alinabuzachis left a comment •

edited

Loading

patchback bot commented Feb 10, 2022 •

edited

Loading