Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

aws_security_group: Deletes and recreates all group rules on changes #2382

Closed
stmarier opened this issue Nov 20, 2017 · 7 comments
Closed

aws_security_group: Deletes and recreates all group rules on changes #2382

stmarier opened this issue Nov 20, 2017 · 7 comments
Labels
enhancement Requests to existing resources that expand the functionality or scope. service/ec2 Issues and PRs that pertain to the ec2 service.
Milestone

Comments

@stmarier
Copy link

Terraform Version

2017/11/20 11:54:47 [INFO] Terraform version: 0.10.8
terraform-provider-aws_v0.1.4_x4

Affected Resource(s)

aws_security_group

Terraform Configuration Files

Unnecessary?

Debug Output

Here are the important parts:

2017-11-20T11:44:36.180-0800 [DEBUG] plugin.terraform-provider-aws_v0.1.4_x4: 2017/11/20 11:44:36 [DEBUG] Revoking security group {
[...]
2017-11-20T11:44:36.618-0800 [DEBUG] plugin.terraform-provider-aws_v0.1.4_x4: 2017/11/20 11:44:36 [DEBUG] [aws-sdk-go] DEBUG: Request ec2/AuthorizeSecurityGroupEgress Details:

Expected Behavior

Terraform should not delete every rule from a security group when the difference between desired state and actual state is only additive.

Actual Behavior

Terraform deletes and then recreates all configured rules on a security group when adding new rules.

Steps to Reproduce

Please list the steps required to reproduce the issue, for example:

  1. Create an aws_security_group resource with an ingress rule of 0.0.0.0/0
  2. Terraform apply
  3. set debug logging
  4. Add 1.1.1.1/32 to the cidr_blocks list
  5. Terraform apply
  6. Note that the existing ingress security group rule is deleted and then re-added.

Important Factoids

The important security group on our end controls access to a critical database. We cannot, in good conscience, embrace a tool that deletes rules from this security group every time we want to make a change.

There's a comment about this in resource_aws_security_group.go:

        // TODO: It'd be nicer to authorize before removing, but then we have
        // to deal with complicated unrolling to get individual CIDR blocks
        // to avoid authorizing already authorized sources. Removing before
        // adding is easier here, and Terraform should be fast enough to
        // not have service issues.

We have processes that monitor the state of these critical resources as part of various compliance controls, and having to explain the appearance of RevokeSecurityGroupIngress in our cloudtrail logs referencing these critical resources is not pleasant.

Given what happens here under the hood, I'm surprised this isn't mentioned in the documentation. If this were better communicated, we probably would have avoided this gotcha entirely by only ever using the aws_security_group_rule resource.

So, 2 feature requests:

  • The documentation should probably be updated to note the fact that, while temporary, every configured security group rule is deleted from this security group during every rule change.

  • It would be cool if Terraform could determine what operations need to happen here. The comment in the resource code seems a little naive (please correct me if I'm wrong) -- I don't feel like we need to choose between "add before remove" and "remove before add", shouldn't we be able to get the difference between current and desired and make changes based on that? If there is some fundamental problem, prioritizing the aforementioned "complicated unrolling" would be greatly appreciated.

@paddycarver paddycarver added the enhancement Requests to existing resources that expand the functionality or scope. label Nov 20, 2017
@radeksimko radeksimko added the service/ec2 Issues and PRs that pertain to the ec2 service. label Jan 28, 2018
@poblahblahblah
Copy link

This caused an outage for us yesterday. We had a long list of IP blocks to whitelist and there was a duplicate IP in the list. Terraform happily dropped all of the entries from the Security Group, but then bailed on recreating all of the entries because the API threw an error about the duplicate entry. The Security Group was empty while we scrambled to look up what the function to remove duplicate entries from a list or spot the offending duplicated entry.

For this case, and I suppose going forward, we are going to distinct() all lists for Security Groups, which is not quite ideal because it only prevents this one specific situation that dropping and recreating all of the entries adds risk of.

@foax
Copy link

foax commented Mar 27, 2018

I have noticed the same behaviour. Although the Revoke/Authorize is quick, it does leave a small period where a security group ruleset has no CIDR block associated with it.

Agree with @stmarier that a better approach would be to determine the differences between security group rules in current and desired state and either add or remove the differences, not remove all rules from current state and add all rules required for desired.

A workaround for the moment is to make the security group changes in the AWS console, then write Terraform config to match.

@lexelby
Copy link

lexelby commented Jun 13, 2018

This causes the output from terraform plan to be incredibly confusing and unintuitive. For anything but a basic security group, trying to figure out what's actually going to change by reading a terraform plan is nearly impossible.

@bflad bflad added this to the v1.27.0 milestone Jul 5, 2018
@bflad
Copy link
Contributor

bflad commented Jul 5, 2018

In #4726, which has been merged into master, the aws_security_group resource will now appropriately make only the necessary individual authorize/revoke rule updates, no matter the grouping within the Terraform configuration (e.g. adding an element to cidr_blocks). This will release with version 1.27.0 of the AWS provider, likely middle of next week. 👍

@bflad bflad closed this as completed Jul 5, 2018
@bflad
Copy link
Contributor

bflad commented Jul 11, 2018

This has been released in version 1.27.0 of the AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

@scalp42
Copy link
Contributor

scalp42 commented May 3, 2019

I'm pretty sure it's still broken.

@ghost
Copy link

ghost commented Mar 30, 2020

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!

@ghost ghost locked and limited conversation to collaborators Mar 30, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement Requests to existing resources that expand the functionality or scope. service/ec2 Issues and PRs that pertain to the ec2 service.
Projects
None yet
Development

No branches or pull requests

8 participants