Implement an identity cache in aws-smithy-runtime #3062

jdisanti · 2023-10-12T00:27:34Z

This PR adds a ResolveCachedIdentity trait, and adapts the existing LazyCredentialsCache implementation to it. It does NOT make this cache used yet, as that will be done as a separate PR to keep code review smaller.

Notable differences from the credentials cache:

Supports multiple different identity types and identity resolvers
Applies cache partitioning to the SharedIdentityResolver so that operation config overrides of the identity resolver will result in the correct identity being resolved.

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 · 2023-10-13T00:12:34Z

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.

rcoh

look good — some potential other ideas in line

rcoh · 2023-10-13T16:47:56Z

rust-runtime/aws-smithy-runtime/src/client/identity/cache/lazy.rs

+    pub fn buffer_time_jitter_fraction(mut self, buffer_time_jitter_fraction: fn() -> f64) -> Self {
+        self.set_buffer_time_jitter_fraction(Some(buffer_time_jitter_fraction));
+        self
+    }


does this need to be pub, even feature gated?

It doesn't look like this is even used in tests anymore. I'm going to make it private and cfg(test) in case its needed in the future.

rcoh · 2023-10-13T16:49:13Z

rust-runtime/aws-smithy-runtime/src/client/identity/cache/lazy.rs

