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",