Repository navigation
fix(storage): plumb custom CA and NO_PROXY into obstore - #2627
Merged
Merged
Conversation
louisnow
requested review from
Aryamanz29,
OnkarVO7,
atlan-ci,
cmgrote and
vaibhavatlan
as code owners
July 10, 2026 10:31
Contributor
📜 Docstring Coverage ReportRESULT: PASSED (minimum: 30.0%, actual: 78.1%) Detailed Coverage ReportThis message was truncated. Download full message |
Contributor
📦 Trivy Vulnerability Scan Results
Report Summary
Scan Result Detailspackages/conformance/uv.lockrequirements.txtuv.lock |
Contributor
📦 Trivy Secret Scan Results
Report Summary
Scan Result Detailspackages/conformance/uv.lockrequirements.txtuv.lock |
Collaborator
☂️ Code Coverage
Overall Coverage
New FilesNo new covered files... Modified FilesNo covered modified files...
|
louisnow
marked this pull request as draft
July 10, 2026 10:54
louisnow
force-pushed
the
feat/obstore-custom-ca-and-no-proxy
branch
from
July 10, 2026 10:56
617c00c to
8f7bf34
Compare
louisnow
changed the title
louisnow
force-pushed
the
feat/obstore-custom-ca-and-no-proxy
branch
6 times, most recently
from
July 10, 2026 11:13
6d0adbb to
c04dc9e
Compare
Object-store traffic behind a private CA should have verified from the start, and NO_PROXY should have been honored — obstore just lacked the client options until 0.11.0, which this repo already pins. Wire both from existing config: - root_certificate: sourced from SSL_CERT_DIR (get_custom_ca_cert_bytes, the same helper the Temporal client uses) so a private-CA-fronted store verifies without disabling checks. obstore adds these roots to the system trust store rather than replacing them, so public endpoints keep validating. Completes SSL_CERT_DIR parity across httpx/aiohttp, Temporal, and obstore. - proxy_excludes: honor NO_PROXY when a proxy URL is set, retiring the prior 'NO_PROXY isn't honored' gap. root_certificate PEM bytes are collapsed to a byte-count in config logs.
louisnow
force-pushed
the
feat/obstore-custom-ca-and-no-proxy
branch
from
July 10, 2026 11:28
c04dc9e to
7a81410
Compare
Contributor
Author
|
@sdk-review |
Contributor
|
🔍 SDK Review (mothership) triggered by @louisnow at 2026-07-10T11:29:25.047Z. Watch the workflow run live — the review summary will appear as a separate comment when complete (typical: 5–30 min, hard cap 2h). 🟥 Run ended without a clean completion — status |
Contributor
SDK Review (mothership): PR #2627 — fix(storage): plumb custom CA and NO_PROXY into obstoreVerdict: READY TO MERGE
Findings
Strengths
CI: all passing (2 Windows unit-test jobs still in progress at review time; no failures) |
atlan-ci
approved these changes
Jul 10, 2026
atlan-ci
left a comment
atlan-ci
left a comment
Collaborator
There was a problem hiding this comment.
SDK reviewer's verdict: READY TO MERGE.
Full review summary is in the comment posted on this PR.
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.
Object storage fronted by a private CA (self-hosted MinIO, an S3-compatible API behind a corp proxy) failed TLS verification, and
NO_PROXYwas ignored. Both should have worked all along — obstore simply didn't expose the client options until 0.11.0, which this repo already pins. This wires them in from config the SDK already reads:root_certificate— custom CAs fromSSL_CERT_DIR, so private-CA stores verify withoutallow_invalid_certificates. Reusesget_custom_ca_cert_bytes(), the same helper the Temporal client already uses, so all TLS clients (httpx/aiohttp, Temporal, obstore) now honorSSL_CERT_DIRconsistently.proxy_excludes—NO_PROXY, forwarded only when a proxy is set.obstore adds
root_certificateto the system trust store rather than replacing it (each PEM in the bundle goes through reqwest'sadd_root_certificate; built-in roots stay in effect), so public endpoints keep validating alongside the custom CA. Config logs showroot_certificateas a byte count, not the raw PEM.Tested with unit tests for both paths plus a real
S3Storebuilt with a trustme CA to confirm obstore accepts the keys.