Fixes #836 - correctly assign primary private IP for aws_network_instance when private_ips are specified

[nbaztec]

Details
This PR fixes #836

Before
The primary_ips were stored as a Set with no guaranteed order. So the primary IP would be randomly assigned from the set, while the primary_ip field would be ignored.

After
The module now respects and sets the primary_ip field when primary_ips is also defined. If not, a random IP is made primary from primary_ips.

[nbaztec]

Anyone watching this?

[thallam08]

Any idea of when this is going to be merged and available? It's biting me at the moment. Is there a workaround?

[nbaztec]

We're currently running a custom built binary from the PR. Would be good if someone actually reviewed/merged it so we can use drop our custom plugin in favor or official plugin.

[Ninir]

Hi folks,

@radeksimko wrote a comment regarding the same work in #1672

@nbaztec : As this work is the same as in the other PR (but a bit younger) can you read this comment and answer in the other PR so that we centralize the work?

Thanks!

[thallam08]

I notice this is marked as "enhancement" - it's actually a bug fix. The correct action is to use the first item in the lists of IPs as the primary IP, anything else is incorrect behavior.

[nbaztec]

I'll be replying in #1672 to centralize the work

[nbaztec]

@Ninir please re-flag/re-review the PR.
Thanks.

[nbaztec]

PS: I don't think this could be a breaking change anymore.

[nbaztec]

The PR needs to be reflagged post the changes.

[nbaztec]

@radeksimko Could you please re-review and re-tag the PR. It has undergone some changes as per discussion in #1672

[nbaztec]

Updated with upstream

[curator]

Can we get some eyes on this? The inability to set the primary is really stopping use of terraform in a number of (particularly) non-ephemeral instance cases.

[thallam08]

@curator What version of the AWS module are you using? I've not had any issues with aws provider 1.16. Could be that I've just been lucky and the order I put the IPs in, matched the hashed order.

[nbaztec]

@thallam08 that's probably the case, since the code always hashes them and then the order (and the primary IP) depends on the lexical ordering of the hash.

[thallam08]

@nbaztec Yes, that was the basis of the problem. "Order in" != "Order out" because an unordered list had been used rather than an ordered list.

But I believe that has now been fixed, as I no longer see this issue on the terraform template that I used to have issue with.

Check the version of the AWS module installed in your .terraform/modules directory.

[nbaztec]

I can't test right now but I still see in master that we are casting from schema.Set which makes it unordered in the first place.

https://github.com/terraform-providers/terraform-provider-aws/blob/master/aws/resource_aws_network_interface.go#L122

Maybe I'll test it over this weekend again. Thanks.

[thallam08]

Yes, that was the issue. I must have struck it lucky! Sent from my ....

…

On 2 May 2018, at 10:33 pm, Nisheeth Barthwal ***@***.***> wrote: I can't test right now but I still see in master that we are casting from schema.Set which makes it unordered in the first place. https://github.com/terraform-providers/terraform-provider-aws/blob/master/aws/resource_aws_network_interface.go#L122 Maybe I'll test it over this weekend again. Thanks. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[curator]

@thallam08 As nbaztec pointed out, 1.16 still produces the issue; right now we're using a fork from way back in the way to deploy systems that have the deterministic requirement for now, but that's not our preferred route obviously. Hopefully an appropriate solution can reach master soon.

[nbaztec]

We are also using a fork like @curator with the changes contained in this PR. I would be more than willing to work further on this PR if the core team can give it a look.

[thallam08]

Can this be merged and released? It bit me again today. This time, when I added another IP, it tried to change the primary IP of an interface. AWS does not allow you to change the primary IP of an interface, so the update failed.

[thallam08]

I've not seen any evidence that this is a breaking change. The current behavior, without this fix, is non-deterministic as it relies on the hash valued of the set for the order. This is a FIX to a significant issue that is breaking the use of multiple IPs on an interface.

[thallam08]

Can the tags for this be changed?

• It's a "bug" not an "enhancement"
• It's not a "breaking change" (well I suppose it does break incorrect behavior)

[nbaztec]

I would agree with @thallam08 - the PR has been update to not be a breaking change but a simple bugfix.
@Ninir

[thallam08]

@Ninir So how do we get the tags on this changed so they match what this change really is, and then get it included in the next release?

[bganderson]

+1 Lets get this merged in

[nbaztec]

Does anyone know of an active maintainer that can take a look into it perhaps?

Closing in on a 1 year anniversary :)

[nbaztec]

Merged with upstream for a more up-to-date PR

[nbaztec]

@bflad would you be so kind as to re-label this long standing PR please :)

[sabretus]

Need this too. Why this case is ignored?

[simonbrady]

Any timeline for merging this please? This PR resolves a significant problem in the provider.

[yn-academia]

This PR seems wrong, as it's still a breaking change, no? All you want to do is include "private_ip" in the list of "private_ips", and then make sure it's passed up to AWS API.

[nbaztec]

@yn-academia the Amazon API explicitly states the use of private_ip for the primary address and any other secondary addresses within private_ips. In the absence of former it designates the first IP in the list as primary. However we use HashSets within the module, for good reasons, and that makes it impossible to be determinastic in regards to the first element in that list. As such the previous behaviour was that a random IP would be chosen as the primary. This fix makes it determinastic again.

[adrmaas]

@radeksimko @Ninir we have also been bitten by this bug and the documentation as it stands is ambiguous.

Perhaps someone could explain how this is being interrupted as breaking change and enhancement?

[yn]

This is a breaking change because users have current infrastructure that was spun up using the existing implementation, and their infrastructure has a random IP assigned as the primary, even when they specified a primary_ip which was then ignored. When they update to a new version of the AWS provider that includes your fix, terraform will want to change the primary IP of their infrastructure. You mark private_ip as ForceNew:true, which means that what terraform will actually want to do is to destroy existing infrastructure and create new infrastructure, but the users' terraform configuration files haven't changed, the only thing that changed was a minor version upgrade of the terraform AWS provider. Sounds like a breaking change to me.

Yes, it breaks configuration that relies on the behavior of an existing bug, and yes, that's unfortunate, but there are probably hundreds of production systems that rely on this now, and you have to think about them.

If you want to make this not a breaking change, you have to do something much uglier - you'd have to keep all existing implementation as is, and then add a new field called actual_private_ip or something like that which does what you want. Since no one's existing infrastructure has actual_private_ip in the configuration, you are more likely to get this merged. If you do go this route, you should probably open new PR, rather than continue building on this one.

The ultimate correct behavior is the one that you are proposing in this PR, but I'd imagine that a breaking change would only be allowed in across a major version upgrade of the terraform AWS provider.

Anyway - I don't work for HashiCorp, I am just a random user of terraform, so this entire comment may be completely wrong, but that's what I would do if I was running a project like this.

[nbaztec]

While what you say is true regarding the fix and it reassigning a new IP if a mismatch occurs, in most general cases the IPs are precomputed due to an automation pipeline and a determinastic IP would've been expected. In the absence of that probably users would've opted to terraform output and exported the IP to enable further usage. For most cases, assuming proper automation, it shouldn't cause much pain to have this fix, however, for some users a manual approach would certainly be required. Personally for me, that may or may not be a reason for a major upgrade. But so far, the maintainers have failed to comment on that as well and no milestone label has been provided to boot.

As such, I would defer any further development or speculative discussion until one of the maintainers share their view on the bug/approach.

[nbaztec]

closing due to lack of a decision.

[ghost]

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!