resource/aws_network_interface: Use first of private_ips as primary

[matt-deboer]

This changes private_ips to List (instead of Set) for more intuitive/predictable behavior, which helps to solve issues #836 and #996.

Before this change, there was no reliable way to configure the primary_ip --> you simply got the ip address whose string value's hash had the lowest lexical sort order. Deterministic, yes, but not determined by the order the ips were specified in the list :)

[justinclayton]

This is pretty important to anyone who needs to attach more than one interface to an instance in a particular order, such as anything using a keepalived-style pattern.

[curator]

Agreed ... this keeps us from building infrastructures where HA is dependent on deterministic addressing order.

[matt-deboer]

@radeksimko any chance of taking another look at this one?

[radeksimko]

Hi @matt-deboer
sorry for the delay on reviewing this one.

I have 2 questions:

1. Do we know for sure that IPs are always sorted the same way? The API doesn't document that. Do you have any confirmation from the AWS support? While I understand the difficulties of referencing sets (something we need to address in the core/language) the reason we use sets instead of lists is also to avoid spurious diffs whenever API returns IPs (or other fields) in a different order. Amazon tells us which one's primary via a special field called Primary which is IMO safer to follow.
2. This would be a breaking change for any users who already have existing configs & state in a couple of ways:
   • depending on the ordering of IPs the state may not be the same as in config and we're now making it the same which will raise diffs in the plan after upgrading.
   • Ordering of IPs in the config was not previously respected, or we rather took the first one based on hash and used that as primary.

On a related note I think that private_ip & private_ips are broken in general. As the former is used to specify primary IP and the second one may be used too, then we need to mark them as ConflictsWith.

The following cfg presents the problem:

resource "aws_vpc" "main" {
  cidr_block = "10.0.0.0/16"
}

resource "aws_subnet" "public_a" {
  vpc_id = "${aws_vpc.main.id}"
  cidr_block = "10.0.0.0/24"
}

resource "aws_network_interface" "test" {
  subnet_id   = "${aws_subnet.public_a.id}"
  private_ip  = "10.0.0.60"
  private_ips = ["10.0.0.51", "10.0.0.40", "10.0.0.30"]
}

If we're absolutely sure that ordering is predictable I'm ok with scheduling this as BC which can be part of the next major release (2.0.0), otherwise we'll need to find another way around.

[thallam08]

This issue might be resolved by updating the documentation. The option "private_ip" is not documented at https://www.terraform.io/docs/providers/aws/r/network_interface.html

[nbaztec]

I tested the above configuration however the privateIP seemed to be ignored:

resource "aws_vpc" "main" {
  cidr_block = "10.0.0.0/16"
}

resource "aws_subnet" "public_a" {
  vpc_id = "${aws_vpc.main.id}"
  cidr_block = "10.0.0.0/24"
  availability_zone = "eu-central-1a"
}

resource "aws_network_interface" "test" {
  subnet_id   = "${aws_subnet.public_a.id}"
  private_ip  = "10.0.0.60"
  private_ips = ["10.0.0.51", "10.0.0.40", "10.0.0.30"]
}

$ terraform apply
aws_vpc.main: Creating...
  assign_generated_ipv6_cidr_block: "" => "false"
  cidr_block:                       "" => "10.0.0.0/16"
  default_network_acl_id:           "" => "<computed>"
  default_route_table_id:           "" => "<computed>"
  default_security_group_id:        "" => "<computed>"
  dhcp_options_id:                  "" => "<computed>"
  enable_classiclink:               "" => "<computed>"
  enable_classiclink_dns_support:   "" => "<computed>"
  enable_dns_hostnames:             "" => "<computed>"
  enable_dns_support:               "" => "true"
  instance_tenancy:                 "" => "<computed>"
  ipv6_association_id:              "" => "<computed>"
  ipv6_cidr_block:                  "" => "<computed>"
  main_route_table_id:              "" => "<computed>"
aws_vpc.main: Creation complete after 2s (ID: vpc-75ec0b1e)
aws_subnet.public_a: Creating...
  assign_ipv6_address_on_creation: "" => "false"
  availability_zone:               "" => "eu-central-1a"
  cidr_block:                      "" => "10.0.0.0/24"
  ipv6_cidr_block:                 "" => "<computed>"
  ipv6_cidr_block_association_id:  "" => "<computed>"
  map_public_ip_on_launch:         "" => "false"
  vpc_id:                          "" => "vpc-75ec0b1e"
