aws_security_group: no way to delete egress rule

[lra]

Terraform Version

Terraform v0.9.2

Affected Resource(s)

aws_security_group

Terraform Configuration Files

resource "aws_vpc" "test-vpc" {
  cidr_block           = "172.20.0.0/16"
  enable_classiclink   = true
  enable_dns_support   = true
  enable_dns_hostnames = true
}

resource "aws_security_group" "test-sg" {
  name   = "test-sg"
  vpc_id = "${aws_vpc.test-vpc.id}"

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

Expected Behavior

When commenting out the egress rule, I except terraform to delete the egress rule on apply

Actual Behavior

When I comment out the egress rule, terraform applies no change.

Steps to Reproduce

1. terraform apply
2. comment out the egress section
3. terraform apply

Workaround

I can taint the security group and apply.

[grubernaut]

Hey @lra, thanks for the issue!

Thought I would add some color as to why this issue is happening. Inside the aws_security_group resource, both ingress and egress config blocks are Optional. Thus we don't manage security group rules if there are no inline configuration blocks, otherwise we'd have breaking behavior when users attempted to add security group rules via the aws_security_group_rule resource.

That being said, there could be a use case for adding an additional boolean parameter remove_all_rules for the aws_security_group resource, though I'm not sure it would be useful in all that many situations. Do you have a use case for removing all of the egress/ingress rules in a security group resource?

[lra]

Do you have a use case for removing all of the egress/ingress rules in a security group resource?

There could be many, an obvious one is to temporary quarantine the instances of a security group.

Terraform is a tool and should not dictate an opinion on things like this. If AWS allows empty security groups, tf should fully support them no?

[grubernaut]

Hey @lra, sorry for the late response to this!

There could be many, an obvious one is to temporary quarantine the instances of a security group.

Fully agreed!

I'm aiming to get to this by tomorrow, and should have a fix in place for you soon.

However, for clarification on:

If AWS allows empty security groups, tf should fully support them no?

Terraform definitely supports empty security groups! However, the issue stems from the face that both egress and ingress configuration blocks are Optional. So we don't manage a security group's rules if those configuration blocks are not specified. This allows users to use the aws_security_group_rule resource without fear that the aws_security_group resource will remove all of their security group rules on a subsequent apply. This means that we'll likely have to add another boolean parameter to the aws_security_group resource that will purge all of the rules attached to a security group resource. Thus, I had to verify that there's a definite need for adding the additional parameter. Hope this backstory helps, and sorry again for the delay in response.

[lra]

Sure

So we don't manage a security group's rules if those configuration blocks are not specified. This allows users to use the aws_security_group_rule resource without fear that the aws_security_group resource will remove all of their security group rules on a subsequent apply.

Why don't you threat aws_security_group_rule as actual ingress and egress properties?

I guess I don't know all the ins and outs, don't bother ;)

[apparentlymart]

@grubernaut this is not really a fully-baked idea, so I'm sure there are some implications of this that would be worth sussing out, but I wonder if we could have this resource leave a note for itself in a boolean computed attribute for whether it thinks it is managing the rules for a particular security group.

Assuming we had this new boolean attribute unmanaged_rules:

• During Create, do d.Set("unmanaged_rules", true) if there are no ingress or egress blocks.
• During Refresh, only update our cached copy of the rules if d.Get("unmanaged_rules").(bool) == false.
• During Update, always update the rules to match the config if d.Get("unmanaged_rules").(bool) == false, even if there are no ingress or egress blocks. Then, after doing that, update the unmanaged_rules flag for next time.

By maintaining this state in a special flag, we can then recognize the difference between "no rules and there never have been" and "no rules but there was before".

I think getting this done would require a state migration to populate a reasonable initial value for this new attribute, but otherwise it seems on the surface like it should work...

[meyertime]

My pull request https://github.com/hashicorp/terraform/pull/14332 would fix this. If an aws_security_group_rules resource is defined with no ingress and/or egress rules, it will enforce that.

[grubernaut]

@apparentlymart You're fairly close to the "proper" and "best" solution that @catsby and I have come up with through internal discussions. Also, thanks for chiming in here, always helps to have second opinions on these things 😺

The initial (intentionally basic) idea above, was to use the boolean remove_all_rules that defaults to false, and conflicts with both ingress and egress. Thus the user would need to explicitly state whether they want to remove all the rules for that resource. This would be an intentionally limited feature add, and it's small enough of a change to roll out prior to the next release.

The reason unmanaged_rules wouldn't be the most effective solution is that it would still leave users in a potentially bad state if they use the aws_security_group_rule resource, as that resource is extremely susceptible to leaking security group rules, and causing mistrust in users. @deftflux explains this situation expertly in their PR #14332.

In order to fully be able to manage all security group rules, we would need to be able to iterate through the entire state of all the aws_security_group_rule resources for this particular statefile. This will likely require some core enhancements, but it will also only help users who wish to configure the entirety of their aws_security_group + _rule resources in a single statefile. We would also gain the benefit of allowing users to specify inline rules in addition to rules via the aws_security_group_rule resource. This would be a huge win, in my opinion, and allow users to gain trust back in Terraform for managing Security Groups in AWS.

I'm imagining this new boolean attribute to be something along the lines of manage_rules. Another reason this is more appealing than the new resource that @deftflux intelligently wrote out, is it allows us to expand this core enhancement to be used in other resources that have this same issue.

@deftflux's solution, however, does help solve the issue for users who have multiple remote statefiles, as it forces the user to contain all of their security group rules in a single resource. However, this doesn't solve the issue for users who still wish to create a single security group in one statefile, and separate rules in multiple different statefiles.

@lra I'm not 100% sure what you mean by your question, I apologize, but hopefully it was answered above.

[meyertime]

Sorry to drop a link without offering any feedback; I just wanted to get some related issues related quickly.

Do you mean defining rules for the same security group in different modules that are deployed separately (with different .tfstate files)? Not that we should be opinionated in our tooling, but that would be a nightmare to maintain. Plus with the way AWS handles security group rules, I'm afraid aws_security_group_rule is the best we could ever hope to get as far as that goes, unless Amazon decides to make rules identifiable entities. It's more an issue with AWS than Terraform, the way I see it.

But if what you really mean is defining rules for the same security group in different .tf configuration files within the same module, just for code organization purposes, one possible solution that comes to mind would be to do something like C# "partial" classes. Basically, a resource could be declared "partial" (or something to that effect) such that it can be re-declared in other files, and Terraform would combine all the arguments together into one resource.

As far as resources knowing about each other, that would be a major paradigm shift in Terraform. Currently, resources are the main boundary. Modules are another higher-level boundary. We need to be able to see and control the ins and outs of those predictably. It might make more sense to introduce a new kind of section that would be associated with a resource where the resource could know about all of them. Then again, we basically already have that: arguments. We may just need more flexibility over how we define those arguments. (e.g. https://github.com/hashicorp/terraform/issues/14037)

As far as I am concerned, though, it makes the most sense to put all the security group rules for a particular group in one place. I work for a large company, and InfoSec has to review all of our security group changes. We are trying to implement a process where InfoSec can simply review a pull request of the Terraform configuration. The configuration has to appear very straight-forward for that to be practical. We can't have rules for the same group sprawled out all over the place.

[jzafran]

I've found a workaround; set egress = [] and the default rule will magically disappear after running terraform.

[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.