github-actions[bot] commented on code in PR #68540:
URL: https://github.com/apache/doris/pull/68540#discussion_r4217149855
##########
fe/fe-filesystem/fe-filesystem-gcs/src/main/java/org/apache/doris/filesystem/gcs/GcsFileSystemProperties.java:
##########
@@ -134,10 +153,63 @@ public Map<String, String> toMap() {
return Collections.unmodifiableMap(kv);
}
+ private static Map<String, String> withGcpProvider(Map<String, String>
properties) {
+ Map<String, String> selected = new HashMap<>(properties);
+ selected.put("provider", "GCP");
+ return selected;
+ }
+
+ public GcsAuth getAuth() {
+ return auth;
+ }
+
+ @Override
+ public Map<String, String> matchedProperties() {
+ Map<String, String> matched = new HashMap<>(super.matchedProperties());
+ if (auth.getMode() != GcsAuth.Mode.HMAC) {
+ matched.put(GcpCredential.CREDENTIAL_PROVIDER_TYPE,
auth.isAnonymous() ? "ANONYMOUS"
+ :
auth.getNativeCredential().orElseThrow().getCredentialProviderType().name());
+ matched.put(GcpCredential.IMPERSONATION_SERVICE_ACCOUNT,
+
auth.getNativeCredential().map(GcpCredential::getImpersonationServiceAccount).orElse(""));
+ }
+ return Collections.unmodifiableMap(matched);
+ }
+
+ @Override
+ protected void customizeS3CompatibleKv(Map<String, String> kv) {
+ if (auth.getNativeCredential().isPresent()) {
+ GcpCredential credential = auth.getNativeCredential().get();
+ kv.remove("AWS_CREDENTIALS_PROVIDER_TYPE");
+ kv.put(GcpCredential.CREDENTIAL_PROVIDER_TYPE,
credential.getCredentialProviderType().name());
+ putIfNotBlank(kv, GcpCredential.IMPERSONATION_SERVICE_ACCOUNT,
+ credential.getImpersonationServiceAccount());
+ }
+ }
+
+ @Override
+ public Map<String, String> toHadoopConfigurationMap() {
+ if (auth.getNativeCredential().isEmpty()) {
+ return super.toHadoopConfigurationMap();
+ }
+ GcpCredential credential = auth.getNativeCredential().get();
+ Map<String, String> cfg = new HashMap<>();
+ cfg.put("fs.gs.impl",
"com.google.cloud.hadoop.fs.gcs.GoogleHadoopFileSystem");
+ String storageRoot = getEndpoint();
+ if (!storageRoot.contains("://")) {
+ storageRoot = "https://"; + storageRoot;
+ }
+ cfg.put("fs.gs.storage.root.url", storageRoot.endsWith("/") ?
storageRoot : storageRoot + "/");
Review Comment:
[P1] Validate the effective Hadoop GCS endpoint before attaching OAuth
credentials. This sets `fs.gs.storage.root.url` from the checked `gs.endpoint`,
but Iceberg's `buildHadoopConfiguration()` applies raw catalog `fs.*`
properties afterward; `fs.gs.storage.root.url=https://attacker.example/`
replaces it for a native `gs://` Hadoop warehouse. The pinned GCS connector
builds its JSON client with that root URL and a credential-bearing request
initializer, so catalog filesystem calls send the FE ADC/Compute Engine token
to the caller-selected host. Hudi and Paimon have the same raw override. Reject
or revalidate endpoint-related `fs.gs.*` overrides when native auth is active.
[Connector
source](https://github.com/GoogleCloudDataproc/hadoop-connectors/blob/v3.1.18/gcsio/src/main/java/com/google/cloud/hadoop/gcsio/GoogleCloudStorageImpl.java).
##########
fe/fe-filesystem/fe-filesystem-gcs/src/main/java/org/apache/doris/filesystem/gcs/GcsFileSystemProperties.java:
##########
@@ -134,10 +153,63 @@ public Map<String, String> toMap() {
return Collections.unmodifiableMap(kv);
}
+ private static Map<String, String> withGcpProvider(Map<String, String>
properties) {
+ Map<String, String> selected = new HashMap<>(properties);
+ selected.put("provider", "GCP");
+ return selected;
+ }
+
+ public GcsAuth getAuth() {
+ return auth;
+ }
+
+ @Override
+ public Map<String, String> matchedProperties() {
+ Map<String, String> matched = new HashMap<>(super.matchedProperties());
+ if (auth.getMode() != GcsAuth.Mode.HMAC) {
+ matched.put(GcpCredential.CREDENTIAL_PROVIDER_TYPE,
auth.isAnonymous() ? "ANONYMOUS"
+ :
auth.getNativeCredential().orElseThrow().getCredentialProviderType().name());
+ matched.put(GcpCredential.IMPERSONATION_SERVICE_ACCOUNT,
+
auth.getNativeCredential().map(GcpCredential::getImpersonationServiceAccount).orElse(""));
+ }
+ return Collections.unmodifiableMap(matched);
+ }
+
+ @Override
+ protected void customizeS3CompatibleKv(Map<String, String> kv) {
+ if (auth.getNativeCredential().isPresent()) {
+ GcpCredential credential = auth.getNativeCredential().get();
+ kv.remove("AWS_CREDENTIALS_PROVIDER_TYPE");
+ kv.put(GcpCredential.CREDENTIAL_PROVIDER_TYPE,
credential.getCredentialProviderType().name());
+ putIfNotBlank(kv, GcpCredential.IMPERSONATION_SERVICE_ACCOUNT,
+ credential.getImpersonationServiceAccount());
+ }
+ }
+
+ @Override
+ public Map<String, String> toHadoopConfigurationMap() {
+ if (auth.getNativeCredential().isEmpty()) {
+ return super.toHadoopConfigurationMap();
+ }
+ GcpCredential credential = auth.getNativeCredential().get();
+ Map<String, String> cfg = new HashMap<>();
+ cfg.put("fs.gs.impl",
"com.google.cloud.hadoop.fs.gcs.GoogleHadoopFileSystem");
Review Comment:
[P2] Preserve Hadoop access to GCS-backed Hudi `s3://` locations. Native GCP
auth returns only `fs.gs.*` here, but this class still advertises `s3` and
`s3a`, and Hudi passes the HMS table location unchanged to
`HoodieTableMetaClient` with this Hadoop configuration. A GCS Hudi table stored
as `s3://bucket/table` therefore selects S3A without the old GCS
endpoint/credentials (or the default S3 implementation), so its `.hoodie`
metadata cannot be read under native auth. Rewrite the Hadoop-facing Hudi path
to `gs://` or provide equivalent OAuth-capable mappings for these aliases.
##########
fe/fe-core/src/main/java/org/apache/doris/catalog/S3Resource.java:
##########
@@ -218,72 +237,109 @@ protected static void pingS3(String bucketName, String
rootPath, Map<String, Str
LOG.info("success to ping s3");
}
+ private static void normalizeProperties(Map<String, String> properties,
String provider) {
+ // Validate the raw aliases before a preferred value can hide
conflicting credentials.
+ GcsAuthResolver.resolve(properties);
+ if (provider == null) {
+ return;
+ }
+ switch (provider.toUpperCase(Locale.ROOT)) {
+ case "GCP":
+ // Normalize before validation, policy checks and persistence,
so FE connector
+ // binding and Resource/Vault protocol builders consume the
same values.
+ GCS_PROPERTY_ALIASES.forEach((alias, key) -> {
+ String value = properties.remove(alias);
+ // Match connector binding: nonblank gs.* values take
precedence.
+ if (StringUtils.isNotBlank(value)) {
+ properties.put(key, value);
+ }
+ });
+ break;
+ default:
+ break;
+ }
+ }
+
@Override
- public void modifyProperties(Map<String, String> properties) throws
DdlException {
+ public synchronized void modifyProperties(Map<String, String>
newProperties) throws DdlException {
+ // Serialize the snapshot, validation and publication. A lock only
around publication
+ // would allow a concurrent ALTER to replace a successful update with
an older snapshot.
+ Map<String, String> properties = new HashMap<>(newProperties);
+ String provider =
StringUtils.defaultIfEmpty(properties.get("provider"),
+ this.properties.get("provider"));
+ // Preserve AWS_* ALTER compatibility before merging with stored
canonical properties.
+ S3ResourceCompat.convertToStdProperties(properties);
+ // Resolve aliases separately so this ALTER wins over persisted values
regardless
+ // of their spelling. Within each map, nonblank gs.* values still take
precedence.
+ normalizeProperties(properties, provider);
+ Map<String, String> effectiveProperties = new
HashMap<>(this.properties);
+ normalizeProperties(effectiveProperties, provider);
+ S3ResourceCompat.convertToStdProperties(effectiveProperties);
+ for (Map.Entry<String, String> update : properties.entrySet()) {
+ // Match persistence: empty updates are ignored, except when
clearing a session token.
+ replaceIfEffectiveValue(effectiveProperties, update.getKey(),
update.getValue());
+ if (S3ResourceCompat.SESSION_TOKEN.equals(update.getKey())
+ || S3ResourceCompat.Env.TOKEN.equals(update.getKey())) {
+ effectiveProperties.put(update.getKey(), update.getValue());
+ }
+ }
if (references.containsValue(ReferenceType.POLICY)) {
// can't change, because remote fs use it info to find data.
List<String> cantChangeProperties =
Arrays.asList(S3ResourceCompat.ENDPOINT, S3ResourceCompat.REGION,
S3ResourceCompat.ROOT_PATH, S3ResourceCompat.BUCKET,
S3ResourceCompat.Env.ENDPOINT,
S3ResourceCompat.Env.REGION,
S3ResourceCompat.Env.ROOT_PATH,
S3ResourceCompat.Env.BUCKET);
- Optional<String> any =
cantChangeProperties.stream().filter(properties::containsKey).findAny();
+ Optional<String> any = cantChangeProperties.stream()
+ .filter(key -> properties.containsKey(key)
+ || !Objects.equals(this.properties.get(key),
effectiveProperties.get(key)))
+ .findAny();
if (any.isPresent()) {
throw new DdlException("current not support modify property :
" + any.get());
}
}
- // compatible with old version, Need convert if modified properties
map uses old properties.
- S3ResourceCompat.convertToStdProperties(properties);
- if (!Strings.isNullOrEmpty(properties.get(S3ResourceCompat.ENDPOINT)))
{
- properties.put(S3ResourceCompat.Env.ENDPOINT,
properties.get(S3ResourceCompat.ENDPOINT));
+ if
(!Strings.isNullOrEmpty(effectiveProperties.get(S3ResourceCompat.ENDPOINT))) {
+ effectiveProperties.put(S3ResourceCompat.Env.ENDPOINT,
effectiveProperties.get(S3ResourceCompat.ENDPOINT));
}
- boolean needCheck = isNeedCheck(properties);
+ for (Map.Entry<String, String> kv : properties.entrySet()) {
+ if (kv.getKey().equalsIgnoreCase(S3ResourceCompat.ROLE_ARN)
+ && !Strings.isNullOrEmpty(kv.getValue())) {
+ effectiveProperties.remove(S3ResourceCompat.ACCESS_KEY);
+ effectiveProperties.remove(S3ResourceCompat.Env.ACCESS_KEY);
+ effectiveProperties.remove(S3ResourceCompat.SECRET_KEY);
+ effectiveProperties.remove(S3ResourceCompat.Env.SECRET_KEY);
+ }
+ if (kv.getKey().equalsIgnoreCase(S3ResourceCompat.ACCESS_KEY)
+ && !Strings.isNullOrEmpty(kv.getValue())) {
+ effectiveProperties.remove(S3ResourceCompat.ROLE_ARN);
+ effectiveProperties.remove(S3ResourceCompat.Env.ROLE_ARN);
+ effectiveProperties.remove(S3ResourceCompat.EXTERNAL_ID);
+ effectiveProperties.remove(S3ResourceCompat.Env.EXTERNAL_ID);
+ }
+ }
+ GcsAuthResolver.resolve(effectiveProperties);
Review Comment:
[P2] Allow an existing GCP resource to change authentication mode. This
validates a merged map that still contains the previous credentials: adding
`gs.credential_provider_type=DEFAULT` to an HMAC resource leaves its stored
AK/SK, and blank AK/SK updates are ignored. The resolver rejects that mix; the
reverse change retains the native selector too. This prevents in-place
migration of a resource used by a storage policy. Clear mutually exclusive
credentials when ALTER explicitly selects the other mode, then validate the
resulting map.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/storage/CloudObjectStoreAdapter.java:
##########
@@ -115,6 +123,8 @@ public static Cloud.ObjectStoreInfoPB.Builder
getObjStoreInfoPB(Map<String, Stri
}
}
+ ObjCredentialFactory.fromProperties(properties, gcsAuth)
Review Comment:
[P2] Include native GCP identity in SHOW CREATE STORAGE VAULT. This writes
the selected provider type and impersonation account into
`ObjectStoreInfoPB.credential`, but
`ShowCreateStorageVaultCommand.getObjectCreateStmt()` reconstructs DDL using
only legacy fields plus `provider=GCP`. Recreating a COMPUTE_ENGINE or
impersonated vault from that DDL therefore selects DEFAULT ADC and can lose
access to its bucket. Emit `gs.credential_provider_type` and
`gs.impersonation_service_account` from the stored credential and cover a SHOW
CREATE round trip.
##########
cloud/src/meta-service/meta_service_resource.cpp:
##########
@@ -1162,28 +1201,72 @@ static int alter_s3_storage_vault_by_id(InstanceInfoPB&
instance, std::unique_pt
new_vault.mutable_obj_info()->clear_role_arn();
new_vault.mutable_obj_info()->clear_external_id();
new_vault.mutable_obj_info()->clear_cred_provider_type();
+ new_vault.mutable_obj_info()->clear_credential();
Review Comment:
[P2] Keep native vault auth when an AK/SK replacement is empty. `ALTER
STORAGE VAULT` accepts `s3.access_key=""` and `s3.secret_key=""`; FE sets both
fields, so the pair-presence check and encryption succeed here, then this line
erases the working GCP credential. The resulting empty keys pass
`validate_obj_authentication()` and are persisted, while recycler skips the
empty pair and has no OAuth credential to use. Require both replacement values
to be nonempty before clearing the current credential, and test empty and
half-empty replacements.
--
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]
