Copilot commented on code in PR #6790:
URL: https://github.com/apache/hive/pull/6790#discussion_r4341248610
##########
iceberg/iceberg-catalog/src/main/java/org/apache/iceberg/hive/CachedClientPool.java:
##########
@@ -124,6 +150,26 @@ public <R> R run(Action<R, IMetaStoreClient, TException>
action, boolean retry)
return clientPool().run(action, retry);
}
+ private static boolean hasUserScopedKeyElement(String cacheKeysConfig) {
+ if (cacheKeysConfig == null || cacheKeysConfig.isEmpty()) {
+ return false;
+ }
+ for (String element : cacheKeysConfig.split(",", -1)) {
+ String trimmed = element.trim();
+ if (trimmed.isEmpty()) {
+ continue;
+ }
+ if (trimmed.toLowerCase(Locale.ROOT).startsWith(CONF_ELEMENT_PREFIX)) {
+ continue;
+ }
+ KeyElementType type =
KeyElementType.valueOf(trimmed.toUpperCase(Locale.ROOT));
+ if (type == KeyElementType.UGI || type == KeyElementType.USER_NAME) {
+ return true;
+ }
+ }
Review Comment:
`hasUserScopedKeyElement` calls `KeyElementType.valueOf(...)` directly,
which will throw `IllegalArgumentException` for invalid elements. This changes
validation behavior (and likely error type/message) compared to
`extractKey(...)` which appears to own cache-key parsing/validation. Consider
reusing the same parsing/validation path as `extractKey` (e.g., a shared parser
that returns both 'has user-scoped elements' + validated elements) or catching
`IllegalArgumentException` and rethrowing a `ValidationException` consistent
with existing behavior.
##########
standalone-metastore/metastore-rest-catalog/src/main/java/org/apache/iceberg/rest/HMSCatalogFactory.java:
##########
@@ -80,6 +85,16 @@ private Catalog createCatalog() {
// This tells HiveCatalog to use Thrift connection to external HMS
if (!StringUtils.isEmpty(configUri)) {
properties.put(CatalogProperties.URI, configUri);
+ // Remote HMS: isolate metastore client pools per authenticated REST
user so set_ugi matches the caller.
+ if (configuration.get(SERVLET_ID_KEY) == null) {
+ properties.put(CatalogProperties.CLIENT_POOL_CACHE_KEYS,
CLIENT_POOL_USER_CACHE_KEY);
+ }
Review Comment:
Keying the HMS client pool cache by `user_name` can create one pooled client
set per distinct REST user. In environments with many users this can materially
increase the number of open Thrift connections and memory usage. Consider
adding/adjusting safeguards (e.g., documenting recommended eviction interval
sizing for REST deployments, or ensuring the cache is bounded) to reduce the
risk of unbounded per-user pool growth.
##########
iceberg/iceberg-catalog/src/main/java/org/apache/iceberg/hive/TokenAuthHiveClientPool.java:
##########
@@ -0,0 +1,93 @@
+/*
+ * 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.iceberg.hive;
+
+import java.io.IOException;
+import java.security.PrivilegedExceptionAction;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.hive.conf.HiveConf;
+import org.apache.hadoop.hive.metastore.HiveMetaStoreClient;
+import org.apache.hadoop.hive.metastore.IMetaStoreClient;
+import org.apache.hadoop.hive.metastore.conf.MetastoreConf;
+import org.apache.hadoop.hive.metastore.utils.SecurityUtils;
+import org.apache.hadoop.security.UserGroupInformation;
+
+/**
+ * HiveClientPool that authenticates to a remote secure HMS as the current
proxy user using
+ * a metastore delegation token obtained by the catalog service principal.
+ */
+class TokenAuthHiveClientPool extends HiveClientPool {
+
+ static final String DELEGATION_TOKEN_SERVICE =
"DelegationTokenForHiveMetaStoreServer";
+
+ private final HiveConf hiveConf;
+
+ TokenAuthHiveClientPool(int poolSize, Configuration conf) {
+ super(poolSize, conf);
+ this.hiveConf = super.hiveConf();
+ }
+
+ private void setDelegationToken() {
+ IMetaStoreClient tokenClient = null;
+ try {
+ UserGroupInformation currentUgi = SecurityUtils.getUGI();
+ if (currentUgi.getAuthenticationMethod() !=
UserGroupInformation.AuthenticationMethod.PROXY) {
+ return;
+ }
+ hiveConf.unset(MetastoreConf.ConfVars.TOKEN_SIGNATURE.getVarname());
+ UserGroupInformation realUser = currentUgi.getRealUser();
+ if (realUser == null) {
+ realUser = UserGroupInformation.getLoginUser();
+ }
+ final UserGroupInformation tokenFetcher = realUser;
+ tokenClient =
+ tokenFetcher.doAs(
+ (PrivilegedExceptionAction<IMetaStoreClient>)
+ () -> new HiveMetaStoreClient(hiveConf));
+ String proxyUser = currentUgi.getUserName();
+ String delegationTokenStr = tokenClient.getDelegationToken(proxyUser,
proxyUser);
Review Comment:
The renewer passed to `getDelegationToken(owner, renewer)` is set to the
proxy user (`proxyUser`). For long-lived services, this can produce tokens that
the catalog service (or login/real user) cannot renew, leading to token expiry
and failures. Consider using the catalog service/real user as the renewer
(e.g., the `tokenFetcher`'s user name), while keeping the owner as the proxy
user.
##########
iceberg/iceberg-catalog/src/main/java/org/apache/iceberg/hive/TokenAuthHiveClientPool.java:
##########
@@ -0,0 +1,93 @@
+/*
+ * 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.iceberg.hive;
+
+import java.io.IOException;
+import java.security.PrivilegedExceptionAction;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.hive.conf.HiveConf;
+import org.apache.hadoop.hive.metastore.HiveMetaStoreClient;
+import org.apache.hadoop.hive.metastore.IMetaStoreClient;
+import org.apache.hadoop.hive.metastore.conf.MetastoreConf;
+import org.apache.hadoop.hive.metastore.utils.SecurityUtils;
+import org.apache.hadoop.security.UserGroupInformation;
+
+/**
+ * HiveClientPool that authenticates to a remote secure HMS as the current
proxy user using
+ * a metastore delegation token obtained by the catalog service principal.
+ */
+class TokenAuthHiveClientPool extends HiveClientPool {
+
+ static final String DELEGATION_TOKEN_SERVICE =
"DelegationTokenForHiveMetaStoreServer";
+
+ private final HiveConf hiveConf;
+
+ TokenAuthHiveClientPool(int poolSize, Configuration conf) {
+ super(poolSize, conf);
+ this.hiveConf = super.hiveConf();
+ }
+
+ private void setDelegationToken() {
+ IMetaStoreClient tokenClient = null;
+ try {
+ UserGroupInformation currentUgi = SecurityUtils.getUGI();
+ if (currentUgi.getAuthenticationMethod() !=
UserGroupInformation.AuthenticationMethod.PROXY) {
+ return;
+ }
+ hiveConf.unset(MetastoreConf.ConfVars.TOKEN_SIGNATURE.getVarname());
+ UserGroupInformation realUser = currentUgi.getRealUser();
+ if (realUser == null) {
+ realUser = UserGroupInformation.getLoginUser();
+ }
+ final UserGroupInformation tokenFetcher = realUser;
+ tokenClient =
+ tokenFetcher.doAs(
+ (PrivilegedExceptionAction<IMetaStoreClient>)
+ () -> new HiveMetaStoreClient(hiveConf));
+ String proxyUser = currentUgi.getUserName();
+ String delegationTokenStr = tokenClient.getDelegationToken(proxyUser,
proxyUser);
+ SecurityUtils.setTokenStr(currentUgi, delegationTokenStr,
DELEGATION_TOKEN_SERVICE);
+ MetastoreConf.setVar(hiveConf, MetastoreConf.ConfVars.TOKEN_SIGNATURE,
DELEGATION_TOKEN_SERVICE);
Review Comment:
`setDelegationToken()` mutates the shared `hiveConf` (`unset` then
`setVar`). If this pool is used concurrently, or if an exception occurs after
`unset` but before `setVar`, the `TokenAuthHiveClientPool` instance can be left
with `TOKEN_SIGNATURE` cleared (affecting subsequent client creation/reconnect)
and can race with other threads. Consider avoiding mutable shared configuration
(e.g., preserve/restore the previous `TOKEN_SIGNATURE` in a `finally`, and/or
use a per-client/per-thread `HiveConf` copy for token-fetch + client creation)
so failures and concurrency do not corrupt pool-wide configuration.
##########
iceberg/iceberg-catalog/src/test/java/org/apache/iceberg/hive/TestCachedClientPool.java:
##########
@@ -147,6 +153,31 @@ public void testCacheKey() throws Exception {
.hasMessageContaining("Conf key element k1 already specified");
}
+ @Test
+ public void testPerUserClientPoolsWhenUserNameInCacheKeys() throws Exception
{
+ UserGroupInformation current = UserGroupInformation.getCurrentUser();
+ UserGroupInformation foo = UserGroupInformation.createProxyUser("foo",
current);
+ UserGroupInformation bar = UserGroupInformation.createProxyUser("bar",
current);
+ HiveConf hiveConf = HIVE_METASTORE_EXTENSION.hiveConf();
+ Map<String, String> properties =
+ ImmutableMap.of(CatalogProperties.CLIENT_POOL_CACHE_KEYS, "user_name");
+
+ CachedClientPool pool = new CachedClientPool(hiveConf, properties);
+ HiveClientPool fooPool = foo.doAs((PrivilegedAction<HiveClientPool>)
pool::clientPool);
+ HiveClientPool barPool = bar.doAs((PrivilegedAction<HiveClientPool>)
pool::clientPool);
Review Comment:
The explicit cast on the method reference is likely to introduce (or hide)
an unchecked-cast warning. Using an explicit lambda with an inferred return
type (e.g., `() -> pool.clientPool()`) avoids the cast and keeps the test
compilation cleaner.
--
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]
