Imdsv2 support #134
Imdsv2 support #134
Conversation
sdk/src/Core/Plugins/EC2Plugin.cs
Outdated
| _logger.Error(e, "Exception occurred while getting EC2 metadata from IMDSv2"); | ||
| _logger.DebugFormat("Unable to get metadata from IMDSv2 endpoint. Falling back to IMDSv1 endpoint"); | ||
|
|
||
| // try IMDSv1 endpoint for metadata |
There was a problem hiding this comment.
I had a similar comment in the node PR - since there's no difference in the document endpoint for v2 and v1 I think we should just have error handling around the token request. The can continue with the same logic, including the token if it's available or otherwise try without it.
There was a problem hiding this comment.
Here's my version in Java which takes that approach, the flow is just
- get token
- get document (include token if we got one)
- parse document
Don't need to treat v1 as a different endpoint
There was a problem hiding this comment.
Oh yes. Good call out. The node PR discussion slipped out of my mind while writing this.
sdk/src/Core/Plugins/EC2Plugin.cs
Outdated
| catch (Exception) | ||
| { | ||
| _logger.DebugFormat("Error occurred while getting EC2 metadata"); | ||
| return null; |
There was a problem hiding this comment.
Not much experience with c# but is it possible to return an empty dictionary here instead of null? No points to do a null check at the caller if it's same semantics as empty
There was a problem hiding this comment.
Can't use ImmutableDictionary since it's only supported for .NetCore and .Net 5. Bummer :(
returning a new empty Dictionary<>() instead to avoid the null check.
|
Is there a reason you don't just use the |
|
Hi @normj thanks for the question! The AWS SDK is fairly heavyweight, with most of its logic in order to support SDK calls. As this is an instrumentation library for user apps, and many of those apps may not even use the AWS SDK in their code, we want to reduce the dependency footprint so that adding tracing has as little impact on the user's app as possible. If a small library containing only the metadata lookup was extracted out of |
|
@anuraaga Totally agree you should not take a reference on AWSSDK.Core just for the metadata but it looks like the project already has a dependency on AWSSDK.Core |
|
@normj Good eyes :) This and some of our other language SDKs have used it too for fetching sampling configuration, but because we proxy requests through the X-Ray domain, which signs the requests itself, it's actually overkill to pull in the machinery of the AWS SDK especially since we don't need to expose these models to users. So we've just begun to move away from using the SDK dependency hoping a future major version can be much more lightweight and improve user experience. Here's an example from the Java SDK where in 3.0.0 we intend to remove the dependency. @srprash Do you think we can add this point to #102? And maybe even add a comment in the |
|
@anuraaga that makes sense to move away from using the SDK for these small use cases and avoid bring in a large dependency that could possibly cause conflict with the versions the application is using. |
* Refactored the EC2Plugin to use IMDSv2 endpoint via HTTP and fallback to v1 if v2 fails * Reafctored code to non-static members. Added unit tests for EC2Plugin * Refactored the logic to fetch metadata in a single flow irrespective of v1 and v2 * Refactored to return empty dict instead of null to drop reduntant null check * Removed some unnecessary variables and assignments
Issue #, if available:
Description of changes:
Adding IMDSv2 support for fetching the EC2 instance metadata via the EC2Plugin. The metadata is fetched from instance identity document with a fallback to V1 if V2 fails.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.