aws_subnet.public_a: Creation complete after 1s (ID: subnet-2844ef43)
aws_network_interface.test: Creating...
  attachment.#:           "" => "<computed>"
  private_dns_name:       "" => "<computed>"
  private_ip:             "" => "10.0.0.60"
  private_ips.#:          "" => "3"
  private_ips.2391807628: "" => "10.0.0.51"
  private_ips.2949480860: "" => "10.0.0.30"
  private_ips.3767277403: "" => "10.0.0.40"
  private_ips_count:      "" => "<computed>"
  security_groups.#:      "" => "<computed>"
  source_dest_check:      "" => "true"
  subnet_id:              "" => "subnet-2844ef43"

However the primary was not set correctly as per the console.

Network interface ID: eni-1cb07249 | Subnet ID: subnet-2844ef43
VPC ID: vpc-75ec0b1e
MAC address: 02:87:95:cf:fe:3c 
Primary private IPv4 IP: 10.0.0.51
Secondary private IPv4 IPs: 10.0.0.30, 10.0.0.40

https://imgur.com/a/UwYMT

[thallam08]

I can confirm that the template above is not working.

Maybe the logic needs to be:

• If "private_ip" is set, then use that and prepend it to the "private_ips" if its not already there
• Or if "private_ip" is not set, then set it to the first IP in the "private_ips" list
• Always set the primary IP for an interface to the "private_ip"

Alternatively drop the "private_ip" as it's not documented and just use the first item in "private_ips"

[thallam08]

The AWS API works by specifying that one of the IP addresses is primary. http://docs.aws.amazon.com/AWSEC2/latest/APIReference/API_PrivateIpAddressSpecification.html

Order is not the determining factor.

Maybe the cleanest fix is to make sure that "private_ip" is honoured as the primary IP address, and this option is documented.

[thallam08]

From the CLI documentation:
`
--private-ip-address (string)

The primary private IPv4 address of the network interface. If you don't specify an IPv4 address, Amazon EC2 selects one for you from the subnet's IPv4 CIDR range. If you specify an IP address, you cannot indicate any IP addresses specified in privateIpAddresses as primary (only one IP address can be designated as primary).

--private-ip-addresses (list)

One or more private IPv4 addresses.

`

[nbaztec]

I agree with @thallam08

After reviewing the AWS API it seems the plugin wasn't handling the private IP as it was supposed to be.
I've updated #1749 to behave in a reliable way and updated the tests accordingly. It's highly important to us that we switch away from our custom plugin to an officially supported/working one.

[thallam08]

There's a related issue that may not have been logged. If you change the list of IPs then the primary IP may change. The AWS CLI allows for adding and removing secondary IPs. As implemented this module does not allow us to do this reliably as it may try to change the primary IP and either have to recreate the resource or fail (eg primary interface).

[yn-academia]

The real issue here is that "private_ip" is ignored completely, even when set. It's set in the state, but it doesn't seem to propagate to AWS as the primary

[thallam08]

That does not work for a non-primary interface. Not sure it always works for primary interface. Sent from my ....

…

On 12 Feb 2019, at 4:42 am, Yuri Niyazov ***@***.***> wrote: If you include both the "private_ip" and "private_ips", and make sure that "private_ip" is included in the list of "private_ips", it does what you want. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[zhelding]

Pull request #21306 has significantly refactored the AWS Provider codebase. As a result, most PRs opened prior to the refactor now have merge conflicts that must be resolved before proceeding.

Specifically, PR #21306 relocated the code for all AWS resources and data sources from a single aws directory to a large number of separate directories in internal/service, each corresponding to a particular AWS service. This separation of code has also allowed for us to simplify the names of underlying functions -- while still avoiding namespace collisions.

We recognize that many pull requests have been open for some time without yet being addressed by our maintainers. Therefore, we want to make it clear that resolving these conflicts in no way affects the prioritization of a particular pull request. Once a pull request has been prioritized for review, the necessary changes will be made by a maintainer -- either directly or in collaboration with the pull request author.

For a more complete description of this refactor, including examples of how old filepaths and function names correspond to their new counterparts: please refer to issue #20000.

For a quick guide on how to amend your pull request to resolve the merge conflicts resulting from this refactor and bring it in line with our new code patterns: please refer to our Service Package Refactor Pull Request Guide.

[YakDriver]

This is obviated by #17846. Let us know if that does not solve for your situation.

[github-actions]

This functionality has been released in v3.74.0 of the Terraform AWS Provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you!

[github-actions]

I'm going to lock this pull request 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 related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.