github-actions[bot] commented on code in PR #65433:
URL: https://github.com/apache/doris/pull/65433#discussion_r3613741915
##########
gensrc/proto/cloud.proto:
##########
@@ -274,6 +274,9 @@ enum CredProviderTypePB {
WEB_IDENTITY = 6; // STSAssumeRoleWebIdentityCredentialsProvider
CONTAINER = 7; // TaskRoleCredentialsProvider
ANONYMOUS = 8; // AnonymousAWSCredentialsProvider
+ // GKE Workload Identity. Requests to the GCS XML API are authenticated
+ // with a short-lived OAuth2 token from the GKE metadata server.
+ GCP_WORKLOAD_IDENTITY = 9;
Review Comment:
[P1] Gate this persisted enum for mixed-version consumers
With the previous proto2 schema, value 9 is an unknown closed-enum value, so
old BE/recycler binaries see this field as unset. Since a Workload Identity
vault deliberately has no AK/SK/role, those consumers build the default AWS
client and never attach OAuth, breaking I/O and recycling during a rolling
upgrade or rollback. Rollback can also preserve tag 17 in the unknown-field set
across an old Meta Service's nominal HMAC transition, allowing a later new
consumer to recover value 9 together with keys. Please gate creation/ALTER
until all consumers advertise support and define/test downgrade handling for
the raw unknown field.
##########
cloud/src/meta-service/meta_service_resource.cpp:
##########
@@ -1146,6 +1162,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)) {
Review Comment:
[P1] Validate the merged credential state for role ALTERs
This validates only an incoming Workload Identity fragment. If the stored
vault already uses Workload Identity and the user alters only `s3.role_arn`, FE
emits the role plus `INSTANCE_PROFILE`, this block is skipped, and the later
role branch persists those AWS credentials while retaining provider GCP and the
GCS endpoint. BE/recycler then construct an AWS STS provider for GCS and I/O
fails; ADD already rejects the same combination. Please validate the complete
merged `ObjectStoreInfoPB` after every credential transition (or reject roles
for provider GCP) and add the outgoing-role negative test.
##########
be/src/io/fs/s3_obj_storage_client.cpp:
##########
@@ -121,6 +121,7 @@ ObjectStorageUploadResponse
S3ObjStorageClient::create_multipart_upload(
CreateMultipartUploadRequest request;
request.WithBucket(opts.bucket).WithKey(opts.key);
request.SetContentType("application/octet-stream");
+ _apply_bearer_token(request);
Review Comment:
[P1] Attach the bearer token after rate limiting
This captures the bearer token before `s3_put_rate_limit()` may sleep. The
limiter supports rates as low as 1 TPS and an unbounded concurrent deficit, so
a cached token with only a few minutes remaining can expire in that queue
before the SDK sends the request. The same ordering is used by every BE and
recycler request, including pagination and delete batches. Please
refresh/replace the header inside each rate-limited callback immediately before
the SDK call, and cover a queued request whose clock advances past token expiry.
##########
cloud/src/meta-service/meta_service_resource.cpp:
##########
@@ -1496,7 +1544,20 @@ void
MetaServiceImpl::alter_storage_vault(google::protobuf::RpcController* contr
}
if (use_credential_provider(obj)) {
- if (!obj.has_provider() || obj.provider() !=
ObjectStoreInfoPB::S3) {
+ if (is_gcp_workload_identity_cred_provider(obj)) {
Review Comment:
[P1] Support Workload Identity in built-in vault creation
This new keyless special case is confined to `ADD_S3_VAULT`. The parallel
initial/built-in paths (`CreateInstanceRequest.vault` and `ADD_BUILT_IN_VAULT`)
still call `add_vault_into_instance()` and then
`create_object_info_with_encrypt()`, whose non-role branch requires nonempty
AK/SK. A canonical GCP Workload Identity vault intentionally has no AK/SK/role,
so cloud control-plane provisioning deterministically returns
`INVALID_ARGUMENT` before persistence even though it carries the same
`StorageVaultPB` and enum. Please share the Workload Identity
validation/persistence logic with those paths and add coverage for both
built-in entry points.
##########
cloud/src/meta-service/meta_service_resource.cpp:
##########
@@ -768,6 +769,21 @@ static bool use_credential_provider(const
ObjectStoreInfoPB& obj) {
return obj.has_cred_provider_type() || has_non_empty_role_arn(obj);
}
+static bool is_gcp_workload_identity_cred_provider(const ObjectStoreInfoPB&
obj) {
+ return obj.has_cred_provider_type() &&
+ obj.cred_provider_type() ==
CredProviderTypePB::GCP_WORKLOAD_IDENTITY;
+}
+
+static bool normalize_gcs_xml_endpoint(std::string* endpoint, bool
allow_legacy_http = false) {
+ trim(*endpoint);
+ if (*endpoint == "storage.googleapis.com" || *endpoint == GCS_XML_ENDPOINT
||
Review Comment:
[P2] Canonicalize legacy GCS hostnames case-insensitively
Existing HMAC vaults store the endpoint string unchanged, and a mixed-case
hostname such as `https://Storage.GoogleApis.Com` is valid because DNS
hostnames are case-insensitive. The FE's new helper also accepts this form, but
this Meta Service helper compares the stored value with case-sensitive
equality, so a credential-only HMAC-to-Workload-Identity ALTER is rejected.
Please compare/parse the scheme and hostname case-insensitively before
persisting the canonical endpoint, and cover a mixed-case legacy migration.
##########
fe/fe-core/src/main/java/org/apache/doris/catalog/S3Resource.java:
##########
@@ -94,6 +109,22 @@ protected void setProperties(ImmutableMap<String, String>
newProperties) throws
Preconditions.checkState(newProperties != null);
this.properties = Maps.newHashMap(newProperties);
+ boolean useWorkloadIdentity =
+
S3Properties.isGcpWorkloadIdentityCredentialsProvider(properties);
+ if (useWorkloadIdentity) {
+ if (!storageVault) {
Review Comment:
[P2] Enforce the storage-vault scope on ALTER RESOURCE
This restriction is applied only by `setProperties()`.
`ResourceMgr.alterResource()` calls `S3Resource.modifyProperties()`, which can
persist `s3.credentials_provider_type=gcp_workload_identity` on an ordinary S3
resource (for example when `s3_validity_check=false`) without checking
`storageVault`. Regular-resource thrift transport does not even send this mode
without a role, so the successful ALTER creates a misleading/nonfunctional
edit-log state. Please share this invariant with the modify path and add a
negative `ALTER RESOURCE` test.
##########
be/src/cloud/cloud_storage_engine.cpp:
##########
@@ -133,7 +133,12 @@ struct VaultCreateFSVisitor {
<< " check_fs: " << check_fs;
auto fs = DORIS_TRY(io::S3FileSystem::create(s3_conf, id));
- if (check_fs && !s3_conf.client_conf.role_arn.empty()) {
+ // Probe connectivity for keyless credential modes (assumed role or
+ // GCP Workload Identity): a misconfigured identity should fail loudly
at startup
+ // rather than on the first tablet write.
+ if (check_fs &&
Review Comment:
[P1] Make the Workload Identity probe reachable
`check_fs` can never be true here: `first_sync_storage_vault` is initialized
to `true`, but `sync_storage_vault()` only enables the check when a
compare-exchange from `false` to `true` succeeds, and there is no write that
makes the flag false. FE now skips its Workload Identity ping because this BE
path is supposed to validate the real metadata identity, so an invalid GKE/GCE
IAM binding is accepted and only fails on the first tablet I/O. Please make the
first-sync state transition reachable and cover this path with
`enable_check_storage_vault=true`.
##########
be/src/util/s3_util.cpp:
##########
@@ -89,6 +90,19 @@ doris::Status is_s3_conf_valid(const S3ClientConf& conf) {
if (conf.region.empty()) {
return Status::InvalidArgument<false>("Invalid s3 conf, empty region");
}
+ if (conf.cred_provider_type == CredProviderType::GcpWorkloadIdentity) {
+ if (conf.provider != io::ObjStorageType::GCP) {
+ return Status::InvalidArgument<false>("GCP workload identity
requires provider GCP");
+ }
+ if (!is_gcs_xml_endpoint(conf.endpoint)) {
Review Comment:
[P1] Refresh the endpoint during the credential transition
The supported HMAC-to-Workload-Identity ALTER canonicalizes a legacy
`storage.googleapis.com` endpoint to `https://storage.googleapis.com`, but a
running BE refreshes the existing filesystem through
`ObjClientHolder::reset()`. That method starts with the old `_conf` and copies
credentials and `cred_provider_type`, but not the new endpoint/provider. The
validation added here therefore rejects the reset configuration with its stale
host-only endpoint, and the old HMAC client (including the credentials Meta
Service just removed) remains installed until restart. Please rebuild from the
full merged client configuration, or at least propagate every identity-defining
field, and test the live transition.
##########
fe/fe-core/src/main/java/org/apache/doris/catalog/S3Resource.java:
##########
@@ -94,6 +109,22 @@ protected void setProperties(ImmutableMap<String, String>
newProperties) throws
Preconditions.checkState(newProperties != null);
this.properties = Maps.newHashMap(newProperties);
+ boolean useWorkloadIdentity =
+
S3Properties.isGcpWorkloadIdentityCredentialsProvider(properties);
+ if (useWorkloadIdentity) {
+ if (!storageVault) {
+ throw new DdlException("gcp_workload_identity is only
supported for storage vaults");
+ }
+ try {
+ String endpoint =
S3Properties.normalizeGcpWorkloadIdentityEndpoint(
Review Comment:
[P2] Normalize AWS aliases before endpoint validation
Mode detection intentionally accepts `AWS_CREDENTIALS_PROVIDER_TYPE`, but
this line reads only `s3.endpoint`; `AWS_ENDPOINT` is not converted until
`requiredS3PingProperties()` runs later. A storage-vault request using the
supported provider/endpoint aliases therefore detects Workload Identity and
immediately rejects a null endpoint. Please run `convertToStdProperties()`
before this branch (or read both aliases) and add a positive alias-based
creation 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]
