builder: allow specifying hostId for placement

[amaxine]

This PR adds support for specifying an exact dedicated host ID in the placement block. We were originally running this when mac2.metal instances weren't publicly available and AWS API behaviour around them was weird enough that Packer couldn't see them in responses that didn't query a specific ID.

This addresses a specific issue we experience - since by default, the source will grab whatever the first available host is, we occasionally find ourselves with the problem of Packer "stealing" from the wrong pool because for various lifecycle reasons, another host was available. With this change, an end user can instead provide a specific host ID (e.g. via a variable, from the response of an external call to the AWS API creating a new dedicated host resource), ensuring that the correct host is used for a Packer build for resources that aren't possible to fully manage on demand - ideal for Mac type instances in AWS.

This should potentially be exclusive with HostResourceGroupArn but I'm not sure how to appropriately model that in a Packer plugin, or whether it's at all necessary.

[hashicorp-cla]

All committers have signed the CLA.

[nywilken]

Hi @amaxine thank you for the contribution. I've add this change to our board so we can work to prioritize the review. So far it looks good I just want to validate your question around exclusivity for other attributes to provide a quality review.

[lbajolet-hashicorp]

Hi @amaxine,

I've walked through the AWS docs, and AFAICT, they do not explicitely state that the two are mutually exclusive. That being said, since host groups are an abstraction over dedicated hosts, it would make sense that if the group ARN is set, the host ID should not be, and vice versa. I did not see anything that points to a valid use-case in specifying both, and I haven't tested what happens with for example awscli if both are specified at once, but I presume either argument has precedence silently (not great), or it errors if they have a check for that.

In any case, I think it would make sense for both to be mutually exclusive, and for the plugin to return an error if both are specified at once.
To implement such a logic, I would look into the Prepare functions, since this is where we walk on the configs and look for inconsistencies, before actually performing the build's steps.

Come to think of it, I would think it'd make sense to also check for the Tenancy at the same time, for consistency with the arguments specified (i.e. it should be host if either the host_id or host_resource_group_arn are specified, and freeform-ish outside of that), but this can be a later improvement, no need to trouble yourself with that if you don't want to.

Let me know if you need help with that, I can take a look if need be.

Besides that I second @nywilken, the code looks good to me, once we add the logic to check for mutual-exclusiveness, this can be merged imho.

[lbajolet-hashicorp]

Hi @amaxine,

FYI, I pushed some changes on your branch to add the extra checks we mentioned in our last review, I also added some tests to make sure the Prepare step works as intended.

@nywilken when you have time could you review those? If you're OK with this reroll, we can merge that PR.

[amaxine]

Thank you for the feedback and the pointers on how to implement mutually exclusive options, and thank you also for picking up the work to finish my PR. Unfortunately in the context of the BSL relicense I have no interest in completing this work, but as I've signed the CLA I'm happy to see the feature get completed.

[nywilken]

Thanks for adding the addition checks. This looks good to me.

[nywilken]

Thank you for the feedback and the pointers on how to implement mutually exclusive options, and thank you also for picking up the work to finish my PR. Unfortunately in the context of the BSL relicense I have no interest in completing this work, but as I've signed the CLA I'm happy to see the feature get completed.

@amaxine Thanks for opening up this change. If it’s any consolation the plugins will remain as MPL2. But I completely understand.

Don’t hesitate to reach out if you run into any issues with the plugin. 💙