github-actions[bot] commented on code in PR #65834: URL: https://github.com/apache/doris/pull/65834#discussion_r3619987965
########## fe/fe-filesystem/fe-filesystem-ozone/src/main/java/org/apache/doris/filesystem/ozone/OzoneFileSystemProperties.java: ########## @@ -0,0 +1,181 @@ +// 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.ozone; + +import org.apache.doris.filesystem.s3.AbstractDelegatingS3Properties; +import org.apache.doris.foundation.property.ConnectorProperty; + +import java.util.Map; +import java.util.Set; + +/** + * Provider-owned Apache Ozone properties (via the Ozone S3 Gateway, static HMAC credentials only). + * + * <p>The public aliases, ordering, defaults, and endpoint requirement follow fe-core + * OzoneProperties: {@code ozone.*} first, falling back to {@code s3.*}; path-style addressing + * defaults to {@code true} (the S3 Gateway does not support virtual-host addressing). + */ +public final class OzoneFileSystemProperties extends AbstractDelegatingS3Properties { + + public static final String ENDPOINT = "ozone.endpoint"; + public static final String REGION = "ozone.region"; + public static final String ACCESS_KEY = "ozone.access_key"; + public static final String SECRET_KEY = "ozone.secret_key"; + public static final String SESSION_TOKEN = "ozone.session_token"; + public static final String MAX_CONNECTIONS = "ozone.connection.maximum"; + public static final String REQUEST_TIMEOUT_MS = "ozone.connection.request.timeout"; + public static final String CONNECTION_TIMEOUT_MS = "ozone.connection.timeout"; + public static final String USE_PATH_STYLE = "ozone.use_path_style"; + public static final String FORCE_PARSING_BY_STANDARD_URI = + "ozone.force_parsing_by_standard_uri"; + + public static final String DEFAULT_REGION = "us-east-1"; + + @ConnectorProperty(names = {ENDPOINT, "s3.endpoint", "AWS_ENDPOINT"}, + required = false, + description = "The endpoint of the Ozone S3 Gateway.") + private String endpoint = ""; + + @ConnectorProperty(names = {REGION, "s3.region"}, Review Comment: [P1] Bind the canonical Ozone region key A map explicitly routed with `provider=OZONE` carries its normalized region as `AWS_REGION` (and the legacy converter will do the same once its dialect signal is preserved). This alias list accepts the other canonical `AWS_*` fields but not `AWS_REGION`, so `eu-west-2` binds as the default `us-east-1` and that wrong value reaches the SDK signer. Please add `AWS_REGION` here and test a non-default canonical value; the current `us-east-1` fixture cannot distinguish successful binding from this loss. Today MERGED-1 masks this on the legacy route because generic S3 still wins, but fixing that routing exposes this independent failure. ########## fe/fe-filesystem/fe-filesystem-s3-base/src/main/java/org/apache/doris/filesystem/s3/S3CompatSignals.java: ########## @@ -0,0 +1,253 @@ +// 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.s3; + +import org.apache.commons.lang3.StringUtils; + +import java.util.Arrays; +import java.util.Collections; +import java.util.List; +import java.util.Locale; +import java.util.Map; + +/** + * Shared routing/validation signals for the S3-compatible dialects (GCS, MinIO, Ozone). + * + * <p>This is dialect plumbing rather than user-facing API, but it must be {@code public}: the + * dialect providers live in sibling plugin jars that share this package name (a split package, + * as with {@code fe-filesystem-hdfs-base}). Package-private access across jars only works when + * both are loaded by the same classloader; the plugin classloaders put them in different runtime + * packages, so package-private members would fail with {@code IllegalAccessError} at runtime. + */ +public final class S3CompatSignals { + + /** Explicit provider hint, e.g. {@code provider=GCS}. Mirrors StorageProperties.FS_PROVIDER_KEY. */ + public static final String PROVIDER_KEY = "provider"; + + public static final String FS_S3_SUPPORT = "fs.s3.support"; + + private static final String FS_SUPPORT_PREFIX = "fs."; + private static final String FS_SUPPORT_SUFFIX = ".support"; + + /** Legacy alias for {@code fs.oss-hdfs.support}, honored by StorageProperties.hasAnyExplicitFsSupport. */ + private static final String DEPRECATED_OSS_HDFS_SUPPORT = "oss.hdfs.enabled"; + + private static final String GCS_ENDPOINT_KEY = "gs.endpoint"; + private static final String GCS_ENDPOINT_SUFFIX = "storage.googleapis.com"; + private static final String AWS_ENDPOINT_INFIX = "amazonaws.com"; + + /** + * Endpoint aliases consulted by the GCS/AWS guesses, compared case-insensitively on the KEY. + * Union of legacy {@code GCSProperties.GS_ENDPOINT_ALIAS} ({@code s3.endpoint}, {@code AWS_ENDPOINT}, + * {@code endpoint}, {@code ENDPOINT}) and this module's own endpoint aliases. + */ + private static final List<String> ENDPOINT_ALIASES_LOWER = Collections.unmodifiableList( + Arrays.asList("gs.endpoint", "s3.endpoint", "aws_endpoint", "endpoint", "aws.endpoint", + "glue.endpoint", "aws.glue.endpoint")); + + /** Credential options that only make sense on AWS S3 proper. */ + private static final String[] AWS_ONLY_CREDENTIAL_KEYS = { + "sts.role_arn", "AWS_ROLE_ARN", S3FileSystemProperties.ROLE_ARN, "glue.role_arn", + "sts.external_id", "AWS_EXTERNAL_ID", S3FileSystemProperties.EXTERNAL_ID, "glue.external_id"}; + + private static final String[] CREDENTIALS_PROVIDER_TYPE_KEYS = { + S3FileSystemProperties.CREDENTIALS_PROVIDER_TYPE, "AWS_CREDENTIALS_PROVIDER_TYPE", + "glue.credentials_provider_type", "iceberg.rest.credentials_provider_type"}; + + private S3CompatSignals() {} + + /** True when any key with the given prefix carries a non-blank value. */ + public static boolean hasPrefixKey(Map<String, String> properties, String prefix) { + for (Map.Entry<String, String> entry : properties.entrySet()) { + if (entry.getKey() != null && entry.getKey().startsWith(prefix) + && StringUtils.isNotBlank(entry.getValue())) { + return true; + } + } + return false; + } + + /** + * True when the raw map requests an AWS-only credential mechanism (AssumeRole role ARN, + * external ID, or a credentials provider type other than DEFAULT/ANONYMOUS). GCS/MinIO/Ozone + * only support static HMAC keys, so their validate() rejects these at binding time instead of + * failing obscurely at runtime. + */ + public static boolean hasAwsOnlyCredentialOptions(Map<String, String> rawProperties) { + for (String key : AWS_ONLY_CREDENTIAL_KEYS) { + if (StringUtils.isNotBlank(rawProperties.get(key))) { + return true; + } + } + for (String key : CREDENTIALS_PROVIDER_TYPE_KEYS) { + String value = rawProperties.get(key); + if (StringUtils.isNotBlank(value) + && !"DEFAULT".equalsIgnoreCase(value.trim()) + && !"ANONYMOUS".equalsIgnoreCase(value.trim())) { + return true; + } + } + return false; + } + + /** True when {@code key=true} (case-insensitive). Mirrors StorageProperties.isFsSupport. */ + public static boolean isFsSupport(Map<String, String> properties, String key) { + return "true".equalsIgnoreCase(properties.getOrDefault(key, "false")); + } + + /** + * True when the user explicitly declared ANY filesystem via {@code fs.<x>.support=true}. + * + * <p>Counterpart of {@code StorageProperties.hasAnyExplicitFsSupport}. That method enumerates the + * known flags one by one; here the check is structural ({@code fs.*.support}) so that this module + * never has to know the full dialect list. Same contract: once any explicit flag is present, all + * {@code guessIsX} heuristics must be disabled so an explicit declaration is never overridden. + */ + public static boolean hasAnyExplicitFsSupport(Map<String, String> properties) { + if (isFsSupport(properties, DEPRECATED_OSS_HDFS_SUPPORT)) { + return true; + } + for (Map.Entry<String, String> entry : properties.entrySet()) { + String key = entry.getKey(); + if (key != null && key.startsWith(FS_SUPPORT_PREFIX) && key.endsWith(FS_SUPPORT_SUFFIX) + && "true".equalsIgnoreCase(entry.getValue())) { + return true; + } + } + return false; + } + + /** + * True when the user explicitly asked for the generic S3 provider ({@code provider=S3} or + * {@code fs.s3.support=true}). Dialect providers must not guess against such a map, and the S3 + * provider uses it as its escape hatch from the dialect yield rule. + * + * <p>Note the {@code _STORAGE_TYPE_} marker is intentionally NOT consulted: the converter stamps Review Comment: [P1] Preserve a dialect signal across legacy conversion The normal FE path does not pass the raw map to these providers: `StoragePropertiesConverter` rewrites every `AbstractS3CompatibleProperties` to canonical `AWS_*` keys plus `_STORAGE_TYPE_=S3` (the inherited `getStorageName()` value). GCS keeps `provider=GCP`, but MinIO and Ozone keep neither their `fs.*.support` flag nor a `provider` hint. Consequently both dedicated `supports` methods return false, while generic S3 treats the marker as explicit and wins. Please preserve an authoritative MinIO/Ozone signal at the conversion boundary and add a test that runs the real legacy object through conversion and provider arbitration; the current `_STORAGE_TYPE_=MINIO/OZONE` fixtures are not values the converter emits. ########## build.sh: ########## @@ -1048,7 +1048,7 @@ if [[ "${BUILD_FE}" -eq 1 ]]; then # Deploy filesystem provider plugins as independent plugin directories # Each sub-directory is one storage backend loaded at runtime by FileSystemPluginManager. FS_PLUGIN_DIR="${DORIS_OUTPUT}/fe/plugins/filesystem" - for fs_module in s3 azure oss cos obs hdfs oss-hdfs jfs local broker http; do + for fs_module in s3 gcs minio ozone azure oss cos obs hdfs oss-hdfs jfs local broker http; do Review Comment: [P1] Add the new plugins to the FE Maven selection list This deployment loop now unpacks GCS, MinIO, and Ozone, but the `FE_MODULES` loop at lines 720-724 still selects only the pre-existing filesystem modules. Because the build runs `mvn package -pl ... -am`, it builds `s3-base` as a dependency of S3 but never builds these three new dependent modules. On a clean checkout (and deterministically with `./build.sh --fe --clean`), their source directories pass the guard and the first missing `target/doris-fe-filesystem-gcs.zip` makes `unzip` abort under `set -e`. Please add all three modules to the selection loop (and ideally assert parity between the build and deployment lists). ########## fe/fe-filesystem/fe-filesystem-s3-base/src/main/java/org/apache/doris/filesystem/s3/AbstractDelegatingS3Properties.java: ########## @@ -0,0 +1,243 @@ +// 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.s3; + +import org.apache.doris.filesystem.FileSystemType; +import org.apache.doris.filesystem.properties.BackendStorageKind; +import org.apache.doris.filesystem.properties.BackendStorageProperties; +import org.apache.doris.filesystem.properties.FileSystemProperties; +import org.apache.doris.filesystem.properties.HadoopStorageProperties; +import org.apache.doris.filesystem.properties.S3CompatibleFileSystemProperties; +import org.apache.doris.filesystem.properties.StorageKind; +import org.apache.doris.foundation.property.ConnectorPropertiesUtils; +import org.apache.doris.foundation.property.ConnectorProperty; +import org.apache.doris.foundation.property.ParamRules; + +import org.apache.commons.lang3.StringUtils; + +import java.lang.reflect.Field; +import java.util.Collections; +import java.util.HashMap; +import java.util.Map; +import java.util.Optional; + +/** + * Base class for S3-compatible dialect properties. Concrete dialects live in their own plugin + * modules (fe-filesystem-gcs, fe-filesystem-minio, fe-filesystem-ozone) and depend on this one. + * + * <p>A dialect differs from plain S3 only in property spelling (aliases), defaults, and the + * allowed credential surface (static HMAC keys only). Subclasses declare the annotated alias + * fields and dialect-specific validation; this class owns the shared plumbing: binding, + * matched-property collection, the canonical {@code AWS_*} delegation map consumed by + * {@link S3FileSystemProperties#of(Map)}, the Hadoop s3a configuration, and masked rendering. + */ +public abstract class AbstractDelegatingS3Properties + implements FileSystemProperties, BackendStorageProperties, HadoopStorageProperties, + S3CompatibleFileSystemProperties { + + // Alias order matches legacy AbstractS3CompatibleProperties.getBucket(), which reads + // s3.bucket first and falls back to AWS_BUCKET, for every S3-compatible dialect. + @ConnectorProperty(names = {"s3.bucket", "AWS_BUCKET"}, + required = false, + description = "The default bucket name.") + protected String bucket = ""; + + @ConnectorProperty(names = {"AWS_ROOT_PATH"}, + required = false, + description = "The root path prefix inside the bucket.") + protected String rootPath = ""; + + private final Map<String, String> rawProperties; + private Map<String, String> matchedProperties = Collections.emptyMap(); + + protected AbstractDelegatingS3Properties(Map<String, String> rawProperties) { + this.rawProperties = Collections.unmodifiableMap(new HashMap<>(rawProperties)); + } + + /** + * Binds raw properties onto the annotated fields (this class and the concrete subclass; + * {@link ConnectorPropertiesUtils} walks the hierarchy) and collects matched keys. + * + * <p>MUST be called from the subclass's static {@code of()} factory AFTER construction: + * subclass field initializers run after this base constructor, so binding inside the + * constructor would be silently overwritten by the dialect defaults. + */ + protected final void bindAndCollect() { + ConnectorPropertiesUtils.bindConnectorProperties(this, rawProperties); + Map<String, String> matched = new HashMap<>(); + for (Field field : ConnectorPropertiesUtils.getConnectorProperties(getClass())) { + String matchedName = ConnectorPropertiesUtils.getMatchedPropertyName(field, rawProperties); + if (StringUtils.isNotBlank(matchedName)) { + matched.put(matchedName, rawProperties.get(matchedName)); + } + } + this.matchedProperties = Collections.unmodifiableMap(matched); + } + + /** + * Shared validation for HMAC-only dialects. {@code aliasPrefix} is the dialect key prefix + * without the dot (e.g. {@code gs}); {@code displayName} the human-readable storage name. + */ + protected final void validateHmacDialect(String aliasPrefix, String displayName) { + new ParamRules() + .requireTogether(new String[] {getAccessKey(), getSecretKey()}, + aliasPrefix + ".access_key and " + aliasPrefix + + ".secret_key must be set together") + .requireAllIfPresent(getSessionToken(), new String[] {getAccessKey(), getSecretKey()}, + aliasPrefix + ".session_token requires " + aliasPrefix + ".access_key and " + + aliasPrefix + ".secret_key") + .check(() -> StringUtils.isBlank(getEndpoint()), + "Property " + aliasPrefix + ".endpoint is required.") + .check(this::hasInvalidUsePathStyle, + "use_path_style must be true or false, got: '" + getUsePathStyle() + "'") + .check(() -> S3CompatSignals.hasAwsOnlyCredentialOptions(rawProperties), + displayName + " supports only HMAC access_key/secret_key credentials; " + + "role_arn/instance-profile style options are not supported") + .validate("Invalid " + displayName + " filesystem properties"); + } + + @Override + public StorageKind kind() { + return StorageKind.OBJECT_STORAGE; + } + + @Override + public FileSystemType type() { + return FileSystemType.S3; + } + + @Override + public BackendStorageKind backendKind() { + return BackendStorageKind.S3_COMPATIBLE; + } + + @Override + public Map<String, String> rawProperties() { + return rawProperties; + } + + @Override + public Map<String, String> matchedProperties() { + return matchedProperties; + } + + @Override + public Optional<BackendStorageProperties> toBackendProperties() { + return Optional.of(this); + } + + @Override + public Optional<HadoopStorageProperties> toHadoopProperties() { + return Optional.of(this); + } + + @Override + public Map<String, String> toMap() { + return toS3CompatibleKv(); + } + + /** + * Canonical {@code AWS_*} keys accepted by {@link S3FileSystemProperties#of(Map)}; this is + * how a dialect delegates to the shared S3 interoperability client. + */ + public final Map<String, String> toS3CompatibleKv() { + Map<String, String> kv = new HashMap<>(); + kv.put("AWS_ENDPOINT", getEndpoint()); + kv.put("AWS_REGION", getRegion()); + putIfNotBlank(kv, "AWS_ACCESS_KEY", getAccessKey()); + putIfNotBlank(kv, "AWS_SECRET_KEY", getSecretKey()); + putIfNotBlank(kv, "AWS_TOKEN", getSessionToken()); + putIfNotBlank(kv, "AWS_BUCKET", bucket); + putIfNotBlank(kv, "AWS_ROOT_PATH", rootPath); + kv.put("AWS_MAX_CONNECTIONS", getMaxConnections()); + kv.put("AWS_REQUEST_TIMEOUT_MS", getRequestTimeoutMs()); + kv.put("AWS_CONNECTION_TIMEOUT_MS", getConnectionTimeoutMs()); + kv.put("use_path_style", getUsePathStyle()); + // Legacy AbstractS3CompatibleProperties emits this marker for EVERY S3-compatible + // dialect when no HMAC pair is configured; without it the shared S3 client falls back + // to the AWS default provider chain and signs requests with ambient env credentials. + if (!hasStaticCredentials()) { Review Comment: [P1] Preserve an explicitly requested DEFAULT credential mode A validated raw GCP resource map can set `AWS_CREDENTIALS_PROVIDER_TYPE=DEFAULT` with no inline AK/SK. Before this routing change, generic S3 preserved that mode and used the environment/system/profile/instance credential chain. The GCS provider now wins, validation explicitly accepts `DEFAULT`, but this branch overwrites it with `ANONYMOUS`, so the same request becomes unsigned. Please preserve a supplied `DEFAULT` and synthesize anonymous only when no mode was requested (or reject `DEFAULT` explicitly if that is the intended contract), with an end-to-end provider/delegate test. This concerns the raw resource path; legacy converted maps without keys were already anonymous. -- 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]
