Uh oh!
There was an error while loading. Please reload this page.
[fix](catalog) apply the jdbc driver_url checks to iceberg/paimon and to ALTER CATALOG - #66483
Open
CalvinKirs wants to merge 1 commit into
Open
[fix](catalog) apply the jdbc driver_url checks to iceberg/paimon and to ALTER CATALOG#66483CalvinKirs wants to merge 1 commit into
CalvinKirs wants to merge 1 commit into
Conversation
hello-stephen
commented
Aug 5, 2026
Contributor
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
CalvinKirsforce-pushed
the
jdbc_iceberg_paimon
branch
from
August 5, 2026 10:14
4b75cd8 to
cd4caedCompareCalvinKirs
commented
Aug 5, 2026
MemberAuthor
run buildall |
hello-stephen
commented
Aug 5, 2026
Contributor
TPC-H: Total hot run time: 28557 ms |
hello-stephen
commented
Aug 5, 2026
Contributor
TPC-DS: Total hot run time: 166484 ms |
hello-stephen
commented
Aug 5, 2026
Contributor
ClickBench: Total hot run time: 23.86 s |
CalvinKirs
commented
Aug 7, 2026
MemberAuthor
run external |
… to ALTER CATALOG The jdbc catalog validated its `driver_url` twice: with a mandatory rule (no path traversal; a bare name must match `[A-Za-z0-9._-]+\.jar`) and with the operator's `jdbc_driver_secure_path` / `jdbc_driver_url_white_list` gate from fe.conf. The iceberg-jdbc and paimon-jdbc catalogs reach the same `URLClassLoader` + `Class.forName` sink from `iceberg.jdbc.driver_url` / `jdbc.driver_url`, but ran only the operator gate, only at CREATE (their `preCreateValidation`); the mandatory rule ran nowhere for them. Separately, none of the three ran either check on ALTER CATALOG. ALTER validates through `validatePropertiesBeforeUpdate` and never reaches `Connector.preCreateValidation`, which is where CREATE applies them, while `resetToUninitialized` makes the new value effective on the next metadata access. An operator who restricted `jdbc_driver_secure_path` therefore had the restriction enforced at CREATE and silently bypassed by a follow-up ALTER (a gap the paimon connector's own javadoc records). - Extract the mandatory rule into `JdbcDriverUrlSecurity` (fe-connector-spi, the one module all three connectors depend on) and call it from each provider's `validateProperties`, which fe-core runs on CREATE and on ALTER, and never on replay -- so existing catalogs and follower startup are unaffected. - Add `ConnectorProvider.driverUrlsToValidate` so a connector can name the jars it would load into the FE JVM; `PluginDrivenExternalCatalog` applies the operator gate to them on ALTER. The declaration is flavor-gated, so a stray `driver_url` on a REST/HMS/filesystem catalog does not turn a previously-accepted catalog into a CREATE/ALTER failure. - Bump `<connector.plugin.api.version>` 6.0 -> 7.0 alongside the surface baseline: a 6.x plugin inherits the empty default and would skip the operator's policy silently, so it must be refused at load time. - AGENTS.md: no new code path may fetch an artifact from a user-supplied URL and load it into a Doris process. The existing `driver_url` paths are grandfathered, not a precedent. Behavior for a remote (http/https) driver jar is unchanged by default: `jdbc_driver_secure_path` still defaults to `*`. What changes is that restricting it now actually holds on every DDL path.
CalvinKirsforce-pushed
the
jdbc_iceberg_paimon
branch
from
August 24, 2026 02:48
cd4caed to
112e145CompareCalvinKirs
commented
Aug 24, 2026
MemberAuthor
run buildall |
CalvinKirs
commented
Aug 24, 2026
MemberAuthor
/review |
Contributor
There was a problem hiding this comment.
Requesting changes for two blocking ALTER CATALOG issues.
- A replay-created degraded catalog treats provider absence as an empty driver declaration, so interactive ALTER can persist an unchecked driver URL that is consumed after the provider is restored.
- The ALTER gate resolves bare jar names with the global JDBC directory even though all three plugin loaders prefer connector-specific
drivers_dir, rejecting otherwise loadable persisted catalogs even on unrelated ALTERs.
Review checkpoints: the change is otherwise focused and the provider-present JDBC/Iceberg/Paimon alias and flavor paths use the same raw winner as their FE/BE consumers. Detached validation normally fails before publication, replay/reset ordering is sound outside the degraded-provider case, API major 7 and the recorded provider method are coherent, and no new protocol, storage-format, concurrency, observability, or material hot-path performance issue was found. URI normalization, current-provider TCCL behavior, checksum scope, and the selective SPI surface baseline were rechecked and did not yield additional patch-introduced findings.
Tests cover the shared grammar, first-party wiring, aliases/flavors, and scheme-bearing policy, but omit the two lifecycle/configuration cases above. This was a static bundle review as required; no local build or tests were run. The live FE UT check is currently failing, while compile, performance, and automated code-review checks are pending. The user focus file supplied no additional focus. Three review rounds converged with no unresolved candidates beyond these two comments.
| return provider.driverUrlsToValidate(properties); | ||
| } | ||
| } | ||
| return Collections.emptyList(); |
Contributor
There was a problem hiding this comment.
[P1] Fail closed when no connector provider matches. Returning an empty list here conflates a matching provider that loads no jar with a replay-created degraded catalog whose provider is absent or API-rejected. In that supported degraded state, validatePropertiesForUpdate also falls through, so an interactive ALTER CATALOG can journal an unchecked driver_url; after an API-7 provider is restored and FE restarts, replay skips validation and lazy initialization hands that persisted value to the JDBC/Iceberg/Paimon driver loader. Please distinguish provider absence from an empty declaration and reject interactive ALTER until a compatible provider is installed, while keeping replay permissive.
| private void checkDriverUrlsAgainstOperatorGate(Map<String, String> candidate) throws DdlException { | ||
| for (String driverUrl : ConnectorFactory.driverUrlsToValidate(getType(), candidate)) { | ||
| try { | ||
| JdbcResource.getFullDriverUrl(driverUrl); |
Contributor
There was a problem hiding this comment.
[P1] Validate bare names using the connector's resolution context. driverUrlsToValidate returns raw values, but this call resolves a bare jar through global Config.jdbc_drivers_dir and can throw if it is absent there. JDBC, Iceberg, and Paimon loaders instead prefer their plugin-specific <name>.confdrivers_dir. Thus a persisted catalog with driver_url=d.jar and the jar only in its supported plugin directory now fails even an unrelated ALTER CATALOG, although lazy initialization can resolve and load it. Please avoid global filesystem resolution for a mandatory-rule-approved bare name, or pass the connector-resolved path/context to the engine gate; cover this with a real provider using a custom drivers_dir.
hello-stephen
commented
Aug 24, 2026
Contributor
TPC-H: Total hot run time: 17141 ms |
hello-stephen
commented
Aug 24, 2026
Contributor
TPC-DS: Total hot run time: 82757 ms |
hello-stephen
commented
Aug 24, 2026
Contributor
ClickBench: Total hot run time: 14.51 s |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Problem Summary:
driver_urlon a jdbc-flavored catalog names a jar that the FE loads into its own JVM(
URLClassLoader+Class.forName(name, true, loader)). Doris guards it in two layers:..path segment, and a bare file name must match[A-Za-z0-9._-]+\.jar;jdbc_driver_secure_path/jdbc_driver_url_white_list.Two gaps:
The iceberg-jdbc and paimon-jdbc catalogs never ran the mandatory rule.
iceberg.catalog.type=jdbcand
paimon.catalog.type=jdbcreach the same class-loading sink throughiceberg.jdbc.driver_url/jdbc.driver_url. TheirpreCreateValidationroutes the value through the fe.conf gate at CREATE, butwith the default
jdbc_driver_secure_path=*that gate accepts everything — nothing forbids a..traversal segment, and the connector-side resolver (
JdbcDriverSupport.resolveDriverUrl) happilyresolves
../against the drivers directory.None of the three ran either check on
ALTER CATALOG. ALTER validates throughPluginDrivenExternalCatalog.validatePropertiesBeforeUpdateand never reachesConnector.preCreateValidation, which is where CREATE applies both layers;resetToUninitializedthen makes the new value effective on the next metadata access. So an operator who narrowed
jdbc_driver_secure_pathgot the restriction enforced atCREATE CATALOGand silently bypassed bya follow-up
ALTER CATALOG ... SET ("driver_url" = ...)— the configuration did not actuallyprotect the catalogs it was meant to protect. (The paimon connector's own javadoc records this as a
known gap "shared by all plugin connectors".)
What this PR does:
JdbcDriverUrlSecurity(fe-connector-spi — the one module allthree connectors depend on), deleting the jdbc-local copy
(
JdbcDorisConnector.checkDriverUrlSecurityRule), and calls it from each provider'svalidateProperties. fe-core runs that hook on CREATE and on ALTER, and never on replay, soexisting catalogs and follower startup are unaffected.
ConnectorProvider.driverUrlsToValidate(properties): the connector names the values it wouldhand to a class loader, and
PluginDrivenExternalCatalogapplies the fe.conf gate to them onALTER. Only the connector knows which property is a jar; only the engine knows the fe.conf policy.
The declaration is flavor-gated, so a stray
driver_urlleft on a REST/HMS/filesystem catalogdoes not turn a previously-accepted catalog into a CREATE/ALTER failure.
<connector.plugin.api.version>6.0->7.0together with the refreshedconnector-plugin-surface.txt, per the rule recorded inConnectorPluginSurfaceTest: an olderplugin inherits the empty default of the new method, so its
driver_urlwould skip the operator'spolicy without any error — it has to be refused at load time rather than silently under-enforced.
AGENTS.md: no new code path may fetch an artifact from a user-supplied URL and load it into aDoris process; the existing
driver_urlpaths are grandfathered, not a precedent. TheADBC catalog's local-only
AdbcDriverPathResolver([feat](catalog) support ADBC catalog that reads external sources over Arrow #66331) is cited there as the model to follow.Release note
Fixed
driver_urlvalidation for jdbc-flavored catalogs: the Iceberg and Paimon JDBC catalogs nowapply the same mandatory driver-jar rules as the JDBC catalog, and
ALTER CATALOGnow appliesjdbc_driver_secure_path/jdbc_driver_url_white_listinstead of onlyCREATE CATALOGdoing so.Check List (For Author)
Test
New:
JdbcDriverUrlSecurityTest(rule semantics, moved with the class),IcebergJdbcDriverUrlSecurityTest,PaimonJdbcDriverUrlSecurityTest(rule reachable on both theprovider and the
preCreateValidationpath; skipped on non-jdbc flavors),PluginDrivenExternalCatalogDriverUrlGateTest(ALTER honours the operator allow-list; a connectorthat declares no jar is untouched).
Each new test was mutation-checked: removing the corresponding production line turns it red.
Full runs (rebased onto master): fe-connector spi/jdbc/paimon suites all green; fe-connector-iceberg
green except
IcebergWritePlanProviderTest#planMergePreservesExplicitlyEmptyReadAcrossConcurrentFirstAppend,which fails identically on unmodified master in the same environment (pre-existing, unrelated to
this change). fe-core plugin/API-version/gate tests
Tests run: 33, Failures: 0, Errors: 0;0 Checkstyle violations.
Behavior changed:
A
driver_urlcontaining a..segment, or a bare file name outside[A-Za-z0-9._-]+\.jar, isnow rejected on the Iceberg/Paimon JDBC catalogs as it already was on the JDBC catalog, and on
ALTER as well as CREATE.
jdbc_driver_secure_path/jdbc_driver_url_white_listnow also applyon ALTER. The default posture is unchanged:
jdbc_driver_secure_pathstill defaults to*, so aremote driver jar is still accepted unless the operator narrows the config — what changes is that
narrowing it now holds on every DDL path. Validation never runs on replay, so no existing catalog
and no follower startup can be broken by this.
Note that ALTER validates the merged property candidate, not only the keys being changed: once
an operator narrows
jdbc_driver_secure_path, any ALTER on a catalog whose storeddriver_urlfalls outside the new allow-list is rejected (fail-closed) until the
driver_urlitself is fixedin the same statement. The catalog keeps working for queries and across restarts either way.
Does this need documentation?