metadata_options → http_tokens = "required" is NOT a safe assumption.

[skyzyx]

This is not a safe assumption to make:

https://github.com/philips-labs/terraform-aws-github-runner/blob/develop/modules/runners/main.tf#L62

For example, we have Base AMIs where certain actions are performed early (as part of /etc/rc.local) which make calls to the IDMS service. Force-requiring a token means that the nodes boot up incorrectly, and never get around to processing the user_data where we connect to Actions.

This should either be configurable, or you need to explicitly point out how this may affect people using the on-board IDMS service, so that they can pre-bake their AMIs with this in-mind. Users need to perform a step to explicitly fetch a token, then include that token in all additional curl calls to 169.254.169.254.

https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/configuring-instance-metadata-service.html

[npalm]

Thanks for creating the issue, any interest to propose a fix in a PR?

[Makeshift]

A warning to those who update, realise that http_tokens are now required, then try to rollback: Terraform will NOT delete metadata option from your launch configurations. You will need to manually edit all of your launch configurations to remove this setting, or delete them and have terraform remake them.

[npalm]

What would be the best way forward? Removing the option for the moment and do the implementation in a new PR?

[Makeshift]

That would be my suggestion. It would be fairly simple to make the option configurable and have it false by default?

Edit: The v2 metadata service will still work even if it isn't force-enabled, I believe

[npalm]

Drawback of setting the option to false is that the default provided user_data script is not working anymore, am I correct?

[Makeshift]

I don't think so, I believe it should still work, as the recommended path for upgrading implies that IMDSv2 is still usable even if it isn't explicitly required by metadata options.

Unless there's a ec2:RoleDelivery condition key I missed somewhere, I think that's the only thing that would need changing, although I haven't looked too deep into it yet as I've been fixing my actions setup this morning 😅

[npalm]

I will create a PR to make the feature configurable, and set the default to optional

[npalm]

@Makeshift @skyzyx I have created a fix, feel free to make comments on the PR (#1377)