Skip to content

feat: Decompose CRT default chain into individual wrappers #1926

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

Merged
merged 3 commits into from
Apr 30, 2025

Conversation

sichanyoo
Copy link
Contributor

Issue #

2380

Description of changes

  • Instead of using CRT's .defaultChain credential resolver, just directly use SDK-side wrappers for each individual CRT credential provider in the same order that .defaultChain uses. This sets the stage for moving off of CRT credential provider one by one, by replacing individual provider's internals that currently uses CRT, with SDK side implementation.

New/existing dependencies impact assessment, if applicable

Conventional Commits

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@sichanyoo sichanyoo requested review from jbelkins and dayaffe April 28, 2025 19:39
Comment on lines +54 to +55
// The error thrown from the last resolver is not caught and instead gets thrown to caller.
return try await resolverFactories[lastIndex]().getIdentity(identityProperties: identityProperties)
Copy link
Contributor

Choose a reason for hiding this comment

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

will this always throw the IMDS error if all resolver factories fail? If so this was a huge pain from the user perspective when we used CRT so we should take this opportunity to throw a more descriptive error

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a great point, should we include that as part of the final breaking change PR tho? To maintain error behavior the same for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

@jbelkins what do you think? Change this now or change this later

Copy link
Contributor

Choose a reason for hiding this comment

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

I tend to agree with changing this now. The error should give some info about the specific providers in the chain that failed. Returning an IMDS error just because that’s the last in the chain does not make it clear what has really happened

Copy link
Contributor

Choose a reason for hiding this comment

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

After discussion, I recommend changing this error along with other breaking changes, i.e. adding to or reordering the chain.

Copy link
Contributor

@jbelkins jbelkins left a comment

Choose a reason for hiding this comment

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

Let's add a "chain failed" error at the same time we add providers to the chain

@jbelkins jbelkins self-requested a review April 30, 2025 17:23
@sichanyoo
Copy link
Contributor Author

After team discussion, decided to add more descriptive error throwing behavior at the end of the default chain overhaul project. The final breaking change PR for the project will re-order credential resolvers to match the SEP as well as throw more descriptive error at the end of the chain if every resolver failed to get credentials.

@sichanyoo sichanyoo merged commit 5895251 into main Apr 30, 2025
41 of 42 checks passed
@sichanyoo sichanyoo deleted the feat/decompose-defaultchain branch April 30, 2025 23:04
sichanyoo added a commit that referenced this pull request May 1, 2025
sichanyoo added a commit that referenced this pull request May 1, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants