New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add AWS WAF Regional support for AWS ALBs #44896

Open
wants to merge 2 commits into
base: devel
from

Conversation

Projects
None yet
7 participants
@bje

bje commented Aug 30, 2018

SUMMARY

Add AWS WAF Regional support for AWS ALBs.

The existing AWS WAF module only works with AWS Cloudfront and not with ALBs. Instead of extending the existing AWS WAF module, I created new aws_waf_regional* components, since AWS themselves did not extend the aws waf CLI tool to support AWS WAF Regional, but instead added a aws waf-regional CLI option.

ISSUE TYPE
  • New Module Pull Request
COMPONENT NAME
  • aws_waf_regional_condition
  • aws_waf_regional_facts
  • aws_waf_regional_rule
  • aws_waf_regional_web_acl
ANSIBLE VERSION
ansible 2.6.2 (v2.6.2-alb-h2-waf-regional cfd25bf55b) last updated 2018/08/30 12:36:19 (GMT +100)
  config file = /Users/bje/devel/launchdarkly/playbooks/ansible.cfg
  configured module search path = [u'/Users/bje/.ansible/plugins/modules', u'/usr/share/ansible/plugins/modules']
  ansible python module location = /Users/bje/devel/launchdarkly/playbooks/src/ansible/lib/ansible
  executable location = /usr/local/bin/ansible
  python version = 2.7.15 (default, Jun 17 2018, 12:46:58) [GCC 4.2.1 Compatible Apple LLVM 9.1.0 (clang-902.0.39.2)]
@ansibot

This comment has been minimized.

Contributor

ansibot commented Aug 30, 2018

@bje this PR contains more than one new module.

Please submit only one new module per pull request. For a detailed explanation, please read the grouped modules documentation

click here for bot help

@ansibot

This comment has been minimized.

Contributor

ansibot commented Aug 30, 2018

Hi @bje,

Thank you for the pullrequest, just so you are aware we have a dedicated Working Group for aws.
You can find other people interested in this in #ansible-aws on Freenode IRC
For more information about communities, meetings and agendas see https://github.com/ansible/community

click here for bot help

@ansibot

This comment has been minimized.

Contributor

ansibot commented Aug 30, 2018

The test ansible-test sanity --test ansible-doc --python 2.6 [explain] failed with 1 error:

lib/ansible/modules/cloud/amazon/aws_waf_regional_associate_elbv2.py:0:0: has a documentation error formatting or is missing documentation.

The test ansible-test sanity --test ansible-doc --python 2.7 [explain] failed with 1 error:

lib/ansible/modules/cloud/amazon/aws_waf_regional_associate_elbv2.py:0:0: has a documentation error formatting or is missing documentation.

The test ansible-test sanity --test ansible-doc --python 3.5 [explain] failed with 1 error:

lib/ansible/modules/cloud/amazon/aws_waf_regional_associate_elbv2.py:0:0: has a documentation error formatting or is missing documentation.

The test ansible-test sanity --test ansible-doc --python 3.6 [explain] failed with 1 error:

lib/ansible/modules/cloud/amazon/aws_waf_regional_associate_elbv2.py:0:0: has a documentation error formatting or is missing documentation.

The test ansible-test sanity --test ansible-doc --python 3.7 [explain] failed with 1 error:

lib/ansible/modules/cloud/amazon/aws_waf_regional_associate_elbv2.py:0:0: has a documentation error formatting or is missing documentation.

The test ansible-test sanity --test docs-build [explain] failed with the error:

Command "/usr/bin/python test/sanity/code-smell/docs-build.py" returned exit status 1.
>>> Standard Error
Traceback (most recent call last):
  File "test/sanity/code-smell/docs-build.py", line 100, in <module>
    main()
  File "test/sanity/code-smell/docs-build.py", line 17, in main
    raise subprocess.CalledProcessError(sphinx.returncode, cmd, output=stdout, stderr=stderr)
subprocess.CalledProcessError: Command '['make', 'singlehtmldocs']' returned non-zero exit status 2.

The test ansible-test sanity --test pep8 [explain] failed with 5 errors:

