924060929 commented on code in PR #66315: URL: https://github.com/apache/doris/pull/66315#discussion_r3749751916
########## fe/fe-filesystem/fe-filesystem-api/src/main/java/org/apache/doris/filesystem/properties/FsCacheKeys.java: ########## @@ -0,0 +1,250 @@ +// 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.properties; + +import java.nio.charset.StandardCharsets; +import java.security.MessageDigest; +import java.security.NoSuchAlgorithmException; +import java.util.Collection; +import java.util.LinkedHashSet; +import java.util.Locale; +import java.util.Map; +import java.util.Set; +import java.util.TreeMap; +import java.util.function.BiConsumer; + +/** + * Credential fingerprints for the Hadoop {@code FileSystem.CACHE} key. + * + * <p>Hadoop keys cached FileSystem instances by {@code (scheme, authority, UGI)} only, so two + * catalogs/TVFs reaching the same bucket or namenode with different credentials would share one + * instance. Doris therefore used to force {@code fs.<schema>.impl.disable.cache=true} everywhere, + * which makes every access (e.g. every JNI scanner split) build a brand-new FileSystem. + * + * <p>Instead, both FE and BE load the Doris-patched {@code org.apache.hadoop.fs.FileSystem} + * shipped in {@code hadoop-deps.jar}, whose cache key carries one extra dimension read from + * {@link #FS_CACHE_KEY_PROPERTY}. Vanilla (unpatched) Hadoop ignores the property, and an + * absent/empty value keeps the vanilla cache-key semantics. + * + * <p><b>Why the key is per scheme.</b> The property is written as + * {@code doris.fs.cache.key.<scheme>}, not as one shared key. A single catalog routinely holds + * more than one storage (an object store plus HDFS), and every consumer merges their property maps + * with {@code putAll} into one map / one {@code Configuration}. Under a single shared name that + * merge is last-writer-wins: every scheme would end up tagged with one arbitrary storage's + * fingerprint, which is both unstable (map iteration order) and unsafe — two catalogs differing + * only in their object-store credentials but sharing an HDFS definition could collapse onto the + * same key. Distinct per-scheme names make the merge lossless, so no merge site has to know this + * mechanism exists. + */ +public final class FsCacheKeys { + + /** + * Reserved Hadoop configuration property carrying a credential fingerprint; mixed into the + * patched {@code FileSystem.CACHE} key. Written per scheme as + * {@code doris.fs.cache.key.<scheme>} (see {@link #fsCacheKeyProperty}); the patched + * FileSystem falls back to this scheme-less name when no per-scheme entry is present. + */ + public static final String FS_CACHE_KEY_PROPERTY = "doris.fs.cache.key"; + + private static final int FINGERPRINT_LENGTH = 32; + + /** + * Raw property prefixes that reach the effective Hadoop {@code Configuration} verbatim, on top + * of the derived map this class fingerprints, and which no provider declares as a + * {@code @ConnectorProperty} alias (so {@link StorageProperties#matchedProperties()} cannot see + * them). Two sites overlay them <em>after</em> the storage's own map, so they win: + * {@code IcebergCatalogFactory.buildHadoopConfiguration} / {@code HudiScanPlanProvider + * .buildHadoopConf} (from the catalog properties) and {@code HdfsProperties + * .extractUserOverriddenHdfsConfig} (into the derived backend map). + * + * <p>Deliberately narrower than those overlay filters: {@code hive.} is overlaid by the Hudi + * site too but configures the metastore client, never a FileSystem, and mixing it in would only + * cost cache entries. + */ + private static final String[] HADOOP_OVERLAY_PREFIXES = {"fs.", "dfs.", "hadoop.", "juicefs."}; + + /** + * Namespace for entries of a <em>derived</em> map mixed into an identity (see + * {@link #derivedIdentityKey}). Keeps a derived key from silently shadowing the raw key of the + * same name when the two disagree — e.g. {@code fs.defaultFS} extracted from {@code uri} versus + * one the user spelled out. + */ + private static final String DERIVED_KEY_NAMESPACE = "@derived."; + + private FsCacheKeys() { + } + + /** The reserved property name carrying the fingerprint for {@code scheme}. */ + public static String fsCacheKeyProperty(String scheme) { + return FS_CACHE_KEY_PROPERTY + "." + scheme.toLowerCase(Locale.ROOT); + } + + /** + * SHA-256 over {@code salt} and the sorted properties, truncated to 32 hex chars. The same + * (salt, properties) pair always yields the same fingerprint — cache hits are preserved across + * queries — while any credential or config change yields a new one. + */ + public static String fingerprintOf(String salt, Map<String, String> props) { + StringBuilder sb = new StringBuilder(salt == null ? "" : salt); + if (props != null) { + new TreeMap<>(props).forEach((k, v) -> sb.append('\n').append(k).append('=').append(v == null ? "" : v)); Review Comment: [P1] Frame each property before hashing instead of concatenating with delimiters. Both property names and values are caller-controlled strings, so the current `\nkey=value` encoding is not injective. For example, with the same common typed properties, these two raw overlays serialize to exactly the same bytes: ``` A: { "fs.ignored": "\nfs.s3a.access.key=AK\nfs.s3a.aws.credentials.provider=org.apache.hadoop.fs.s3a.SimpleAWSCredentialsProvider\nfs.s3a.secret.key=SK" } B: { "fs.ignored": "", "fs.s3a.access.key": "AK", "fs.s3a.aws.credentials.provider": "org.apache.hadoop.fs.s3a.SimpleAWSCredentialsProvider", "fs.s3a.secret.key": "SK" } ``` However, the connector's raw `fs.*` overlay passes A to Hadoop as one ignored multiline value, while B installs explicit S3A credentials. Because their fingerprints are equal, `FileSystem.Cache.Key` can return the same client for these different effective configurations, defeating the credential-isolation guarantee. Please hash length-prefixed key/value bytes (including the salt) or another unambiguous framing, and add a collision regression test for embedded newline/`=` characters. -- 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]
