From 838052eb0d1b47cc38b256ecce71031065168c51 Mon Sep 17 00:00:00 2001 From: bmangoen Date: Mon, 6 Oct 2025 11:00:44 +0200 Subject: [PATCH 1/4] Skip reconciliation when using sailoperator.io/ignore annotation Signed-off-by: bmangoen --- .../istiorevision/istiorevision_controller.go | 3 ++- pkg/predicate/predicate.go | 15 +++++++++++++++ 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/controllers/istiorevision/istiorevision_controller.go b/controllers/istiorevision/istiorevision_controller.go index 6d4dd4907..0b276cb73 100644 --- a/controllers/istiorevision/istiorevision_controller.go +++ b/controllers/istiorevision/istiorevision_controller.go @@ -321,7 +321,8 @@ func (r *Reconciler) SetupWithManager(mgr ctrl.Manager) error { // cluster-scoped resources Watches(&rbacv1.ClusterRole{}, ownedResourceHandler). Watches(&rbacv1.ClusterRoleBinding{}, ownedResourceHandler). - Watches(&admissionv1.MutatingWebhookConfiguration{}, ownedResourceHandler). + // TODO: this is a temporary hack until we implement the correct solution to ignore specific fields in the resource. + Watches(&admissionv1.MutatingWebhookConfiguration{}, ownedResourceHandler, builder.WithPredicates(predicate2.IgnoreUpdateWhenAnnotation())). Watches(&admissionv1.ValidatingWebhookConfiguration{}, ownedResourceHandler, builder.WithPredicates(validatingWebhookConfigPredicate())). // +lint-watches:ignore: IstioCNI (not found in charts, but this controller needs to watch it to update the IstioRevision status) diff --git a/pkg/predicate/predicate.go b/pkg/predicate/predicate.go index fc4f7466c..1e0a8ed63 100644 --- a/pkg/predicate/predicate.go +++ b/pkg/predicate/predicate.go @@ -25,3 +25,18 @@ func IgnoreUpdate() predicate.Funcs { UpdateFunc: func(e event.UpdateEvent) bool { return false }, } } + +// IgnoreUpdateWhenAnnotation returns a predicate that ignores update events +// when the sailoperator.io/ignore annotation is set to "true" on the resource. +// TODO: this is a temporary hack until we implement the correct solution to ignore specific fields in the resource. +func IgnoreUpdateWhenAnnotation() predicate.Funcs { + return predicate.Funcs{ + UpdateFunc: func(e event.UpdateEvent) bool { + if e.ObjectOld == nil || e.ObjectNew == nil { + return false + } + + return e.ObjectNew.GetAnnotations()["sailoperator.io/ignore"] != "true" + }, + } +} From e169df4b192fd4ae8f8f23e8d57288d52b115136 Mon Sep 17 00:00:00 2001 From: bmangoen Date: Mon, 6 Oct 2025 15:22:15 +0200 Subject: [PATCH 2/4] Watch all resources with IgnoreUpdateWhenAnnotation predicate Signed-off-by: bmangoen --- .../istiorevision/istiorevision_controller.go | 39 ++++++++++++------- pkg/predicate/predicate_test.go | 24 ++++++++++++ 2 files changed, 48 insertions(+), 15 deletions(-) diff --git a/controllers/istiorevision/istiorevision_controller.go b/controllers/istiorevision/istiorevision_controller.go index 0b276cb73..299ac1c83 100644 --- a/controllers/istiorevision/istiorevision_controller.go +++ b/controllers/istiorevision/istiorevision_controller.go @@ -289,41 +289,50 @@ func (r *Reconciler) SetupWithManager(mgr ctrl.Manager) error { Watches(&v1.IstioRevision{}, mainObjectHandler). Named("istiorevision"). + // We use predicate.IgnoreUpdateWhenAnnotation() so that we skip the reconciliation + // when the sailoperator.io/ignore annotation is set to "true" on the resource. + // TODO: this is a temporary hack until we implement the correct solution to ignore specific fields in the resource. + // namespaced resources - Watches(&corev1.ConfigMap{}, ownedResourceHandler). - Watches(&appsv1.Deployment{}, ownedResourceHandler). // we don't ignore the status here because we use it to calculate the IstioRevision status + Watches(&corev1.ConfigMap{}, ownedResourceHandler, builder.WithPredicates(predicate2.IgnoreUpdateWhenAnnotation())). + // We don't ignore the status for Deployments because we use it to calculate the IstioRevision status + Watches(&appsv1.Deployment{}, ownedResourceHandler, builder.WithPredicates(predicate2.IgnoreUpdateWhenAnnotation())). // +lint-watches:ignore: Endpoints (older versions of istiod chart create Endpoints for remote installs, but this controller watches EndpointSlices) // +lint-watches:ignore: EndpointSlice (istiod chart creates Endpoints for remote installs, but this controller watches EndpointSlices) - Watches(&discoveryv1.EndpointSlice{}, endpointSliceHandler). - Watches(&corev1.Service{}, ownedResourceHandler, builder.WithPredicates(ignoreStatusChange())). + Watches(&discoveryv1.EndpointSlice{}, endpointSliceHandler, builder.WithPredicates(predicate2.IgnoreUpdateWhenAnnotation())). + Watches(&corev1.Service{}, ownedResourceHandler, + builder.WithPredicates(ignoreStatusChange(), predicate2.IgnoreUpdateWhenAnnotation())). // +lint-watches:ignore: NetworkPolicy (FIXME: NetworkPolicy has not yet been added upstream, but is WIP) - Watches(&networkingv1.NetworkPolicy{}, ownedResourceHandler, builder.WithPredicates(ignoreStatusChange())). + Watches(&networkingv1.NetworkPolicy{}, ownedResourceHandler, + builder.WithPredicates(ignoreStatusChange(), predicate2.IgnoreUpdateWhenAnnotation())). // We use predicate.IgnoreUpdate() so that we skip the reconciliation when a pull secret is added to the ServiceAccount. // This is necessary so that we don't remove the newly-added secret. // TODO: this is a temporary hack until we implement the correct solution on the Helm-render side Watches(&corev1.ServiceAccount{}, ownedResourceHandler, builder.WithPredicates(predicate2.IgnoreUpdate())). - Watches(&rbacv1.Role{}, ownedResourceHandler). - Watches(&rbacv1.RoleBinding{}, ownedResourceHandler). - Watches(&policyv1.PodDisruptionBudget{}, ownedResourceHandler, builder.WithPredicates(ignoreStatusChange())). - Watches(&autoscalingv2.HorizontalPodAutoscaler{}, ownedResourceHandler, builder.WithPredicates(ignoreStatusChange())). + Watches(&rbacv1.Role{}, ownedResourceHandler, builder.WithPredicates(predicate2.IgnoreUpdateWhenAnnotation())). + Watches(&rbacv1.RoleBinding{}, ownedResourceHandler, builder.WithPredicates(predicate2.IgnoreUpdateWhenAnnotation())). + Watches(&policyv1.PodDisruptionBudget{}, ownedResourceHandler, + builder.WithPredicates(ignoreStatusChange(), predicate2.IgnoreUpdateWhenAnnotation())). + Watches(&autoscalingv2.HorizontalPodAutoscaler{}, ownedResourceHandler, + builder.WithPredicates(ignoreStatusChange(), predicate2.IgnoreUpdateWhenAnnotation())). // +lint-watches:ignore: Namespace (not found in charts, but must be watched to reconcile IstioRevision when its namespace is created) - Watches(&corev1.Namespace{}, nsHandler, builder.WithPredicates(ignoreStatusChange())). + Watches(&corev1.Namespace{}, nsHandler, builder.WithPredicates(ignoreStatusChange()), builder.WithPredicates(predicate2.IgnoreUpdateWhenAnnotation())). // +lint-watches:ignore: Pod (not found in charts, but must be watched to reconcile IstioRevision when a pod references it) - Watches(&corev1.Pod{}, podHandler, builder.WithPredicates(ignoreStatusChange())). + Watches(&corev1.Pod{}, podHandler, builder.WithPredicates(ignoreStatusChange(), predicate2.IgnoreUpdateWhenAnnotation())). // +lint-watches:ignore: IstioRevisionTag (not found in charts, but must be watched to reconcile IstioRevision when a pod references it) Watches(&v1.IstioRevisionTag{}, revisionTagHandler). // cluster-scoped resources - Watches(&rbacv1.ClusterRole{}, ownedResourceHandler). - Watches(&rbacv1.ClusterRoleBinding{}, ownedResourceHandler). - // TODO: this is a temporary hack until we implement the correct solution to ignore specific fields in the resource. + 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.ValidatingWebhookConfiguration{}, ownedResourceHandler, builder.WithPredicates(validatingWebhookConfigPredicate())). + Watches(&admissionv1.ValidatingWebhookConfiguration{}, ownedResourceHandler, + builder.WithPredicates(validatingWebhookConfigPredicate(), 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). diff --git a/pkg/predicate/predicate_test.go b/pkg/predicate/predicate_test.go index 08dca50cb..e67186ff1 100644 --- a/pkg/predicate/predicate_test.go +++ b/pkg/predicate/predicate_test.go @@ -18,6 +18,8 @@ import ( "testing" "github.com/stretchr/testify/assert" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "sigs.k8s.io/controller-runtime/pkg/event" ) @@ -28,3 +30,25 @@ func TestIgnoreUpdate(t *testing.T) { assert.Equal(t, true, predicate.Delete(event.DeleteEvent{})) assert.Equal(t, true, predicate.Generic(event.GenericEvent{})) } + +func TestIgnoreUpdateWhenAnnotation(t *testing.T) { + predicate := IgnoreUpdateWhenAnnotation() + assert.Equal(t, false, predicate.Update(event.UpdateEvent{ + ObjectOld: &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{}, + }, + ObjectNew: &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + "sailoperator.io/ignore": "true", + }, + }, + Data: map[string]string{ + "foo": "bar", + }, + }, + })) + assert.Equal(t, true, predicate.Create(event.CreateEvent{})) + assert.Equal(t, true, predicate.Delete(event.DeleteEvent{})) + assert.Equal(t, true, predicate.Generic(event.GenericEvent{})) +} From b0a21f131790204455f032e00475cbd3796572da Mon Sep 17 00:00:00 2001 From: bmangoen Date: Wed, 8 Oct 2025 17:47:48 +0200 Subject: [PATCH 3/4] fix: add integration tests This commit adds the step to test the skip reconciliation when the sailoperator.io/ignore annotation is set to true on a resource. Signed-off-by: bmangoen --- .../istiorevision/istiorevision_controller.go | 4 --- pkg/predicate/predicate.go | 1 - tests/integration/api/istiorevision_test.go | 26 +++++++++++++++++++ 3 files changed, 26 insertions(+), 5 deletions(-) diff --git a/controllers/istiorevision/istiorevision_controller.go b/controllers/istiorevision/istiorevision_controller.go index 299ac1c83..2026ed13b 100644 --- a/controllers/istiorevision/istiorevision_controller.go +++ b/controllers/istiorevision/istiorevision_controller.go @@ -289,10 +289,6 @@ func (r *Reconciler) SetupWithManager(mgr ctrl.Manager) error { Watches(&v1.IstioRevision{}, mainObjectHandler). Named("istiorevision"). - // We use predicate.IgnoreUpdateWhenAnnotation() so that we skip the reconciliation - // when the sailoperator.io/ignore annotation is set to "true" on the resource. - // TODO: this is a temporary hack until we implement the correct solution to ignore specific fields in the resource. - // namespaced resources Watches(&corev1.ConfigMap{}, ownedResourceHandler, builder.WithPredicates(predicate2.IgnoreUpdateWhenAnnotation())). // We don't ignore the status for Deployments because we use it to calculate the IstioRevision status diff --git a/pkg/predicate/predicate.go b/pkg/predicate/predicate.go index 1e0a8ed63..14c2c0c65 100644 --- a/pkg/predicate/predicate.go +++ b/pkg/predicate/predicate.go @@ -28,7 +28,6 @@ func IgnoreUpdate() predicate.Funcs { // IgnoreUpdateWhenAnnotation returns a predicate that ignores update events // when the sailoperator.io/ignore annotation is set to "true" on the resource. -// TODO: this is a temporary hack until we implement the correct solution to ignore specific fields in the resource. func IgnoreUpdateWhenAnnotation() predicate.Funcs { return predicate.Funcs{ UpdateFunc: func(e event.UpdateEvent) bool { diff --git a/tests/integration/api/istiorevision_test.go b/tests/integration/api/istiorevision_test.go index f13fbdd6e..e1cd55d04 100644 --- a/tests/integration/api/istiorevision_test.go +++ b/tests/integration/api/istiorevision_test.go @@ -561,6 +561,32 @@ var _ = Describe("IstioRevision resource", Label("istiorevision"), Ordered, func Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(sa), sa)).To(Succeed()) Expect(sa.ImagePullSecrets).To(ContainElement(corev1.LocalObjectReference{Name: "other-pull-secret"})) }) + + It("skips reconcile when sailoperator.io/ignore annotation is set to true on a resource", func() { + waitForInFlightReconcileToFinish() + + webhook := &admissionv1.MutatingWebhookConfiguration{ + ObjectMeta: metav1.ObjectMeta{ + Name: "istio-sidecar-injector-" + revName + "-" + istioNamespace, + }, + } + Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(webhook), webhook)).To(Succeed()) + + GinkgoWriter.Println("webhook:", webhook) + + expectNoReconciliation(istioRevisionController, func() { + By("adding sailoperator.io/ignore annotation to ConfigMap") + webhook.Annotations = map[string]string{ + "sailoperator.io/ignore": "true", + } + webhook.Labels["app"] = "sidecar-injector-test" + Expect(k8sClient.Update(ctx, webhook)).To(Succeed()) + }) + + Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(webhook), webhook)).To(Succeed()) + Expect(webhook.Annotations["sailoperator.io/ignore"]).To(Equal("true")) + Expect(webhook.Labels["app"]).To(Equal("sidecar-injector-test")) + }) }) DescribeTableSubtree("reconciling when revision is in use", From 682c3dc489f3dda31f35a21b18e91991d17e96a6 Mon Sep 17 00:00:00 2001 From: bmangoen Date: Fri, 10 Oct 2025 11:10:10 +0200 Subject: [PATCH 4/4] Add more steps for unit tests This commit adds more unit tests cases for IgnoreUpdateWhenAnnotation predicate. Signed-off-by: bmangoen --- pkg/predicate/predicate_test.go | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/pkg/predicate/predicate_test.go b/pkg/predicate/predicate_test.go index e67186ff1..8a876e107 100644 --- a/pkg/predicate/predicate_test.go +++ b/pkg/predicate/predicate_test.go @@ -33,6 +33,38 @@ func TestIgnoreUpdate(t *testing.T) { func TestIgnoreUpdateWhenAnnotation(t *testing.T) { predicate := IgnoreUpdateWhenAnnotation() + // Object does not contain sailoperator.io/ignore annotation + // so reconciliation should be done and both objects should be equal + assert.Equal(t, true, predicate.Update(event.UpdateEvent{ + ObjectOld: &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{}, + }, + ObjectNew: &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{}, + Data: map[string]string{ + "foo": "bar", + }, + }, + })) + // Object has sailoperator.io/ignore annotation set with wrong value + // so reconciliation should be done and both objects should be equal + assert.Equal(t, true, predicate.Update(event.UpdateEvent{ + ObjectOld: &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{}, + }, + ObjectNew: &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + "sailoperator.io/ignore": "wrongvalue", + }, + }, + Data: map[string]string{ + "foo": "bar", + }, + }, + })) + // Object has sailoperator.io/ignore annotation set to "true" + // so reconciliation should be skipped assert.Equal(t, false, predicate.Update(event.UpdateEvent{ ObjectOld: &corev1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{},