+        LazyCache::new(
+            self.time_source.unwrap_or_default(),
+            self.sleep_impl.unwrap_or_else(|| {
+                default_async_sleep().expect("no default sleep implementation available")


should this come from a runtime plugin / runtime components?

Good call out. I refactored it so that these come in through a RuntimeComponents argument on ResolveIdentity so that they will work correctly with config overrides.

rcoh · 2023-10-13T16:50:59Z

rust-runtime/aws-smithy-runtime/src/client/identity/cache/lazy.rs

+        let mut partition = self.partitions.read().unwrap().get(&key).cloned();
+        if partition.is_none() {
+            let mut partitions = self.partitions.write().unwrap();
+            // Another thread could have inserted the partition before we acquired the lock,
+            // so double check before inserting it.
+            partitions
+                .entry(key)
+                .or_insert_with(|| ExpiringCache::new(self.buffer_time));
+            drop(partitions);
+
+            partition = self.partitions.read().unwrap().get(&key).cloned();
+        }
+        partition.expect("inserted above if not present")


because it's so easy to mess up RWLock caches, you may want to consider writing the code in a way where you explictly drop() the locks

rcoh · 2023-10-13T16:52:18Z

rust-runtime/aws-smithy-runtime/src/client/identity/cache/lazy.rs

+                .or_insert_with(|| ExpiringCache::new(self.buffer_time));
+            drop(partitions);
+
+            partition = self.partitions.read().unwrap().get(&key).cloned();


since this pattern only works for append-only caches, it may be worth noting that entries are never removed.

rcoh · 2023-10-13T16:54:01Z

rust-runtime/aws-smithy-runtime/src/client/identity/cache/lazy.rs

+                            let jitter = self
+                                .buffer_time
+                                .mul_f64((self.buffer_time_jitter_fraction)());
+
+                            // Logging for cache miss should be emitted here as opposed to after the call to
+                            // `cache.get_or_load` above. In the case of multiple threads concurrently executing
+                            // `cache.get_or_load`, logging inside `cache.get_or_load` ensures that it is emitted
+                            // only once for the first thread that succeeds in populating a cache value.
+                            tracing::info!(
+                                "identity cache miss occurred; added new AWS identity (took {:?})",
+                                time.now().duration_since(start_time)
+                            );


should probably note the impact of jitter on the buffer time in the docs above.

rcoh · 2023-10-13T16:54:54Z

rust-runtime/aws-smithy-runtime/src/client/identity/cache.rs

+        NoCache.into_shared()
+    }
+
+    /// Configure a lazy identity cache.


one more line that identities are lazily loaded when requested would probably be helpful here

rcoh · 2023-10-13T16:55:26Z

rust-runtime/aws-smithy-runtime/src/client/identity/cache.rs

+    fn resolve_cached_identity<'a>(
+        &'a self,
+        resolver: SharedIdentityResolver,
+        config_bag: &'a ConfigBag,


do we want to be giving it the entire config bag? should this have just auth params instead?

Identity resolution isn't supposed to have access to the auth parameters per the spec.

rcoh · 2023-10-13T16:56:18Z

rust-runtime/aws-smithy-runtime/src/expiring_cache.rs

+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+use std::future::Future;


do we want this to be in runtime-api instead? I think we've hit other issues where this primitive wasn't accessible to the dependency tree.

It looks like aws-config is the only other place that uses it, and that depends on aws-smithy-runtime. So I think it's fine where it is. We can always move/re-export it later if we need to.

github-actions · 2023-10-13T20:58:19Z

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.

github-actions · 2023-10-16T23:43:09Z

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.

github-actions · 2023-10-17T17:14:00Z

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.

github-actions · 2023-10-17T17:51:32Z

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.

jdisanti added the needs-sdk-review label Oct 12, 2023

jdisanti requested a review from a team as a code owner October 12, 2023 00:27

jdisanti force-pushed the jdisanti-identity-cache branch 3 times, most recently from 80861fb to 67665e7 Compare October 12, 2023 23:36

smithy-lang deleted a comment from github-actions bot Oct 12, 2023

jdisanti force-pushed the jdisanti-identity-cache branch from 67665e7 to 4574ba7 Compare October 12, 2023 23:55

smithy-lang deleted a comment from github-actions bot Oct 12, 2023

rcoh approved these changes Oct 13, 2023

View reviewed changes

Base automatically changed from jdisanti-async-trait-lifetimes to main October 13, 2023 17:19

jdisanti requested a review from a team as a code owner October 13, 2023 17:19

jdisanti added 9 commits October 13, 2023 10:50

Implement an identity cache in aws-smithy-runtime

bd042cf

Fix clippy

7ed5088

Fix doc comment

dcc3694

Fix feature issue

5a545ad

Fix some CI issues

7751d02

Fix more CI issues

d18e9ca

Remove redundant bound from ResolveCachedIdentity

05305dd

Incorporate feedback

c40a607

Merge remote-tracking branch 'origin/main' into jdisanti-identity-cache

b7b7b97

jdisanti force-pushed the jdisanti-identity-cache branch from 4574ba7 to b7b7b97 Compare October 13, 2023 20:39

Merge branch 'main' into jdisanti-identity-cache

c476c5a

Merge remote-tracking branch 'origin/main' into jdisanti-identity-cache

2a09cb6

Merge remote-tracking branch 'origin/main' into jdisanti-identity-cache

b8f676f

jdisanti added this pull request to the merge queue Oct 17, 2023

Merged via the queue into main with commit d48acae Oct 17, 2023
40 of 41 checks passed

jdisanti deleted the jdisanti-identity-cache branch October 17, 2023 20:35

Velfi removed the needs-sdk-review label Oct 19, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement an identity cache in aws-smithy-runtime #3062

Implement an identity cache in aws-smithy-runtime #3062

jdisanti commented Oct 12, 2023

github-actions bot commented Oct 13, 2023

rcoh left a comment

rcoh Oct 13, 2023

jdisanti Oct 13, 2023

rcoh Oct 13, 2023

jdisanti Oct 13, 2023

rcoh Oct 13, 2023

rcoh Oct 13, 2023

rcoh Oct 13, 2023

rcoh Oct 13, 2023

rcoh Oct 13, 2023

jdisanti Oct 13, 2023

rcoh Oct 13, 2023

jdisanti Oct 13, 2023

github-actions bot commented Oct 13, 2023

github-actions bot commented Oct 16, 2023

github-actions bot commented Oct 17, 2023

github-actions bot commented Oct 17, 2023

Implement an identity cache in aws-smithy-runtime #3062

Implement an identity cache in aws-smithy-runtime #3062

Conversation

jdisanti commented Oct 12, 2023

github-actions bot commented Oct 13, 2023

rcoh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Oct 13, 2023

github-actions bot commented Oct 16, 2023

github-actions bot commented Oct 17, 2023

github-actions bot commented Oct 17, 2023