github-actions[bot] commented on code in PR #68540: URL: https://github.com/apache/doris/pull/68540#discussion_r4236791171
########## fe/fe-filesystem/fe-filesystem-gcs-common/src/main/java/org/apache/doris/filesystem/gcs/auth/GcsAuthResolver.java: ########## @@ -0,0 +1,166 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +package org.apache.doris.filesystem.gcs.auth; + + +import java.util.Locale; +import java.util.Map; +import java.util.Optional; + +/** + * Resolves GCS authentication without creating clients or loading credentials. + * Vault ALTER still preserves field presence when constructing its patch from this result. + */ +public final class GcsAuthResolver { + private static final String CREDENTIAL_PROVIDER_TYPE = GcpCredential.CREDENTIAL_PROVIDER_TYPE; + private static final String IMPERSONATION_SERVICE_ACCOUNT = GcpCredential.IMPERSONATION_SERVICE_ACCOUNT; + private static final String[] ACCESS_KEYS = {"gs.access_key", "s3.access_key", "AWS_ACCESS_KEY", "access_key"}; + private static final String[] SECRET_KEYS = {"gs.secret_key", "s3.secret_key", "AWS_SECRET_KEY", "secret_key"}; + private static final String[] TOKENS = {"gs.session_token", "s3.session_token", "AWS_TOKEN", "session_token"}; + private static final String[] AWS_PROVIDERS = {"s3.credentials_provider_type", "AWS_CREDENTIALS_PROVIDER_TYPE"}; + private static final String[] AWS_ROLES = {"s3.role_arn", "AWS_ROLE_ARN", "s3.external_id", "AWS_EXTERNAL_ID"}; + + private GcsAuthResolver() { + } + + public static Optional<GcsAuth> resolve(Map<String, String> properties) { + boolean hasNativeProperties = hasNativeCredentialProperties(properties); + boolean hasGcsSelector = guessIsGcs(properties) + || "true".equalsIgnoreCase(getPropertyIgnoreCase(properties, "fs.gcs.support")); + boolean hasAccessKey = hasNonBlankProperty(properties, ACCESS_KEYS); + boolean hasSecretKey = hasNonBlankProperty(properties, SECRET_KEYS); + boolean hasLegacyAnonymous = hasLegacyAnonymousProvider(properties); + String provider = getPropertyIgnoreCase(properties, "provider"); + if (isNotBlank(provider) && !"GCP".equalsIgnoreCase(provider) && !"GCS".equalsIgnoreCase(provider)) { + if (hasNativeProperties) { + throw new IllegalArgumentException("Native GCP authentication requires provider=GCP, but found: " + + provider); + } + // Legacy GCS configurations may explicitly select the S3-compatible protocol. + // Preserve explicit HMAC/anonymous authentication without enabling native ADC for S3. + // Omitted credentials still belong to S3's default chain, even at a GCS endpoint. + if (!"S3".equalsIgnoreCase(provider) || !hasGcsSelector + || !(hasAccessKey || hasSecretKey || hasLegacyAnonymous)) { + return Optional.empty(); + } + } + if (!hasNativeProperties && !hasGcsSelector) { + return Optional.empty(); + } + + String providerType = getPropertyIgnoreCase(properties, CREDENTIAL_PROVIDER_TYPE); + String impersonation = getPropertyIgnoreCase(properties, IMPERSONATION_SERVICE_ACCOUNT); + GcsAuth.Mode mode; + if (hasNativeProperties) { + GcpCredentialProviderType type = providerType == null ? GcpCredentialProviderType.DEFAULT + : GcpCredentialProviderType.fromString(CREDENTIAL_PROVIDER_TYPE, providerType); + mode = type == GcpCredentialProviderType.ANONYMOUS ? GcsAuth.Mode.ANONYMOUS + : type == GcpCredentialProviderType.COMPUTE_ENGINE ? GcsAuth.Mode.COMPUTE_ENGINE : GcsAuth.Mode.ADC; + } else if (hasLegacyAnonymous) { + mode = GcsAuth.Mode.ANONYMOUS; + } else if (hasAccessKey || hasSecretKey) { + mode = GcsAuth.Mode.HMAC; + } else { + mode = GcsAuth.Mode.ADC; + } + + // The storage properties validate paired HMAC keys. Raw Vault patches may update only one key. + if (mode != GcsAuth.Mode.HMAC) { + if (hasAccessKey || hasSecretKey || hasNonBlankProperty(properties, TOKENS)) { + throw new IllegalArgumentException(CREDENTIAL_PROVIDER_TYPE + + " cannot be used together with access key, secret key, or session token."); + } + if (hasNonBlankProperty(properties, AWS_ROLES) + || ((hasNativeProperties || mode != GcsAuth.Mode.ANONYMOUS) + && hasNonBlankProperty(properties, AWS_PROVIDERS))) { Review Comment: [P1] Preserve legacy GCP use of the AWS default credential chain. A persisted `provider=GCP` resource with `s3.credentials_provider_type=DEFAULT` and no static keys previously used ambient HMAC credentials, but this resolver now chooses ADC and rejects the nonblank AWS selector. On a BE report, `PushStoragePolicyTask` fails to publish the replayed resource and retains its read lock because the exception skips `readUnlock`; CREATE and ALTER using this previously valid configuration also fail. Keep explicit legacy DEFAULT on its existing HMAC/default-chain path when no native GCP credential selector is supplied, and cover replay plus policy push. ########## fe/fe-iceberg-common/src/main/java/org/apache/doris/connector/iceberg/GcpS3FileIOAwsClientFactory.java: ########## @@ -0,0 +1,74 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +package org.apache.doris.connector.iceberg; + +import org.apache.doris.filesystem.gcs.GcpAuth; +import org.apache.doris.filesystem.gcs.GcsFileSystemProperties; + +import org.apache.iceberg.aws.s3.S3FileIOAwsClientFactory; +import software.amazon.awssdk.services.s3.S3AsyncClient; +import software.amazon.awssdk.services.s3.S3Client; + +import java.io.IOException; +import java.io.UncheckedIOException; +import java.util.HashMap; +import java.util.Map; + +/** Supplies Iceberg S3FileIO clients configured with native GCP bearer authentication. */ +public class GcpS3FileIOAwsClientFactory implements S3FileIOAwsClientFactory { + private static final long serialVersionUID = 1L; + + private Map<String, String> properties; + + public GcpS3FileIOAwsClientFactory() { + } + + @Override + public void initialize(Map<String, String> properties) { + this.properties = new HashMap<>(properties); + } + + @Override + public S3Client s3() { + try { + return GcpAuth.createS3Client(gcsProperties()); Review Comment: [P2] Keep independent AWS Iceberg tables on their own S3 client. A native GCP catalog sets this as its single `s3.client-factory`, and `ResolvingFileIO` also sends every `s3://`/`s3a://` location here. For an AWS table in the same HMS/REST catalog, `s3()` builds a bearer-authenticated GCS client pointed at `storage.googleapis.com`, so its AWS metadata reads fail even when AWS credentials are configured. The earlier gs/hdfs FileIO dispatch fix does not select a different client for this S3 location. Route S3 table locations by their actual storage identity and test a mixed GCS/AWS read. ########## fe/fe-filesystem/fe-filesystem-gcs/src/main/java/org/apache/doris/filesystem/gcs/GcsFileSystemProvider.java: ########## @@ -44,6 +48,82 @@ public class GcsFileSystemProvider implements FileSystemProvider<GcsFileSystemPr private static final String STORAGE_TYPE_GCS = "GCS"; private static final String FS_GCS_SUPPORT = "fs.gcs.support"; + private static final Map<String, String> RESOURCE_ALIASES = Map.ofEntries( + Map.entry("gs.endpoint", "s3.endpoint"), + Map.entry("gs.access_key", "s3.access_key"), + Map.entry("gs.secret_key", "s3.secret_key"), + Map.entry("gs.session_token", "s3.session_token"), + Map.entry("gs.connection.maximum", "s3.connection.maximum"), + Map.entry("gs.connection.request.timeout", "s3.connection.request.timeout"), + Map.entry("gs.connection.timeout", "s3.connection.timeout"), + Map.entry("gs.use_path_style", "use_path_style"), + Map.entry("gs.force_parsing_by_standard_uri", "force_parsing_by_standard_uri")); + + @Override + public Map<String, String> normalizeProperties(Map<String, String> properties, Map<String, String> context) { + GcsAuthResolver.resolve(properties); + Map<String, String> normalized = new HashMap<>(properties); + String selected = context.get("provider"); + if (selected == null || selected.isBlank()) { + if (!GcsAuthResolver.guessIsGcs(context)) { + return normalized; + } + selected = "GCP"; + normalized.put("provider", selected); + } + if ("GCP".equalsIgnoreCase(selected)) { + RESOURCE_ALIASES.forEach((alias, key) -> { + String value = normalized.remove(alias); + if (value != null && !value.isBlank()) { + normalized.put(key, value); + } + }); + } + return normalized; Review Comment: [P2] Drop dormant GCS aliases before persisting an S3 resource. With `provider=S3`, an ALTER containing `gs.endpoint` returns here unchanged, so `S3Resource.modifyProperties` stores that otherwise ignored key. A later ALTER containing only `provider=GCP` normalizes the stored map and lets the old alias overwrite `s3.endpoint` (likewise `gs.access_key`/`gs.secret_key` overwrite HMAC keys), silently changing the destination or identity without those fields in the second ALTER. This is reachable for an unreferenced resource with `s3_validity_check=false`; it can later be used by a policy. Reject or remove inactive aliases, and test the two-ALTER sequence. -- 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]
