Remove unnecessary manual retrieval of AWS credentials.

[teo-tsirpanis]

SC-54624

At the start of S3::init_client(), we try to acquire AWS credentials and fail if they are empty or have expired. This does not make much sense to do because the credentials provider is responsible for refreshing expired credentials, and this PR removes this check. After that, the S3::credentials_provider_ field was removed from the class and became a local variable because of a lack of uses.

---

TYPE: NO_HISTORY

[robertbindar]

Looks good to me, please clarify the inline comment before we merge, thanks!

[robertbindar]

Can you please double check that Aws::S3::S3Client doesn't hold a raw ptr or some sort of weak ptr for credentials_provider so we are sure the lifetime of your new local variable is extended properly?

[teo-tsirpanis]

Through several layers of indirection, the credentials provider is stored in a shared pointer.

[ypatia]

Is there a way we can test this change manually?

[teo-tsirpanis]

We can test it in an EC2 instance, and it's also being tested by the S3 end-to-end tests that use assume role with web identity.

[ypatia]

We can test it in an EC2 instance, and it's also being tested by the S3 end-to-end tests that use assume role with web identity.

AFAIU the s3 e2e test exercises the positive path. My concern is if we tested in any way, manual or automatic, the scenarios that would hit those paths that are now removed as redundant:

      if (credentials.IsExpired()) {
        throw S3Exception("AWS credentials are expired.");
      } else if (!s3_params_.no_sign_request_ && credentials.IsEmpty()) {
        throw S3Exception(
            "AWS credentials were not provided. For public S3 data, consider "
            "setting the vfs.s3.no_sign_request config option.");
      }

Other than that the change looks ok to me.

[teo-tsirpanis]

In your snippet I believe the if path will never be hit unless the credentials provider is wrongly implemented.

The else if path was being hit when a credentials source was configured and failed to obtain certificates, and it had the disadvantage of being vague and potentially inaccurate. By removing this, this error

[2024-09-20 18:36:26.940] [Process: 2948] [error] [1726846586023233300-Global] S3: AWS credentials were not provided. For public S3 data, consider setting the vfs.s3.no_sign_request config option.

will be replaced by this error which clearly indicates what went wrong:

[2024-09-20 18:49:28.374] [Process: 17892] [error] [1726847366993116300-Global] S3: Error while listing with prefix 's3://my_bucket/quickstart_dense_array/__schema/' and delimiter '/'[Error Type: 15] [HTTP Response Code: 403] [Exception: AccessDenied] [Headers: 'content-type' = 'application/xml' 'date' = 'Fri, 20 Sep 2024 15:49:27 GMT' 'server' = 'AmazonS3' 'transfer-encoding' = 'chunked' 'x-amz-bucket-region' = 'us-east-1' 'x-amz-id-2' = 'o0qQ5FrF7/AOkqDBeYI+Cuhq+Id4VcZeIU+BPl9nnbfxHuWaBvEt4/vIOdQS2x2Z+ipRYttcWiY=' 'x-amz-request-id' = 'HTMQ602W78SVX8MA'] : Access Denied

You can try it yourself by running export TILEDB_VFS_S3_AWS_ROLE_ARN=arn:aws:iam::000000000000:role/role-name and trying to perform an operation on S3.

[ihnorton]

/backport to release-2.26

[github-actions]

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

[github-actions]

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

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

Applying: Remove unnecessary manual retrieval of AWS credentials.
Applying: Make `S3::credentials_provider_` a local variable.
error: sha1 information is lacking or useless (tiledb/sm/filesystem/s3.cc).
error: could not build fake ancestor
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 0002 Make `S3::credentials_provider_` a local variable.
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

[github-actions]

@ihnorton 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.