search

hashicorp / packer-plugin-amazon

Packer plugin for Amazon AMI Builder
https://www.packer.io/docs/builders/amazon
Mozilla Public License 2.0
76 stars 112 forks source link

Instance SSH access: use all attempts to get public IP #309

Closed sparrc closed 1 year ago

sparrc commented 1 year ago

Previously the ssh access IP would fallback to the private ip address on the first "try" if a public IP is not available yet.

This changes the logic to only fallback to private IP address if we are on the final attempt at finding an IP address.

Precedence of always preferring public IP is left unchanged.

related to #40 and #308

sparrc commented 1 year ago

For context, https://github.com/hashicorp/packer-plugin-amazon/pull/308 is the PR that fixes the root cause of our issue, but I thought I'd submit this change as another potential PR.

I believe most users, when not specifying "ssh_interface", would expect this function to try to find a public IP first before falling back to private IP.

sparrc commented 1 year ago

thanks for considering it @lbajolet-hashicorp !

I definitely understand if you decide not to merge, as it is a change in behavior.

I would say that in it's favor, packer already silently prefers public ip addresses when nothing is specified, so I think it's fairly low risk as someone who wanted packer to prefer private ip addresses would have already needed to specify that using the ssh_interface arg.

Either way, thanks again for considering and discussing :)

lbajolet-hashicorp commented 1 year ago

Hi @sparrc,

Regarding the code we've agreed that the logic looks sound, to highlight the potential change in behaviour, we're labelling this as a bump minor, that way clients will be able to pin the current version if they want to keep the existing behaviour.

On another note, have you experienced the issue? If so, could you provide a template to reproduce the issue? If possible we'd like to add an acceptance test for this before merging.

Thanks again!

sparrc commented 1 year ago

Yes I did experience this issue, it was the same issue as https://github.com/hashicorp/packer/issues/10347

Now with HEAD of main I can't repro the issue (because of https://github.com/hashicorp/packer-plugin-amazon/pull/308).

But if you checkout a commit before #308 was merged, and launched an ami build with a spot instance, then you would see packer trying to SSH to the instance's private ip address almost every time, since at that point the public ip address is not ready or assigned yet.

But as far as I know #308 does not guarantee the public ip is ready, so I believe if you happen to get a particularly slow ENI provisioning in AWS packer may still try to connect to the private IP. This is why I'd also like to merge this PR, to add a further guarantee that packer will try to get the public IP.