github-actions[bot] commented on code in PR #64766:
URL: https://github.com/apache/doris/pull/64766#discussion_r3464394586
##########
cloud/src/recycler/s3_accessor.cpp:
##########
@@ -237,13 +237,14 @@ std::optional<S3Conf> S3Conf::from_obj_store_info(const
ObjectStoreInfoPB& obj_i
}
}
+ if (obj_info.has_cred_provider_type()) {
Review Comment:
Now that role-less vaults can carry `cred_provider_type`, this value also
reaches the recycler. However, if `aws_credentials_provider_version` is set to
the still-accepted `v1`, `_get_aws_credentials_provider_v1` only honors
`InstanceProfile`; provider-only modes such as `CONTAINER`, `ENV`, or
`ANONYMOUS` fall through to `DefaultAWSCredentialsProviderChain` instead of the
requested provider. That can make a newly accepted provider-only vault fail to
access its bucket, or use a different credential source than the vault
declares. Please either make the v1 path honor `s3_conf.cred_provider_type` for
role-less providers, or reject/guard provider-only vaults when the selected
credential-provider implementation cannot support them.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/property/storage/S3Properties.java:
##########
@@ -696,12 +701,21 @@ public static Cloud.ObjectStoreInfoPB.Builder
getObjStoreInfoPB(Map<String, Stri
builder.setUsePathStyle(value.equalsIgnoreCase("true"));
}
Review Comment:
This makes `credentials_provider_type` available in the `ObjectStoreInfoPB`
built for both CREATE and ALTER, but the Cloud ALTER path still ignores a
provider-type-only update. `ALTER STORAGE VAULT ...
PROPERTIES("s3.credentials_provider_type" = "container")` is allowed by
`S3StorageVault.ALLOW_ALTER_PROPERTIES` and reaches
`alter_s3_storage_vault_by_id` with `has_cred_provider_type()` set and no role
ARN. That function only updates `cred_provider_type` inside `if
(obj_info.has_role_arn())`, so the request can return OK while leaving the
stored vault on its previous AK/SK credentials. Please either handle
`has_cred_provider_type()` as a standalone credential update in the ALTER path,
or reject provider-type-only ALTER requests until that state is supported.
##########
cloud/src/meta-service/meta_service_resource.cpp:
##########
@@ -1306,13 +1316,15 @@ static ObjectStoreInfoPB
object_info_pb_factory(ObjectStorageDesc& obj_desc,
last_item.set_user_id(obj.user_id());
}
- if (!obj.has_role_arn()) {
+ if (!use_credential_provider(obj)) {
last_item.set_ak(std::move(cipher_ak_sk_pair.first));
last_item.set_sk(std::move(cipher_ak_sk_pair.second));
last_item.mutable_encryption_info()->CopyFrom(encryption_info);
} else {
- last_item.set_role_arn(role_arn);
- last_item.set_external_id(external_id);
+ if (has_non_empty_role_arn(obj)) {
+ last_item.set_role_arn(role_arn);
+ last_item.set_external_id(external_id);
Review Comment:
This introduces a stored S3 vault state where AK/SK are empty and
`cred_provider_type` carries the credentials mode. The matching `SHOW CREATE
STORAGE VAULT` path still always emits `s3.access_key`/`s3.secret_key` from the
proto and never emits `s3.credentials_provider_type`, role ARN, or external ID,
so a provider-only vault will show a non-equivalent CREATE statement with blank
static credentials and no provider mode. Please update
`ShowCreateStorageVaultCommand.getObjectCreateStmt` to emit the stored
credential fields according to the proto state and avoid printing blank AK/SK
for provider-based vaults.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
