Skip to content
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

Aws ses rule set module for inbound email processing #42781

Merged
merged 7 commits into from
Nov 14, 2018

Conversation

orthanc
Copy link
Contributor

@orthanc orthanc commented Jul 14, 2018

SUMMARY

This is intended to superceed the abandoned pull request #22854 by @tomislacker which was trying to contribute modules for managing SES rule_sets and rules to allow for email to be recieved and processed with SES.

I've updated the modules from that PR to adhear to the same naming conventions and behaviours as the other SES modules and comply with the current AWS module guidelines including integration tests.

The major behavioural changes from the starting point modules are:

  • Module name changed to start with aws_ to match the preferred convention
  • Omitting the active parameter does not change the activation of the rule set rather than deactiving the rule set as it did in the earlier PR
  • New force option that can be set when state=absent to allow the deletion of active rule sets
  • Information on the current rule-sets is returned on success.

Examples:

 name: Create default rule set and activate it if not already
  aws_ses_rule_set:
    name: default-rule-set
    state: present
    active: yes

- name: Create some arbitrary rule set but do not activate it
  aws_ses_rule_set:
    name: arbitrary-rule-set
    state: present

- name: Explicitly deactivate the default rule set leaving no active rule set
  aws_ses_rule_set:
    name: default-rule-set
    state: present
    active: no

- name: Remove an arbitary inactive rule set
  aws_ses_rule_set:
    name: arbitrary-rule-set
    state: absent

- name: Remove an ruleset even if we have to first deactivate it to remove it
  aws_ses_rule_set:
    name: default-rule-set
    state: absent
    force: yes
ISSUE TYPE
  • New Module Pull Request
COMPONENT NAME

aws_ses_rule_set

ANSIBLE VERSION
ansible 2.7.0.dev0 (aws_ses_rule_set_module 88ffb1de43) last updated 2018/07/14 23:05:38 (GMT +1300)
  config file = None
  configured module search path = [u'USER_HOME/.ansible/plugins/modules', u'/usr/share/ansible/plugins/modules']
  ansible python module location = ANSIBLE_CHECKOUT/lib/ansible
  executable location = ANSIBLE_CHECKOUT/bin/ansible
  python version = 2.7.15rc1 (default, Apr 15 2018, 21:51:34) [GCC 7.3.0]
ADDITIONAL INFORMATION

This module is the first of three in a chain that should fully superceed #22854 fully:

  • aws_ses_rule_set - management of rule sets
  • aws_ses_rule_set_facts - get information on existing rule sets and the rules within them
  • aws_ses_rule - management of individual rules within a rule set

aws_ses_rule_set_facts is required in order to write integration tests for aws_ses_rule. Similarly we can't fully test aws_ses_rule_set_facts without aws_ses_rule to enable the creation of rules.

So to comply with the one module per PR rule the plan is to contribute aws_ses_rule_set_facts with a basic test suite focusing on the rule-set info, then include additional tests for facts abouut the rules in a rule set as part of the subsequent PR for aws_ses_rule.

@ansibot ansibot added affects_2.7 This issue/PR affects Ansible v2.7 aws cloud core_review In order to be merged, this PR must follow the core review workflow. module This issue/PR relates to a module. needs_triage Needs a first human triage before being processed. new_module This PR includes a new module. new_plugin This PR includes a new plugin. support:community This issue/PR relates to code supported by the Ansible community. support:core This issue/PR relates to code supported by the Ansible Engineering Team. test This PR relates to tests. labels Jul 14, 2018
@ansibot ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed core_review In order to be merged, this PR must follow the core review workflow. labels Jul 14, 2018
@orthanc
Copy link
Contributor Author

orthanc commented Jul 14, 2018

Failing test is due to missing AWS permission in the CI build

ClientError: An error occurred (AccessDenied) when calling the ListReceiptRuleSets operation: User: arn:aws:sts::966509639900:assumed-role/ansible-core-ci-test-prod/prod=shippable=ansible=ansible=74119.66 is not authorized to perform: ses:ListReceiptRuleSets

Relevant permission updates are included in compute-policy.json

@mattclay mattclay added the needs_ci_update This PR is blocked as it requires an update to CI infrastructure before tests can pass in CI. label Jul 16, 2018
@jborean93 jborean93 removed the needs_triage Needs a first human triage before being processed. label Jul 19, 2018
@s-hertel s-hertel removed the needs_ci_update This PR is blocked as it requires an update to CI infrastructure before tests can pass in CI. label Jul 19, 2018
@ansibot ansibot added core_review In order to be merged, this PR must follow the core review workflow. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Jul 19, 2018
Copy link
Contributor

@s-hertel s-hertel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The module looks great but the tests are a little unstable. Those should probably be fixed if possible before merging. We're tracking unstable and broken AWS tests here https://github.com/ansible/ansible/projects/21 and trying to not add more.

- name: assert changed is False
assert:
that:
- result.changed == False
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to be periodically failing with changed is True. I think the module may need a waiter.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might be wrong, but I don't think this is a timing / eventual consistency thing.

