Pass AWS client configuration to STSProfileWithWebIdentityCredentialsProvider.
#4641
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…CredentialsProvider`.
KiterLuc
approved these changes
Jan 17, 2024
| @@ -132,11 +132,15 @@ class /* AWS_IDENTITY_MANAGEMENT_API */ | |||
| const Aws::String& externalID, | |||
| Aws::STS::STSClient* client); | |||
|
|
|||
There was a problem hiding this comment.
We could document this function and the one above, not a blocker as this is high priority.
teo-tsirpanis
changed the title
STSProfileWithWebIdentityCredentialsProvider.STSProfileWithWebIdentityCredentialsProvider.
ihnorton
approved these changes
Jan 17, 2024
Shelnutt2
reviewed
Jan 17, 2024
| Aws::Client::ClientConfiguration config; | ||
| config.scheme = Aws::Http::Scheme::HTTPS; | ||
| config.region = tmpRegion; | ||
| AssumeRoleWithWebIdentityRequest request; |
There was a problem hiding this comment.
Is this change based on upstream changes? We seem to be missing the retry strategy now?
Shelnutt2
reviewed
Jan 17, 2024
| config.retryStrategy = | ||
| Aws::MakeShared<Aws::Client::SpecifiedRetryableErrorsRetryStrategy>( | ||
| CLASS_TAG, retryableErrors, 3 /*maxRetries*/); | ||
| auto outcome = client->AssumeRoleWithWebIdentity(request); |
There was a problem hiding this comment.
Why the change from GetAssumeRoleWithWebIdentityCredentials?
There was a problem hiding this comment.
The changes in this method's logic were to replace the use of internal APIs with public ones.
There was a problem hiding this comment.
Is there a requirement to do that now? Seeing how we've already had issues for 2 releases, in my opinion it'd be much better to minimize changes needed to solve the issue. We can then follow up with refactoring and additional testing.
If there is a high degree of confidence and there has been sufficient testing and validation of these changes, then I'm okay to move forward, but I don't like mixing defect fixes and refactoring that is not required for the defect fix.
There was a problem hiding this comment.
These are the changes that got successfully validated yesterday. We have to use the public STSClient class provided by the user because it has been created with the right client configuration.
github-actions bot
pushed a commit
that referenced
this pull request
Jan 17, 2024
…sProvider`. (#4641) This PR applies a similar fix to #4616 but for `STSProfileWithWebIdentityCredentialsProvider`. That class was also adjusted to prevent a memory leak, to use the user-provided `STSClient` if available, and to use public APIs. The issue has been validated to be fixed. --- TYPE: BUG DESC: Fix HTTP requests for AWS assume role with web identity not honoring config options. (cherry picked from commit 4498063)
github-actions bot
pushed a commit
that referenced
this pull request
Jan 17, 2024
…sProvider`. (#4641) This PR applies a similar fix to #4616 but for `STSProfileWithWebIdentityCredentialsProvider`. That class was also adjusted to prevent a memory leak, to use the user-provided `STSClient` if available, and to use public APIs. The issue has been validated to be fixed. --- TYPE: BUG DESC: Fix HTTP requests for AWS assume role with web identity not honoring config options. (cherry picked from commit 4498063)
2 tasks
teo-tsirpanis
added a commit
that referenced
this pull request
Sep 24, 2024
…in. (#5315) [SC-55378](https://app.shortcut.com/tiledb-inc/story/55378/ensure-all-aws-clients-and-credentials-providers-are-being-passed-client-configuration) This PR applies a similar fix to #4641, but for the `DefaultAWSCredentialsProviderChain` which was being implicitly used when we don't pass any credentials provider to `S3Client` (it's now explicitly used). Because `DefaultAWSCredentialsProviderChain` does not accept a `ClientConfiguration`, I copied its sources, as well as the sources of all applicable credentials providers and updated them to accept a `ClientConfiguration`. This is a temporary measure until the changes to the providers are upstreamed to the AWS SDK. You are encouraged to review this PR by looking at each commit individually. --- TYPE: BUG DESC: Fix HTTP requests for AWS default credentials provider chain not honoring config options.
teo-tsirpanis
added a commit
that referenced
this pull request
Sep 24, 2024
…in. (#5315) [SC-55378](https://app.shortcut.com/tiledb-inc/story/55378/ensure-all-aws-clients-and-credentials-providers-are-being-passed-client-configuration) This PR applies a similar fix to #4641, but for the `DefaultAWSCredentialsProviderChain` which was being implicitly used when we don't pass any credentials provider to `S3Client` (it's now explicitly used). Because `DefaultAWSCredentialsProviderChain` does not accept a `ClientConfiguration`, I copied its sources, as well as the sources of all applicable credentials providers and updated them to accept a `ClientConfiguration`. This is a temporary measure until the changes to the providers are upstreamed to the AWS SDK. You are encouraged to review this PR by looking at each commit individually. --- TYPE: BUG DESC: Fix HTTP requests for AWS default credentials provider chain not honoring config options.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR applies a similar fix to #4616 but for
STSProfileWithWebIdentityCredentialsProvider. That class was also adjusted to prevent a memory leak, to use the user-providedSTSClientif available, and to use public APIs.The issue has been validated to be fixed.
TYPE: BUG
DESC: Fix HTTP requests for AWS assume role with web identity not honoring config options.