feat: add support for account ID in IMDS credentials #1570
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ianbotsf
added
the
breaking-change
ianbotsf
added
acknowledge-api-break
|
A new generated diff is ready to view.
|
This comment has been minimized.
This comment has been minimized.
lauzadis
reviewed
Apr 14, 2025
Comment on lines
62
to
64
| client = ImdsClient { | ||
| platformProvider = this@DefaultChainCredentialsProvider.platformProvider | ||
| engine = this@DefaultChainCredentialsProvider.engine |
Member
There was a problem hiding this comment.
@ param client a preconfigured IMDS client with which to retrieve instance metadata. If an instance is passed, the caller is responsible for closing it.
Should we be closing this ImdsClient that the default chain creates?
Contributor
Author
There was a problem hiding this comment.
Yep, we should be! Will fix.
| this.platformProvider = this@ImdsCredentialsProvider.platformProvider | ||
| } | ||
|
|
||
| // FIXME This only resolves from env vars and sys props but we need to resolve from profiles too |
Member
There was a problem hiding this comment.
I know you said you're working on a bigger refactor for profile keys, is that in progress now? I think we already have everything available that you'd need to read from profile here
Contributor
Author
There was a problem hiding this comment.
It's not yet in progress so I'll add profile support now similarly to how we currently resolve other profile settings.
| override suspend fun resolve(attributes: Attributes): Credentials { | ||
| if (AwsSdkSetting.AwsEc2MetadataDisabled.resolve(platformProvider) == true) { | ||
| private suspend fun resolveUnderLock(): Credentials { | ||
| println("**** Resolving creds (instanceProfileName=$instanceProfileName; apiVersion=$apiVersion; urlBase=$urlBase)") |
|
|
||
| override suspend fun resolve(attributes: Attributes): Credentials { | ||
| if (AwsSdkSetting.AwsEc2MetadataDisabled.resolve(platformProvider) == true) { | ||
| private suspend fun resolveUnderLock(): Credentials { |
Member
There was a problem hiding this comment.
nit/naming: There's no mutex used here, it seems strange to call it resolveUnderLock
Contributor
Author
There was a problem hiding this comment.
That was meant to describe the conditions under which the method must be called. 🤔 What's a better name for this?
Member
There was a problem hiding this comment.
It's not a strong opinion, I think something like resolveSingleFlight or doResolve might be a bit clearer
Comment on lines
-94
to
-100
| nextRefresh = if (resp.expiration != null && resp.expiration < clock.now()) { | ||
| coroutineContext.warn<ImdsCredentialsProvider> { | ||
| "Attempting credential expiration extension due to a credential service availability issue. " + | ||
| "A refresh of these credentials will be attempted again in " + | ||
| "${ DEFAULT_CREDENTIALS_REFRESH_SECONDS / 60 } minutes." | ||
| } | ||
| clock.now() + DEFAULT_CREDENTIALS_REFRESH_SECONDS.seconds |
Member
There was a problem hiding this comment.
Are we intentionally dropping this HOSM / static stability behavior?
Contributor
Author
There was a problem hiding this comment.
No, we still mostly support the requirements of the static stability spec. This change removes some of the messaging around expiry but we'll still attempt to use expired credentials as required. We should update the CachedCredentialsProvider to handle messaging about expiry. I'll leave a TODO.
Member
There was a problem hiding this comment.
The static stability SEP says we MUST log a message when using expired credentials
Member
There was a problem hiding this comment.
Having JSON test resources in a .kt file seems weird, is there any reason we can't use the resources folder like we do for other test resources? Is it because this test is common, not JVM-only?
Contributor
Author
There was a problem hiding this comment.
Yes, this test is common and there're no non-JVM resource facilities in Kotlin yet. This continues the pattern we use in other tests like EndpointUrlConfigNamesTest, DefaultRegionProviderChainTest, StandardRetryIntegrationTest, etc.
lauzadis
reviewed
Apr 14, 2025
Comment on lines
49
to
55
| @Deprecated("This constructor supports parameters which are no longer used in the implementation. It will be removed in version 1.5.") | ||
| public constructor( | ||
| profileOverride: String? = null, | ||
| client: Lazy<InstanceMetadataProvider> = lazy { ImdsClient() }, | ||
| platformProvider: PlatformEnvironProvider = PlatformProvider.System, | ||
| @Suppress("UNUSED_PARAMETER") clock: Clock = Clock.System, | ||
| ) : this(profileOverride, client.value, platformProvider = platformProvider as? PlatformProvider ?: PlatformProvider.System) |
Member
There was a problem hiding this comment.
We'll need to merge this deprecation into main, right?
Contributor
Author
There was a problem hiding this comment.
Yes, I originally planned to push this change to main directly but I've since added backwards incompatibilities beyond just deprecation so I'll split this change out now.
Contributor
Author
There was a problem hiding this comment.
Actually, I've realized now I can probably just merge this directly into main with a few tweaks. Updating PR...
Contributor
Author
There was a problem hiding this comment.
...except this PR's branch upstream is already v1.5-main so I'll cherry pick the commits over to a new branch and new PR #1573.
Member
There was a problem hiding this comment.
For the future, it's possible to change the destination branch if you edit the PR at the top
* close auto-created ImdsClient in default creds chain * adding support for profile settings * removing println calls leftover from debug * rename resolveUnderLock to resolveSingleFlight * add FIXME for static stability messaging/caching in CachedCredentialsProvider * re-adding removed/broken members for API compatibility
|
|
A new generated diff is ready to view.
|
Contributor
Author
|
Superseded by #1573 |
Affected ArtifactsChanged in size
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue #
(none)
Description of changes
This change adds support for sourcing account ID from IMDS credentials. There are some breaking changes around the
ImdsCredentialsProviderconstructor to remove no-longer-necessary parameters/fields and forEC2MetadataErrorto more gracefully model HTTP status codes.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.