[Feature] Add shared credential-refresh lifecycle to AWSCredentialsProvider by kai-ion · Pull Request #3930 · aws/aws-sdk-cpp

kai-ion · 2026-09-17T20:30:38Z

Issue #, if available:

Description of changes:
Add shared credential-refresh lifecycle to AWSCredentialsProvider

Check all that applies:

Did a review by yourself.
Added proper tests to cover this PR. (If tests are not applicable, explain.)
Checked if this PR is a breaking (APIs have been changed) change.
Checked if this PR will not introduce cross-platform inconsistent behavior.
Checked if this PR would require a ReadMe/Wiki update.

Check which platforms you have built SDK on to verify the correctness of this PR.

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

sbiscigl · 2026-09-24T21:28:08Z

+        class AWS_CORE_API CredentialsRefreshProvider : public AWSCredentialsProvider
+        {
+        public:
+            explicit CredentialsRefreshProvider(std::shared_ptr<CredentialsSource> source);


so lets think about this constructor and the new interface CredentialsSource. credentials refresh provider is meant to be like other SDKs where we wrap a existing credentials provider and cache that. there are however several issues with this

CredentialsSource is not a AWSCredentialsProvider

in the default chain we do something like

class chain(): public AWSCredentialsProvider { public: chain() { providers.push_back(std::make_shared<SomeCredentialsProvider>); } private: std::vector<AWSCredentialsProvider> providers }

now all of our implementations of AWSCredentialsProvider specifically in this example SomeCredentialsProvider how have to implement these functions. and implement a new interface. im not strictly against this we just need to weigh whether we want to do this or not.

this approach has two problems:

double caching/no caching

what about wrapping credentials provider that already cache like all of the CRT credentials providers. you are more or less unwrapping those, and making a parallel caching API which is a lot of surface area. additionally if you remove the caching layer, existing users of the credentials provider now have no caching.

_direct usage of the crednetials provider has no caching

directly constructing a credentials provider has no caching necessarily because only in the chain we wrap it.

before we do this, we need to evaluate how and where this caching will be added, and answer these questions.

Answered in the comment below

sbiscigl · 2026-09-30T14:00:56Z

+            virtual Aws::Utils::DateTime CurrentTime() const;
+
+        private:
+            std::shared_ptr<AWSCredentialsProvider> m_delegate;


if CredentialsCachingStateImpl is a unique pointer why is AWSCredentialsProvider a shared?

Providers are handed around as shared_ptr everywhere: AWSCredentialsProviderChain stores them that way (and keeps a second shared_ptr in m_cachedProvider for the last successful one), and the client constructors and ClientConfiguration take them that way. So a customer wrapping their own provider usually already holds a shared_ptr and may hand the same one to more than one client.

we can make cachingstate a shared pointer but I'm not sure what is the benefit to making it a shared pointer (im not willing to die on this hill tho)

sbiscigl · 2026-09-30T14:06:03Z

+        {
+        public:
+            explicit CredentialsCachingProvider(std::shared_ptr<AWSCredentialsProvider> delegate);
+            ~CredentialsCachingProvider() override;


needs final class, or virtual destructor

changed to a final

sbiscigl · 2026-09-30T14:34:45Z

+              }
+              return Aws::Internal::RefreshResult<AWSCredentials>::Fresh(credentials, credentials.GetExpiration());
+          },
+          [this]() { return CurrentTime(); }))


So lets start with the objective here which is "i need to mock a clock so i can test a class that is dependent on time". here you added to the public API a overridable API. that is something we do not want to do. it makes consumers pay for the testing mock you added.

There are two options:

Dont mock it. Why cant the values created in the tests mimic real events. i.e. the return of GetAWSCredentials is 100 percent mockable. why isnt the method of the delegate returning credentials with the values instead of injecting time stamps. I really think this is the way we should go, and will need to know a concrete reason why we cant do this, because otherwise we are adding to the public API just for testing.

passing in a std::function to the constructor, not a virtual function on the class. This is infact one of the explicit thing from effective c++ that you should not do. Never call virtual functions during
construction or destruction.. theres no reason for this to be virtual, passing it in the constructor as a callback makes more sense.

Yaa thats bad. Removed this public api

sbiscigl · 2026-09-30T14:37:32Z

+        }
+
+    protected:
+        DateTime CurrentTime() const override { return *m_now; }


why is this protected, and why is datetime a shared pointer

Removed this whole class

sbiscigl · 2026-09-30T14:38:50Z

+                NonRecoverable
+            };
+
+            RefreshResult() = default;


why is this default constructible, how is it in a valid state after default construction?

Removed the default construction

sbiscigl · 2026-09-30T14:41:26Z

+            };
+
+            RefreshResult() = default;
+            RefreshResult(Status status, CredentialsT credentials, Aws::Crt::Optional<Aws::Utils::DateTime> expiration,


you have three static functions that return three different sets of "valid variants" why is this a public constructor and not a private constructor. that would follow the factory pattern.

Yup updated as part of the other comment

sbiscigl · 2026-09-30T14:43:36Z

+        };
+
+        // Advisory window for a given credential lifetime.
+        inline std::chrono::milliseconds ComputeAdvisoryWindow(std::chrono::milliseconds lifetime)


all of these inlines are public in the header, why are they not private function in the class that uses them, having them as free-floating in the header makes is do anyone including thie header and use them. additioanlly these should likely be local to a cpp.

Youre right, moving the inline functions to private functions

sbiscigl · 2026-09-30T14:45:11Z

+        {
+            if (jitter01 < 0.0) { jitter01 = 0.0; }
+            if (jitter01 > 1.0) { jitter01 = 1.0; }
+            const int64_t span = hi.count() - lo.count();


what if this is negative?

ScaleJitter now returns lo when hi <= lo, so an inverted range can't produce a negative duration.

sbiscigl · 2026-09-30T14:45:36Z

+        }
+
+        // Feature gate (dark ship): off unless AWS_NEW_CREDENTIAL_REFRESH_2026 is "true".
+        inline bool IsNewCredentialsRefreshEnabled()


this function isnt called why is it here?

this is the gate that gets used later in the wiring PR

sbiscigl · 2026-09-30T14:47:26Z

+         * backoff, a single in-flight fetch, and serving the last-good credentials when a fetch fails.
+         * Empty credentials from the wrapped provider mark a failed fetch.
+         */
+        class AWS_CORE_API CredentialsCachingProvider : public AWSCredentialsProvider


how is this going to work in the SDK. how will it work in the existing deafult chain, and how will existing users who specifically use a credentials provider use this?

We are adding a cache member CachingCrendialsProvider to providers, and refresh behavior delegates to it

kai-ion force-pushed the refresh branch 2 times, most recently from b8d4051 to f47c72f Compare September 18, 2026 19:59

kai-ion marked this pull request as ready for review September 18, 2026 19:59

sbiscigl reviewed Sep 22, 2026

View reviewed changes

Comment thread src/aws-cpp-sdk-core/include/aws/core/auth/AWSCredentialsProvider.h Outdated

sbiscigl reviewed Sep 22, 2026

View reviewed changes

Comment thread src/aws-cpp-sdk-core/source/auth/AWSCredentialsProvider.cpp Outdated

kai-ion force-pushed the refresh branch from 7bf13e7 to 3fefc4c Compare September 23, 2026 17:13

sbiscigl reviewed Sep 24, 2026

View reviewed changes

sbiscigl reviewed Sep 30, 2026

View reviewed changes

Add shared credential-refresh lifecycle to AWSCredentialsProvider

867da2b

kai-ion force-pushed the refresh branch from afd7b1f to 867da2b Compare October 2, 2026 19:05

Conversation

kai-ion commented Sep 17, 2026

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kai-ion Sep 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

kai-ion Sep 30, 2026 •

edited

Loading