From 518ffd957289b219c24fe69fa5480b1d747fbd66 Mon Sep 17 00:00:00 2001 From: Daniel Grimm Date: Fri, 6 Mar 2026 17:30:59 +0100 Subject: [PATCH] Fix infinite reconciliation on webhook resources (#1651) istiod updates the CABundle and FailurePolicy on its ValidatingWebhookConfigurations and MutatingWebhookConfigurations. The controller needs to ignore those changes to avoid continuous reconciliation of the control plane. Signed-off-by: Daniel Grimm --- .../istiorevision/istiorevision_controller.go | 33 +++++++----- tests/integration/api/istiorevision_test.go | 50 +++++++++++++++++++ 2 files changed, 70 insertions(+), 13 deletions(-) diff --git a/controllers/istiorevision/istiorevision_controller.go b/controllers/istiorevision/istiorevision_controller.go index 6fd8757e7f..d87bca35b5 100644 --- a/controllers/istiorevision/istiorevision_controller.go +++ b/controllers/istiorevision/istiorevision_controller.go @@ -20,7 +20,6 @@ import ( "fmt" "path" "reflect" - "regexp" "github.com/go-logr/logr" v1 "github.com/istio-ecosystem/sail-operator/api/v1" @@ -283,8 +282,10 @@ func (r *Reconciler) SetupWithManager(mgr ctrl.Manager) error { // cluster-scoped resources Watches(&rbacv1.ClusterRole{}, ownedResourceHandler). Watches(&rbacv1.ClusterRoleBinding{}, ownedResourceHandler). - Watches(&admissionv1.MutatingWebhookConfiguration{}, ownedResourceHandler). - Watches(&admissionv1.ValidatingWebhookConfiguration{}, ownedResourceHandler, builder.WithPredicates(validatingWebhookConfigPredicate())). + Watches(&admissionv1.MutatingWebhookConfiguration{}, ownedResourceHandler, + builder.WithPredicates(webhookConfigPredicate())). + Watches(&admissionv1.ValidatingWebhookConfiguration{}, ownedResourceHandler, + builder.WithPredicates(webhookConfigPredicate())). // +lint-watches:ignore: IstioCNI (not found in charts, but this controller needs to watch it to update the IstioRevision status) Watches(&v1.IstioCNI{}, istioCniHandler). @@ -663,21 +664,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--validator and istio-validator[-]- - // 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) }, } } @@ -686,9 +687,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 } } } diff --git a/tests/integration/api/istiorevision_test.go b/tests/integration/api/istiorevision_test.go index 4970418f45..3d2492359f 100644 --- a/tests/integration/api/istiorevision_test.go +++ b/tests/integration/api/istiorevision_test.go @@ -552,6 +552,56 @@ var _ = Describe("IstioRevision resource", Label("istiorevision"), Ordered, func Expect(sa.ImagePullSecrets).To(ContainElement(corev1.LocalObjectReference{Name: "other-pull-secret"})) }) + 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()) + + beforeCount := getIstioRevisionReconcileCount(Default) + + 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()) + + Consistently(func(g Gomega) { + afterCount := getIstioRevisionReconcileCount(g) + g.Expect(afterCount).To(Equal(beforeCount)) + }, 5*time.Second).Should(Succeed(), "IstioRevision was reconciled when it shouldn't have been") + }) + + 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()) + + beforeCount := getIstioRevisionReconcileCount(Default) + + 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()) + + Consistently(func(g Gomega) { + afterCount := getIstioRevisionReconcileCount(g) + g.Expect(afterCount).To(Equal(beforeCount)) + }, 5*time.Second).Should(Succeed(), "IstioRevision was reconciled when it shouldn't have been") + }) + DescribeTableSubtree("reconciling when revision is in use", func(name, revision string, nsLabels, podLabels map[string]string) { BeforeAll(func() {