sunchao commented on code in PR #6023:
URL: https://github.com/apache/datafusion-comet/pull/6023#discussion_r4066755718
##########
spark/src/main/java/org/apache/comet/cloud/s3/CometS3CredentialDispatcher.java:
##########
@@ -74,19 +79,43 @@ public static long ensureInitialized(
catalogProperties == null
? Collections.emptyMap()
: Collections.unmodifiableMap(new HashMap<>(catalogProperties));
+ // Key on a digest of the properties, not the values themselves: the
KEY_TO_HANDLE map is
+ // static and lives for the JVM lifetime, and the property bag may carry
secrets (vended
+ // credentials, static keys). The full map is still handed to initialize()
below; only the
+ // long-lived cache key is reduced to a digest. A distinct config still
yields a distinct key.
InstanceKey key =
- new InstanceKey(providerClassName, dispatchKey == null ? "" :
dispatchKey, snapshot);
+ new InstanceKey(
+ providerClassName, dispatchKey == null ? "" : dispatchKey,
digestOf(snapshot));
return KEY_TO_HANDLE.computeIfAbsent(
key,
k -> {
- CometS3CredentialProvider provider =
instantiate(k.providerClassName);
- provider.initialize(k.catalogProperties);
+ CometS3CredentialProvider provider = instantiate(providerClassName);
+ provider.initialize(snapshot);
long handle = HANDLE_SEQ.getAndIncrement();
INSTANCES.put(handle, new RegisteredProvider(provider, k));
return handle;
});
}
+ /** Stable SHA-256 digest of the property bag, so secret values are not
retained in the key. */
+ private static String digestOf(Map<String, String> props) {
+ MessageDigest md;
+ try {
+ md = MessageDigest.getInstance("SHA-256");
+ } catch (NoSuchAlgorithmException e) {
+ throw new IllegalStateException("SHA-256 not available", e);
+ }
+ // Sort by key for order-independence; NUL separators avoid key/value
boundary ambiguity.
+ for (Map.Entry<String, String> e : new TreeMap<>(props).entrySet()) {
+ md.update(e.getKey().getBytes(StandardCharsets.UTF_8));
+ md.update((byte) 0);
+ String v = e.getValue();
+ md.update(v == null ? new byte[] {1} :
v.getBytes(StandardCharsets.UTF_8));
+ md.update((byte) 0);
Review Comment:
### Correctness
[P2] Preserve property boundaries before hashing the cache key
Could the digest input use length-prefixed fields (and an explicit null
tag), with a regression test for embedded U+0000? If an arbitrary vendor
property value contains NUL, the current separators are ambiguous:
`{"fs.s3a.a":"x\u0000fs.s3a.identity\u0000TENANT-B"}` and
`{"fs.s3a.a":"x","fs.s3a.identity":"TENANT-B"}` produce identical input bytes.
With the same provider class and dispatch key, I reproduced both maps receiving
the same handle and the second request returning the first map's synthetic
identity. Reversing their order reverses which identity wins. The previous
map-based key keeps them separate. The unfiltered Iceberg FileIO bag reaches
this method through protobuf strings and JNI's modified UTF-8 conversion
without rejecting U+0000. This is conditional on such vendor values, and comes
from the serialization before SHA-256. Ordinary key changes still separate
correctly.
##########
spark/src/main/java/org/apache/comet/cloud/s3/AdapterSupport.java:
##########
@@ -0,0 +1,151 @@
+/*
+ * 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.comet.cloud.s3;
+
+import java.lang.reflect.Constructor;
+import java.lang.reflect.Method;
+import java.lang.reflect.Modifier;
+import java.net.URI;
+import java.util.Map;
+
+import org.apache.hadoop.conf.Configuration;
+
+/** Config and reflection helpers shared by the built-in S3 credential
provider adapters. */
+final class AdapterSupport {
+
+ private AdapterSupport() {}
+
+ /**
+ * Rebuilds a Hadoop {@link Configuration} from the forwarded {@code
fs.s3a.*} map. The adapters
+ * run on the executor without a live {@code S3AFileSystem}, so keys are
copied onto a fresh
+ * Configuration (which still loads core-site defaults).
+ */
+ static Configuration toConfiguration(Map<String, String> props) {
+ Configuration conf = new Configuration();
+ for (Map.Entry<String, String> entry : props.entrySet()) {
+ if (entry.getValue() != null) {
+ conf.set(entry.getKey(), entry.getValue());
+ }
+ }
+ return conf;
+ }
+
+ /**
+ * Per-bucket then global lookup, mirroring Comet's native {@code fs.s3a}
resolution: {@code
+ * fs.s3a.bucket.<bucket>.<property>} wins over {@code fs.s3a.<property>}.
Returns null if neither
+ * is set (after trimming).
+ */
+ static String lookup(Map<String, String> props, String bucket, String
property) {
+ String perBucket = props.get("fs.s3a.bucket." + bucket + "." + property);
+ if (perBucket != null && !perBucket.trim().isEmpty()) {
+ return perBucket.trim();
+ }
+ String global = props.get("fs.s3a." + property);
+ if (global != null && !global.trim().isEmpty()) {
+ return global.trim();
+ }
+ return null;
+ }
+
+ /** Returns the public static no-arg method {@code name} on {@code clazz},
or null if absent. */
+ private static Method staticMethod(Class<?> clazz, String name) {
+ try {
+ Method m = clazz.getMethod(name);
+ return Modifier.isStatic(m.getModifiers()) ? m : null;
+ } catch (NoSuchMethodException e) {
+ return null;
+ }
+ }
+
+ /**
+ * Instantiates a credential-provider delegate, trying the same ordered
conventions for both the
+ * v1 and v2 adapters so their {@code @Public} contract is identical: the
Hadoop-style {@code
+ * (URI, Configuration)} and {@code (Configuration)} constructors first
(matching {@code
+ * S3AUtils.getInstanceFromReflection}), then the SDK static factories
{@code create()} / {@code
+ * builder().build()} / {@code getInstance()}, then a public no-arg
constructor.
+ *
+ * <p>Factory return types must be assignable to {@code targetType} (as
Hadoop's {@code
+ * getFactoryMethod} requires), so an unrelated {@code static String
create()} is skipped rather
+ * than invoked and failing later with a {@code ClassCastException}. Returns
an untyped instance;
+ * the caller casts to its SDK provider interface.
+ */
+ static Object instantiateDelegate(
+ Class<?> targetType, Class<?> clazz, URI uri, Configuration conf) throws
Exception {
+ Constructor<?> uriConf = constructor(clazz, URI.class,
Configuration.class);
+ if (uriConf != null) {
+ return uriConf.newInstance(uri, conf);
+ }
+ Constructor<?> confOnly = constructor(clazz, Configuration.class);
+ if (confOnly != null) {
+ return confOnly.newInstance(conf);
+ }
+ Method create = factoryMethod(clazz, "create", targetType);
+ if (create != null) {
+ return create.invoke(null);
+ }
+ Method builder = staticMethod(clazz, "builder");
+ if (builder != null) {
+ // Resolve build() off builder()'s declared (public) return type, not
the runtime object's
+ // class, which may be a non-public implementation.
+ Method build = builder.getReturnType().getMethod("build");
+ if (targetType.isAssignableFrom(build.getReturnType())) {
Review Comment:
### Correctness
[P2] Accept valid builders whose build return type is erased
Could this account for an inherited generic `build()` return type before
rejecting the builder? A public `Builder extends SdkBuilder<Builder,
MyProvider>` can inherit `T build()` without redeclaring it. Reflection then
reports `Object`, although `builder().build()` returns a valid provider. A
synthetic builder-only v2 provider using that public AWS interface succeeds
through the adapter at `333d8fd1`, but at this head the new check skips it and
falls through to a missing no-arg constructor (`NoSuchMethodException`). The
existing builder fixture redeclares a concrete return type, so it does not
cover this case. This affects that generic builder shape, not providers with a
concrete covariant `build()` declaration or an earlier valid factory.
--
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]
