From 3d2f26c0a24c2d07e285eb2e8082efe11501861b Mon Sep 17 00:00:00 2001 From: Daniel Grimm Date: Thu, 5 Mar 2026 16:53:44 +0100 Subject: [PATCH] Fix infinite reconciliation on webhook resources 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 | 32 +++++++++------ tests/integration/api/istiorevision_test.go | 40 +++++++++++++++++++ 2 files changed, 59 insertions(+), 13 deletions(-) diff --git a/controllers/istiorevision/istiorevision_controller.go b/controllers/istiorevision/istiorevision_controller.go index b54b55eb5..85419a491 100644 --- a/controllers/istiorevision/istiorevision_controller.go +++ b/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--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) }, } } @@ -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 } } } diff --git a/tests/integration/api/istiorevision_test.go b/tests/integration/api/istiorevision_test.go index cbba82419..a0c23b860 100644 --- a/tests/integration/api/istiorevision_test.go +++ b/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",