github-actions[bot] commented on code in PR #63068:
URL: https://github.com/apache/doris/pull/63068#discussion_r3362006845
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/iceberg/IcebergRestExternalCatalog.java:
##########
@@ -17,16 +17,236 @@
package org.apache.doris.datasource.iceberg;
+import org.apache.doris.catalog.InfoSchemaDb;
+import org.apache.doris.catalog.MysqlDb;
+import org.apache.doris.common.util.Util;
import org.apache.doris.datasource.CatalogProperty;
+import org.apache.doris.datasource.ExternalDatabase;
+import org.apache.doris.datasource.ExternalTable;
+import org.apache.doris.datasource.InitCatalogLog;
+import org.apache.doris.datasource.SessionContext;
+import org.apache.doris.datasource.property.metastore.IcebergRestProperties;
+import org.apache.doris.datasource.property.metastore.MetastoreProperties;
+import com.google.common.collect.Lists;
+import org.apache.iceberg.rest.RESTSessionCatalog;
+import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.Logger;
+
+import java.util.Collections;
+import java.util.List;
import java.util.Map;
+import java.util.Optional;
+import java.util.stream.Collectors;
+
+public class IcebergRestExternalCatalog extends IcebergExternalCatalog
implements IcebergUserSessionCatalog {
-public class IcebergRestExternalCatalog extends IcebergExternalCatalog {
+ private static final Logger LOG =
LogManager.getLogger(IcebergRestExternalCatalog.class);
public IcebergRestExternalCatalog(long catalogId, String name, String
resource, Map<String, String> props,
String comment) {
super(catalogId, name, comment);
catalogProperty = new CatalogProperty(resource, props);
}
+ @Override
+ public void onClose() {
+ super.onClose();
+ // The default Catalog is RESTSessionCatalog.asCatalog(empty), which
is not itself Closeable; the
+ // closeable REST client/auth lives on the RESTSessionCatalog owned by
IcebergRestProperties, so close
+ // it here. For a REST catalog the metastore properties are always
IcebergRestProperties; they are only
+ // null before any properties have been parsed (e.g. closing a
never-initialized catalog).
+ MetastoreProperties metaProps =
catalogProperty.getMetastoreProperties();
+ if (metaProps != null) {
+ ((IcebergRestProperties) metaProps).closeRestSessionCatalog();
+ }
+ }
+
+ @Override
+ public List<String> getDbNames() {
+ SessionContext ctx = SessionContext.current();
+ if (!shouldBypassDatabaseCache(ctx)) {
+ return super.getDbNames();
+ }
+ makeSureInitialized();
+ return listLocalDatabaseNamesWithoutCache(ctx);
+ }
+
+ @Override
+ public List<String> getDbNamesOrEmpty() {
+ SessionContext ctx = SessionContext.current();
+ if (!shouldBypassDatabaseCache(ctx)) {
+ return super.getDbNamesOrEmpty();
+ }
+ try {
+ return getDbNames();
+ } catch (Exception e) {
+ LOG.warn("failed to get db names in catalog {}", getName(), e);
+ return Collections.emptyList();
+ }
+ }
+
+ @Override
+ public ExternalDatabase<? extends ExternalTable> getDbNullable(String
dbName) {
+ if (dbName == null || dbName.isEmpty() || isSystemDatabase(dbName)) {
+ return super.getDbNullable(dbName);
+ }
+ SessionContext ctx = SessionContext.current();
+ if (!shouldBypassDatabaseCache(ctx)) {
+ return super.getDbNullable(dbName);
+ }
+ try {
+ makeSureInitialized();
+ } catch (Exception e) {
+ LOG.warn("failed to get db {} in catalog {}", dbName, getName(),
e);
+ return null;
+ }
+ return getDbNullableWithoutCache(ctx, dbName);
+ }
+
+ @Override
+ protected boolean shouldBypassTableNameCache(SessionContext ctx) {
+ return shouldBypassDatabaseCache(ctx);
+ }
+
+ @Override
+ protected SessionContext getCatalogInitializationSessionContext() {
+ SessionContext ctx = SessionContext.current();
+ return shouldBypassDatabaseCache(ctx) ? ctx : SessionContext.empty();
+ }
Review Comment:
This makes the shared catalog initialization depend on the current request's
delegated credential. If the first access to an `iceberg.rest.session=user`
catalog is a delegated request, `makeSureInitialized()` calls
`IcebergRestProperties.initializeCatalog(..., ctx)`, and
`getIcebergRestCatalogPropertiesForCatalogInit(ctx)` installs that user's token
into the `RESTSessionCatalog` built at initialization time. That
`RESTSessionCatalog` is then stored in `IcebergRestProperties` and reused by
the default `catalog`, by `IcebergMetadataOps.defaultViewCatalog`, and by later
delegated sessions.
A concrete failure case is: user A is the first to run `SHOW DATABASES` on
an uninitialized REST catalog with
`iceberg.rest.oauth2.delegated-token-mode=access_token`; initialization removes
the configured OAuth credential/server properties and sets
`OAuth2Properties.TOKEN` to A's token. Later a non-delegated internal/cache
call, or another user B's call where the session credentials do not fully
override initialization-time auth state, can run against a shared REST client
initialized with A's token. This reintroduces cross-user credential
contamination at the catalog-client layer even though database/table caches are
bypassed.
Please keep the shared `RESTSessionCatalog` initialization session-neutral,
or create/own a per-session REST catalog when initialization really requires a
delegated token. The long-lived catalog objects should not be initialized with
a request-local credential.
--
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]
