Repository navigation
[fix](plugin) Allow legacy unversioned authentication plugins - #67286
Merged
CalvinKirs merged 3 commits intoOct 30, 2026
Merged
Conversation
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Member
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 16967 ms |
Contributor
TPC-DS: Total hot run time: 82569 ms |
Contributor
ClickBench: Total hot run time: 14.73 s |
Contributor
|
/review |
Member
Author
|
run external |
Member
Author
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Request changes: the legacy compatibility branch makes a directory-loader manifest lookup failure indistinguishable from a genuinely unversioned legacy artifact, so an unverifiable authentication plugin can be published.
Critical checkpoints:
- Goal and proof: readable legacy authentication JARs are now accepted with a warning, while blank, malformed, and incompatible declarations remain rejected. The goal is not fully met because unresolved or unreadable defining-JAR metadata also enters the legacy branch; the missing loader-level negative test is called out inline.
- Scope and clarity: the five-file change is focused, and the generic gate keeps legacy behavior opt-in.
- Concurrency and lifecycle: no new shared mutable state or lock order is introduced; the immutable gate and warning run inside the existing load lifecycle. External JAR mutation is not protected by that lock and exposes the accepted issue. Existing
PluginLoadExceptioncleanup would close the classloader once read failures are propagated. - Compatibility and parallel paths: repository history supports authentication API 1.0 as the pre-manifest contract, and the fixed legacy major is compared with the independently filtered current major, so a future 2.x FE rejects it. Connector, filesystem, and lineage remain strict. The classpath-provider path also remains fail-closed on JAR I/O errors; only the directory path has the inline inconsistency.
- Tests and results: the added tests correctly cover readable absence, warning selection, malformed legacy configuration, and legacy/current major mismatch. No result files changed. I did not run local builds or tests because this review environment forbids them; the live PR currently reports Compile, FE UT, and CheckStyle passing.
- Observability and performance: the warning is useful and negligible for true legacy loads, but becomes misleading on the accepted failure path. No other hot-path or observability concern was found.
- Configuration, persistence, transactions, data writes, and FE-BE protocol variables: not applicable to this change.
- User focus: no additional focus was provided.
Review completion: two bounded rounds completed with two full-coverage lanes and one separate risk-focused lane. Round 2 returned NO_NEW_VALUABLE_FINDINGS in all lanes; every candidate was accepted, deduplicated, or dismissed with evidence. One inline issue remains.
| return "no " + manifestAttribute + " in the plugin jar MANIFEST; this FE serves " + familyLabel | ||
| + " plugin API " + expectedVersion + ". Declare the attribute and rebuild the plugin" | ||
| + " against this Doris release."; | ||
| if (declaredVersion == null) { |
Contributor
There was a problem hiding this comment.
[P2] Distinguish an absent declaration from a failed manifest lookup
On the directory-loading path, null does not prove that a readable defining JAR genuinely omitted this attribute. readFromDefiningJar also returns null after every JarFile/manifest IOException, or when the fallback scan cannot find the factory class. For example, lazy loading can define a self-contained factory and then race a JAR removal or permission/read failure before the separate manifest opens; this branch then admits and publishes the unverifiable plugin as legacy 1.0. Preserve an explicit successfully-read-but-absent result for this legacy case, reject unresolved/read failures at STAGE_API_VERSION, and add a loader-level negative test alongside the true-absence case.
### What problem does this PR solve? Issue Number: N/A Related PR: apache#67286 Problem Summary: The directory plugin loader used null for both a genuinely absent API-version declaration and a failed defining-JAR lookup. With legacy authentication compatibility enabled, an unreadable JAR, malformed manifest, or unresolved factory could therefore be admitted as API 1.0. Propagate lookup failures as apiVersion load failures before construction and publication, retain missing defining-JAR locations instead of falling back to another copy of the class, and keep release-version display metadata best effort. Add deterministic loader tests for readable legacy plugins, explicit invalid declarations, corrupt JARs/manifests, missing defining JARs, and unavailable provenance. ### Release note Authentication plugins with unreadable or unresolved API-version metadata are rejected. Readable legacy plugins without a declaration continue to load with a warning. ### Check List (For Author) - Test: Unit Test - run-fe-ut.sh with CUSTOM_MVN scoping the reactor to fe-extension-loader and fe-authentication-handler: 59 tests passed, including six new loader tests; Checkstyle passed for all 10 selected reactor modules. Full build.sh --fe was stopped after incomplete native third-party dependencies triggered an unrelated dependency rebuild. - Behavior changed: Yes (failed API-version metadata verification cannot use legacy compatibility) - Does this need documentation: No
Member
Author
|
run buildall |
Member
Author
Local pipeline review — ✅ PASSschema: doris-repo-review/v1
status: PASS
pr: apache/doris#67286
commit: 785744e577ae64ae697746b84215132fd7e3a492
base: 0cfc81c6163b79a6717e165e4c9dc1c8e27faa60
reviewed_at: 2026-10-14T11:40+08:00
reviewer: CalvinKirs
model: gpt-6.1-sol
effort: high
findings: {blocker: 0, major: 0, minor: 0, nit: 0}
rounds: 1
converged: trueNotes for maintainers
Reviewed locally with the |
Contributor
TPC-H: Total hot run time: 29513 ms |
Contributor
TPC-DS: Total hot run time: 152897 ms |
Contributor
ClickBench: Total hot run time: 24.63 s |
hubgeter
approved these changes
Oct 14, 2026
Contributor
FE Regression Coverage ReportIncrement line coverage |
Member
Author
|
run feut |
Contributor
FE UT Coverage ReportIncrement line coverage `` 🎉 |
Member
Author
|
/review |
Contributor
There was a problem hiding this comment.
Static review completed for PR 67286 at exact head 785744e. The change meets its stated goal: authentication directory plugins may use a genuinely absent manifest declaration as documented legacy API 1.0, while explicit empty, malformed, or incompatible declarations remain rejected and other plugin families remain strict. The loader now distinguishes a readable manifest with an absent attribute from missing, unreadable, malformed, or unresolved defining-jar metadata; those failures become STAGE_API_VERSION failures and the runtime classloader is closed before the per-directory failure is recorded. Manifest provenance and the display-only version path were checked across all in-tree callers. The added unit coverage was inspected but not executed because this review is static-only. Concurrency, lifecycle/cleanup, compatibility, parallel family paths, error staging, observability, and focused user concerns (none supplied) were reviewed with no new defect found. Existing inline P2 comment 3922803703 is addressed by this head and is not reposted. Review converged after one round; no P0/P1 findings or remaining supported-path coverage gaps remain. Unsupported custom or special CodeSource loaders were considered but do not establish a finding for this PR.
Review converged after 1 round(s).
Contributor
|
PR approved by anyone and no changes requested. |
github-actions
Bot
added
the
approved
Contributor
|
PR approved by at least one committer and no changes requested. |
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 free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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?
#66211
Problem Summary:
The authentication plugin API-version gate rejects every plugin jar that does not contain
Doris-Authentication-Plugin-Api-Version. Authentication plugins were distributed before this manifest contract existed, so upgrading FE breaks those legacy artifacts even when they implement the current v1 API.This change adds an opt-in legacy mapping to the generic API gate and configures only the authentication family to treat a genuinely absent declaration as API 1.0: