Skip to content

[Improvement-17813][ApiServer][Part2] Add an optional connectivity check on datasource create and update - #18739

Open
njnu-seafish wants to merge 4 commits into
apache:devfrom
njnu-seafish:Improvement-17813-part2
Open

njnu-seafish wants to merge 4 commits into
apache:devfrom
njnu-seafish:Improvement-17813-part2

Conversation

@njnu-seafish

Copy link
Copy Markdown
Contributor

Was this PR generated or assisted by AI?

YES. Optimize and refine the code using AI after writing it.

Purpose of the pull request

Part of #17813.

This PR implements the "connectivity check on save" route from the #17813 discussion
(option 1 there), in the shape that discussion converged on:

So this PR adds api.datasource-connection-check-enabled (default false):

  • default off: zero behavior change on upgrade — no config to migrate, nothing to add to incompatible.md.
  • opt-in: deployments where the API server can reach the datasources get a hard guarantee that nothing un-connectable is ever persisted.

One placement detail worth calling out: on update, the check runs after the password resolution (the existing blank-keeps-old-password backfill), so it always tests the effective credentials that are about to be persisted — when the user keeps the old password, the connectivity is verified against that old password. The unit test locks this with an ArgumentCaptor assertion.

This PR complements the write-path guard PR (Improvement-17813 branch, "Treat the mask value returned by query APIs as an unmodified password when updating a datasource"): with the guard merged, editing a datasource without touching the password resolves to the old password, this check then validates it end-to-end, and the save succeeds — the two layers close the loop.

Brief change log

  • ApiConfig: new datasourceConnectionCheckEnabled flag (default false), bound to api.datasource-connection-check-enabled.
  • DataSourceServiceImpl: createDataSource / updateDataSource call checkConnectionBeforePersist with the assembled ConnectionParam (gated by the flag); on update this happens after the password backfill.
  • Config synced to: api application.yaml, standalone-server application.yaml, k8s values.yaml (customizedConfig example), configuration.md (en + zh).
  • Unit tests: 5 new cases in DataSourceServiceTest + 1 in ApiConfigTest.
  • api-test (per the review request in [Fix-17813][ApiServer] Correct the password processing logic during data source modification #18387, "You should add api-test or e2e-test to verify it"): two compose scenarios
    (datasource-connection-check-disabled / datasource-connection-check-enabled),
    DataSourceConnectionCheckDisabledAPITest / DataSourceConnectionCheckEnabledAPITest asserting create/update with an unreachable host succeeds when disabled and fails when enabled, plus a DataSourcePage client and postJson/putJson helpers.

Verify this pull request

Unit tests (18/18 in DataSourceServiceTest, 4/4 in ApiConfigTest):

  • disabled (default): create succeeds, and the connectivity check is never invoked (verified with never())
  • enabled: the check receives the submitted password on create, and a failed check rejects the save (CONNECTION_TEST_FAILURE) with no row persisted (insert never called)
  • enabled + update with a blank password: the check receives the old password from the store (ArgumentCaptor), and the persisted connectionParams keep it
  • ApiConfigTest asserts the yaml binding takes effect

Pull Request Notice

Pull Request Notice

If your pull request contains incompatible change, you should also add it to docs/docs/en/guide/upgrade/incompatible.md

This branch has not been deployed

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant