Adding CRTProfileCredentialsProvider for CRT-based profile credential #3713
Conversation
Adding CRTProfileCredentialsProvider for CRT-based profile credential
| void Reload() override; | ||
|
|
||
| private: | ||
| class Impl; |
There was a problem hiding this comment.
So when trying to do pimpl with the profile credentials provider i see it more so going like this.
In the real profile credentials provider
class AWS_CORE_API ProfileConfigFileAWSCredentialsProvider : public AWSCredentialsProvider
{
public:
ProfileConfigFileAWSCredentialsProvider(long refreshRateMs = REFRESH_THRESHOLD);
ProfileConfigFileAWSCredentialsProvider(const char* profile, long refreshRateMs = REFRESH_THRESHOLD);
AWSCredentials GetAWSCredentials() override;
static Aws::String GetCredentialsProfileFilename();
static Aws::String GetProfileDirectory();
protected:
void Reload() override;
private:
void RefreshIfExpired();
Aws::String m_profileToUse;
Aws::Config::AWSConfigFileProfileConfigLoader m_credentialsFileLoader;
long m_loadFrequencyMs;
};becomes
class ProfileCredentialsProviderImpl;
class AWS_CORE_API ProfileConfigFileAWSCredentialsProvider : public AWSCredentialsProvider
{
public:
ProfileConfigFileAWSCredentialsProvider(long refreshRateMs = REFRESH_THRESHOLD);
ProfileConfigFileAWSCredentialsProvider(const char* profile, long refreshRateMs = REFRESH_THRESHOLD);
AWSCredentials GetAWSCredentials() override;
static Aws::String GetCredentialsProfileFilename();
static Aws::String GetProfileDirectory();
protected:
void Reload() override;
Aws::UniquePtr<ProfileCredentialsProviderImpl> m_impl;
};then in the cpp file for for ProfileConfigFileAWSCredentialsProvider we construct a point to ProfileCredentialsProviderImpl.
That is step one to move the existing credentials provider to PIMPL.
Then step two is your change, ProfileCredentialsProviderImpl is changed to the implementation of your existing CRTProfileCredentialsProvider.
So:
Step 1: change ProfileConfigFileAWSCredentialsProvider to use PIMPL to current existing implementation called ProfileCredentialsProviderImpl.
Step 2: change ProfileCredentialsProviderImpl to us the implementation from this PR.
…ofile-credentials-provider
…ofile-credentials-provider
…le provider to the default chain
merge branch 'main' of https://github.com/aws/aws-sdk-cpp into crt-profile-credentials-provider
| * CRT-based credentials provider that sources credentials from config files with full SEP compliance. | ||
| * Supports assume role, credential_source, role chaining, and all SEP scenarios. | ||
| */ | ||
| class AWS_CORE_API CRTProfileCredentialsProvider : public AWSCredentialsProvider |
There was a problem hiding this comment.
lets not name this CRTProfileCredentialsProvider because its just going to be the new ProfileCredentialsProvider. lets just name it ProfileCredentialsProvider. also please add a AWS_DEPRECATED to the old class so that people using it directly will get a warning to update to the new one.
| protected: | ||
| void Reload() override; | ||
| class CRTProfileCredentialsProviderImp; | ||
| std::shared_ptr<CRTProfileCredentialsProviderImp> m_impl; |
| std::shared_ptr<CRTProfileCredentialsProviderImp> m_impl; | ||
|
|
||
| private: | ||
| void RefreshIfExpired(); |
There was a problem hiding this comment.
why do we still need this private method? cant we jsut move it to the impl?
| long m_loadFrequencyMs; | ||
|
|
||
| CRTProfileCredentialsProviderImp(long refreshRateMs) : | ||
| m_profileToUse(Aws::Auth::GetConfigProfileName()), |
There was a problem hiding this comment.
we should still keep data members as private. ideally the impl should just be the interface of a aws credentials provider
|
|
||
| void CRTProfileCredentialsProvider::Reload() | ||
| { | ||
| m_impl->m_credentialsFileLoader.Load(); |
There was a problem hiding this comment.
this should just call m_impl->Reload() and all the logic should be moved to the impl
|
|
||
| void CRTProfileCredentialsProvider::RefreshIfExpired() | ||
| { | ||
| ReaderLockGuard guard(m_reloadLock); |
There was a problem hiding this comment.
this should just call m_impl->RefreshIfExpired() and all the logic shold be move to the impl class
|
|
||
| AWSCredentials CRTProfileCredentialsProvider::GetAWSCredentials() | ||
| { | ||
| RefreshIfExpired(); |
There was a problem hiding this comment.
this should just call m_impl->GetAWSCredentials() and all of this logic should be moved there
| m_loadFrequencyMs(refreshRateMs){} | ||
|
|
||
| CRTProfileCredentialsProviderImp(const char* profile, long refreshRateMs) : | ||
| m_profileToUse(profile), |
There was a problem hiding this comment.
nit formatting -- does clang format like this? if so diregard
| #include <aws/core/platform/FileSystem.h> | ||
| #include <aws/core/utils/logging/LogMacros.h> | ||
| #include <aws/core/client/UserAgent.h> | ||
| #include <cstdlib> |
There was a problem hiding this comment.
do we need cstdlib/string.h/climits here? they seem sort of suspect imports
There was a problem hiding this comment.
Agreed, will remove.
pulimsr
force-pushed
the
crt-profile-credentials-provider
branch
from
5b6c82c to
621afbf
Compare
2 participants
Adding CRTProfileCredentialsProvider for CRT-based profile credential
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.