Conversation
b8d4051 to
f47c72f
Compare
| * 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(); |
There was a problem hiding this comment.
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(); }, |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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.
Issue #, if available:
Description of changes:
Add shared credential-refresh lifecycle to AWSCredentialsProvider
Check all that applies:
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.