[improvement](cloud) Support configurable S3 credentials providers#62788
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
bc6765a to
f769e01
Compare
|
/review |
f769e01 to
24f241c
Compare
|
run buildall |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
I can't approve this PR as-is because the new credential-provider support is only partially wired through the FE/BE/cloud paths.
- Goal / correctness: Not met yet. The PR aims to support configurable S3 credential providers for cloud storage vaults and S3 storage parameters, but several user-visible paths still drop or mis-handle the new setting.
- Scope / focus: The diff is focused, but the feature is incomplete because not all receivers and alter flows were updated.
- Concurrency: No new concurrency or lock-order issue stood out in the touched code.
- Lifecycle: No new special lifecycle problem was identified in the touched paths.
- Config changes: No new config item was added, but existing
aws_credentials_provider_versionstill supports bothv1andv2, so both paths must behave correctly. - Compatibility: The thrift/proto enum expansion is not safe yet because not every consumer understands the new values.
- Parallel code paths: Not fully updated. The SQL vault alter path, MetaService
ALTER_OBJ_INFOpath, and supportedv1provider-selection path still diverge. - Conditional logic: The new alter logic still ignores
cred_provider_typein one live path. - Test coverage: FE unit coverage was added for role-based mapping, but coverage is still missing for non-role
TS3StorageParam, BE thrift decoding,ALTER STORAGE VAULTprovider-only updates,alter_obj_info, andv1behavior. - Observability: Existing logging looks sufficient for this change.
- Persistence / metadata: Existing transaction/persistence structure looks fine, but the altered provider type is not written in all paths.
- Data writes: No new data-plane atomicity issue was identified beyond the metadata propagation bugs above.
- FE-BE variable propagation: Incomplete; the new field/enums are not handled consistently on the receiving side.
- Performance: No material performance issue found in the changed code.
- User focus: No additional user-provided review focus was supplied.
Please fix the propagation/alter-path gaps and add end-to-end tests for the missing scenarios before merging.
| SIMPLE = 1, // SimpleAWSCredentialsProvider, corresponding to (ak, sk) | ||
| INSTANCE_PROFILE = 2 // InstanceProfileCredentialsProvider | ||
| INSTANCE_PROFILE = 2, // InstanceProfileCredentialsProvider | ||
| ENV = 3, // EnvironmentAWSCredentialsProvider |
There was a problem hiding this comment.
Adding new thrift enum values here also requires updating the BE decoder in be/src/util/s3_util.cpp::cred_provider_type_from_thrift(). That switch still handles only DEFAULT, SIMPLE, and INSTANCE_PROFILE; when FE sends one of these new values through TS3StorageParam (for example from PushStoragePolicyTask), BE falls into the default arm. Because that arm contains __builtin_unreachable(), this is undefined behavior rather than a safe fallback.
| @@ -1639,7 +1642,7 @@ | |||
There was a problem hiding this comment.
ALTER_OBJ_INFO still cannot change cred_provider_type. The early-return above only compares role_arn and external_id, and this assignment reads from the existing record it instead of the request object. So /MetaService/http/alter_obj_info either returns "not changed" or writes the old/default provider type back, which means the new provider types never persist on this live API.
|
|
||
| if (s3_conf.cred_provider_type == CredProviderType::InstanceProfile) { | ||
| if (s3_conf.role_arn.empty()) { | ||
| return std::make_shared<CustomAwsCredentialsProviderChain>(); |
There was a problem hiding this comment.
This wires the new provider-type support only into the v2 branch. get_aws_credentials_provider() still dispatches to _get_aws_credentials_provider_v1() when aws_credentials_provider_version=v1, and that implementation below only honors INSTANCE_PROFILE for role-based configs. A vault persisted with role_arn + ENV/CONTAINER/WEB_IDENTITY/... will therefore stop using STSAssumeRoleCredentialsProvider entirely on a supported v1 deployment.
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
FE Regression Coverage ReportIncrement line coverage |
|
Behavioral Change (Needs Attention) Issues Found
|
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
24f241c to
895964c
Compare
|
run buildall |
FE UT Coverage ReportIncrement line coverage |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
FE Regression Coverage ReportIncrement line coverage |
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
2 similar comments
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
github-actions
Bot
added
the
approved
|
PR approved by at least one committer and no changes requested. |
272a1b6 to
276ba2d
Compare
|
run buildall |
TPC-H: Total hot run time: 29899 ms |
FE UT Coverage ReportIncrement line coverage |
TPC-DS: Total hot run time: 170212 ms |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
FE Regression Coverage ReportIncrement line coverage |
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
github-actions
Bot
added
the
approved
|
PR approved by at least one committer and no changes requested. |
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
FE Regression Coverage ReportIncrement line coverage |
|
skip check_coverage |
|
/review |
There was a problem hiding this comment.
I found one additional blocking issue in the BE storage-policy/resource-push path and left it inline. I did not duplicate the existing review threads; the already-known S3Properties provider propagation gaps and cloud recycler v1 gap remain covered by those threads.
Critical checkpoints:
- Goal/test: The PR aims to propagate additional AWS credentials provider types through FE, thrift/proto, BE, and cloud recycler. The added FE unit test covers serialization for role_arn, but the BE v1 runtime path is still not covered and currently misbehaves for the newly decoded thrift values.
- Scope: The implementation is mostly focused on provider-type propagation, but it introduces parallel FE/BE/cloud mappings that must stay consistent.
- Concurrency/lifecycle: No new shared mutable state or lock-order changes found in the reviewed code. The provider objects follow existing S3 client/accessor lifecycles.
- Config: Existing aws_credentials_provider_version remains relevant; the new values work differently between v1 and v2 paths. The inline issue is specifically a v1 behavior gap.
- Compatibility/protocol: New thrift/proto enum values require all consumers to handle them consistently. BE decoding was added, but the decoded values are not honored by the v1 credential provider flow.
- Parallel paths: FE, BE, cloud meta-service, and cloud recycler all have parallel provider handling. Existing review threads already cover cloud recycler v1 and FE serialization gaps; the new inline comment covers the distinct BE path.
- Tests: I did not run tests in this automated review. Current tests do not exercise BE v1 credential-provider behavior for role_arn plus ENV/CONTAINER/WEB_IDENTITY.
- Observability/data correctness/transactions: No new transaction or storage-version correctness concern found. No additional observability requirement beyond existing S3 credential failure paths.
- User focus: No additional user-provided review focus was specified.
| case TCredProviderType::INSTANCE_PROFILE: | ||
| return CredProviderType::InstanceProfile; | ||
| case TCredProviderType::ENV: | ||
| return CredProviderType::Env; |
There was a problem hiding this comment.
Decoding these new thrift values exposes a BE v1 credential path that still does not honor them for role-based configs. S3Properties.getS3TStorageParam() can now send role_arn + ENV/CONTAINER/WEB_IDENTITY/...; get_s3_client_conf() stores that type, but with aws_credentials_provider_version != v2, S3ClientFactory::_get_aws_credentials_provider_v1() only enters the assume-role branch for CredProviderType::InstanceProfile. For ENV, CONTAINER, or WEB_IDENTITY, it skips the role_arn entirely and falls through to anonymous credentials because AK/SK are empty. This is distinct from the existing recycler-v1 thread: this is the BE storage-policy/resource-push path enabled by the new thrift decoder. Please either reject non-INSTANCE_PROFILE role providers when v1 is active or make the BE v1 path assume the role using the selected base provider.
…62788) ### What problem does this PR solve? Support configuring the AWS S3 credentials provider type for cloud storage vaults and S3 storage parameters. This allows IAM role based access to use providers other than instance profile, such as environment, system properties, web identity, container, default chain, and anonymous credentials. The credential provider type is propagated through FE properties, thrift/proto definitions, meta service storage vault handling, and cloud recycler S3 accessor creation. ``` CREATE storage vault IF NOT EXISTS s3_vault_2 PROPERTIES ( "type"="S3", "s3.endpoint"="xxx", "s3.role_arn" = "xxx", "s3.region" = "us-west-1", "s3.root.path" = "xxx", "s3.bucket" = "xxx", "provider" = "S3", "use_path_style" = "false", "s3.credentials_provider_type" = "WEB_IDENTITY", "s3_validity_check" = "false" ); ``` ``` curl "127.0.0.1:5678/MetaService/http/get_obj_store_info ``` ``` { "id": "3", "name": "s3_vault_2", "obj_info": { "ctime": "1777011933", "mtime": "1777011933", "id": "3", "bucket": "xxx", "prefix": "wyx_test", "endpoint": "xxx", "region": "us-west-1", "provider": "S3", "external_endpoint": "", "sse_enabled": true, "use_path_style": false, "cred_provider_type": "WEB_IDENTITY", "role_arn": "xxx", "external_id": "" } } ```
…pache#62788) Support configuring the AWS S3 credentials provider type for cloud storage vaults and S3 storage parameters. This allows IAM role based access to use providers other than instance profile, such as environment, system properties, web identity, container, default chain, and anonymous credentials. The credential provider type is propagated through FE properties, thrift/proto definitions, meta service storage vault handling, and cloud recycler S3 accessor creation. ``` CREATE storage vault IF NOT EXISTS s3_vault_2 PROPERTIES ( "type"="S3", "s3.endpoint"="xxx", "s3.role_arn" = "xxx", "s3.region" = "us-west-1", "s3.root.path" = "xxx", "s3.bucket" = "xxx", "provider" = "S3", "use_path_style" = "false", "s3.credentials_provider_type" = "WEB_IDENTITY", "s3_validity_check" = "false" ); ``` ``` curl "127.0.0.1:5678/MetaService/http/get_obj_store_info ``` ``` { "id": "3", "name": "s3_vault_2", "obj_info": { "ctime": "1777011933", "mtime": "1777011933", "id": "3", "bucket": "xxx", "prefix": "wyx_test", "endpoint": "xxx", "region": "us-west-1", "provider": "S3", "external_endpoint": "", "sse_enabled": true, "use_path_style": false, "cred_provider_type": "WEB_IDENTITY", "role_arn": "xxx", "external_id": "" } } ```
…pache#62788) ### What problem does this PR solve? Support configuring the AWS S3 credentials provider type for cloud storage vaults and S3 storage parameters. This allows IAM role based access to use providers other than instance profile, such as environment, system properties, web identity, container, default chain, and anonymous credentials. The credential provider type is propagated through FE properties, thrift/proto definitions, meta service storage vault handling, and cloud recycler S3 accessor creation. ``` CREATE storage vault IF NOT EXISTS s3_vault_2 PROPERTIES ( "type"="S3", "s3.endpoint"="xxx", "s3.role_arn" = "xxx", "s3.region" = "us-west-1", "s3.root.path" = "xxx", "s3.bucket" = "xxx", "provider" = "S3", "use_path_style" = "false", "s3.credentials_provider_type" = "WEB_IDENTITY", "s3_validity_check" = "false" ); ``` ``` curl "127.0.0.1:5678/MetaService/http/get_obj_store_info ``` ``` { "id": "3", "name": "s3_vault_2", "obj_info": { "ctime": "1777011933", "mtime": "1777011933", "id": "3", "bucket": "xxx", "prefix": "wyx_test", "endpoint": "xxx", "region": "us-west-1", "provider": "S3", "external_endpoint": "", "sse_enabled": true, "use_path_style": false, "cred_provider_type": "WEB_IDENTITY", "role_arn": "xxx", "external_id": "" } } ```
6 participants
What problem does this PR solve?
Support configuring the AWS S3 credentials provider type for cloud storage vaults and S3 storage parameters. This allows IAM role based access to use providers other than instance profile, such as environment, system
properties, web identity, container, default chain, and anonymous credentials. The credential provider type is propagated through FE properties, thrift/proto definitions, meta service storage vault handling, and cloud
recycler S3 accessor creation.
Issue Number: close #xxx
Related PR: #xxx
Problem Summary:
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)