Improve handling of security group changes

[cbarbour]

Terraform Version

• 0.8.4
• 0.8.1

Affected Resource(s)

• aws_instance
• aws_db_instance
• Others?

Probably not a core issue.

Terraform Configuration Files

resource "aws_security_group" "allow_all" {
  name = "allow_all"
  description = "Allow all inbound traffic"

  ingress {
      from_port = 0
      to_port = 0
      protocol = "-1"
      cidr_blocks = ["0.0.0.0/0"]
  }

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

provider "aws" {
    region = "us-west-2"
}

data "aws_ami" "ubuntu" {
  most_recent = true
  filter {
    name = "name"
    values = ["ubuntu/images/hvm-ssd/ubuntu-trusty-14.04-amd64-server-*"]
  }
  filter {
    name = "virtualization-type"
    values = ["hvm"]
  }
  owners = ["099720109477"] # Canonical
}

resource "aws_instance" "web" {
    ami = "${data.aws_ami.ubuntu.id}"
    instance_type = "t2.micro"
    security_groups = [ "${aws_security_group.allow_all.id}" ]
}

Steps to reproduce

1. Make a change to the security group that forces a new resource.
   • E.g. Change name/description

Expected Behavior

• In VPC, dependent resources should be modified with new Security group
• In EC2 classic, dependent resources should be recreated when appropriate. (E.g. aws_instances)

Actual Behavior

Varies by Terraform release

0.8.1

• aws_instances in VPC forced to be recreated when modification is appropriate
• aws_db_instance in VPC forced to be recreated when modification is appropriate (!)
• I haven't checked impact on ALB, ECS, etc.

0.8.4:

• Change ignored on aws_instances
• Security group removal/recreation fails due to resource dependencies
• aws_alb & aws_db_instance correctly marked for update

Important Factoids

0.8.1 behavior is dangerous and could result in unexpected data loss if user fails to check plan before applying change; user has no reason to expect changing a SG will destroy an EC2 instance.

It would have been good to track these changes with issues so that they appear in the changelog.

References

• db_instance: 1ff623a
• ec2_instance: 564dd36 & 65ff5b3
• alb: d7e9a2e & ec7ee63

[cbarbour]

efs_mount_target is also affected.

[catsby]

Hey @cbarbour – 

Regarding aws_instance resources: inside a VPC need to use the vpc_security_group_ids configuration option and reference the Security Groups by ID. For instances in EC2 Classic, users should use the security_groups option, and reference the security groups by name.

The behavior between the two environments is different; In Classic, you cannot change the security group without rebooting the instance. Because Terraform does not support rebooting instances, you get the behavior of destroying the instance and recreating it with a Security Group change.

VPC instances can have the groups changed without a reboot, so that operation is a simple update and does not require recreation.

• https://www.terraform.io/docs/providers/aws/r/instance.html#security_groups

I see that that security_groups is referencing a group by ID in the example above, suggesting to me that you're creating these resources in the default VPC, which has more lenient behavior and accepts security groups by name, but only if you are using the default VPC:

• http://docs.aws.amazon.com/AWSEC2/latest/APIReference/API_RunInstances.html (see comments on SecurityGroup)

The difference in behavior and the slight inconsistency in the API are two of the reasons they are split out into separate options.

The EC2 commit you reference, 564dd36 (#5193) , was reverted in e9c4d4f (#5571). The ChangeLog is maintained by humans so there’s always the chance of error, however the revert would explain why #5193 is not mentioned there.

I modified your example to use a VPC and use vpc_security_groups_ids (gist here: https://gist.github.com/catsby/8d8d5f70f7b3b6f60d0693a0b41e98b5), and ran terraform apply. After it successfully created, I then modified the Security Group description. My next terraform plan correctly showed a recreation of the Security Group, and a modification of the instance:

Refreshing Terraform state in-memory prior to plan...
The refreshed state will be used to calculate this plan, but will not be
persisted to local or remote state storage.

aws_vpc.test_vpc: Refreshing state... (ID: vpc-626de405)
data.aws_ami.ubuntu: Refreshing state...
aws_subnet.test_subnet: Refreshing state... (ID: subnet-d5dcf18d)
aws_security_group.allow_all: Refreshing state... (ID: sg-47cf473f)
aws_instance.web: Refreshing state... (ID: i-02312c5817450a32c)
The Terraform execution plan has been generated and is shown below.
Resources are shown in alphabetical order for quick scanning. Green resources
will be created (or destroyed and then created if an existing resource
exists), yellow resources are being changed in-place, and red resources
will be destroyed. Cyan entries are data sources to be read.

Your plan was also saved to the path below. Call the "apply" subcommand
with this plan file and Terraform will exactly execute this execution
plan.

Path: create.tfplan

~ aws_instance.web
    vpc_security_group_ids.#: "" => "<computed>"

-/+ aws_security_group.allow_all
    description:                         "Allow all inbound traffic" => "inbound traffic" (forces new resource)
    egress.#:                            "1" => "1"
    egress.482069346.cidr_blocks.#:      "1" => "1"
    egress.482069346.cidr_blocks.0:      "0.0.0.0/0" => "0.0.0.0/0"
    egress.482069346.from_port:          "0" => "0"
    egress.482069346.prefix_list_ids.#:  "0" => "0"
    egress.482069346.protocol:           "-1" => "-1"
    egress.482069346.security_groups.#:  "0" => "0"
    egress.482069346.self:               "false" => "false"
    egress.482069346.to_port:            "0" => "0"
    ingress.#:                           "1" => "1"
    ingress.482069346.cidr_blocks.#:     "1" => "1"
    ingress.482069346.cidr_blocks.0:     "0.0.0.0/0" => "0.0.0.0/0"
    ingress.482069346.from_port:         "0" => "0"
    ingress.482069346.protocol:          "-1" => "-1"
    ingress.482069346.security_groups.#: "0" => "0"
    ingress.482069346.self:              "false" => "false"
    ingress.482069346.to_port:           "0" => "0"
    name:                                "allow_all" => "allow_all"
    owner_id:                            "187416307283" => "<computed>"
    tags.%:                              "1" => "1"
    tags.Name:                           "tf-test-sg-change" => "tf-test-sg-change"
    vpc_id:                              "vpc-626de405" => "vpc-626de405"


Plan: 1 to add, 1 to change, 1 to destroy.

The same rational is applied to DB Instances; DB Instances in the Classic environment have specialized behavior and need to use the security_group_names for Classic env. databases, and databases in VPC’s need to use vpc_security_group_ids.

Regarding on v0.8.4 "Change ignored on aws_instances” can you clarify if you used the configuration above, and that you were changing the description on the Security Group?

It’s not clear to me how ALBs or EFS Mount targets are affected here, could you please clarify? Thanks!

[catsby]

Hey I'm going to close this for now. If you have any more information please let me know!

[cbarbour]

Sorry, I missed your previous response. I'll ensure that my configuration matches your comments, and update if I have any further problems.

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