Skip to content

Antalya 26.6 Backport of #126138 - Revert "Revert "Verify the server certificate hostname on outbound TLS connections"" - #2669

Merged
zvonand merged 1 commit into
antalya-26.6from
backports/antalya-26.6/126138
Oct 25, 2026
Merged

zvonand merged 1 commit into
antalya-26.6from
backports/antalya-26.6/126138

Conversation

@zvonand

@zvonand zvonand commented Oct 25, 2026

Copy link
Copy Markdown
Member

Changelog category (leave one):

  • Not for changelog (changelog entry is not required)

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):

...

Documentation entry for user-facing changes

...

CI/CD Options

Exclude tests:

  • Fast test
  • Integration Tests
  • Stateless tests
  • Stateful tests
  • Unit tests
  • Performance tests
  • Aarch64 tests
  • All with ASAN
  • All with TSAN
  • All with MSAN
  • All with UBSAN
  • All with Coverage
  • All Regression
  • Disable CI Cache

Regression jobs to run:

  • Fast suites (mostly <1h)
  • Aggregate Functions (2h)
  • Alter (1.5h)
  • Benchmark (30m)
  • CAS (content-addressed storage; Antalya only)
  • ClickHouse Keeper (1h)
  • Iceberg (2h)
  • LDAP (1h)
  • OAuth (5m)
  • Parquet (1.5h)
  • RBAC (1.5h)
  • SSL Server (1h)
  • S3 (2h)
  • S3 Export (2h)
  • Swarms (30m)
  • Tiered Storage (2h)

…michev/revert-116724

Revert "Revert "Verify the server certificate hostname on outbound TLS connections""
@github-actions

Copy link
Copy Markdown

Workflow [PR], commit [0a50510]

@zvonand

zvonand commented Oct 25, 2026

Copy link
Copy Markdown
Member Author

@blau-ai

@blau-ai

blau-ai commented Oct 25, 2026

Copy link
Copy Markdown
Collaborator

CI triage — 4 failing checks, 0 caused by this PR

This is a backport of upstream ClickHouse#126138 (default openSSL.client.extendedVerification → true, i.e. verify the server cert hostname on outbound TLS). The diff only touches SSLManager.cpp, ServerSettings.cpp, docs, and four TLS integration tests (test_accept_invalid_certificate, test_acme_tls, test_bind_host, test_ssl_cert_authentication). None of the four red checks is in that blast radius — all builds, Fast test, unit tests, the integration-test shards (which include the modified TLS tests), stress and regression suites are green.

Check Verdict Cause
DCO ❌ not PR-code Backported merge commit has no Signed-off-by
SQLLogic test ❌ not PR Job timeout (exit 137, 3h cap)
Stateless (distributed plan, 1/2) ❌ not PR Pre-existing aggregation crash
Stateless (cas s3 storage, 1/2) ❌ not PR Same pre-existing aggregation crash

1. DCO — process, not code

The single head commit 0a505106 (merge of upstream ClickHouse#126138, authored by Alexey Milovidov) carries no Signed-off-by: trailer, so the cncf/dco2 app fails. This is unrelated to the change itself.
Next step: a maintainer either adds the DCO override/label used for upstream backports, or the commit is re-authored with -s. I can't do that here — it would require rewriting the commit and force-pushing, which is outside my permissions.

2. SQLLogic test — infra timeout (safe to re-run)

[20:04:14] WARNING: Timeout exceeded [10800] for [2773]
[20:04:26] WARNING: Job timed out: [SQLLogic test], timeout [10800], exit code [137]
[20:04:26] ERROR: Run failed with exit code [137]

The complete-test phase was still running when the 3h job cap hit (statements-test alone took ~1h44m). No test asserted a wrong result — the job was killed. SQLLogic exercises SQL execution, which this TLS-only PR does not touch.
Next step: re-run. If it keeps timing out it's a runner/sharding issue to raise independently of this backport.

3 & 4. Stateless tests — pre-existing aggregation crash (safe to re-run)

Both shards report Failed: 0 tests; they went red because the server died on a logical-error assertion, identical signature in both (STID 3994-4c1d):

Logical error: 'GroupingAggregatedTransform pushed bucket N twice'.
  src/Processors/Transforms/MergingAggregatedMemoryEfficientTransform.cpp:63
  DB::GroupingAggregatedTransform::pushData(...)
  DB::GroupingAggregatedTransform::tryPushTwoLevelData()

This is a crash in the two-level / distributed aggregation merge path — reachable only by the distributed plan and cas s3 storage configs that hammer two-level aggregation. It has no connection to TLS client-cert verification; the PR changes nothing in src/Processors or the aggregation engine. Classic flaky/pre-existing aggregation bug.
Next step: re-run the two shards. If the crash reproduces it should be tracked as an aggregation-engine issue on antalya-26.6, separate from this backport.

Health check

The backport itself looks healthy: it builds on all configs, and the integration tests that cover the behavioral change (hostname verification now on by default, with extendedVerification=false added to the test configs that legitimately connect by IP / mismatched CN) pass. All four red checks are process (DCO) or environment (timeout / pre-existing crash) — none is a regression introduced by this change. Recommend handling the DCO sign-off and re-running the three test checks.

🤖 blau-ai · evidence: result_pr.json, sqllogic_test/job.log for head 0a505106

@zvonand
zvonand merged commit d8c4850 into antalya-26.6 Oct 25, 2026
308 of 316 checks passed
@zvonand zvonand added the verified Approved for release label Oct 25, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants