Repository navigation
[fix](be): fix BE access to aws under eks pod identity - #66555
Owen-CH-Leung wants to merge 5 commits into
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Owen-CH-Leung
changed the title
Owen-CH-Leung
changed the title
|
/review |
|
@sollhui PTAL |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Requesting changes for two blocking issues: explicit CONTAINER routing still cannot use standard EKS Pod Identity, and supported stale/prebuilt third-party paths can compile this patch against AWS SDK 1.11.219.
Critical checkpoint conclusions
- Goal, correctness, scope, and parallel paths: The default custom chain now forwards the rotating token-file path, but the public explicit
CONTAINERroutes in BE S3, Cloud recycler, and MSK remain relative-URI-only, including STS base-provider paths. This is the first inline blocker. - Concurrency, lifecycle, error handling, and special conditions: Token-file refresh locking, expiry-driven reload, provider/client ownership, mock-server request completion, environment/file cleanup, relative/full URI precedence, and empty-provider behavior were checked. No additional issue survived; the explicit-route failure is covered by the first inline comment.
- Configuration, dynamic behavior, compatibility, and FE/BE variables: No new runtime variable or protocol/storage-format change is introduced. Existing FE/property/thrift/PB selection makes explicit
CONTAINERreachable. The AWS SDK pin is a build-time compatibility change, but stale installs and the macOS prebuilt path are not invalidated; this is the second inline blocker. - Tests and results: The new unit test exercises two token-file reads in the default chain, but it does not cover explicit
CONTAINERor its assume-role consumers, and no test protects dependency-version invalidation. Per the automated-review instructions, I did not run builds or tests. - Observability and performance: Authorization-token secrecy is preserved, provider-selection logging remains adequate, and refresh stays expiry-driven; no additional hot-path or observability issue was found.
- Persistence, transactions, and data writes: Not applicable to this credential-provider and third-party dependency change.
- User focus:
review_focus.txtcontained no additional focus points; the full PR was reviewed. - Completion status: The review converged after round 2. All independent reviewers returned
NO_NEW_VALUABLE_FINDINGSagainst the unchanged two-finding set, every candidate was resolved, and there were no pre-existing live inline threads to duplicate.
Separately, the current PR title checker is failing because the title uses fix(be): rather than the repository's required [fix](be) form.
| AddProvider(Aws::MakeShared<TaskRoleCredentialsProvider>( | ||
| DefaultCredentialsProviderChainTag, absoluteUri.c_str(), token.c_str())); | ||
| const auto tokenPath = Aws::Environment::GetEnv(AWS_EKS_CONTAINER_AUTHORIZATION_TOKEN_FILE); | ||
| AddProvider(Aws::MakeShared<GeneralHTTPCredentialsProvider>( |
There was a problem hiding this comment.
[P1] Apply the EKS provider wiring to explicit CONTAINER routes
This fixes CustomAwsCredentialsProviderChain, but CONTAINER is also a supported public provider mode. The BE S3 factory, Cloud recycler, and MSK auth paths still construct TaskRoleCredentialsProvider from only AWS_CONTAINER_CREDENTIALS_RELATIVE_URI. Standard EKS injects only AWS_CONTAINER_CREDENTIALS_FULL_URI plus AWS_CONTAINER_AUTHORIZATION_TOKEN_FILE, so those branches pass an empty URI, create no HTTP client, and return no credentials (including when used as an STS base provider). Please centralize this construction so every explicit container-provider path forwards the relative URI, full URI, inline token, and rotating token-file path, and add coverage for explicit CONTAINER mode rather than only the default custom chain.
There was a problem hiding this comment.
Kafka MSK also need support
Owen-CH-Leung
changed the title
… the credentialProvider path to the explicit container mode and MSK
…6851) ### What problem does this PR solve? Related PR: #66555 ### Problem Summary: Bumps the pinned `aws-sdk-cpp` from **1.11.219** to **1.11.221** in `thirdparty/vars.sh`. No source changes. **1.11.221** is the first release that ships `GeneralHTTPCredentialsProvider.h`, which is the prerequisite for solving #66554. This PR is deliberately pin-only so that the pre-built thirdparty artifacts and the docker image are re-published against **1.11.221** before any code depends on the new header. #66555 is the follow-up PR that uses it.
|
Apology for the spam. Will create another PR |
What problem does this PR solve?
Issue Number: close #66554
Problem Summary
Fixes the issue above by forwarding
AWS_CONTAINER_AUTHORIZATION_TOKEN_FILEalongside the inline token. Given a file path, the provider will re-read it before every fetch, so credentials survive token rotation. ECS, which sets only the inline variableAWS_CONTAINER_AUTHORIZATION_TOKEN, is unaffected.Why the aws-sdp-cpp bump
The
GeneralHTTPCredentialsProviderdoes not exist before 1.11.221. The only container provider available at the currently pinned version isTaskRoleCredentialsProvider, which takes the token as a string and has no notion of a token file. 1.11.221 is the first version whoseGeneralHTTPCredentialsProviderre-reads the token file.Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)