Skip to content

[fix](be) Use trusted JDBC catalog metadata for connection tests#62464

Open
CalvinKirs wants to merge 1 commit intoapache:masterfrom
CalvinKirs:fix/be-jdbc-driver-resource-id
Open

[fix](be) Use trusted JDBC catalog metadata for connection tests#62464
CalvinKirs wants to merge 1 commit intoapache:masterfrom
CalvinKirs:fix/be-jdbc-driver-resource-id

Conversation

@CalvinKirs
Copy link
Copy Markdown
Member

What problem does this PR solve?

Issue Number: None

Related PR: None

Problem Summary: Prevent BE test_jdbc_connection from trusting caller-supplied JDBC driver metadata and validation SQL. BE now fetches trusted JDBC catalog metadata from FE by catalog id, and the JDBC tester verifies the driver checksum before loading the driver jar.

Release note

Harden JDBC connection testing so the backend only loads trusted driver metadata from the catalog.

Check List (For Author)

  • Test: FE unit test / Manual test
    • FE unit test: ./run-fe-ut.sh --run org.apache.doris.datasource.jdbc.JdbcExternalCatalogTest
    • Manual test: build-support/check-format.sh
    • Manual test: cd fe && mvn -pl fe-core,be-java-extensions/jdbc-scanner -am validate -DskipTests -Dmaven.build.cache.enabled=false -rf :fe-core
    • No BE unit test: ./run-be-ut.sh --run --filter=JdbcUtilsTest.* -j 4 failed in CMake dependency detection (missing OpenMP_C)
  • Behavior changed: Yes (JDBC connection test now ignores caller-supplied driver metadata and SQL, and uses trusted catalog metadata instead)
  • Does this need documentation: No

What problem does this PR solve?

Issue Number: close #xxx

Related PR: #xxx

Problem Summary:

Release note

None

Check List (For Author)

  • Test

    • Regression test
    • Unit Test
    • Manual test (add detailed scripts or steps below)
    • No need to test or manual test. Explain why:
      • This is a refactor/code format and no logic has been changed.
      • Previous test can cover this change.
      • No code files have been changed.
      • Other reason
  • Behavior changed:

    • No.
    • Yes.
  • Does this need documentation?

    • No.
    • Yes.

Check List (For Reviewer who merge this PR)

  • Confirm the release note
  • Confirm test cases
  • Confirm document
  • Add branch pick label

### What problem does this PR solve?

Issue Number: None

Related PR: None

Problem Summary: Prevent BE test_jdbc_connection from trusting caller-supplied JDBC driver metadata and validation SQL. BE now fetches trusted JDBC catalog metadata from FE by catalog id, and the JDBC tester verifies the driver checksum before loading the driver jar.

### Release note

Harden JDBC connection testing so the backend only loads trusted driver metadata from the catalog.

### Check List (For Author)

- Test: FE unit test / Manual test
    - FE unit test: ./run-fe-ut.sh --run org.apache.doris.datasource.jdbc.JdbcExternalCatalogTest
    - Manual test: build-support/check-format.sh
    - Manual test: cd fe && mvn -pl fe-core,be-java-extensions/jdbc-scanner -am validate -DskipTests -Dmaven.build.cache.enabled=false -rf :fe-core
    - No BE unit test: ./run-be-ut.sh --run --filter=JdbcUtilsTest.* -j 4 failed in CMake dependency detection (missing OpenMP_C)
- Behavior changed: Yes (JDBC connection test now ignores caller-supplied driver metadata and SQL, and uses trusted catalog metadata instead)
- Does this need documentation: No
@hello-stephen
Copy link
Copy Markdown
Contributor

Thank you for your contribution to Apache Doris.
Don't know what should be done next? See How to process your PR.

Please clearly describe your PR:

  1. What problem was fixed (it's best to include specific error reporting information). How it was fixed.
  2. Which behaviors were modified. What was the previous behavior, what is it now, why was it modified, and what possible impacts might there be.
  3. What features were added. Why was this function added?
  4. Which code was refactored and why was this part of the code refactored?
  5. Which functions were optimized and what is the difference before and after the optimization?

@CalvinKirs
Copy link
Copy Markdown
Member Author

run buildall

@CalvinKirs
Copy link
Copy Markdown
Member Author

/review

Copy link
Copy Markdown
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found 1 blocking issue.

  1. Blocking: CREATE JDBC CATALOG now fails during checkWhenCreating() when test_connection=true and a BE is alive. CatalogFactory.createCatalog() calls catalog.checkWhenCreating() before CatalogMgr.createCatalogImpl() registers the catalog, but the new BE test path sends only catalog_id and FrontendServiceImpl.getJdbcTestConnectionInfo() resolves it from Env.getCurrentEnv().getCatalogMgr(). For a brand-new catalog that lookup returns null, so the BE-side connectivity test rejects the request even though the FE already has the trusted in-memory properties. The new unit tests only cover request/table builders and do not exercise this creation-time path.

Critical checkpoint conclusions:

  • Goal of the task: Partially achieved. The PR removes caller-controlled JDBC metadata from the BE test path, but it breaks JDBC catalog creation/testing in the default test_connection=true flow.
  • Minimality/focus: Mostly focused, but the new trusted-metadata lookup adds a lifecycle dependency on catalog registration that the previous code path did not have.
  • Concurrency: No new lock-order or shared-state concurrency issue was identified in the reviewed paths.
  • Lifecycle management: Blocking issue present. The new FE lookup assumes the catalog already exists in CatalogMgr, which is false while checkWhenCreating() is still running.
  • Configuration changes: No new config items.
  • Compatibility / parallel paths: No blocking protocol-compatibility issue found in the optional new RPC/proto fields, and existing JDBC scan paths still use FE-provided checksum metadata.
  • Special conditional checks: No additional blocker found.
  • Test coverage: Insufficient for this change. Added FE unit tests do not cover the creation-time BE connection test path that regressed.
  • Observability: Existing returned status/error messages are adequate for this failure path.
  • Transaction / persistence / data writes: Not applicable for this review.
  • FE-BE variable passing: The new catalog_id propagation exists, but it does not work for the creation-time path because the catalog is not yet registered.
  • Performance: The extra FE RPC is acceptable for a non-hot connection-test path.
  • Other issues: None beyond the blocker above were confirmed.

Overall opinion: request changes until the creation-time lifecycle regression is fixed.

}

private TTableDescriptor buildTestConnectionThrift() throws DdlException {
public PJdbcTestConnectionRequest buildTestConnectionRequest(TOdbcTableType tableType) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking: this switches the BE connectivity test to resolve the catalog back out of CatalogMgr, but checkWhenCreating() runs before the new catalog is registered there. The create flow is CatalogFactory.createCatalog(...)->catalog.checkWhenCreating() first, and only later CatalogMgr.createCatalogImpl(...)->createCatalogInternal(...) adds the catalog. In that window FrontendServiceImpl.getJdbcTestConnectionInfo() will see getCatalog(request.getCatalogId()) == null, so CREATE CATALOG ... PROPERTIES('type'='jdbc', ...) now fails whenever test_connection=true and a BE is available. Please keep using the already-validated in-memory catalog properties during creation, or add a separate trusted path that does not depend on the catalog already existing in CatalogMgr.

@hello-stephen
Copy link
Copy Markdown
Contributor

BE UT Coverage Report

Increment line coverage 0.00% (0/32) 🎉

Increment coverage report
Complete coverage report

Category Coverage
Function Coverage 53.08% (20158/37975)
Line Coverage 36.61% (189548/517736)
Region Coverage 32.92% (147384/447762)
Branch Coverage 34.01% (64448/189484)

@hello-stephen
Copy link
Copy Markdown
Contributor

BE Regression && UT Coverage Report

Increment line coverage 62.50% (20/32) 🎉

Increment coverage report
Complete coverage report

Category Coverage
Function Coverage 71.72% (26663/37179)
Line Coverage 54.69% (282272/516092)
Region Coverage 51.54% (232860/451800)
Branch Coverage 53.07% (100851/190017)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants