SREP-2356: Added validating webhook to block user modification of network operator CRD #454
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@ratnam915: This pull request references SREP-2356 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 task to target the "4.22.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. |
|
[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 |
| } | ||
|
|
||
| // Check if critical migration fields have been modified | ||
| if hasCriticalMigrationFieldChanges(oldObject, object) { |
There was a problem hiding this comment.
The concern I have is that this webhook blocks everyone including backplane elevate which I would assume is necessary for SREP to have, in case we need to modify those fields for whatever reason.
ocm backplane elevate "testing webhook" -- patch network.operator.openshif
t.io cluster --type='merge' --patch '{"spec":{"migration":{"networkType":"
OpenShiftSDN"}}}'
Error from server (Forbidden): admission webhook "network-operator-validation.managed.openshift.io" denied the request: Modification of critical migration fields (spec.migration.networkType and related migration configuration) is not allowed, even for cluster-admin users. These fields are managed by the Cluster Network Operator and manual changes can disrupt CNI migrations.
ERRO[0002] exit status 1
We could introduce some logic e.g.
to allow backplane elevate to modify it, unless there is a justification why this is blocked?There was a problem hiding this comment.
@diakovnec : The acceptance criteria for the ticket mentions:
'Webhook prevents even cluster-admin users from modifying critical fields'
However as suggested we could add this logic if necessary
There was a problem hiding this comment.
I think SREP should have a way to have access to modify those objects
could be similar to what we did e.g.
…be able to make changes to the network operator
|
|
||
| const ( | ||
| WebhookName string = "network-operator-validation" | ||
| docString string = `Managed OpenShift customers may not modify critical fields in the network.operator CRD (such as spec.migration.networkType) because it can disrupt Cluster Network Operator operations and CNI migrations. Only backplane-cluster-admin and SRE service accounts are allowed to modify these critical fields. Regular cluster-admin users (system:admin) are explicitly blocked.` |
There was a problem hiding this comment.
system:admin is not on the list of allowed, hence the massage is missleading
There was a problem hiding this comment.
Hey @diakovnec : The message explicity states : 'Regular cluster-admin users (system:admin) are explicitly blocked'
Do we still need any changes around this?
| } | ||
|
|
||
| // Also check request.UserInfo.Groups directly (some webhooks use this pattern) | ||
| allGroups := append(request.AdmissionRequest.UserInfo.Groups, request.UserInfo.Groups...) |
There was a problem hiding this comment.
looks like this might create a duplicates of records if both username and groups returns with the same value e.g., I've looked into other webhooks and its handled with something like
I don't think its critical but maybe we can implement similar pattern.
There was a problem hiding this comment.
@diakovnec : Changes have been made based on the examples you have shared and also to make sure any duplication doesn't happen
|
@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. |
3 participants
This PR has been created as a solution for the Jira ticket :
https://issues.redhat.com/browse/SREP-2356
The PR contains the introduction of a new webhook i.e network-operator-validation which would prevent user modifications to network.operator CRD for critical fields even for cluster-admins.
Below are the fields that are covered as part of the webhook:
spec.migration.networkType
spec.migration.mode
spec.migration.features
spec.migration.mtu.network
spec.migration.mtu.machine