Feature Request: Support reusable aws_security_group_rule resources

[rafikk]

Apologies if this has already been discussed, but a search didn't turn up anything relevant.

I'd like to use Terraform to provision security groups from a set of composable rules. E.g., I'd like to have rules that, e.g., allow SSH ingress, allow HTTP ingress, allow access from a known trusted IP, and then build security groups by combining different sets of rules.

I've been using aws_security_groups to that effect by defining narrow security groups that just have a single ingress rule, e.g.

resource "aws_security_group" "allow_ingress_http" {
  name = "allow_ingress_http"
  description = "Security group to allow HTTP access on the default port 80"
  vpc_id = "${aws_vpc.main.id}"

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

and then combining the desired security groups on the instances. Like this:

resource "aws_instance" "nginx" {
  ...
  vpc_security_group_ids = [
    "${aws_security_group.allow_egress_all.id}",
    "${aws_security_group.allow_ingress_ssh.id}",
    "${aws_security_group.allow_ingress_http.id}",
    "${aws_security_group.nginx.id}",
  ]
  ...
}

This was working moderately well until I hit the AWS limit on max number of security groups per instance. (I know I can request a limit increase from AWS and I have).

Meanwhile, aws_security_group_rule is just sitting there wanting to be the answer to this problem. My proposal is to deprecate the security_group_id parameter and move the rule specification to the aws_security_group resource. I'd like to be able to do this or something like it:

resource "aws_security_group_rule" "allow_egress_all" {
  type = "egress"
  cidr_blocks = ["0.0.0.0/0"]
  from_port = 0
  to_port = 0
  protocol = "-1"
}

resource "aws_security_group_rule" "allow_ingress_http" {
  type = "ingress"
  from_port = 80
  to_port = 80
  protocol = "tcp"
}

resource "aws_security_group_rule" "allow_ingress_ssh" {
  type = "ingress"
  from_port = 22
  to_port = 22
  protocol = "tcp"
}

resource "aws_security_group" "nginx" {
  ...
  rules = [
    "${aws_security_group_rule.allow_egress_all}",
    "${aws_security_group_rule.allow_ingress_http}",
    "${aws_security_group_rule.allow_ingress_ssh}"
  ]
  ...
}

resource "aws_instance" "nginx" {
  ...
  vpc_security_group_ids = [
    "${aws_security_group.nginx.id}",
  ]
  ...
}

This has the added benefit of clarifying that rules is mutually exclusive with ingress and egress blocks.

An alternative is to add security_group_ids in addition to security_group_id and make the many-to-one specification there.

I understand the complications of implementing this given the current Terraform architecture, but is there any disagreement that this is superior to the status quo?

[phinze]

Hi @rafikk - thanks for the well-explained use case!

Adding a rules collection to aws_security_group definitely reads nicely, but there were several motivations for migrating rule to a top level resource that this would undo:

• Making the addition of of a rule its own node in the dependency graph can prevent cycles Security Groups need to be able to depend on each other #539
• Separate rule resources allow rules to be added to an SG after its creation

Making a rules attribute requires that all the rules be known at SG creation time, and prevents a separate module from adding its own rules.

So perhaps the security_group_ids alternative is preferable, but you lose a bit of the nice readability there. Tagging so we can think about it a bit.

[rafikk]

Thank you for the prompt response @phinze! Like I said, I think the rules collection is more elegant, but I'm happy to settle for either/any solution to the problem.

A digression regarding the background on aws_security_group_rule, though. I've read through the issue previously and thought then that the approach chosen was a bit of a hack. It is definitely a case where the implementation leaks into the grammar. It's simply a statement of fact that some circular dependencies are valid and that the assumptions of the dependency graph are incomplete. Security groups are just one instance of such a resource. Creating a new resource type every time a circular dependency needs arbitrating isn't a sustainable practice. E.g., why not create an aws_instance_tag resource type? Maybe I want one of my tags to depend on something that depends on the instance?

Regarding your second point, aws_security_group allows ingress/egress rules to be modified after creation without a destroy/create step, so the rule resource type doesn't add any additional benefit there.

(I'm happy to write up my thoughts on what I believe to be a better general solution, but it's a bit out of scope for this issue. I'll open up another issue later).

Back to the matter at hand: the ironic thing here is that aws_security_group_rule does have semantic significance despite its sordid history. It's an incredibly useful concept as a reusable component of security groups. It's kind of the worst of both worlds right now: it offers no additional capabilities beyond ingress/egress attributes and pollutes the DSL with the shortcomings of the implementation.

[willmcg]

I've run into the same issue with the new aws_security_group_rule usage constraints. Having a 1-to-1 mapping of a rule to a security group is really blowing out my configuration unnecessarily with redundant rule resources because (as @rafikk says) I have to create replicated rules for each security group and cannot re-use common rules across different security groups.

Breaking out rules into top level resources seems to be a reasonable compromise to achieve the stated goals without having to introduce a bunch of automatic "magic" that would construct a non-cyclic DAG to eliminate cycles. I don't mind having the resource hierarchy be explicitly acyclic by design if it reduces implementation complexity. It would be nice to have the security groups specify a list of rule resources but I can see where that presents problems with cyclic dependencies in the resource hierarchy.

Maybe there is a middle ground compromise that meets the goals?

A couple of approaches that come to mind:

1. Allow the aws_security_group_rule resource to provide a list of security groups to which it will be applied. This has the pro of keeping the acyclic property of the design but the con that a low level rule resource needs to specify all its attachments to higher level security group resources which is backwards to the way you would normally think about things.
2. As per some other AWS resource types, create an intermediate security group association resource that associates a security group with a set of security group rules. This has the pros of preserving the acyclic property as well as allowing re-use of common rule resources without explicit references from the rules to security group resources. The downside is that we get another resource type.

For the current implementation of the aws_security_group_rule resource, it is unfortunate that we have propagated the "source_security_group_id" naming from the AWS APIs which is historical from the days before VPCs where there were only ingress rules. Now VPCs have security groups that allow egress rules and this naming is unnecessarily confusing for new users as it actually refers to the destination security group. With a separate association resource, the security_group_id would refer to the source/destination group for the rule rather than defining attachment to a security group and the source_security_group_id would go away.

[rafikk]

@willmcg 👍 I would support either of those compromises.

I haven't had a chance to write up my thoughts regarding breaking cyclic dependencies and don't want to blow the scope of this issue out of proportion. There have been a few practical solutions suggested in this thread that I think would make Terraform more useful for defining security groups.

[rhartkopf]

Thanks all for not settling for an "okay" solution. Forgive my general ignorance of programming logic, but could evaluation of rule relationships be deferred until after creation of all security groups? I'm not super familiar with Go but I know it does have a 'defer' statement for purposes such as this... though now that I recall, any variables in that statement might be evaluated at the time 'defer' is called, which would put us right back where we started with cyclic dependency errors.

It would be lovely if a solution such as @willmcg 's second suggestion could be implemented, but without having an explicit intermediate resource type. If you have a separate function that defines a relationship between "$aws_security_group.my_group" and "$aws_security_group.my_group.rules", it could act as an invisible resource type, but the relationship could still be defined in the aws_security_group config as @rafikk proposed.

[phinze]

Hey folks, great conversation here.

In general my strategy is:

(1) First prefer explicit, verbose, and flexible modelling
(2) Then layer on syntactic sugar and convenience features to make configs easier to read and write

To that end, I'd actually consider the current implementation of the sub-resource syntax of AWS security group rules a legacy implementation. I'm planning to circle back to convert that syntax to sugar for top-level resources, but we'll have to write state migration code to maintain BC.

As for SG rule reuse, I'm leaning towards @willmcg's approach #2.

Creating "yet another top level resource" for now for the association retains cross-module flexibility while supporting the SG rule reuse use case. Picture a security team publishing a module of "approved rules" that application infrastructure authors could attach to their security groups to.

With the feature I'm planning called "nestable resources" I believe we should be able to allow the config to express the relationship in any direction while retaining the proper dependency ordering.

I'll file an issue soon with my proposal for the "nestable resources" provider feature that would clean this up, but in the meantime I think its important for us to prefer proper dependency modelling over concise configs.

[rafikk]

Nested resources is exactly what I had in mind when I opened this issue. The ingress and egress blocks inside of aws_security_group already have a resource representation in the aws_security_group_rule resource. Terraform should recognize them as such. Happy to weigh in on the proposal once you've opened the issue.

In the meantime, seems like we're converging on @willmcg second approach. I just want to clarify that if we do implement that approach, it should support a set of associated rules and not be required for every combination of rule+security group.

[rhartkopf]

I'd be very happy with association resources with sets of rules. The config verbosity will be balanced by not having to declare otherwise identical rules for each security group, so for now it will be a form/function win-win in most environments.

@phinze I'm looking forward to seeing the nestable resources feature.

[eedwardsdisco]

@phinze +1 to supporting reusable rules through an association resource

my rule files are sooooo verbose... :(

It's really hard to manage, and tons of unnecessary duplication, as per the reasons listed above.

[jleclanche]

I found this issue while looking at aws_security_group_rule usage. The way things are laid out right now are really just far too verbose =/

@phinze you mentioned over a year ago you'd be filing a nestable resource provider feature. What came out of this? Is there any hope of improving the state of security groups? It feels silly that it's not possible to create reusable rules that we can apply to our servers. The super-basic use case is, "Create a rule for ssh, apply to two servers, create a rule for http/https, apply to only one of the two servers", and that's not supported without duplication.

Edit: Looks like some of the issue is due to #1874 too.

[apparentlymart]

Thank you to @rafikk for opening this feature request and to everyone else for the great discussion that followed.

Improving the security group declaration approach would be awesome, but at this time we (the Terraform team) don't have any plans to move forward with this. Priorities may change in future, but for now we suggest to use the aws_security_group_rule resource (accepting that it has some limitations as discussed here) and implement re-usable security group rules using Terraform's "module" concept:

module "ssh_access" {
  source = "./security-rules/ssh" # or could be in a separate repo to re-use across projects

  security_group_id = "${aws_security_group.example.id}"
}

Then in security-rules/ssh:

variable "security_group_id" {}

resource "aws_security_group_rule" "ssh" {
  type        = "ingress"
  from_port   = 22
  to_port     = 22
  protocol    = "tcp"
  cidr_blocks = ["10.0.0.0/8"]

  security_group_id = "${var.security_group_id}"
}

We understand that this is not a super-elegant solution, but it's something we can achieve with Terraform's current primitives to tide us over until such time as we find a more appropriate way to represent these relationships.

We're trying to close out some of these older, broad enhancement issues that don't have a concrete action plan. That doesn't mean that something like described here will never happen, but we want to be honest about the fact that there are no immediate plans to work in this area.

Thanks again to everyone who contributed here! This discussion will be archived as a great reference for future discussion in this area.

[kitchen]

chiming in here because I'm asking the same question:

I feel like this needs to be done at the aws level. IAM instance roles and security groups to me are mostly 1:1, and IAM has managed policies you can declare and attach to roles, it would be nice if they had the same thing for security groups.

Can fake that at the terraform level but that's a lot harder to reason about, especially when thinking about importing, which, with security groups, is already a bit of a ... struggle :)

[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 have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.