ansible-collections / amazon.aws

Ansible Collection for Amazon AWS
GNU General Public License v3.0
304 stars 332 forks source link

ec2_group: Add `state: [present/absent]` for individual group rules #629

Open LawrenceWarren opened 2 years ago

LawrenceWarren commented 2 years ago

Summary

Current situation

I am using Terraform to deploy an EMR cluster and related configuration, and then provisioning Ansible from Terraform to configure Kylin on this EMR cluster.

Now, part of my companies infrastructure requires us to add the EMR security group as an ingress rule to our MSK security group, so that Kylin can talk to Kafka. The MSK security group already exists, and has several other ingress rules from other services already added to it. Adding the ingress rule is currently done manually.

I have configured our Ansible playbook to add an ingress rule to the MSK security group, for the newly deployed EMR cluster:

name: Add EMR security group to MSK as an ingress rule
amazon.aws.ec2_group:
  name: "msk_sg_name"
  validate_certs: true
  purge_rules: false
  purge_rules_egress: false
  purge_tags: false
  rules:
   -  group_name: "emr_sg_name"
      ports:
        - 12345
        - 54321
      proto: tcp
      rule_desc: "Rule created by Ansible from terraform_module_name"
  rules_egress: [] 

Great! This does exactly what is needed - creates an ingress rule.

The problem

However, our EMR SG name changes every deployment. We don't often cycle Kylin, but what this will result in is that every time the playbook is rerun, we will gain an additional ingress rule, without tidying up the old ones.

Feature request

In the same way that the security group itself has a state: [present/absent]option, which allows you to create or delete an entire security group, add this on a per-rule basis.

This would allow us to tidy up our security group rules as we continue to update and redepoy our Kylin stack.

Issue Type

Feature Idea

Component Name

ec2_group

Additional Information

Here is an example of how this feature may be implemented.

name: Add new EMR security group to MSK as an ingress rule, delete old ingress rule
amazon.aws.ec2_group:
  name: "msk_sg_name"
  validate_certs: true
  purge_rules: false
  purge_rules_egress: false
  purge_tags: false
  rules:
   # Delete old rule
   -  group_name: "old_emr_sg_name"
      state: absent
   # Create new rule
   -  group_name: "new_emr_sg_name"
      ports:
        - 12345
        - 54321
      proto: tcp
      rule_desc: "Rule created by Ansible from terraform_module_name"
     state: present
  rules_egress: [] 

Code of Conduct

alinabuzachis commented 2 years ago

@LawrenceWarren Thank you for reporting this. If I understood correctly your problem, setting purge_rules: true should solve your issue. When purge_rules: true, the old rules are removed and the new ones are applied. https://github.com/ansible-collections/amazon.aws/blob/main/plugins/modules/ec2_group.py#L186

Let me know please if this is what you meant.

LawrenceWarren commented 2 years ago

@alinabuzachis Thanks for the response. Unfortunately, purge_rules: true would not do as needed.

The Security Group already exists in AWS, with many other rules to other services attached. Setting purge_rules: true would delete all of these rules, which we don't want.

What this request is asking for is a way to delete specific rules, in the same way you can add specific rules one at a time using purge_rules: false.

ansibullbot commented 2 years ago

cc @adq @jillr @s-hertel @tremble click here for bot help

jillr commented 2 years ago

I definitely appreciate the problem here, I'm undecided on how we could best solve it though. The module code that manages rules in ec2_group is fairly complex already, and we've run into bugs in the past when trying to add seemingly small features. I do believe we have things stable now but generally I think we should remain thoughtful about overloading large modules. The EC2 API makes it very easy to end up with modules that try to do everything. :smile:

There's a few other routes we could take for more fine-grained rule management than what's currently available. We could add a new module to allow finer-grained management rules within a SG, though that feels a bit like overkill.

The ec2_group_info module can provide a list of all existing rules, which could then be manipulated to only contain the new desired rule state if that data isn't able to be obtained from some other source (vars, etc). Doing that directly in the playbook ends up looking like a lot of Jinja programming though which is suboptimal. A better option would be a filter plugin.

If someone has a reasonable implementation of something that solves for this use case it would be good to talk over it more in a review.

tremble commented 1 year ago

For what it's worth, I think the best solution would be to implement an ec2_security_group_rule module. As jillr mentioned ec2_security_group is already very complicated, and adding this functionality will likely make the module less reliable and harder to test.