Skip to content

[Feature] Add shared credential-refresh lifecycle to AWSCredentialsProvider - #3930

Open
kai-ion wants to merge 1 commit into
mainfrom
refresh
Open

kai-ion wants to merge 1 commit into
mainfrom
refresh

Conversation

@kai-ion

@kai-ion kai-ion commented Sep 17, 2026

Copy link
Copy Markdown
Collaborator

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.

  • Linux
  • Windows
  • Android
  • MacOS
  • IOS
  • Other Platforms

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

@kai-ion
kai-ion force-pushed the refresh branch 2 times, most recently from b8d4051 to f47c72f Compare September 18, 2026 19:59
@kai-ion
kai-ion marked this pull request as ready for review September 18, 2026 19:59
* One attempt against the credential source, classified fresh/recoverable/non-recoverable.
* Providers on the refresh lifecycle override this; the default returns a recoverable failure.
*/
virtual Aws::Auth::RefreshResult<AWSCredentials> FetchCredentialsFromSource();

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

so FetchCredentialsFromSource is a protected virtual function on AWSCredentialsProvider. so a inheriting class can override it. but FetchCredentialsFromSource is not used in the GetAWSCredentials as that is the method the SDK calls to fetch credentials. so how is the override actually planned on being used?

AWSCredentialsProvider::AWSCredentialsProvider()
: m_lastLoadedMs(0),
m_refreshState(new Aws::Internal::CredentialRefreshStateImpl(
[this]() { return FetchCredentialsFromSource(); },

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

you've created a side-car crendeitals provider in the crednetials provider, so you're packaging two things in one, and its the incorrect layering.

you should have a caching credentials provider over the existing credentials provider that executes the caching

class CachingCredentialsProvider: public CredentialsProvider {
   CachingCredentialsProvider(CredentialsProvider* credentialsProvider) {
       ....
   }

   GetCredentials() {
      if (cache) { return cached_creds}
      cache(delegated_creds_provider->GetCreds)
   }
}

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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants