Fix infinite reconciliation on webhook resources

controllers/istiorevision/istiorevision_controller.go

@@ -19,7 +19,6 @@ import (
 	"errors"
 	"fmt"
 	"reflect"
-	"regexp"
 
 	"github.com/go-logr/logr"
 	v1 "github.com/istio-ecosystem/sail-operator/api/v1"
@@ -312,9 +311,10 @@ func (r *Reconciler) SetupWithManager(mgr ctrl.Manager) error {
 		// cluster-scoped resources
 		Watches(&rbacv1.ClusterRole{}, ownedResourceHandler, builder.WithPredicates(predicate2.IgnoreUpdateWhenAnnotation())).
 		Watches(&rbacv1.ClusterRoleBinding{}, ownedResourceHandler, builder.WithPredicates(predicate2.IgnoreUpdateWhenAnnotation())).
-		Watches(&admissionv1.MutatingWebhookConfiguration{}, ownedResourceHandler, builder.WithPredicates(predicate2.IgnoreUpdateWhenAnnotation())).
+		Watches(&admissionv1.MutatingWebhookConfiguration{}, ownedResourceHandler,
+			builder.WithPredicates(webhookConfigPredicate(), predicate2.IgnoreUpdateWhenAnnotation())).
 		Watches(&admissionv1.ValidatingWebhookConfiguration{}, ownedResourceHandler,
-			builder.WithPredicates(validatingWebhookConfigPredicate(), predicate2.IgnoreUpdateWhenAnnotation())).
+			builder.WithPredicates(webhookConfigPredicate(), predicate2.IgnoreUpdateWhenAnnotation())).
 
 		// +lint-watches:ignore: IstioCNI (not found in charts, but this controller needs to watch it to update the IstioRevision status)
 		Watches(&v1.IstioCNI{}, istioCniHandler).
@@ -741,21 +741,21 @@ func specWasUpdated(oldObject client.Object, newObject client.Object) bool {
 	return oldObject.GetGeneration() != newObject.GetGeneration()
 }
 
-func validatingWebhookConfigPredicate() predicate.Funcs {
+func webhookConfigPredicate() predicate.Funcs {
 	return predicate.Funcs{
 		UpdateFunc: func(e event.TypedUpdateEvent[client.Object]) bool {
 			if e.ObjectOld == nil || e.ObjectNew == nil {
 				return false
 			}
 
-			if matched, _ := regexp.MatchString("istiod-.*-validator|istio-validator.*", e.ObjectNew.GetName()); matched {
-				// Istiod updates the caBundle and failurePolicy fields in istiod-<ns>-validator and istio-validator[-<rev>]-<ns>
-				// webhook configs. We must ignore changes to these fields to prevent an endless update loop.
-				clearIgnoredFields(e.ObjectOld)
-				clearIgnoredFields(e.ObjectNew)
-				return !reflect.DeepEqual(e.ObjectNew, e.ObjectOld)
-			}
-			return true
+			// Istiod updates the caBundle and failurePolicy fields in its webhook configs.
+			// We must ignore changes to these fields to prevent an endless update loop.
+			// We must use deep copies to avoid mutating the shared informer cache.
+			oldCopy := e.ObjectOld.DeepCopyObject().(client.Object)
+			newCopy := e.ObjectNew.DeepCopyObject().(client.Object)
+			clearIgnoredFields(oldCopy)
+			clearIgnoredFields(newCopy)
+			return !reflect.DeepEqual(newCopy, oldCopy)
 		},
 	}
 }
@@ -764,9 +764,15 @@ func clearIgnoredFields(obj client.Object) {
 	obj.SetResourceVersion("")
 	obj.SetGeneration(0)
 	obj.SetManagedFields(nil)
-	if webhookConfig, ok := obj.(*admissionv1.ValidatingWebhookConfiguration); ok {
+	switch webhookConfig := obj.(type) {
+	case *admissionv1.ValidatingWebhookConfiguration:
 		for i := range len(webhookConfig.Webhooks) {
 			webhookConfig.Webhooks[i].FailurePolicy = nil
+			webhookConfig.Webhooks[i].ClientConfig.CABundle = nil
+		}
+	case *admissionv1.MutatingWebhookConfiguration:
+		for i := range len(webhookConfig.Webhooks) {
+			webhookConfig.Webhooks[i].ClientConfig.CABundle = nil
 		}
 	}
 }

tests/integration/api/istiorevision_test.go

@@ -680,6 +680,46 @@ var _ = Describe("IstioRevision resource", Label("istiorevision"), Ordered, func
 			Expect(webhook.Annotations["sailoperator.io/ignore"]).To(Equal("true"))
 			Expect(webhook.Labels["app"]).To(Equal("sidecar-injector-test"))
 		})
+
+		It("skips reconcile when only caBundle and failurePolicy are updated on MutatingWebhookConfiguration", func() {
+			waitForInFlightReconcileToFinish()
+
+			webhook := &admissionv1.MutatingWebhookConfiguration{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "istio-sidecar-injector-" + revName + "-" + istioNamespace,
+				},
+			}
+			Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(webhook), webhook)).To(Succeed())
+
+			expectNoReconciliation(istioRevisionController, func() {
+				By("updating caBundle and failurePolicy on MutatingWebhookConfiguration")
+				for i := range webhook.Webhooks {
+					webhook.Webhooks[i].ClientConfig.CABundle = []byte("new-ca-bundle-data")
+					webhook.Webhooks[i].FailurePolicy = ptr.Of(admissionv1.Fail)
+				}
+				Expect(k8sClient.Update(ctx, webhook)).To(Succeed())
+			})
+		})
+
+		It("skips reconcile when only caBundle and failurePolicy are updated on ValidatingWebhookConfiguration", func() {
+			waitForInFlightReconcileToFinish()
+
+			webhook := &admissionv1.ValidatingWebhookConfiguration{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: fmt.Sprintf("istio-validator-%s-%s", revName, istioNamespace),
+				},
+			}
+			Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(webhook), webhook)).To(Succeed())
+
+			expectNoReconciliation(istioRevisionController, func() {
+				By("updating caBundle and failurePolicy on ValidatingWebhookConfiguration")
+				for i := range webhook.Webhooks {
+					webhook.Webhooks[i].ClientConfig.CABundle = []byte("new-ca-bundle-data")
+					webhook.Webhooks[i].FailurePolicy = ptr.Of(admissionv1.Fail)
+				}
+				Expect(k8sClient.Update(ctx, webhook)).To(Succeed())
+			})
+		})
 	})
 
 	DescribeTableSubtree("reconciling when revision is in use",