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

resource/aws_instance: vpc_security_group_ids always showing update #1993

Closed
hashibot opened this issue Oct 20, 2017 · 18 comments · Fixed by #2338
Closed

resource/aws_instance: vpc_security_group_ids always showing update #1993

hashibot opened this issue Oct 20, 2017 · 18 comments · Fixed by #2338
Labels
bug Addresses a defect in current functionality. service/ec2 Issues and PRs that pertain to the ec2 service.
Milestone

Comments

@hashibot
Copy link

This issue was originally opened by @Yuxael as hashicorp/terraform#16411. It was migrated here as a result of the provider split. The original body of the issue is below.


Hi,
I have noticed lately that terraform wants to change instance every time I run terraform plan/apply. Specifically, it's showing that instance hasn't got any vpc_security_group_ids attached. I have set vpc_security_group_ids = ["${aws_security_group.external.id}"] in my tf file, terraform creates this security group, attach it to instance, etc., but for some reason in tfstate this information is saved as shown below:

  security_groups.# = 1
  security_groups.vvvvvvvvv = vanilla-stage_external
[...]
  vpc_security_group_ids.# = 0

Terraform Version

1.10.0 & 1.10.7
aws provider plugin: 1.0 & 1.1

Terraform Configuration Files

provider "aws" {
    region = "${var.ec2_region}"
    access_key = "${var.aws_access_key}"
    secret_key = "${var.aws_secret_key}"
}

