morningman commented on PR #63068:
URL: https://github.com/apache/doris/pull/63068#issuecomment-4463795501
# Improvement Suggestions for PR #63068
## `sessionId` is regenerated on every forwarded request, defeating
Iceberg's session cache on the master FE
**Where**:
`fe/fe-core/src/main/java/org/apache/doris/datasource/SessionContext.java:43-48`
and
`fe/fe-core/src/main/java/org/apache/doris/qe/ConnectProcessor.java:842-851`
```java
// SessionContext.java
public static SessionContext of(DelegatedCredential delegatedCredential) {
if (delegatedCredential == null) return empty();
return new SessionContext(UUID.randomUUID().toString(),
delegatedCredential);
}
```
```java
// ConnectProcessor.java
static void restoreForwardedSessionContext(ConnectContext context,
TMasterOpRequest request) {
if (!request.isSetDelegatedCredentialToken()) return;
...
context.setSessionContext(SessionContext.of(new
DelegatedCredential(...)));
}
```
On the follower FE, `SessionContext.of()` is called once at login and the
sessionId is stable for the connection's lifetime. But on the master FE,
`restoreForwardedSessionContext` runs once *per forwarded request*, so the same
user-credential-on-master gets a fresh sessionId every time.
Iceberg REST's `CatalogProperties.AUTH_SESSION_TIMEOUT_MS` (which this PR
plumbs through via `iceberg.rest.session-timeout`) keys its OAuth session cache
by sessionId. So:
- Token-exchange mode: every forwarded request triggers a full OAuth
token-exchange round-trip on the master. This will be both slow and easy to
rate-limit at the IDP.
- Plain access-token mode: every request opens a fresh internal
`RESTSessionCatalog` session. REST servers that count active sessions (e.g. for
accounting) will see one session per query rather than one per user.
Suggested fix: serialise the originating sessionId across Thrift alongside
the token, so master reuses it. Add a Thrift field
`delegated_credential_session_id` and pass it into `SessionContext` rather than
minting a new UUID.
```thrift
struct TMasterOpRequest {
...
1007: optional string delegated_credential_session_id
}
```
```java
// ConnectProcessor.java
String sessionId = request.isSetDelegatedCredentialSessionId()
? request.getDelegatedCredentialSessionId()
: UUID.randomUUID().toString();
context.setSessionContext(SessionContext.of(sessionId, new
DelegatedCredential(...)));
```
## `IcebergRestProperties.initCatalog` pulls a session from `ConnectContext`
at catalog-create time
**Where**:
`fe/fe-core/src/main/java/org/apache/doris/datasource/property/metastore/IcebergRestProperties.java:211-218`
```java
@Override
public Catalog initCatalog(String catalogName, Map<String, String>
catalogProps,
List<StorageProperties> storagePropertiesList) {
catalogProps.putAll(getIcebergRestCatalogPropertiesForCatalogInit(currentSessionContext()));
...
}
```
`initCatalog` is called during catalog *lifecycle initialization* (e.g. on
`CREATE CATALOG` or on first lazy load), not per-request. Whatever happens to
be in `ConnectContext.get()` at that moment leaks into the catalog's
initialization properties. In `user-session` + ACCESS_TOKEN mode, this means
the catalog can end up with the *first user's token* baked into
`OAuth2Properties.TOKEN`, permanently.
In practice the per-request
`IcebergSessionCatalogAdapter.delegatedCatalog(ctx)` bypasses this global
catalog, so this only matters when code falls back to the plain `catalog` field
— but several places in `IcebergMetadataOps` still do that (the
`catalog(SessionContext)` method only switches when
`isIcebergRestUserSessionEnabled()`, not when the ctx itself carries a
credential).
Suggested fix: pass `SessionContext.empty()` from `initCatalog`. The init
path should never inherit a user context — by construction it represents
catalog-level bootstrap state.
```java
@Override
public Catalog initCatalog(String catalogName, Map<String, String>
catalogProps,
List<StorageProperties> storagePropertiesList) {
catalogProps.putAll(getIcebergRestCatalogPropertiesForCatalogInit(SessionContext.empty()));
...
}
```
---
## Internal callers that pass `SessionContext.empty()` will trip `requires
delegated credential` when user-session is on
**Where**:
`fe/fe-core/src/main/java/org/apache/doris/datasource/iceberg/IcebergMetadataOps.java:1222-1244`
```java
private Catalog catalog(SessionContext ctx) {
if (isIcebergRestUserSessionEnabled()) {
return sessionCatalogAdapter.delegatedCatalog(ctx); // requires
non-empty ctx
}
return catalog;
}
```
There are still legacy overloads like `tableExist(String, String)` that call
`tableExist(SessionContext.empty(), ...)`. When user-session is enabled, every
such call ends up in `delegatedCatalog(SessionContext.empty())` →
`requireDelegatedCredential(...)` → throws.
In normal query paths this is fine (the calling user always has a context).
But Doris has many *background* paths that may invoke catalog ops without an
active ConnectContext:
- meta cache refresh in `ExternalMetaCacheMgr`
- partition info preloading
- DDL replay on follower restart
These will explode the first time someone enables user-session, in ways that
are hard to reproduce in unit tests.
Suggested fix: decide explicitly per call site whether it's a user request
or a system request:
- User requests → require a ctx; assert on `SessionContext.empty()`.
- System requests → bypass `delegatedCatalog` and go through the bootstrap
`catalog` (which uses the catalog's own service credential, not a user's
token). The execution authenticator is already wired in for this — extend that
mechanism.
Either way, the legacy no-ctx overloads should be deprecated or removed so
the choice is made consciously at the call site.
## Dead code: `IcebergMetadataOps.useSessionCatalog`
**Where**:
`fe/fe-core/src/main/java/org/apache/doris/datasource/iceberg/IcebergMetadataOps.java:1246-1248`
```java
private boolean useSessionCatalog(SessionContext ctx) {
return ctx != null && ctx.hasDelegatedCredential() &&
isIcebergRestUserSessionEnabled();
}
```
Defined, never called. Either delete it, or — better — use it inside
`catalog(ctx)` / `namespaces(ctx)` so the empty-ctx legacy callers (see
MEDIUM-4) take a sensible fallback path.
---
## Six near-identical copies of `currentSessionContext()`
**Where**: `ExternalDatabase.java`, `IcebergExternalCatalog.java`,
`IcebergExternalTable.java`, `IcebergMetadataOps.java`,
`IcebergRestProperties.java`, `IcebergUtils.java`
Every copy is the same six lines:
```java
private static SessionContext currentSessionContext() {
ConnectContext context = ConnectContext.get();
if (context == null) return SessionContext.empty();
SessionContext sessionContext = context.getSessionContext();
return sessionContext == null ? SessionContext.empty() : sessionContext;
}
```
Move it to `SessionContext.current()` (or
`SessionContext.fromCurrentConnect()`) so the next person who needs to add
logging, telemetry, or a non-ThreadLocal source doesn't have to chase six files.
--
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]
