github-actions[bot] commented on code in PR #65433:
URL: https://github.com/apache/doris/pull/65433#discussion_r3636131402
##########
be/src/util/s3_util.h:
##########
@@ -101,6 +101,7 @@ struct S3ClientConf {
hash_code ^= request_timeout_ms;
hash_code ^= connect_timeout_ms;
hash_code ^= use_virtual_addressing;
+ hash_code ^= need_override_endpoint;
Review Comment:
[P1] Do not use this XOR as client identity
This makes two supported configurations collide deterministically:
`(use_virtual_addressing=false, need_override_endpoint=true)` and `(true,
false)` contribute the same value when every other field matches. Both fields
are independently populated from properties, while the factory cache and
`ObjClientHolder::reset()` trust this hash without comparing the configuration.
Whichever client is created first can therefore be reused for the other
configuration with the wrong endpoint override and addressing mode. Please use
order-sensitive hash combining and verify equality on lookup/reset (or key by
the full configuration), and add a test for the opposite boolean pairs.
##########
be/src/cloud/cloud_storage_engine.cpp:
##########
@@ -405,6 +417,10 @@ Status
CloudStorageEngine::start_bg_threads(std::shared_ptr<WorkloadGroup> wg_sp
return Status::OK();
}
+bool CloudStorageEngine::_should_check_storage_vault() {
+ return !_storage_vault_synced.exchange(true) &&
config::enable_check_storage_vault;
Review Comment:
[P1] Probe keyless vaults added after the first sync
This process-wide bit is consumed by the first non-empty vault sync, even if
that list contains only HDFS or static-credential vaults. FE skips its
connectivity ping for Workload Identity and CREATE/ALTER merely triggers
another `sync_storage_vault()`, so a later Workload Identity vault (or an
HMAC-to-WI transition) reaches both visitors with `check_fs == false` and is
published without ever calling `exists()` under the real identity. Invalid IAM
therefore appears to succeed and fails only on first data I/O. Please track
validation per new/changed keyless vault, or otherwise force the probe for
these post-startup updates, and cover both ADD and ALTER timelines.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/property/storage/S3Properties.java:
##########
@@ -702,7 +729,21 @@ public static Cloud.ObjectStoreInfoPB.Builder
getObjStoreInfoPB(Map<String, Stri
}
if (hasCredentialsProviderType(properties)) {
- builder.setCredProviderType(getCredProviderTypePB(properties));
+ if (isGcpWorkloadIdentityCredentialsProvider(properties)) {
+ Preconditions.checkArgument(!builder.hasProvider() ||
builder.getProvider() == Provider.GCP,
+ "%s=%s is only supported with provider GCP",
+ CREDENTIALS_PROVIDER_TYPE,
GCP_WORKLOAD_IDENTITY_CREDENTIALS_PROVIDER);
+ Preconditions.checkArgument(!builder.hasAk() &&
!builder.hasSk(),
+ "%s=%s cannot be used together with %s/%s",
+ CREDENTIALS_PROVIDER_TYPE,
GCP_WORKLOAD_IDENTITY_CREDENTIALS_PROVIDER,
+ ACCESS_KEY, SECRET_KEY);
+
Preconditions.checkArgument(Strings.isNullOrEmpty(properties.get(S3Properties.ROLE_ARN)),
+ "%s=%s cannot be used together with %s",
+ CREDENTIALS_PROVIDER_TYPE,
GCP_WORKLOAD_IDENTITY_CREDENTIALS_PROVIDER, ROLE_ARN);
+
builder.setCredProviderType(CredProviderTypePB.GCP_WORKLOAD_IDENTITY);
Review Comment:
[P2] Preserve Workload Identity in SHOW CREATE
This enum is the only persisted selector for keyless authentication, but
`ShowCreateStorageVaultCommand.getObjectCreateStmt()` never emits
`s3.credentials_provider_type`; it emits the masked `s3.access_key` and
`s3.secret_key` properties instead, even though this mode stores no static
keys. The output therefore describes the wrong authentication mechanism and
gives no indication that this is a keyless vault. Please emit
`gcp_workload_identity` for this enum and omit the static-key properties, with
a SHOW CREATE test for a keyless vault.
##########
cloud/src/meta-service/meta_service_resource.cpp:
##########
@@ -1146,6 +1207,38 @@ static int alter_s3_storage_vault_by_id(InstanceInfoPB&
instance, std::unique_pt
return -1;
}
+ if (is_gcp_workload_identity_cred_provider(obj_info)) {
+ if (obj_info.has_ak() || obj_info.has_role_arn()) {
+ code = MetaServiceCode::INVALID_ARGUMENT;
+ msg = "invalid argument, GCP workload identity can not be set
together with ak/sk or "
+ "role_arn";
+ LOG(WARNING) << msg;
+ return -1;
+ }
+ if (!new_vault.obj_info().has_provider() ||
Review Comment:
[P1] Abort the transaction when this ALTER is rejected
A request can combine `VAULT_NAME` with the new credential-provider
property. The helper applies the rename to `instance.storage_vault_names`
before reaching this validation; when an existing non-GCP vault is rejected
here it returns `-1`, but the `ALTER_S3_VAULT` caller ignores that return and
still commits the mutated `InstanceInfoPB`. The vault PB was never put, so the
RPC reports an error while its name map (and possibly default-vault name)
changes underneath it. Please propagate the helper failure before the shared
commit tail, and preferably defer the name mutation until all validation
succeeds; add a negative rename-plus-WI test.
--
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]
