New module for AWS SES DKIM identity #71

stefanhorning · 2020-05-14T10:11:51Z

SUMMARY

New module aws_ses_identity_dkim. I already opened a PR for this once quite a while ago, see ansible/ansible#53637

ISSUE TYPE

New Module Pull Request

COMPONENT NAME

aws_ses_identity_dkim

ADDITIONAL INFORMATION

Allows to setup DKIM configurations on AWS SES identities (i.e. validated domains or email-addresses).

stefanhorning · 2020-05-14T10:47:06Z

IAM user for CI tests still needs the ses:VerifyDomainDkim permission to be able to run the integration test for this module.

tremble

I apologise that the review on ansible/ansible didn't get done.

@jillr Now that we're in our own namespace I personally would suggest naming this module ses_identity_dkim rather than aws_ses_identity_dkim (it's FQCN would currently be community.aws.aws_ses_identity_dkim which seems silly) I know going through and mass-renaming will be painful, but for new modules I think it's worth starting as we'd like to go forward.

please also update
meta/action_groups.yml

This means that module_defaults: { group/aws: .... } works

plugins/modules/aws_ses_identity_dkim.py

tremble · 2020-05-14T10:56:55Z

plugins/modules/aws_ses_identity_dkim.py

+        return response
+    except (BotoCoreError) as e:
+        module.fail_json_aws(e, msg='Failed to enable DKIM for {identity}.'.format(identity=identity))
+    except (ClientError) as e:


If you've got the explicit error it would be better to catch it with except is_boto3_error_code('ErrorType') there are other reasons ClientError can be thrown.

ok, will look into that

plugins/modules/aws_ses_identity_dkim.py

tests/integration/targets/aws_ses_identity_dkim/tasks/main.yml

tremble · 2020-05-14T11:09:00Z

plugins/modules/aws_ses_identity_dkim.py

+            "identity": dict(required=True, type='str'),
+            "state": dict(default='enabled', choices=['enabled', 'disabled']),
+        },
+        supports_check_mode=False,


not supporting check mode is irritating for users.

Ok, I didn't realize it was that important. I just set it to false here as in my last PR we got lost in implementing check-mode logic. So thought I would simply skip this this time. :P

tremble · 2020-05-14T11:09:38Z

tests/integration/targets/aws_ses_identity_dkim/tasks/main.yml

+        that:
+          - email_result.changed == False
+          - email_result.dkim_attributes.dkim_enabled == False
+


Please test delete mode (including trying to re-delete the object)

There is not delete mode as actions are just made on the existing identities. Hence also using the enable/disable terminology instead of present/absent (or add/delete).

The deletion of the test objects is done in the always: section with the already existing aws_ses_identity module.

What's the advantage of this being a separate module instead of extending aws_ses_identity?

tremble · 2020-05-14T11:11:25Z

plugins/modules/aws_ses_identity_dkim.py

+def ses_verify_dkim_domain(module, client, identity):
+    domain = identity.split('@')[-1]
+    try:
+        response = client.verify_domain_dkim(Domain=domain)


using except is_boto_error_code('SomeErrorIndicatingAbsence'): it should be possible to catch 'NotFound' and return a null here. This might make it easier to add check_mode support.

But isn't it better to fail if s.th. goes wrong here? As returning with a null here, will just break anything a user was intending to do with return values.

I meant to attach this comment to get_identity_dkim_settings thinking that this was creating a separate object... If this is just setting a flag on an SES identity, then it's probably better handled as an extension of aws_ses_identity

That's a valid point. I guess I just started a seperate module back then because I thought there is not much overlap between the two modules in terms of logic and params needed. But as both modules use the same Boto3 module one could also argue to keep things together. Not sure what the general policy is for AWS modules. As it seems newer modules rather go the one purpose one module approach, but I could be wrong.

We should probably decide on this first before putting more effort into this PR.

tremble · 2020-05-14T11:25:05Z

CI policy can be updated with a PR against https://github.com/mattclay/aws-terminator/tree/master/aws/policy

stefanhorning · 2020-05-14T13:36:05Z

Also opened PR against aws-terminator for SES permissions: mattclay/aws-terminator#99

stefanhorning · 2020-05-14T13:39:29Z

I apologise that the review on ansible/ansible didn't get done.

No worries, it was partly my own fault. Lost focus after a while of discussion and suggested changes on how to best implement check mode and keep responses idempotent etc.
So I just thought I would give it another shot starting from the initial commit from back then.

@jillr Now that we're in our own namespace I personally would suggest naming this module ses_identity_dkim rather than aws_ses_identity_dkim (it's FQCN would currently be community.aws.aws_ses_identity_dkim which seems silly) [...]

Renamed the module now

jillr

Thanks for this module. A couple of things here that stood out while testing the policy change, I was able to test enough to get the policies approved and deployed though.

jillr · 2020-05-14T23:59:20Z

tests/integration/targets/ses_identity_dkim/tasks/main.yml

+      region: "{{ aws_region }}"
+  no_log: yes
+
+- block:


Suggested change

- block:

block:

jillr · 2020-05-14T23:59:23Z

tests/integration/targets/ses_identity_dkim/tasks/main.yml

+      aws_secret_key: "{{ aws_secret_key }}"
+      security_token: "{{ security_token | default(omit) }}"
+      region: "{{ aws_region }}"
+  no_log: yes


no_log needs to be used if setting authentication via set_fact and a yaml anchor, but not with module_defaults. This has the effect of suppressing all task output for the entire playbook.

That said, module_defaults for new collection modules is still WIP. For testing I've added this module to lib/ansible/config/module_defaults.yml in my local checkout of devel but we're going to need to coordinate making sure that when this lands, the necessary bits are in place for these tests to run in CI (apologies for the unfortunate timing of so many things being in-flights still).

Normally this would need to be:

module_defaults: group/aws:

But let's pause on making any actual changes until this PR is further along: ansible/ansible#67291

Module defaults can now be added by listing the module in action_groups in https://github.com/ansible-collections/community.aws/blob/main/meta/runtime.yml

jillr · 2020-05-15T00:17:05Z

plugins/modules/ses_identity_dkim.py

+        ses_verify_dkim_domain(module, client, identity)
+        enable_identity_dkim_settings(module, client, identity)
+        # TODO: find a way to easily detect changes (updates to identity)
+        changed = True


This is always returning changed=True, which causes your assertion for - name: Enable DKIM verification on email identity (rerun) to fail.

ansibullbot · 2020-08-19T23:34:53Z

cc @s-hertel @wimnat
click here for bot help

stefanhorning · 2022-02-11T09:14:30Z

will close this, as I don't think it's going anywhere.

* Remove legacy files in plugins/module_utils/aws * ci_complete * Clean up remaining instances of module_utils[./]aws

stefanhorning added 4 commits May 14, 2020 12:09

New module for AWS SES DKIM identity

8ca2497

Fixed import, boilerplate lines

4ffb547

Move tests to correct dir

da558bb

Remove Check-Mode related tests, module won't support check-mode for now

cc9dd06

stefanhorning mentioned this pull request May 14, 2020

New AWS SES module to enable DKIM on an identity… ansible/ansible#53637

Closed

tremble requested changes May 14, 2020

View reviewed changes

stefanhorning mentioned this pull request May 14, 2020

Added SES DKIM permissions, needed for new ses_identity_dkim module mattclay/aws-terminator#99

Merged

stefanhorning added 3 commits May 14, 2020 15:08

Rename aws_ses_identity_dkim to ses_identity_dkim

fedfef1

Add some code review improvements

8750a10

Fix in test file

43287ce

jillr reviewed May 15, 2020

View reviewed changes

jillr changed the base branch from master to main July 2, 2020 19:48

Commit change

816d3a1

ansibullbot added the plugins plugin (any type) label Aug 28, 2020

tremble added the pr_day Has been reviewed during a PR review Day. https://github.com/ansible/community/issues/407 label Dec 1, 2020

Fix module utils namespace

6dd6b72

stefanhorning closed this Feb 11, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New module for AWS SES DKIM identity #71

New module for AWS SES DKIM identity #71

stefanhorning commented May 14, 2020 •

edited

stefanhorning commented May 14, 2020

tremble left a comment

tremble May 14, 2020

stefanhorning May 14, 2020

tremble May 14, 2020

stefanhorning May 14, 2020

tremble May 14, 2020

stefanhorning May 14, 2020

tremble May 14, 2020

tremble May 14, 2020

stefanhorning May 14, 2020

tremble May 14, 2020

stefanhorning May 20, 2020

tremble commented May 14, 2020

stefanhorning commented May 14, 2020

stefanhorning commented May 14, 2020 •

edited

jillr left a comment

jillr May 14, 2020

jillr May 14, 2020

jillr Jul 6, 2020

jillr May 15, 2020

ansibullbot commented Aug 19, 2020

stefanhorning commented Feb 11, 2022

New module for AWS SES DKIM identity #71

New module for AWS SES DKIM identity #71

Conversation

stefanhorning commented May 14, 2020 • edited

SUMMARY

ISSUE TYPE

COMPONENT NAME

ADDITIONAL INFORMATION

stefanhorning commented May 14, 2020

tremble left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tremble commented May 14, 2020

stefanhorning commented May 14, 2020

stefanhorning commented May 14, 2020 • edited

jillr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ansibullbot commented Aug 19, 2020

stefanhorning commented Feb 11, 2022

stefanhorning commented May 14, 2020 •

edited

stefanhorning commented May 14, 2020 •

edited