Fix Paimon DLF request signing: sign every request, cache only credentials#111611
Fix Paimon DLF request signing: sign every request, cache only credentials#111611alesapin wants to merge 2 commits into
Conversation
…tials The DLF v4 `Authorization` header was computed once for the first request (`GET /v1/paimon/config` at `CREATE DATABASE` time), cached in `PaimonToken::dlf_generated_authorization`, and reused for every subsequent request. The signature covers the canonical request (method, resource path, query parameters and signed headers), so a cached value is invalid for any other request, and the DLF server answers 401 to e.g. `GET /v1/paimon/databases?maxResults=100`. The retry that was supposed to recover never fired because it compared `HTTPException::code` (the ClickHouse error code, 86) with the HTTP status 401. The bug stayed invisible because table listing swallows catalog errors, so `SHOW TABLES` just returned an empty list. Follow the reference implementation (`DLFDefaultSigner` in Apache Paimon): compute the signature for every request anew and cache only the credentials, which are already stored in `PaimonToken`. Drop the cache (which was also a data race: a `mutable` string written without synchronization from concurrent listing threads) and the dead retry. Also fix the same `code`-vs-HTTP-status confusion in `existsTable` and `tryGetTableMetadata`: HTTP 404 there was meant to map to "table does not exist" but was rethrown instead, because `e.code()` is never 404. Extend `test_paimon_rest_catalog` to assert that listing, `DESC` and `SELECT` work through the DLF-authenticated catalog: the DLF mock server shares the warehouse with the bearer one, so the results must match. Without the fix the listing silently returns an empty result. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Workflow [PR], commit [f20f511] AI ReviewSummaryThis PR fixes Paimon DLF request signing by recomputing the Findings
Final Verdict
|
| # queries verify that signing works for requests other than the initial | ||
| # config one made at CREATE DATABASE time. | ||
| assert ( | ||
| node.query("SHOW TABLES;", database="paimon_rest_db_dlf") |
There was a problem hiding this comment.
This regression test proves the per-request DLF signing fix on successful catalog calls, but it does not cover the second behavior changed in this PR: existsTable and tryGetTableMetadata now treat HTTP 404 as “table missing” via getHTTPStatus() instead of rethrowing. None of the new assertions hits that path, so CI would still stay green if one of those call sites regressed back to e.code() later.
Please add a missing-table assertion through the user-visible surfaces backed by those methods, for example EXISTS TABLE returning 0 and a metadata lookup such as SHOW CREATE TABLE reporting that the table does not exist.
|
I don't understand source logic in original PR. Basically it cached a checksum about some request-specific information like datetime or query parameters. It doesn't make sense to cache it, because next request will have different datetime and likely query params. So checksum will not match and we will issue request again in retry logic. |
|
cc @JiaQiTang98 |
…alog The previous version of the test asserted that the DLF-authenticated database sees the table created through the bearer server. That was wrong: the mock REST servers keep their catalogs in memory (only table data lives on the shared warehouse volume), so the DLF server's catalog is always empty and `SHOW TABLES` legitimately returned an empty list: https://s3.amazonaws.com/clickhouse-test-reports/json.html?PR=111611&sha=0e8694b3be688800c4ae2f829510f51b2e807b81&name_0=PR&name_1=Integration%20tests%20%28amd_asan_ubsan%2C%20flaky%29 Create a database and a table on the DLF server through its REST API instead, signing the requests with a Python port of `DLFAuthSignature` from Apache Paimon 1.1.1 (the version the mock server is built with), and assert that listing, `DESC` and `SELECT` through the DLF catalog return the created content. The signer was validated against the C++ implementation by pointing a locally built server at a mock that recomputes and compares the signature the same way the Java server does: all requests matched. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
I don't understand test change :( |
The DLF v4
Authorizationheader for the Paimon REST catalog was computed once for the first request (GET /v1/paimon/configatCREATE DATABASEtime), cached inPaimonToken::dlf_generated_authorization, and reused for every subsequent request. The DLF v4 signature covers the canonical request (method, resource path, query parameters and signed headers), so a cached signature is invalid for any other request, and the server answers 401 to e.g.GET /v1/paimon/databases?maxResults=100. A retry that was supposed to recover never fired because it comparedHTTPException::code(the ClickHouse error code, 86) with the HTTP status 401. The bug stayed invisible because table listing swallows catalog errors, soSHOW TABLESon a DLF-authenticated catalog silently returned an empty list.This follows the reference implementation (
DLFAuthSignaturein Apache Paimon), which computes the signature for every request anew and caches only the credentials — in our case already stored inPaimonToken. The signature cache is removed (it was also a data race: amutablestring written without synchronization from concurrent listing threads) together with the dead retry.The same
code-vs-HTTP-status confusion is fixed inexistsTableandtryGetTableMetadata: HTTP 404 there was meant to map to "table does not exist" but was rethrown instead, becausee.code()is never 404.test_paimon_rest_catalogis extended to create a database and a table on the DLF mock server through its REST API (signing the requests with a Python port ofDLFAuthSignaturefrom Apache Paimon 1.1.1, the version the mock server is built with) and to assert that listing,DESCandSELECTwork through the DLF-authenticated catalog. The mock servers keep their catalogs in memory, so the DLF server needs its own content; without the fix, the listing silently returns an empty result and the metadata requests fail with HTTP 401.Related: #92011
Related: #111379
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):
Fix DLF authentication in the Paimon REST catalog: the request signature was computed once and reused, so every catalog request after the first one failed with HTTP 401 and
SHOW TABLESreturned an empty list. Also fix HTTP 404 detection in table existence checks.🤖 Generated with Claude Code