lib/ansible/module_utils/aws/waf_regional.py:167:1: E302 expected 2 blank lines, found 1
lib/ansible/modules/cloud/amazon/aws_waf_regional_associate_elbv2.py:73:1: E302 expected 2 blank lines, found 1
lib/ansible/modules/cloud/amazon/aws_waf_regional_associate_elbv2.py:87:78: E231 missing whitespace after ','
lib/ansible/modules/cloud/amazon/aws_waf_regional_associate_elbv2.py:112:1: E305 expected 2 blank lines after class or function definition, found 1
lib/ansible/modules/cloud/amazon/aws_waf_regional_rule.py:168:13: E265 block comment should start with '# '

The test ansible-test sanity --test validate-modules [explain] failed with 10 errors:

lib/ansible/modules/cloud/amazon/aws_waf_regional_associate_elbv2.py:0:0: E303 DOCUMENTATION fragment missing: ['aws', 'elb_application_lb']
lib/ansible/modules/cloud/amazon/aws_waf_regional_associate_elbv2.py:0:0: E305 DOCUMENTATION.module: not a valid value for dictionary value @ data['module']. Got 'aws_waf_associate_elbv2'
lib/ansible/modules/cloud/amazon/aws_waf_regional_associate_elbv2.py:0:0: E307 version_added should be 2.7. Currently 2.6
lib/ansible/modules/cloud/amazon/aws_waf_regional_condition.py:0:0: E307 version_added should be 2.7. Currently 2.6
lib/ansible/modules/cloud/amazon/aws_waf_regional_condition.py:0:0: E325 argument_spec for "purge_filters" defines type="bool" but documentation does not
lib/ansible/modules/cloud/amazon/aws_waf_regional_facts.py:0:0: E307 version_added should be 2.7. Currently 2.6
lib/ansible/modules/cloud/amazon/aws_waf_regional_rule.py:0:0: E307 version_added should be 2.7. Currently 2.6
lib/ansible/modules/cloud/amazon/aws_waf_regional_rule.py:0:0: E325 argument_spec for "purge_conditions" defines type="bool" but documentation does not
lib/ansible/modules/cloud/amazon/aws_waf_regional_web_acl.py:0:0: E307 version_added should be 2.7. Currently 2.6
lib/ansible/modules/cloud/amazon/aws_waf_regional_web_acl.py:0:0: E325 argument_spec for "purge_rules" defines type="bool" but documentation does not

click here for bot help

@s-hertel s-hertel removed the needs_triage label Aug 31, 2018

@ansibot ansibot added the stale_ci label Sep 8, 2018

@mjmayer

This comment has been minimized.

Contributor

mjmayer commented Nov 8, 2018

@bje I understand your justification for creating an entire new module for the regional WAF. After a cursory look at botocore difference between WAF and WAFRegional, the only method difference I saw was associate_web_acl. It seems like it would be easier to add that method to the existing Ansible WAF modules, rather than creating new modules and that are nearly identical to the existing WAF Module.

conditions:
description: >
list of conditions used in the rule. Each condition should
contain I(type): which is one of [C(byte), C(geo), C(ip), C(size), C(sql) or C(xss)]

This comment has been minimized.

@gundalow

gundalow Nov 13, 2018

Contributor
Suggested change Beta
contain I(type): which is one of [C(byte), C(geo), C(ip), C(size), C(sql) or C(xss)]
contain I(type) which is one of [C(byte), C(geo), C(ip), C(size), C(sql) or C(xss)]
description: >
list of conditions used in the rule. Each condition should
contain I(type): which is one of [C(byte), C(geo), C(ip), C(size), C(sql) or C(xss)]
I(negated): whether the condition should be negated, and C(condition),

This comment has been minimized.

@gundalow

gundalow Nov 13, 2018

Contributor
Suggested change Beta
I(negated): whether the condition should be negated, and C(condition),
I(negated) whether the condition should be negated, and C(condition),

The : is what's breaking yaml parsing

@DaveHewy

This comment has been minimized.

DaveHewy commented Nov 14, 2018

