Enable AWS check with IMDSv2 #391
Conversation
|
I will automatically update this comment whenever this PR is modified |
There was a problem hiding this comment.
Thanks @itcarroll. This LGTM. My understanding is that IMDSv2 is broadly available and in some cases required for security reasons. There may be some super old instance types where only IMDSv1 is available, but I'm not certain about that, and it also seems like enough of an edge case that I'm okay with the changes you have here (would need to add extra logic to try IMDSv1 is IMDSv2 fails). That said, I'd appreciate an extra +1 from someone else who is familiar with this code path.
Re: testing, I've manually tested this locally and on us-west-2 using Coiled and confirmed it's working as expected. I don't think we have any automated AWS testing at the moment.
I was going to suggest using boto3 to check, because it must implement this, right? boto/boto3#313 :( |
|
The ec2-metadata package mentioned in that boto3 feature request is possibly a better solution that making https requests. The AWS check should really be whether the instance is in us-west-2 anyways, right? Not just that it is an EC2 instance? |
Could you elaborate a bit here? I'm curious what advantages that package has over making a direct https request
Correct. See #395 for adding the region part to the check. |
Well, I thought one advantage is that it was aware of both IMDS versions, but I now see that the package docs actually say that it exclusively uses IMDSv2. In that case, the minor advantage is that the ec2-metadata may be more likely to stay current with future breaking changes. E.g. they updated to IMDSv2 in 2020. That's not really convincing though.
|
|
@jrbourbeau I guess #395 makes this obsolete? |
|
We'll want both this PR (which makes us compatible with IMDSv2) and #395 (which adds the extra us-west-2 check, along with some other variable renames). These two PRs will conflict with each other a small amount, so I'd like to get this PR merged first and then I'll handle the merge conflicts over in #395 separately |
There was a problem hiding this comment.
Thanks @itcarroll. Let's go ahead without an additional +1 for the changes here. I'm happy with them and we can always follow-up as needed if others have thoughts
Fixes #390 with a small change to the check for instance metadata query. For IMDSv2, the first request must be a PUT to "http://169.254.169.254/latest/api/token". A Response OK on that seems like a sufficient check.
Tests ... I would love to add a test. I have no idea how a test that depends on running on an EC2 instance could be set up. If you would like to provide guidance, happy to tackle.