SREP-2356 bug fix : Adding service account to fix the issue#467
SREP-2356 bug fix : Adding service account to fix the issue#467ratnam915 wants to merge 4 commits intoopenshift:masterfrom
Conversation
WalkthroughExpanded webhook authorization to allow Cluster Network Operator and Managed Upgrade Operator service accounts to modify migration.networkType; added tests for those accounts; introduced a validating webhook configuration resource; and enabled ClassicEnabled and HypershiftEnabled (return true). Changes
Sequence Diagram(s)sequenceDiagram
participant SA as ServiceAccount (CNO / MUO)
participant APIS as API Server
participant WH as Validation Webhook (openshift-validation-webhook)
participant NS as Network.operator resource
SA->>APIS: PATCH/UPDATE `network` (migration.networkType)
APIS->>WH: POST /network-operator-validation (admission review)
WH-->>APIS: Respond ALLOW or DENY
APIS-->>SA: Return success or error
Note right of APIS: If ALLOW -> persist update to `network` resource (NS)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: ratnam915 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![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/networkoperator/networkoperator_test.go (1)
421-467: New CNO test case is correct; consider adding a negative test to lock in least-privilege intent.The happy-path test mirrors the SRE SA case and correctly validates the new allowance. However, there is currently no test for a different service account in the same
openshift-network-operatornamespace (e.g.,system:serviceaccount:openshift-network-operator:some-other-sa). Such a test would beExpectAllowed: falseunder the least-privilege fix suggested in the main file and would prevent the overly broad group grant from silently re-appearing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/webhooks/networkoperator/networkoperator_test.go` around lines 421 - 467, Add a negative test alongside the new CNO happy-path case in networkoperator_test.go that ensures only the specific Cluster Network Operator SA is allowed: duplicate the test case (the block with Name "Cluster Network Operator (CNO) service account modifying migration.networkType should be allowed") but change the AdmissionRequest.UserInfo.Username to "system:serviceaccount:openshift-network-operator:some-other-sa" (or another non-CNO SA in the same namespace) and set ExpectAllowed: false; keep the same Kind (operator.openshift.io/Network), Operation (admissionv1.Update), and the Object/OldObject payloads so the test asserts the least-privilege behavior is enforced.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/webhooks/networkoperator/networkoperator.go`:
- Around line 47-51: Replace the broad namespace-scoped group entry in
sreAdminGroups with the specific CNO service account username so only the
cluster-network-operator SA gets migration privileges: remove
"system:serviceaccounts:openshift-network-operator" from sreAdminGroups and add
the exact service account string (match the pattern used in allowedUsers, e.g.,
"system:serviceaccount:openshift-network-operator:cluster-network-operator");
update the inline comment near the sreAdminGroups declaration and the other
comments that mention allowedUsers (the comments at the locations referencing
"Only backplane-cluster-admin and SRE service accounts are allowed" and the
later mention) to explicitly note the cluster-network-operator SA (CNO) rather
than implying the whole namespace. Ensure you edit the variables named
sreAdminGroups and allowedUsers and the nearby comment text accordingly.
---
Nitpick comments:
In `@pkg/webhooks/networkoperator/networkoperator_test.go`:
- Around line 421-467: Add a negative test alongside the new CNO happy-path case
in networkoperator_test.go that ensures only the specific Cluster Network
Operator SA is allowed: duplicate the test case (the block with Name "Cluster
Network Operator (CNO) service account modifying migration.networkType should be
allowed") but change the AdmissionRequest.UserInfo.Username to
"system:serviceaccount:openshift-network-operator:some-other-sa" (or another
non-CNO SA in the same namespace) and set ExpectAllowed: false; keep the same
Kind (operator.openshift.io/Network), Operation (admissionv1.Update), and the
Object/OldObject payloads so the test asserts the least-privilege behavior is
enforced.
| sreAdminGroups = []string{ | ||
| "system:serviceaccounts:openshift-backplane-srep", | ||
| // CNO runs in openshift-network-operator and must be able to patch network.operator for CNI migration | ||
| "system:serviceaccounts:openshift-network-operator", |
There was a problem hiding this comment.
Do we need to add the MUO service account to enable cluster upgrade?
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
pkg/webhooks/networkoperator/networkoperator.go (2)
49-52: Overly broad namespace-scoped group for MUO (same pattern as previously flagged CNO entry).
"system:serviceaccounts:openshift-managed-upgrade-operator"grants every service account in that namespace the ability to modify critical migration fields, not just the specific MUO SA. The same concern was raised for the CNO entry on line 50. Both entries should use the exact SA username format already established byallowedUsers:allowedUsers = []string{ "backplane-cluster-admin", + "system:serviceaccount:openshift-network-operator:cluster-network-operator", + "system:serviceaccount:openshift-managed-upgrade-operator:managed-upgrade-operator", } sreAdminGroups = []string{ "system:serviceaccounts:openshift-backplane-srep", - // CNO runs in openshift-network-operator and must be able to patch network.operator for CNI migration - "system:serviceaccounts:openshift-network-operator", - // Managed Upgrade Operator may need to modify network.operator during upgrades - "system:serviceaccounts:openshift-managed-upgrade-operator", }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/webhooks/networkoperator/networkoperator.go` around lines 49 - 52, The entries "system:serviceaccounts:openshift-managed-upgrade-operator" and the analogous "system:serviceaccounts:openshift-network-operator" grant whole-namespace serviceaccount groups too much privilege; replace each namespace-wide group with the exact service account username (the single SA format used in allowedUsers) so only the MUO and CNO service accounts can modify network.operator — update the list in networkoperator.go to use the explicit service account principal(s) matching the existing allowedUsers pattern (e.g., system:serviceaccount:<namespace>:<serviceaccount>) and remove the broad namespace group entries.
63-63: Stale inline comments still don't mention CNO or MUO.Lines 63 and 92 still say "Only backplane-cluster-admin and SRE service accounts are allowed," which was already flagged in the prior review cycle. Both should be updated to reflect the CNO and MUO additions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/webhooks/networkoperator/networkoperator.go` at line 63, Update the stale inline comment "Only backplane-cluster-admin and SRE service accounts are allowed" in pkg/webhooks/networkoperator/networkoperator.go at both occurrences to include the new CNO and MUO service accounts; locate the two comment instances near the webhook/validation logic (the comment above the service-account allowlist/validation code—search for that exact string) and change it to explicitly mention backplane-cluster-admin, SRE, CNO, and MUO so the comment matches current behavior and policy.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/webhooks/networkoperator/networkoperator.go`:
- Line 21: The docString constant currently lists allowed actors but omits the
Managed Upgrade Operator (MUO) even though MUO was added to sreAdminGroups;
update the docString (variable name docString) to include MUO alongside the
Cluster Network Operator (CNO) in the sentence that names allowed service
accounts—e.g., change "Cluster Network Operator (CNO) service account" to
"Cluster Network Operator (CNO) and Managed Upgrade Operator (MUO) service
accounts" so the documentation matches the sreAdminGroups change.
---
Duplicate comments:
In `@pkg/webhooks/networkoperator/networkoperator.go`:
- Around line 49-52: The entries
"system:serviceaccounts:openshift-managed-upgrade-operator" and the analogous
"system:serviceaccounts:openshift-network-operator" grant whole-namespace
serviceaccount groups too much privilege; replace each namespace-wide group with
the exact service account username (the single SA format used in allowedUsers)
so only the MUO and CNO service accounts can modify network.operator — update
the list in networkoperator.go to use the explicit service account principal(s)
matching the existing allowedUsers pattern (e.g.,
system:serviceaccount:<namespace>:<serviceaccount>) and remove the broad
namespace group entries.
- Line 63: Update the stale inline comment "Only backplane-cluster-admin and SRE
service accounts are allowed" in pkg/webhooks/networkoperator/networkoperator.go
at both occurrences to include the new CNO and MUO service accounts; locate the
two comment instances near the webhook/validation logic (the comment above the
service-account allowlist/validation code—search for that exact string) and
change it to explicitly mention backplane-cluster-admin, SRE, CNO, and MUO so
the comment matches current behavior and policy.
ℹ️ Review info
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (2)
pkg/webhooks/networkoperator/networkoperator.gopkg/webhooks/networkoperator/networkoperator_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/webhooks/networkoperator/networkoperator_test.go
There was a problem hiding this comment.
♻️ Duplicate comments (1)
pkg/webhooks/networkoperator/networkoperator.go (1)
21-21:⚠️ Potential issue | 🟡 Minor
docStringstill omits the Managed Upgrade Operator.The string names CNO as an allowed actor but does not mention MUO, which is also intended to be authorized.
📝 Proposed fix
-docString string = `... Only backplane-cluster-admin, SRE service accounts, and the Cluster Network Operator (CNO) service account are allowed to modify these critical fields. ...` +docString string = `... Only backplane-cluster-admin, SRE service accounts, the Cluster Network Operator (CNO) service account, and the Managed Upgrade Operator (MUO) service account are allowed to modify these critical fields. ...`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/webhooks/networkoperator/networkoperator.go` at line 21, The docString constant in networkoperator.go omits the Managed Upgrade Operator (MUO) from the list of allowed actors; update the docString variable (docString) to explicitly include "Managed Upgrade Operator (MUO)" alongside the Cluster Network Operator (CNO) so the message correctly lists MUO as an authorized modifier.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@pkg/webhooks/networkoperator/networkoperator.go`:
- Line 21: The docString constant in networkoperator.go omits the Managed
Upgrade Operator (MUO) from the list of allowed actors; update the docString
variable (docString) to explicitly include "Managed Upgrade Operator (MUO)"
alongside the Cluster Network Operator (CNO) so the message correctly lists MUO
as an authorized modifier.
ℹ️ Review info
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (2)
build/selectorsyncset.yamlpkg/webhooks/networkoperator/networkoperator.go
|
@ratnam915: 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. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/webhooks/networkoperator/networkoperator.go`:
- Around line 47-53: The two CNO/MUO entries in sreAdminGroups are invalid group
strings and should be removed and added as specific allowedUsers using the
service account username format "system:serviceaccount:<namespace>:<name>"
(singular); update pkg/webhooks/networkoperator/networkoperator.go by removing
"system:serviceaccounts:openshift-network-operator:cluster-network-operator" and
"system:serviceaccounts:openshift-managed-upgrade-operator:managed-upgrade-operator"
from sreAdminGroups and add
"system:serviceaccount:openshift-network-operator:cluster-network-operator" and
"system:serviceaccount:openshift-managed-upgrade-operator:managed-upgrade-operator"
to the allowedUsers list (so isAllowedUserGroup will match via groups and
isAllowedUser via exact usernames), and while here verify that ClassicEnabled()
and HypershiftEnabled() behavior (functions referenced by the webhook enablement
logic) is intentional for applying this webhook to Hypershift clusters.
ℹ️ Review info
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (2)
pkg/webhooks/networkoperator/networkoperator.gopkg/webhooks/networkoperator/networkoperator_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/webhooks/networkoperator/networkoperator_test.go
| sreAdminGroups = []string{ | ||
| "system:serviceaccounts:openshift-backplane-srep", | ||
| // CNO runs in openshift-network-operator and must be able to patch network.operator for CNI migration | ||
| "system:serviceaccounts:openshift-network-operator:cluster-network-operator", | ||
| // Managed Upgrade Operator may need to modify network.operator during upgrades | ||
| "system:serviceaccounts:openshift-managed-upgrade-operator:managed-upgrade-operator", | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n pkg/webhooks/networkoperator/networkoperator.go | head -60Repository: openshift/managed-cluster-validating-webhooks
Length of output: 2782
🏁 Script executed:
cat -n pkg/webhooks/networkoperator/networkoperator.go | sed -n '280,310p'Repository: openshift/managed-cluster-validating-webhooks
Length of output: 1406
🏁 Script executed:
rg -n "openshift-network-operator|openshift-managed-upgrade-operator|cluster-network-operator|managed-upgrade-operator" --type go -A 2 -B 2Repository: openshift/managed-cluster-validating-webhooks
Length of output: 3738
🏁 Script executed:
cat -n pkg/webhooks/networkoperator/networkoperator_test.go | sed -n '420,435p'Repository: openshift/managed-cluster-validating-webhooks
Length of output: 847
🏁 Script executed:
cat -n pkg/webhooks/networkoperator/networkoperator_test.go | sed -n '470,485p'Repository: openshift/managed-cluster-validating-webhooks
Length of output: 886
🏁 Script executed:
cat -n pkg/webhooks/networkoperator/networkoperator.go | sed -n '70,120p'Repository: openshift/managed-cluster-validating-webhooks
Length of output: 2820
🏁 Script executed:
cat -n pkg/webhooks/networkoperator/networkoperator.go | sed -n '155,210p'Repository: openshift/managed-cluster-validating-webhooks
Length of output: 2259
🏁 Script executed:
rg -n "isAllowedUserGroup" pkg/webhooks/networkoperator/networkoperator.go -A 25Repository: openshift/managed-cluster-validating-webhooks
Length of output: 2599
🏁 Script executed:
cat -n pkg/webhooks/networkoperator/networkoperator.go | sed -n '210,235p'Repository: openshift/managed-cluster-validating-webhooks
Length of output: 1094
🏁 Script executed:
web_search
"Kubernetes service account groups format" "system:serviceaccounts"Repository: openshift/managed-cluster-validating-webhooks
Length of output: 218
🌐 Web query:
Kubernetes service account groups "system:serviceaccounts" format RBAC
💡 Result:
In Kubernetes RBAC, ServiceAccounts are authenticated with a fixed username and group scheme:
-
ServiceAccount username (kind:
ServiceAccountidentity):
system:serviceaccount:<namespace>:<serviceaccountname>(kubernetes.io) -
Group containing all ServiceAccounts (cluster-wide):
system:serviceaccounts(kubernetes.io) -
Group containing all ServiceAccounts in a specific namespace:
system:serviceaccounts:<namespace>(kubernetes.io)
RBAC binding examples
Bind a role to all ServiceAccounts in namespace qa:
subjects:
- kind: Group
name: system:serviceaccounts:qa
apiGroup: rbac.authorization.k8s.ioBind a role to all ServiceAccounts in the cluster:
subjects:
- kind: Group
name: system:serviceaccounts
apiGroup: rbac.authorization.k8s.ioCitations:
- 1: https://kubernetes.io/docs/reference/access-authn-authz/rbac/?utm_source=openai
- 2: https://kubernetes.io/docs/reference/access-authn-authz/rbac/?utm_source=openai
- 3: https://kubernetes.io/docs/reference/access-authn-authz/rbac/?utm_source=openai
- 4: https://kubernetes.io/docs/reference/access-authn-authz/rbac/?utm_source=openai
- 5: https://kubernetes.io/docs/reference/access-authn-authz/rbac/?utm_source=openai
CNO and MUO entries in sreAdminGroups use an invalid Kubernetes group format — authorization will silently never match.
Service account identities in Kubernetes are expressed as a username with format system:serviceaccount:<namespace>:<name> (singular). The only group strings Kubernetes assigns to service accounts are:
system:serviceaccounts(all SAs cluster-wide)system:serviceaccounts:<namespace>(all SAs in a namespace)
The two entries:
"system:serviceaccounts:openshift-network-operator:cluster-network-operator"
"system:serviceaccounts:openshift-managed-upgrade-operator:managed-upgrade-operator"use a four-segment system:serviceaccounts:<namespace>:<name> format that does not exist in Kubernetes RBAC. When Kubernetes issues a token for these service accounts, it will never include these strings in the Groups array — only system:serviceaccounts:openshift-network-operator and system:serviceaccounts:openshift-managed-upgrade-operator respectively. As a result, the isAllowedUserGroup check will always deny CNO and MUO requests.
The previous review's proposed fix was correct: use the SA username (singular serviceaccount) and place it in allowedUsers, not sreAdminGroups.
🔒 Proposed fix: move specific SA identities to allowedUsers
// Users allowed to modify critical migration fields
// backplane-cluster-admin and system:admin are allowed
allowedUsers = []string{
"backplane-cluster-admin",
+ // CNO must be able to patch network.operator for CNI migration
+ "system:serviceaccount:openshift-network-operator:cluster-network-operator",
+ // Managed Upgrade Operator may need to modify network.operator during upgrades
+ "system:serviceaccount:openshift-managed-upgrade-operator:managed-upgrade-operator",
}
- // Groups allowed to modify critical migration fields (SRE, CNO, and Managed Upgrade Operator)
+ // Groups allowed to modify critical migration fields (SRE)
sreAdminGroups = []string{
"system:serviceaccounts:openshift-backplane-srep",
- // CNO runs in openshift-network-operator and must be able to patch network.operator for CNI migration
- "system:serviceaccounts:openshift-network-operator:cluster-network-operator",
- // Managed Upgrade Operator may need to modify network.operator during upgrades
- "system:serviceaccounts:openshift-managed-upgrade-operator:managed-upgrade-operator",
}Additionally, ClassicEnabled() and HypershiftEnabled() (lines 291, 294) both now return true, enabling the webhook on both cluster types. Verify that applying this webhook to HyperShift clusters is intentional, as hosted control planes may have different network operator behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/webhooks/networkoperator/networkoperator.go` around lines 47 - 53, The
two CNO/MUO entries in sreAdminGroups are invalid group strings and should be
removed and added as specific allowedUsers using the service account username
format "system:serviceaccount:<namespace>:<name>" (singular); update
pkg/webhooks/networkoperator/networkoperator.go by removing
"system:serviceaccounts:openshift-network-operator:cluster-network-operator" and
"system:serviceaccounts:openshift-managed-upgrade-operator:managed-upgrade-operator"
from sreAdminGroups and add
"system:serviceaccount:openshift-network-operator:cluster-network-operator" and
"system:serviceaccount:openshift-managed-upgrade-operator:managed-upgrade-operator"
to the allowedUsers list (so isAllowedUserGroup will match via groups and
isAllowedUser via exact usernames), and while here verify that ClassicEnabled()
and HypershiftEnabled() behavior (functions referenced by the webhook enablement
logic) is intentional for applying this webhook to Hypershift clusters.
2 participants
SREP-2356 bug fix : Adding service account to fix the issue
Summary by CodeRabbit
New Features
Improvements
Tests