provider/aws: Support additional changes to security groups of instance without forcing new

[innossh]

With this pull request, security_groups can now be updated without forcing a new instance.

API reference for Modify Instance Attribute:
http://docs.aws.amazon.com/AWSEC2/latest/APIReference/API_ModifyInstanceAttribute.html

[description]
First apply the following sample config:

resource "aws_instance" "web" {
    ami = "ami-383c1956"
    instance_type = "t2.micro"
    security_groups = [
        "${aws_security_group.tf_test_foo.name}"
    ]
}

resource "aws_security_group" "tf_test_foo" {
  name = "tf_test_foo"
  description = "foo"

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

resource "aws_security_group" "tf_test_bar" {
  name = "tf_test_bar"
  description = "bar"

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

Next change the security_groups in aws_instance :

resource "aws_instance" "web" {
...
    security_groups = [
        "${aws_security_group.tf_test_bar.name}" # changed
    ]
}
...

In this case, terraform plan results into:

-/+ aws_instance.web
    ami:                        "ami-383c1956" => "ami-383c1956"
...
    security_groups.#:          "1" => "1"
    security_groups.1111111111: "" => "tf_test_bar" (forces new resource)
    security_groups.2222222222: "tf_test_foo" => ""
...

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

The instance will be destroyed when the re-creating is not necessary. 😞
But with this pull request, like this:

~ aws_instance.web
    security_groups.1111111111: "" => "tf_test_bar"
    security_groups.2222222222: "tf_test_foo" => ""


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

[innossh]

I want to change security_groups of instance without forcing new. 😃
Could anyone check this one? Thanks.

[levenaux]

This would be incredibly helpful. Never really understood why It destroys the instance just to change the security group.

[stack72]

@innossh thanks so much for the PR. Currently running all the Instance tests to make sure all is well. Pending those going green, will merge :)

[stack72]

make testacc TEST=./builtin/providers/aws TESTARGS='-run=TestAccAWSInstance_' 2>~/tf.log
==> Checking that code complies with gofmt requirements...
/Users/stacko/Code/go/bin/stringer
go generate $(go list ./... | grep -v /vendor/)
TF_ACC=1 go test ./builtin/providers/aws -v -run=TestAccAWSInstance_ -timeout 120m
=== RUN   TestAccAWSInstance_basic
--- PASS: TestAccAWSInstance_basic (207.34s)
=== RUN   TestAccAWSInstance_blockDevices
--- PASS: TestAccAWSInstance_blockDevices (148.30s)
=== RUN   TestAccAWSInstance_sourceDestCheck
--- PASS: TestAccAWSInstance_sourceDestCheck (273.32s)
=== RUN   TestAccAWSInstance_disableApiTermination
--- PASS: TestAccAWSInstance_disableApiTermination (194.67s)
=== RUN   TestAccAWSInstance_vpc
--- PASS: TestAccAWSInstance_vpc (157.30s)
=== RUN   TestAccAWSInstance_multipleRegions
--- PASS: TestAccAWSInstance_multipleRegions (167.49s)
=== RUN   TestAccAWSInstance_NetworkInstanceSecurityGroups
=== RUN   TestAccAWSInstance_NetworkInstanceVPCSecurityGroupIDs
--- PASS: TestAccAWSInstance_NetworkInstanceVPCSecurityGroupIDs (218.34s)
=== RUN   TestAccAWSInstance_tags
--- PASS: TestAccAWSInstance_tags (151.04s)
=== RUN   TestAccAWSInstance_privateIP
--- PASS: TestAccAWSInstance_privateIP (287.71s)
=== RUN   TestAccAWSInstance_associatePublicIPAndPrivateIP
--- PASS: TestAccAWSInstance_associatePublicIPAndPrivateIP (257.65s)
=== RUN   TestAccAWSInstance_rootBlockDeviceMismatch
--- PASS: TestAccAWSInstance_rootBlockDeviceMismatch (171.78s)
=== RUN   TestAccAWSInstance_forceNewAndTagsDrift
--- PASS: TestAccAWSInstance_forceNewAndTagsDrift (316.09s)

[phinze]

Hey folks, so I'm a little concerned about this change in relation to behavior on EC2 Classic. There's some legacy in the upstream APIs we need to treat properly.

So the RunInstances action has two attributes:

• The SecurityGroupId field maps to vpc_security_group_ids in Terraform, which can be updated w/o replacement just fine.
• The SecurityGroup field maps to security_groups in Terraform. Note that it says it only is meant to be used in either EC2 Classic or with a Default VPC.

Now if you check out ModifyInstanceAttribute:

We just have GroupId, which is marked as EC2-VPC only. The API does not seem to provide any way to properly update Classic SecurityGroups via security group names. It's for this reason that we had security_groups marked as ForceNew.

It looks like the strategy here is to convert SG names into SG IDs and drop them into the GroupId field, but I don't believe this is going to work properly in classic environments.

Indeed, our nightly acceptance tests and my local testing reveal this test consistently failing:

--- FAIL: TestAccAWSInstance_NetworkInstanceSecurityGroups (137.08s)
        testing.go:148: Step 0 error: Error applying: 1 error(s) occurred:

                * aws_instance.foo_instance: InvalidGroup.NotFound: The security group 'sg-157cf472' does not exist in default VPC 'vpc-531e1431'
                        status code: 400, request id:

I'd be happy to make this work more consistently if we can, but I think we need to double check the behavior in Classic environments before we release a change like this.

[innossh]

@phinze I haven't noticed about classic environments. 😢
Thank you for the information and your time.

[vitroth]

FYI, I noticed that while this PR got reverted, the changelog entry for this still exists in the changelog for 0.6.13: provider/aws: aws_instance now allows changes to security groups without force new resource (#5193)

[stack72]

Hi @vitroth

you are indeed correct - i will take care of that - thanks for pointing it out

Paul

[catsby]

Hey @cbarbour – I followed up in #11276 but wanted to post here as well that this change not appearing in the changelog is likely because it was reverted in #5571

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