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_instance associate_public_ip_address #227

Closed
hashibot opened this issue Jun 13, 2017 · 23 comments · Fixed by #1340
Closed

aws_instance associate_public_ip_address #227

hashibot opened this issue Jun 13, 2017 · 23 comments · Fixed by #1340
Labels
bug Addresses a defect in current functionality.

Comments

@hashibot
Copy link

This issue was originally opened by @mengesb as hashicorp/terraform#8244. It was migrated here as part of the provider split. The original body of the issue is below.


Related to hashicorp/terraform#5764 I'd like to re-raise the issue of associate_public_ip_address in the aws_instance resource.

If the subnet being used has a map_public_ip = true setting, it will always map a public IP. I rather this be interpreted as, "If not specified, use map_public_ip setting for the subnet". It seems this is not the case, and that the subnet settings can override the aws_instance settings.

Is this an API limitation? Even if I create a subnet with public IP mapping, there are circumstances where I want the VMs created to not have public IPs. I know that if I have a subnet that DOES NOT map public IPs, and I set associate_public_ip_address = true on the aws_instance resource, I get a public IP. When I manually create an instance via the AWS console, I get the option to disable the 'auto-assign public IP', accept the subnet default, or enable. Seems pretty clear-cut here that if I specify false for associate_public_ip_address I should NOT get a public IP no matter what the subnet settings are.

Terraform Version

0.7.0

Affected Resource(s)

  • aws_instance

Terraform Configuration Files

resource "aws_instance" "instance" {
  ami = ..
  ebs_optimized = ..
  instance_type = ..
  associate_public_ip_address = false
  subnet_id = ..
 ...
}

Debug Output

N/A

Panic Output

N/A

Expected Behavior

if associate_public_ip_address is set to false, I get an instance without a public IP. If `associate_public_ip_address is set to true, I get an instance with a public IP. This doesn't matter whether or not the subnet is set to automatically map public IPs or not.

Actual Behavior

if associate_public_ip_address is set to false, I get an instance with a public IP if the subnet is configured to map_public_ip = true (done in terraform or not done in terraform).
if associate_public_ip_address is set to false, I get an instance without a public IP if the subnet is configured to map_public_ip = false (done in terraform or not done in terraform).

Steps to Reproduce


  1. Create terraform plan with aws instance resource. set associate_public_ip_address to false
  2. Use a subnet id that maps public IPs to instances (map_public_ip = true)
  3. terraform apply
  4. VM has public IP associated

  1. Create terraform plan with aws instance resource. set associate_public_ip_address to true
  2. Use a subnet id that maps public IPs to instances (map_public_ip = false)
  3. terraform apply
  4. VM has public IP associated

  1. Create terraform plan with aws instance resource. set associate_public_ip_address to false
  2. Use a subnet id that maps public IPs to instances (map_public_ip = false)
  3. terraform apply
  4. VM does not have a public IP associated

  1. Create terraform plan with aws instance resource. set associate_public_ip_address to true
  2. Use a subnet id that maps public IPs to instances (map_public_ip = true)
  3. terraform apply
  4. VM has public IP associated

  1. Create terraform plan with aws instance resource. do not specify associate_public_ip_address
  2. Use a subnet id that maps public IPs to instances (map_public_ip = true)
  3. terraform apply
  4. VM has public IP associated

  1. Create terraform plan with aws instance resource. do not specify associate_public_ip_address
  2. Use a subnet id that maps public IPs to instances (map_public_ip = false)
  3. terraform apply
  4. VM does not have a public IP associated

Important Factoids

N/A

References

Are there any other GitHub issues (open or closed) or Pull Requests that should be linked here? For example:

@hashibot hashibot added the bug Addresses a defect in current functionality. label Jun 13, 2017
@alejandroEsc
Copy link

+1 on this ticket

@grubernaut
Copy link
Contributor

Hey @mengesb + @alejandroEsc,

Thanks for the issue! However, I believe this is working as expected. More than happy to discuss reasons why I might be wrong, however 😄

The associate_public_ip_address is an optional and computed value. If the value is not set, than we don't specify the AssociatePublicIPAddress field in the instance create API call. From what I read of the issue, this is working as expected, omitting the field map_public_ip on the subnet.

Then, during the Read of the created instance, the associate_public_ip_address attribute is set to true if the instance's primary network interface has a public IP address associated with it. This is helpful for dependent resources that key off of the boolean value.

This all being said, it sounds like the issue would be made much clearer if associate_public_ip_address was no longer a Computed attribute, and only was applicable if no subnet_id was specified. And we also had a has_public_ip_address Computed attribute to satisfy the dependent resource requirements.

All of this being said, however, if you require a much more granularity in instance-level networking, the network_interface attribute would be the best bet here.

For now, I'm going to close this issue, but if we discuss further that it needs to be fixed, we can re-open and re-evaluate.

Thanks again!

@alejandroEsc
Copy link

alejandroEsc commented Aug 1, 2017

@grubernaut please reopen and in encourage you to warn before closing next time.
You are actually quite wrong about how this flag is working. Please read carefully and try this out on your own.

Summary:
When set to true or not set it is currently working as expected.
However regardless of how a subnet is public/private, when the value is set to false, because it is a Nullable-Boolean as defined by aws apis and because of how Zeros are calculated in code, the flag is wrongly omitted from the object definition which means it is set to the default (do as subnet settings). This is the wrong behavior. Please review my PR as it should illuminate a bit further what is going wrong.

@alejandroEsc
Copy link

@grubernaut also, I would have to review, but I do NOT remember this value being a computed value, how would that possibly be the case?

@mengesb
Copy link
Contributor

mengesb commented Aug 1, 2017

@grubernaut this issue attends from a tri-state problem that was in the history of the original issues (more than one).

  • If the subnets default setting for mapping public IPs is true, and the user creates an instance not specifying to map a public IP (default), then the instance gets a public IP. This is OK.

  • If the subnets default setting for mapping public IPs is false, and the user creates an instance not specifying to map a public IP (default), then the instance does not get a public IP. This is OK.

  • If the subnets default setting for mapping public IPs is true, and the user creates an instance specifying to map a public IP (true), then the instance gets a public IP. This is OK.

  • If the subnets default setting for mapping public IPs is false, and the user creates an instance specifying to map a public IP (true), then the instance gets a public IP. This is OK.

  • If the subnets default setting for mapping public IPs is false, and the user creates an instance specifying to NOT map a public IP (false), then the instance does not get a public IP. This is OK.

  • If the subnets default setting for mapping public IPs is true, and the user creates an instance specifying NPT to map a public IP (false), then the instance gets a public IP. This is NOT OK.

User provided information should take precedence over anything computed. In this case terraform can't create a private IP only system in a subnet that has associate public IP set to true

@grubernaut
Copy link
Contributor

grubernaut commented Aug 1, 2017

Hey @alejandroEsc + @mengesb,

Thank you for the replies!

First of all, would like to apologize for pre-emptively closing the issues and PR's, going to re-open for re-evaluation. Also apologies for the late response, been in meetings.

Going to respond as best as I can to the points made:

I would have to review, but I do NOT remember this value being a computed value, how would that possibly be the case?

associate_public_ip_address was made Computed in Terraform v0.7.8 with this commit: hashicorp/terraform#9560

However regardless of how a subnet is public/private, when the value is set to false, because it is a Nullable-Boolean as defined by aws apis and because of how Zeros are calculated in code, the flag is wrongly omitted from the object definition which means it is set to the default (do as subnet settings).

So, I will fully stand by the fact that there is a bug with the associate_public_ip_address attribute if it is set to false,
and a subnet_id is supplied. However, I believe this is a result of a core issue in Terraform on how we handle boolean
attributes that are explicitly set to false. This, however, is not caused by the nullable-boolean that the AWS API expects.
We have some conditional logic inside of the provider's Create function that isn't handling the explicitly false attribute correctly.

User provided information should take precedence over anything computed. In this case terraform can't create a private IP only system in a subnet that has associate public IP set to true.

Absolutely agree!

We're currently discussing internally the best way to solve this issue without incurring the breaking change by making the attribute a string value, and should come up with a solution soon.

Thanks, and apologies again for the premature issue close.

@grubernaut grubernaut reopened this Aug 1, 2017
@alejandroEsc
Copy link

@grubernaut thank you for the prompt reply. The solution, as per my investigation, would require in the most complete way to change your schema types in such a way to allow for nullable-types as valid option schema types. That, in my opinion would be a huge change and any other change would most likely be more hacky band-aid. Since we are parsing from yaml, i do not believe that my PRs are breaking changes.

@grubernaut
Copy link
Contributor

Hey all,

Just a quick note, we haven't forgotten about you 😄.
Working on the release currently, and other issues, but planning to get back to this shortly.

Thanks!

@grubernaut
Copy link
Contributor

Hey all, sorry for the delay on this.
Just pushed #1340, which should provide the behavior that is truly expected.
This also doesn't incur a breaking change based on the HCL parsing from Terraform, and is a much safer and simpler solution than the breaking change to use a string type for the attribute.

Thanks!

@alejandroEsc
Copy link

@grubernaut please RE-OPEN my PRs again and let's compare. I dont think you quite get the whole story. You essentially do what i did in one of my PRs that you closed, but you are again relying on d.GetOkExists("associate_public_ip_address") which does not work as expected. Please read those PRs carefully as i make note of the issue and why the problem is addressed that way.

@grubernaut
Copy link
Contributor

@alejandroEsc, I'm more than happy to discuss why you feel it's not working as expected.
I fully understand why you made your PR's to use strings instead of using a boolean, however, that is a breaking change for downstream users. Please read the core enhancement for GetOkExists(), as I literally just added that for this exact purpose, and works as expected.
#1200, also breaks the network_interface attribute, causing further breaking changes. I'd be more than happy to re-open your pull requests and relay these comments on why the contribution doesn't fix the issue on each pull request as well.

@alejandroEsc
Copy link

@grubernaut please add here the PR where the functionality of GerOkExists() has changed. In addition I would like to mention that my changes is not a breaking change. I took extreme care not to do so.

@alejandroEsc
Copy link

@grubernaut the change was pointed to me but another collaborator. Yeah the addition of that new function is something i guess you accepted the risk of potential abuse, and it was something i had considered but went against it for a few reasons some philosophical and some practical. Could you verify for me the following cases work:

Tests to perform on Launch Configurations and Instances:

  • Leave off the flag, start new instance in a public subnet and instance starts up as default (public)
  • Leave off the flag, start new instance in a public private subnet and instance starts as default (private)
  • add flag as public, start new instance in public subnet and instance starts up as public
  • add flag as public, start instance in a private subnet and instance starts as private
  • add flag as private, start new instance in public subnet and instance starts up as private
  • add flag as private, start new instance in private subnet and instance starts as private

please note that distinction between the three cases: default, public, private they are very different, Do the same tests for the both instances and launch configurations please. I will add this to your pr also.

@grubernaut
Copy link
Contributor

Hey @alejandroEsc, sure no problem.

The PR to Terraform core that adds GetOkExists is here: hashicorp/terraform#15723

And the PR's listed to change the boolean attributes to string attributes incur a breaking change on existing terraform statefiles. For an example, try creating an aws_instance resource with the aws provider compiled outside of your suggested changes. Use a boolean value for associate_public_ip_address, like so:

resource "aws_instance" "bar" {
  ami = "ami-22b9a343" # us-west-2
  instance_type = "t2.micro"
  associate_public_ip_address = true
  subnet_id = "${aws_subnet.public_subnet.id}"
}

Then compile your PR, and observe the differences. The breaking change would require each user specifying a boolean value for that attribute, to wrap strings around the attribute's value.

The PR also does not handle a boolean being explicitly set inside of a user's config. If a user attempts to use the current behavior of a literal boolean, your PR causes the parsed "string" value to be "0". This, alone, clearly demonstrates the breaking-change incurred in your PR's.

Removing the conflict on the network_interface configuration block, also causes a breaking change, as those two attributes will directly conflict with one another if a user has a pre-configured network_interface that replaces the primary network interface on the instance, causing the associate_public_ip_address attribute to be masked by the network_interface's attributes.

Also, the PR doesn't handle the conversion of the returned struct into a string during the Read function. Thus, the returned boolean from the AWS API call will attempt to be cast into a string during the call to d.Set(), thus fully requiring the user to change their attributes to use literal strings instead of a boolean.

This can, however, be fixed with a state migration function on the associate_public_ip_address attribute, but maintaining the type on the attribute is a much safer path forwards.

@grubernaut
Copy link
Contributor

@alejandroEsc expanded the tests to cover each case listed.

$ make testacc TEST=./aws TESTARGS="-run=TestAccAWSInstance_associatePublic_"
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSInstance_associatePublic_ -timeout 120m
=== RUN   TestAccAWSInstance_associatePublic_defaultPrivate
--- PASS: TestAccAWSInstance_associatePublic_defaultPrivate (106.54s)
=== RUN   TestAccAWSInstance_associatePublic_defaultPublic
--- PASS: TestAccAWSInstance_associatePublic_defaultPublic (215.38s)
=== RUN   TestAccAWSInstance_associatePublic_explicitPublic
--- PASS: TestAccAWSInstance_associatePublic_explicitPublic (112.23s)
=== RUN   TestAccAWSInstance_associatePublic_explicitPrivate
--- PASS: TestAccAWSInstance_associatePublic_explicitPrivate (224.96s)
=== RUN   TestAccAWSInstance_associatePublic_overridePublic
--- PASS: TestAccAWSInstance_associatePublic_overridePublic (117.78s)
=== RUN   TestAccAWSInstance_associatePublic_overridePrivate
--- PASS: TestAccAWSInstance_associatePublic_overridePrivate (112.48s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	889.375s

@mengesb
Copy link
Contributor

mengesb commented Aug 4, 2017 via email

@iancustoica
Copy link

iancustoica commented Aug 9, 2017

Hi,

Sorry but maybe I misunderstood something - I am trying to create an ec2 instance in a vpc that is out of my control where the default behaviour is to attach public ips and I want terraform to force this NOT to happen, so I have associate_public_ip_address = false in my resource declaration, but when I run terraform apply it still keeps getting a public ip and any subsequent terraform apply destroys and recreates the instance. Wasn't this supposed to be fixed? Is there anything I can do about it?

Terraform version

terraform -v
Terraform v0.10.0

Resource declaration

resource "aws_instance" "test" {
  count                       = 1
  ami                         = "${var.ami}"
  instance_type               = "t2.medium"
  key_name                    = "${var.key_name}"
  availability_zone           = "${var.availability_zone}"
  iam_instance_profile        = "${var.iam_instance_profile}"
  user_data                   = "${var.test_env_user_data}"
  vpc_security_group_ids      = "${var.infra_dev_vpcs}"
  subnet_id                   = "${var.subnet_id}"
  associate_public_ip_address = false
}

Results

terraform plan
...
  + aws_instance.test
      ami:                               "ami-01ccc867"
      associate_public_ip_address:       "false"
...
terraform apply
aws_instance.test: Creating...
  ami:                               "" => "ami-01ccc867"
  associate_public_ip_address:       "" => "false"
...
Apply complete! Resources: 1 added, 0 changed, 0 destroyed.

Note how after creation the value of associate_public_ip_address has changed to true.

terraform show
aws_instance.test:
  id = i-0b039f020f9bc7e4a
  ami = ami-01ccc867
  associate_public_ip_address = true
...
terraform plan
...
-/+ aws_instance.tyk-prometheus (new resource required)
      ami:                               "ami-01ccc867" => "ami-01ccc867"
      associate_public_ip_address:       "true" => "false" (forces new resource)
...

@grubernaut
Copy link
Contributor

Hi @iancustoica, thanks for the report!

This was just merged into the aws provider yesterday and should be released with the next version of the aws provider v1.0.0. It is confirmed to not be working with the currently released version of the provider.
Thanks!

@grubernaut
Copy link
Contributor

@iancustoica in reply to "Is there anything I can do about it":

If you compile the aws provider from master and use the compiled provider, it should work for you.

@iancustoica
Copy link

Roger that, thank you very much for taking the time and answering my questions - I will wait until the next release.

@grubernaut
Copy link
Contributor

@iancustoica awesome, happy to help! Please let us know if you need anything else, thanks!

@mickaelperrin
Copy link

@grubernaut I am still having the issue.

My versions:

terraform -v
Terraform v0.11.11
+ provider.aws v1.57.0

Am I missing something ?

I have a minimal config file that doesn't work:

provider "aws" {
  region = "${var.aws_region}"
}

resource "aws_instance" "testserver" {

  ami = "${lookup(var.amis, "ubuntu_18.04")}"
  instance_type = "${var.instance_type}"
  associate_public_ip_address = false

  tags {
    Name = "testserver"
  }
}

The result of terraform plan next after terraform apply:

-/+ aws_instance.testserver (new resource required)
      id:                           "i-0a262ea7106b73f33" => <computed> (forces new resource)
      ami:                          "ami-01096ce6152c92c71" => "ami-01096ce6152c92c71"
      arn:                          "arn:aws:ec2:eu-west-3:502080163780:instance/i-0a262ea7106b73f33" => <computed>
      associate_public_ip_address:  "true" => "false" (forces new resource)

@ghost
Copy link

ghost commented Apr 1, 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 Apr 1, 2020
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.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants