[ROSAENG-61345] harden NetworkPolicy ingress to pod-label selectors fo… - #326
[ROSAENG-61345] harden NetworkPolicy ingress to pod-label selectors fo…#326charlesgong wants to merge 1 commit into
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: charlesgong The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
|
Important Review available on request
Reviews should be triggered manually for repositories with fewer than 10 stars. Select Trigger review above or comment ⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Pro Plus Run ID: WalkthroughThe change adds exported pod-label constants and applies namespace-specific caller pod selectors to generated NetworkPolicies. Tests cover monitoring, managed-upgrade-operator, RHOBS, and OBO namespaces, including fleet-mode agents. The Tekton pipeline reference also changes. ChangesNetworkPolicy caller selection
Tekton pipeline revision
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pkg/ocmagenthandler/ocmagenthandler_networkpolicy_test.go (2)
85-89: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winCover both selectors and the fallback branch.
The MUO, RHOBS, and OBO cases assert only
PodSelector. Also assert thatNamespaceSelectormatchestestNamespacein each case. Add a case that verifiescallerPodSelectorreturnsnilfor an unsupported namespace and that the policy keeps namespace-only behavior.As per coding guidelines, unit tests are required for all new functions and should prioritize meaningful coverage over metric-only coverage.
Also applies to: 98-102, 111-115
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/ocmagenthandler/ocmagenthandler_networkpolicy_test.go` around lines 85 - 89, Expand the network policy tests for the MUO, RHOBS, and OBO cases to assert each ingress rule’s NamespaceSelector matches testNamespace in addition to PodSelector. Add coverage for callerPodSelector with an unsupported namespace, verifying it returns nil and the resulting policy preserves namespace-only ingress behavior.Source: Coding guidelines
69-73: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd diagnostic messages to the new assertions.
The added
Expectcalls do not include failure messages. Add messages that identify the namespace and expected selector so a failure states which ingress contract broke.As per coding guidelines, assertion messages must include meaningful failure messages.
Also applies to: 75-77, 85-89, 98-102, 111-115
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/ocmagenthandler/ocmagenthandler_networkpolicy_test.go` around lines 69 - 73, Add meaningful failure messages to every new Expect assertion in the ingress network policy tests, including the block “Should restrict ingress to alertmanager pods in the monitoring namespace” and the referenced assertion ranges. Messages must identify the monitoring namespace, ingress contract, and expected pod selector or label values so failures clearly indicate what was violated.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@pkg/ocmagenthandler/ocmagenthandler_networkpolicy_test.go`:
- Around line 85-89: Expand the network policy tests for the MUO, RHOBS, and OBO
cases to assert each ingress rule’s NamespaceSelector matches testNamespace in
addition to PodSelector. Add coverage for callerPodSelector with an unsupported
namespace, verifying it returns nil and the resulting policy preserves
namespace-only ingress behavior.
- Around line 69-73: Add meaningful failure messages to every new Expect
assertion in the ingress network policy tests, including the block “Should
restrict ingress to alertmanager pods in the monitoring namespace” and the
referenced assertion ranges. Messages must identify the monitoring namespace,
ingress contract, and expected pod selector or label values so failures clearly
indicate what was violated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 976491ab-b2aa-4f86-9b07-5beac5b64db9
📒 Files selected for processing (3)
pkg/consts/ocmagenthandler/ocmagenthandler.gopkg/ocmagenthandler/ocmagenthandler_networkpolicy.gopkg/ocmagenthandler/ocmagenthandler_networkpolicy_test.go
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #326 +/- ##
==========================================
+ Coverage 65.03% 65.31% +0.28%
==========================================
Files 23 23
Lines 1570 1583 +13
==========================================
+ Hits 1021 1034 +13
Misses 471 471
Partials 78 78
🚀 New features to boost your workflow:
|
openshift-ci
Bot
added
needs-rebase
f3bc0b4 to
37d2d70
Compare
| MatchLabels: map[string]string{"kubernetes.io/metadata.name": namespace}, | ||
| }, | ||
| } | ||
| if podSelector := callerPodSelector(namespace); podSelector != nil { |
There was a problem hiding this comment.
There's no condition nil, are we going to allow it? What happens if the podSelector is nil? i feel only NamespaceSelector enforced and podSelector is more like permissive mode.
…r fleet namespaces Extend callerPodSelector to cover NamespaceRHOBS and NamespaceOBO so that fleet-mode NetworkPolicies use combined namespace+pod selectors instead of namespace-only selectors. This eliminates the residual confused-deputy gap where any pod in the permitted namespace could reach the ocm-agent HTTP API. Adds constants for MUO/RHOBS/OBO pod labels and fills in test coverage for MUO, RHOBS, and OBO pod selector assertions. Review fixes: - OBOPodLabelValue corrected from "main" to "hypershift-monitoring-stack", confirmed via: oc get po -n openshift-observability-operator -l alertmanager=hypershift-monitoring-stack. RHOBSPodLabelValue is left pending cluster verification (open review thread). - callerPodSelector no longer has a silent nil fallback: an ingress peer with only NamespaceSelector matches every pod in that namespace, so a nil podSelector for an unhandled namespace would silently defeat the pod-level restriction this policy is meant to enforce. Unhandled namespaces now panic (programming error) instead of degrading to a namespace-wide policy; added a regression test for this. - Refreshed the UBI9 minimal base image tag (9.8-1786927599) via `make boilerplate-update` to satisfy boilerplate's live generate-check. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com> Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
037f16f to
a5b78d0
Compare
|
@charlesgong: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
3 participants
What type of PR is this?
bug / security hardening
What this PR does / why we need it?
Extends callerPodSelector in ocmagenthandler_networkpolicy.go to cover the RHOBS (observatorium-mst-production) and OBO (openshift-observability-operator) fleet-mode namespaces with combined namespace + pod-label selectors.
Previously, only openshift-monitoring (Alertmanager) and openshift-managed-upgrade-operator (MUO) had pod-level selectors. The RHOBS and OBO NetworkPolicies used namespace-only selectors, meaning any pod running in those namespaces could reach the ocm-agent HTTP API and trigger privileged OCM state-change calls — a confused-deputy gap.
With this change, all four permitted caller namespaces use a combined NamespaceSelector + PodSelector in their ingress peer, so only the specific caller pods (not any pod in the namespace) can reach the agent.
Also adds:
Which Jira/Github issue(s) this PR fixes?
ROSAENG-61345
Special notes for your reviewer:
The RHOBSPodLabelValue and OBOPodLabelValue constants in pkg/consts/ocmagenthandler/ocmagenthandler.go are marked TODO and must be verified against a live fleet-mode cluster before this PR is merged:
oc get pods -n observatorium-mst-production --show-labels
oc get pods -n openshift-observability-operator --show-labels
No changes to the agent HTTP stack or caller configuration — this is a NetworkPolicy-only change confined to ocm-agent-operator.
Pre-checks (if applicable):
make generatecommand locally to validate code changesSummary by CodeRabbit
Security Enhancements
Tests