github-actions[bot] commented on code in PR #67157: URL: https://github.com/apache/doris/pull/67157#discussion_r3877539380
########## fe/fe-core/src/main/java/org/apache/doris/datasource/lance/LanceOssStorageProvider.java: ########## @@ -0,0 +1,111 @@ +// 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 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"; + + /** + * 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()); Review Comment: [P1] Preserve Doris's anonymous OSS mode When both keys are blank, `OSSProperties` explicitly selects anonymous access, and the new test labels this case anonymous. This adapter merely omits the credentials, but the pinned BE OpenDAL 0.56 skips signing only when `allow_anonymous` is set (FE OpenDAL 0.57 accepts the same compatibility flag). With no ambient Alibaba credential, both the directory namespace and dataset open therefore fail in credential loading instead of issuing an unsigned request. Please emit `allow_anonymous=true` for this Doris state and cover an actual provider open/read, not only the absence of credential keys. ########## fe/fe-core/src/main/java/org/apache/doris/datasource/lance/LanceExternalCatalog.java: ########## @@ -78,7 +78,9 @@ public class LanceExternalCatalog extends ExternalCatalog { private static final long ALLOCATOR_LIMIT = 256L * 1024 * 1024; private static final int MAX_PROVIDER_MESSAGE_BYTES = 1024; private static final String[] RUNTIME_SENSITIVE_OPTION_KEYS = { - "aws_access_key_id", "aws_secret_access_key", "aws_session_token" + "aws_access_key_id", "aws_secret_access_key", "aws_session_token", + "oss_access_key_id", "oss_secret_access_key", "oss_security_token", Review Comment: [P1] Apply OSS redaction to every provider failure path These new values are consulted only by the overload used from `indexMetadataLoadFailure`. Catalog initialization, the CREATE connection test, and latest/version/vector metadata failures still use the REST-only sanitizer, and `safeCause` returns the original throwable when REST auth is absent, so an OSS provider message/cause can expose the credentials this change intends to redact. Even the index path misses Lance's supported `base_<id>.oss_secret_access_key` and token keys because it performs exact lookups. Please route all provider-facing failures through the runtime-option sanitizer, always replace the cause, and collect sensitive values from supported base-scoped key suffixes as well. ########## fe/fe-core/src/main/java/org/apache/doris/datasource/lance/LanceOssStorageProvider.java: ########## @@ -0,0 +1,111 @@ +// 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 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"; + + /** + * 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()); Review Comment: [P1] Forward the configured OSS addressing style `OSSProperties` supports and preserves `oss.use_path_style=true`, but this adapter drops it. Both pinned OpenDAL OSS implementations default to virtual-host addressing and require `addressing_style=path` to honor that Doris setting, so an endpoint configured for path-style access is contacted as `bucket.endpoint` and all metadata/scans fail. Please translate the explicit true value to `addressing_style=path` (leaving the default virtual mode otherwise) and add it to the mapping test. ########## fe/fe-core/src/main/java/org/apache/doris/datasource/property/metastore/LanceFileSystemMetastoreProperties.java: ########## @@ -102,9 +102,9 @@ 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)) { Review Comment: [P1] Normalize and validate the OSS warehouse before accepting it Doris's OSS URI contract accepts `oss://bucket.oss-<region>.aliyuncs.com/path` and normalizes it to `oss://bucket/path`, but this new branch stores the qualified URI unchanged. Lance then treats the full host as the bucket and OpenDAL builds `bucket.oss-<region>.aliyuncs.com.<configured-endpoint>`, so this supported Doris form cannot initialize the directory namespace. The same scheme-only check also accepts no-authority forms such as `oss:/path`, which Lance rejects later. Please run the warehouse through the selected OSS URI normalization (and require a bucket authority) before passing it as the namespace root. -- 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]
