Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Support IMDSv2 in launch template #43

Merged

Conversation

olivermeyer
Copy link
Contributor

This PR adds support for IMDSv2 to the compute EC2 launch template. See the doc for metadata options for more info.

@olivermeyer olivermeyer force-pushed the launch-template-support-imdsv2 branch from e9998e7 to 7ea7926 Compare October 25, 2022 13:12
@olivermeyer
Copy link
Contributor Author

@oavdeev tagging you for visibility : )

@savingoyal savingoyal requested a review from oavdeev November 1, 2022 15:05
Copy link
Member

@oavdeev oavdeev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we make it off by default so its backwards compatible? it seems like right now if someone upgrades their deployment using this template, they'll have instance metadata disabled and their metaflow tasks will stop working

variable "launch_template_http_put_response_hop_limit" {
type = number
description = "The desired HTTP PUT response hop limit for instance metadata requests. Can be an integer from 1 to 64"
default = 1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is 1 a good default? I vaguely remember that it needs to be 2 for docker since docker networking adds another hop

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know tbh. I just know TF defaults to 1, so it seems to make sense to do the same here to avoid unexpected changes in people's plans. Can bump to 2 if that's preferred.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think would matter for the current users of the module since its only IMDSv1 and IMDSv1 doesn't use the hop limit. Can you test it with IMDSv2 tokens required and hop limit set to 1, does it work?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, I increased the default to 2.

@olivermeyer
Copy link
Contributor Author

can we make it off by default so its backwards compatible? it seems like right now if someone upgrades their deployment using this template, they'll have instance metadata disabled and their metaflow tasks will stop working

For some reason I thought the TF default for http_endpoint was disabled but it's actually enabled - just fixed it. All three variables now default to the TF default so upgrading without overriding them should have no impact.

@olivermeyer olivermeyer requested a review from oavdeev November 4, 2022 13:44
@olivermeyer olivermeyer force-pushed the launch-template-support-imdsv2 branch from 0460dfe to 3fd4899 Compare November 4, 2022 13:46
@olivermeyer
Copy link
Contributor Author

Hi @oavdeev, is there anything I can do to help release this? We're currently running Metaflow off my fork and that's not nice

@oavdeev
Copy link
Member

oavdeev commented Nov 18, 2022

Sorry for the delay! I'll cut a release next week

@oavdeev oavdeev merged commit 9d1acdd into outerbounds:master Nov 18, 2022
@olivermeyer olivermeyer deleted the launch-template-support-imdsv2 branch November 24, 2022 10:59
@olivermeyer
Copy link
Contributor Author

@oavdeev bumping this again :)

Would it make sense to add something like https://github.com/cycjimmy/semantic-release-action to the repo so that releases are cut automatically after merging?

@oavdeev
Copy link
Member

oavdeev commented Nov 29, 2022

Hey @olivermeyer sorry for the delay, @ob-uk just cut a new release 0.8.0! As for the github action, I think its a good idea eventually but for now we don't have enough automated testing to be cutting releases completely on auto-pilot. Today we do at least some manual testing for each release to make sure it works as expected.

@olivermeyer
Copy link
Contributor Author

Hey @olivermeyer sorry for the delay, @ob-uk just cut a new release 0.8.0! As for the github action, I think its a good idea eventually but for now we don't have enough automated testing to be cutting releases completely on auto-pilot. Today we do at least some manual testing for each release to make sure it works as expected.

Awesome thanks! And makes sense about testing.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants