Switch to Rust CEL implementation #309
Open
Conversation
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
…hase 1) Updated operator dependencies as part of Phase 1 of the dependency update plan: Major updates: - kopf: ^1.37.1 → ^1.38.0 - kubernetes: ^29.0 → ^34.0 - rich: ^13.3 → ^14.0 - structlog: ^23.1.0 → ^25.4.0 - prometheus-client: ^0.16.0 → ^0.23.1 - opentelemetry-sdk: ^1.24.0 → ^1.37.0 - opentelemetry-exporter-otlp: ^1.24.0 → ^1.37.0 - opentelemetry-exporter-prometheus: ^0.46b0 → ^0.58b0 - pytest: ^8.1 → ^8.4 - ruff: ^0.3 → ^0.14 Kept pydantic at ^1.10.7 for Phase 1 (Phase 2 will migrate to pydantic 2.x). Testing: - Built and deployed operator to kind cluster with updated dependencies - Manual testing confirmed NetworkAssertion resources work correctly (HTTP and DNS checks) - PolicyReport resources created successfully - Integration tests: 2 passed (HTTP tests), 7 failed due to pre-existing CEL validation bug in CLI (unrelated to these updates) The failing integration tests are caused by a pre-existing issue where the probe outputs error messages before JSON when CEL validation fails, breaking JSON parsing. This issue existed before these dependency updates. Related to #306
Upgrades the CEL library from 0.3.1 to 0.5.3, which includes important fixes for stderr output that was interfering with JSON parsing in the operator. Key changes in CEL 0.5.3: - CEL validation errors no longer print to stderr (changed warn! to debug!) - This prevents error messages from breaking JSON output parsing - Removes string indexing support (not used in netchecks) - Strict CEL mode only (already the case for netchecks) Impact: - Integration tests: 3 passing (up from 2), 6 failing (down from 7) - DNS test now passes (previously failed due to JSON parsing errors) - All CEL validation errors are properly handled as exceptions - Clean JSON output from probe pods The remaining 6 test failures are unrelated to the CEL upgrade and appear to be pre-existing test infrastructure issues. Related to #306
Migrate both CLI and operator to Pydantic 2.x: - Update pydantic to ^2.0 in both projects - Add pydantic-settings ^2.0 to operator - Migrate operator config from BaseSettings inner Config class to SettingsConfigDict - Migrate custom settings source from function to PydanticBaseSettingsSource class - All unit and integration tests passing Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Update all CLI dependencies to their latest compatible versions: - click: 8.1.7 → 8.3.0 - dnspython: 2.7.0 → 2.8.0 - pydantic: 2.9.2 → 2.12.1 - typer: 0.13.1 → 0.19.2 - urllib3: 2.2.3 → 2.5.0 - Plus updates to test dependencies (pytest, pluggy, etc.) Breaking change fix: - Remove mix_stderr=False from CliRunner() - parameter removed in click 8.3.0 Testing: - CLI unit tests: 23 passing (1 pre-existing failure) - Integration tests: 9/9 passing - Docker image builds successfully Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
The Rust CEL library doesn't properly handle Python dict subclasses when evaluating expressions. When accessing members on a dict subclass instance, CEL returns None instead of the expected values. This commit adds a `materialize()` method to LazyFileLoadingDict that converts it to a regular dict, and calls this before passing contexts to CEL evaluation. This maintains the lazy loading behavior while ensuring compatibility with the Rust CEL implementation. This is a workaround until the upstream issue is resolved: hardbyte/python-common-expression-language#22 Fixes test_run_test_with_external_dir_context which was failing because directory context loading was broken. Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Comment on lines
45
to
51
| # Evaluate the CEL expression | ||
| try: | ||
| context = prgm.evaluate(activation) | ||
| except CELEvalError: | ||
| result = cel.evaluate(validation_rule, env) | ||
| except Exception as e: | ||
| # Note this can fail if the context is missing a key e.g. the probe | ||
| # failed to return a value for a key that the validation rule expects | ||
|
|
||
| return False |
There was a problem hiding this comment.
The new Rust-backed evaluate_cel_with_context now wraps cel.evaluate in a broad except Exception and returns False for any failure. This means malformed validation rules (e.g. syntax errors) are no longer surfaced to callers via ValueError as documented, but are silently interpreted as a failing validation result. A mis-typed rule will therefore produce a failed check instead of surfacing a configuration error, making it difficult for users to notice mistakes in their CEL expressions. Consider detecting parse errors separately and raising ValueError so invalid rules are not silently ignored.
Useful? React with 👍 / 👎.
Pull Request Test Coverage Report for Build 18539798968Details
💛 - Coveralls |
Deploying netchecks-docs with
|
| Latest commit: |
6fcf736
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://15c433bb.netchecks-docs.pages.dev |
| Branch Preview URL: | https://switch-to-rust-cel-implement.netchecks-docs.pages.dev |
- Change typos locale from en-au to en-us for US spelling - Remove unused imports in operator/tests/test_config.py and tests/test_context.py - Remove unused variable in test_no_environment_variable_set - Add noqa comment for intentional type() check in test_materialize
- Initialize instead of Initialise - summarize instead of summarise - visualize instead of visualise - behavior instead of behaviour - customize instead of customise (except Pydantic method name) - Add exception for Pydantic's settings_customise_sources method
- Fix remaining British spellings in docs and examples - Change extend-identifiers to extend-words in typos config - Add customise exception for Pydantic method name
|
/test |
The integration test passed locally with ConfigMap directory context loading. This commit triggers CI to verify the fix works in the CI environment.
…covery The VIRTUAL_ENV environment variable was commented out in the Dockerfile, causing Python to not find packages installed in the virtual environment. This resulted in ModuleNotFoundError for common-expression-language and other dependencies, causing pods to fail to start in Kubernetes. The integration test test_use_external_config_map_data was timing out because pods would crash on startup when trying to import the Rust CEL library. This fix uncomments the VIRTUAL_ENV=/app/.venv line to properly configure the Python environment in the container. Fixes the failing integration test in CI.
When the ConfigMap integration test fails, capture and print pod logs and pod descriptions to help diagnose the failure in CI. This will help identify why pods are failing to start in the CI environment when they work locally.
When Kubernetes mounts a ConfigMap as a directory, it creates symlinks with names like '..data' and '..2025_10_14_19_45_09.xxx' for atomic updates. The LazyFileLoadingDict was listing these files during __init__ but then rejecting them in __getitem__ because they start with '..'. This caused the materialize() method to fail with: KeyError: 'Invalid key: ..2025_10_14_19_45_09.1502224970. Path separators and relative paths are not allowed.' Fix by filtering out all files starting with '.' during initialization, which excludes both hidden files and Kubernetes metadata symlinks. Fixes the integration test failure in test_use_external_config_map_data.
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.
This PR upgrades the project's core dependencies, migrating from Pydantic v1 to v2 and switching from the Python celpy CEL library to the Rust-based common-expression-language library. This modernizes the dependency stack and improves performance for CEL expression evaluation.
Changes
Breaking Changes
None - all changes are internal implementation details. The external API remains unchanged.
Testing
All tests passing:
Critical regression test test_run_test_with_external_dir_context now passes, verifying directory-based context loading works correctly with the new CEL library.
Migration Notes
Related Issues