-
Notifications
You must be signed in to change notification settings - Fork 926
Use Ec2MetadataClient in defaults mode #6301
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: feature/master/use-imds-client
Are you sure you want to change the base?
Use Ec2MetadataClient in defaults mode #6301
Conversation
...ain/java/software/amazon/awssdk/awscore/internal/defaultsmode/AutoDefaultsModeDiscovery.java
Show resolved
Hide resolved
core/imds/src/main/java/software/amazon/awssdk/imds/internal/BaseEc2MetadataClient.java
Outdated
Show resolved
Hide resolved
...zon/awssdk/awscore/internal/defaultsmode/AutoDefaultsModeDiscoveryEc2MetadataClientTest.java
Show resolved
Hide resolved
...src/main/java/software/amazon/awssdk/imds/internal/DefaultEc2MetadataClientWithFallback.java
Show resolved
Hide resolved
...zon/awssdk/awscore/internal/defaultsmode/AutoDefaultsModeDiscoveryEc2MetadataClientTest.java
Outdated
Show resolved
Hide resolved
core/imds/src/main/java/software/amazon/awssdk/imds/internal/Ec2MetadataSharedClient.java
Outdated
Show resolved
Hide resolved
Adding the logic to close the client Addressing PR feedback
return Optional.ofNullable(ec2InstanceRegion); | ||
} catch (Exception exception) { | ||
} catch (SdkClientException e) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What was this change for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To fix a SpotBugs warning that complained about catching Exception when no exception was thrown and client.get() actually throws SdkClientException (and its subclasses), so changed this to more specific exception.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we do RuntimeException
, instead? This would catch the same exceptions as before, but not upset spotbugs. Unless we want a potential behavior change?
...zon/awssdk/awscore/internal/defaultsmode/AutoDefaultsModeDiscoveryEc2MetadataClientTest.java
Show resolved
Hide resolved
...src/main/java/software/amazon/awssdk/imds/internal/DefaultEc2MetadataClientWithFallback.java
Outdated
Show resolved
Hide resolved
return this; | ||
} | ||
|
||
public synchronized Ec2MetadataClient build() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This synchronized uses a different lock from the containing class's synchronized. Maybe we should just manually create a reentrant lock and share that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, Thanks for this, updated to use reentrant lock.
* This provides resource efficiency by sharing a single HTTP client across all IMDS-backed providers | ||
*/ | ||
@SdkProtectedApi | ||
public final class Ec2MetadataSharedClient { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of having a separate protected class and API for this, should it be a feature of Ec2MetadataClient? E.g. if they do Ec2MetadataClient.builder().build()
, we use a shared HTTP client instead of a new one? We'd still use reference counting to close the shared HTTP client when no metadata clients are using it anymore.
That would remove the need for another protected API we need to maintain/remember to use, and improve the creation time of multiple Ec2MetadataClients for customers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we make Ec2MetadataClient.builder().build() calls use a shared HTTP client, then client.close() would only decrement the reference count instead of actually closing resources, this would be a behavior change for users right? They expect their client to be fully closed but it's not closing. Alternatively, if we kept user-created clients separate from internal provider clients, we'd need to add a new public API feature to Ec2MetadataClient for providers to use shared clients, but that would be a public API change and users could also access.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we changed the default to use a shared client, how would that behavior change negatively affect the customer?
core/imds/src/main/java/software/amazon/awssdk/imds/internal/RequestMarshaller.java
Outdated
Show resolved
Hide resolved
* An Implementation of the Ec2Metadata Interface with IMDSv1 fallback support. | ||
* This client is identical to DefaultEc2MetadataClient but provides automatic fallback | ||
* to IMDSv1 when IMDSv2 token retrieval fails (except for 400 errors). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this just be a feature of Ec2MetadataClient
as well? Ec2MetadataClient.builder().v1FallbackAllowed(true).build()
It would remove the duplicated code between the two implementations, make the usage of v1 something customers can choose to use in their own Ec2MetadataClients, and reduce the number of APIs we need to manage/remember to use.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would be a public API change, and we only want IMDSv1 fallback for backward compatibility in internal providers so didn't go for that approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we can reduce the number of protected APIs and simplify the code, it might be worth having v1 fallback be a public API.
I'm concerned about the duplication and additional protected surface area the current approach creates. Can you think of other ways to reduce it, without making a public API for enabling v1 fallback?
@SdkInternalApi | ||
@SdkProtectedApi | ||
public final class DefaultSdkHttpClientBuilder implements SdkHttpClient.Builder { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Darn, I see we already released something that uses this, so this is just backfilling it to mark it as protected. If we needed it to be protected, we should have moved it out of the internal package or ideally not have made it protected. Too late for that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah :(
Added the comment about it.
|
Refactoring BaseEc2MetadataClient constructor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Matt still has some concerns, but I think they were all part of the design that we reviewed while he was on vacation. It's all compromises for not introducing the v1 fallback available as a public API on the IMDS, which is what we agreed on during design.
AutoDefaultsModeDiscovery changes to use Ec2MetadataClient
Motivation and Context
This change migrates IMDS-backed providers from using internal utilities to the public Ec2MetadataClient API.
This PR specifically addresses the Auto defaults mode discovery component, which is one of three planned migrations (along with InstanceProfileCredentialsProvider and InstanceProfileRegionProvider).
Github Feature Request: #5876
Modifications
Testing
Screenshots (if appropriate)
Types of changes
Checklist
mvn install
succeedsscripts/new-change
script and following the instructions. Commit the new file created by the script in.changes/next-release
with your changes.License