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

[WIP] Provide ENI for AWS Instance. #4231

Conversation

johnrengelman
Copy link
Contributor

This is start for support for things like #3105.
Specifically, this address the use case of wanting to provide a known ENI to an AWS instance as it's primary network interface (for example, create an ENI, use it in the route table and boot an instance onto that ENI).

ni := &ec2.InstanceNetworkInterfaceSpecification{
NetworkInterfaceId: aws.String(networkInterfaceID),
DeviceIndex: aws.Int64(int64(0)),
SubnetId: aws.String(subnetID),
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it valid for subnetID to not be specified when configuring an instance with an pre-existing ENI? Here, we haven't verified if subnetID is empty or not

Copy link
Contributor

Choose a reason for hiding this comment

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

We're also don't yet consider private_ip or vpc_security_group_ids, are those coming, or do they not apply here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This will need some more work I just wanted to get it out there before
going on travel.
Subnet_id cannot be specified with network interface.
I'll have to look into the others. My guess is that they should be be
specified since they'll be defined on the ENI.
On Wed, Dec 9, 2015 at 11:12 AM Clint notifications@github.com wrote:

In builtin/providers/aws/resource_aws_instance.go
#4231 (comment):

@@ -986,7 +997,15 @@ func buildAwsInstanceOpts(
}
}

  • if hasSubnet && associatePublicIPAddress {
  • if hasNetworkInterface {
  •   ni := &ec2.InstanceNetworkInterfaceSpecification{
    
  •       NetworkInterfaceId: aws.String(networkInterfaceID),
    
  •       DeviceIndex:        aws.Int64(int64(0)),
    
  •       SubnetId:           aws.String(subnetID),
    

We're also don't yet consider private_ip or vpc_security_group_ids, are
those coming, or do they not apply here?


Reply to this email directly or view it on GitHub
https://github.com/hashicorp/terraform/pull/4231/files#r47120749.

Copy link
Contributor

Choose a reason for hiding this comment

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

I thought it was probably a WIP, thanks. Ping us when you're ready!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@catsby -
I'm not sure the best way to proceed on this.
When creating an ENI, you can specify private_ip and security_groups, however it seems that when assigning that to an Instance, if you also specify security_groups then the groups on the instance will replace the groups on the ENI.
However, setting the private_ip will not replace the existing private_ip on the ENI (but does not cause an error).
Setting subnet_id causes an error to occur.

I'm thinking the best way is that if specifying network_interface_id, then we disallow subnet_id, security_group, vpc_security_group_id, and private_ip from being specified and force their configuration via the ENI. Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On that note, if we go that path, what's the best way to validate that certain attributes aren't specified together? ConflictsWith won't work because they are Computed attributes as well.

Copy link

Choose a reason for hiding this comment

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

As far as the EC2 API is concerned, several of the parameters for an instance are effectively delegates for the parameters of the network interface that EC2 implicitly creates for the instance. In the case where existing network interfaces are explicitly supplied using NetworkInterface.N parameters, EC2 does not create an implicit network interfaces and these delegated parameters are invalid. Specifically, PrivateIpAddress, SecurityGroup.N, SecurityGroupId.N, and SubnetId are all mutually exclusive with NetworkInterface.N. Furthermore, the instance's sourceDestCheck attribute is inapplicable if network interfaces have been explicitly supplied to the instance.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hey @johnrengelman terribly sorry for the silence here.
Looks at the SDK, it seems you are not supposed to supply the subnet id if you're supplying a network interface id, so I think you're idea on not allowing the others is correct.

Regarding validation, I honestly don't know from this vantage, we may just need to fall through to any error returned from AWS

@catsby catsby added the waiting-response An issue/pull request is waiting for a response from the community label Dec 9, 2015
@catsby catsby changed the title Provide ENI for AWS Instance. WIP: Provide ENI for AWS Instance. Dec 9, 2015
@davidhoyt
Copy link

Where is this at? Can it be merged?

@johnrengelman
Copy link
Contributor Author

@davidhoyt I've been waiting for some direction on how to implement this particular feature. See @inkblot's comments. The modeling in Terraform gets a little wonky here.

This has dropped off my radar as a priority since I came to it when trying to implement some better support for NAT routers in AWS and with the release of NAT Gateway, I am no longer reliant on this.

@catsby
Copy link
Contributor

catsby commented Jan 29, 2016

Sorry for the silence here friends – I relied with what I hope is good direction in this comment, let me know if this is something you're still willing to finish.

Thanks!

@radeksimko radeksimko added the wip label Feb 7, 2016
@phinze phinze changed the title WIP: Provide ENI for AWS Instance. [WIP] Provide ENI for AWS Instance. Feb 9, 2016
@phinze phinze removed the wip label Feb 9, 2016
@lamdor
Copy link
Contributor

lamdor commented Mar 30, 2016

@johnrengelman Is this waiting on anything? I found myself needing this. Looking over the code, it appears to be fine given @catsby's comment.

@johnrengelman
Copy link
Contributor Author

@rubbish it needs some work on how to properly model some of the data. From what I remember, I was having issues with validating that a user wasn't providing conflicting data.
This is low on my list and I don't expect to get back to it anytime soon, so if someone wants to take up the mantle, I'm all for it.

@stack72
Copy link
Contributor

stack72 commented Jul 30, 2016

Hi @johnrengelman

What is the status of this PR? Is this something that you think you would get back to or is this something we should close off?

Paul

@stack72 stack72 self-assigned this Jul 30, 2016
@johnrengelman
Copy link
Contributor Author

@stack72 - I'm not going to get back around to this. If someone wants to pick up and run with it, that would be awesome.

@stack72 stack72 removed their assignment Aug 8, 2016
@mitchellh mitchellh removed the waiting-response An issue/pull request is waiting for a response from the community label Dec 1, 2016
@stack72
Copy link
Contributor

stack72 commented Dec 5, 2016

Closing this out since this particular version of the fix isn't going to be finished - thanks for all the work put in here @johnrengelman

@ghost
Copy link

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

10 participants