Add rule revocation capabilities to ec2_group

[Sodki]

ISSUE TYPE

• Feature Pull Request

COMPONENT NAME

ec2_group

ANSIBLE VERSION

ansible 2.3.0.0
  config file = 
  configured module search path = Default w/o overrides
  python version = 2.7.12+ (default, Sep 17 2016, 12:08:02) [GCC 6.2.0 20160914]

SUMMARY

ec2_group allows us to create security groups, to remove security groups and to add new rules to security groups. What it doesn't allow us to do is to remove rules, so we can't easily revoke rules that have been set. With this change we can.

Fixes #20456

[ansibot]

cc @willthames
click here for bot help

[ansibot]

The test ansible-test sanity --test pep8 failed with the following error:

lib/ansible/modules/cloud/amazon/ec2_group.py:643:8: E114 indentation is not a multiple of four (comment)

click here for bot help

[s-hertel]

The code/documentation looks good. I guess using the option to purge rules isn't working well for your use cases?

[Sodki]

The problem with purging rules is that you need to calculate the difference between the current rules and the rules that you want to remove, which is a bit daunting, specially if you're using it extensively.

Revocation capabilities are already present in AWS's API and Boto, this PR just makes Ansible aware of them.

[s-hertel]

@Sodki Makes sense, it sounds tedious until it's automated. Hm. What is your process for adding rules/egress rules? How do you get the ips and ports?

[Sodki]

We mostly deal with security groups. Let's say that a service is responsible for making itself work, so when it comes alive it will open the necessary ports on the services it needs. When it's time to destroy that service, it cleans itself up by revoking the access it initially created.

[Sodki]

I just hit a wall and realised this alone isn't necessarily idempotent when using group names. Group ids should work just fine.

If we do the revocation over and over again, it is idempotent, but as soon as the group is removed, this module will fail because ec2_group will try to create a group from a revocation rule and fail because no description was given.

[shaunbrady]

This type of feature would be excellent.

[captainkerk]

It looks like these changes were approved... is this going to be merged soon?

[s-hertel]

@captainkerk Hopefully!

@Sodki Can you document the lack of idempotence when using group names for this? And can you fix this to exit gracefully if the group doesn't exist instead of failing?

[captainkerk]

@Sodki If i understand correctly: when revoking a rule referencing a source security group by name, if the name is not found, the module attempts to create the security group to satisfy the condition? But it fails because it tries to create it without having a description?

@s-hertel It seems like you are advocating for the module to fail silently. If the example that @Sodki gave happens, I think the module should fail with a helpful message. This will cause the user to fix the problem (remove the now defunct security group reference from the revocation list) rather than thinking the security group being managed is configured as defined.

[s-hertel]

@captainkerk I'm definitely not advocating for it to fail silently! I haven't run this using group name, but it sounded like it might be raising an exception. If that is the case, it needs to be caught and handled. Regardless though, this needs to be added to the documentation.

[wimnat]

I do not think this change should be made in ec2_group.

I think you're better off having a separate module ec2_group_rule that allows the addition and removal of individual rules to a group. If the group didn't exist then the module you just report changed=False and, if you like, an additional message saying the group didn't exist.

It's similar to how the elb classic modules work - one for managing an elb and one for managing the instances attached to that elb. Something similar coming for target groups too.

[ryansb]

I think if the idea is to be able to force rules not to exist, a new state parameter would be a better fit, if not a new module for just the rules. How about something like this instead:

- ec2_group:
    name: foobar
    description: Group rules so Foo can access the Bar service
    rules:
      - to_port: 443
        from_port: 443
        proto: tcp
        cidr_ip: 0.0.0.0/0
- ec2_group:
    name: foobar
    state: denied
    rules:
      - to_port: 80
        from_port: 80
        proto: tcp
        cidr_ip: 0.0.0.0/0

So for rules to revoke/deny, you'd have a separate task that would use the same code paths to get info about what rules are in a group, but without adding a parallel set of options (and the confusion of purge vs. revoke option sets). That way, you don't have weird questions like "if someone specifies a rule in both rules and revoke_rules what happens?"

So instead of adding a new parameter, how do you feel about a state instead? @wimnat @Sodki

[wimnat]

I still prefer the new module option to be honest.

The beauty of Ansible and desired state config in general is the simplicity. I like simple state is present or state is absent options. Whenever I see more state options I start to worry about code complexity.

# I know absolutely that this is going to try and leave me with an ec2 security group
ec2_group:
  state: present

# I know absolutely that this is going to try and leave me without the ec2 security group rule
ec2_group_rule:
  state: absent

[ansibot]

cc @adq
click here for bot help

[s-hertel]

This will probably not rebase cleanly since ec2_group has been refactored for boto3. If you're still interested in this, an ec2_group_rule module as wimnat suggested would probably be a better approach than overload this module.