Skip to content
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

fix default credential chain not respecting endpoint URL overrides #3873

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

aajtodd
Copy link
Contributor

@aajtodd aajtodd commented Oct 10, 2024

Motivation and Context

awslabs/aws-sdk-rust#1193

Description

This PR fixes a customer reported bug where the default chain doesn't respect AWS_ENDPOINT_URL/AWS_ENDPOINT_URL_<SERVICE> environment variables or the equivalents in AWS shared config (~/.aws/config).

This fix is a little nuanced and frankly gross but there isn't a better option that I can see right now that isn't way more invasive. The crux of the issue is that when we implemented support for this feature (1, 2, 3) we really only made it work for clients created via ConfigLoader::load(). Internally the default chain credential provider constructs STS and SSO clients but it does so using ProviderConfig by mapping this to SdkConfig via ProviderConfig::client_config(). This conversion is used in several places and it doesn't take any of the required logic into account to setup EnvServiceConfig which is what generated SDK's ultimately use to figure out the endpoint URL from either environment/profile (example client which ultimately ends up in EnvServiceConfig here).

The fix applied here is nuanced in that we update the conversion to provide a EnvServiceConfig but it relies on the profile to have been parsed already or else you'll get an empty/default profile. This generally works for the profile provider since the first thing we do is load the profile but in isolation it may not work as expected. I've added tests for STS to cover all cases but SSO credentials and token providers do NOT currently respect shared config endpoint URL keys. Fixing this is possible but involved since we require an async context to ensure a profile is loaded already and in many places where we construct SdkConfig from ProviderConfig we are in non async function.

Testing

Tested repro + additional integration tests

Future

This does not fix awslabs/aws-sdk-rust#1194 which was discovered as a bug/gap. Fixing it would be outside the scope of this PR.

SSO/token provider is instantiated sometimes before we have parsed a profile. This PR definitely fixes the STS provider for all configuration scenarios but the SSO related client usage may still have some edge cases when configured via profiles since we often instantiate them before parsing a profile. When we surveyed other SDKs there were several that failed to respect these variables and haven't received issues around this which leads me to believe this isn't likely a problem in practice (most likely due to SSO being used in local development most often where redirecting that endpoint doesn't make much sense anyway).

Checklist

  • For changes to the AWS SDK, generated SDK code, or SDK runtime crates, I have created a changelog entry Markdown file in the .changelog directory, specifying "aws-sdk-rust" in the applies_to key.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Copy link

A new generated diff is ready to view.

  • AWS SDK (ignoring whitespace)
  • No codegen difference in the Client Test
  • No codegen difference in the Server Test
  • No codegen difference in the Server Test Python
  • No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

@aajtodd aajtodd marked this pull request as ready for review October 15, 2024 14:52
@aajtodd aajtodd requested review from a team as code owners October 15, 2024 14:53
Copy link

A new generated diff is ready to view.

  • AWS SDK (ignoring whitespace)
  • No codegen difference in the Client Test
  • No codegen difference in the Server Test
  • No codegen difference in the Server Test Python
  • No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

Copy link

A new generated diff is ready to view.

  • AWS SDK (ignoring whitespace)
  • No codegen difference in the Client Test
  • No codegen difference in the Server Test
  • No codegen difference in the Server Test Python
  • No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

Copy link
Contributor

@ysaito1001 ysaito1001 left a comment

Choose a reason for hiding this comment

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

Great work on PR description and new tests added. The change looks good, without disturbing existing code to fix the bug.

Copy link
Contributor

@landonxjames landonxjames left a comment

Choose a reason for hiding this comment

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

Did we ever get around to starting that list of things we would like to change in a V2 if we ever get around to it? If so the whole ProvderConfig thing should probably go near the top.

I think this is the best solution we could hope for given the current setup.

@aajtodd
Copy link
Contributor Author

aajtodd commented Oct 17, 2024

Did we ever get around to starting that list of things we would like to change in a V2 if we ever get around to it? If so the whole ProvderConfig thing should probably go near the top.

I think this is the best solution we could hope for given the current setup.

I don't see a list anywhere maybe we should start one and make it a dumping ground for future ideas.

Copy link

A new generated diff is ready to view.

  • AWS SDK (ignoring whitespace)
  • No codegen difference in the Client Test
  • No codegen difference in the Server Test
  • No codegen difference in the Server Test Python
  • No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

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.

Environment and shared profile config for ignoring endpoint urls is not respected for service specific config
3 participants