Merge 'stable' into 'develop' with resolved conflicts. #594
Conversation
Signed-off-by: Fatih Acar <[email protected]>
Use GraphQL instead of graphql
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 <[email protected]>
Fix typing on relogin decorator
WalkthroughThis pull request modifies TLS/SSL context handling throughout the codebase. The Node.js version specification is removed from Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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: |
7b4adf7
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://d79847c5.infrahub-sdk-python.pages.dev |
| Branch Preview URL: | https://pog-stable-to-develop-202510.infrahub-sdk-python.pages.dev |
Codecov Report❌ Patch coverage is
@@ Coverage Diff @@
## develop #594 +/- ##
===========================================
- Coverage 76.05% 75.43% -0.63%
===========================================
Files 108 108
Lines 10604 9325 -1279
Branches 2242 1855 -387
===========================================
- Hits 8065 7034 -1031
+ Misses 1964 1802 -162
+ Partials 575 489 -86
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 13 files 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: 2
📜 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 (4)
.nvmrc(0 hunks)infrahub_sdk/client.py(11 hunks)infrahub_sdk/config.py(4 hunks)tests/unit/sdk/test_client.py(2 hunks)
💤 Files with no reviewable changes (1)
- .nvmrc
🧰 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/config.pytests/unit/sdk/test_client.pyinfrahub_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
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
🧠 Learnings (4)
📚 Learning: 2025-08-24T13:35:12.998Z
Learnt from: CR
Repo: opsmill/infrahub-sdk-python PR: 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
📚 Learning: 2025-08-24T13:35:12.998Z
Learnt from: CR
Repo: opsmill/infrahub-sdk-python PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-24T13:35:12.998Z
Learning: Maintain a dual client pattern: InfrahubClient (async) and InfrahubClientSync (sync) must provide identical interfaces
Applied to files:
tests/unit/sdk/test_client.py
📚 Learning: 2025-08-24T13:35:12.998Z
Learnt from: CR
Repo: opsmill/infrahub-sdk-python PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-24T13:35:12.998Z
Learning: Applies to infrahub_sdk/client.py : Support authentication via API tokens or JWT with automatic refresh
Applied to files:
infrahub_sdk/client.py
📚 Learning: 2025-08-24T13:35:12.998Z
Learnt from: CR
Repo: opsmill/infrahub-sdk-python PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-24T13:35:12.998Z
Learning: Applies to infrahub_sdk/client.py : Use HTTPX for transport with proxy support (single proxy or HTTP/HTTPS mounts)
Applied to files:
infrahub_sdk/client.py
🧬 Code graph analysis (2)
tests/unit/sdk/test_client.py (2)
infrahub_sdk/config.py (4)
Config(161-204)tls_context(139-155)set_ssl_context(157-158)clone(187-204)infrahub_sdk/client.py (2)
clone(921-923)clone(1782-1784)
infrahub_sdk/client.py (1)
infrahub_sdk/config.py (1)
tls_context(139-155)
⏰ 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: Cloudflare Pages
| @property | ||
| def tls_context(self) -> ssl.SSLContext: | ||
| if self._ssl_context: | ||
| return self._ssl_context | ||
|
|
||
| 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) | ||
|
|
||
| if self._ssl_context is None: | ||
| self._ssl_context = ssl.create_default_context() | ||
|
|
||
| return self._ssl_context | ||
|
|
||
| def set_ssl_context(self, context: ssl.SSLContext) -> None: | ||
| self._ssl_context = context |
There was a problem hiding this comment.
Guard against stale/insecure TLS contexts.
Once _ssl_context is populated, this property returns it forever. Because we eagerly load the context in BaseClient.__init__, changing config.tls_insecure or config.tls_ca_file later never takes effect—e.g., starting in insecure mode leaves verify_mode == CERT_NONE even after you flip tls_insecure=False. That’s an immediate security regression. Please invalidate the cache whenever the relevant settings change (or track the settings used to build the cache). One approach:
-_ssl_context: ssl.SSLContext | None = PrivateAttr(default=None)
+_ssl_context: ssl.SSLContext | None = PrivateAttr(default=None)
+_ssl_context_key: tuple[bool, str | None] | Literal["custom"] | None = PrivateAttr(default=None)
@property
def tls_context(self) -> ssl.SSLContext:
- if self._ssl_context:
+ cache_key: tuple[bool, str | None] | Literal["custom"] = (
+ "custom" if self._ssl_context_key == "custom" else (self.tls_insecure, self.tls_ca_file)
+ )
+ if self._ssl_context and self._ssl_context_key == cache_key:
return self._ssl_context
@@
- if self.tls_insecure:
+ 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
+ self._ssl_context_key = cache_key
+ return self._ssl_context
@@
- if self.tls_ca_file:
- self._ssl_context = ssl.create_default_context(cafile=self.tls_ca_file)
+ if self.tls_ca_file:
+ self._ssl_context = ssl.create_default_context(cafile=self.tls_ca_file)
+ self._ssl_context_key = cache_key
+ return self._ssl_context
@@
- if self._ssl_context is None:
- self._ssl_context = ssl.create_default_context()
-
- return self._ssl_context
+ self._ssl_context = ssl.create_default_context()
+ self._ssl_context_key = cache_key
+ return self._ssl_context
@@
def set_ssl_context(self, context: ssl.SSLContext) -> None:
- self._ssl_context = context
+ self._ssl_context = context
+ self._ssl_context_key = "custom"Any similar invalidation is fine so long as updating the config rebuilds the context.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In infrahub_sdk/config.py around lines 138 to 158, the tls_context property
caches a single SSLContext and never rebuilds it when config attributes change,
which can leave an insecure context (CERT_NONE) active after toggling
tls_insecure or updating tls_ca_file; fix by invalidating or rebuilding the
cached _ssl_context whenever relevant settings change—either (a) add setters for
tls_insecure and tls_ca_file that clear self._ssl_context (set to None) whenever
their values are modified, or (b) store the values used to build the current
context and in tls_context compare current settings to those stored and rebuild
if they differ; ensure set_ssl_context still works (overwrites cache) and
document that changing config rebuilds the context on next access.
| client.config.tls_ca_file = str(CURRENT_DIRECTORY / "test_data/path-2.pem") | ||
| third = client.config.tls_context | ||
|
|
||
| assert third is first | ||
| assert contexts == [ | ||
| (str(CURRENT_DIRECTORY / "test_data/path-1.pem"), first), | ||
| ] |
There was a problem hiding this comment.
Rebuild SSL context when configuration changes.
This test codifies that mutating Config.tls_ca_file after first access leaves the cached context untouched, so the client keeps using the old trust store. With validate_assignment=True, callers expect changing tls_ca_file (or toggling tls_insecure) to take effect immediately. Right now the cache masks those updates, which becomes a security risk when trying to re-enable verification. Please adjust the implementation to invalidate the cache when relevant fields change and update this test to expect a fresh context instead of the stale one.
🤖 Prompt for AI Agents
In tests/unit/sdk/test_client.py around lines 75 to 81, the test currently
expects the cached TLS context to remain the same after mutating
client.config.tls_ca_file; instead, update the implementation to
invalidate/rebuild the cached tls_context when relevant Config fields change (at
least tls_ca_file and tls_insecure) so changes take effect immediately; modify
the Config class (e.g., in the setters or in __setattr__ when
validate_assignment is enabled) to clear the cached context on those
assignments, and update this test to assert that a new context object is
returned and that the recorded contexts list includes both the original and the
new (i.e., expect a fresh context rather than the stale one).
Summary by CodeRabbit
New Features
Bug Fixes
Chores