github-actions[bot] commented on code in PR #66890:
URL: https://github.com/apache/doris/pull/66890#discussion_r3803809679
##########
fe/fe-connector/fe-connector-hms/src/main/java/org/apache/doris/connector/hms/HmsConfHelper.java:
##########
@@ -58,21 +69,92 @@ public static HiveConf createHiveConf(Map<String, String>
properties) {
// (fixes SecurityUtil.<clinit>) but cannot fix this conf-cached CL.
Pinning here keeps the whole
// hive-metastore class graph in one loader.
hiveConf.setClassLoader(HmsConfHelper.class.getClassLoader());
+ addConfResources(hiveConf, confResources);
for (Map.Entry<String, String> entry : properties.entrySet()) {
+ // A blank username was ignored by the legacy copy-if-present
path; preserving a resource value (or
+ // the "hadoop" default) avoids createRemoteUser("") failing
before the first HMS RPC.
+ if ("hadoop.username".equals(entry.getKey()) &&
isBlank(entry.getValue())) {
+ continue;
+ }
hiveConf.set(entry.getKey(), entry.getValue());
}
// A kerberized HMS requires SASL transport on the metastore Thrift
connection. The legacy fe-core
// HMSBaseProperties.initHadoopAuthenticator auto-enabled
hive.metastore.sasl.enabled whenever the
// metastore/hadoop auth was kerberos; preserve that here so a catalog
that only declares kerberos auth
// (without an explicit hive.metastore.sasl.enabled) still negotiates
SASL, instead of opening a plain
// TSocket that a kerberized metastore drops with TTransportException.
- if
("kerberos".equalsIgnoreCase(properties.get("hadoop.security.authentication"))
- ||
"kerberos".equalsIgnoreCase(properties.get("hive.metastore.authentication.type")))
{
+ String hmsAuthType =
properties.get("hive.metastore.authentication.type");
+ boolean explicitSimple = "simple".equalsIgnoreCase(hmsAuthType);
Review Comment:
**[P1] Make explicit SIMPLE override resource-provided SASL**
The resource base is loaded before this block, but `explicitSimple` only
prevents us from setting SASL to true; it never clears an inherited
`hive.metastore.sasl.enabled=true`. A catalog that points at a simple HMS while
reusing a cluster hive-site.xml can therefore still enter HiveMetaStoreClient's
Kerberos/SASL transport instead of the configured SIMPLE `set_ugi` path. The
Iceberg/Paimon assembler has the same gap because `toHiveConfOverrides` emits
`true` for Kerberos but no `false` for explicit SIMPLE. This is distinct from
the earlier storage-Kerberos issue: the stale value now comes from the
lower-precedence resource. Please emit an explicit false override after
resource layering (for both builders) and cover a resource-backed
SASL-true/SIMPLE catalog.
##########
fe/fe-connector/fe-connector-paimon/src/main/java/org/apache/doris/connector/paimon/PaimonConnector.java:
##########
@@ -489,14 +502,40 @@ private Catalog createCatalogFromContext(CatalogContext
catalogContext, String f
ClassLoader previous = Thread.currentThread().getContextClassLoader();
try {
Thread.currentThread().setContextClassLoader(getClass().getClassLoader());
- return context.executeAuthenticated(() ->
CatalogFactory.createCatalog(catalogContext));
+ return context.executeAuthenticated(() -> {
+ // Paimon's CachedClientPool eagerly opens a SIMPLE client in
the HiveCatalog constructor, so
+ // construction must cross the HMS boundary too. FileIO only
stores its configuration here and
+ // still creates filesystems lazily under the connector's
separate storage authenticator.
+ Catalog catalog = hmsAuth == null
+ ? CatalogFactory.createCatalog(catalogContext)
+ : hmsAuth.doAs(() ->
CatalogFactory.createCatalog(catalogContext));
Review Comment:
**[P1] Keep Paimon's eager FileIO work under the storage identity**
This inner `doAs` wraps the whole SDK factory, not just eager HMS client
creation. In Paimon 1.3.1, `HiveCatalog.createHiveCatalog` calls
`FileIO.get(...)` and `fileIO.checkOrMkdirs(warehouse)` before it constructs
the catalog/client pool, so a SIMPLE HMS user nested inside a Kerberos storage
`doAs` performs that warehouse access as the HMS user. Mixed-identity catalogs
can fail construction or create/access the warehouse under the wrong owner.
Please keep factory FileIO work under the storage boundary and scope HMS
authentication to client-pool creation/RPCs; the regression should construct a
real mixed-identity catalog and observe the UGI used by the warehouse check.
##########
fe/fe-connector/fe-connector-hive/src/main/java/org/apache/doris/connector/hive/HiveConnector.java:
##########
@@ -591,14 +592,19 @@ private HmsClient createClient() {
// getHmsClientProperties(), not the raw map: the metastore URI must
reach HiveConf under its canonical
// key even when the catalog spells it with the "uri" short form.
- HmsClientConfig config = new
HmsClientConfig(props.getHmsClientProperties(), poolSize);
+ AbstractHmsMetaStoreProperties hms = (AbstractHmsMetaStoreProperties)
MetaStoreProviders.bindForType(
+ HmsClientConfig.METASTORE_TYPE_HMS,
props.getHmsClientProperties(), Collections.emptyMap());
+ HmsClientConfig config = new HmsClientConfig(hms.getConfResources(),
+ HmsConfHelper.mergeCatalogProperties(
+ props.getHmsClientProperties(),
hms.toHiveConfOverrides("")),
Review Comment:
**[P1] Preserve the FE-configured HMS socket timeout**
This new client assembly passes a blank default to `toHiveConfOverrides`,
which makes the shared parser write an explicit 10-second
`hive.metastore.client.socket.timeout`. `DefaultConnectorContext` already
forwards the deployment's `hive_metastore_client_timeout_second` specifically
for this SPI call, and Iceberg/Paimon consume it; Hive and the parallel Hudi
call instead ignore (for example) an operator's 60-second setting and time out
longer HMS operations at 10 seconds. Please resolve the connector/environment
value here and in Hudi before building the actual client config, and cover a
non-default environment value with no per-catalog timeout.
##########
fe/fe-connector/fe-connector-iceberg/src/main/java/org/apache/doris/connector/iceberg/IcebergConnector.java:
##########
@@ -1248,45 +1277,42 @@ private HadoopAuthenticator pluginAuthenticator() {
}
/**
- * Resolves the plugin-side Kerberos authenticator for the catalog, or
{@code null} for a non-Kerberos
- * catalog. Two Kerberos sources are covered, in precedence order:
- * <ol>
- * <li><b>Storage</b> Kerberos — the raw {@code
hadoop.security.authentication=kerberos} passthrough
- * (HDFS / data-lake login), built from the storage Hadoop
configuration. Unchanged prior behavior;
- * when storage is Kerberos this single login also carries the HMS
metastore RPC (same UGI).</li>
- * <li><b>HMS-metastore</b> Kerberos with non-Kerberos storage — a
secured Hive Metastore whose data
- * storage is simple (e.g. a Kerberized HMS over S3). Legacy fe-core
served this from the fe-core
- * {@code IcebergHMSMetaStoreProperties} HMS authenticator
(delivered via {@code DefaultConnectorContext});
- * once the fe-core iceberg property cluster is deleted the
connector must own it. This mirrors
- * {@code HMSBaseProperties.initHadoopAuthenticator}: the HMS client
principal/keytab facts
- * ({@link HmsMetaStoreProperties#kerberos()}) feed a {@link
KerberosAuthenticationConfig}, so the
- * {@code doAs} logs in the same client identity fe-core used. The
HMS <em>service</em> principal /
- * SASL settings ride the catalog's own HiveConf ({@code
hms.toHiveConfOverrides}), not the login.</li>
- * </ol>
- * Package-visible + static for direct unit testing (mirrors the {@code
metaFailureMessage} helpers).
+ * Resolves only the storage-side Kerberos authenticator used by FileIO.
HMS authentication is intentionally
+ * resolved separately by {@link #buildHmsAuthenticator} and applied at
the client-pool boundary.
*/
static HadoopAuthenticator buildPluginAuthenticator(Map<String, String>
properties,
Map<String, String> storageHadoopConfig) {
if
("kerberos".equalsIgnoreCase(properties.get(HADOOP_SECURITY_AUTHENTICATION))) {
return HadoopAuthenticator.getHadoopAuthenticator(
IcebergCatalogFactory.buildHadoopConfiguration(properties,
storageHadoopConfig));
}
- if
(IcebergCatalogProperties.TYPE_HMS.equals(IcebergCatalogProperties.of(properties).getFlavor()))
{
- HmsMetaStoreProperties hms = (HmsMetaStoreProperties)
MetaStoreProviders.bindForType(
- IcebergCatalogProperties.TYPE_HMS, properties,
storageHadoopConfig);
- Optional<KerberosAuthSpec> spec = hms.kerberos();
- if (spec.isPresent() && spec.get().hasCredentials()) {
- Configuration conf =
-
IcebergCatalogFactory.buildHadoopConfiguration(properties, storageHadoopConfig);
- conf.set("hadoop.security.authentication", "kerberos");
- conf.set("hive.metastore.sasl.enabled", "true");
- return HadoopAuthenticator.getHadoopAuthenticator(
- new
KerberosAuthenticationConfig(spec.get().getPrincipal(), spec.get().getKeytab(),
conf));
- }
- }
return null;
Review Comment:
**[P1] Compute Iceberg's default HMS owner under the HMS identity**
This now returns null for an HMS-Kerberos/simple-storage catalog, so the
outer plugin context runs the DDL under the FE process identity and only the
later client-pool callback enters the HMS `doAs`. Iceberg 1.10.1 computes a new
table's default owner with `HiveHadoopUtil.currentUser()` in
`HiveTableOperations.doCommit` before `persistTable` calls `metaClients.run`;
the create RPC is authenticated, but the persisted owner is the FE process user
rather than the configured HMS principal. Before this change, the HMS fallback
covered that computation. Please derive/set `HiveCatalog.HMS_TABLE_OWNER` under
the HMS authenticator without wrapping FileIO, and add a mixed-identity
create-table test that checks both the persisted owner and storage UGI.
##########
fe/fe-connector/fe-connector-paimon/src/main/java/org/apache/doris/connector/paimon/PaimonHmsClientPool.java:
##########
@@ -0,0 +1,119 @@
+// 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.connector.paimon;
+
+import org.apache.doris.kerberos.HadoopAuthenticator;
+
+import org.apache.hadoop.hive.metastore.IMetaStoreClient;
+import org.apache.paimon.catalog.Catalog;
+import org.apache.paimon.catalog.DelegateCatalog;
+import org.apache.paimon.client.ClientPool;
+import org.apache.paimon.hive.HiveCatalog;
+import org.apache.thrift.TException;
+
+import java.io.IOException;
+import java.io.UncheckedIOException;
+import java.lang.reflect.Field;
+import java.security.PrivilegedAction;
+
+/** Applies the HMS identity at Paimon's metastore client acquisition and RPC
boundary. */
+final class PaimonHmsClientPool implements ClientPool<IMetaStoreClient,
TException> {
+
+ private final ClientPool<IMetaStoreClient, TException> delegate;
+ private final HadoopAuthenticator authenticator;
+
+ private PaimonHmsClientPool(ClientPool<IMetaStoreClient, TException>
delegate,
+ HadoopAuthenticator authenticator) {
+ this.delegate = delegate;
+ this.authenticator = authenticator;
+ }
+
+ static ClientPool<IMetaStoreClient, TException> wrap(
+ ClientPool<IMetaStoreClient, TException> delegate,
HadoopAuthenticator authenticator) {
+ return new PaimonHmsClientPool(delegate, authenticator);
+ }
+
+ static Catalog install(Catalog catalog, HadoopAuthenticator authenticator)
{
+ if (authenticator == null) {
+ return catalog;
+ }
+ Catalog root = DelegateCatalog.rootCatalog(catalog);
+ if (!(root instanceof HiveCatalog)) {
+ throw new IllegalStateException("Expected a Paimon HiveCatalog for
HMS authentication");
+ }
+ try {
+ Field clients = HiveCatalog.class.getDeclaredField("clients");
+ clients.setAccessible(true);
+ @SuppressWarnings("unchecked")
+ ClientPool<IMetaStoreClient, TException> delegate =
+ (ClientPool<IMetaStoreClient, TException>)
clients.get(root);
+ // Paimon exposes no client-pool injection seam; replacing only
this field prevents the HMS user
+ // from leaking into FileIO while covering both cached-pool client
creation and every metastore RPC.
+ clients.set(root, wrap(delegate, authenticator));
Review Comment:
**[P1] Apply the HMS boundary to loader-created Paimon catalogs too**
Replacing `clients` on this one root object does not cover the
`HiveCatalogLoader` retained by tables loaded from it. Paimon 1.3.1's loader
creates a fresh `HiveCatalog` with an unwrapped `CachedClientPool`; ordinary
Doris `beginQuerySnapshot -> table.latestSnapshot()` can reuse the handle's
transient table and reach that loader outside the table-resolution auth scope.
A user-supplied `ugi`/`user_name` pool key makes the split immediate; with the
canonical config keys, access eviction lets the loader recreate its eager
SIMPLE client under the caller rather than the HMS UGI. Please make the
boundary part of every catalog/pool created by the retained loader, and test a
loaded table across pool recreation rather than only `wrap(delegate).run(...)`.
##########
fe/fe-connector/fe-connector-iceberg/src/main/java/org/apache/doris/connector/iceberg/IcebergConnector.java:
##########
@@ -969,8 +974,32 @@ private Catalog createCatalog() {
LOG.info("Creating Iceberg catalog '{}' flavor='{}' impl='{}'",
catalogName, flavor,
catalogOptions.get(CatalogProperties.CATALOG_IMPL));
- return buildCatalogAuthenticated(flavor,
- () -> CatalogUtil.buildIcebergCatalog(catalogName,
catalogOptions, conf));
+ return buildCatalogAuthenticated(flavor, () -> {
+ if (!IcebergCatalogProperties.TYPE_HMS.equals(flavor)) {
+ return CatalogUtil.buildIcebergCatalog(catalogName,
catalogOptions, conf);
+ }
+ HadoopAuthenticator hmsAuth = buildHmsAuthenticator(properties,
storageHadoopConfig);
+ Catalog catalog = CatalogUtil.buildIcebergCatalog(catalogName,
catalogOptions, conf);
+ return IcebergHmsClientPool.install(catalog, hmsAuth);
+ });
+ }
+
+ static String appendHmsCacheKeys(String existing) {
+ String keys = appendCacheKey(existing, "conf:hadoop.username");
+ keys = appendCacheKey(keys, "conf:hive.metastore.client.principal");
+ return appendCacheKey(keys, "conf:hadoop.kerberos.principal");
+ }
+
+ static String appendCacheKey(String existing, String required) {
+ if (StringUtils.isBlank(existing)) {
+ return required;
+ }
+ for (String element : existing.split(",")) {
+ if (required.equalsIgnoreCase(element.trim())) {
Review Comment:
**[P1] Do not deduplicate case-sensitive conf keys case-insensitively**
The SDKs treat only the `conf:` prefix case-insensitively; they preserve the
suffix and call Hadoop `Configuration.get` with it. If a user already supplied
`conf:HADOOP.USERNAME`, this comparison suppresses the required
`conf:hadoop.username`, but extraction reads the uppercase property as null.
Same-URI catalogs with different lowercase `hadoop.username` values can then
derive the same JVM-static pool key and reuse the first catalog's connected
client. The Paimon copy has the same bug. Please require/replace the exact
canonical suffix (while handling the prefix as supported) and test a mis-cased
existing key with two different users.
--
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]
