rfc43: fix identity cache partition #3566
Conversation
|
A new generated diff is ready to view.
A new doc preview is ready to view. |
| use aws_smithy_runtime::client::http::test_util::infallible_client_fn; | ||
|
|
||
| #[tokio::test] | ||
| async fn test_identity_cache_reused_by_default() { |
There was a problem hiding this comment.
If an integration test is service agnostic, it's a good practice we write it in Kotlin. Since the test uses credentials_provider, you can use awsSdkIntegrationTest (example).
There was a problem hiding this comment.
Does this end up on every generated client? If so is that what we want here?
It does use credentials_provider but with fake/test credentials. It really is just testing the cache behavior and interaction between SdkConfig and generated client config. It doesn't make a real request.
There was a problem hiding this comment.
I don't think it does. awsSdkIntegrationTest is a test helper that tests codegen itself but does not run against generated AWS SDKs.
It really is just testing the cache behavior and interaction between SdkConfig and generated client config. It doesn't make a real request.
That does sound like service-agnostic, and that's what awsSdkIntegrationTest is for
There was a problem hiding this comment.
Just throwing one last thing on the table before leaving tests in integration-tests/s3, can we directly construct SdkConfig in tests with necessary fields without going through aws_config::defaults? I'm assuming the tests won't need default configs in order to verify what they are supposed to test? If we don't use aws-config, then we may be able to avoid the problem we discussed offline.
But I am ok with what we have, and wanted to double-check.
There was a problem hiding this comment.
We can which would allow us to test some of this but not how different behavior versions behave in loading (which still require a subsequent PR to fully implement) if I understand correctly how it's all wired.
CHANGELOG.next.toml
Outdated
| [[smithy-rs]] | ||
| message = """ | ||
| Fixes the identity resolver types (`credentials_provider()` and `token_provider()`) from `SdkConfig` to have | ||
| a consistent identity cache partition when re-used across different clients. | ||
| """ | ||
| references = ["smithy-rs#3427"] | ||
| meta = { "breaking" = false, "tada" = false, "bug" = true } | ||
| authors = ["aajtodd"] |
There was a problem hiding this comment.
I would make this changelog entry [[aws-sdk-rust]] since credentials_provider and token_provider are AWS-related concepts, rather than general smithy concepts.
In addition, we can also create a changelog entry for [[smithy-rs]] mentioning that SharedIdentityResolver now respects a cache partition reserved by the passed-in IdentityResolver.
There was a problem hiding this comment.
Sure I can update it as suggested but I want to take issue with this comment for a second:
since credentials_provider and token_provider are AWS-related concepts, rather than general smithy concepts.
From Smithy and SRA standpoint they are not AWS SDK specific. They are "AWS" concepts in that they originate from AWS but they are not "AWS SDK" only concepts. A customer could use sigv4 in their own model if they wanted (or internally within Amazon/AWS). The way Rust has this organized now is not entirely correct. Sigv4 and token provider support should be in the Smithy runtime (the specific providers like SsoTokenProvider or DefaultCredentialsChain are of course AWS SDK specific though).
There was a problem hiding this comment.
From Smithy and SRA standpoint they are not AWS SDK specific
Good point. To be clear, what I meant was a tooling standpoint that's processing CHANGELOG.next.toml. When we write a changelog entry for [smithy-rs], that entry only appears in the release notes in the smithy-rs repository, but not in the aws-sdk-rust repository. So when those who only use Smithy SDK, but not AWS SDK, see a changelog entry mentioning credentials_provider and token_provider they don't find the methods because they are defined in the AWS runtime crates & generated by AWS codegen decorators. It can confuse customers.
The way Rust has this organized now is not entirely correct. Sigv4 and token provider support should be in the Smithy runtime
Interesting. Till we address it, I'd be consistent with the way it has been.
|
A new generated diff is ready to view.
A new doc preview is ready to view. |
aajtodd
force-pushed
the
atodd/fix-identity-cache
branch
from
a59d580
to
ceb5b99
Compare
aajtodd
changed the title
|
A new generated diff is ready to view.
A new doc preview is ready to view. |
Motivation and Context
Original issue: #3427
RFC: #3544
Description
Fixes the
SharedCredentialsProviderandSharedTokenProviderto re-use a consistent cache partition by default.SdkConfigdoes not create an identity cache by default still, that will need to be a follow on PR that gates it behind a new behavior version. Ideally we do that with the stalled stream protection work in #3527Testing
Added new unit and integration tests.
Checklist
CHANGELOG.next.tomlif I made changes to the smithy-rs codegen or runtime cratesBy submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.