feat(client): add ssl context cache and config #581
Conversation
WalkthroughBaseClient now loads TLS context early in init and passes ConfigBase.tls_context as the HTTP client's verification parameter (verify=self.config.tls_context) for both async and sync request paths. ConfigBase gained a private _ssl_context, a tls_context property that lazily creates or returns an ssl.SSLContext, and set_ssl_context(context) to set it; Config.clone propagates the SSL context. Tests were added covering TLS context caching, CA-file behavior, tls_insecure handling, and custom TLS context usage. The package public exports now include Config. Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Deploying infrahub-sdk-python with
|
| Latest commit: |
3898abb
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://a3391749.infrahub-sdk-python.pages.dev |
| Branch Preview URL: | https://fac-ssl-context.infrahub-sdk-python.pages.dev |
Codecov Report❌ Patch coverage is
@@ Coverage Diff @@
## stable #581 +/- ##
==========================================
+ Coverage 75.69% 75.80% +0.11%
==========================================
Files 100 100
Lines 8894 8918 +24
Branches 1751 1758 +7
==========================================
+ Hits 6732 6760 +28
+ Misses 1680 1677 -3
+ Partials 482 481 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1 file with indirect coverage changes 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
tests/unit/sdk/test_client.py (3)
32-48: Extend test coverage to sync client.The test correctly validates default SSL context caching. However, both
InfrahubClient(async) andInfrahubClientSyncuse the same_get_verify_configmethod fromBaseClient, so consider adding a similar test for the sync client to ensure consistent behavior across both implementations.
51-73: Extend test coverage to sync client.The test correctly validates CA file-based SSL context caching and cache invalidation when the file path changes. Consider adding similar test coverage for
InfrahubClientSyncto ensure consistent behavior.
76-86: Add test coverage for custom SSL context and sync client.The test correctly validates that
tls_insecure=Truebypasses SSL context creation. However:
- Consider adding similar test coverage for
InfrahubClientSync.- No test validates that a custom
tls_ssl_contextprovided in the config is actually used by_get_verify_config.As per learnings: Use the custom pytest plugin (infrahub_sdk.pytest_plugin) fixtures for clients, configuration, and Infrahub-specific testing.
infrahub_sdk/client.py (2)
147-148: Consider thread safety for cache attributes.The cache attributes
_cached_ssl_contextand_cached_ssl_context_sourceare not thread-safe. While this is typically fine forInfrahubClient(async), theInfrahubClientSynccould be used in multi-threaded environments where concurrent access to these attributes might cause race conditions (e.g., one thread invalidating the cache while another is reading it).
156-177: Improve error handling for invalid CA files.The method correctly implements SSL context caching with appropriate priority (custom context → insecure → CA file → default). However, consider:
Error handling: If
tls_ca_filepoints to an invalid or non-existent file,create_default_context(cafile=...)will raise an exception. Consider wrapping this in a try-except to provide a clearer error message.Defensive checks: The RuntimeError checks on lines 168 and 176 seem unnecessary since
create_default_context()typically doesn't return None. If you want to keep them for defensive programming, clarify the rationale in a comment.Documentation: The priority order isn't documented in the Config class. Consider adding a note in the docstring of
tls_ssl_contextfield explaining when it takes precedence.Example error handling:
if self.config.tls_ca_file: if self._cached_ssl_context is None or self._cached_ssl_context_source != self.config.tls_ca_file: try: self._cached_ssl_context = create_default_context(cafile=self.config.tls_ca_file) self._cached_ssl_context_source = self.config.tls_ca_file except (FileNotFoundError, PermissionError) as exc: raise RuntimeError(f"Failed to load CA file '{self.config.tls_ca_file}': {exc}") from exc if self._cached_ssl_context is None: raise RuntimeError("Failed to create SSL context using provided CA file") return self._cached_ssl_context
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
infrahub_sdk/client.py(4 hunks)infrahub_sdk/config.py(2 hunks)tests/unit/sdk/test_client.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
When implementing Infrahub checks, subclass InfrahubCheck and override validate(data); do not implement or rely on a check() method
Files:
infrahub_sdk/client.pyinfrahub_sdk/config.pytests/unit/sdk/test_client.py
infrahub_sdk/client.py
📄 CodeRabbit inference engine (CLAUDE.md)
infrahub_sdk/client.py: Use HTTPX for transport with proxy support (single proxy or HTTP/HTTPS mounts)
Support authentication via API tokens or JWT with automatic refresh
Files:
infrahub_sdk/client.py
infrahub_sdk/config.py
📄 CodeRabbit inference engine (CLAUDE.md)
infrahub_sdk/config.py: Use a Pydantic-based Config class with environment variable support for configuration
Environment variables for configuration must use the INFRAHUB_ prefix
Validate mutual exclusivity between INFRAHUB_PROXY and {INFRAHUB_PROXY_MOUNTS_HTTP, INFRAHUB_PROXY_MOUNTS_HTTPS}
Files:
infrahub_sdk/config.py
tests/unit/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Place and write unit tests under tests/unit/ (isolated component tests)
Files:
tests/unit/sdk/test_client.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use the custom pytest plugin (infrahub_sdk.pytest_plugin) fixtures for clients, configuration, and Infrahub-specific testing
Files:
tests/unit/sdk/test_client.py
🧠 Learnings (1)
📚 Learning: 2025-08-24T13:35:12.998Z
Learnt from: CR
PR: opsmill/infrahub-sdk-python#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-24T13:35:12.998Z
Learning: Applies to tests/**/*.py : Use the custom pytest plugin (infrahub_sdk.pytest_plugin) fixtures for clients, configuration, and Infrahub-specific testing
Applied to files:
tests/unit/sdk/test_client.py
🧬 Code graph analysis (2)
infrahub_sdk/client.py (1)
infrahub_sdk/context.py (1)
RequestContext(10-13)
tests/unit/sdk/test_client.py (2)
infrahub_sdk/client.py (16)
InfrahubClient(318-1607)get(390-406)get(409-425)get(428-444)get(447-463)get(466-482)get(485-501)get(503-561)get(2107-2123)get(2126-2142)get(2145-2161)get(2164-2180)get(2183-2199)get(2202-2218)get(2220-2278)_get_verify_config(156-177)tests/unit/sdk/conftest.py (1)
client(32-33)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: unit-tests (3.11)
- GitHub Check: unit-tests (3.12)
- GitHub Check: unit-tests (3.13)
- GitHub Check: unit-tests (3.10)
- GitHub Check: unit-tests (3.9)
- GitHub Check: integration-tests-latest-infrahub
- GitHub Check: Cloudflare Pages
🔇 Additional comments (5)
infrahub_sdk/config.py (1)
4-4: LGTM!The import is correctly added to support the new SSLContext type in the configuration.
tests/unit/sdk/test_client.py (1)
6-6: LGTM!The import correctly uses the public API by importing
Configfrominfrahub_sdk, aligning with the module's public exports.infrahub_sdk/client.py (3)
10-10: LGTM!The imports are correctly added to support SSL context creation and type annotations.
1053-1053: LGTM!The change correctly uses the centralized
_get_verify_config()method for TLS verification, providing consistent behavior and caching across all HTTP requests.
2777-2777: LGTM!The change correctly uses the centralized
_get_verify_config()method, ensuring the sync client has the same TLS verification and caching behavior as the async client.
fatih-acar
force-pushed
the
fac-ssl-context
branch
from
ae4c8af to
4ff80bd
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
tests/unit/sdk/test_data/path-1.pemis excluded by!**/*.pemtests/unit/sdk/test_data/path-2.pemis excluded by!**/*.pem
📒 Files selected for processing (3)
infrahub_sdk/client.py(3 hunks)infrahub_sdk/config.py(4 hunks)tests/unit/sdk/test_client.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- infrahub_sdk/config.py
- infrahub_sdk/client.py
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
When implementing Infrahub checks, subclass InfrahubCheck and override validate(data); do not implement or rely on a check() method
Files:
tests/unit/sdk/test_client.py
tests/unit/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Place and write unit tests under tests/unit/ (isolated component tests)
Files:
tests/unit/sdk/test_client.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use the custom pytest plugin (infrahub_sdk.pytest_plugin) fixtures for clients, configuration, and Infrahub-specific testing
Files:
tests/unit/sdk/test_client.py
🧬 Code graph analysis (1)
tests/unit/sdk/test_client.py (2)
infrahub_sdk/config.py (2)
Config(164-206)tls_context(139-158)infrahub_sdk/client.py (14)
get(365-381)get(384-400)get(403-419)get(422-438)get(441-457)get(460-476)get(478-536)get(2082-2098)get(2101-2117)get(2120-2136)get(2139-2155)get(2158-2174)get(2177-2193)get(2195-2253)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: integration-tests-latest-infrahub
🔇 Additional comments (5)
tests/unit/sdk/test_client.py (5)
2-2: LGTM!The import additions are appropriate. Path is needed for the CURRENT_DIRECTORY constant, and Config is now part of the public API as intended by this PR.
Also applies to: 7-7
32-33: LGTM!Standard pattern for defining the test directory path.
83-94: LGTM!The test correctly verifies that when
tls_insecure=True, no SSL context is created andFalseis returned for httpx'sverifyparameter. The use ofAssertionErrorin the fake ensures the test fails if context creation is attempted.
35-52: Monkeypatch target is correctConfig imports
ssldirectly, so patchingssl.create_default_contextworks as intended and the caching test requires no changes.
54-81: Clarify TLS context caching behavior when updating tls_ca_file. The test confirms that changingtls_ca_fileafter first access does not recreate the SSL context, matching the current implementation. If this is intended, document the behavior; otherwise, clear the cached context when the CA file is updated.
fatih-acar
force-pushed
the
fac-ssl-context
branch
2 times, most recently
from
9f288e7 to
2623336
Compare
|
@ogenstad thank you for the relevant suggestions, it's much better now. |
There was a problem hiding this comment.
LGTM, I added a suggestion along with a question about when we're returning False for tls_insecure.
Also can you please add a newsfragment?
One change in behaviour could be that when we move to creating the SSL context like we do that the behaviour gets slightly modified with regards to situations when we use the CA file as an environment variable.
Within Infrahub we have this code, it's quite hacky but you see that the parameters to the SSL context is different if we're loading a file cafile vs when we are loading a string from an environment variable cadata.
tls_ca_path = Path(self.tls_ca_bundle)
try:
possibly_file = tls_ca_path.exists()
except OSError:
# Raised if the filename is too long which can indicate
# that the value is a PEM certificate in string form.
possibly_file = False
if possibly_file and tls_ca_path.is_file():
context = ssl.create_default_context(cafile=str(tls_ca_path))
else:
context = ssl.create_default_context()
context.load_verify_locations(cadata=self.tls_ca_bundle)It might be that HTTPX manages this part differently internally (or it was never tested).
Can you please verify with your branch the behaviour if you do a:
export INFRAHUB_TLS_CA_FILE=PEM-CERT-AS-STRINGI thought we had tests for that in the backend but now see that we don't seem to have that :(
This allows configuring a custom SSL context or cache a default one if not supplied instead of re-creating a default ssl context on each request (default httpx behavior). Signed-off-by: Fatih Acar <fatih@opsmill.com>
fatih-acar
force-pushed
the
fac-ssl-context
branch
from
2623336 to
3898abb
Compare
I think this is expected. Checking the infrahub code, I see that |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
infrahub_sdk/config.py (1)
149-155: Consider using elif/else for clearer control flow.The current implementation uses sequential
ifstatements where the second condition (line 152) only evaluates true when the first condition (line 149) is false. Usingelifandelsewould make this fallback logic more explicit.Apply this diff to clarify the control flow:
if self.tls_ca_file: self._ssl_context = ssl.create_default_context(cafile=self.tls_ca_file) - - if self._ssl_context is None: + else: self._ssl_context = ssl.create_default_context() return self._ssl_context
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
tests/unit/sdk/test_data/path-1.pemis excluded by!**/*.pemtests/unit/sdk/test_data/path-2.pemis excluded by!**/*.pem
📒 Files selected for processing (3)
infrahub_sdk/client.py(3 hunks)infrahub_sdk/config.py(4 hunks)tests/unit/sdk/test_client.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- infrahub_sdk/client.py
- tests/unit/sdk/test_client.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
When implementing Infrahub checks, subclass InfrahubCheck and override validate(data); do not implement or rely on a check() method
Files:
infrahub_sdk/config.py
infrahub_sdk/config.py
📄 CodeRabbit inference engine (CLAUDE.md)
infrahub_sdk/config.py: Use a Pydantic-based Config class with environment variable support for configuration
Environment variables for configuration must use the INFRAHUB_ prefix
Validate mutual exclusivity between INFRAHUB_PROXY and {INFRAHUB_PROXY_MOUNTS_HTTP, INFRAHUB_PROXY_MOUNTS_HTTPS}
Files:
infrahub_sdk/config.py
🧠 Learnings (1)
📚 Learning: 2025-08-24T13:35:12.998Z
Learnt from: CR
PR: opsmill/infrahub-sdk-python#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-24T13:35:12.998Z
Learning: Applies to infrahub_sdk/config.py : Environment variables for configuration must use the INFRAHUB_ prefix
Applied to files:
infrahub_sdk/config.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: unit-tests (3.13)
- GitHub Check: unit-tests (3.12)
- GitHub Check: unit-tests (3.10)
- GitHub Check: unit-tests (3.11)
- GitHub Check: integration-tests-latest-infrahub
- GitHub Check: Cloudflare Pages
🔇 Additional comments (4)
infrahub_sdk/config.py (4)
3-3: LGTM! Import additions are appropriate.The
sslmodule andPrivateAttrimports are necessary for the SSL context caching feature. UsingPrivateAttrcorrectly addresses the serialization concerns raised in previous reviews.Also applies to: 7-7
82-82: LGTM! Correct use of PrivateAttr.The private attribute correctly addresses the serialization and environment variable concerns from previous reviews. The underscore prefix and
PrivateAttrensure this field won't be exposed through Pydantic's serialization or environment variable mechanisms.
157-158: LGTM! Simple and correct setter method.The
set_ssl_contextmethod provides a clean interface for setting a custom SSL context, which is useful for advanced use cases where the user needs full control over TLS configuration.
201-204: LGTM! SSL context is correctly propagated to cloned config.The
clonemethod properly transfers the cached SSL context to the new instance using theset_ssl_contextmethod, ensuring consistent TLS configuration across cloned configs.
| if self.tls_insecure: | ||
| self._ssl_context = ssl.SSLContext(ssl.PROTOCOL_TLS_CLIENT) | ||
| self._ssl_context.check_hostname = False | ||
| self._ssl_context.verify_mode = ssl.CERT_NONE | ||
| return self._ssl_context | ||
|
|
||
| if self.tls_ca_file: | ||
| self._ssl_context = ssl.create_default_context(cafile=self.tls_ca_file) |
There was a problem hiding this comment.
Validate mutual exclusivity of tls_insecure and tls_ca_file.
When tls_insecure=True, the tls_ca_file is silently ignored (lines 149-150 are never reached). This could confuse users who set both options, expecting the CA file to be used.
Consider adding a model validator to enforce mutual exclusivity:
+ @model_validator(mode="after")
+ def validate_tls_config(self) -> Self:
+ if self.tls_insecure and self.tls_ca_file:
+ raise ValueError("'tls_insecure' and 'tls_ca_file' are mutually exclusive")
+ return self
+
@property
def tls_context(self) -> ssl.SSLContext:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if self.tls_insecure: | |
| self._ssl_context = ssl.SSLContext(ssl.PROTOCOL_TLS_CLIENT) | |
| self._ssl_context.check_hostname = False | |
| self._ssl_context.verify_mode = ssl.CERT_NONE | |
| return self._ssl_context | |
| if self.tls_ca_file: | |
| self._ssl_context = ssl.create_default_context(cafile=self.tls_ca_file) | |
| @model_validator(mode="after") | |
| def validate_tls_config(self) -> Self: | |
| if self.tls_insecure and self.tls_ca_file: | |
| raise ValueError("'tls_insecure' and 'tls_ca_file' are mutually exclusive") | |
| return self | |
| @property | |
| def tls_context(self) -> ssl.SSLContext: | |
| if self.tls_insecure: | |
| self._ssl_context = ssl.SSLContext(ssl.PROTOCOL_TLS_CLIENT) | |
| self._ssl_context.check_hostname = False | |
| self._ssl_context.verify_mode = ssl.CERT_NONE | |
| return self._ssl_context | |
| if self.tls_ca_file: | |
| self._ssl_context = ssl.create_default_context(cafile=self.tls_ca_file) |
🤖 Prompt for AI Agents
In infrahub_sdk/config.py around lines 143 to 150, the code silently ignores
tls_ca_file when tls_insecure=True; add a model-level validation that enforces
mutual exclusivity by checking both attributes after initialization and raising
a clear ValueError (or ValidationError) if both tls_insecure and tls_ca_file are
provided; implement this as a pydantic @root_validator (or equivalent) in the
Config model so users receive a deterministic error instead of having
tls_ca_file silently ignored.
| if self.tls_ca_file: | ||
| self._ssl_context = ssl.create_default_context(cafile=self.tls_ca_file) |
There was a problem hiding this comment.
Validate tls_ca_file exists before passing to SSL context.
Line 150 passes tls_ca_file to ssl.create_default_context without verifying the file exists. This will raise a runtime error with a potentially unclear message if the file is missing or inaccessible.
Add a field validator to check file existence:
+ @field_validator("tls_ca_file")
+ @classmethod
+ def validate_tls_ca_file(cls, value: str | None) -> str | None:
+ if value is not None:
+ from pathlib import Path
+ if not Path(value).is_file():
+ raise ValueError(f"TLS CA file not found: {value}")
+ return value
+
@property
def tls_context(self) -> ssl.SSLContext:🤖 Prompt for AI Agents
In infrahub_sdk/config.py around lines 149-150, the code passes self.tls_ca_file
into ssl.create_default_context without verifying the path exists; add a
validator that checks tls_ca_file is not None and points to an existing,
readable file (use pathlib.Path(self.tls_ca_file).is_file() or os.path.exists +
os.access), raise a clear ValueError with a helpful message if the file is
missing or inaccessible, and only call ssl.create_default_context(cafile=...)
when the validator passes. Ensure the validator runs during config
construction/validation so runtime ssl.create_default_context is never called
with an invalid path.
Ah yes the option is named differently. I think a problem here is the description of tls_ca_file: str | None = Field(default=None, description="File path to CA cert or bundle in PEM format")This is a case where the oxford comma would have been good as you can read this in a number of different ways. Let's ignore this for now since it's outside of the scope of your PR but I think we should clean it up a bit. For consistency we should perhaps start to replace |
This allows configuring a custom SSL context or cache a default one if not supplied instead of re-creating a default ssl context on each request (default httpx behavior).
Summary by CodeRabbit
New Features
Refactor
Bug Fixes