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

provider/aws: Allow Instance to be computed in EIPs #3036

Merged
merged 2 commits into from
Oct 8, 2015

Conversation

catsby
Copy link
Contributor

@catsby catsby commented Aug 20, 2015

If you have an EIP attached to a network interface, which then has an Instance attached to it, Terraform plans will want to remove the instance attachment of the EIP, even though there is no explicit connection made by the user:

resource "aws_eip" "nat_ip" {
  network_interface = "${aws_network_interface.nat_eni.id}"
}

resource "aws_network_interface" "nat_eni" {
    # other attributes
  attachment {
    instance = "${aws_instance.nat.id}"
    device_index = 1
  }
}

resource "aws_instance" "nat" {
  ami = "ami-21f78e11"
    # other attributes
}

After applying, future plans will show this:

~ aws_eip.nat_ip
    instance: "i-<instance_id>" => ""

This PR make instance a computed attribute, and also removes the ConflictsWith restraint from network_interface. Oddly enough, the AssociateAddress API docs say you can't have both of these specified, but we do here and get no error.

Fixes #2952

@stack72
Copy link
Contributor

stack72 commented Aug 20, 2015

yay! This looks such a simple change :) Thanks @catsby

@phinze
Copy link
Contributor

phinze commented Aug 20, 2015

will want to destroy the instance associated with the EIP

This just means "remove the instance attachment of the EIP" not "destroy the instance" right?

Oddly enough, the AssociateAddress API docs say you can't have both of these specified, but we do here and get no error.

I think they're both populated in the Describe API call but you can't send both of them when you Associate - so I think ConflictsWith still makes sense for a config perspective?

Or maybe I'm misunderstanding how we have both of them specified here. Maybe you can 'splain me more?

@stack72
Copy link
Contributor

stack72 commented Aug 20, 2015

@phinze yes, it will remove the attachment

@phinze
Copy link
Contributor

phinze commented Aug 20, 2015

@stack72 cool - for a second i was like "what are you doing, Terraform?!" 😅

@catsby
Copy link
Contributor Author

catsby commented Aug 20, 2015

This just means "remove the instance attachment of the EIP" not "destroy the instance" right?

haha, yeah that's my bad. It will not destroy the instance. I updated to match your wording 😄

but you can't send both of them when you Associate - so I think ConflictsWith still makes sense for a config perspective?

well, you can send them both..

2015/08/20 11:34:32 terraform-provider-aws: 2015/08/20 11:34:32 [DEBUG] EIP Allocate: {
2015/08/20 11:34:32 terraform-provider-aws:   AllocationId: "eipalloc-7cb33619",
2015/08/20 11:34:32 terraform-provider-aws:   Domain: "vpc",
2015/08/20 11:34:32 terraform-provider-aws:   PublicIp: "54.191.221.60"
2015/08/20 11:34:32 terraform-provider-aws: }
2015/08/20 11:34:32 terraform-provider-aws: 2015/08/20 11:34:32 [INFO] EIP ID: eipalloc-7cb33619 (domain: vpc)
2015/08/20 11:34:32 terraform-provider-aws: 2015/08/20 11:34:32 [DEBUG] EIP associate configuration: {
2015/08/20 11:34:32 terraform-provider-aws:   AllocationId: "eipalloc-7cb33619",
2015/08/20 11:34:32 terraform-provider-aws:   InstanceId: "i-f14bfa37",
2015/08/20 11:34:32 terraform-provider-aws:   NetworkInterfaceId: "eni-30cf9979"
2015/08/20 11:34:32 terraform-provider-aws: } (domain: vpc)
2015/08/20 11:34:33 [DEBUG] EIP describe configuration: {
  AllocationIds: ["eipalloc-7cb33619"]
} (domain: vpc)
...
[root debug stuff]
...
2015/08/20 11:34:33 waiting for all plugin processes to complete...
Apply complete! Resources: 1 added, 0 changed, 0 destroyed.

but it seems to favor the InstanceId and attach the EIP to that, and uses the default ENI .. basically ignoring the NetworkInterfaceId you sent...

Unfortunately, I can't have both the ConflictsWith on network_interface and Computed on instance, Terraform won't let me. Thoughts, @phinze ?

@jszwedko
Copy link
Contributor

jszwedko commented Sep 4, 2015

Would documentation be sufficient for discouraging the user from setting both @phinze @catsby ? Or maybe a warning?

I guess it might be nicer to have ConflictsWith ignore computed attributes.

@phinze
Copy link
Contributor

phinze commented Sep 17, 2015

Yeah I think for now we just document the limitation and then ship it. 👍 🚢

@catsby catsby force-pushed the b-aws-eip-computed-instance branch from 8ab38cd to 9d973f1 Compare October 8, 2015 14:31
@catsby
Copy link
Contributor Author

catsby commented Oct 8, 2015

Documented the limitation/behavior in 9d973f1, pull this in now

catsby added a commit that referenced this pull request Oct 8, 2015
provider/aws: Allow Instance to be computed in EIPs
@catsby catsby merged commit 4962ef1 into master Oct 8, 2015
@radeksimko radeksimko deleted the b-aws-eip-computed-instance branch October 8, 2015 14:57
@ghost
Copy link

ghost commented Apr 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 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.

@ghost ghost locked and limited conversation to collaborators Apr 30, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow External Instances Attached to EIP
4 participants