rfc: change default identity cache and partitioning #3544
Conversation
There was a problem hiding this comment.
design LGTM. I think this is missing a change checklist item for actually using the cache partition in the identity cache. It may be helpful to also add some color about how the goal of the identity cache partitions was to create a concept of pointer equality for identity providers so that we could decouple the identity resolver themselves from the cache implementation.
May be helpful to answer the question of how we will create new partitions etc.
|
I think the RFC may also want to discuss the alternatives to address this issue |
My understanding is that the cache partition is already used? and it's just that a |
That is coming from |
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
|
|
||
| ```rust,ignore | ||
| let config = aws_config::defaults(BehaviorVersion::latest()) | ||
| .set_identity_cache(None) |
There was a problem hiding this comment.
This apparently doesn't exist on ConfigLoader, it only exists on the explicit client config. We'll probably instead need something like no_identity_cache() to match no_credentials()
aajtodd
force-pushed
the
atodd/rfc-identity-cache-partition
branch
from
e63895f
to
4a8757c
Compare
| each time a client is created. | ||
| * More specifically this happens every time a `SharedIdentityResolver` is [created](https://github.com/smithy-lang/smithy-rs/blob/release-2024-03-25/rust-runtime/aws-smithy-runtime-api/src/client/identity.rs#L197). The conversion from [`From<SdkConfig>`](https://github.com/awslabs/aws-sdk-rust/blob/release-2024-04-01/sdk/sts/src/config.rs#L1207) | ||
| sets the credentials provider which [associates](https://github.com/awslabs/aws-sdk-rust/blob/release-2024-04-01/sdk/sts/src/config.rs#L960) it as | ||
| the identity resolver for the auth scheme. Internally this is [converted](https://github.com/awslabs/aws-sdk-rust/blob/release-2024-04-01/sdk/aws-smithy-runtime-api/src/client/runtime_components.rs#L663) to `SharedIdentityResolver` which creates the new partition. The end result is the credentials |
There was a problem hiding this comment.
If it were already a SharedIdentityResolver, then the IntoShared implementation should detect that and wouldn't create a new instance. So that means it must be a SharedCredentialsProvider that's getting converted into a SharedIdentityResolver. This could be made clearer here.
|
|
||
| ``` | ||
|
|
||
| Additionally a new behavior version must be introduced that conditionally creates a default `IdentityCache` on `SdkConfig` |
There was a problem hiding this comment.
Most of the defaults are provided by runtime plugins later on. Why would this one get a default at this stage?
There was a problem hiding this comment.
If we don't then each client gets their own identity cache rather than sharing one. Even if we fix the cache partitioning the default behavior will still end up being that multiple clients created from SdkConfig don't actually share cached results.
This can be worked around of course by explicitly providing a cache to SdkConfig but if the default behavior we want is to enable caching it probably makes sense to just make it so.
aajtodd
force-pushed
the
atodd/rfc-identity-cache-partition
branch
from
4a8757c
to
16799c0
Compare
aajtodd
force-pushed
the
atodd/rfc-identity-cache-partition
branch
from
16799c0
to
0922240
Compare
## 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 --> Original issue: #3427 RFC: #3544 ## Description <!--- Describe your changes in detail --> Fixes the `SharedCredentialsProvider` and `SharedTokenProvider` to re-use a consistent cache partition by default. `SdkConfig` does 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 #3527 ## Testing <!--- Please describe in detail how you tested your changes --> <!--- Include details of your testing environment, and the tests you ran to --> <!--- see how your change affects other areas of the code, etc. --> Added new unit and integration tests. ## Checklist <!--- If a checkbox below is not applicable, then please DELETE it rather than leaving it unchecked --> - [X] I have updated `CHANGELOG.next.toml` if I made changes to the smithy-rs codegen or runtime crates ---- _By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice._
Motivation and Context
Original issue: #3427
See the motivation section of the RFC.
Description
Proposes a new RFC that changes the default behavior of
SdkConfigandSharedCredentialsProvider/SharedTokenProviderw.r.t caching and cache partitions.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.