test: new tests for network policy#1240
Merged
dbasunag merged 1 commit intoopendatahub-io:mainfrom Mar 20, 2026
Merged
Conversation
Signed-off-by: Debarati Basu-Nag <dbasunag@redhat.com>
|
The following are automatically added/executed:
Available user actions:
Supported labels{'/cherry-pick', '/hold', '/lgtm', '/wip', '/build-push-pr-image', '/verified'} |
Contributor
📝 WalkthroughWalkthroughConsolidated model catalog database validation tests by restructuring fixture organization, consolidating secret and network policy tests into a unified test module, and introducing a new NetworkPolicy fixture to support network policy validation tests. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment Tip CodeRabbit can generate a title for your PR based on the changes.Add |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@tests/model_registry/model_catalog/db_check/test_model_catalog_db_validation.py`:
- Around line 71-78: The test
test_postgres_network_policy_allows_only_catalog_pods currently only validates
the first ingress peer via from_selector and can miss other peers (e.g., ipBlock
or other podSelectors); update the test in model_catalog_postgres_network_policy
to iterate over
model_catalog_postgres_network_policy.instance.spec.ingress[0]["from"] and
assert for each entry that it does not contain an ipBlock and that any
podSelector.matchLabels has component == "model-catalog", failing if any extra
peers are present or labeled differently so all ingress peers are validated, not
just the first.
- Around line 61-69: The test_postgres_network_policy_restricts_to_port_5432
currently only checks spec.ingress[0].ports[0], which allows additional ports to
slip through; update the test to iterate over all ingress rules and all ports
(e.g., for each entry in spec.ingress and each port in ports) and assert every
port.port == 5432 and every port.protocol == "TCP", and also assert there is
exactly one ingress rule and that each ingress.ports list contains only the
allowed port(s) to ensure no extra ports are permitted.
- Around line 80-100: The test currently only checks that NetworkPolicy exists;
update test_postgres_network_policy_recreated_after_deletion to also validate
the recreated NetworkPolicy's ingress/egress rules are secure by retrieving the
NetworkPolicy object (via the NetworkPolicy helper returned by TimeoutSampler)
and asserting its spec matches expected constraints (e.g., allowed
peers/namespaceSelector, allowed ports like 5432, and no overly permissive from:
[]/ipBlock: 0.0.0.0/0). Use or add an expected policy representation (e.g.,
expected_postgres_network_policy() or inline expected_rules) and compare against
np.obj.spec (or relevant accessor) and fail the test with clear assertion
messages if rules are missing/too permissive; keep the existing wait loop
(TimeoutSampler, NetworkPolicy, get_postgres_pod_in_namespace, LOGGER) but add
the security assertions immediately after confirming np.exists before breaking.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 78b4cef5-3120-4c69-bf17-7de03410b80c
📒 Files selected for processing (3)
tests/model_registry/model_catalog/db_check/conftest.pytests/model_registry/model_catalog/db_check/test_model_catalog_db_validation.pytests/model_registry/model_catalog/db_check/test_model_catalog_secrets.py
💤 Files with no reviewable changes (1)
- tests/model_registry/model_catalog/db_check/test_model_catalog_secrets.py
Comment on lines
+61
to
+69
| def test_postgres_network_policy_restricts_to_port_5432(self, model_catalog_postgres_network_policy): | ||
| """Test that NetworkPolicy only allows TCP 5432 ingress""" | ||
| spec = model_catalog_postgres_network_policy.instance.spec | ||
| assert "Ingress" in spec.policyTypes, "NetworkPolicy should have Ingress policy type" | ||
| assert len(spec.ingress) == 1, "NetworkPolicy should have exactly one ingress rule" | ||
|
|
||
| port = spec.ingress[0].ports[0] | ||
| assert port.port == 5432, "NetworkPolicy should allow only PostgreSQL port 5432" | ||
| assert port.protocol == "TCP", "NetworkPolicy port should use TCP protocol" |
Contributor
There was a problem hiding this comment.
High: Line 67 only validates the first ingress port, allowing extra open ports to slip through (CWE-284).
Severity: High.
Exploit scenario: a policy with ports [5432, 8080] still passes because only index 0 is asserted.
Remediation diff
- port = spec.ingress[0].ports[0]
- assert port.port == 5432, "NetworkPolicy should allow only PostgreSQL port 5432"
- assert port.protocol == "TCP", "NetworkPolicy port should use TCP protocol"
+ ports = spec.ingress[0].ports or []
+ assert len(ports) == 1, "NetworkPolicy must expose exactly one ingress port"
+ port = ports[0]
+ assert port.port == 5432, "NetworkPolicy should allow only PostgreSQL port 5432"
+ assert port.protocol == "TCP", "NetworkPolicy port should use TCP protocol"
+ assert getattr(port, "endPort", None) in (None, 5432), "Port range expansion is not allowed"As per coding guidelines, **: "REVIEW PRIORITIES: 1. Security vulnerabilities (provide severity, exploit scenario, and remediation code)".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@tests/model_registry/model_catalog/db_check/test_model_catalog_db_validation.py`
around lines 61 - 69, The test_postgres_network_policy_restricts_to_port_5432
currently only checks spec.ingress[0].ports[0], which allows additional ports to
slip through; update the test to iterate over all ingress rules and all ports
(e.g., for each entry in spec.ingress and each port in ports) and assert every
port.port == 5432 and every port.protocol == "TCP", and also assert there is
exactly one ingress rule and that each ingress.ports list contains only the
allowed port(s) to ensure no extra ports are permitted.
tests/model_registry/model_catalog/db_check/test_model_catalog_db_validation.py
Show resolved
Hide resolved
tests/model_registry/model_catalog/db_check/test_model_catalog_db_validation.py
Show resolved
Hide resolved
|
Status of building tag latest: success. |
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
3 participants
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.
Pull Request
Summary
Related Issues
How it has been tested
Additional Requirements
Summary by CodeRabbit