There can only be one active rule set per-region per-account. I think what's happening is the python2 and python3 tests are running in parallel and if we're unlucky, the other test creates an active rule in the gap here and the test fails becuse this rule is no-longer active so making it active is a change.

Hard to be sure because it's intermittent, but I've repeatedly run ths test on an ec2 instance in the same region as the SES instance and not had a single failure, but when I run 2 instances in parallel of just this test it fails reliably as described.

I'll have to give some thought to how to have a reliable test here since the active ruleset really is global state.

Any advice or thoughts would be appreciated.

name: "{{ default_rule_set }}"
<<: *aws_connection_info
register: result
- name: assert not changed and still active
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one is also sometimes failing with changed is True

@ansibot ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed core_review In order to be merged, this PR must follow the core review workflow. labels Jul 23, 2018
@mattclay
Copy link
Member

CI failure in integration tests: https://app.shippable.com/github/ansible/ansible/runs/75475/66/tests

@mattclay mattclay added the ci_verified Changes made in this PR are causing tests to fail. label Jul 26, 2018
@ansibot ansibot removed the ci_verified Changes made in this PR are causing tests to fail. label Jul 28, 2018
@ansibot
Copy link
Contributor

ansibot commented Jul 28, 2018

The test ansible-test sanity --test integration-aliases [explain] failed with 1 error:

test/integration/targets/aws_ses_rule_set/aliases:0:0: missing alias `shippable/aws/group[1-2]` or `unsupported`

The test ansible-test sanity --test yamllint [explain] failed with 1 error:

test/integration/targets/aws_ses_rule_set/defaults/main.yaml:9:1: empty-lines too many blank lines (1 > 0)

click here for bot help

@ansibot ansibot added ci_verified Changes made in this PR are causing tests to fail. and removed ci_verified Changes made in this PR are causing tests to fail. labels Jul 28, 2018
@ansibot
Copy link
Contributor

ansibot commented Jul 28, 2018

The test ansible-test sanity --test yamllint [explain] failed with 1 error:

test/integration/targets/aws_ses_rule_set/defaults/main.yaml:9:1: empty-lines too many blank lines (1 > 0)

click here for bot help

@orthanc
Copy link
Contributor Author

orthanc commented Jul 29, 2018

Latest update should fix the intermittent test failures. As suggested on the IRC channel I've introduced a per region/account lock around tests that rely on the rule set being active since there can only be one active rule set.

Explanation of the locking approach is in the comment at the top of obtain-lock.yaml

Basically it's creating cloudwatch log groups as the central coordination resource. This necessitated updating the testing iam policies to allow access to cloudwatch logs. The integration build is now failing because of the missing cloudwatch logs permissions.

In testing the locking I found an bug in the module that was also causing intermittent failures which is fixed by this commit. Essentially the bug was that when state=absent and force=true I was deactivating the active rule set even if it wasn't the rule set being deleted. This is fixed by only deactivating the active rule set if it's the rule set being deleted.

@mattclay mattclay added the needs_ci_update This PR is blocked as it requires an update to CI infrastructure before tests can pass in CI. label Jul 30, 2018
@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Aug 7, 2018
@s-hertel s-hertel removed the needs_ci_update This PR is blocked as it requires an update to CI infrastructure before tests can pass in CI. label Nov 14, 2018
@ansibot ansibot added core_review In order to be merged, this PR must follow the core review workflow. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. labels Nov 14, 2018
@s-hertel s-hertel merged commit b70d5d9 into ansible:devel Nov 14, 2018
@s-hertel
Copy link
Contributor

@orthanc Thanks for the fantastic work, as usual. Sorry this took a while to address the needs_ci_update.

mjmayer pushed a commit to mjmayer/ansible that referenced this pull request Nov 30, 2018
* Add module ses_rule_set for Amazon SES

* Update behaviours and naming to be consistent with other aws_ses_ modules.

* Add global lock around tests using active rule sets to prevent intermittent test failures.

* Fix deletion of rule sets so that we don't inactivate the active rule set
when force deleting an inactive rule set.
Tomorrow9 pushed a commit to Tomorrow9/ansible that referenced this pull request Dec 4, 2018
* Add module ses_rule_set for Amazon SES

* Update behaviours and naming to be consistent with other aws_ses_ modules.

* Add global lock around tests using active rule sets to prevent intermittent test failures.

* Fix deletion of rule sets so that we don't inactivate the active rule set
when force deleting an inactive rule set.
@ansible ansible locked and limited conversation to collaborators Jul 22, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affects_2.7 This issue/PR affects Ansible v2.7 aws cloud core_review In order to be merged, this PR must follow the core review workflow. module This issue/PR relates to a module. new_module This PR includes a new module. new_plugin This PR includes a new plugin. support:community This issue/PR relates to code supported by the Ansible community. support:core This issue/PR relates to code supported by the Ansible Engineering Team. test This PR relates to tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants