New ec2_group_facts module to be able to get facts from EC2 security groups #2591

Sodki · 2016-07-20T11:14:41Z

ISSUE TYPE

New Module Pull Request

COMPONENT NAME

ec2_group_facts

ANSIBLE VERSION

ansible 2.1.0.0
  config file = /etc/ansible/ansible.cfg
  configured module search path = Default w/o overrides

SUMMARY

Ansible doesn't have a way to obtain information from EC2 security groups, which is very useful. This implementation is heavily inspired on ec2_snapshot_facts.

The only option available is filter. I didn't implement boto3's GroupNames and GroupIds because they're a source of major confusion (default vs non-default VPC) and filter has that functionality anyway, so I thought it was better to keep it simple.

I know there are other PRs for the same module, but they were either broken or had a larger scope than what was intended. I'll list them here for reference:
#1939
#1908
#1352

dfjaimes · 2016-07-20T12:26:57Z

#1939 was closed a while back, so its irrelevant here
#1352 is certainly out of scope
#1908 isn't broken, nor is it out of scope - its just waiting on review again

I wrote it back in the boto2 days then retrofitted it to boto3 for the PR, so if the GroupNames and GroupIds aren't needed, I could simply pull that logic out - but, that hasn't been brought up in past reviews so I've never given it any thought.

…groups

gregdek · 2016-07-20T16:25:25Z

Thanks @Sodki for this new module. When this module receives 'shipit' comments from two community members and any 'needs_revision' comments have been resolved, we will mark for inclusion.

[This message brought to you by your friendly Ansibull-bot.]

Sodki · 2016-07-21T09:36:04Z

@calamityman Cool, at the time of writing this your PR was still in old boto. Instead of spamming your PR with comments, can I just send a bunch of PRs to your own branch, to try and make it better? Then I can close this one down.

dfjaimes · 2016-07-21T12:38:05Z

@Sodki of course. I'd be more than happy to work with you on this module if you've got some improvements.

Sodki · 2016-07-25T15:37:13Z

@calamityman Unfortunately it doesn't seem to be possible to send PRs to your branch, since it's a fork. I'll comment straight to your PR.

Sodki · 2016-08-15T14:26:32Z

#2619 is another PR related to this one.

Sodki · 2016-09-14T10:46:13Z

ready_for_review

aairey · 2016-10-24T13:38:01Z

Any update on this? Seems stalled.

Sodki · 2016-10-24T14:54:33Z

@aairey Well, the module is finished and works, but no one has reviewed it yet. I don't know if there's something else that I need to do to speed things up.

ryansb

Overall looks great, and works in my environment. Just a couple changes before we can get this merged in.

ryansb · 2016-11-10T14:42:44Z

cloud/amazon/ec2_group_facts.py

+
+# Gather facts about any security group with a tag key Name and value Example
+- ec2_group_facts:
+    filters:


On this example can you add a comment to the effect of "The quotes around tag:Name are important because of the colon in the value" because not everyone realizes the significance there.

Your comment was spot on, so I just used it verbatim.

ryansb · 2016-11-10T14:56:49Z

cloud/amazon/ec2_group_facts.py

+# Gather facts about a security group by id
+- ec2_group_facts:
+    filters:
+      group-id: sg-12345678


It would be really nice to be able to do underscore-separated tags as well, since that's typically how folks expect to do params. Do you think that'd be a useful addition?

diff --git a/cloud/amazon/ec2_group_facts.py b/cloud/amazon/ec2_group_facts.py index 777fc59..bc9a75d 100644 --- a/cloud/amazon/ec2_group_facts.py +++ b/cloud/amazon/ec2_group_facts.py @@ -121,9 +121,11 @@ def main(): else: module.fail_json(msg="region must be specified") + sanitized_filters = {k.replace('_', '-'): v for k, v in module.params.get("filters", {}).items() if 'tag:' not in k} + sanitized_filters.update({k: v for k, v in module.params.get("filters", {}).items() if 'tag:' in k}) try: security_groups = connection.describe_security_groups( - Filters=ansible_dict_to_boto3_filter_list(module.params.get("filters")) + Filters=ansible_dict_to_boto3_filter_list(sanitized_filters) ) except ClientError as e: module.fail_json(msg=e.message)

I think it's a good idea. I've implemented it a little bit different from you, but I think it's easier to read. I've also changed the documentation accordingly.

ryansb · 2016-11-10T14:58:31Z

cloud/amazon/ec2_group_facts.py

+            Filters=ansible_dict_to_boto3_filter_list(module.params.get("filters"))
+        )
+    except ClientError as e:
+        module.fail_json(msg=e.message)


Here can you add the exception to the failure, something like:

import traceback # do this with the other imports, not in the exception module.fail_json(msg=e.message, exception=traceback.format_exc(e))

It's done. Let me know if you disagree with the location of the import. I really don't have others like that one and I wanted to keep the boto ones separate.

ryansb · 2016-11-10T14:58:54Z

cloud/amazon/ec2_group_facts.py

+short_description: Gather facts about ec2 security groups in AWS.
+description:
+    - Gather facts about ec2 security groups in AWS.
+version_added: "2.2"


Need to update this to 2.3

It's been updated to 2.3.

…amples

Sodki · 2016-11-11T12:14:57Z

Thank you @ryansb for your review. I've implemented all your change requests as separate commits because I think it's easier to understand the flow. If you're happy I can then squash them all before merging.

ryansb · 2016-11-11T13:41:45Z

cloud/amazon/ec2_group_facts.py

+# Gather facts about a security group with multiple filters, also mixing the use of underscores as filter keys
+- ec2_group_facts:
+    filters:
+      group_id: sg-12345678


ryansb · 2016-11-11T13:43:17Z

cloud/amazon/ec2_group_facts.py

+
+    sanitized_filters = module.params.get("filters")
+    for key in sanitized_filters:
+      sanitized_filters[key.replace("_", "-")] = sanitized_filters.pop(key)


There's one issue with this: if someone uses a filter such as "tag:my_tag_name": value this code will replace it and make the filter tag:my-tag-name. In the example I gave it skipped replacing _ for any tag: filters.

Oh, I completely forgot about that edge case. It's fixed now.

ryansb · 2016-11-11T13:44:19Z

@Sodki really close, just that last comment on filter underscore-replacement. No worries about squashing this PR, I can do so on merge. Thanks for doing separate commits, makes reviewing much easier. 😄

Sodki · 2016-11-11T14:02:09Z

Alright, I fixed the last issue. Thanks again for taking the time to review this.

Add ec2_group_facts module to be able to get facts from EC2 security …

9e7b650

…groups

Sodki force-pushed the devel branch from c0d0b02 to 9e7b650 Compare July 20, 2016 12:57

gregdek added cloud new_plugin community_review labels Jul 20, 2016

ryansb suggested changes Nov 10, 2016

View reviewed changes

Henrique Rodrigues added 4 commits November 11, 2016 11:14

Update to Ansible 2.3

095a398

Add more information regarding the use of quotes on a value in the ex…

75f6364

…amples

Add traceback to the main boto3 operation exception

cdd6281

Allow using underscores instead of dashes as filter keys

2f9d91a

Add examples for alternative filter key, using underscores

bf0fd65

ryansb reviewed Nov 11, 2016

View reviewed changes

Don't replace underscores with dashes when dealing with tags

9429a79

ryansb approved these changes Nov 11, 2016

View reviewed changes

ryansb merged commit 09f6af8 into ansible:devel Nov 11, 2016

stefanhorning mentioned this pull request Nov 28, 2016

New module for finding existing EC2 security groups based on filters #2191

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New ec2_group_facts module to be able to get facts from EC2 security groups #2591

New ec2_group_facts module to be able to get facts from EC2 security groups #2591

Sodki commented Jul 20, 2016

dfjaimes commented Jul 20, 2016

gregdek commented Jul 20, 2016

Sodki commented Jul 21, 2016

dfjaimes commented Jul 21, 2016

Sodki commented Jul 25, 2016

Sodki commented Aug 15, 2016

Sodki commented Sep 14, 2016

aairey commented Oct 24, 2016

Sodki commented Oct 24, 2016 •

edited

Loading

ryansb left a comment

ryansb Nov 10, 2016

Sodki Nov 11, 2016

ryansb Nov 10, 2016

Sodki Nov 11, 2016

ryansb Nov 10, 2016

Sodki Nov 11, 2016

ryansb Nov 10, 2016

Sodki Nov 11, 2016

Sodki commented Nov 11, 2016

ryansb Nov 11, 2016

ryansb Nov 11, 2016

Sodki Nov 11, 2016

ryansb commented Nov 11, 2016

Sodki commented Nov 11, 2016

New ec2_group_facts module to be able to get facts from EC2 security groups #2591

New ec2_group_facts module to be able to get facts from EC2 security groups #2591

Conversation

Sodki commented Jul 20, 2016

ISSUE TYPE

COMPONENT NAME

ANSIBLE VERSION

SUMMARY

dfjaimes commented Jul 20, 2016

gregdek commented Jul 20, 2016

Sodki commented Jul 21, 2016

dfjaimes commented Jul 21, 2016

Sodki commented Jul 25, 2016

Sodki commented Aug 15, 2016

Sodki commented Sep 14, 2016

aairey commented Oct 24, 2016

Sodki commented Oct 24, 2016 • edited Loading

ryansb 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

Sodki commented Nov 11, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryansb commented Nov 11, 2016

Sodki commented Nov 11, 2016

Sodki commented Oct 24, 2016 •

edited

Loading