Pass AWS client configuration to the default credentials provider chain. #5315

teo-tsirpanis · 2024-09-18T14:04:07Z

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.

…viders.

teo-tsirpanis · 2024-09-18T14:07:23Z

tiledb/sm/filesystem/s3/STSCredentialsProvider.cc

+    Aws::Vector<Aws::String> retryableErrors;
+    retryableErrors.push_back("IDPCommunicationError");
+    retryableErrors.push_back("InvalidIdentityToken");


The default retry strategies for the credentials providers retry some errors that our S3-specific strategy does not. How about I use a dedicated retry strategy (in a copy of the client config) for the credentials providers?

I thatvis a good idea to use a separate retry strategy.

tiledb/sm/filesystem/s3/AWSCredentialsProviderChain.cc

Shelnutt2 · 2024-09-19T05:29:12Z

tiledb/sm/filesystem/s3/STSCredentialsProvider.cc

+        Aws::Client::ClientConfiguration config)
+    : m_initialized(false) {
+  // check environment variables
+  Aws::String tmpRegion = Aws::Environment::GetEnv("AWS_DEFAULT_REGION");


As a follow up, can we improve the region support here? It's be nice to allow using the user set config of vfs.s3.region. It's a little unexpected for users using any of the credentials providers that the region for sta is only read from the env or profile.

We encountered this oddity with a customer running in AWS China and struggling to understand why STS was trying to hit us-east-1 instead of the cn-notth-1 that was needed and configured via the TileDB config for region.

Fixed. If the config has a region specified, it will not be overridden. I will make sure to include this to my upstream fix.

If the config has a region specified, it will not be overridden

After further thought this won't work because the region of a config always gets initialized with a value. I think the best thing to do here is always get the region in the client provider from the config, and in a subsequent PR update the default value of vfs.s3.region from us-east-1 to letting the AWS SDK choose, which would align the SDK's and TileDB's view of the default AWS region.

Updated to always get the region from the client config. Will later open a story to consider changing the default value of vfs.s3.region.

In thinking on this, won't this change now mean that if a user set their profile or env variables they are ignored? Can we make the sequence something like what the SDK does itself?

TileDB Config

ENV var

Profile

hard coded default?

In thinking on this, won't this change now mean that if a user set their profile or env variables they are ignored?

Yes, but this change in the credentials provider is more likely to be accepted upstream. This in combination with changing the default value of vfs.s3.region (SC-55556) will achieve the sequence you described.

…der.

teo-tsirpanis · 2024-09-19T12:30:23Z

Validated successfully after running the following code:

import tiledb

ctx = tiledb.Ctx({"ssl.ca_file": "/etc/dummy-file", "vfs.s3.logging_level": "trace"})
vfs = tiledb.VFS(ctx=ctx)
vfs.ls("s3://my-bucket")

and finding these lines in the AWS logs:

[INFO] 2024-09-19 12:25:06.205 STSAssumeRoleWithWebIdentityCredentialsProvider [139956143503168] Credentials have expired, attempting to renew from STS.
[TRACE] 2024-09-19 12:25:06.205 STSResourceClient [139956143503168] Retrieving credentials from https://sts.us-east-1.amazonaws.com
[…]
[DEBUG] 2024-09-19 12:25:06.209 CURL [139956143503168] (Text) Connected to sts.us-east-1.amazonaws.com (72.21.206.96) port 443

[DEBUG] 2024-09-19 12:25:06.209 CURL [139956143503168] (Text) ALPN: curl offers http/1.1

[DEBUG] 2024-09-19 12:25:06.209 CURL [139956143503168] (SSLDataOut) 5 bytes
[DEBUG] 2024-09-19 12:25:06.209 CURL [139956143503168] (Text) TLSv1.3 (OUT), TLS handshake, Client hello (1):

[DEBUG] 2024-09-19 12:25:06.209 CURL [139956143503168] (SSLDataOut) 512 bytes
[DEBUG] 2024-09-19 12:25:06.209 CURL [139956143503168] (Text) error setting certificate file: /etc/dummy-file

[DEBUG] 2024-09-19 12:25:06.209 CURL [139956143503168] (Text) error setting certificate file: /etc/dummy-file

This is ready for review.

…entials provider endpoint.

teo-tsirpanis · 2024-09-24T13:09:43Z

/backport to release-2.26

github-actions · 2024-09-24T13:09:58Z

Started backporting to release-2.26: https://github.com/TileDB-Inc/TileDB/actions/runs/11014481076

github-actions · 2024-09-24T13:10:07Z

@teo-tsirpanis backporting to release-2.26 failed, the patch most likely resulted in conflicts:

$ git am --3way --ignore-whitespace --keep-non-patch changes.patch

Applying: Copy AWS SDK credentials provider sources.
Applying: Integrate the source files into the codebase.
Applying: Support passing `ClientConfiguration` to the vendored credentials providers.
Using index info to reconstruct a base tree...
M	tiledb/sm/filesystem/s3.cc
M	tiledb/sm/filesystem/s3.h
Falling back to patching base and 3-way merge...
Auto-merging tiledb/sm/filesystem/s3.h
CONFLICT (content): Merge conflict in tiledb/sm/filesystem/s3.h
Auto-merging tiledb/sm/filesystem/s3.cc
CONFLICT (content): Merge conflict in tiledb/sm/filesystem/s3.cc
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config advice.mergeConflict false"
Patch failed at 0003 Support passing `ClientConfiguration` to the vendored credentials providers.
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

github-actions · 2024-09-24T13:10:08Z

@teo-tsirpanis an error occurred while backporting to release-2.26, please check the run log for details!

Error: git am failed, most likely due to a merge conflict.

…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.

…credentials provider chain. (#5315) (#5318) Backport of #5315 to release-2.26 --- TYPE: BUG DESC: Fix HTTP requests for AWS default credentials provider chain not honoring config options.

teo-tsirpanis added 3 commits September 18, 2024 12:33

Copy AWS SDK credentials provider sources.

1692370

Integrate the source files into the codebase.

1c4ddfb

Support passing ClientConfiguration to the vendored credentials pro…

03b5e9f

…viders.

teo-tsirpanis commented Sep 18, 2024

View reviewed changes

Fix errors.

73ae35f

teo-tsirpanis force-pushed the teo/default-aws-credprov-chain-clientconfig branch from bcce064 to 73ae35f Compare September 18, 2024 14:59

teo-tsirpanis commented Sep 18, 2024

View reviewed changes

tiledb/sm/filesystem/s3/AWSCredentialsProviderChain.cc Show resolved Hide resolved

Shelnutt2 reviewed Sep 19, 2024

View reviewed changes

teo-tsirpanis added 2 commits September 19, 2024 13:02

Do not change the region if already set in the STS web identity provi…

e7281d5

…der.

Use a different retry strategy for the credentials providers.

4279bb9

This comment was marked as outdated.

Sign in to view

teo-tsirpanis marked this pull request as ready for review September 19, 2024 12:30

teo-tsirpanis requested a review from ihnorton September 19, 2024 12:30

teo-tsirpanis added 2 commits September 20, 2024 02:32

Always use the region specified in the client config for the STS cred…

6d2c907

…entials provider endpoint.

Merge branch 'dev' into teo/default-aws-credprov-chain-clientconfig

6494d89

ihnorton approved these changes Sep 24, 2024

View reviewed changes

teo-tsirpanis merged commit 1c25f09 into dev Sep 24, 2024
63 checks passed

teo-tsirpanis deleted the teo/default-aws-credprov-chain-clientconfig branch September 24, 2024 10:16

ihnorton added the backports-2.26.2 label Sep 24, 2024

teo-tsirpanis mentioned this pull request Sep 24, 2024

[Backport release-2.26] Pass AWS client configuration to the default credentials provider chain. (#5315) #5318

Merged

jdblischak mentioned this pull request Sep 27, 2024

The nightly build job failed on Friday (2024-09-27) TileDB-Inc/TileDB-VCF#777

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pass AWS client configuration to the default credentials provider chain. #5315

Pass AWS client configuration to the default credentials provider chain. #5315

teo-tsirpanis commented Sep 18, 2024

teo-tsirpanis Sep 18, 2024

Shelnutt2 Sep 19, 2024

teo-tsirpanis Sep 19, 2024

Shelnutt2 Sep 19, 2024

teo-tsirpanis Sep 19, 2024

teo-tsirpanis Sep 19, 2024

teo-tsirpanis Sep 19, 2024

Shelnutt2 Sep 20, 2024

teo-tsirpanis Sep 20, 2024

This comment was marked as outdated.

teo-tsirpanis commented Sep 19, 2024

teo-tsirpanis commented Sep 24, 2024

github-actions bot commented Sep 24, 2024

github-actions bot commented Sep 24, 2024

github-actions bot commented Sep 24, 2024

Pass AWS client configuration to the default credentials provider chain. #5315

Pass AWS client configuration to the default credentials provider chain. #5315

Conversation

teo-tsirpanis commented Sep 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as outdated.

teo-tsirpanis commented Sep 19, 2024

teo-tsirpanis commented Sep 24, 2024

github-actions bot commented Sep 24, 2024

github-actions bot commented Sep 24, 2024

github-actions bot commented Sep 24, 2024