Repository navigation
feat: support mtls for query api - #254
NguyenHoangSon96 wants to merge 5 commits into
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
New required constructor parameters break existing non-mTLS callers, while invalid TLS certificate configurations prevent plaintext mock servers from initializing.
5 open issues
Keep new options optional and preserve positional argument compatibility · New Pass None for TLS certificates when no certificate is supplied · New Preserve private keys supplied through client options · New Clean up each HTTP server after its test iteration · New Skip the Polars test when the optional dependency is unavailable · New
What changed in this PR
This PR adds mutual TLS (mTLS) support to Arrow Flight queries, extending the client’s certificate settings to the query API.
Changes:
- Pass client certificates and private keys into Flight options, including bundled PEM support.
- Add TLS and mTLS query/write tests and TLS-capable mock servers.
- Add certificate fixtures and a script to regenerate them.
| File | Description |
|---|---|
tests/util/mocks.py |
Adds TLS configuration to the Flight mock server. |
tests/test_influxdb_client_3.py |
Adds TLS and mTLS success and failure cases. |
tests/data/certificates/other-server.pem |
Bundled credentials for rejection tests. |
tests/data/certificates/other-server.key |
Private key for untrusted test credentials. |
tests/data/certificates/other-server.crt |
Certificate for rejection tests. |
tests/data/certificates/influxdb.pem |
Bundled server certificate and key. |
tests/data/certificates/influxdb.key |
Private key for TLS test servers. |
tests/data/certificates/influxdb.crt |
Certificate for trusted TLS test servers. |
tests/data/certificates/client.pem |
Bundled client credentials for mTLS tests. |
tests/data/certificates/client.key |
Client private key for mTLS tests. |
tests/data/certificates/client.crt |
Client certificate for mTLS tests. |
scripts/generate-self-signed-cert.sh |
Generates self-signed certificate fixtures. |
influxdb_client_3/query/query_api.py |
Adds certificate options and forwards credentials to Flight. |
influxdb_client_3/__init__.py |
Connects client certificate settings to query options. |
Review effort: Balanced
6041d64 to
0d280ff
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #254 +/- ##
==========================================
+ Coverage 89.45% 89.79% +0.33%
==========================================
Files 28 28
Lines 2049 2096 +47
==========================================
+ Hits 1833 1882 +49
+ Misses 216 214 -2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Note
Copilot was unable to run its full agentic suite in this review.
Copilot review overview
9 open issues
create_write_secure_serverexplicitly starts the HTTP server (server.start()), but the query… · New This creates atls_certificatesentry even when only one ofserver_certorserver_cert_keyis… · New Defaultingprivate_keytocert_chaincan silently misconfigure mTLS whencert_chaincontains… · New This script uses pipelines (e.g.,openssl genpkey ... | openssl pkcs8 ...). With onlyset -e,… · New Private keys are being committed into the repository undertests/data/certificates. Even if these… · New These assertions assume the thrown exception has a.reasonattribute, which is not guaranteed for… · New Similar to the write test, this assumes an exception.messageattribute, which is not a stable… · Newraise eresets the traceback context. Use a bareraiseto preserve the original stack trace… · New The comment describingdo_getis currently indented inside__init__, which is misleading (it… · New
5 resolved since last review
Pass None for TLS certificates when no certificate is supplied Keep new options optional and preserve positional argument compatibility Skip the Polars test when the optional dependency is unavailable Clean up each HTTP server after its test iteration Preserve private keys supplied through client options
Review effort: Lite
| @@ -0,0 +1,28 @@ | |||
| -----BEGIN PRIVATE KEY----- | |||
cf0ffc5 to
5fa6b54
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The query path ignores cert_key_password, preventing mTLS authentication with encrypted private keys, while the SSL example unconditionally loads placeholder certificate paths and fails with FileNotFoundError.
3 open issues
8 resolved since last review
This creates atls_certificatesentry even when only one ofserver_certorserver_cert_keyis…create_write_secure_serverexplicitly starts the HTTP server (server.start()), but the query… Similar to the write test, this assumes an exception.messageattribute, which is not a stable… These assertions assume the thrown exception has a.reasonattribute, which is not guaranteed for… This script uses pipelines (e.g.,openssl genpkey ... | openssl pkcs8 ...). With onlyset -e,… Defaultingprivate_keytocert_chaincan silently misconfigure mTLS whencert_chaincontains… The comment describingdo_getis currently indented inside__init__, which is misleading (it…raise eresets the traceback context. Use a bareraiseto preserve the original stack trace…
Review effort: Balanced
14ae8e7 to
687bf95
Compare
9c5d659 to
136b1ca
Compare



Closes #
Proposed Changes
Checklist