ROSAENG-60641: Allow privileged service accounts to create NetworkPolicies in openshift-ingress#581
Conversation
|
@ugiordan: GitHub didn't allow me to request PR reviews from the following users: michaelcourcy. Note that only openshift members and repo collaborators can review this PR, and authors cannot review their own PRs. DetailsIn response to this:
Instructions 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. |
WalkthroughThe webhook now allows privileged service accounts to manage ChangesPrivileged NetworkPolicy allowance
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 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 |
|
/assign @joshbranham |
|
@ugiordan: 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. |
![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 (1)
pkg/webhooks/networkpolicies/networkpolicies.go (1)
99-108: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: de-duplicate the privileged-SA group loop.
This loop is identical to the one at Lines 85-91 (only the message differs). Consider extracting a small helper (e.g.
isPrivilegedServiceAccountGroup(request)) to keep the two paths in sync.🤖 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/webhooks/networkpolicies/networkpolicies.go` around lines 99 - 108, The privileged service account group check is duplicated in the NetworkPolicies admission flow, which can drift between the two paths. Extract the shared loop into a small helper such as isPrivilegedServiceAccountGroup(request) or similar in networkpolicies.go, and reuse it from both call sites so the matching logic stays identical while only the Allowed message text remains different.
🤖 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.
Inline comments:
In `@pkg/webhooks/networkpolicies/networkpolicies.go`:
- Around line 99-108: The privileged service account early return in the network
policy admission flow bypasses the default-ingress protection check, so update
the logic in the NetworkPolicies webhook to keep the ingressName == "default"
guard applying even for matched privileged groups. Adjust the matching in the
request.UserInfo.Groups loop so the allow path only grants access when the
target policy is not the default ingress, and tighten
privilegedServiceAccountGroupsRe if exact group matching is intended by
anchoring the pattern or comparing the full group string directly. Keep the
behavior centered around the existing admission decision in the webhook handler.
---
Nitpick comments:
In `@pkg/webhooks/networkpolicies/networkpolicies.go`:
- Around line 99-108: The privileged service account group check is duplicated
in the NetworkPolicies admission flow, which can drift between the two paths.
Extract the shared loop into a small helper such as
isPrivilegedServiceAccountGroup(request) or similar in networkpolicies.go, and
reuse it from both call sites so the matching logic stays identical while only
the Allowed message text remains different.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: b16a835e-611b-4905-81c9-286737aaa194
📒 Files selected for processing (2)
pkg/webhooks/networkpolicies/networkpolicies.gopkg/webhooks/networkpolicies/networkpolicies_test.go
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: joshbranham, ugiordan 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
|
/retest |
…in openshift-ingress The openshift-ingress namespace has a special-case check that only allows NetworkPolicies whose podSelector targets a non-default ingress controller. This blocks privileged service accounts (matching redhat-*, openshift-*, etc.) from creating NetworkPolicies for non-ingress-controller pods deployed in that namespace. OCP 4.22 introduced a deny-all NetworkPolicy in openshift-ingress, making it necessary for operators like RHOAI to create explicit allow policies for their pods (kube-auth-proxy, payload-processing). Without this fix, the webhook blocks these policies even from privileged SAs. Add the privileged SA check before the ingress controller label check so that trusted operators can manage NetworkPolicies in openshift-ingress without requiring the ingress controller label. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
This will need to be rebased |
ugiordan
force-pushed
the
fix/networkpolicies-privileged-sa-openshift-ingress
branch
from
b89dd2b to
12cb46e
Compare
done |
|
/test ci/prow/pr-check |
|
/retest |
joshbranham
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@ugiordan: This pull request references ROSAENG-60641 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
4 participants
Summary
OCP 4.22 introduced a deny-all NetworkPolicy (
openshift-ingress-deny-all) in theopenshift-ingressnamespace via NE-2501. This requires operators that deploy pods intoopenshift-ingressto create explicit allow NetworkPolicies for their pods.The current webhook logic has a special case for
openshift-ingressthat skips the privileged SA check entirely and only allows NPs whose podSelector containsingresscontroller.operator.openshift.io/deployment-ingresscontrollerwith a non-defaultvalue. This blocks privileged service accounts (matchingredhat-*,openshift-*, etc.) from creating NetworkPolicies for non-ingress-controller pods in that namespace.The flow today:
isAllowedNamespace("openshift-ingress")returnstrue(explicit carve-out on line 119)!isAllowedNamespaceblock where the privileged SA regex is evaluatedopenshift-ingressspecific check which only looks at the ingress controller labelThis PR adds the privileged SA check before the ingress controller label check in the
openshift-ingressblock, so that trusted operators can manage NetworkPolicies for their own pods without requiring the ingress controller label.Jira: ROSAENG-60641
Context
RHOAI deploys pods into
openshift-ingressas part of its gateway architecture. With the OCP 4.22 deny-all in place, the operator needs to create NetworkPolicies for those pods. On ROSA, the webhook blocks the operator SA (system:serviceaccount:redhat-ods-operator:*) from doing so, even though it matches the privileged SA regex (redhat-*). This makes RHOAI non-functional on ROSA + OCP 4.22.Validation
rhoaiServiceAccount()test helper simulatingsystem:serviceaccount:redhat-ods-operator:redhat-ods-operator-controller-managerredhat-*) can CRUD NetworkPolicies inopenshift-ingresswithout the ingress controller labelopenshift-*) can CRUD NetworkPolicies inopenshift-ingresswithout the ingress controller labelopenshift-ingresswithout the labelSummary by CodeRabbit
openshift-ingressnamespace are now allowed to manage network policies even when the usual ingress-related label is missing.