Is there going to be any more movement on this PR?
@mjmayer 's comments about preference perhaps for an optional flag on the existing waf_lib might be quicker?

@mjmayer

This comment has been minimized.

Contributor

mjmayer commented Nov 14, 2018

@DaveHewy I'm working on a branch that uses the existing WAF modules. I will start a new PR once I'm done and link to it here. I'm guessing bje's PR isn't going to see any movement.

@DaveHewy

This comment has been minimized.

DaveHewy commented Nov 14, 2018

@mjmayer as am i.
got an immediate need for it.

all i really needed to do was pass the resource=computed_resource to the boto client instantiation & add a module param to toggle waf-regional on and off. short-sighted i know, but for now less duplication. did you come up with similar solution?

its also worth noting - i found no need to toy around with the region. since the module.param['region'] can just be used for both purposes

check that: i'm also seeing a difference in the list-rules method

@mjmayer

This comment has been minimized.

Contributor

mjmayer commented Nov 14, 2018

@DaveHewy Yes, I came up with a similar solution. You will also need to add a waiter for WAFRegional

https://github.com/mjmayer/ansible/blob/d665758be92b49209278b99904d8a72f3b026401/lib/ansible/module_utils/aws/waiters.py#L308-L313

@DaveHewy

This comment has been minimized.

DaveHewy commented Nov 14, 2018

@mjmayer actual error
An exception occurred during task execution. To see the full traceback, use -vvv. The error was: OperationNotPageableError: Operation cannot be paginated: list_rules

@mjmayer

This comment has been minimized.

Contributor

mjmayer commented Nov 14, 2018

@DaveHewy I'm seeing the same issue when creating a new WAF rule. My initial testing was with creating a WAF Condition. I will investigate further.

@mjmayer

This comment has been minimized.

Contributor

mjmayer commented Nov 14, 2018

@DaveHewy I fixed one of the pagination errors in the aws_waf_rule. It won't work correctly at the moment if you have more than 100 rules.

https://github.com/mjmayer/ansible/tree/aws_waf_region

@DaveHewy

This comment has been minimized.

DaveHewy commented Nov 15, 2018

@mjmayer
taken a look at the WIP branch.

Here's a simple paginator snippet that works well enough;

resp = client.list_rules() 
rules = []
while resp:
    rules += resp['Rules']
    resp = client.list_rules(NextMarker=resp['NextMarker']) if 'NextMarker' in resp else None

Also if i'd consider dropping;

cloudfront=dict(type='bool', default=True),

in favour of

waf_regional=dict(type='bool', default=True),

or something thats service agnostic and reflective of the terminology used through the GUI for global vs regional.

Left a few inline comments as well. Let me know what you think.

@DaveHewy

This comment has been minimized.

DaveHewy commented Nov 15, 2018

@mjmayer there is also large scale changes required under aws_waf_rule.py

around these lines:

        all_conditions[condition_type] = dict()
        try:
            paginator = client.get_paginator(method)
@mjmayer

This comment has been minimized.

Contributor

mjmayer commented Nov 15, 2018

@DaveHewy Thanks for all the suggestions! The pagination code you provided will speed development up for me. I probably won't get to most of this until friday.

@DaveHewy

This comment has been minimized.

DaveHewy commented Nov 20, 2018

@mjmayer I now have this all rudimentally working. If of any interest let me know.

@mjmayer

This comment has been minimized.

Contributor

mjmayer commented Nov 20, 2018

@DaveHewy I have a working module as well, I'm just about to start writing the integration tests before I initiate a PR.

I'm interested in seeing your code to see where I could optimize mine.

@DaveHewy

This comment has been minimized.

DaveHewy commented Nov 20, 2018

@mjmayer ill raise visibility of it shortly.
im actually struggling with a nice way to keep the ansible/module_utils/aws/waf.py mods anyway. seems I can't import submodules relative to the custom ansible module i've created. nor can I find a reliable way of injecting files without horrible pre-tasks in ansible copying/installing deps on the host.

@mjmayer mjmayer referenced this pull request Nov 20, 2018

Open

Aws waf region #48953

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment