New Resource: aws_security_group_rules

[meyertime]

This was originally over at hashicorp/terraform#14332 before builtin providers were split out. So here it is for the split AWS provider.

I wrote and ran acceptance tests and everything, so it is ready to merge. I could not, however, verify one classic (pre-VPC) test because our AWS account was created post-VPC and does not support the classic style.

The following is the original pull request write-up:

I am fixing a long-standing issue with Terraform managing AWS security group rules. Currently, there are two ways to configure security group rules, and they each have serious short-comings:

1. Define all rules inline within the aws_security_group resource.
   • AWS supports referring to other security groups within security group rules. If rules are defined inline, then a circular dependency is possible when two security groups refer to each other within their rules. Terraform cannot resolve this dependency issue. The only way to do this currently is to have at least one group in the circular dependency chain use the second approach rather than define inline rules.
   • If no ingress rules and/or no egress rules are defined inline, the aws_security_group resource will not manage one or both types of rules. In other words, there is no way to define that a security group should have no ingress rules and/or no egress rules. Upon applying, Terraform will not remove any of those rules.
2. Define all rules as separate aws_security_group_rule resources.
   • When defined this way, Terraform does not manage the entire security group as one would expect; it only manages any rules created by Terraform that exactly match what Terraform previously thought the rules were. So if a manual change is made to a security group outside of Terraform, such as by using the AWS console, Terraform will not correct that change properly. This fact makes this approach unacceptable from a security standpoint for managing security groups, since it does not ensure that the security groups in AWS exactly match the Terraform configuration.
     • The reason for this is because each security group rule resource only knows about itself; it does not know about the other security group rule resources, so it does not have the complete picture of all the rules that should appear in the group. Also, in AWS, security group rules are not entities with their own IDs, so they cannot be tracked if they are modified outside of Terraform. Therefore, none of the rule resources have enough information to know whether an existing rule in AWS does not match any rule resource and should thus be removed or replaced.
     • On a side note, each rule does have enough information to change or remove the exact rule it created when it was last applied, and it does so. However, only if that rule still exists exactly as it was. If it is tampered with in any way, the tampered rule will remain.
   • This approach has the same short-coming of not being able to define that there should be no ingress and/or no egress rules.
   • A minor disadvantage of this approach is that it makes the configuration rather verbose: every security group rule must be defined as a separate resource section and must separately specify the security group ID to which it belongs.

Now for my solution:

• Define all rules within a single aws_security_group_rules resource. (Note the s; that's "rules" plural.) This solution takes the best of the two approaches above and then some:
  • Because rules are defined in a separate resource, like aws_security_group_rule, there is no circular dependency in Terraform's dependency graph when rules from two security groups refer to each other. Terraform will simply create the security groups first, then create the rules.
  • Because all rules are defined within a single resource, like inline rules within aws_security_group, the resource has enough information to verify that the rules in AWS exactly match the rules defined in the configuration. Any manual changes done outside of Terraform will be undone.
  • Since this rules resource is for the purpose of managing the rules and, unlike aws_security_group, does not have to support other resources managing the rules instead, a rules resource that defines no ingress and/or no egress rules can be understood to mean that none should exist. This is an advantage over both previous approaches.
  • While slightly more verbose than aws_security_group, this solution would be much less verbose than having to define many aws_security_group_rule resources. Only one separate resource section with one specification of the security group ID is required to define the rules. Other than that, the syntax matches that of the inline rules within aws_security_group.

Yes, this involves adding yet another resource for defining AWS security group rules. But all three of these can still have a distinct purpose:

• aws_security_group is obviously necessary for defining a security group that is managed by Terraform. The optional ability to define security group rules inline may remain for those who never refer to other security groups within their rules.

• aws_security_group_rule would only be safely used for adding a rule to an existing security group while preserving any rules that already exist in that group that are somehow managed elsewhere. There may be some out there who might want to do this, but due to the short-comings of this approach, I would strongly recommend deprecating this, or at least pointing users to the new rules resource as the standard approach.

• aws_security_group_rules would be the standard approach for defining the exact set of rules that should belong to a given security group.

[joshuaspence]

Looking forward to this change

[hpAB]

hi,
can we have an example of usage of aws_security_group_rules

[meyertime]

Once this pull request is merged and the new version is released, the Terraform website will have examples of the new rules resource. But basically it is like aws_security_group_rule in that it has a security_group_id property to identify which security group the rules belong to, but then it is like aws_security_group in having ingress and egress sections to define the rules inline. For instance:

resource "aws_security_group_rules" "foo" {
    security_group_id = "${aws_security_group.my_security_group.id}"

    ingress {
        protocol = "tcp"
        from_port = 80
        to_port = 80
        cidr_blocks = [ "0.0.0.0/0" ]
    }

    egress {
        protocol = "tcp"
        from_port = 80
        to_port = 80
        cidr_blocks = [ "0.0.0.0/0" ]
    }
}

But once again, this pull request isn't merged yet, so at this point, this won't work unless you make your own build of the Terraform aws provider.

So feel free to pester HashiCorp to hurry up and click the "Merge" button.

[STRML]

This is more or less a blocker when attempting to import existing infrastructure into a TF config that uses aws_security_group_rule. Please merge.

[brandonivey]

Really need this. Security groups that reference each other directly is an absolute requirement, and the only reason we haven't migrated our SG creation to Terraform yet..

[meyertime]

@radeksimko It has been 5 months... can I get a review?

I know there is a merge conflict now, but only because it has taken this long... I'll resolve the conflict once I get a review.

[dstreby]

Are there any updates on this? Would love to see this merged.

[meyertime]

¯\_(ツ)_/¯

[jonlil]

It hurts to see code of this importance just being ignored! 😞 @radeksimko

[jechol]

Why not yet merged?

[the-real-cphillips]

This would be a HUGE help, any chance we can get some traction on this?
Nice work @deftflux

[brodygov]

👍 This continues to be a major problem with managing security groups via terraform.

[mcolley73]

Pretty please.

[jsnod]

👍 Badly need this...

[paultyng]

@deftflux we really appreciate the work done on this PR, and I know first hand about many of the rough edges around security groups. I apologize for the lack of an update here, especially with the amount of work you have put in.

We are waiting to see what kind of effects the improved Terraform 0.12 syntax (specifically for and for-each) has on solving issues with security groups. After 0.12, the plan is to take a step back and approach this more holistically to make sure we cover all the cases (circular referencing, self referencing, rule flattening / diffing) etc.

The current set of resources are a minefield of issues, you enumerate quite a few of them, but there are many more not covered in your write up, and we worry adding another model to define / manage rules just confuses the situation more. This resource may be the appropriate way to implement this, but we think that needs to be coupled with deprecating some of the other methods and covering all the cases with preferably a single design and we need to make sure we are considering all those possibilities (and the new iteration features) when we design it.

[meyertime]

@paultyng Thanks for the feedback! Yeah, I had a feeling the new HCL features had something to do with the delay here.

Unfortunately, from what I've read of the new features, I don't think that they alone will solve this issue. It would certainly make writing inline rules better, but would not solve the circular dependency problem with inline rules or the incomplete information problem with individual rule resources. Without significant changes to Terraform itself, I don't think there's any other way to provide a method that solves both problems. But as you alluded to, it doesn't cover all use cases, so we can't deprecate the other methods.

One or both problems would have to be solved with changes to Terraform in order to open up more possibilities here. So some way of resolving circular dependencies between resources (perhaps some kind of partial apply of a resource with known arguments followed by one or more additional applies as more arguments become known) and/or some way of sharing information between certain resources (perhaps the concept of some kind of sub-resource, where the parent resource is responsible for applying them and knows about all sub-resources).

Anyways, it's nice to know that it's on your radar at least. Looking forward to seeing what you come up with.

[nywilken]

Hi @deftflux @paultyng this is a great discussion, and I would like to follow up once Terraform 0.12 has been released. For now, I am going to add the terraform-0-12 label and close this pull request. If there are additional comments please feel free to add them to thread, but note that they may not be responded to until post Terraform v0.12.

Cheers

[brandonivey]

This sucks. Thanks for ignoring this awesome and necessary feature admins

[STRML]

Honestly, closing the PR is not a good way to say "we would like to follow up on this." Yeah, it's not a sexy feature, why it's needed is complicated, but it's absolutely necessary to actually manage SGs effectively. Why not keep it open, label it, and continue to talk about how it can be prioritized and get into the next release?

[jakauppila]

@nywilken @bflad Now that v0.12 is released, can this PR be re-opened for discussion/implementation? Most of our security group usage references other security groups, so it's kind of critical to have this functionality to be able to detect and remediate configuration drift.

[jakauppila]

I opened #9032 to continue the discussion with these changes against master.

[ghost]

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!