resource "aws_security_group" "external" {
    name = "${var.project_name}_external" # vanilla-stage_external

    ingress {
        from_port = 0
        to_port = 0
        protocol = "-1"
        self = true
    }

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

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

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

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

resource "aws_instance" "app" {
    instance_type = "${var.ec2_instance_type}"
    ami = "${lookup(var.images, var.ec2_region)}"
    vpc_security_group_ids = ["${aws_security_group.external.id}"]
    key_name = "${var.project_name}"
    root_block_device = {
        volume_type = "gp2"
        volume_size = 40
    }
    tags = {
        Name = "${var.project_name}"
    }
}

Debug Output

https://gist.github.com/Yuxael/e39a652084695c90fde549a0dcd42640

Expected Behavior

terraform plan/apply should show no changes.

Actual Behavior

An execution plan has been generated and is shown below.
Resource actions are indicated with the following symbols:
  ~ update in-place

Terraform will perform the following actions:

  ~ aws_instance.app
      vpc_security_group_ids.#:         "0" => "1"
      vpc_security_group_ids.zzzzzzzzz: "" => "sg-yyyyyyyy"


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

Steps to Reproduce

  1. terraform init
  2. terraform plan
  3. terraform apply
  4. terraform plan/apply

Important Factoids

tfstate samle:

  security_groups.# = 1
  security_groups.vvvvvvvvv = vanilla-stage_external
...
  vpc_security_group_ids.# = 0
@abinashmeher999
Copy link
Contributor

@Yuxael Could you try adding vpc_id to your resource "aws_security_group" "external" and see if the problem persists. Seems like the security group you are creating isn't getting associated to any vpc.

One more thing you would want to verify is that you have a default VPC and a default subnet defined in your account.

@Yuxael
Copy link

Yuxael commented Oct 23, 2017

@abinashmeher999 If you look at the debug log you can clearly see that AWS api returns all the informations related to VPC. My account supports only VPC instances, there is only one VPC in that region and it's set as default, security group is associated to this VPC, etc. Also terraform show displays vpc_id in SG external. Anyway, I tried adding vpc_id, and result is the same.

data "aws_vpc" "default" {
  default = true
}

resource "aws_security_group" "external" {
    name = "${var.project_name}_external"
#    vpc_id = "${data.aws_vpc.default.id}"
    vpc_id = "vpc-xxxxxxxx"

None of the above worked.

@sdot257
Copy link

sdot257 commented Oct 23, 2017

I'm running into the same issue.

main.tf

resource "aws_instance" "instance" {
  count                   = "${var.num_instances}"
  ami                     = "${var.image_id}"
  instance_type           = "${var.instance_type}"
  key_name                = "${var.key_name}"
  subnet_id               = "${var.subnet_id}"
  vpc_security_group_ids  = ["${var.security_groups}"]

tfstate file

"security_groups.#": "1",
"security_groups.1983511901": "Prod-01-PriWebSec-XXXXXXX",
"vpc_security_group_ids.#": "0"

@Yuxael
Copy link

Yuxael commented Oct 31, 2017

There seems to be PR #1911 which is supposed to fix this, but IMHO it's more of a workaround than fix, as it just changes what is saved in tfstate in security_groups - security groups ids instead of names. This should be clarified and clearly stated, that security_groups is used with EC2-Classic or non-VPC instances, otherwise vpc_security_group_ids is mandatory.

@WheresWardy
Copy link

So does #1911 imply that we should be using a security_groups block instead? When using a version of the provider >= 1.0, and doing a terraform refresh, the vpc_security_group_ids blocks in the tfstate become security_groups blocks that require a name instead of ID, but that seems against the documentation and common sense in terms of the usage of those two blocks? This is kind of a major stumbling block from us moving onto a newer version of the provider than 0.1.4 right now.

@Yuxael
Copy link

Yuxael commented Nov 2, 2017

My guess is that the author if that PR just made a quick-fix for #1799 that seems to work without actually understanding what's the root of the problem. As you already noticed, it's against everything stated either in docs or by TF guys. There is a very nice explanation how it should work and what are the plans to fix this mess.
hashicorp/terraform#6416 (comment)

idubinskiy added a commit to idubinskiy/terraform-provider-aws that referenced this issue Nov 17, 2017
…urity_group_ids

Unlike instances launched into a default VPC are allowed to refer to their security groups by ID, like other VPCs, or by name, like EC2-Classic. The current implementation of the function that reads instance security group data assumes that instances in a default VPC refer to their security groups by name. This causes an instance resource that is launched in a default VPC but using the `vpc_security_group_ids` parameter (instead of the `security_groups` parameter) to always have a diff in plans post-creation showing that the security groups need to be added to it.

This commit changes the behavior of the function that reads instance security data to be able to store BOTH the security group names and IDs in the case of an instance in a default VPC. Because both the `security_groups` and `vpc_security_group_ids` parameters are marked as "computed", it's valid to provide either in the resource configuration even though both will end up in state.

This commit also adds a failing test for the case of using `vpc_security_group_ids` with a default VPC, and ensures that both default VPC import tests are run in a region with a default VPC (which specifically must _not_ be an EC2-Classic region).

Fixes hashicorp#1799
Fixes hashicorp#1993
@dhinus
Copy link
Contributor

dhinus commented Dec 5, 2017

I'm having the same issue, but I found out that it only happens in a region where I have a Default VPC, but no Default Subnet. This seems to confuse Terraform, which requires a security group ID to create an instance, but then (after a refresh) writes a security group NAME to the .tfstate file.

In regions where I have both a Default VPC and Default Subnets, it's working correctly: I can specify a security group NAME and the NAME is written to the .tfstate file.

@WheresWardy
Copy link

I'm not sure if that's the same issue; in the VPCs in which I have this issue, my VPCs have default subnets. And I'm not sure if terraform should be dependent on resources that aren't required existing (we have lots of accounts that don't have a "Default VPC" for example).

The problem here is that I don't want security group names written to the tfstate file at all; the existing vpc_security_group_ids should be maintained, and not replaced by security_groups with security group names in them.

@dhinus
Copy link
Contributor

dhinus commented Jan 5, 2018

Apologies for the long delay -- I have done some further investigation.

It is indeed the same issue, but adding a specific subnet_id to the aws_instance resource leads to some subtle differences.

Scenario 1: default subnet of a default VPC

The following works for me:

resource "aws_security_group" "sg" {
  name = "test-sg"
}

resource "aws_instance" "instance" {
  ami = "xxxxxx"
  instance_type = "t2.nano"
  key_name = "my-key"

  security_groups = ["${aws_security_group.sg.name}"]
}

Note that I am using the name of the SG and I am omitting the subnet_id argument.

This is what the docs used to recommend until May 2016:

security_groups - (Optional) A list of security group names to associate with.
If you are within a non-default VPC, you'll need to use vpc_security_group_ids instead.

The docs have changed in this commit and now state:

If you are creating Instances in a VPC, use vpc_security_group_ids instead.

cc-ing @catsby who authored that change, he might be able to provide some additional insight.

Scenario 2: non-default subnet of a default VPC

Trying to use security_groups as above, but adding the subnet_id argument to create the instance in a specific subnet of the same default VPC does not work, causing the following error on apply:

* aws_instance.instance: Error launching source instance: InvalidGroup.NotFound: The security group 'test-sg' does not exist in VPC 'vpc-******'
        status code: 400, request id: 4547c5d3-576f-4c19-8318-29f8a406052a

You might thing that in this scenario you should then use vpc_security_groups_id. Doing that lets you create the instance just fine, but you end up with the issue described by the OP, as Terraform thinks that because we are in a default VPC, SG names should be stored in tfstate, not IDs. It should also check if the instance is in a default subnet, before jumping to that conclusion. Or perhaps store both the names and the IDs in the tfstate, as implemented in #2338.

Scenario 3: non-default subnet of non-default VPC

In this scenario, using vpc_security_group_ids works fine!

dhinus added a commit to buildo/terraform-aws-cranehost that referenced this issue Jan 5, 2018
If no subnet_id is specified and a default subnet exists in the Default
VPC, the instance will be automatically created in the default subnet.

Using SG names instead of IDs should also resolve the issue where
`terraform plan` is not clean after a cranehost is created.

See this issue for additional details: hashicorp/terraform-provider-aws#1993
@bflad bflad added the service/ec2 Issues and PRs that pertain to the ec2 service. label Jan 16, 2018
@bflad
Copy link
Contributor

bflad commented Jan 17, 2018

Hi everyone! Sorry you have been having trouble with this aws_instance bug relating to vpc_security_group_ids. It turns out this has been reported and discussed in quite a few separate issues (#1445, #1799, #1993, #2034, #2036, #2319). In order to consolidate efforts, I have closed and commented on the other issues to tell everyone to vote and follow this issue.

Due to the high volume of reports surrounding this, the maintainers will be looking into this sometime in the near future (including the already open PRs: #1911, #2338). There are some nuances around this configuration that make it harder than a quick fix and we certainly do not want to make the situation worse. We'll keep you updated.

@bflad bflad changed the title Terraform saves vpc_security_group_ids as security_groups in tfstate resource/aws_instance: vpc_security_group_ids always showing update Jan 17, 2018
@nodesocket
Copy link

👍 @bflad. Look forward to a fix. Cosmetic annoyance.

@bflad bflad added this to the v1.9.0 milestone Jan 29, 2018
@bflad
Copy link
Contributor

bflad commented Jan 31, 2018

For watchers of all the issues and two PRs here, we are very sorry this got lost in the shuffle. The first PR likely should have been reviewed and potentially merged long ago. Our apologies on dropping the ball here. We have been working really hard to get the backlog under control and organized the last few weeks (e.g. you will notice a lot more repository labels now available and on every open issue so hopefully people can find related issues easier).

After some extensive acceptance testing we believe that #2338 will properly handle the issue here, which will be merged with an additional testing commit from me. This will be released in v1.9.0 of the AWS provider, which currently is planned for next week.

For those that are able, we would greatly appreciate some additional community pre-release testing to ensure this is backwards compatible with all the configurations out in the wild. Instructions for building this provider yourself and installing it manually are documented in the README.

Tests passed: 54
=== RUN   TestAccAWSInstanceDataSource_keyPair
--- PASS: TestAccAWSInstanceDataSource_keyPair (108.91s)
=== RUN   TestAccAWSInstance_importInEc2Classic
--- PASS: TestAccAWSInstance_importInEc2Classic (116.60s)
=== RUN   TestAccAWSInstanceDataSource_SecurityGroups
--- PASS: TestAccAWSInstanceDataSource_SecurityGroups (121.15s)
=== RUN   TestAccAWSInstance_importInDefaultVpcBySgId
--- PASS: TestAccAWSInstance_importInDefaultVpcBySgId (148.90s)
=== RUN   TestAccAWSInstanceDataSource_rootInstanceStore
--- PASS: TestAccAWSInstanceDataSource_rootInstanceStore (153.50s)
=== RUN   TestAccAWSInstanceDataSource_privateIP
--- PASS: TestAccAWSInstanceDataSource_privateIP (153.59s)
=== RUN   TestAccAWSInstanceDataSource_AzUserData
--- PASS: TestAccAWSInstanceDataSource_AzUserData (160.89s)
=== RUN   TestAccAWSInstanceDataSource_gp2IopsDevice
--- PASS: TestAccAWSInstanceDataSource_gp2IopsDevice (193.66s)
=== RUN   TestAccAWSInstanceDataSource_tags
--- PASS: TestAccAWSInstanceDataSource_tags (197.20s)
=== RUN   TestAccAWSInstance_noAMIEphemeralDevices
--- PASS: TestAccAWSInstance_noAMIEphemeralDevices (87.36s)
=== RUN   TestAccAWSInstance_ipv6AddressCountAndSingleAddressCausesError
--- PASS: TestAccAWSInstance_ipv6AddressCountAndSingleAddressCausesError (42.91s)
=== RUN   TestAccAWSInstance_blockDevices
--- PASS: TestAccAWSInstance_blockDevices (131.39s)
=== RUN   TestAccAWSInstance_GP2IopsDevice
--- PASS: TestAccAWSInstance_GP2IopsDevice (253.93s)
=== RUN   TestAccAWSInstanceDataSource_VPC
--- PASS: TestAccAWSInstanceDataSource_VPC (269.38s)
=== RUN   TestAccAWSInstance_basic
--- PASS: TestAccAWSInstance_basic (289.08s)
=== RUN   TestAccAWSInstance_GP2WithIopsValue
--- PASS: TestAccAWSInstance_GP2WithIopsValue (183.99s)
=== RUN   TestAccAWSInstanceDataSource_VPCSecurityGroups
--- PASS: TestAccAWSInstanceDataSource_VPCSecurityGroups (306.89s)
=== RUN   TestAccAWSInstance_vpc
--- PASS: TestAccAWSInstance_vpc (199.26s)
=== RUN   TestAccAWSInstance_importBasic
--- PASS: TestAccAWSInstance_importBasic (401.54s)
=== RUN   TestAccAWSInstance_multipleRegions
--- PASS: TestAccAWSInstance_multipleRegions (207.27s)
=== RUN   TestAccAWSInstance_disableApiTermination
--- PASS: TestAccAWSInstance_disableApiTermination (304.20s)
=== RUN   TestAccAWSInstance_ipv6_supportAddressCountWithIpv4
--- PASS: TestAccAWSInstance_ipv6_supportAddressCountWithIpv4 (223.56s)
=== RUN   TestAccAWSInstance_userDataBase64
--- PASS: TestAccAWSInstance_userDataBase64 (508.39s)
=== RUN   TestAccAWSInstanceDataSource_basic
--- PASS: TestAccAWSInstanceDataSource_basic (533.63s)
=== RUN   TestAccAWSInstance_withIamInstanceProfile
--- PASS: TestAccAWSInstance_withIamInstanceProfile (182.97s)
=== RUN   TestAccAWSInstance_NetworkInstanceSecurityGroups
--- PASS: TestAccAWSInstance_NetworkInstanceSecurityGroups (302.88s)
=== RUN   TestAccAWSInstance_associatePublicIPAndPrivateIP
--- PASS: TestAccAWSInstance_associatePublicIPAndPrivateIP (104.52s)
=== RUN   TestAccAWSInstancesDataSource_tags
--- PASS: TestAccAWSInstancesDataSource_tags (554.95s)
=== RUN   TestAccAWSInstancesDataSource_basic
--- PASS: TestAccAWSInstancesDataSource_basic (562.99s)
=== RUN   TestAccAWSInstance_volumeTagsComputed
--- PASS: TestAccAWSInstance_volumeTagsComputed (277.26s)
=== RUN   TestAccAWSInstance_ipv6_supportAddressCount
--- PASS: TestAccAWSInstance_ipv6_supportAddressCount (378.17s)
=== RUN   TestAccAWSInstanceDataSource_blockDevices
--- PASS: TestAccAWSInstanceDataSource_blockDevices (579.96s)
=== RUN   TestAccAWSInstance_instanceProfileChange
--- PASS: TestAccAWSInstance_instanceProfileChange (330.84s)
=== RUN   TestAccAWSInstance_keyPairCheck
--- PASS: TestAccAWSInstance_keyPairCheck (194.83s)
=== RUN   TestAccAWSInstance_changeInstanceType
--- PASS: TestAccAWSInstance_changeInstanceType (175.72s)
=== RUN   TestAccAWSInstance_associatePublic_defaultPublic
--- PASS: TestAccAWSInstance_associatePublic_defaultPublic (143.11s)
=== RUN   TestAccAWSInstance_associatePublic_explicitPublic
--- PASS: TestAccAWSInstance_associatePublic_explicitPublic (176.69s)
=== RUN   TestAccAWSInstance_rootBlockDeviceMismatch
--- PASS: TestAccAWSInstance_rootBlockDeviceMismatch (291.97s)
=== RUN   TestAccAWSInstance_associatePublic_defaultPrivate
--- PASS: TestAccAWSInstance_associatePublic_defaultPrivate (224.63s)
=== RUN   TestAccAWSInstance_privateIP
--- PASS: TestAccAWSInstance_privateIP (401.37s)
=== RUN   TestAccAWSInstance_associatePublic_explicitPrivate
--- PASS: TestAccAWSInstance_associatePublic_explicitPrivate (226.95s)
=== RUN   TestAccAWSInstance_sourceDestCheck
--- PASS: TestAccAWSInstance_sourceDestCheck (673.07s)
=== RUN   TestAccAWSInstance_associatePublic_overridePrivate
--- PASS: TestAccAWSInstance_associatePublic_overridePrivate (208.84s)
=== RUN   TestAccAWSInstance_associatePublic_overridePublic
--- PASS: TestAccAWSInstance_associatePublic_overridePublic (231.08s)
=== RUN   TestAccAWSInstance_forceNewAndTagsDrift
--- PASS: TestAccAWSInstance_forceNewAndTagsDrift (367.12s)
=== RUN   TestAccAWSInstance_addSecondaryInterface
--- PASS: TestAccAWSInstance_addSecondaryInterface (343.60s)
=== RUN   TestAccAWSInstance_tags
--- PASS: TestAccAWSInstance_tags (655.37s)
=== RUN   TestAccAWSInstance_rootInstanceStore
--- PASS: TestAccAWSInstance_rootInstanceStore (825.92s)
=== RUN   TestAccAWSInstance_primaryNetworkInterfaceSourceDestCheck
--- PASS: TestAccAWSInstance_primaryNetworkInterfaceSourceDestCheck (398.48s)
=== RUN   TestAccAWSInstance_volumeTags
--- PASS: TestAccAWSInstance_volumeTags (660.88s)
=== RUN   TestAccAWSInstance_addSecurityGroupNetworkInterface
--- PASS: TestAccAWSInstance_addSecurityGroupNetworkInterface (420.18s)
=== RUN   TestAccAWSInstance_importInDefaultVpcBySgName
--- PASS: TestAccAWSInstance_importInDefaultVpcBySgName (993.69s)
=== RUN   TestAccAWSInstance_NetworkInstanceVPCSecurityGroupIDs
--- PASS: TestAccAWSInstance_NetworkInstanceVPCSecurityGroupIDs (779.25s)
=== RUN   TestAccAWSInstance_primaryNetworkInterface
--- PASS: TestAccAWSInstance_primaryNetworkInterface (524.46s)

@bflad
Copy link
Contributor

bflad commented Feb 9, 2018

This has been released in terraform-provider-aws version 1.9.0. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

@webertrlz
Copy link

Still happening here:

Terraform v0.11.13
+ provider.aws v2.8.0
+ provider.template v2.1.1

@abdelhegazi
Copy link

abdelhegazi commented May 23, 2019

Hey Brian @bflad
I am not sure if this issue has been resolved by now as your message wasn't clear to me but we are still having same issue here

$ terraform version
Terraform v0.11.13
+ provider.aws v2.11.0
+ provider.null v2.1.2
+ provider.template v2.1.2

and everytime we run terraform plan/apply this is what we see and it causes the recreation of the instance


      root_block_device.#:          "1" => <computed>
      security_groups.#:            "0" => "1" (forces new resource)
      security_groups.1165462307:   "" => "sg-000a1dee2ec28fdf6" (forces new resource)

@abdelhegazi
Copy link

abdelhegazi commented May 23, 2019

[UPDATE]
using vpc_security_group_ids instead of security_groups is definitely FIXING the problem
👍 👍 👍
Actually this is has been pointed to at the resource documentation but it really needs to be highlighted more than this @bflad https://www.terraform.io/docs/providers/aws/r/instance.html#vpc_security_group_ids

resource "aws_instance" "hsm_client" {
  ami             = "${data.aws_ami.amazon-linux-2.id}"
  instance_type   = "t2.medium"
  key_name        = "${var.hsm_client_keypair}"
  subnet_id       = "${element(var.pub_nets, count.index)}"
  user_data       = "${data.template_file.client_user_data.rendered}"
  # security_groups = ["${aws_security_group.sg_hsm_client.id}"]
  vpc_security_group_ids = ["${aws_security_group.sg_hsm_client.id}"]

  tags = {
    Name = "HSM Client"
  }

And this is working very well with me

@atoledo
Copy link

atoledo commented Jun 6, 2019

Still reproducible in Terraform v0.12.0, not sure if it's because I don't have a default VPC in my account.

@ghost
Copy link

ghost commented Nov 3, 2019

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 Nov 3, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Addresses a defect in current functionality. service/ec2 Issues and PRs that pertain to the ec2 service.
Projects
None yet