github-actions[bot] commented on code in PR #67157: URL: https://github.com/apache/doris/pull/67157#discussion_r3888958515
########## fe/fe-core/src/main/java/org/apache/doris/datasource/lance/LanceOssStorageProvider.java: ########## @@ -0,0 +1,133 @@ +// 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.datasource.lance; + +import org.apache.doris.datasource.property.storage.OSSProperties; +import org.apache.doris.datasource.property.storage.StorageProperties; + +import com.google.common.collect.ImmutableMap; +import org.apache.commons.lang3.StringUtils; + +import java.util.HashMap; +import java.util.List; +import java.util.Map; + +/** Alibaba Cloud OSS storage, which Lance reaches through its OpenDAL OSS provider. */ +final class LanceOssStorageProvider implements LanceStorageProvider { + + static final LanceOssStorageProvider INSTANCE = new LanceOssStorageProvider(); + + private static final String ENDPOINT = "oss_endpoint"; + private static final String ACCESS_KEY_ID = "oss_access_key_id"; + private static final String SECRET_ACCESS_KEY = "oss_secret_access_key"; + private static final String REGION = "oss_region"; + private static final String SECURITY_TOKEN = "oss_security_token"; + private static final String ADDRESSING_STYLE = "addressing_style"; + private static final String ALLOW_ANONYMOUS = "allow_anonymous"; + + /** + * Lance exposes the {@code oss_*} names as its public storage-option vocabulary and normalizes + * them to OpenDAL's field names before constructing the operator. Both spellings are accepted, + * so collapse only these known pairs before merging static and namespace-vended options. + */ + private static final Map<String, String> PUBLIC_BY_ALIAS = ImmutableMap.<String, String>builder() + .put("endpoint", ENDPOINT) + .put(ENDPOINT, ENDPOINT) + .put("access_key_id", ACCESS_KEY_ID) + .put(ACCESS_KEY_ID, ACCESS_KEY_ID) + .put("access_key_secret", SECRET_ACCESS_KEY) + .put(SECRET_ACCESS_KEY, SECRET_ACCESS_KEY) + .put("region", REGION) + .put(REGION, REGION) + .put("security_token", SECURITY_TOKEN) + .put(SECURITY_TOKEN, SECURITY_TOKEN) + .build(); + + private LanceOssStorageProvider() { + } + + @Override + public Map<String, String> fromDorisProperties(List<StorageProperties> storageProperties) { + Map<String, String> result = new HashMap<>(); + OSSProperties properties = selectOss(storageProperties); + if (properties == null) { + return result; + } + putIfNotEmpty(result, ENDPOINT, properties.getEndpoint()); + putIfNotEmpty(result, ACCESS_KEY_ID, properties.getAccessKey()); + putIfNotEmpty(result, SECRET_ACCESS_KEY, properties.getSecretKey()); + putIfNotEmpty(result, REGION, properties.getRegion()); + putIfNotEmpty(result, SECURITY_TOKEN, properties.getSessionToken()); + + // Doris reads a blank key pair as a request for anonymous access. Lance forwards options it + // does not recognize straight to OpenDAL, whose OSS service only skips request signing when + // allow_anonymous is set; without it the open fails in credential loading instead of + // issuing the unsigned request Doris asked for. Decide from what was actually emitted above + // rather than re-testing the properties, so a credential this class considers present can + // never be paired with a claim that there is none. + if (!result.containsKey(ACCESS_KEY_ID) && !result.containsKey(SECRET_ACCESS_KEY)) { + result.put(ALLOW_ANONYMOUS, "true"); + } + + // Lance snapshots the host's OSS_*/AWS_*/ALIBABA_CLOUD_* environment into the same config + // map before storage options are applied, so state both addressing styles explicitly the + // way the S3 provider does. Leaving the default implicit would let an exported + // OSS_ADDRESSING_STYLE outrank an explicit oss.use_path_style=false. + String usePathStyle = properties.getUsePathStyle(); + if (StringUtils.isNotEmpty(usePathStyle)) { + result.put(ADDRESSING_STYLE, Boolean.parseBoolean(usePathStyle) ? "path" : "virtual"); + } + return result; + } + + @Override + public Map<String, String> normalizeVended(Map<String, String> vendedOptions) { + Map<String, String> result = new HashMap<>(); + if (vendedOptions == null) { + return result; + } + vendedOptions.forEach((key, value) -> { Review Comment: [P1] Reconcile the OSS auth tuple after vending The normalized vended map is overlaid on the new static map one key at a time, leaving unrelated auth state behind. An anonymous catalog plus vended AK/SK retains `allow_anonymous=true`; both pinned OpenDAL OSS versions then keep every FE metadata read and BE scan unsigned. Likewise, replacing static `AK0/SK0/TOKEN0` with a valid tokenless `AK1/SK1` retains the stale `TOKEN0` and signs with a mismatched tuple. Please reconcile key, secret, token, and anonymous mode as one unit after the merge: clear anonymous mode when an effective pair is present, clear a stale token when the pair is replaced without one, and reject one-sided pairs. This is distinct from the existing static-only anonymous thread because it occurs only when namespace-vended auth overlays the static state added here. ########## fe/fe-core/src/main/java/org/apache/doris/datasource/lance/LanceExternalCatalog.java: ########## @@ -498,12 +516,26 @@ String sanitizedRootCauseMessage(Throwable throwable, String datasetUri, return truncateUtf8(removeControlCharacters(message), MAX_PROVIDER_MESSAGE_BYTES); } + /** + * Always hand back a rebuilt cause. Returning the original once left every catalog without REST + * auth - a directory namespace on OSS, say - free to carry provider text holding its + * credentials, which the message beside it had just redacted. + */ private Throwable safeCause(Throwable throwable) { - if (StringUtils.isNotEmpty(catalogProperty.getOrDefault(REST_BEARER_TOKEN, "")) - || StringUtils.isNotEmpty(catalogProperty.getOrDefault(REST_API_KEY, ""))) { - return new RuntimeException(sanitizedRootCauseMessage(throwable)); + return sanitizedCause(throwable, sanitizedRootCauseMessage(throwable)); + } + + private static Throwable sanitizedCause(Throwable throwable, String sanitizedMessage) { + // The rebuilt cause keeps the provider's credentials out of the user-visible error, but it + // also drops the original type, stack and suppressed exceptions. Keep them at debug level: + // the untouched text can hold those same credentials, so it must not reach the log by + // default, and an operator debugging a provider failure can turn it on deliberately. + if (LOG.isDebugEnabled()) { + LOG.debug("Lance provider failure, replaced by a redacted cause", throwable); Review Comment: [P1] Keep the original provider failure out of logs This DEBUG call serializes the original throwable even though the adjacent code acknowledges that it can contain OSS/S3 credentials and provider-controlled newlines. Thus the returned exception is redacted, but enabling DEBUG writes the same secrets and unsanitized text to the FE log for every newly routed failure path. This is a different sink from the existing user-visible cause thread. Please log only the rebuilt sanitized throwable/message (or non-sensitive operation/type metadata), and cover the log appender in the redaction test. ########## fe/fe-core/src/main/java/org/apache/doris/datasource/lance/LanceOssStorageProvider.java: ########## @@ -0,0 +1,133 @@ +// 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.datasource.lance; + +import org.apache.doris.datasource.property.storage.OSSProperties; +import org.apache.doris.datasource.property.storage.StorageProperties; + +import com.google.common.collect.ImmutableMap; +import org.apache.commons.lang3.StringUtils; + +import java.util.HashMap; +import java.util.List; +import java.util.Map; + +/** Alibaba Cloud OSS storage, which Lance reaches through its OpenDAL OSS provider. */ +final class LanceOssStorageProvider implements LanceStorageProvider { + + static final LanceOssStorageProvider INSTANCE = new LanceOssStorageProvider(); + + private static final String ENDPOINT = "oss_endpoint"; + private static final String ACCESS_KEY_ID = "oss_access_key_id"; + private static final String SECRET_ACCESS_KEY = "oss_secret_access_key"; + private static final String REGION = "oss_region"; + private static final String SECURITY_TOKEN = "oss_security_token"; + private static final String ADDRESSING_STYLE = "addressing_style"; + private static final String ALLOW_ANONYMOUS = "allow_anonymous"; + + /** + * Lance exposes the {@code oss_*} names as its public storage-option vocabulary and normalizes + * them to OpenDAL's field names before constructing the operator. Both spellings are accepted, + * so collapse only these known pairs before merging static and namespace-vended options. + */ + private static final Map<String, String> PUBLIC_BY_ALIAS = ImmutableMap.<String, String>builder() + .put("endpoint", ENDPOINT) + .put(ENDPOINT, ENDPOINT) + .put("access_key_id", ACCESS_KEY_ID) + .put(ACCESS_KEY_ID, ACCESS_KEY_ID) + .put("access_key_secret", SECRET_ACCESS_KEY) + .put(SECRET_ACCESS_KEY, SECRET_ACCESS_KEY) + .put("region", REGION) + .put(REGION, REGION) + .put("security_token", SECURITY_TOKEN) + .put(SECURITY_TOKEN, SECURITY_TOKEN) + .build(); + + private LanceOssStorageProvider() { + } + + @Override + public Map<String, String> fromDorisProperties(List<StorageProperties> storageProperties) { + Map<String, String> result = new HashMap<>(); + OSSProperties properties = selectOss(storageProperties); + if (properties == null) { + return result; + } + putIfNotEmpty(result, ENDPOINT, properties.getEndpoint()); + putIfNotEmpty(result, ACCESS_KEY_ID, properties.getAccessKey()); + putIfNotEmpty(result, SECRET_ACCESS_KEY, properties.getSecretKey()); + putIfNotEmpty(result, REGION, properties.getRegion()); + putIfNotEmpty(result, SECURITY_TOKEN, properties.getSessionToken()); + + // Doris reads a blank key pair as a request for anonymous access. Lance forwards options it + // does not recognize straight to OpenDAL, whose OSS service only skips request signing when + // allow_anonymous is set; without it the open fails in credential loading instead of + // issuing the unsigned request Doris asked for. Decide from what was actually emitted above + // rather than re-testing the properties, so a credential this class considers present can + // never be paired with a claim that there is none. + if (!result.containsKey(ACCESS_KEY_ID) && !result.containsKey(SECRET_ACCESS_KEY)) { + result.put(ALLOW_ANONYMOUS, "true"); + } + + // Lance snapshots the host's OSS_*/AWS_*/ALIBABA_CLOUD_* environment into the same config + // map before storage options are applied, so state both addressing styles explicitly the + // way the S3 provider does. Leaving the default implicit would let an exported + // OSS_ADDRESSING_STYLE outrank an explicit oss.use_path_style=false. + String usePathStyle = properties.getUsePathStyle(); + if (StringUtils.isNotEmpty(usePathStyle)) { + result.put(ADDRESSING_STYLE, Boolean.parseBoolean(usePathStyle) ? "path" : "virtual"); + } + return result; + } + + @Override + public Map<String, String> normalizeVended(Map<String, String> vendedOptions) { + Map<String, String> result = new HashMap<>(); + if (vendedOptions == null) { + return result; + } + vendedOptions.forEach((key, value) -> { + String publicKey = PUBLIC_BY_ALIAS.getOrDefault(key, key); Review Comment: [P1] Normalize aliases inside base scopes This lookup only rewrites whole option keys, so a vended `base_1.endpoint` remains canonical while the new catalog value is `oss_endpoint`. FE Lance resolves base 1 first, producing both `endpoint=<vended>` and `oss_endpoint=<static>`; its later OSS alias normalization removes `oss_endpoint` and reinserts it as `endpoint`, silently overwriting the base-specific value. The same ordering defeats scoped canonical access key, secret, region, and token aliases. Before this patch OSS contributed no static map, so the scoped value was preserved. Please normalize supported suffixes inside a valid `base_<id>.` prefix (and reject conflicts per scope), then cover a static catalog plus a registered base whose endpoint/credentials differ. ########## fe/fe-core/src/main/java/org/apache/doris/datasource/property/metastore/LanceFileSystemMetastoreProperties.java: ########## @@ -102,9 +105,45 @@ private static void validateWarehouse(String warehouse) { return; } String scheme = uri.getScheme().toLowerCase(Locale.ROOT); - if (!"file".equals(scheme) && !"s3".equals(scheme)) { + if (!"file".equals(scheme) && !"s3".equals(scheme) && !"oss".equals(scheme)) { throw new IllegalArgumentException("Unsupported Lance filesystem warehouse scheme '" + scheme - + "'; first phase supports local/file and s3"); + + "'; supported schemes are local/file, s3, and oss"); } + // An object-store root names its bucket in the authority. Lance reads that authority as the + // bucket and fails deep inside the store when it is absent, so reject the no-authority form + // here where the message can still name the property. + if (("s3".equals(scheme) || "oss".equals(scheme)) && StringUtils.isBlank(uri.getAuthority())) { + throw new IllegalArgumentException( + "Lance " + scheme + " warehouse must name a bucket, as in " + scheme + + "://bucket/path, but was: " + warehouse); + } + } + + /** + * Doris accepts an OSS URL that spells out the endpoint in its authority and normalizes it to + * the bare bucket. Lance takes the authority as the bucket verbatim, so a warehouse left in the + * qualified form would address {@code bucket.oss-<region>.aliyuncs.com.<endpoint>}. Apply the + * same normalization Doris applies elsewhere before the root reaches the namespace. + */ + private static String normalizeWarehouse(String warehouse) { + if (StringUtils.isBlank(warehouse)) { + return warehouse; + } + URI uri; + try { + uri = URI.create(warehouse); + } catch (IllegalArgumentException e) { + return warehouse; + } + if (uri.getScheme() == null || !"oss".equals(uri.getScheme().toLowerCase(Locale.ROOT))) { + return warehouse; + } + // OSS-HDFS is the one oss:// form whose qualified authority is the required spelling - + // OSSHdfsProperties validates such a URL without ever rewriting it - so leave it alone. + String authority = uri.getAuthority(); + if (authority != null && authority.toLowerCase(Locale.ROOT).contains(OSS_HDFS_MARKER)) { + return warehouse; Review Comment: [P1] Do not accept OSS-HDFS through the Lance OSS provider Preserving this authority makes the catalog look supported, but Doris selects `OSSHdfsProperties` for it while `LanceOssStorageProvider` accepts only `OSSProperties`, so the namespace receives no OSS endpoint or credentials. Both pinned Lance stacks then route the `oss://` root to OpenDAL OSS, require an endpoint, and interpret the full `bkt.<region>.oss-dls.aliyuncs.com` authority as the bucket; the Jindo/HDFS map is never passed to either dataset opener. The new test only checks string preservation, so a real catalog still cannot initialize or scan. Please reject OSS-HDFS roots until a Lance-compatible provider exists, or implement that provider and cover an actual namespace open plus BE scan. -- 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]
