Skip to content

UID2-4739: use DefaultCredentialsProvider for S3 clients in CloudStorageS3#613

Open
sophia-chen-ttd wants to merge 2 commits into
mainfrom
sch-UID2-4739-default-credentials-provider
Open

UID2-4739: use DefaultCredentialsProvider for S3 clients in CloudStorageS3#613
sophia-chen-ttd wants to merge 2 commits into
mainfrom
sch-UID2-4739-default-credentials-provider

Conversation

@sophia-chen-ttd
Copy link
Copy Markdown
Contributor

@sophia-chen-ttd sophia-chen-ttd commented Jun 3, 2026

Summary

  • CloudStorageS3 explicitly constructed WebIdentityTokenFileCredentialsProvider, reading AWS_ROLE_ARN and AWS_WEB_IDENTITY_TOKEN_FILE env vars directly — tying it hard to OIDC/IRSA
  • Replaced with DefaultCredentialsProvider.create(), which automatically resolves credentials via the standard AWS SDK credential chain (IRSA, EKS Pod Identity, instance profiles, env vars, etc.)
  • The AWS_WEB_IDENTITY_TOKEN_FILE env var read is removed as it was only used to construct the old provider

Why

WebIdentityTokenFileCredentialsProvider only works with IRSA (OIDC-based service account annotation). With EKS Pod Identity (the newer, preferred mechanism), credentials are injected differently and this explicit provider will fail to pick them up. DefaultCredentialsProvider handles both IRSA and EKS Pod Identity transparently, and will continue to work with any future AWS credential mechanism without code changes.

⚠️ Historical context — read before merging

The original code contained this comment (now removed):

In theory new InstanceProfileCredentialsProvider() or even omitting credentials provider should work, but for some unknown reason it doesn't. The credential it provides look realistic, but are not valid. After a lot of experimentation and help of Abu Abraham and Isaac Wilson the only working solution we've found was to explicitly extract env vars populated by the service account from the role and to manually set it on the credentials provider.

This failure was most likely caused by an old AWS SDK v2 version that did not fully auto-detect IRSA credentials via the default chain. Early SDK v2 releases had gaps in WebIdentityTokenCredentialsProvider auto-detection, requiring explicit construction from env vars. This was a known SDK issue fixed in later versions.

Modern AWS SDK v2 includes WebIdentityTokenCredentialsProvider in the default chain and detects AWS_WEB_IDENTITY_TOKEN_FILE automatically. DefaultCredentialsProvider also handles EKS Pod Identity via the container credentials endpoint (AWS_CONTAINER_CREDENTIALS_FULL_URI).

This change must be explicitly tested in a non-prod EKS environment before merging — the historical comment is a warning that this code path failed silently once. Do not assume the theoretical fix works without verification.

Test plan

  • Unit tests pass (mvn test)
  • Deploy to a non-prod environment and confirm S3 operations succeed — this is the critical test given the historical failure above
  • Confirm existing IRSA-based environments continue to work (DefaultCredentialsProvider still picks up IRSA env vars via its WebIdentityTokenCredentialsProvider step in the chain)
  • After EKS Pod Identity migration: confirm S3 operations succeed with Pod Identity credentials

🤖 Generated with Claude Code

…ageS3

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor Author

@sophia-chen-ttd sophia-chen-ttd left a comment

Choose a reason for hiding this comment

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

The swap to DefaultCredentialsProvider is the correct direction: it supports IRSA (WebIdentityTokenFile), EKS Pod Identity (container credentials endpoint), instance profile, and all other standard chain entries transparently. The old comment's 'unknown reason' failure was almost certainly an early AWS SDK v2 version that predated reliable IRSA support in the default chain — the PR's explanation is credible. The change is minimal and low-risk, but removal of the historical warning, lack of credential test coverage, and resource-lifecycle subtleties are worth calling out.

Comment thread src/main/java/com/uid2/shared/cloud/CloudStorageS3.java
Comment thread src/main/java/com/uid2/shared/cloud/CloudStorageS3.java
Comment thread src/main/java/com/uid2/shared/cloud/CloudStorageS3.java
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.

1 participant