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

Replace credentials cache with identity cache #3077

Merged
merged 12 commits into from
Oct 21, 2023

Conversation

jdisanti
Copy link
Collaborator

@jdisanti jdisanti commented Oct 18, 2023

This PR replaces the credentials cache with the new identity cache, and adds config validation via the SharedConfigValidator runtime component and ValidateConfig trait.


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

@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@jdisanti jdisanti marked this pull request as ready for review October 20, 2023 01:41
@jdisanti jdisanti requested a review from a team as a code owner October 20, 2023 01:41
@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@jdisanti jdisanti added the breaking-change This will require a breaking change label Oct 20, 2023
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.

Looks fantastic! With this, have we already addressed the known credentials_cache/credentials_provider duality issue?

@@ -827,35 +834,5 @@ mod loader {
let num_requests = num_requests.load(Ordering::Relaxed);
assert!(num_requests > 0, "{}", num_requests);
}

#[tokio::test]
async fn time_source_is_passed() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this test no longer applicable or did it not add much value in the first place?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The time source and sleep aren't stored in the cache anymore, but rather, are passed in on every cache request. So there's nothing to test here.

///
/// This trait can be used to validate that certain required components or config values
/// are available, and provide an error with helpful instructions if they are not.
pub trait ValidateConfig: fmt::Debug + Send + Sync {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is great. Love the idea of having a centralized place for validating config 🎉

@jdisanti
Copy link
Collaborator Author

With this, have we already addressed the known credentials_cache/credentials_provider duality issue?

Yes. They're completely isolated from each other in config now.

let token = match preloaded_token {
Some(token) => Ok(token),
Some(token) => {
tracing::debug!(
Copy link
Collaborator

Choose a reason for hiding this comment

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

we probably want this at trace

@@ -102,6 +102,12 @@ pub use aws_types::{
/// Load default sources for all configuration with override support
pub use loader::ConfigLoader;

/// Types for configuring identity caching.
pub mod identity {
pub use aws_smithy_runtime::client::identity::IdentityCache;
Copy link
Collaborator

Choose a reason for hiding this comment

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

any issue with the re-export aws_smithy_runtime here?

Copy link
Collaborator Author

@jdisanti jdisanti Oct 20, 2023

Choose a reason for hiding this comment

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

I don't think it belongs in aws-smithy-runtime-api since that crate is intended for library authors, and concrete built-in identity cache implementations aren't needed for that. So that leaves us with two options: 1. put them in aws-smithy-runtime, or 2. create a new crate for them.

I opted to go for 1, but that probably means we have to make aws-smithy-runtime a stable crate.

/// Override the credentials cache used to build [`SdkConfig`](aws_types::SdkConfig).
/// Override the identity cache used to build [`SdkConfig`](aws_types::SdkConfig).
///
/// The identity cache caches credentials and SSO tokens. By default, a lazy cache is used
Copy link
Collaborator

Choose a reason for hiding this comment

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

AWS credentials?

Comment on lines 359 to 360
/// // change the load timeout to 10 seconds
/// .load_timeout(Duration::from_secs(10))
Copy link
Collaborator

Choose a reason for hiding this comment

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

may want to note that there may be other timeouts that could trigger

/// .load()
/// .await;
/// # }
/// ```
pub fn credentials_cache(mut self, credentials_cache: CredentialsCache) -> Self {
Copy link
Collaborator

Choose a reason for hiding this comment

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

deprecate credentials_cache and leave a breadcrumb?

@@ -44,6 +44,7 @@ pub struct InterceptorError {

impl InterceptorError {
interceptor_error_fn!(read_before_execution => ReadBeforeExecution (with source));
interceptor_error_fn!(read_after_configuration => ReadAfterConfiguration (with source));
Copy link
Collaborator

Choose a reason for hiding this comment

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

did you still mean to add this error? I could be searching badly but I don't see it used

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Nope. I meant to remove this. Good catch!

@@ -187,6 +187,8 @@ impl AuthScheme for SharedAuthScheme {
}
}

impl ValidateConfig for SharedAuthScheme {}
Copy link
Collaborator

Choose a reason for hiding this comment

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

should these be delegating to their inner members? (either now or eventially)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Eventually, yes. I figure we'll add them as they're needed.


#[derive(Clone)]
enum ValidatorInner {
BaseConfigStaticFn(fn(&RuntimeComponentsBuilder, &ConfigBag) -> Result<(), BoxError>),
Copy link
Collaborator

Choose a reason for hiding this comment

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

not sure it's worth having the two variants—I think you could turn one into the other during construction?

doesn't really matter anyway though—it's private.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was just avoiding unnecessary allocation.

Comment on lines 103 to 104
/// Creates a base client validator from a function.
pub fn base_client_config_fn(
Copy link
Collaborator

Choose a reason for hiding this comment

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

example may be helpful

Comment on lines +68 to 73
tracing::trace!(concat!(
"running `",
stringify!($interceptor),
"` interceptors"
));
let mut result: Result<(), (&str, BoxError)> = Ok(());
Copy link
Collaborator

Choose a reason for hiding this comment

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

if we logging here, may be nice to log how many too

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We don't actually know the count until we've looped over them since we only have an iterator here.

Copy link
Collaborator

@rcoh rcoh left a comment

Choose a reason for hiding this comment

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

Look great!

@jdisanti jdisanti requested a review from a team as a code owner October 20, 2023 22:37
@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@jdisanti jdisanti added this pull request to the merge queue Oct 20, 2023
Merged via the queue into main with commit 40f4662 Oct 21, 2023
40 of 41 checks passed
@jdisanti jdisanti deleted the jdisanti-use-identity-cache branch October 21, 2023 00:02
github-merge-queue bot pushed a commit that referenced this pull request Nov 10, 2023
…only at operation level (#3156)

## Motivation and Context
Fixes awslabs/aws-sdk-rust#901

## Description
This PR is a rework of #3021
whose fix was inadvertently discarded during
#3077. The way we fix the issue
is slightly different. In this PR, we add an identity resolver to
runtime components within `set_credentials_provider`, instead of using
`ServiceConfig.OperationConfigOverride`.

## Testing
Added a Kotlin integration test to `CredentialProviderConfigTest.kt`
based on the customer reported issue.

## 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 AWS
SDK, generated SDK code, or SDK 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._
rcoh pushed a commit that referenced this pull request Nov 13, 2023
…only at operation level (#3156)

Fixes awslabs/aws-sdk-rust#901

This PR is a rework of #3021
whose fix was inadvertently discarded during
#3077. The way we fix the issue
is slightly different. In this PR, we add an identity resolver to
runtime components within `set_credentials_provider`, instead of using
`ServiceConfig.OperationConfigOverride`.

Added a Kotlin integration test to `CredentialProviderConfigTest.kt`
based on the customer reported issue.

<!--- 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 AWS
SDK, generated SDK code, or SDK 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._
rcoh pushed a commit that referenced this pull request Nov 13, 2023
…only at operation level (#3156)

Fixes awslabs/aws-sdk-rust#901

This PR is a rework of #3021
whose fix was inadvertently discarded during
#3077. The way we fix the issue
is slightly different. In this PR, we add an identity resolver to
runtime components within `set_credentials_provider`, instead of using
`ServiceConfig.OperationConfigOverride`.

Added a Kotlin integration test to `CredentialProviderConfigTest.kt`
based on the customer reported issue.

<!--- 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 AWS
SDK, generated SDK code, or SDK 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._
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change This will require a breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants