Connect to a secured ClickHouse catalog: header credentials, verified TLS, bounded retries - #151
Merged
Merged
Conversation
…d retries The native catalog client could only speak plain http://host:port with no credentials, so capture storage had no way to reach a production ClickHouse behind a password or TLS. It also treated every failure alike: nothing was retried, not even a connection that was refused before a byte was sent. ClickHouseClient now takes a ClickHouseConnection (scheme, host, port, user, password, ca_file, ca_path, allow_insecure_http, timeouts, max_attempts), validated when the client is built: - user and password travel as X-ClickHouse-User / X-ClickHouse-Key headers, never in the URL; no redirect is followed, so they reach only this host; - https always verifies the peer and its name, with an optional private CA file or hashed directory (CURLOPT_CAINFO / CAPATH); - a password over plain http is refused unless allow_insecure_http, and that flag is refused with https, where it would read as a downgrade; - a host carrying userinfo, a scheme, a port or a path is refused, and the message never repeats the value, which may hold a password; - a refused or unresolvable connection is retried for any statement (nothing reached the server); a reset, empty reply or 5xx is retried for reads only (first keyword SELECT/WITH/SHOW/DESCRIBE/EXISTS/CHECK); a write that may have reached the server is never repeated. Timeouts are not retried, so a timed-out call still returns within one request timeout; - reads carry wait_end_of_query=1, so an exception part-way through a result arrives as an error status instead of a truncated 200 that parses as rows. It is a URL setting: statement bytes are unchanged, and the lease byte-identity tests (query_log text) pass untouched. StorageServiceConfig's host/port/timeouts become one ClickHouseConnection, and _dmi_native_store reads the same clickhouse_* keys for the service and for CaptureReader, which used to get host and port only. conformance_catalog's open accepts the same options, so the new CPU suite can drive the client. Evidence: tests/test_native_catalog_connection.py (CPU, a recording fake ClickHouse and a TLS fake on a CA generated per session with the openssl CLI) failed 55 of 58 before this change; the 56 kept in this commit pass. Of the three that passed before, two only show the old plain-http client could not complete a TLS handshake at all, and one that the old driver ignored the host it was given. The existing live suites pass unchanged, the lease byte-identity tests included; the one local failure needs a CREATE USER grant the local server's default user lacks.
…nfig
The native client can now reach a secured ClickHouse, but a user had no way
to ask it to: NativeCaptureStorageConfig carried only host, port and
timeouts, and NativeCaptureReader shared the writer's everything.
New fields, validated at construction with the field named in the error:
clickhouse_scheme ("http"/"https"), clickhouse_user, clickhouse_password
(repr=False), clickhouse_ca_file, clickhouse_ca_path,
clickhouse_allow_insecure_http, and an optional reader account,
clickhouse_reader_user / clickhouse_reader_password (repr=False), which
NativeCaptureReader uses in place of the writer's; the service never sees
it. The rules mirror the native client's so a mistake is caught before any
native object exists: a password over http needs the opt-in, the opt-in is
refused with https, a CA needs https, a password needs its user, the host is
a bare host (userinfo refused without echoing it), and no credential may
hold CR, LF or NUL, since each travels as a header.
clickhouse_request_timeout_s must now be at least 10 s: a publish runs
server-side for up to the writer's 5 s publish timeout, and a client that
gives up sooner reports an unknown outcome and quarantines the writer over a
statement that may have committed. The check lives here, not in the native
config, because the live suites deliberately drive the binding with 2-5 s
timeouts against a stalled catalog.
Evidence: 35 new cases in test_native_capture_storage_wiring.py, 29 of which
failed before this change (the other 6 passed only because an unknown
keyword or an unchecked host raised or passed for the wrong reason); two
connection tests show the reader account reaching the wire and the service
keeping the writer's. A new live test runs the whole path -- schema, lease,
index, publish, search, resolve, hydrate -- over https through a TLS
terminator with a per-test CA in front of the local ClickHouse, reads back
byte-identical payloads with the CA as a file and as a hashed directory,
and is refused with a certificate error without it. The live suites:
218 passed, 1 failed (the known local CREATE USER grant).
The live TLS round trip failed once in a full live run with failed_handshakes == 0: the terminator counts a refused handshake on its own thread, and the client can report the certificate error and return before that thread has seen the alert. The CPU suite's untrusted-CA test asserts the same counter right after the client returns and carries the same race. Both now poll the counter for up to 5 s before asserting it, which keeps the assertion (exactly one handshake, refused, never retried) and drops the ordering assumption. Stressed after the change: the live test 8 of 8 runs, the CPU connection suite 10 of 10 runs (58 passed each).
…t 500s fail once Review findings on the catalog client's retry rules: * A statement opening with WITH counted as a read, but ClickHouse parses `WITH 1 AS x INSERT INTO t SELECT x` as an INSERT, which then got wait_end_of_query=1 and a retry after a reset or 5xx. A WITH statement is now a read only if the word INSERT appears nowhere in it (conservative: misjudging a read as a write only costs its retries). * ClickHouse answers HTTP 500 for permanent errors too (TOO_MANY_ROWS, ACCESS_DENIED, READONLY, throwIf, ILLEGAL_TYPE_OF_ARGUMENT), so reads were repeated up to max_attempts for nothing -- a row limit rescanned up to the limit three times. The client now reads the X-ClickHouse-Exception-Code header (or a "Code: N." body) and retries a read's 5xx only for a short allowlist of transient codes, names checked with errorCodeToName on 25.12. A 5xx naming no ClickHouse error (a proxy's 502/503/504) is retried as before; writes are unchanged. * A positive timeout below 1 ms truncated to 0, which libcurl reads as "its default": no bound on the whole request. Timeouts now round up to whole milliseconds. * The refused-connection test could pass without a retry if the first attempt landed after listen(). execute() can report its attempt count and the driver returns it, so the test now asserts a retry happened. * Comments: CURLOPT_CAINFO/CAPATH replace libcurl's built-in default for that option rather than adding to it, and the credential headers reach an environment-configured proxy over plain http.
The reader always sends its query limits (max_rows_to_read, max_execution_time, ...) as settings, so an account made read-only with a readonly=1 profile fails every read with Code 164 (READONLY). The docs and the config comment now say to limit it with GRANT SELECT or a readonly=2 profile. They also stop claiming clickhouse_ca_file/ca_path add to the system roots: each replaces libcurl's built-in default for that option, and whether the system roots survive depends on the libcurl build.
zaoxing
force-pushed
the
feat/catalog-client-auth-tls
branch
from
September 24, 2026 22:59
76f8eab to
6d7c6c0
Compare
Conflicts were side-by-side additions: helpers in native_capture.py and new tests appended at the ends of the storage wiring and live test files; both sides kept. One semantic fix: this branch required clickhouse_request_timeout_s to be at least twice a fixed 5 s _PUBLISH_TIMEOUT_S, written when the config did not expose the publish timeout. #150 made publish_timeout_s a config field, so the rule is now twice the configured publish_timeout_s, checked after the lease fields are validated; the integration doc says so, and a wiring test pins it (a 7 s publish cap needs a 14 s request timeout).
zaoxing
added a commit
that referenced
this pull request
Sep 26, 2026
One conflict, in src/dmi/storage/native_capture.py: this branch added NativeSinkConfig where main added the connection and lease validation helpers; both kept, helpers first. On the merged tree: pytest -m cpu 2520 passed; the capture storage, catalog lease, capture chain and reader parity live suites 126 passed. The ring, sink and engine code is identical to 52b9627, which passed the GPU suites (test_ring_engine 174/174, test_record_failure_policy_gpu 8/8).
zaoxing
added a commit
that referenced
this pull request
Sep 28, 2026
#150 was squash-merged as 7419fd0, and main then took #151, #152 and #139. The specs cited #150's head c0361d7; they now cite main at 71be2af. - storage_service.cpp moved up two lines (#151 builds the ClickHouse client from one ClickHouseConnection), native_capture.py moved with #149/#151/#152, and deciding_read() is now clickhouse_client.cpp:374. - #151 also made execute() retry a read after a transient failure, up to max_attempts (3 by default), and never a write that may have reached the server. The README's O1 caveat, its Limitations entry and LIMITS 3 in LeaseLifecycle.tla now say a lease request's reads can take up to three request timeouts, and RenewIfDue says the quarantining exception is the first to outlast those retries. No modelled outcome changes. - Refs that missed the code they describe, in files main did not change: the O1a quote is storage_service.h:233-234, not storage_service.cpp; the renewal in publish_snapshot is catalog_writer.cpp:490 and :579; publish_snapshot ends at :669; the config check with the quorum rule is :148-169; the chunk loop is :531; the watermark read-back is :608-628 (:611-627 for its refusal); the version allocator's statement lines; reject_live's comparison is lease_coordinator.cpp:222; and the start wait's knob checks are native_capture.py:351-354. - The README says what 204a8d2 is now that #150's branch is squashed. Comment and prose changes only; every verdict is unchanged.
This was referenced Sep 28, 2026
This was referenced Sep 28, 2026
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.
Milestone B4 of the native capture production plan. DMI's native capture path can now connect to a ClickHouse catalog that requires a password and TLS. It also bounds retries, so a write is never repeated.
What changes
One connection type shared by all callers.
ClickHouseConnection(innative/csrc/catalog/clickhouse_client.{h,cpp}) is used by the storage service, the capture reader and theconformance_catalogdriver.X-ClickHouse-User/X-ClickHouse-Keyheaders. They never go in the URL, and they never appear in errors orrepr(). CR, LF and NUL are refused, and so is a password with no user.ca_file) or a directory (ca_path).CURLOPT_CAINFO/CAPATHreplace libcurl's built-in default, so whether the system roots survive depends on the libcurl build. Debian/Ubuntu builds keep them.allow_insecure_http=True, even on loopback.?,#and whitespace are all refused. Bracketed IPv6 is accepted.WITH … INSERTcounts as a write.wait_end_of_query=1is added for reads only, as a URL parameter. Statement bytes are unchanged, so the lease byte-identity tests pass without edits.Python.
NativeCaptureStorageConfiggains:clickhouse_schemeclickhouse_userclickhouse_password(repr=False)clickhouse_ca_fileclickhouse_ca_pathclickhouse_allow_insecure_httpValidation matches the C++ rules, and the request timeout must be at least 10 s because of the publish and quorum timeouts. The reader account should be limited with
GRANT SELECTor areadonly=2profile.readonly=1refuses the query-limit settings that every read sends.Nothing under
src/dmi/storage/capture/changed.Evidence
pytest -m cpugave 2403 passed, 0 skipped.tests/test_native_catalog_connection.py: 68 fake-server tests covering credentials, TLS, host validation and retry classification.wait_end_of_queryon every statement, and ignored the CA file. Each change turned at least one test red.Independent review
The verdict was ship after minor fixes, with no majors. All the findings are fixed in a692ce0 and 76f8eab:
WITH … INSERTcounted as a read, so it could run twice. It now counts as a write.readonly=1account fails. The docs now sayGRANT SELECT/readonly=2.http_proxyreceives the credential headers;attemptscount.Not in this PR