DNM: add virt annotation remover webhook#2088
DNM: add virt annotation remover webhook#2088weshayutin wants to merge 2 commits intoopenshift:oadp-devfrom
Conversation
WalkthroughAdds a new KubeVirt Annotations Remover component: API types and deepcopy support, CRD and CSV manifest updates, RBAC and manager env var, controller reconciliation (Deployment, Service, MutatingWebhookConfiguration), readiness check, and comprehensive unit tests. Changes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: weshayutin 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
🤖 Fix all issues with AI agents
In `@config/rbac/role.yaml`:
- Around line 38-49: The MutatingWebhookConfiguration RBAC rule grants
unnecessary permissions; edit the role definition in config/rbac/role.yaml and
remove the "list" and "watch" verbs for apiGroups: admissionregistration.k8s.io
/ resources: mutatingwebhookconfigurations so the verbs are exactly "create",
"delete", "get", "patch", "update" (this aligns with how
internal/controller/kubevirt_annotations_remover.go uses Get (line ~75), Delete
(line ~84) and CreateOrUpdate (around line ~418) for the fixed-name webhook).
🧹 Nitpick comments (2)
internal/controller/kubevirt_annotations_remover_test.go (1)
532-543: Usestrings.Containsinstead of custom helpers.The
containsStrandcontainsSubstringfunctions duplicate functionality provided by Go's standard librarystrings.Contains.♻️ Suggested simplification
+import "strings" -func containsStr(s, substr string) bool { - return len(s) >= len(substr) && (s == substr || len(s) > 0 && containsSubstring(s, substr)) -} - -func containsSubstring(s, substr string) bool { - for i := 0; i <= len(s)-len(substr); i++ { - if s[i:i+len(substr)] == substr { - return true - } - } - return false -}Then replace
containsStr(err.Error(), tt.errorContains)withstrings.Contains(err.Error(), tt.errorContains).internal/controller/kubevirt_annotations_remover.go (1)
72-72: Remove unusedlogparameters.The
logparameter is declared but unused in these functions. Either use the logger for debugging/tracing or remove the parameter.♻️ Option 1: Remove unused parameters
-func (r *DataProtectionApplicationReconciler) cleanupKubevirtAnnotationsRemover(log logr.Logger) (bool, error) { +func (r *DataProtectionApplicationReconciler) cleanupKubevirtAnnotationsRemover() (bool, error) { -func (r *DataProtectionApplicationReconciler) reconcileKubevirtAnnotationsRemoverService(log logr.Logger) error { +func (r *DataProtectionApplicationReconciler) reconcileKubevirtAnnotationsRemoverService() error { -func (r *DataProtectionApplicationReconciler) reconcileKubevirtAnnotationsRemoverDeployment(log logr.Logger) error { +func (r *DataProtectionApplicationReconciler) reconcileKubevirtAnnotationsRemoverDeployment() error { -func (r *DataProtectionApplicationReconciler) reconcileKubevirtAnnotationsRemoverWebhook(log logr.Logger) error { +func (r *DataProtectionApplicationReconciler) reconcileKubevirtAnnotationsRemoverWebhook() error {And update the callers in
ReconcileKubevirtAnnotationsRemoveraccordingly.Also applies to: 182-182, 239-239, 407-407
| - apiGroups: | ||
| - admissionregistration.k8s.io | ||
| resources: | ||
| - mutatingwebhookconfigurations | ||
| verbs: | ||
| - create | ||
| - delete | ||
| - get | ||
| - list | ||
| - patch | ||
| - update | ||
| - watch |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find the overall structure and locate the controller code
fd --type f -e go | head -20Repository: openshift/oadp-operator
Length of output: 925
🏁 Script executed:
# Search for MutatingWebhookConfiguration usage in Go files
rg -n 'MutatingWebhookConfiguration' --type go -A 3 -B 3Repository: openshift/oadp-operator
Length of output: 5404
🏁 Script executed:
# Search for List and Watch patterns related to MutatingWebhookConfiguration
rg -n '(List|Watch).*MutatingWebhookConfiguration|MutatingWebhookConfiguration.*(List|Watch)' --type go -A 2 -B 2Repository: openshift/oadp-operator
Length of output: 49
🏁 Script executed:
# Check for client operations on MutatingWebhookConfiguration
rg -n 'mutatingwebhookconfigurations|webhook.*config' --type go -i -A 2 | head -50Repository: openshift/oadp-operator
Length of output: 4585
Remove unnecessary list and watch verbs from MutatingWebhookConfiguration RBAC rule.
The controller in internal/controller/kubevirt_annotations_remover.go manages only a single webhook configuration by fixed name (see line 405-406). The actual operations used are: get (line 75), delete (line 84), and create/update/patch via CreateOrUpdate (line 418). The list and watch verbs are not used and should be removed to maintain least privilege.
Update the verbs to: create, delete, get, patch, update.
🧰 Tools
🪛 Trivy (0.69.1)
[error] 38-49: Manage webhookconfigurations
ClusterRole 'manager-role' should not have access to resources ["mutatingwebhookconfigurations", "validatingwebhookconfigurations"] for verbs ["create", "update", "patch", "delete", "deletecollection", "impersonate", "*"]
Rule: KSV-0114
(IaC/Kubernetes)
🤖 Prompt for AI Agents
In `@config/rbac/role.yaml` around lines 38 - 49, The MutatingWebhookConfiguration
RBAC rule grants unnecessary permissions; edit the role definition in
config/rbac/role.yaml and remove the "list" and "watch" verbs for apiGroups:
admissionregistration.k8s.io / resources: mutatingwebhookconfigurations so the
verbs are exactly "create", "delete", "get", "patch", "update" (this aligns with
how internal/controller/kubevirt_annotations_remover.go uses Get (line ~75),
Delete (line ~84) and CreateOrUpdate (around line ~418) for the fixed-name
webhook).
|
/retest |
|
@weshayutin: The following tests failed, say
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. |
1 participant
Why the changes were made
Unclear atm how quickly the kubevirt org will be able to remove velero annotations:
https://issues.redhat.com/browse/CNV-75370
How to test the changes made