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

Environment and shared profile config for ignoring endpoint urls is not respected for service specific config #1194

Closed
aajtodd opened this issue Sep 20, 2024 · 5 comments · Fixed by smithy-lang/smithy-rs#3873
Labels
bug This issue is a bug. p2 This is a standard priority issue pending-release This issue will be fixed by an approved PR that hasn't been released yet.

Comments

@aajtodd
Copy link
Contributor

aajtodd commented Sep 20, 2024

Describe the bug

AWS_IGNORE_CONFIGURED_ENDPOINT_URLS and ignore_configured_endpoint_urls are used to disable endpoint urls via environment and shared profile config.

We seem to respect them for the global config when using config loader but not for service specific env/keys (example)

Expected Behavior

Service specific environment should also take these settings into account when loading from SdkConfig.

Current Behavior

Service clients still use environment settings.

Reproduction Steps

let aws_config = aws_config::defaults(aws_config::BehaviorVersion::latest()).load().await;
let sts = aws_sdk_sts::Client::new(&aws_config);
let result = sts.get_caller_identity().send().await;
println!("resolved ident: {:#?}", result);
export AWS_ENDPOINT_URL_STS=http://127.0.0.1:4566
export AWS_IGNORE_CONFIGURED_ENDPOINT_URLS=true
export AWS_PROFILE=my-profile
RUST_LOG=trace cargo run

Possible Solution

No response

Additional Information/Context

No response

Version

`aws-config: 1.5.6` and latest service clients (sts `1.43.0` at this time).

Environment details (OS name and version, etc.)

all

Logs

No response

@aajtodd aajtodd added bug This issue is a bug. p2 This is a standard priority issue labels Sep 20, 2024
@aajtodd
Copy link
Contributor Author

aajtodd commented Oct 15, 2024

related issue: #1193

@aajtodd
Copy link
Contributor Author

aajtodd commented Oct 15, 2024

NOTE the SEP has this to say:

SDKs MUST read a parameter using the standard parameter resolution logic (idiomatic client configuration, environment variable, and shared configuration file) boolean option to disable the feature. The client configuration and shared configuration profile value MUST be named ignore_configured_endpoint_urls, and the environment variable AWS_IGNORE_CONFIGURED_ENDPOINT_URLS.

This would indicate SdkConfig should also have an option for programmatically disabling endpoint URL detection via environment/shared profiles. This would also be how we change codegen From<SdkConfig> implementation to look at that field and decide whether to look at the service specific env/profile keys.

We only implemented support for ignoring these at the config loader level .

github-merge-queue bot pushed a commit to smithy-lang/smithy-rs that referenced this issue Oct 21, 2024
…3873)

## Motivation and Context
<!--- Why is this change required? What problem does it solve? -->
<!--- If it fixes an open issue, please link to the issue here -->
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](#3568),
[2](#3493),
[3](#3488)) we really only
made it work for clients created via
[`ConfigLoader::load()`](https://github.com/smithy-lang/smithy-rs/blob/release-2024-10-09/aws/rust-runtime/aws-config/src/lib.rs#L871).
Internally the default chain credential provider constructs `STS` and
`SSO` clients but it does so using
[`ProviderConfig`](https://github.com/smithy-lang/smithy-rs/blob/release-2024-10-09/aws/rust-runtime/aws-config/src/provider_config.rs#L36)
by mapping this to `SdkConfig` via
[`ProviderConfig::client_config()`](https://github.com/smithy-lang/smithy-rs/blob/release-2024-10-09/aws/rust-runtime/aws-config/src/provider_config.rs#L199).
This conversion is used in several places and it doesn't take any of the
required logic into account to setup
[`EnvServiceConfig`](https://github.com/smithy-lang/smithy-rs/blob/release-2024-10-09/aws/rust-runtime/aws-config/src/lib.rs#L859-L862)
which is what generated SDK's ultimately use to figure out the endpoint
URL from either environment/profile ([example
client](https://github.com/awslabs/aws-sdk-rust/blob/release-2024-10-09/sdk/sts/src/config.rs#L1214-L1221)
which ultimately ends up in `EnvServiceConfig`
[here](https://github.com/smithy-lang/smithy-rs/blob/release-2024-10-09/aws/rust-runtime/aws-config/src/env_service_config.rs#L18)).

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
- [X] 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

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

@aajtodd aajtodd added the pending-release This issue will be fixed by an approved PR that hasn't been released yet. label Oct 21, 2024
@aajtodd
Copy link
Contributor Author

aajtodd commented Oct 21, 2024

This has been fixed and staged for release. Will go out when we next release smithy-rs

Copy link

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue is a bug. p2 This is a standard priority issue pending-release This issue will be fixed by an approved PR that hasn't been released yet.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant