DNM: add virt annotation remover webhook#2088
Open
weshayutin wants to merge 3 commits intoopenshift:oadp-devfrom
Open
DNM: add virt annotation remover webhook#2088weshayutin wants to merge 3 commits intoopenshift:oadp-devfrom
weshayutin wants to merge 3 commits intoopenshift:oadp-devfrom
Conversation
Contributor
WalkthroughAdds a KubeVirt Annotations Remover feature: API types and deepcopy support, CRD/CSV and RBAC updates, manager env var, new reconciler managing Service/Deployment/MutatingWebhookConfiguration, readiness integration, and comprehensive unit tests. Changes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes ✨ Finishing touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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}
Contributor
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
Comment on lines
+38
to
+49
| - apiGroups: | ||
| - admissionregistration.k8s.io | ||
| resources: | ||
| - mutatingwebhookconfigurations | ||
| verbs: | ||
| - create | ||
| - delete | ||
| - get | ||
| - list | ||
| - patch | ||
| - update | ||
| - watch |
Contributor
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).
Contributor
Author
|
/retest |
Contributor
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 `@internal/controller/readiness.go`:
- Around line 301-310: The function
updateKubevirtAnnotationsRemoverReadinessCondition is left unterminated and its
logic was accidentally merged into updateKubevirtDatamoverReadinessCondition,
causing duplicate Name fields and duplicate isReady assignments and wrong
condition types; fix by restoring separation: close
updateKubevirtAnnotationsRemoverReadinessCondition with its own return path
(handle !r.checkKubevirtAnnotationsRemoverEnabled() by setting
apimeta.SetStatusCondition with
oadpv1alpha1.ConditionKubevirtAnnotationsRemoverReady and returning), then
remove all KubevirtAnnotationsRemover-specific code from
updateKubevirtDatamoverReadinessCondition, ensure that
updateKubevirtDatamoverReadinessCondition uses
r.checkKubevirtDatamoverEnabled(), sets apimeta.SetStatusCondition with
oadpv1alpha1.ConditionKubevirtDatamoverReady, eliminates duplicate Name fields
and duplicate isReady assignments, and return the correct (isReady, error) from
each function.
Comment on lines
+301
to
+310
| // updateKubevirtAnnotationsRemoverReadinessCondition checks the kubevirt-velero-annotations-remover | ||
| // deployment readiness and updates the condition. | ||
| func (r *DataProtectionApplicationReconciler) updateKubevirtAnnotationsRemoverReadinessCondition() (bool, error) { | ||
| // If not enabled, mark as disabled | ||
| if !r.checkKubevirtAnnotationsRemoverEnabled() { | ||
| apimeta.SetStatusCondition(&r.dpa.Status.Conditions, metav1.Condition{ | ||
| Type: oadpv1alpha1.ConditionKubevirtAnnotationsRemoverReady, | ||
| Status: metav1.ConditionTrue, | ||
| Reason: oadpv1alpha1.ReasonComponentDisabled, | ||
| Message: "KubeVirt annotations remover webhook is disabled", |
Contributor
There was a problem hiding this comment.
Fix broken readiness methods (compile error + mixed conditions).
Lines 301-310 and Lines 323-367 leave updateKubevirtAnnotationsRemoverReadinessCondition unterminated and inject KubevirtAnnotationsRemover logic into updateKubevirtDatamoverReadinessCondition, including a duplicate Name field and duplicate isReady assignment. This breaks the build and misreports readiness.
🔧 Proposed fix to separate the two readiness paths
func (r *DataProtectionApplicationReconciler) updateKubevirtAnnotationsRemoverReadinessCondition() (bool, error) {
// If not enabled, mark as disabled
if !r.checkKubevirtAnnotationsRemoverEnabled() {
apimeta.SetStatusCondition(&r.dpa.Status.Conditions, metav1.Condition{
Type: oadpv1alpha1.ConditionKubevirtAnnotationsRemoverReady,
Status: metav1.ConditionTrue,
Reason: oadpv1alpha1.ReasonComponentDisabled,
Message: "KubeVirt annotations remover webhook is disabled",
- })
+ })
+ return true, nil
+ }
+
+ deployment := &appsv1.Deployment{}
+ err := r.Get(r.Context, types.NamespacedName{
+ Name: kubevirtAnnotationsRemoverName,
+ Namespace: r.dpa.Namespace,
+ }, deployment)
+ if err != nil {
+ if k8serror.IsNotFound(err) {
+ apimeta.SetStatusCondition(&r.dpa.Status.Conditions, metav1.Condition{
+ Type: oadpv1alpha1.ConditionKubevirtAnnotationsRemoverReady,
+ Status: metav1.ConditionFalse,
+ Reason: oadpv1alpha1.ReasonComponentNotFound,
+ Message: "KubeVirt annotations remover deployment not found",
+ })
+ return false, nil
+ }
+ return false, err
+ }
+
+ replicas := int32(1)
+ if deployment.Spec.Replicas != nil {
+ replicas = *deployment.Spec.Replicas
+ }
+ isReady := deployment.Status.ReadyReplicas >= 1 &&
+ deployment.Status.ReadyReplicas == replicas
+
+ if isReady {
+ apimeta.SetStatusCondition(&r.dpa.Status.Conditions, metav1.Condition{
+ Type: oadpv1alpha1.ConditionKubevirtAnnotationsRemoverReady,
+ Status: metav1.ConditionTrue,
+ Reason: oadpv1alpha1.ReasonDeploymentReady,
+ Message: fmt.Sprintf("KubeVirt annotations remover ready: %d/%d replicas", deployment.Status.ReadyReplicas, replicas),
+ })
+ } else {
+ apimeta.SetStatusCondition(&r.dpa.Status.Conditions, metav1.Condition{
+ Type: oadpv1alpha1.ConditionKubevirtAnnotationsRemoverReady,
+ Status: metav1.ConditionFalse,
+ Reason: oadpv1alpha1.ReasonDeploymentNotReady,
+ Message: fmt.Sprintf("KubeVirt annotations remover not ready: %d/%d replicas ready", deployment.Status.ReadyReplicas, replicas),
+ })
}
-
-func (r *DataProtectionApplicationReconciler) updateKubevirtDatamoverReadinessCondition() (bool, error) {
+ return isReady, nil
+}
+
+func (r *DataProtectionApplicationReconciler) updateKubevirtDatamoverReadinessCondition() (bool, error) {
// If plugin not enabled, mark as disabled (which counts as ready)
if !r.checkKubevirtDatamoverEnabled() {
apimeta.SetStatusCondition(&r.dpa.Status.Conditions, metav1.Condition{
Type: oadpv1alpha1.ConditionKubevirtDatamoverReady,
Status: metav1.ConditionTrue,
@@
deployment := &appsv1.Deployment{}
err := r.Get(r.Context, types.NamespacedName{
- Name: kubevirtAnnotationsRemoverName,
Name: kubevirtDatamoverObjectName,
Namespace: r.dpa.Namespace,
}, deployment)
if err != nil {
if k8serror.IsNotFound(err) {
apimeta.SetStatusCondition(&r.dpa.Status.Conditions, metav1.Condition{
- Type: oadpv1alpha1.ConditionKubevirtAnnotationsRemoverReady,
- Status: metav1.ConditionFalse,
- Reason: oadpv1alpha1.ReasonComponentNotFound,
- Message: "KubeVirt annotations remover deployment not found",
Type: oadpv1alpha1.ConditionKubevirtDatamoverReady,
Status: metav1.ConditionFalse,
Reason: oadpv1alpha1.ReasonComponentNotFound,
Message: "KubeVirt DataMover controller deployment not found",
})
return false, nil
@@
- isReady := deployment.Status.ReadyReplicas >= 1 &&
- deployment.Status.ReadyReplicas == replicas
-
- if isReady {
- apimeta.SetStatusCondition(&r.dpa.Status.Conditions, metav1.Condition{
- Type: oadpv1alpha1.ConditionKubevirtAnnotationsRemoverReady,
- Status: metav1.ConditionTrue,
- Reason: oadpv1alpha1.ReasonDeploymentReady,
- Message: fmt.Sprintf("KubeVirt annotations remover ready: %d/%d replicas", deployment.Status.ReadyReplicas, replicas),
- })
- } else {
- apimeta.SetStatusCondition(&r.dpa.Status.Conditions, metav1.Condition{
- Type: oadpv1alpha1.ConditionKubevirtAnnotationsRemoverReady,
- Status: metav1.ConditionFalse,
- Reason: oadpv1alpha1.ReasonDeploymentNotReady,
- Message: fmt.Sprintf("KubeVirt annotations remover not ready: %d/%d replicas ready", deployment.Status.ReadyReplicas, replicas),
- })
- }
-
- isReady := deployment.Status.ReadyReplicas >= 1 && deployment.Status.ReadyReplicas == replicas
+ isReady := deployment.Status.ReadyReplicas >= 1 && deployment.Status.ReadyReplicas == replicasAlso applies to: 323-367
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/controller/readiness.go` around lines 301 - 310, The function
updateKubevirtAnnotationsRemoverReadinessCondition is left unterminated and its
logic was accidentally merged into updateKubevirtDatamoverReadinessCondition,
causing duplicate Name fields and duplicate isReady assignments and wrong
condition types; fix by restoring separation: close
updateKubevirtAnnotationsRemoverReadinessCondition with its own return path
(handle !r.checkKubevirtAnnotationsRemoverEnabled() by setting
apimeta.SetStatusCondition with
oadpv1alpha1.ConditionKubevirtAnnotationsRemoverReady and returning), then
remove all KubevirtAnnotationsRemover-specific code from
updateKubevirtDatamoverReadinessCondition, ensure that
updateKubevirtDatamoverReadinessCondition uses
r.checkKubevirtDatamoverEnabled(), sets apimeta.SetStatusCondition with
oadpv1alpha1.ConditionKubevirtDatamoverReady, eliminates duplicate Name fields
and duplicate isReady assignments, and return the correct (isReady, error) from
each function.
|
@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. |
kaovilai
reviewed
Feb 18, 2026
| - create | ||
| - delete | ||
| - get | ||
| - list |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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