Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support passing ClientConfiguration to web identity credentials provider. #3116

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

teo-tsirpanis
Copy link
Contributor

Issue #, if available:

Description of changes: This PR updates the constructor of STSAssumeRoleWebIdentityCredentialsProvider to accept an optional ClientConfiguration parameter.

Also the provider no longer tries to determine itself the region to use and instead always defers to the region specified in the config. This shouldn't have any practical effect since the default constructor of ClientConfiguration has a similar (and slightly more extensive) logic either way.

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.

Copy link
Contributor

@sbera87 sbera87 left a comment

Choose a reason for hiding this comment

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

looks ok to me otherwise. Now region needs to be passed through the config, there is no fallback

@teo-tsirpanis
Copy link
Contributor Author

Now region needs to be passed through the config, there is no fallback

@sbera87 what do you mean? I don't understand.

@@ -25,7 +25,7 @@ namespace Aws
class AWS_CORE_API STSAssumeRoleWebIdentityCredentialsProvider : public AWSCredentialsProvider
{
public:
STSAssumeRoleWebIdentityCredentialsProvider();
STSAssumeRoleWebIdentityCredentialsProvider(Aws::Client::ClientConfiguration config = {});
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm really sorry for a a big delay in the handling of this PR.
I've been running a CI check and just noticed that

it is actually changing the default constructor to make an IMDS call (to resolve a region, not always though, but still...), this is a significant change of existing behavior.

could you please change it to

STSAssumeRoleWebIdentityCredentialsProvider(Aws::Client::ClientConfiguration config = {Aws::Client::ClientConfigurationInitValues{true}});

so IMDS won't be auto-enabled?

We should be able to merge the PR shortly after this.
Thank you!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@SergeyRyabinin
Copy link
Contributor

looks ok to me otherwise. Now region needs to be passed through the config, there is no fallback

The client config will fallback to the us-east-1 if not set:

region = Aws::String(Aws::Region::US_EAST_1);

@SergeyRyabinin
Copy link
Contributor

Sorry, it has failed the build.

In file included from /tmp/aws-sdk-cpp/src/aws-cpp-sdk-core/source/auth/AWSCredentialsProviderChain.cpp:7:0,
                 from src/aws-cpp-sdk-core/ub_core.cpp:3:
/tmp/aws-sdk-cpp/src/aws-cpp-sdk-core/include/aws/core/auth/STSCredentialsProvider.h:28:147: error: no matching function for call to 'Aws::Client::ClientConfigurationInitValues::ClientConfigurationInitValues(<brace-enclosed initializer list>)'
             STSAssumeRoleWebIdentityCredentialsProvider(Aws::Client::ClientConfiguration config = {Aws::Client::ClientConfigurationInitValues{true}});
                                                                                                                                                   ^
In file included from /tmp/aws-sdk-cpp/src/aws-cpp-sdk-core/include/aws/core/internal/AWSHttpResourceClient.h:11:0,
                 from /tmp/aws-sdk-cpp/src/aws-cpp-sdk-core/include/aws/core/auth/AWSCredentialsProvider.h:17,
                 from /tmp/aws-sdk-cpp/src/aws-cpp-sdk-core/source/auth/AWSCredentialsProvider.cpp:7,
                 from src/aws-cpp-sdk-core/ub_core.cpp:2:
/tmp/aws-sdk-cpp/src/aws-cpp-sdk-core/include/aws/core/client/ClientConfiguration.h:67:16: note: candidate: constexpr Aws::Client::ClientConfigurationInitValues::ClientConfigurationInitValues()
         struct ClientConfigurationInitValues {
                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/tmp/aws-sdk-cpp/src/aws-cpp-sdk-core/include/aws/core/client/ClientConfiguration.h:67:16: note:   candidate expects 0 arguments, 1 provided
/tmp/aws-sdk-cpp/src/aws-cpp-sdk-core/include/aws/core/client/ClientConfiguration.h:67:16: note: candidate: constexpr Aws::Client::ClientConfigurationInitValues::ClientConfigurationInitValues(const Aws::Client::ClientConfigurationInitValues&)
/tmp/aws-sdk-cpp/src/aws-cpp-sdk-core/include/aws/core/client/ClientConfiguration.h:67:16: note:   no known conversion for argument 1 from 'bool' to 'const Aws::Client::ClientConfigurationInitValues&'
/tmp/aws-sdk-cpp/src/aws-cpp-sdk-core/include/aws/core/client/ClientConfiguration.h:67:16: note: candidate: constexpr Aws::Client::ClientConfigurationInitValues::ClientConfigurationInitValues(Aws::Client::ClientConfigurationInitValues&&)
/tmp/aws-sdk-cpp/src/aws-cpp-sdk-core/include/aws/core/client/ClientConfiguration.h:67:16: note:   no known conversion for argument 1 from 'bool' to 'Aws::Client::ClientConfigurationInitValues&&'
In file included from /tmp/aws-sdk-cpp/src/aws-cpp-sdk-core/source/auth/AWSCredentialsProviderChain.cpp:7:0,
                 from src/aws-cpp-sdk-core/ub_core.cpp:3:
/tmp/aws-sdk-cpp/src/aws-cpp-sdk-core/include/aws/core/auth/STSCredentialsProvider.h:28:148: error: could not convert '{<expression error>}' from '<brace-enclosed initializer list>' to 'Aws::Client::ClientConfiguration'
             STSAssumeRoleWebIdentityCredentialsProvider(Aws::Client::ClientConfiguration config = {Aws::Client::ClientConfigurationInitValues{true}});

Let me handle the update for this PR.

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.

3 participants