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: EC2 instance - multiple private ips #6387

Closed
wants to merge 2 commits into from

Conversation

Djuke
Copy link
Contributor

@Djuke Djuke commented Apr 28, 2016

Feature: Adds support for multiple private IPs to the instance's first network interface.

Test: Passed all TestAccAWSInstance acceptance tests.

I wanted to add the ConflictsWith attribute to private_ip and private_ips but it conflicts with the computed argument which is still required for private_ip and should for private_ips.

@radeksimko
Copy link
Member

Thanks for the PR,
this may conflict with #4231 - at least in git-sense, so I'll just reference it here for any future readers, especially for someone who may later pick that linked PR up, but based on the last comments in that thread it doesn't look like this PR should be waiting for it.

I'll get to the actual code review hopefully soon.

@Djuke Djuke force-pushed the ec2-private-ips branch from 9a3dd1a to 3c83fce Compare May 11, 2016 07:51
@Djuke
Copy link
Contributor Author

Djuke commented May 19, 2016

this may conflict with #6761, but it is the same fix

@radeksimko
Copy link
Member

I just merged your other PR #6761 as that one was easier to review - would you mind rebasing this one & resolving conflicts?

Thanks.

@radeksimko radeksimko added the waiting-response An issue/pull request is waiting for a response from the community label May 29, 2016
@Djuke Djuke force-pushed the ec2-private-ips branch from 6df656f to c50025b Compare May 30, 2016 06:50
@radeksimko radeksimko removed the waiting-response An issue/pull request is waiting for a response from the community label May 30, 2016
@Mil0dV
Copy link

Mil0dV commented Jun 21, 2016

Ping. (we'd very much like to see this make 0.7 :))

@radeksimko radeksimko changed the title [WIP] aws ec2 multiple private ips provider/aws: EC2 instance - multiple private ips Jul 9, 2016
@@ -1049,7 +1060,24 @@ func buildAwsInstanceOpts(
}
}

if hasSubnet && associatePublicIPAddress {
private_ips := d.Get("private_ips").(*schema.Set).List()
if len(private_ips) != 0 {
Copy link
Member

@radeksimko radeksimko Jul 9, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we'll need to refactor the whole conditional block to keep handling EC2 Classic vs VPC correctly.
I'm still ok with deprecating private_ip in favour of private_ips, but we will need to add some checks for EC2 Classic && len(private_ips) > 1 cases. I think we should error out as early as we can in such cases.

See #7568 which may help in making decisions. We need to be prepared for scenarios where we don't have the list of supported platforms though - I'd suggest we 1st try assigning multiple IPs and error out with an informative error message (e.g. ("Assigning %d IPs failed: %s (hint: EC2 Classic accounts don't support multiple private IPs for EC2 instances)", len(ips), err)).

I'm aware that it may be difficult to test on EC2 Classic and many/most of us run VPC-only accounts, but we can at least stick to expectations described in AWS docs: http://docs.aws.amazon.com/AWSEC2/latest/UserGuide/using-vpc.html#differences-ec2-classic-vpc

I recommend carefully reading specifically the following two sections:

  • Multiple private IP addresses
  • Accessing the Internet

@radeksimko
Copy link
Member

radeksimko commented Jul 9, 2016

Firstly sorry for the delay in providing feedback.


We don't currently have any good deprecation mechanisms for Computed fields unfortunately which may need addressing before we actually remove the old field, otherwise some users may get seriously confused.

The following

"${aws_instance.example.private_ip}"

would not print any warning nor error as Deprecated field neither if the field was missing in the schema. The second behaviour/bug has something to do with internal schema validation, I'm pretty sure it's logged somewhere under a different issue.

TL;DR What you did with Deprecated attribute is the best you can do today. 👍


I left you some comments above. The most important one is EC2 Classic compatibility.

@radeksimko radeksimko added the waiting-response An issue/pull request is waiting for a response from the community label Jul 9, 2016
* `private_ip` - (Optional, Deprecated) Private IP address to associate with the
instance in a VPC. This
attribute is deprecated, please use the `private_ips` attribute instead.
* `private_ips` - (Optional) A list of private IP addresses to accociate with the instance's first network interface in a VPC.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

associate not accociate

@frankh
Copy link

frankh commented Mar 21, 2017

Is there any update on this?

@paddycarver
Copy link
Contributor

Hi! This PR has been open for quite some time and waiting response from the community without any activity, so I'm going to close it out. We do this to try to keep our attention on PRs that are actively in-progress, it's not a comment on the code itself. If there is still interest in this PR, please feel free to reply, and we'll re-open this. Thanks!

@paddycarver paddycarver closed this May 8, 2017
@sergibarroso
Copy link

Is there any update on that, I would like to have this feature available in Terraform.

@ghost
Copy link

ghost commented Apr 12, 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 12, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement provider/aws waiting-response An issue/pull request is waiting for a response from the community
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants