From bb9df067a5c032e1dce4d5555c3fdd2d18ff4dd7 Mon Sep 17 00:00:00 2001 From: Daniel Grimm Date: Fri, 6 Mar 2026 15:51:52 +0100 Subject: [PATCH 1/2] feat: add FieldIgnore functionality this is a cleaner API around ignoring certain fields in watches and when installing charts, through a PostRender func. The goal is to eventually expose this API to the user, but for now it'll just help to keep track of our default ignores that we have implemented to fix specific bugs. Signed-off-by: Daniel Grimm --- cmd/main.go | 4 +- controllers/istiocni/istiocni_controller.go | 28 +- .../istiorevision/istiorevision_controller.go | 93 +-- .../istiorevisiontag_controller.go | 4 +- controllers/webhook/webhook_controller.go | 6 +- controllers/ztunnel/ztunnel_controller.go | 23 +- pkg/fieldignore/defaults.go | 43 ++ pkg/fieldignore/fieldignore.go | 359 ++++++++++ pkg/fieldignore/fieldignore_test.go | 672 ++++++++++++++++++ pkg/helm/chartmanager.go | 44 +- pkg/helm/postrenderer.go | 74 +- pkg/helm/postrenderer_test.go | 192 ++++- tests/integration/api/fieldignore_test.go | 404 +++++++++++ tests/integration/api/istiorevision_test.go | 40 -- tests/integration/api/suite_test.go | 4 +- 15 files changed, 1795 insertions(+), 195 deletions(-) create mode 100644 pkg/fieldignore/defaults.go create mode 100644 pkg/fieldignore/fieldignore.go create mode 100644 pkg/fieldignore/fieldignore_test.go create mode 100644 tests/integration/api/fieldignore_test.go diff --git a/cmd/main.go b/cmd/main.go index c2b02b151a..9d456ae7fb 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -30,6 +30,7 @@ import ( "github.com/istio-ecosystem/sail-operator/controllers/ztunnel" "github.com/istio-ecosystem/sail-operator/pkg/config" "github.com/istio-ecosystem/sail-operator/pkg/enqueuelogger" + "github.com/istio-ecosystem/sail-operator/pkg/fieldignore" "github.com/istio-ecosystem/sail-operator/pkg/helm" "github.com/istio-ecosystem/sail-operator/pkg/scheme" "github.com/istio-ecosystem/sail-operator/pkg/version" @@ -202,7 +203,8 @@ func main() { os.Exit(1) } - chartManager := helm.NewChartManager(mgr.GetConfig(), os.Getenv("HELM_DRIVER")) + chartManager := helm.NewChartManager(mgr.GetConfig(), os.Getenv("HELM_DRIVER"), + helm.WithFieldIgnoreRules(fieldignore.IntoUntypedAll(fieldignore.DefaultRules))) err = istio.NewReconciler(reconcilerCfg, mgr.GetClient(), mgr.GetScheme(), tlsConfig). SetupWithManager(mgr) diff --git a/controllers/istiocni/istiocni_controller.go b/controllers/istiocni/istiocni_controller.go index b0724f72cf..ba54dea90a 100644 --- a/controllers/istiocni/istiocni_controller.go +++ b/controllers/istiocni/istiocni_controller.go @@ -25,6 +25,7 @@ import ( "github.com/istio-ecosystem/sail-operator/pkg/constants" "github.com/istio-ecosystem/sail-operator/pkg/enqueuelogger" "github.com/istio-ecosystem/sail-operator/pkg/errlist" + "github.com/istio-ecosystem/sail-operator/pkg/fieldignore" "github.com/istio-ecosystem/sail-operator/pkg/helm" "github.com/istio-ecosystem/sail-operator/pkg/predicate" sharedreconcile "github.com/istio-ecosystem/sail-operator/pkg/reconcile" @@ -161,17 +162,20 @@ func (r *Reconciler) SetupWithManager(mgr ctrl.Manager) error { Named("istiocni"). // namespaced resources - Watches(&corev1.ConfigMap{}, ownedResourceHandler). - Watches(&appsv1.DaemonSet{}, ownedResourceHandler). - Watches(&corev1.ResourceQuota{}, ownedResourceHandler). + Watches(&corev1.ConfigMap{}, ownedResourceHandler, + builder.WithPredicates(fieldignore.RulesFor(fieldignore.DefaultRules, &corev1.ConfigMap{}).NewPredicate())). + Watches(&appsv1.DaemonSet{}, ownedResourceHandler, + builder.WithPredicates(fieldignore.RulesFor(fieldignore.DefaultRules, &appsv1.DaemonSet{}).NewPredicate())). + Watches(&corev1.ResourceQuota{}, ownedResourceHandler, + builder.WithPredicates(fieldignore.RulesFor(fieldignore.DefaultRules, &corev1.ResourceQuota{}).NewPredicate())). // +lint-watches:ignore: NetworkPolicy (FIXME: NetworkPolicy has not yet been added upstream, but is WIP) - Watches(&networkingv1.NetworkPolicy{}, ownedResourceHandler, builder.WithPredicates(predicate.IgnoreUpdate())). - - // 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(predicate.IgnoreUpdate())). + Watches(&networkingv1.NetworkPolicy{}, ownedResourceHandler, + builder.WithPredicates(fieldignore.RulesFor(fieldignore.DefaultRules, &networkingv1.NetworkPolicy{}).NewPredicate(), predicate.IgnoreUpdate())). + Watches(&corev1.ServiceAccount{}, ownedResourceHandler, + builder.WithPredicates( + fieldignore.RulesFor(fieldignore.DefaultRules, &corev1.ServiceAccount{}).NewPredicate(), + predicate.IgnoreUpdateWhenAnnotation())). // TODO: only register NetAttachDef if the CRD is installed (may also need to watch for CRD creation) // Owns(&multusv1.NetworkAttachmentDefinition{}). @@ -179,8 +183,10 @@ func (r *Reconciler) SetupWithManager(mgr ctrl.Manager) error { // cluster-scoped resources // +lint-watches:ignore: Namespace (not present in charts, but must be watched to reconcile IstioCni when its namespace is created) Watches(&corev1.Namespace{}, namespaceHandler). - Watches(&rbacv1.ClusterRole{}, ownedResourceHandler). - Watches(&rbacv1.ClusterRoleBinding{}, ownedResourceHandler). + Watches(&rbacv1.ClusterRole{}, ownedResourceHandler, + builder.WithPredicates(fieldignore.RulesFor(fieldignore.DefaultRules, &rbacv1.ClusterRole{}).NewPredicate())). + Watches(&rbacv1.ClusterRoleBinding{}, ownedResourceHandler, + builder.WithPredicates(fieldignore.RulesFor(fieldignore.DefaultRules, &rbacv1.ClusterRoleBinding{}).NewPredicate())). Complete(reconciler.NewStandardReconcilerWithFinalizer[*v1.IstioCNI](r.Client, r.Reconcile, r.Finalize, constants.FinalizerName)) } diff --git a/controllers/istiorevision/istiorevision_controller.go b/controllers/istiorevision/istiorevision_controller.go index b96e8e20f2..5b5bda7876 100644 --- a/controllers/istiorevision/istiorevision_controller.go +++ b/controllers/istiorevision/istiorevision_controller.go @@ -26,6 +26,7 @@ import ( "github.com/istio-ecosystem/sail-operator/pkg/constants" "github.com/istio-ecosystem/sail-operator/pkg/enqueuelogger" "github.com/istio-ecosystem/sail-operator/pkg/errlist" + "github.com/istio-ecosystem/sail-operator/pkg/fieldignore" "github.com/istio-ecosystem/sail-operator/pkg/helm" predicate2 "github.com/istio-ecosystem/sail-operator/pkg/predicate" sharedreconcile "github.com/istio-ecosystem/sail-operator/pkg/reconcile" @@ -274,29 +275,41 @@ func (r *Reconciler) SetupWithManager(mgr ctrl.Manager) error { Named("istiorevision"). // namespaced resources - Watches(&corev1.ConfigMap{}, ownedResourceHandler, builder.WithPredicates(predicate2.IgnoreUpdateWhenAnnotation())). + Watches(&corev1.ConfigMap{}, ownedResourceHandler, + builder.WithPredicates(fieldignore.RulesFor(fieldignore.DefaultRules, &corev1.ConfigMap{}).NewPredicate(), 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())). + Watches(&appsv1.Deployment{}, ownedResourceHandler, + builder.WithPredicates(fieldignore.RulesFor(fieldignore.DefaultRules, &appsv1.Deployment{}).NewPredicate(), 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, builder.WithPredicates(predicate2.IgnoreUpdateWhenAnnotation())). + Watches(&discoveryv1.EndpointSlice{}, endpointSliceHandler, + builder.WithPredicates( + fieldignore.RulesFor(fieldignore.DefaultRules, &discoveryv1.EndpointSlice{}).NewPredicate(), + predicate2.IgnoreUpdateWhenAnnotation())). Watches(&corev1.Service{}, ownedResourceHandler, - builder.WithPredicates(ignoreStatusChange(), predicate2.IgnoreUpdateWhenAnnotation())). + builder.WithPredicates( + fieldignore.RulesFor(fieldignore.DefaultRules, &corev1.Service{}).NewPredicate(), + 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(), 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, builder.WithPredicates(predicate2.IgnoreUpdateWhenAnnotation())). - Watches(&rbacv1.RoleBinding{}, ownedResourceHandler, builder.WithPredicates(predicate2.IgnoreUpdateWhenAnnotation())). + builder.WithPredicates( + fieldignore.RulesFor(fieldignore.DefaultRules, &networkingv1.NetworkPolicy{}).NewPredicate(), + ignoreStatusChange(), predicate2.IgnoreUpdateWhenAnnotation())). + Watches(&corev1.ServiceAccount{}, ownedResourceHandler, + builder.WithPredicates(fieldignore.RulesFor(fieldignore.DefaultRules, &corev1.ServiceAccount{}).NewPredicate(), predicate2.IgnoreUpdateWhenAnnotation())). + Watches(&rbacv1.Role{}, ownedResourceHandler, + builder.WithPredicates(fieldignore.RulesFor(fieldignore.DefaultRules, &rbacv1.Role{}).NewPredicate(), predicate2.IgnoreUpdateWhenAnnotation())). + Watches(&rbacv1.RoleBinding{}, ownedResourceHandler, + builder.WithPredicates(fieldignore.RulesFor(fieldignore.DefaultRules, &rbacv1.RoleBinding{}).NewPredicate(), predicate2.IgnoreUpdateWhenAnnotation())). Watches(&policyv1.PodDisruptionBudget{}, ownedResourceHandler, - builder.WithPredicates(ignoreStatusChange(), predicate2.IgnoreUpdateWhenAnnotation())). + builder.WithPredicates( + fieldignore.RulesFor(fieldignore.DefaultRules, &policyv1.PodDisruptionBudget{}).NewPredicate(), + ignoreStatusChange(), predicate2.IgnoreUpdateWhenAnnotation())). Watches(&autoscalingv2.HorizontalPodAutoscaler{}, ownedResourceHandler, - builder.WithPredicates(ignoreStatusChange(), predicate2.IgnoreUpdateWhenAnnotation())). + builder.WithPredicates( + fieldignore.RulesFor(fieldignore.DefaultRules, &autoscalingv2.HorizontalPodAutoscaler{}).NewPredicate(), + 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()), builder.WithPredicates(predicate2.IgnoreUpdateWhenAnnotation())). @@ -308,12 +321,20 @@ func (r *Reconciler) SetupWithManager(mgr ctrl.Manager) error { Watches(&v1.IstioRevisionTag{}, revisionTagHandler). // cluster-scoped resources - Watches(&rbacv1.ClusterRole{}, ownedResourceHandler, builder.WithPredicates(predicate2.IgnoreUpdateWhenAnnotation())). - Watches(&rbacv1.ClusterRoleBinding{}, ownedResourceHandler, builder.WithPredicates(predicate2.IgnoreUpdateWhenAnnotation())). + Watches(&rbacv1.ClusterRole{}, ownedResourceHandler, + builder.WithPredicates(fieldignore.RulesFor(fieldignore.DefaultRules, &rbacv1.ClusterRole{}).NewPredicate(), predicate2.IgnoreUpdateWhenAnnotation())). + Watches(&rbacv1.ClusterRoleBinding{}, ownedResourceHandler, + builder.WithPredicates( + fieldignore.RulesFor(fieldignore.DefaultRules, &rbacv1.ClusterRoleBinding{}).NewPredicate(), + predicate2.IgnoreUpdateWhenAnnotation())). Watches(&admissionv1.MutatingWebhookConfiguration{}, ownedResourceHandler, - builder.WithPredicates(webhookConfigPredicate(), predicate2.IgnoreUpdateWhenAnnotation())). + builder.WithPredicates( + fieldignore.RulesFor(fieldignore.DefaultRules, &admissionv1.MutatingWebhookConfiguration{}).NewPredicate(), + predicate2.IgnoreUpdateWhenAnnotation())). Watches(&admissionv1.ValidatingWebhookConfiguration{}, ownedResourceHandler, - builder.WithPredicates(webhookConfigPredicate(), predicate2.IgnoreUpdateWhenAnnotation())). + builder.WithPredicates( + fieldignore.RulesFor(fieldignore.DefaultRules, &admissionv1.ValidatingWebhookConfiguration{}).NewPredicate(), + 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). @@ -725,42 +746,6 @@ func specWasUpdated(oldObject client.Object, newObject client.Object) bool { return oldObject.GetGeneration() != newObject.GetGeneration() } -func webhookConfigPredicate() predicate.Funcs { - return predicate.Funcs{ - UpdateFunc: func(e event.TypedUpdateEvent[client.Object]) bool { - if e.ObjectOld == nil || e.ObjectNew == nil { - return false - } - - // 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) - }, - } -} - -func clearIgnoredFields(obj client.Object) { - obj.SetResourceVersion("") - obj.SetGeneration(0) - obj.SetManagedFields(nil) - 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 - } - } -} - func wrapEventHandler(logger logr.Logger, handler handler.EventHandler) handler.EventHandler { return enqueuelogger.WrapIfNecessary(v1.IstioRevisionKind, logger, handler) } diff --git a/controllers/istiorevisiontag/istiorevisiontag_controller.go b/controllers/istiorevisiontag/istiorevisiontag_controller.go index 1317d4595b..aaad3ad3ca 100644 --- a/controllers/istiorevisiontag/istiorevisiontag_controller.go +++ b/controllers/istiorevisiontag/istiorevisiontag_controller.go @@ -27,6 +27,7 @@ import ( "github.com/istio-ecosystem/sail-operator/pkg/constants" "github.com/istio-ecosystem/sail-operator/pkg/enqueuelogger" "github.com/istio-ecosystem/sail-operator/pkg/errlist" + "github.com/istio-ecosystem/sail-operator/pkg/fieldignore" "github.com/istio-ecosystem/sail-operator/pkg/helm" "github.com/istio-ecosystem/sail-operator/pkg/reconciler" "github.com/istio-ecosystem/sail-operator/pkg/revision" @@ -251,7 +252,8 @@ func (r *Reconciler) SetupWithManager(mgr ctrl.Manager) error { // cluster-scoped resources Watches(&v1.Istio{}, operatorResourcesHandler). Watches(&v1.IstioRevision{}, operatorResourcesHandler). - Watches(&admissionv1.MutatingWebhookConfiguration{}, ownedResourceHandler). + Watches(&admissionv1.MutatingWebhookConfiguration{}, ownedResourceHandler, + builder.WithPredicates(fieldignore.RulesFor(fieldignore.DefaultRules, &admissionv1.MutatingWebhookConfiguration{}).NewPredicate())). Complete(reconciler.NewStandardReconcilerWithFinalizer[*v1.IstioRevisionTag](r.Client, r.Reconcile, r.Finalize, constants.FinalizerName)) } diff --git a/controllers/webhook/webhook_controller.go b/controllers/webhook/webhook_controller.go index d43ebfe736..5103551267 100644 --- a/controllers/webhook/webhook_controller.go +++ b/controllers/webhook/webhook_controller.go @@ -30,6 +30,7 @@ import ( "github.com/istio-ecosystem/sail-operator/pkg/config" "github.com/istio-ecosystem/sail-operator/pkg/constants" "github.com/istio-ecosystem/sail-operator/pkg/enqueuelogger" + "github.com/istio-ecosystem/sail-operator/pkg/fieldignore" "github.com/istio-ecosystem/sail-operator/pkg/reconciler" "github.com/istio-ecosystem/sail-operator/pkg/revision" admissionv1 "k8s.io/api/admissionregistration/v1" @@ -189,7 +190,10 @@ func (r *Reconciler) SetupWithManager(mgr ctrl.Manager) error { // we use the Watches function instead of For(), so that we can wrap the handler so that events that cause the object to be enqueued are logged // +lint-watches:ignore: IstioRevision (not found in charts, but this is the main resource watched by this controller) - Watches(&admissionv1.MutatingWebhookConfiguration{}, objectHandler, builder.WithPredicates(ownedByRemoteIstioRevisionPredicate(mgr.GetClient()))). + Watches(&admissionv1.MutatingWebhookConfiguration{}, objectHandler, + builder.WithPredicates( + fieldignore.RulesFor(fieldignore.DefaultRules, &admissionv1.MutatingWebhookConfiguration{}).NewPredicate(), + ownedByRemoteIstioRevisionPredicate(mgr.GetClient()))). Named("mutatingwebhookconfiguration"). Complete(reconciler.NewStandardReconciler[*admissionv1.MutatingWebhookConfiguration](r.Client, r.Reconcile)) } diff --git a/controllers/ztunnel/ztunnel_controller.go b/controllers/ztunnel/ztunnel_controller.go index 2799b323df..72b43bbb01 100644 --- a/controllers/ztunnel/ztunnel_controller.go +++ b/controllers/ztunnel/ztunnel_controller.go @@ -26,6 +26,7 @@ import ( "github.com/istio-ecosystem/sail-operator/pkg/constants" "github.com/istio-ecosystem/sail-operator/pkg/enqueuelogger" "github.com/istio-ecosystem/sail-operator/pkg/errlist" + "github.com/istio-ecosystem/sail-operator/pkg/fieldignore" "github.com/istio-ecosystem/sail-operator/pkg/helm" "github.com/istio-ecosystem/sail-operator/pkg/predicate" sharedreconcile "github.com/istio-ecosystem/sail-operator/pkg/reconcile" @@ -184,20 +185,22 @@ func (r *Reconciler) SetupWithManager(mgr ctrl.Manager) error { Named("ztunnel"). // namespaced resources - Watches(&corev1.ConfigMap{}, ownedResourceHandler). - Watches(&appsv1.DaemonSet{}, ownedResourceHandler). - Watches(&corev1.ResourceQuota{}, ownedResourceHandler). - - // 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(predicate.IgnoreUpdate())). + Watches(&corev1.ConfigMap{}, ownedResourceHandler, + builder.WithPredicates(fieldignore.RulesFor(fieldignore.DefaultRules, &corev1.ConfigMap{}).NewPredicate())). + Watches(&appsv1.DaemonSet{}, ownedResourceHandler, + builder.WithPredicates(fieldignore.RulesFor(fieldignore.DefaultRules, &appsv1.DaemonSet{}).NewPredicate())). + Watches(&corev1.ResourceQuota{}, ownedResourceHandler, + builder.WithPredicates(fieldignore.RulesFor(fieldignore.DefaultRules, &corev1.ResourceQuota{}).NewPredicate())). + Watches(&corev1.ServiceAccount{}, ownedResourceHandler, + builder.WithPredicates(fieldignore.RulesFor(fieldignore.DefaultRules, &corev1.ServiceAccount{}).NewPredicate(), predicate.IgnoreUpdateWhenAnnotation())). // cluster-scoped resources // +lint-watches:ignore: Namespace (not present in charts, but must be watched to reconcile ZTunnel when its namespace is created) Watches(&corev1.Namespace{}, namespaceHandler). - Watches(&rbacv1.ClusterRole{}, ownedResourceHandler). - Watches(&rbacv1.ClusterRoleBinding{}, ownedResourceHandler). + Watches(&rbacv1.ClusterRole{}, ownedResourceHandler, + builder.WithPredicates(fieldignore.RulesFor(fieldignore.DefaultRules, &rbacv1.ClusterRole{}).NewPredicate())). + Watches(&rbacv1.ClusterRoleBinding{}, ownedResourceHandler, + builder.WithPredicates(fieldignore.RulesFor(fieldignore.DefaultRules, &rbacv1.ClusterRoleBinding{}).NewPredicate())). Watches(&v1.Istio{}, operatorResourcesHandler). Watches(&v1.IstioRevision{}, operatorResourcesHandler). Complete(reconciler.NewStandardReconcilerWithFinalizer[*v1.ZTunnel](r.Client, r.Reconcile, r.Finalize, constants.FinalizerName)) diff --git a/pkg/fieldignore/defaults.go b/pkg/fieldignore/defaults.go new file mode 100644 index 0000000000..44123b62cb --- /dev/null +++ b/pkg/fieldignore/defaults.go @@ -0,0 +1,43 @@ +// Copyright Istio Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package fieldignore + +import ( + admissionv1 "k8s.io/api/admissionregistration/v1" + corev1 "k8s.io/api/core/v1" +) + +// DefaultRules defines the default set of fields to ignore on resources managed +// by the operator. These prevent reconciliation loops caused by other +// controllers (e.g. istiod, pull-secret injectors, Azure admission enforcer) +// updating fields that the operator also manages via Helm. +// +// Use RulesFor to extract typed rules for a specific resource type. +var DefaultRules = []RuleSet{ + TypedRuleSet[*admissionv1.ValidatingWebhookConfiguration]{ + {Fields: []string{"webhooks[*].failurePolicy"}, Scope: IgnoreScopeReconcileAndUpgrade}, + {Fields: []string{"webhooks[*].clientConfig.caBundle"}}, + }, + TypedRuleSet[*admissionv1.MutatingWebhookConfiguration]{ + {Fields: []string{"webhooks[*].clientConfig.caBundle"}}, + // AKS manipulates MutatingWebhookConfigurations. See https://github.com/istio-ecosystem/sail-operator/issues/1148 + {Fields: []string{"webhooks[*].namespaceSelector.matchExpressions[key=kubernetes.azure.com/managedby]"}, Scope: IgnoreScopeReconcile}, + }, + TypedRuleSet[*corev1.ServiceAccount]{ + {Fields: []string{"imagePullSecrets"}}, + {Fields: []string{"automountServiceAccountToken"}}, + {Fields: []string{"secrets"}}, + }, +} diff --git a/pkg/fieldignore/fieldignore.go b/pkg/fieldignore/fieldignore.go new file mode 100644 index 0000000000..c72ac8a56d --- /dev/null +++ b/pkg/fieldignore/fieldignore.go @@ -0,0 +1,359 @@ +// Copyright Istio Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package fieldignore + +import ( + "fmt" + "reflect" + "strings" + + "github.com/istio-ecosystem/sail-operator/pkg/scheme" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/event" + "sigs.k8s.io/controller-runtime/pkg/predicate" +) + +// IgnoreScope controls when a field ignore rule takes effect. +type IgnoreScope string + +const ( + // IgnoreScopeAlways is the zero value: the field is always stripped from + // Helm output (install and upgrade) and always ignored in the predicate. + IgnoreScopeAlways IgnoreScope = "" + + // IgnoreScopeReconcile means the field is ignored by the predicate only. + // Helm renders the field normally on both install and upgrade. + IgnoreScopeReconcile IgnoreScope = "Reconcile" + + // IgnoreScopeReconcileAndUpgrade means the field is ignored by the + // predicate, and stripped from Helm output on upgrades but not on initial + // installs. Use this when Helm should set the initial value but another + // controller manages it afterward. + IgnoreScopeReconcileAndUpgrade IgnoreScope = "ReconcileAndUpgrade" +) + +// FieldIgnoreRule defines a set of fields to ignore for resources of a specific type. +// +// Field paths use dot notation with [*] for array wildcards: +// - "webhooks[*].failurePolicy" → deletes failurePolicy from each webhook +// - "webhooks[*].clientConfig.caBundle" → deletes caBundle nested inside each webhook +// - "spec.template.metadata.annotations" → deletes a deeply nested field +type FieldIgnoreRule[T client.Object] struct { + // Name is an optional exact name match. Empty matches all names. + Name string `json:"name,omitempty"` + + // Fields is the list of field paths to ignore. + Fields []string `json:"fields"` + + // Scope controls when this rule takes effect. See IgnoreScope constants. + Scope IgnoreScope `json:"scope,omitempty"` +} + +// RuleSet is an interface for type-safe rule collections that +// can be stored together in a single slice regardless of their type parameter. +type RuleSet interface { + IntoUntyped() []UntypedFieldIgnoreRule +} + +// RulesFor returns the typed rules from a mixed slice that match type T. +func RulesFor[T client.Object](allRules []RuleSet, _ T) TypedRuleSet[T] { + var result TypedRuleSet[T] + for _, rules := range allRules { + if typed, ok := rules.(TypedRuleSet[T]); ok { + result = append(result, typed...) + } + } + return result +} + +// IntoUntypedAll flattens a mixed slice of typed rule sets into a single untyped slice. +func IntoUntypedAll(allRules []RuleSet) []UntypedFieldIgnoreRule { + var result []UntypedFieldIgnoreRule + for _, rules := range allRules { + result = append(result, rules.IntoUntyped()...) + } + return result +} + +// TypedRuleSet is a type-safe collection of field ignore rules for a specific resource type. +type TypedRuleSet[T client.Object] []FieldIgnoreRule[T] + +// IntoUntyped converts typed rules into untyped rules for use with manifests. +func (rules TypedRuleSet[T]) IntoUntyped() []UntypedFieldIgnoreRule { + gvk := gvkFor[T]() + result := make([]UntypedFieldIgnoreRule, len(rules)) + for i, r := range rules { + result[i] = UntypedFieldIgnoreRule{ + Group: gvk.Group, + Version: gvk.Version, + Kind: gvk.Kind, + Name: r.Name, + Fields: r.Fields, + Scope: r.Scope, + } + } + return result +} + +// NewPredicate returns a predicate that ignores changes to fields specified by +// the given typed rules. On update events it converts both old and new objects to +// unstructured maps, removes the ignored fields (plus standard metadata noise +// like resourceVersion, generation, managedFields), and only triggers +// reconciliation when the remaining content differs. +func (rules TypedRuleSet[T]) NewPredicate() predicate.Funcs { + return predicate.Funcs{ + UpdateFunc: func(e event.UpdateEvent) bool { + if e.ObjectOld == nil || e.ObjectNew == nil { + return false + } + return objectsChangedIgnoringFields(e.ObjectOld, e.ObjectNew, rules) + }, + } +} + +// UntypedFieldIgnoreRule is the untyped version used for manifest matching. +// This is primarily for Helm post-rendering and runtime matching against unstructured manifests. +type UntypedFieldIgnoreRule struct { + Group string `json:"group"` + Version string `json:"version"` + Kind string `json:"kind"` + Name string `json:"name,omitempty"` + Fields []string `json:"fields"` + Scope IgnoreScope `json:"scope,omitempty"` +} + +// MatchesManifest returns true if the untyped rule applies to an unstructured manifest map. +func (r UntypedFieldIgnoreRule) MatchesManifest(manifest map[string]any) bool { + apiVersion, _, _ := unstructured.NestedString(manifest, "apiVersion") + kind, _, _ := unstructured.NestedString(manifest, "kind") + name, _, _ := unstructured.NestedString(manifest, "metadata", "name") + + gv, err := schema.ParseGroupVersion(apiVersion) + if err != nil { + return false + } + if r.Group != gv.Group || r.Version != gv.Version || r.Kind != kind { + return false + } + if r.Name != "" && r.Name != name { + return false + } + return true +} + +// RemoveFieldsFromManifest removes ignored fields from an unstructured manifest map. +// Rules with Scope=Reconcile are never applied (predicate-only). Rules with +// Scope=ReconcileAndUpgrade are only applied when isUpdate is true. +func RemoveFieldsFromManifest(manifest map[string]any, rules []UntypedFieldIgnoreRule, isUpdate bool) { + for _, rule := range rules { + switch rule.Scope { + case IgnoreScopeReconcile: + continue + case IgnoreScopeReconcileAndUpgrade: + if !isUpdate { + continue + } + } + if !rule.MatchesManifest(manifest) { + continue + } + for _, field := range rule.Fields { + removeFieldPath(manifest, field) + } + } +} + +func objectsChangedIgnoringFields[T client.Object](oldObj, newObj client.Object, rules TypedRuleSet[T]) bool { + name := newObj.GetName() + + // Collect fields from all matching rules regardless of Scope. + // Scope only controls post-renderer behavior (whether Helm strips the + // field). In the predicate we always want to ignore these fields to avoid + // unnecessary reconciliation. + var matchingFields []string + for _, rule := range rules { + if rule.Name == "" || rule.Name == name { + matchingFields = append(matchingFields, rule.Fields...) + } + } + if len(matchingFields) == 0 { + return true + } + + oldMap, err := runtime.DefaultUnstructuredConverter.ToUnstructured(oldObj) + if err != nil { + return true + } + newMap, err := runtime.DefaultUnstructuredConverter.ToUnstructured(newObj) + if err != nil { + return true + } + + clearMetadataFields(oldMap) + clearMetadataFields(newMap) + + for _, field := range matchingFields { + removeFieldPath(oldMap, field) + removeFieldPath(newMap, field) + } + + return !reflect.DeepEqual(oldMap, newMap) +} + +// gvkFor derives the GVK for a client.Object type from the global scheme. +func gvkFor[T client.Object]() schema.GroupVersionKind { + var t T + typ := reflect.TypeOf(t) + var instance reflect.Value + if typ.Kind() == reflect.Pointer { + instance = reflect.New(typ.Elem()) + } else { + instance = reflect.New(typ) + } + obj := instance.Interface().(client.Object) + gvks, _, err := scheme.Scheme.ObjectKinds(obj) + if err != nil || len(gvks) == 0 { + panic("no GVK found for type " + typ.String()) + } + return gvks[0] +} + +// clearMetadataFields removes standard metadata fields that change on every +// update and should never trigger reconciliation. +func clearMetadataFields(obj map[string]any) { + if metadata, ok := obj["metadata"].(map[string]any); ok { + delete(metadata, "resourceVersion") + delete(metadata, "generation") + delete(metadata, "managedFields") + } +} + +// removeFieldPath removes a field from a nested map using dot-separated path +// notation. Array wildcards ([*]) cause the operation to be applied to every +// element of the array. A [key=value] predicate matches only array elements +// where the given field equals the given value. +func removeFieldPath(obj map[string]any, path string) { + segments := splitFieldPath(path) + removeFieldSegments(obj, segments) +} + +// splitFieldPath splits a dot-separated field path into segments, but does not +// split on dots inside bracket expressions (e.g. [key=kubernetes.azure.com/managedby]). +func splitFieldPath(path string) []string { + var segments []string + depth := 0 + start := 0 + for i := 0; i < len(path); i++ { + switch path[i] { + case '[': + depth++ + case ']': + depth-- + case '.': + if depth == 0 { + segments = append(segments, path[start:i]) + start = i + 1 + } + } + } + segments = append(segments, path[start:]) + return segments +} + +// parseArrayPredicate parses an array segment like "webhooks[*]" or +// "matchExpressions[key=value]". Returns the array key, predicate field, +// predicate value, and whether a predicate was found at all. +func parseArrayPredicate(seg string) (arrayKey, predField, predValue string, hasPredicate bool) { + openBracket := strings.IndexByte(seg, '[') + if openBracket < 0 || !strings.HasSuffix(seg, "]") { + return seg, "", "", false + } + arrayKey = seg[:openBracket] + inner := seg[openBracket+1 : len(seg)-1] + if inner == "*" { + return arrayKey, "*", "", true + } + if eqIdx := strings.IndexByte(inner, '='); eqIdx >= 0 { + return arrayKey, inner[:eqIdx], inner[eqIdx+1:], true + } + return seg, "", "", false +} + +func removeFieldSegments(obj map[string]any, segments []string) { + if len(segments) == 0 || obj == nil { + return + } + + seg := segments[0] + remaining := segments[1:] + + arrayKey, predField, predValue, hasPredicate := parseArrayPredicate(seg) + if hasPredicate { + if predField == "*" { + if len(remaining) == 0 { + delete(obj, arrayKey) + return + } + arr, ok := obj[arrayKey].([]any) + if !ok { + return + } + for _, item := range arr { + if m, ok := item.(map[string]any); ok { + removeFieldSegments(m, remaining) + } + } + return + } + + arr, ok := obj[arrayKey].([]any) + if !ok { + return + } + if len(remaining) == 0 { + filtered := make([]any, 0, len(arr)) + for _, item := range arr { + m, ok := item.(map[string]any) + if !ok || fmt.Sprintf("%v", m[predField]) != predValue { + filtered = append(filtered, item) + } + } + obj[arrayKey] = filtered + return + } + for _, item := range arr { + if m, ok := item.(map[string]any); ok { + if fmt.Sprintf("%v", m[predField]) == predValue { + removeFieldSegments(m, remaining) + } + } + } + return + } + + if len(remaining) == 0 { + delete(obj, seg) + return + } + + child, ok := obj[seg].(map[string]any) + if !ok { + return + } + removeFieldSegments(child, remaining) +} diff --git a/pkg/fieldignore/fieldignore_test.go b/pkg/fieldignore/fieldignore_test.go new file mode 100644 index 0000000000..0da21276b9 --- /dev/null +++ b/pkg/fieldignore/fieldignore_test.go @@ -0,0 +1,672 @@ +// Copyright Istio Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package fieldignore + +import ( + "testing" + + "github.com/google/go-cmp/cmp" + admissionv1 "k8s.io/api/admissionregistration/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/event" +) + +func TestRemoveFieldPath(t *testing.T) { + tests := []struct { + name string + obj map[string]any + path string + expected map[string]any + }{ + { + name: "simple top-level field", + obj: map[string]any{"foo": "bar", "baz": "qux"}, + path: "foo", + expected: map[string]any{"baz": "qux"}, + }, + { + name: "nested field", + obj: map[string]any{"spec": map[string]any{"replicas": 3, "selector": "app"}}, + path: "spec.replicas", + expected: map[string]any{"spec": map[string]any{"selector": "app"}}, + }, + { + name: "array wildcard", + obj: map[string]any{ + "webhooks": []any{ + map[string]any{"name": "w1", "failurePolicy": "Fail"}, + map[string]any{"name": "w2", "failurePolicy": "Ignore"}, + }, + }, + path: "webhooks[*].failurePolicy", + expected: map[string]any{ + "webhooks": []any{ + map[string]any{"name": "w1"}, + map[string]any{"name": "w2"}, + }, + }, + }, + { + name: "nested array wildcard", + obj: map[string]any{ + "webhooks": []any{ + map[string]any{ + "name": "w1", + "clientConfig": map[string]any{"caBundle": "abc", "url": "https://example.com"}, + }, + }, + }, + path: "webhooks[*].clientConfig.caBundle", + expected: map[string]any{ + "webhooks": []any{ + map[string]any{ + "name": "w1", + "clientConfig": map[string]any{"url": "https://example.com"}, + }, + }, + }, + }, + { + name: "non-existent field is a no-op", + obj: map[string]any{"foo": "bar"}, + path: "nonexistent.field", + expected: map[string]any{"foo": "bar"}, + }, + { + name: "bare array wildcard removes entire array", + obj: map[string]any{ + "webhooks": []any{ + map[string]any{"name": "w1"}, + map[string]any{"name": "w2"}, + }, + "other": "value", + }, + path: "webhooks[*]", + expected: map[string]any{"other": "value"}, + }, + { + name: "non-existent array is a no-op", + obj: map[string]any{"foo": "bar"}, + path: "items[*].field", + expected: map[string]any{"foo": "bar"}, + }, + { + name: "[key=value] removes matching elements", + obj: map[string]any{ + "matchExpressions": []any{ + map[string]any{"key": "azure", "operator": "Exists"}, + map[string]any{"key": "other", "operator": "In"}, + }, + }, + path: "matchExpressions[key=azure]", + expected: map[string]any{ + "matchExpressions": []any{ + map[string]any{"key": "other", "operator": "In"}, + }, + }, + }, + { + name: "[key=value] recurses into matching elements only", + obj: map[string]any{ + "matchExpressions": []any{ + map[string]any{"key": "azure", "operator": "Exists"}, + map[string]any{"key": "other", "operator": "In"}, + }, + }, + path: "matchExpressions[key=azure].operator", + expected: map[string]any{ + "matchExpressions": []any{ + map[string]any{"key": "azure"}, + map[string]any{"key": "other", "operator": "In"}, + }, + }, + }, + { + name: "[key=value] with non-existent key is a no-op", + obj: map[string]any{ + "matchExpressions": []any{ + map[string]any{"key": "azure", "operator": "Exists"}, + }, + }, + path: "matchExpressions[key=nonexistent]", + expected: map[string]any{ + "matchExpressions": []any{ + map[string]any{"key": "azure", "operator": "Exists"}, + }, + }, + }, + { + name: "nested wildcard + [key=value] predicate", + obj: map[string]any{ + "webhooks": []any{ + map[string]any{ + "name": "w1", + "namespaceSelector": map[string]any{ + "matchExpressions": []any{ + map[string]any{"key": "azure", "operator": "Exists"}, + map[string]any{"key": "keep", "operator": "In"}, + }, + }, + }, + map[string]any{ + "name": "w2", + "namespaceSelector": map[string]any{ + "matchExpressions": []any{ + map[string]any{"key": "azure", "operator": "DoesNotExist"}, + }, + }, + }, + }, + }, + path: "webhooks[*].namespaceSelector.matchExpressions[key=azure]", + expected: map[string]any{ + "webhooks": []any{ + map[string]any{ + "name": "w1", + "namespaceSelector": map[string]any{ + "matchExpressions": []any{ + map[string]any{"key": "keep", "operator": "In"}, + }, + }, + }, + map[string]any{ + "name": "w2", + "namespaceSelector": map[string]any{ + "matchExpressions": []any{}, + }, + }, + }, + }, + }, + { + name: "[key=value] with dots in value", + obj: map[string]any{ + "webhooks": []any{ + map[string]any{ + "name": "w1", + "namespaceSelector": map[string]any{ + "matchExpressions": []any{ + map[string]any{"key": "kubernetes.azure.com/managedby", "operator": "In", "values": []any{"aks"}}, + map[string]any{"key": "keep", "operator": "In"}, + }, + }, + }, + }, + }, + path: "webhooks[*].namespaceSelector.matchExpressions[key=kubernetes.azure.com/managedby]", + expected: map[string]any{ + "webhooks": []any{ + map[string]any{ + "name": "w1", + "namespaceSelector": map[string]any{ + "matchExpressions": []any{ + map[string]any{"key": "keep", "operator": "In"}, + }, + }, + }, + }, + }, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + removeFieldPath(tc.obj, tc.path) + if diff := cmp.Diff(tc.expected, tc.obj); diff != "" { + t.Errorf("removeFieldPath mismatch (-want +got):\n%s", diff) + } + }) + } +} + +func TestSplitFieldPath(t *testing.T) { + tests := []struct { + path string + want []string + }{ + {"foo", []string{"foo"}}, + {"foo.bar", []string{"foo", "bar"}}, + {"webhooks[*].failurePolicy", []string{"webhooks[*]", "failurePolicy"}}, + {"spec.someField[key=some.key/andSlash]", []string{"spec", "someField[key=some.key/andSlash]"}}, + } + + for _, tc := range tests { + t.Run(tc.path, func(t *testing.T) { + got := splitFieldPath(tc.path) + if diff := cmp.Diff(tc.want, got); diff != "" { + t.Errorf("splitFieldPath(%q) mismatch (-want +got):\n%s", tc.path, diff) + } + }) + } +} + +func TestParseArrayPredicate(t *testing.T) { + tests := []struct { + seg string + arrayKey string + predField string + predValue string + hasPred bool + }{ + {"webhooks[*]", "webhooks", "*", "", true}, + {"matchExpressions[key=foo]", "matchExpressions", "key", "foo", true}, + {"plain", "plain", "", "", false}, + {"matchExpressions[key=kubernetes.azure.com/managedby]", "matchExpressions", "key", "kubernetes.azure.com/managedby", true}, + } + + for _, tc := range tests { + t.Run(tc.seg, func(t *testing.T) { + arrayKey, predField, predValue, hasPred := parseArrayPredicate(tc.seg) + if arrayKey != tc.arrayKey || predField != tc.predField || predValue != tc.predValue || hasPred != tc.hasPred { + t.Errorf("parseArrayPredicate(%q) = (%q, %q, %q, %v), want (%q, %q, %q, %v)", + tc.seg, arrayKey, predField, predValue, hasPred, + tc.arrayKey, tc.predField, tc.predValue, tc.hasPred) + } + }) + } +} + +func TestMatchesManifest(t *testing.T) { + tests := []struct { + name string + rule UntypedFieldIgnoreRule + manifest map[string]any + want bool + }{ + { + name: "matching manifest with exact name", + rule: UntypedFieldIgnoreRule{ + Group: "admissionregistration.k8s.io", + Version: "v1", + Kind: "ValidatingWebhookConfiguration", + Name: "istiod-istio-system-validator", + Fields: []string{"webhooks[*].failurePolicy"}, + }, + manifest: map[string]any{ + "apiVersion": "admissionregistration.k8s.io/v1", + "kind": "ValidatingWebhookConfiguration", + "metadata": map[string]any{"name": "istiod-istio-system-validator"}, + }, + want: true, + }, + { + name: "matching manifest with no name requirement", + rule: UntypedFieldIgnoreRule{ + Group: "admissionregistration.k8s.io", + Version: "v1", + Kind: "ValidatingWebhookConfiguration", + Fields: []string{"webhooks[*].failurePolicy"}, + }, + manifest: map[string]any{ + "apiVersion": "admissionregistration.k8s.io/v1", + "kind": "ValidatingWebhookConfiguration", + "metadata": map[string]any{"name": "any-webhook"}, + }, + want: true, + }, + { + name: "non-matching kind", + rule: UntypedFieldIgnoreRule{ + Group: "admissionregistration.k8s.io", + Version: "v1", + Kind: "MutatingWebhookConfiguration", + Fields: []string{"webhooks[*].failurePolicy"}, + }, + manifest: map[string]any{ + "apiVersion": "admissionregistration.k8s.io/v1", + "kind": "ValidatingWebhookConfiguration", + "metadata": map[string]any{"name": "istiod-istio-system-validator"}, + }, + want: false, + }, + { + name: "non-matching name", + rule: UntypedFieldIgnoreRule{ + Group: "admissionregistration.k8s.io", + Version: "v1", + Kind: "ValidatingWebhookConfiguration", + Name: "istiod-istio-system-validator", + Fields: []string{"webhooks[*].failurePolicy"}, + }, + manifest: map[string]any{ + "apiVersion": "admissionregistration.k8s.io/v1", + "kind": "ValidatingWebhookConfiguration", + "metadata": map[string]any{"name": "other-webhook"}, + }, + want: false, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + got := tc.rule.MatchesManifest(tc.manifest) + if got != tc.want { + t.Errorf("MatchesManifest() = %v, want %v", got, tc.want) + } + }) + } +} + +func TestRemoveFieldsFromManifest(t *testing.T) { + reconcileAndUpgradeRules := []UntypedFieldIgnoreRule{ + { + Group: "admissionregistration.k8s.io", + Version: "v1", + Kind: "ValidatingWebhookConfiguration", + Fields: []string{"webhooks[*].failurePolicy"}, + Scope: IgnoreScopeReconcileAndUpgrade, + }, + } + reconcileOnlyRules := []UntypedFieldIgnoreRule{ + { + Group: "admissionregistration.k8s.io", + Version: "v1", + Kind: "ValidatingWebhookConfiguration", + Fields: []string{"webhooks[*].failurePolicy"}, + Scope: IgnoreScopeReconcile, + }, + } + + webhookManifest := func() map[string]any { + return map[string]any{ + "apiVersion": "admissionregistration.k8s.io/v1", + "kind": "ValidatingWebhookConfiguration", + "metadata": map[string]any{"name": "test"}, + "webhooks": []any{map[string]any{"name": "w1", "failurePolicy": "Fail"}}, + } + } + + tests := []struct { + name string + rules []UntypedFieldIgnoreRule + manifest map[string]any + isUpdate bool + expected map[string]any + }{ + { + name: "removes field on update", + rules: reconcileAndUpgradeRules, + manifest: webhookManifest(), + isUpdate: true, + expected: map[string]any{ + "apiVersion": "admissionregistration.k8s.io/v1", + "kind": "ValidatingWebhookConfiguration", + "metadata": map[string]any{"name": "test"}, + "webhooks": []any{map[string]any{"name": "w1"}}, + }, + }, + { + name: "preserves field on install when Scope=ReconcileAndUpgrade", + rules: reconcileAndUpgradeRules, + manifest: webhookManifest(), + isUpdate: false, + expected: webhookManifest(), + }, + { + name: "Scope=Reconcile never strips field on install", + rules: reconcileOnlyRules, + manifest: webhookManifest(), + isUpdate: false, + expected: webhookManifest(), + }, + { + name: "Scope=Reconcile never strips field on update", + rules: reconcileOnlyRules, + manifest: webhookManifest(), + isUpdate: true, + expected: webhookManifest(), + }, + { + name: "non-matching manifest is untouched", + rules: reconcileAndUpgradeRules, + manifest: map[string]any{ + "apiVersion": "apps/v1", + "kind": "Deployment", + "metadata": map[string]any{"name": "test"}, + "spec": map[string]any{"replicas": 1}, + }, + isUpdate: true, + expected: map[string]any{ + "apiVersion": "apps/v1", + "kind": "Deployment", + "metadata": map[string]any{"name": "test"}, + "spec": map[string]any{"replicas": 1}, + }, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + RemoveFieldsFromManifest(tc.manifest, tc.rules, tc.isUpdate) + if diff := cmp.Diff(tc.expected, tc.manifest); diff != "" { + t.Errorf("RemoveFieldsFromManifest mismatch (-want +got):\n%s", diff) + } + }) + } +} + +func TestNewPredicateWithEmptyName(t *testing.T) { + rules := TypedRuleSet[*admissionv1.ValidatingWebhookConfiguration]{ + {Fields: []string{"webhooks[*].failurePolicy"}}, + } + pred := rules.NewPredicate() + + failPolicy := admissionv1.Fail + ignorePolicy := admissionv1.Ignore + + tests := []struct { + name string + oldObj *admissionv1.ValidatingWebhookConfiguration + newObj *admissionv1.ValidatingWebhookConfiguration + want bool + }{ + { + name: "ignores failurePolicy change on any webhook name", + oldObj: &admissionv1.ValidatingWebhookConfiguration{ + ObjectMeta: metav1.ObjectMeta{Name: "any-webhook-name"}, + Webhooks: []admissionv1.ValidatingWebhook{ + {Name: "w1", FailurePolicy: &failPolicy}, + }, + }, + newObj: &admissionv1.ValidatingWebhookConfiguration{ + ObjectMeta: metav1.ObjectMeta{Name: "any-webhook-name"}, + Webhooks: []admissionv1.ValidatingWebhook{ + {Name: "w1", FailurePolicy: &ignorePolicy}, + }, + }, + want: false, + }, + { + name: "detects non-ignored change on any webhook name", + oldObj: &admissionv1.ValidatingWebhookConfiguration{ + ObjectMeta: metav1.ObjectMeta{Name: "another-webhook"}, + Webhooks: []admissionv1.ValidatingWebhook{ + {Name: "w1", FailurePolicy: &failPolicy}, + }, + }, + newObj: &admissionv1.ValidatingWebhookConfiguration{ + ObjectMeta: metav1.ObjectMeta{Name: "another-webhook"}, + Webhooks: []admissionv1.ValidatingWebhook{ + {Name: "w1-renamed", FailurePolicy: &failPolicy}, + }, + }, + want: true, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + result := pred.Update(event.UpdateEvent{ObjectOld: tc.oldObj, ObjectNew: tc.newObj}) + if result != tc.want { + t.Errorf("pred.Update() = %v, want %v", result, tc.want) + } + }) + } +} + +func TestNewPredicate(t *testing.T) { + rules := TypedRuleSet[*admissionv1.ValidatingWebhookConfiguration]{ + {Name: "istiod-istio-system-validator", Fields: []string{"webhooks[*].failurePolicy"}}, + } + pred := rules.NewPredicate() + + failPolicy := admissionv1.Fail + ignorePolicy := admissionv1.Ignore + + tests := []struct { + name string + oldObj *admissionv1.ValidatingWebhookConfiguration + newObj *admissionv1.ValidatingWebhookConfiguration + want bool + }{ + { + name: "ignores failurePolicy-only change", + oldObj: &admissionv1.ValidatingWebhookConfiguration{ + ObjectMeta: metav1.ObjectMeta{Name: "istiod-istio-system-validator"}, + Webhooks: []admissionv1.ValidatingWebhook{ + {Name: "w1", FailurePolicy: &failPolicy}, + }, + }, + newObj: &admissionv1.ValidatingWebhookConfiguration{ + ObjectMeta: metav1.ObjectMeta{Name: "istiod-istio-system-validator"}, + Webhooks: []admissionv1.ValidatingWebhook{ + {Name: "w1", FailurePolicy: &ignorePolicy}, + }, + }, + want: false, + }, + { + name: "detects non-ignored field change", + oldObj: &admissionv1.ValidatingWebhookConfiguration{ + ObjectMeta: metav1.ObjectMeta{Name: "istiod-istio-system-validator"}, + Webhooks: []admissionv1.ValidatingWebhook{ + {Name: "w1", FailurePolicy: &failPolicy}, + }, + }, + newObj: &admissionv1.ValidatingWebhookConfiguration{ + ObjectMeta: metav1.ObjectMeta{Name: "istiod-istio-system-validator"}, + Webhooks: []admissionv1.ValidatingWebhook{ + {Name: "w1-renamed", FailurePolicy: &ignorePolicy}, + }, + }, + want: true, + }, + { + name: "non-matching name passes through", + oldObj: &admissionv1.ValidatingWebhookConfiguration{ + ObjectMeta: metav1.ObjectMeta{Name: "some-other-webhook"}, + Webhooks: []admissionv1.ValidatingWebhook{ + {Name: "w1", FailurePolicy: &failPolicy}, + }, + }, + newObj: &admissionv1.ValidatingWebhookConfiguration{ + ObjectMeta: metav1.ObjectMeta{Name: "some-other-webhook"}, + Webhooks: []admissionv1.ValidatingWebhook{ + {Name: "w1", FailurePolicy: &ignorePolicy}, + }, + }, + want: true, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + result := pred.Update(event.UpdateEvent{ObjectOld: tc.oldObj, ObjectNew: tc.newObj}) + if result != tc.want { + t.Errorf("pred.Update() = %v, want %v", result, tc.want) + } + }) + } +} + +func TestNewPredicateMutatingWebhookConfiguration(t *testing.T) { + rules := TypedRuleSet[*admissionv1.MutatingWebhookConfiguration]{ + {Fields: []string{"webhooks[*].clientConfig.caBundle"}}, + } + pred := rules.NewPredicate() + + tests := []struct { + name string + oldObj *admissionv1.MutatingWebhookConfiguration + newObj *admissionv1.MutatingWebhookConfiguration + want bool + }{ + { + name: "ignores caBundle-only change", + oldObj: &admissionv1.MutatingWebhookConfiguration{ + ObjectMeta: metav1.ObjectMeta{Name: "istio-sidecar-injector"}, + Webhooks: []admissionv1.MutatingWebhook{ + {Name: "w1", ClientConfig: admissionv1.WebhookClientConfig{CABundle: []byte("old-ca")}}, + }, + }, + newObj: &admissionv1.MutatingWebhookConfiguration{ + ObjectMeta: metav1.ObjectMeta{Name: "istio-sidecar-injector"}, + Webhooks: []admissionv1.MutatingWebhook{ + {Name: "w1", ClientConfig: admissionv1.WebhookClientConfig{CABundle: []byte("new-ca")}}, + }, + }, + want: false, + }, + { + name: "detects non-ignored field change", + oldObj: &admissionv1.MutatingWebhookConfiguration{ + ObjectMeta: metav1.ObjectMeta{Name: "istio-sidecar-injector"}, + Webhooks: []admissionv1.MutatingWebhook{ + {Name: "w1", ClientConfig: admissionv1.WebhookClientConfig{CABundle: []byte("old-ca")}}, + }, + }, + newObj: &admissionv1.MutatingWebhookConfiguration{ + ObjectMeta: metav1.ObjectMeta{Name: "istio-sidecar-injector"}, + Webhooks: []admissionv1.MutatingWebhook{ + {Name: "w1-renamed", ClientConfig: admissionv1.WebhookClientConfig{CABundle: []byte("new-ca")}}, + }, + }, + want: true, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + result := pred.Update(event.UpdateEvent{ObjectOld: tc.oldObj, ObjectNew: tc.newObj}) + if result != tc.want { + t.Errorf("pred.Update() = %v, want %v", result, tc.want) + } + }) + } +} + +func TestIntoUntyped(t *testing.T) { + rules := TypedRuleSet[*admissionv1.ValidatingWebhookConfiguration]{ + {Name: "istiod-validator", Fields: []string{"webhooks[*].failurePolicy"}, Scope: IgnoreScopeReconcileAndUpgrade}, + } + + untyped := rules.IntoUntyped() + if len(untyped) != 1 { + t.Fatalf("expected 1 untyped rule, got %d", len(untyped)) + } + r := untyped[0] + if r.Group != "admissionregistration.k8s.io" || r.Version != "v1" || r.Kind != "ValidatingWebhookConfiguration" { + t.Errorf("GVK = %s/%s %s, want admissionregistration.k8s.io/v1 ValidatingWebhookConfiguration", r.Group, r.Version, r.Kind) + } + if r.Name != "istiod-validator" { + t.Errorf("Name = %q, want %q", r.Name, "istiod-validator") + } + if len(r.Fields) != 1 || r.Fields[0] != "webhooks[*].failurePolicy" { + t.Errorf("Fields = %v, want [webhooks[*].failurePolicy]", r.Fields) + } + if r.Scope != IgnoreScopeReconcileAndUpgrade { + t.Errorf("Scope = %q, want %q", r.Scope, IgnoreScopeReconcileAndUpgrade) + } +} diff --git a/pkg/helm/chartmanager.go b/pkg/helm/chartmanager.go index ceda3a0ab6..d5bd8efd89 100644 --- a/pkg/helm/chartmanager.go +++ b/pkg/helm/chartmanager.go @@ -15,6 +15,7 @@ package helm import ( + "bytes" "context" "errors" "fmt" @@ -22,6 +23,7 @@ import ( "github.com/go-logr/logr" "github.com/istio-ecosystem/sail-operator/pkg/constants" + "github.com/istio-ecosystem/sail-operator/pkg/fieldignore" "helm.sh/helm/v4/pkg/action" chartv2 "helm.sh/helm/v4/pkg/chart/v2" "helm.sh/helm/v4/pkg/kube" @@ -39,6 +41,7 @@ type ChartManager struct { restClientGetter genericclioptions.RESTClientGetter driver string managedByValue string + fieldIgnoreRules []fieldignore.UntypedFieldIgnoreRule } // ChartManagerOption is a functional option for configuring a ChartManager. @@ -53,6 +56,15 @@ func WithManagedByValue(v string) ChartManagerOption { } } +// WithFieldIgnoreRules configures the field ignore rules that the post-renderer +// uses to strip fields from rendered manifests. +// See fieldignore.IgnoreScope for how the Scope field controls stripping behavior. +func WithFieldIgnoreRules(rules []fieldignore.UntypedFieldIgnoreRule) ChartManagerOption { + return func(cm *ChartManager) { + cm.fieldIgnoreRules = rules + } +} + // NewChartManager creates a new Helm chart manager using cfg as the configuration // that Helm will use to connect to the cluster when installing or uninstalling // charts, and using the specified driver to store information about releases @@ -163,8 +175,17 @@ func (h *ChartManager) upgradeOrInstallChart( if releaseExists { log.V(2).Info("Performing helm upgrade", "chartName", chart.Name()) + // Strip ReconcileAndUpgrade-scoped fields from the stored release + // manifest before upgrade. Without this, Helm's three-way merge sees + // these fields in the old manifest but not in the new (post-rendered) + // manifest, interprets the absence as a deletion, and overwrites + // in-cluster values set by other controllers (e.g. istiod). + if err := h.stripUpgradeOnlyFieldsFromRelease(cfg, relV1); err != nil { + return nil, fmt.Errorf("failed to strip fields from stored release %s: %w", releaseName, err) + } + updateAction := action.NewUpgrade(cfg) - updateAction.PostRenderer = NewHelmPostRenderer(ownerReference, "", true, h.managedByValue) + updateAction.PostRenderer = NewHelmPostRenderer(ownerReference, "", true, h.managedByValue, h.fieldIgnoreRules) updateAction.MaxHistory = 1 updateAction.SkipCRDs = true updateAction.DisableOpenAPIValidation = true @@ -179,7 +200,7 @@ func (h *ChartManager) upgradeOrInstallChart( log.V(2).Info("Performing helm install", "chartName", chart.Name()) installAction := action.NewInstall(cfg) - installAction.PostRenderer = NewHelmPostRenderer(ownerReference, "", false, h.managedByValue) + installAction.PostRenderer = NewHelmPostRenderer(ownerReference, "", false, h.managedByValue, h.fieldIgnoreRules) installAction.Namespace = namespace installAction.ReleaseName = releaseName installAction.SkipCRDs = true @@ -250,3 +271,22 @@ func (h *ChartManager) ListReleases(ctx context.Context) ([]release.Releaser, er listAction.AllNamespaces = true return listAction.Run() } + +// stripUpgradeOnlyFieldsFromRelease removes ReconcileAndUpgrade-scoped fields +// from the stored release manifest so that Helm's three-way merge does not +// treat their absence in the post-rendered new manifest as a deletion. +func (h *ChartManager) stripUpgradeOnlyFieldsFromRelease(cfg *action.Configuration, rel *releasev1.Release) error { + if len(h.fieldIgnoreRules) == 0 { + return nil + } + pr := NewHelmPostRenderer(nil, "", true, h.managedByValue, h.fieldIgnoreRules) + modified, err := pr.Run(bytes.NewBufferString(rel.Manifest)) + if err != nil { + return err + } + if modified.String() == rel.Manifest { + return nil + } + rel.Manifest = modified.String() + return cfg.Releases.Update(rel) +} diff --git a/pkg/helm/postrenderer.go b/pkg/helm/postrenderer.go index 067a3583bf..0f93970d5f 100644 --- a/pkg/helm/postrenderer.go +++ b/pkg/helm/postrenderer.go @@ -16,14 +16,13 @@ package helm import ( "bytes" - "fmt" "io" "strings" "github.com/istio-ecosystem/sail-operator/pkg/constants" + "github.com/istio-ecosystem/sail-operator/pkg/fieldignore" "gopkg.in/yaml.v3" "helm.sh/helm/v4/pkg/postrenderer" - admissionregistrationv1 "k8s.io/api/admissionregistration/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" @@ -37,22 +36,26 @@ const ( // NewHelmPostRenderer creates a Helm PostRenderer that adds the following to each rendered manifest: // - adds the "managed-by" label with the given managedByValue // - adds the specified OwnerReference -// It also removes the failurePolicy field from ValidatingWebhookConfigurations on updates, so -// the in-cluster setting stays as-is, to prevent clashing with the istiod validation controller. -func NewHelmPostRenderer(ownerReference *metav1.OwnerReference, ownerNamespace string, isUpdate bool, managedByValue string) postrenderer.PostRenderer { +// It also removes fields from rendered manifests according to the provided FieldIgnoreRules. +// See fieldignore.IgnoreScope for how the Scope field controls install vs. upgrade behavior. +func NewHelmPostRenderer(ownerReference *metav1.OwnerReference, ownerNamespace string, isUpdate bool, + managedByValue string, fieldIgnoreRules []fieldignore.UntypedFieldIgnoreRule, +) postrenderer.PostRenderer { return HelmPostRenderer{ - ownerReference: ownerReference, - ownerNamespace: ownerNamespace, - isUpdate: isUpdate, - managedByValue: managedByValue, + ownerReference: ownerReference, + ownerNamespace: ownerNamespace, + isUpdate: isUpdate, + managedByValue: managedByValue, + fieldIgnoreRules: fieldIgnoreRules, } } type HelmPostRenderer struct { - ownerReference *metav1.OwnerReference - ownerNamespace string - isUpdate bool - managedByValue string + ownerReference *metav1.OwnerReference + ownerNamespace string + isUpdate bool + managedByValue string + fieldIgnoreRules []fieldignore.UntypedFieldIgnoreRule } var _ postrenderer.PostRenderer = HelmPostRenderer{} @@ -86,15 +89,7 @@ func (pr HelmPostRenderer) Run(renderedManifests *bytes.Buffer) (modifiedManifes return nil, err } - // Strip ValidatingWebhookConfiguration webhooks[].failurePolicy field if we're upgrading, - // to avoid overwriting the value set in-cluster by the istiod validation controller. On - // initial install we still want to set the field per the Helm template. - if pr.isUpdate { - manifest, err = pr.removeValidatingWebhookFailurePolicy(manifest) - if err != nil { - return nil, fmt.Errorf("error removing ValidatingWebhookConfiguration failurePolicy: %v", err) - } - } + fieldignore.RemoveFieldsFromManifest(manifest, pr.fieldIgnoreRules, pr.isUpdate) if err := encoder.Encode(manifest); err != nil { return nil, err @@ -103,41 +98,6 @@ func (pr HelmPostRenderer) Run(renderedManifests *bytes.Buffer) (modifiedManifes return modifiedManifests, nil } -func (pr HelmPostRenderer) removeValidatingWebhookFailurePolicy(manifest map[string]any) (map[string]any, error) { - apiVersion, _, _ := unstructured.NestedString(manifest, "apiVersion") - if apiVersion != admissionregistrationv1.SchemeGroupVersion.String() { - return manifest, nil - } - kind, _, _ := unstructured.NestedString(manifest, "kind") - if kind != "ValidatingWebhookConfiguration" { - return manifest, nil - } - - webhooksAny, found, err := unstructured.NestedFieldNoCopy(manifest, "webhooks") - if err != nil { - return nil, err - } - if !found { - return manifest, nil - } - - webhooks, ok := webhooksAny.([]interface{}) - if !ok { - return nil, fmt.Errorf("expected webhooks to be []interface{}, got %T", webhooksAny) - } - - for _, webhookAny := range webhooks { - webhook, ok := webhookAny.(map[string]any) - if !ok { - return nil, fmt.Errorf("expected webhook to be map[string]interface{}, got %T", webhookAny) - } - - delete(webhook, "failurePolicy") - } - - return manifest, nil -} - func (pr HelmPostRenderer) addOwnerReference(manifest map[string]any) (map[string]any, error) { if pr.ownerReference == nil { return manifest, nil diff --git a/pkg/helm/postrenderer_test.go b/pkg/helm/postrenderer_test.go index 179a1dd0ad..1d1afdd0ed 100644 --- a/pkg/helm/postrenderer_test.go +++ b/pkg/helm/postrenderer_test.go @@ -20,17 +20,60 @@ import ( "github.com/google/go-cmp/cmp" "github.com/istio-ecosystem/sail-operator/pkg/constants" + "github.com/istio-ecosystem/sail-operator/pkg/fieldignore" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) +// defaultTestRules replicates the built-in field ignore rules used in production. +var defaultTestRules = []fieldignore.UntypedFieldIgnoreRule{ + { + Group: "admissionregistration.k8s.io", + Version: "v1", + Kind: "ValidatingWebhookConfiguration", + Fields: []string{"webhooks[*].failurePolicy"}, + Scope: fieldignore.IgnoreScopeReconcileAndUpgrade, + }, + { + Group: "admissionregistration.k8s.io", + Version: "v1", + Kind: "ValidatingWebhookConfiguration", + Fields: []string{"webhooks[*].clientConfig.caBundle"}, + }, + { + Group: "admissionregistration.k8s.io", + Version: "v1", + Kind: "MutatingWebhookConfiguration", + Fields: []string{"webhooks[*].clientConfig.caBundle"}, + }, + { + Group: "", + Version: "v1", + Kind: "ServiceAccount", + Fields: []string{"imagePullSecrets"}, + }, + { + Group: "", + Version: "v1", + Kind: "ServiceAccount", + Fields: []string{"automountServiceAccountToken"}, + }, + { + Group: "", + Version: "v1", + Kind: "ServiceAccount", + Fields: []string{"secrets"}, + }, +} + func TestHelmPostRenderer(t *testing.T) { testCases := []struct { - name string - ownerReference *metav1.OwnerReference - ownerNamespace string - isUpdate bool - input string - expected string + name string + ownerReference *metav1.OwnerReference + ownerNamespace string + isUpdate bool + fieldIgnoreRules []fieldignore.UntypedFieldIgnoreRule + input string + expected string }{ { name: "cluster-scoped owner", @@ -222,9 +265,10 @@ spec: `, }, { - name: "ValidatingWebhookConfiguration create", - ownerReference: nil, - ownerNamespace: "", + name: "ValidatingWebhookConfiguration create", + ownerReference: nil, + ownerNamespace: "", + fieldIgnoreRules: defaultTestRules, input: `apiVersion: admissionregistration.k8s.io/v1 kind: ValidatingWebhookConfiguration metadata: @@ -245,10 +289,11 @@ webhooks: `, }, { - name: "ValidatingWebhookConfiguration update", - ownerReference: nil, - ownerNamespace: "", - isUpdate: true, + name: "ValidatingWebhookConfiguration update", + ownerReference: nil, + ownerNamespace: "", + isUpdate: true, + fieldIgnoreRules: defaultTestRules, input: `apiVersion: admissionregistration.k8s.io/v1 kind: ValidatingWebhookConfiguration metadata: @@ -265,6 +310,118 @@ metadata: name: istio-validator webhooks: - name: rev.validation.istio.io +`, + }, + { + name: "ValidatingWebhookConfiguration strips caBundle on install", + ownerReference: nil, + ownerNamespace: "", + fieldIgnoreRules: defaultTestRules, + input: `apiVersion: admissionregistration.k8s.io/v1 +kind: ValidatingWebhookConfiguration +metadata: + name: istio-validator +webhooks: +- name: rev.validation.istio.io + failurePolicy: Fail + clientConfig: + caBundle: c29tZS1jYS1idW5kbGU= + service: + name: istiod +`, + expected: `apiVersion: admissionregistration.k8s.io/v1 +kind: ValidatingWebhookConfiguration +metadata: + labels: + managed-by: sail-operator + name: istio-validator +webhooks: + - clientConfig: + service: + name: istiod + failurePolicy: Fail + name: rev.validation.istio.io +`, + }, + { + name: "MutatingWebhookConfiguration strips caBundle on update", + ownerReference: nil, + ownerNamespace: "", + isUpdate: true, + fieldIgnoreRules: defaultTestRules, + input: `apiVersion: admissionregistration.k8s.io/v1 +kind: MutatingWebhookConfiguration +metadata: + name: istio-sidecar-injector +webhooks: +- name: rev.namespace.sidecar-injector.istio.io + clientConfig: + caBundle: c29tZS1jYS1idW5kbGU= + service: + name: istiod +`, + expected: `apiVersion: admissionregistration.k8s.io/v1 +kind: MutatingWebhookConfiguration +metadata: + labels: + managed-by: sail-operator + name: istio-sidecar-injector +webhooks: + - clientConfig: + service: + name: istiod + name: rev.namespace.sidecar-injector.istio.io +`, + }, + { + name: "ServiceAccount strips imagePullSecrets on install", + ownerReference: nil, + ownerNamespace: "", + fieldIgnoreRules: defaultTestRules, + input: `apiVersion: v1 +kind: ServiceAccount +metadata: + name: istiod + namespace: istio-system +automountServiceAccountToken: false +imagePullSecrets: +- name: my-pull-secret +secrets: +- name: istiod-token-abc +`, + expected: `apiVersion: v1 +kind: ServiceAccount +metadata: + labels: + managed-by: sail-operator + name: istiod + namespace: istio-system +`, + }, + { + name: "ServiceAccount strips imagePullSecrets on update", + ownerReference: nil, + ownerNamespace: "", + isUpdate: true, + fieldIgnoreRules: defaultTestRules, + input: `apiVersion: v1 +kind: ServiceAccount +metadata: + name: istiod + namespace: istio-system +automountServiceAccountToken: false +imagePullSecrets: +- name: my-pull-secret +secrets: +- name: istiod-token-abc +`, + expected: `apiVersion: v1 +kind: ServiceAccount +metadata: + labels: + managed-by: sail-operator + name: istiod + namespace: istio-system `, }, { @@ -306,10 +463,11 @@ spec: for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { postRenderer := HelmPostRenderer{ - ownerReference: tc.ownerReference, - ownerNamespace: tc.ownerNamespace, - isUpdate: tc.isUpdate, - managedByValue: constants.ManagedByLabelValue, + ownerReference: tc.ownerReference, + ownerNamespace: tc.ownerNamespace, + isUpdate: tc.isUpdate, + managedByValue: constants.ManagedByLabelValue, + fieldIgnoreRules: tc.fieldIgnoreRules, } actual, err := postRenderer.Run(bytes.NewBufferString(tc.input)) diff --git a/tests/integration/api/fieldignore_test.go b/tests/integration/api/fieldignore_test.go new file mode 100644 index 0000000000..744dea40d5 --- /dev/null +++ b/tests/integration/api/fieldignore_test.go @@ -0,0 +1,404 @@ +//go:build integration + +// Copyright Istio Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package integration + +import ( + "context" + "fmt" + "time" + + v1 "github.com/istio-ecosystem/sail-operator/api/v1" + "github.com/istio-ecosystem/sail-operator/pkg/istioversion" + . "github.com/istio-ecosystem/sail-operator/pkg/test/util/ginkgo" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + admissionv1 "k8s.io/api/admissionregistration/v1" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/client" + + "istio.io/istio/pkg/ptr" +) + +var _ = Describe("FieldIgnore rules", Label("fieldignore"), Ordered, func() { + const ( + pilotImage = "sail-operator/test:latest" + ) + + SetDefaultEventuallyPollingInterval(time.Second) + SetDefaultEventuallyTimeout(30 * time.Second) + + ctx := context.Background() + + istioNamespace := "fieldignore-test" + revName := "fieldignore-rev" + mutatingWebhookName := "istio-sidecar-injector-" + revName + "-" + istioNamespace + validatingWebhookName := fmt.Sprintf("istio-validator-%s-%s", revName, istioNamespace) + saName := "istiod-" + revName + + rev := &v1.IstioRevision{} + + BeforeAll(func() { + Step("Creating namespace") + ns := &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: istioNamespace}} + Expect(k8sClient.Create(ctx, ns)).To(Succeed()) + + Step("Creating IstioRevision") + rev = &v1.IstioRevision{ + ObjectMeta: metav1.ObjectMeta{ + Name: revName, + }, + Spec: v1.IstioRevisionSpec{ + Version: istioversion.Default, + Namespace: istioNamespace, + Values: &v1.Values{ + Global: &v1.GlobalConfig{ + IstioNamespace: ptr.Of(istioNamespace), + }, + Revision: ptr.Of(revName), + Pilot: &v1.PilotConfig{ + Image: ptr.Of(pilotImage), + }, + }, + }, + } + Expect(k8sClient.Create(ctx, rev)).To(Succeed()) + + Step("Waiting for webhook configurations and ServiceAccount to be created") + Eventually(k8sClient.Get).WithArguments(ctx, + client.ObjectKey{Name: mutatingWebhookName}, + &admissionv1.MutatingWebhookConfiguration{}).Should(Succeed()) + Eventually(k8sClient.Get).WithArguments(ctx, + client.ObjectKey{Name: validatingWebhookName}, + &admissionv1.ValidatingWebhookConfiguration{}).Should(Succeed()) + Eventually(k8sClient.Get).WithArguments(ctx, + client.ObjectKey{Name: saName, Namespace: istioNamespace}, + &corev1.ServiceAccount{}).Should(Succeed()) + }) + + AfterAll(func() { + Step("Deleting IstioRevision") + deleteAllIstioRevisions(ctx) + + Step("Deleting namespace") + ns := &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: istioNamespace}} + Expect(k8sClient.Delete(ctx, ns)).To(Succeed()) + }) + + Describe("Helm post-rendering on initial install", func() { + It("sets failurePolicy on ValidatingWebhookConfiguration (Scope=ReconcileAndUpgrade)", func() { + webhook := &admissionv1.ValidatingWebhookConfiguration{} + Expect(k8sClient.Get(ctx, client.ObjectKey{Name: validatingWebhookName}, webhook)).To(Succeed()) + Expect(webhook.Webhooks).ToNot(BeEmpty()) + Expect(webhook.Webhooks[0].FailurePolicy).ToNot(BeNil(), + "failurePolicy should be set on initial install because ReconcileAndUpgrade rules are not applied on install") + }) + + It("strips caBundle from ValidatingWebhookConfiguration (Scope=Always)", func() { + webhook := &admissionv1.ValidatingWebhookConfiguration{} + Expect(k8sClient.Get(ctx, client.ObjectKey{Name: validatingWebhookName}, webhook)).To(Succeed()) + Expect(webhook.Webhooks).ToNot(BeEmpty()) + Expect(webhook.Webhooks[0].ClientConfig.CABundle).To(BeEmpty(), + "caBundle should be stripped on initial install because the rule has Scope=Always") + }) + + It("strips caBundle from MutatingWebhookConfiguration (Scope=Always)", func() { + webhook := &admissionv1.MutatingWebhookConfiguration{} + Expect(k8sClient.Get(ctx, client.ObjectKey{Name: mutatingWebhookName}, webhook)).To(Succeed()) + Expect(webhook.Webhooks).ToNot(BeEmpty()) + Expect(webhook.Webhooks[0].ClientConfig.CABundle).To(BeEmpty(), + "caBundle should be stripped on initial install because the rule has Scope=Always") + }) + }) + + // ── Predicate: reconciliation skipping ─────────────────────────────── + + Describe("predicate filtering", func() { + It("skips reconciliation when only caBundle changes on MutatingWebhookConfiguration", func() { + waitForInFlightReconcileToFinish() + + webhook := &admissionv1.MutatingWebhookConfiguration{} + Expect(k8sClient.Get(ctx, client.ObjectKey{Name: mutatingWebhookName}, webhook)).To(Succeed()) + + expectNoReconciliation(istioRevisionController, func() { + for i := range webhook.Webhooks { + webhook.Webhooks[i].ClientConfig.CABundle = []byte("injected-ca-bundle") + } + Expect(k8sClient.Update(ctx, webhook)).To(Succeed()) + }) + }) + + It("skips reconciliation when only caBundle changes on ValidatingWebhookConfiguration", func() { + waitForInFlightReconcileToFinish() + + webhook := &admissionv1.ValidatingWebhookConfiguration{} + Expect(k8sClient.Get(ctx, client.ObjectKey{Name: validatingWebhookName}, webhook)).To(Succeed()) + + expectNoReconciliation(istioRevisionController, func() { + for i := range webhook.Webhooks { + webhook.Webhooks[i].ClientConfig.CABundle = []byte("injected-ca-bundle") + } + Expect(k8sClient.Update(ctx, webhook)).To(Succeed()) + }) + }) + + It("skips reconciliation when only failurePolicy changes on ValidatingWebhookConfiguration", func() { + waitForInFlightReconcileToFinish() + + webhook := &admissionv1.ValidatingWebhookConfiguration{} + Expect(k8sClient.Get(ctx, client.ObjectKey{Name: validatingWebhookName}, webhook)).To(Succeed()) + + expectNoReconciliation(istioRevisionController, func() { + for i := range webhook.Webhooks { + webhook.Webhooks[i].FailurePolicy = ptr.Of(admissionv1.Ignore) + } + Expect(k8sClient.Update(ctx, webhook)).To(Succeed()) + }) + }) + + It("skips reconciliation when both caBundle and failurePolicy change on ValidatingWebhookConfiguration", func() { + waitForInFlightReconcileToFinish() + + webhook := &admissionv1.ValidatingWebhookConfiguration{} + Expect(k8sClient.Get(ctx, client.ObjectKey{Name: validatingWebhookName}, webhook)).To(Succeed()) + + expectNoReconciliation(istioRevisionController, func() { + for i := range webhook.Webhooks { + webhook.Webhooks[i].ClientConfig.CABundle = []byte("another-ca-bundle") + webhook.Webhooks[i].FailurePolicy = ptr.Of(admissionv1.Fail) + } + Expect(k8sClient.Update(ctx, webhook)).To(Succeed()) + }) + }) + + It("skips reconciliation when Azure matchExpression is added to MutatingWebhookConfiguration", func() { + waitForInFlightReconcileToFinish() + + webhook := &admissionv1.MutatingWebhookConfiguration{} + Expect(k8sClient.Get(ctx, client.ObjectKey{Name: mutatingWebhookName}, webhook)).To(Succeed()) + + expectNoReconciliation(istioRevisionController, func() { + for i := range webhook.Webhooks { + if webhook.Webhooks[i].NamespaceSelector == nil { + webhook.Webhooks[i].NamespaceSelector = &metav1.LabelSelector{} + } + webhook.Webhooks[i].NamespaceSelector.MatchExpressions = append( + webhook.Webhooks[i].NamespaceSelector.MatchExpressions, + metav1.LabelSelectorRequirement{ + Key: "kubernetes.azure.com/managedby", + Operator: metav1.LabelSelectorOpNotIn, + Values: []string{"aks"}, + }, + ) + } + Expect(k8sClient.Update(ctx, webhook)).To(Succeed()) + }) + }) + + It("skips reconciliation when imagePullSecrets changes on ServiceAccount", func() { + waitForInFlightReconcileToFinish() + + sa := &corev1.ServiceAccount{} + Expect(k8sClient.Get(ctx, client.ObjectKey{Name: saName, Namespace: istioNamespace}, sa)).To(Succeed()) + + expectNoReconciliation(istioRevisionController, func() { + sa.ImagePullSecrets = append(sa.ImagePullSecrets, corev1.LocalObjectReference{Name: "injected-pull-secret"}) + Expect(k8sClient.Update(ctx, sa)).To(Succeed()) + }) + }) + + It("skips reconciliation when automountServiceAccountToken changes on ServiceAccount", func() { + waitForInFlightReconcileToFinish() + + sa := &corev1.ServiceAccount{} + Expect(k8sClient.Get(ctx, client.ObjectKey{Name: saName, Namespace: istioNamespace}, sa)).To(Succeed()) + + expectNoReconciliation(istioRevisionController, func() { + sa.AutomountServiceAccountToken = ptr.Of(false) + Expect(k8sClient.Update(ctx, sa)).To(Succeed()) + }) + }) + + It("skips reconciliation when secrets changes on ServiceAccount", func() { + waitForInFlightReconcileToFinish() + + sa := &corev1.ServiceAccount{} + Expect(k8sClient.Get(ctx, client.ObjectKey{Name: saName, Namespace: istioNamespace}, sa)).To(Succeed()) + + expectNoReconciliation(istioRevisionController, func() { + sa.Secrets = append(sa.Secrets, corev1.ObjectReference{Name: "auto-token-xyz"}) + Expect(k8sClient.Update(ctx, sa)).To(Succeed()) + }) + }) + + It("triggers reconciliation when a non-ignored field changes on MutatingWebhookConfiguration", func() { + waitForInFlightReconcileToFinish() + + webhook := &admissionv1.MutatingWebhookConfiguration{} + Expect(k8sClient.Get(ctx, client.ObjectKey{Name: mutatingWebhookName}, webhook)).To(Succeed()) + originalName := webhook.Webhooks[0].Name + + webhook.Webhooks[0].Name = "tampered.webhook.test" + Expect(k8sClient.Update(ctx, webhook)).To(Succeed()) + + Eventually(func(g Gomega) { + g.Expect(k8sClient.Get(ctx, client.ObjectKey{Name: mutatingWebhookName}, webhook)).To(Succeed()) + g.Expect(webhook.Webhooks[0].Name).To(Equal(originalName)) + }).Should(Succeed(), "non-ignored field should be reverted by reconciliation") + }) + }) + + Describe("Helm post-rendering on upgrade", func() { + It("preserves in-cluster failurePolicy after reconciliation", func() { + Step("Setting failurePolicy to a custom value in-cluster") + webhook := &admissionv1.ValidatingWebhookConfiguration{} + Expect(k8sClient.Get(ctx, client.ObjectKey{Name: validatingWebhookName}, webhook)).To(Succeed()) + for i := range webhook.Webhooks { + webhook.Webhooks[i].FailurePolicy = ptr.Of(admissionv1.Ignore) + } + Expect(k8sClient.Update(ctx, webhook)).To(Succeed()) + + Step("Triggering reconciliation by modifying a non-ignored field") + Expect(k8sClient.Get(ctx, client.ObjectKey{Name: validatingWebhookName}, webhook)).To(Succeed()) + webhook.Labels["app"] = "tampered" + Expect(k8sClient.Update(ctx, webhook)).To(Succeed()) + + Step("Verifying label is reverted but failurePolicy is preserved") + Eventually(func(g Gomega) { + g.Expect(k8sClient.Get(ctx, client.ObjectKey{Name: validatingWebhookName}, webhook)).To(Succeed()) + g.Expect(webhook.Labels["app"]).To(Equal("istiod"), + "non-ignored field should be reverted") + g.Expect(webhook.Webhooks[0].FailurePolicy).To(HaveValue(Equal(admissionv1.Ignore)), + "failurePolicy should be preserved because Scope=ReconcileAndUpgrade strips it from Helm output on upgrade") + }).Should(Succeed()) + }) + + It("preserves in-cluster caBundle on MutatingWebhookConfiguration after reconciliation", func() { + Step("Setting caBundle in-cluster") + webhook := &admissionv1.MutatingWebhookConfiguration{} + Expect(k8sClient.Get(ctx, client.ObjectKey{Name: mutatingWebhookName}, webhook)).To(Succeed()) + for i := range webhook.Webhooks { + webhook.Webhooks[i].ClientConfig.CABundle = []byte("istiod-injected-ca") + } + Expect(k8sClient.Update(ctx, webhook)).To(Succeed()) + + Step("Triggering reconciliation by modifying a non-ignored field") + Expect(k8sClient.Get(ctx, client.ObjectKey{Name: mutatingWebhookName}, webhook)).To(Succeed()) + webhook.Labels["app"] = "tampered" + Expect(k8sClient.Update(ctx, webhook)).To(Succeed()) + + Step("Verifying label is reverted but caBundle is preserved") + Eventually(func(g Gomega) { + g.Expect(k8sClient.Get(ctx, client.ObjectKey{Name: mutatingWebhookName}, webhook)).To(Succeed()) + g.Expect(webhook.Labels["app"]).To(Equal("sidecar-injector"), + "non-ignored field should be reverted") + g.Expect(webhook.Webhooks[0].ClientConfig.CABundle).To(Equal([]byte("istiod-injected-ca")), + "caBundle should be preserved because the rule strips it from Helm output") + }).Should(Succeed()) + }) + + It("preserves in-cluster caBundle on ValidatingWebhookConfiguration after reconciliation", func() { + Step("Setting caBundle in-cluster") + webhook := &admissionv1.ValidatingWebhookConfiguration{} + Expect(k8sClient.Get(ctx, client.ObjectKey{Name: validatingWebhookName}, webhook)).To(Succeed()) + for i := range webhook.Webhooks { + webhook.Webhooks[i].ClientConfig.CABundle = []byte("istiod-injected-ca") + } + Expect(k8sClient.Update(ctx, webhook)).To(Succeed()) + + Step("Triggering reconciliation by modifying a non-ignored field") + Expect(k8sClient.Get(ctx, client.ObjectKey{Name: validatingWebhookName}, webhook)).To(Succeed()) + webhook.Labels["app"] = "tampered" + Expect(k8sClient.Update(ctx, webhook)).To(Succeed()) + + Step("Verifying label is reverted but caBundle is preserved") + Eventually(func(g Gomega) { + g.Expect(k8sClient.Get(ctx, client.ObjectKey{Name: validatingWebhookName}, webhook)).To(Succeed()) + g.Expect(webhook.Labels["app"]).To(Equal("istiod"), + "non-ignored field should be reverted") + g.Expect(webhook.Webhooks[0].ClientConfig.CABundle).To(Equal([]byte("istiod-injected-ca")), + "caBundle should be preserved because the rule strips it from Helm output") + }).Should(Succeed()) + }) + + It("preserves in-cluster imagePullSecrets on ServiceAccount after reconciliation", func() { + Step("Setting imagePullSecrets in-cluster") + sa := &corev1.ServiceAccount{} + Expect(k8sClient.Get(ctx, client.ObjectKey{Name: saName, Namespace: istioNamespace}, sa)).To(Succeed()) + sa.ImagePullSecrets = append(sa.ImagePullSecrets, corev1.LocalObjectReference{Name: "preserved-pull-secret"}) + Expect(k8sClient.Update(ctx, sa)).To(Succeed()) + + Step("Triggering reconciliation by modifying a non-ignored field (label)") + Expect(k8sClient.Get(ctx, client.ObjectKey{Name: saName, Namespace: istioNamespace}, sa)).To(Succeed()) + sa.Labels["app"] = "tampered" + Expect(k8sClient.Update(ctx, sa)).To(Succeed()) + + Step("Verifying label is reverted but imagePullSecrets is preserved") + Eventually(func(g Gomega) { + g.Expect(k8sClient.Get(ctx, client.ObjectKey{Name: saName, Namespace: istioNamespace}, sa)).To(Succeed()) + g.Expect(sa.Labels["app"]).To(Equal("istiod"), + "non-ignored label should be reverted by reconciliation") + g.Expect(sa.ImagePullSecrets).To(ContainElement(corev1.LocalObjectReference{Name: "preserved-pull-secret"}), + "imagePullSecrets should be preserved because the rule strips it from Helm output") + }).Should(Succeed()) + }) + + It("preserves in-cluster automountServiceAccountToken on ServiceAccount after reconciliation", func() { + Step("Setting automountServiceAccountToken in-cluster") + sa := &corev1.ServiceAccount{} + Expect(k8sClient.Get(ctx, client.ObjectKey{Name: saName, Namespace: istioNamespace}, sa)).To(Succeed()) + sa.AutomountServiceAccountToken = ptr.Of(false) + Expect(k8sClient.Update(ctx, sa)).To(Succeed()) + + Step("Triggering reconciliation by modifying a non-ignored field (label)") + Expect(k8sClient.Get(ctx, client.ObjectKey{Name: saName, Namespace: istioNamespace}, sa)).To(Succeed()) + sa.Labels["app"] = "tampered" + Expect(k8sClient.Update(ctx, sa)).To(Succeed()) + + Step("Verifying label is reverted but automountServiceAccountToken is preserved") + Eventually(func(g Gomega) { + g.Expect(k8sClient.Get(ctx, client.ObjectKey{Name: saName, Namespace: istioNamespace}, sa)).To(Succeed()) + g.Expect(sa.Labels["app"]).To(Equal("istiod"), + "non-ignored label should be reverted by reconciliation") + g.Expect(sa.AutomountServiceAccountToken).To(HaveValue(Equal(false)), + "automountServiceAccountToken should be preserved because the rule strips it from Helm output") + }).Should(Succeed()) + }) + + It("preserves in-cluster secrets on ServiceAccount after reconciliation", func() { + Step("Setting secrets in-cluster") + sa := &corev1.ServiceAccount{} + Expect(k8sClient.Get(ctx, client.ObjectKey{Name: saName, Namespace: istioNamespace}, sa)).To(Succeed()) + sa.Secrets = append(sa.Secrets, corev1.ObjectReference{Name: "preserved-token-secret"}) + Expect(k8sClient.Update(ctx, sa)).To(Succeed()) + + Step("Triggering reconciliation by modifying a non-ignored field (label)") + Expect(k8sClient.Get(ctx, client.ObjectKey{Name: saName, Namespace: istioNamespace}, sa)).To(Succeed()) + sa.Labels["app"] = "tampered" + Expect(k8sClient.Update(ctx, sa)).To(Succeed()) + + Step("Verifying label is reverted but secrets is preserved") + Eventually(func(g Gomega) { + g.Expect(k8sClient.Get(ctx, client.ObjectKey{Name: saName, Namespace: istioNamespace}, sa)).To(Succeed()) + g.Expect(sa.Labels["app"]).To(Equal("istiod"), + "non-ignored label should be reverted by reconciliation") + g.Expect(sa.Secrets).To(ContainElement(corev1.ObjectReference{Name: "preserved-token-secret"}), + "secrets should be preserved because the rule strips it from Helm output") + }).Should(Succeed()) + }) + }) +}) diff --git a/tests/integration/api/istiorevision_test.go b/tests/integration/api/istiorevision_test.go index 47b8336c67..23c3b87114 100644 --- a/tests/integration/api/istiorevision_test.go +++ b/tests/integration/api/istiorevision_test.go @@ -678,46 +678,6 @@ 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", diff --git a/tests/integration/api/suite_test.go b/tests/integration/api/suite_test.go index 15deae54f6..d3965e1368 100644 --- a/tests/integration/api/suite_test.go +++ b/tests/integration/api/suite_test.go @@ -28,6 +28,7 @@ import ( "github.com/istio-ecosystem/sail-operator/controllers/istiorevisiontag" "github.com/istio-ecosystem/sail-operator/controllers/ztunnel" "github.com/istio-ecosystem/sail-operator/pkg/config" + "github.com/istio-ecosystem/sail-operator/pkg/fieldignore" "github.com/istio-ecosystem/sail-operator/pkg/helm" "github.com/istio-ecosystem/sail-operator/pkg/scheme" "github.com/istio-ecosystem/sail-operator/pkg/test" @@ -81,7 +82,8 @@ var _ = BeforeSuite(func() { panic(err) } - chartManager = helm.NewChartManager(mgr.GetConfig(), "") + chartManager = helm.NewChartManager(mgr.GetConfig(), "", + helm.WithFieldIgnoreRules(fieldignore.IntoUntypedAll(fieldignore.DefaultRules))) operatorNs := &corev1.Namespace{ObjectMeta: v1.ObjectMeta{Name: operatorNamespace}} Expect(k8sClient.Create(context.TODO(), operatorNs)).To(Succeed()) From c6b2461324e05afa9fcea90d8c4d518856c5c778 Mon Sep 17 00:00:00 2001 From: Nick Fox Date: Mon, 4 May 2026 16:31:21 -0400 Subject: [PATCH 2/2] Refactor types for FieldIgnoreRules Removes the "Untyped" variants of FieldIgnoreRules in favor of a passing `Unstructured` as the specific type for generic rules coming from either helm manifest or user input. Signed-off-by: Nick Fox --- cmd/main.go | 2 +- controllers/istiocni/istiocni_controller.go | 20 +- .../istiorevision/istiorevision_controller.go | 47 ++--- .../istiorevisiontag_controller.go | 4 +- controllers/webhook/webhook_controller.go | 2 +- controllers/ztunnel/ztunnel_controller.go | 17 +- pkg/fieldignore/defaults.go | 58 ++++-- pkg/fieldignore/fieldignore.go | 108 ++++------- pkg/fieldignore/fieldignore_test.go | 183 +++++++++++------- pkg/helm/chartmanager.go | 4 +- pkg/helm/postrenderer.go | 4 +- pkg/helm/postrenderer_test.go | 56 ++---- tests/integration/api/suite_test.go | 2 +- 13 files changed, 238 insertions(+), 269 deletions(-) diff --git a/cmd/main.go b/cmd/main.go index 9d456ae7fb..68bd519c78 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -204,7 +204,7 @@ func main() { } chartManager := helm.NewChartManager(mgr.GetConfig(), os.Getenv("HELM_DRIVER"), - helm.WithFieldIgnoreRules(fieldignore.IntoUntypedAll(fieldignore.DefaultRules))) + helm.WithFieldIgnoreRules(fieldignore.DefaultIgnoreRules)) err = istio.NewReconciler(reconcilerCfg, mgr.GetClient(), mgr.GetScheme(), tlsConfig). SetupWithManager(mgr) diff --git a/controllers/istiocni/istiocni_controller.go b/controllers/istiocni/istiocni_controller.go index ba54dea90a..97fdca811e 100644 --- a/controllers/istiocni/istiocni_controller.go +++ b/controllers/istiocni/istiocni_controller.go @@ -162,19 +162,15 @@ func (r *Reconciler) SetupWithManager(mgr ctrl.Manager) error { Named("istiocni"). // namespaced resources - Watches(&corev1.ConfigMap{}, ownedResourceHandler, - builder.WithPredicates(fieldignore.RulesFor(fieldignore.DefaultRules, &corev1.ConfigMap{}).NewPredicate())). - Watches(&appsv1.DaemonSet{}, ownedResourceHandler, - builder.WithPredicates(fieldignore.RulesFor(fieldignore.DefaultRules, &appsv1.DaemonSet{}).NewPredicate())). - Watches(&corev1.ResourceQuota{}, ownedResourceHandler, - builder.WithPredicates(fieldignore.RulesFor(fieldignore.DefaultRules, &corev1.ResourceQuota{}).NewPredicate())). + Watches(&corev1.ConfigMap{}, ownedResourceHandler). + Watches(&appsv1.DaemonSet{}, ownedResourceHandler). + Watches(&corev1.ResourceQuota{}, ownedResourceHandler). // +lint-watches:ignore: NetworkPolicy (FIXME: NetworkPolicy has not yet been added upstream, but is WIP) - Watches(&networkingv1.NetworkPolicy{}, ownedResourceHandler, - builder.WithPredicates(fieldignore.RulesFor(fieldignore.DefaultRules, &networkingv1.NetworkPolicy{}).NewPredicate(), predicate.IgnoreUpdate())). + Watches(&networkingv1.NetworkPolicy{}, ownedResourceHandler, builder.WithPredicates(predicate.IgnoreUpdate())). Watches(&corev1.ServiceAccount{}, ownedResourceHandler, builder.WithPredicates( - fieldignore.RulesFor(fieldignore.DefaultRules, &corev1.ServiceAccount{}).NewPredicate(), + fieldignore.ServiceAccountIgnoreRules.NewPredicate(), predicate.IgnoreUpdateWhenAnnotation())). // TODO: only register NetAttachDef if the CRD is installed (may also need to watch for CRD creation) @@ -183,10 +179,8 @@ func (r *Reconciler) SetupWithManager(mgr ctrl.Manager) error { // cluster-scoped resources // +lint-watches:ignore: Namespace (not present in charts, but must be watched to reconcile IstioCni when its namespace is created) Watches(&corev1.Namespace{}, namespaceHandler). - Watches(&rbacv1.ClusterRole{}, ownedResourceHandler, - builder.WithPredicates(fieldignore.RulesFor(fieldignore.DefaultRules, &rbacv1.ClusterRole{}).NewPredicate())). - Watches(&rbacv1.ClusterRoleBinding{}, ownedResourceHandler, - builder.WithPredicates(fieldignore.RulesFor(fieldignore.DefaultRules, &rbacv1.ClusterRoleBinding{}).NewPredicate())). + Watches(&rbacv1.ClusterRole{}, ownedResourceHandler). + Watches(&rbacv1.ClusterRoleBinding{}, ownedResourceHandler). Complete(reconciler.NewStandardReconcilerWithFinalizer[*v1.IstioCNI](r.Client, r.Reconcile, r.Finalize, constants.FinalizerName)) } diff --git a/controllers/istiorevision/istiorevision_controller.go b/controllers/istiorevision/istiorevision_controller.go index 5b5bda7876..eb36a87592 100644 --- a/controllers/istiorevision/istiorevision_controller.go +++ b/controllers/istiorevision/istiorevision_controller.go @@ -275,41 +275,26 @@ func (r *Reconciler) SetupWithManager(mgr ctrl.Manager) error { Named("istiorevision"). // namespaced resources - Watches(&corev1.ConfigMap{}, ownedResourceHandler, - builder.WithPredicates(fieldignore.RulesFor(fieldignore.DefaultRules, &corev1.ConfigMap{}).NewPredicate(), predicate2.IgnoreUpdateWhenAnnotation())). + 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(fieldignore.RulesFor(fieldignore.DefaultRules, &appsv1.Deployment{}).NewPredicate(), predicate2.IgnoreUpdateWhenAnnotation())). + 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, - builder.WithPredicates( - fieldignore.RulesFor(fieldignore.DefaultRules, &discoveryv1.EndpointSlice{}).NewPredicate(), - predicate2.IgnoreUpdateWhenAnnotation())). + Watches(&discoveryv1.EndpointSlice{}, endpointSliceHandler, builder.WithPredicates(predicate2.IgnoreUpdateWhenAnnotation())). Watches(&corev1.Service{}, ownedResourceHandler, - builder.WithPredicates( - fieldignore.RulesFor(fieldignore.DefaultRules, &corev1.Service{}).NewPredicate(), - ignoreStatusChange(), predicate2.IgnoreUpdateWhenAnnotation())). + 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( - fieldignore.RulesFor(fieldignore.DefaultRules, &networkingv1.NetworkPolicy{}).NewPredicate(), - ignoreStatusChange(), predicate2.IgnoreUpdateWhenAnnotation())). + builder.WithPredicates(ignoreStatusChange(), predicate2.IgnoreUpdateWhenAnnotation())). Watches(&corev1.ServiceAccount{}, ownedResourceHandler, - builder.WithPredicates(fieldignore.RulesFor(fieldignore.DefaultRules, &corev1.ServiceAccount{}).NewPredicate(), predicate2.IgnoreUpdateWhenAnnotation())). - Watches(&rbacv1.Role{}, ownedResourceHandler, - builder.WithPredicates(fieldignore.RulesFor(fieldignore.DefaultRules, &rbacv1.Role{}).NewPredicate(), predicate2.IgnoreUpdateWhenAnnotation())). - Watches(&rbacv1.RoleBinding{}, ownedResourceHandler, - builder.WithPredicates(fieldignore.RulesFor(fieldignore.DefaultRules, &rbacv1.RoleBinding{}).NewPredicate(), predicate2.IgnoreUpdateWhenAnnotation())). + builder.WithPredicates(fieldignore.ServiceAccountIgnoreRules.NewPredicate(), predicate2.IgnoreUpdateWhenAnnotation())). + Watches(&rbacv1.Role{}, ownedResourceHandler, builder.WithPredicates(predicate2.IgnoreUpdateWhenAnnotation())). + Watches(&rbacv1.RoleBinding{}, ownedResourceHandler, builder.WithPredicates(predicate2.IgnoreUpdateWhenAnnotation())). Watches(&policyv1.PodDisruptionBudget{}, ownedResourceHandler, - builder.WithPredicates( - fieldignore.RulesFor(fieldignore.DefaultRules, &policyv1.PodDisruptionBudget{}).NewPredicate(), - ignoreStatusChange(), predicate2.IgnoreUpdateWhenAnnotation())). + builder.WithPredicates(ignoreStatusChange(), predicate2.IgnoreUpdateWhenAnnotation())). Watches(&autoscalingv2.HorizontalPodAutoscaler{}, ownedResourceHandler, - builder.WithPredicates( - fieldignore.RulesFor(fieldignore.DefaultRules, &autoscalingv2.HorizontalPodAutoscaler{}).NewPredicate(), - ignoreStatusChange(), predicate2.IgnoreUpdateWhenAnnotation())). + 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()), builder.WithPredicates(predicate2.IgnoreUpdateWhenAnnotation())). @@ -321,19 +306,15 @@ func (r *Reconciler) SetupWithManager(mgr ctrl.Manager) error { Watches(&v1.IstioRevisionTag{}, revisionTagHandler). // cluster-scoped resources - Watches(&rbacv1.ClusterRole{}, ownedResourceHandler, - builder.WithPredicates(fieldignore.RulesFor(fieldignore.DefaultRules, &rbacv1.ClusterRole{}).NewPredicate(), predicate2.IgnoreUpdateWhenAnnotation())). - Watches(&rbacv1.ClusterRoleBinding{}, ownedResourceHandler, - builder.WithPredicates( - fieldignore.RulesFor(fieldignore.DefaultRules, &rbacv1.ClusterRoleBinding{}).NewPredicate(), - predicate2.IgnoreUpdateWhenAnnotation())). + Watches(&rbacv1.ClusterRole{}, ownedResourceHandler, builder.WithPredicates(predicate2.IgnoreUpdateWhenAnnotation())). + Watches(&rbacv1.ClusterRoleBinding{}, ownedResourceHandler, builder.WithPredicates(predicate2.IgnoreUpdateWhenAnnotation())). Watches(&admissionv1.MutatingWebhookConfiguration{}, ownedResourceHandler, builder.WithPredicates( - fieldignore.RulesFor(fieldignore.DefaultRules, &admissionv1.MutatingWebhookConfiguration{}).NewPredicate(), + fieldignore.MutatingWebhookIgnoreRules.NewPredicate(), predicate2.IgnoreUpdateWhenAnnotation())). Watches(&admissionv1.ValidatingWebhookConfiguration{}, ownedResourceHandler, builder.WithPredicates( - fieldignore.RulesFor(fieldignore.DefaultRules, &admissionv1.ValidatingWebhookConfiguration{}).NewPredicate(), + fieldignore.ValidatingWebhookIgnoreRules.NewPredicate(), predicate2.IgnoreUpdateWhenAnnotation())). // +lint-watches:ignore: IstioCNI (not found in charts, but this controller needs to watch it to update the IstioRevision status) diff --git a/controllers/istiorevisiontag/istiorevisiontag_controller.go b/controllers/istiorevisiontag/istiorevisiontag_controller.go index aaad3ad3ca..83d57aa5d7 100644 --- a/controllers/istiorevisiontag/istiorevisiontag_controller.go +++ b/controllers/istiorevisiontag/istiorevisiontag_controller.go @@ -253,8 +253,8 @@ func (r *Reconciler) SetupWithManager(mgr ctrl.Manager) error { Watches(&v1.Istio{}, operatorResourcesHandler). Watches(&v1.IstioRevision{}, operatorResourcesHandler). Watches(&admissionv1.MutatingWebhookConfiguration{}, ownedResourceHandler, - builder.WithPredicates(fieldignore.RulesFor(fieldignore.DefaultRules, &admissionv1.MutatingWebhookConfiguration{}).NewPredicate())). - Complete(reconciler.NewStandardReconcilerWithFinalizer[*v1.IstioRevisionTag](r.Client, r.Reconcile, r.Finalize, constants.FinalizerName)) + builder.WithPredicates(fieldignore.ValidatingWebhookIgnoreRules.NewPredicate())). + Complete(reconciler.NewStandardReconcilerWithFinalizer(r.Client, r.Reconcile, r.Finalize, constants.FinalizerName)) } func (r *Reconciler) determineStatus(ctx context.Context, tag *v1.IstioRevisionTag, diff --git a/controllers/webhook/webhook_controller.go b/controllers/webhook/webhook_controller.go index 5103551267..3fa02e1433 100644 --- a/controllers/webhook/webhook_controller.go +++ b/controllers/webhook/webhook_controller.go @@ -192,7 +192,7 @@ func (r *Reconciler) SetupWithManager(mgr ctrl.Manager) error { // +lint-watches:ignore: IstioRevision (not found in charts, but this is the main resource watched by this controller) Watches(&admissionv1.MutatingWebhookConfiguration{}, objectHandler, builder.WithPredicates( - fieldignore.RulesFor(fieldignore.DefaultRules, &admissionv1.MutatingWebhookConfiguration{}).NewPredicate(), + fieldignore.MutatingWebhookIgnoreRules.NewPredicate(), ownedByRemoteIstioRevisionPredicate(mgr.GetClient()))). Named("mutatingwebhookconfiguration"). Complete(reconciler.NewStandardReconciler[*admissionv1.MutatingWebhookConfiguration](r.Client, r.Reconcile)) diff --git a/controllers/ztunnel/ztunnel_controller.go b/controllers/ztunnel/ztunnel_controller.go index 72b43bbb01..95353d4517 100644 --- a/controllers/ztunnel/ztunnel_controller.go +++ b/controllers/ztunnel/ztunnel_controller.go @@ -185,22 +185,17 @@ func (r *Reconciler) SetupWithManager(mgr ctrl.Manager) error { Named("ztunnel"). // namespaced resources - Watches(&corev1.ConfigMap{}, ownedResourceHandler, - builder.WithPredicates(fieldignore.RulesFor(fieldignore.DefaultRules, &corev1.ConfigMap{}).NewPredicate())). - Watches(&appsv1.DaemonSet{}, ownedResourceHandler, - builder.WithPredicates(fieldignore.RulesFor(fieldignore.DefaultRules, &appsv1.DaemonSet{}).NewPredicate())). - Watches(&corev1.ResourceQuota{}, ownedResourceHandler, - builder.WithPredicates(fieldignore.RulesFor(fieldignore.DefaultRules, &corev1.ResourceQuota{}).NewPredicate())). + Watches(&corev1.ConfigMap{}, ownedResourceHandler). + Watches(&appsv1.DaemonSet{}, ownedResourceHandler). + Watches(&corev1.ResourceQuota{}, ownedResourceHandler). Watches(&corev1.ServiceAccount{}, ownedResourceHandler, - builder.WithPredicates(fieldignore.RulesFor(fieldignore.DefaultRules, &corev1.ServiceAccount{}).NewPredicate(), predicate.IgnoreUpdateWhenAnnotation())). + builder.WithPredicates(fieldignore.ServiceAccountIgnoreRules.NewPredicate(), predicate.IgnoreUpdateWhenAnnotation())). // cluster-scoped resources // +lint-watches:ignore: Namespace (not present in charts, but must be watched to reconcile ZTunnel when its namespace is created) Watches(&corev1.Namespace{}, namespaceHandler). - Watches(&rbacv1.ClusterRole{}, ownedResourceHandler, - builder.WithPredicates(fieldignore.RulesFor(fieldignore.DefaultRules, &rbacv1.ClusterRole{}).NewPredicate())). - Watches(&rbacv1.ClusterRoleBinding{}, ownedResourceHandler, - builder.WithPredicates(fieldignore.RulesFor(fieldignore.DefaultRules, &rbacv1.ClusterRoleBinding{}).NewPredicate())). + Watches(&rbacv1.ClusterRole{}, ownedResourceHandler). + Watches(&rbacv1.ClusterRoleBinding{}, ownedResourceHandler). Watches(&v1.Istio{}, operatorResourcesHandler). Watches(&v1.IstioRevision{}, operatorResourcesHandler). Complete(reconciler.NewStandardReconcilerWithFinalizer[*v1.ZTunnel](r.Client, r.Reconcile, r.Finalize, constants.FinalizerName)) diff --git a/pkg/fieldignore/defaults.go b/pkg/fieldignore/defaults.go index 44123b62cb..9ea9c9fc37 100644 --- a/pkg/fieldignore/defaults.go +++ b/pkg/fieldignore/defaults.go @@ -15,29 +15,53 @@ package fieldignore import ( + "slices" + admissionv1 "k8s.io/api/admissionregistration/v1" corev1 "k8s.io/api/core/v1" + "sigs.k8s.io/controller-runtime/pkg/client" ) // DefaultRules defines the default set of fields to ignore on resources managed // by the operator. These prevent reconciliation loops caused by other // controllers (e.g. istiod, pull-secret injectors, Azure admission enforcer) // updating fields that the operator also manages via Helm. -// -// Use RulesFor to extract typed rules for a specific resource type. -var DefaultRules = []RuleSet{ - TypedRuleSet[*admissionv1.ValidatingWebhookConfiguration]{ - {Fields: []string{"webhooks[*].failurePolicy"}, Scope: IgnoreScopeReconcileAndUpgrade}, - {Fields: []string{"webhooks[*].clientConfig.caBundle"}}, - }, - TypedRuleSet[*admissionv1.MutatingWebhookConfiguration]{ - {Fields: []string{"webhooks[*].clientConfig.caBundle"}}, - // AKS manipulates MutatingWebhookConfigurations. See https://github.com/istio-ecosystem/sail-operator/issues/1148 - {Fields: []string{"webhooks[*].namespaceSelector.matchExpressions[key=kubernetes.azure.com/managedby]"}, Scope: IgnoreScopeReconcile}, - }, - TypedRuleSet[*corev1.ServiceAccount]{ - {Fields: []string{"imagePullSecrets"}}, - {Fields: []string{"automountServiceAccountToken"}}, - {Fields: []string{"secrets"}}, - }, +var DefaultIgnoreRules = slices.Concat( + convertToGenericRules(ValidatingWebhookIgnoreRules), + convertToGenericRules(MutatingWebhookIgnoreRules), + convertToGenericRules(ServiceAccountIgnoreRules), +) + +// convertToGenericRules converts a slice of a specific impl and returns a generic slice. +func convertToGenericRules[T client.Object](rules RuleSet[T]) RuleSet[client.Object] { + var rs RuleSet[client.Object] + for _, rule := range rules { + if rule.Name != "" { + rs = append(rs, NewFieldIgnoreRuleWithName[client.Object](rule.obj, rule.Name, rule.Fields, rule.Scope)) + } else { + rs = append(rs, NewFieldIgnoreRule[client.Object](rule.obj, rule.Fields, rule.Scope)) + } + } + return rs +} + +var ValidatingWebhookIgnoreRules = RuleSet[*admissionv1.ValidatingWebhookConfiguration]{ + NewFieldIgnoreRule(&admissionv1.ValidatingWebhookConfiguration{}, []string{"webhooks[*].failurePolicy"}, IgnoreScopeReconcileAndUpgrade), + NewFieldIgnoreRule(&admissionv1.ValidatingWebhookConfiguration{}, []string{"webhooks[*].clientConfig.caBundle"}, IgnoreScopeAlways), +} + +var MutatingWebhookIgnoreRules = RuleSet[*admissionv1.MutatingWebhookConfiguration]{ + NewFieldIgnoreRule(&admissionv1.MutatingWebhookConfiguration{}, []string{"webhooks[*].clientConfig.caBundle"}, IgnoreScopeAlways), + // AKS manipulates MutatingWebhookConfigurations. See https://github.com/istio-ecosystem/sail-operator/issues/1148 + NewFieldIgnoreRule( + &admissionv1.MutatingWebhookConfiguration{}, + []string{"webhooks[*].namespaceSelector.matchExpressions[key=kubernetes.azure.com/managedby]"}, + IgnoreScopeReconcile, + ), +} + +var ServiceAccountIgnoreRules = RuleSet[*corev1.ServiceAccount]{ + NewFieldIgnoreRule(&corev1.ServiceAccount{}, []string{"imagePullSecrets"}, IgnoreScopeAlways), + NewFieldIgnoreRule(&corev1.ServiceAccount{}, []string{"automountServiceAccountToken"}, IgnoreScopeAlways), + NewFieldIgnoreRule(&corev1.ServiceAccount{}, []string{"secrets"}, IgnoreScopeAlways), } diff --git a/pkg/fieldignore/fieldignore.go b/pkg/fieldignore/fieldignore.go index c72ac8a56d..107ca4923a 100644 --- a/pkg/fieldignore/fieldignore.go +++ b/pkg/fieldignore/fieldignore.go @@ -22,8 +22,8 @@ import ( "github.com/istio-ecosystem/sail-operator/pkg/scheme" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" - "k8s.io/apimachinery/pkg/runtime/schema" "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/apiutil" "sigs.k8s.io/controller-runtime/pkg/event" "sigs.k8s.io/controller-runtime/pkg/predicate" ) @@ -54,6 +54,11 @@ const ( // - "webhooks[*].clientConfig.caBundle" → deletes caBundle nested inside each webhook // - "spec.template.metadata.annotations" → deletes a deeply nested field type FieldIgnoreRule[T client.Object] struct { + // obj is an empty instance of the object type. It is used to extract the GVK. + // If go supported accessing a subset of the Type parameter's methods, we could avoid this. + // e.g. T.GetGroupVersionKind() + obj T + // Name is an optional exact name match. Empty matches all names. Name string `json:"name,omitempty"` @@ -64,58 +69,32 @@ type FieldIgnoreRule[T client.Object] struct { Scope IgnoreScope `json:"scope,omitempty"` } -// RuleSet is an interface for type-safe rule collections that -// can be stored together in a single slice regardless of their type parameter. -type RuleSet interface { - IntoUntyped() []UntypedFieldIgnoreRule -} - -// RulesFor returns the typed rules from a mixed slice that match type T. -func RulesFor[T client.Object](allRules []RuleSet, _ T) TypedRuleSet[T] { - var result TypedRuleSet[T] - for _, rules := range allRules { - if typed, ok := rules.(TypedRuleSet[T]); ok { - result = append(result, typed...) - } +func NewFieldIgnoreRule[T client.Object](obj T, fields []string, scope IgnoreScope) FieldIgnoreRule[T] { + return FieldIgnoreRule[T]{ + obj: obj, + Fields: fields, + Scope: scope, } - return result } -// IntoUntypedAll flattens a mixed slice of typed rule sets into a single untyped slice. -func IntoUntypedAll(allRules []RuleSet) []UntypedFieldIgnoreRule { - var result []UntypedFieldIgnoreRule - for _, rules := range allRules { - result = append(result, rules.IntoUntyped()...) +func NewFieldIgnoreRuleWithName[T client.Object](obj T, name string, fields []string, scope IgnoreScope) FieldIgnoreRule[T] { + return FieldIgnoreRule[T]{ + obj: obj, + Name: name, + Fields: fields, + Scope: scope, } - return result } -// TypedRuleSet is a type-safe collection of field ignore rules for a specific resource type. -type TypedRuleSet[T client.Object] []FieldIgnoreRule[T] - -// IntoUntyped converts typed rules into untyped rules for use with manifests. -func (rules TypedRuleSet[T]) IntoUntyped() []UntypedFieldIgnoreRule { - gvk := gvkFor[T]() - result := make([]UntypedFieldIgnoreRule, len(rules)) - for i, r := range rules { - result[i] = UntypedFieldIgnoreRule{ - Group: gvk.Group, - Version: gvk.Version, - Kind: gvk.Kind, - Name: r.Name, - Fields: r.Fields, - Scope: r.Scope, - } - } - return result -} +// RuleSet is a collection of field ignore rules for a specific resource type. +type RuleSet[T client.Object] []FieldIgnoreRule[T] // NewPredicate returns a predicate that ignores changes to fields specified by // the given typed rules. On update events it converts both old and new objects to // unstructured maps, removes the ignored fields (plus standard metadata noise // like resourceVersion, generation, managedFields), and only triggers // reconciliation when the remaining content differs. -func (rules TypedRuleSet[T]) NewPredicate() predicate.Funcs { +func (rules RuleSet[T]) NewPredicate() predicate.Funcs { return predicate.Funcs{ UpdateFunc: func(e event.UpdateEvent) bool { if e.ObjectOld == nil || e.ObjectNew == nil { @@ -126,40 +105,37 @@ func (rules TypedRuleSet[T]) NewPredicate() predicate.Funcs { } } -// UntypedFieldIgnoreRule is the untyped version used for manifest matching. +// GenericFieldIgnoreRule is the generic version used for manifest matching. // This is primarily for Helm post-rendering and runtime matching against unstructured manifests. -type UntypedFieldIgnoreRule struct { - Group string `json:"group"` - Version string `json:"version"` - Kind string `json:"kind"` - Name string `json:"name,omitempty"` - Fields []string `json:"fields"` - Scope IgnoreScope `json:"scope,omitempty"` -} +type GenericFieldIgnoreRule = FieldIgnoreRule[client.Object] // MatchesManifest returns true if the untyped rule applies to an unstructured manifest map. -func (r UntypedFieldIgnoreRule) MatchesManifest(manifest map[string]any) bool { +func MatchesManifest(rule GenericFieldIgnoreRule, manifest map[string]any) bool { apiVersion, _, _ := unstructured.NestedString(manifest, "apiVersion") kind, _, _ := unstructured.NestedString(manifest, "kind") name, _, _ := unstructured.NestedString(manifest, "metadata", "name") - gv, err := schema.ParseGroupVersion(apiVersion) + // Unstructured is handled by this even if the type is not registered in the scheme. + gvk, err := apiutil.GVKForObject(rule.obj, scheme.Scheme) if err != nil { return false } - if r.Group != gv.Group || r.Version != gv.Version || r.Kind != kind { + + if gvk.GroupVersion().Identifier() != apiVersion || gvk.Kind != kind { return false } - if r.Name != "" && r.Name != name { + + if rule.Name != "" && rule.Name != name { return false } + return true } // RemoveFieldsFromManifest removes ignored fields from an unstructured manifest map. // Rules with Scope=Reconcile are never applied (predicate-only). Rules with // Scope=ReconcileAndUpgrade are only applied when isUpdate is true. -func RemoveFieldsFromManifest(manifest map[string]any, rules []UntypedFieldIgnoreRule, isUpdate bool) { +func RemoveFieldsFromManifest(manifest map[string]any, rules []GenericFieldIgnoreRule, isUpdate bool) { for _, rule := range rules { switch rule.Scope { case IgnoreScopeReconcile: @@ -169,7 +145,7 @@ func RemoveFieldsFromManifest(manifest map[string]any, rules []UntypedFieldIgnor continue } } - if !rule.MatchesManifest(manifest) { + if !MatchesManifest(rule, manifest) { continue } for _, field := range rule.Fields { @@ -178,7 +154,7 @@ func RemoveFieldsFromManifest(manifest map[string]any, rules []UntypedFieldIgnor } } -func objectsChangedIgnoringFields[T client.Object](oldObj, newObj client.Object, rules TypedRuleSet[T]) bool { +func objectsChangedIgnoringFields[T client.Object](oldObj, newObj client.Object, rules RuleSet[T]) bool { name := newObj.GetName() // Collect fields from all matching rules regardless of Scope. @@ -215,24 +191,6 @@ func objectsChangedIgnoringFields[T client.Object](oldObj, newObj client.Object, return !reflect.DeepEqual(oldMap, newMap) } -// gvkFor derives the GVK for a client.Object type from the global scheme. -func gvkFor[T client.Object]() schema.GroupVersionKind { - var t T - typ := reflect.TypeOf(t) - var instance reflect.Value - if typ.Kind() == reflect.Pointer { - instance = reflect.New(typ.Elem()) - } else { - instance = reflect.New(typ) - } - obj := instance.Interface().(client.Object) - gvks, _, err := scheme.Scheme.ObjectKinds(obj) - if err != nil || len(gvks) == 0 { - panic("no GVK found for type " + typ.String()) - } - return gvks[0] -} - // clearMetadataFields removes standard metadata fields that change on every // update and should never trigger reconciliation. func clearMetadataFields(obj map[string]any) { diff --git a/pkg/fieldignore/fieldignore_test.go b/pkg/fieldignore/fieldignore_test.go index 0da21276b9..ac30eb9dff 100644 --- a/pkg/fieldignore/fieldignore_test.go +++ b/pkg/fieldignore/fieldignore_test.go @@ -20,6 +20,9 @@ import ( "github.com/google/go-cmp/cmp" admissionv1 "k8s.io/api/admissionregistration/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime/schema" + "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/event" ) @@ -281,19 +284,16 @@ func TestParseArrayPredicate(t *testing.T) { func TestMatchesManifest(t *testing.T) { tests := []struct { name string - rule UntypedFieldIgnoreRule + rule GenericFieldIgnoreRule manifest map[string]any want bool }{ { name: "matching manifest with exact name", - rule: UntypedFieldIgnoreRule{ - Group: "admissionregistration.k8s.io", - Version: "v1", - Kind: "ValidatingWebhookConfiguration", - Name: "istiod-istio-system-validator", - Fields: []string{"webhooks[*].failurePolicy"}, - }, + rule: NewFieldIgnoreRuleWithName[client.Object]( + &admissionv1.ValidatingWebhookConfiguration{}, + "istiod-istio-system-validator", + []string{"webhooks[*].failurePolicy"}, IgnoreScopeAlways), manifest: map[string]any{ "apiVersion": "admissionregistration.k8s.io/v1", "kind": "ValidatingWebhookConfiguration", @@ -303,12 +303,9 @@ func TestMatchesManifest(t *testing.T) { }, { name: "matching manifest with no name requirement", - rule: UntypedFieldIgnoreRule{ - Group: "admissionregistration.k8s.io", - Version: "v1", - Kind: "ValidatingWebhookConfiguration", - Fields: []string{"webhooks[*].failurePolicy"}, - }, + rule: NewFieldIgnoreRule[client.Object]( + &admissionv1.ValidatingWebhookConfiguration{}, + []string{"webhooks[*].failurePolicy"}, IgnoreScopeAlways), manifest: map[string]any{ "apiVersion": "admissionregistration.k8s.io/v1", "kind": "ValidatingWebhookConfiguration", @@ -318,12 +315,9 @@ func TestMatchesManifest(t *testing.T) { }, { name: "non-matching kind", - rule: UntypedFieldIgnoreRule{ - Group: "admissionregistration.k8s.io", - Version: "v1", - Kind: "MutatingWebhookConfiguration", - Fields: []string{"webhooks[*].failurePolicy"}, - }, + rule: NewFieldIgnoreRule[client.Object]( + &admissionv1.MutatingWebhookConfiguration{}, + []string{"webhooks[*].failurePolicy"}, IgnoreScopeAlways), manifest: map[string]any{ "apiVersion": "admissionregistration.k8s.io/v1", "kind": "ValidatingWebhookConfiguration", @@ -333,13 +327,10 @@ func TestMatchesManifest(t *testing.T) { }, { name: "non-matching name", - rule: UntypedFieldIgnoreRule{ - Group: "admissionregistration.k8s.io", - Version: "v1", - Kind: "ValidatingWebhookConfiguration", - Name: "istiod-istio-system-validator", - Fields: []string{"webhooks[*].failurePolicy"}, - }, + rule: NewFieldIgnoreRuleWithName[client.Object]( + &admissionv1.ValidatingWebhookConfiguration{}, + "istiod-istio-system-validator", + []string{"webhooks[*].failurePolicy"}, IgnoreScopeAlways), manifest: map[string]any{ "apiVersion": "admissionregistration.k8s.io/v1", "kind": "ValidatingWebhookConfiguration", @@ -347,11 +338,85 @@ func TestMatchesManifest(t *testing.T) { }, want: false, }, + { + name: "unstructured object with matching GVK", + rule: NewFieldIgnoreRule[client.Object]( + newUnstructuredWithGVK("example.io", "v1", "MyCustomResource"), + []string{"spec.someField"}, IgnoreScopeAlways), + manifest: map[string]any{ + "apiVersion": "example.io/v1", + "kind": "MyCustomResource", + "metadata": map[string]any{"name": "test"}, + }, + want: true, + }, + { + name: "unstructured object with matching GVK and name", + rule: NewFieldIgnoreRuleWithName[client.Object]( + newUnstructuredWithGVK("example.io", "v1", "MyCustomResource"), + "specific-name", + []string{"spec.someField"}, IgnoreScopeAlways), + manifest: map[string]any{ + "apiVersion": "example.io/v1", + "kind": "MyCustomResource", + "metadata": map[string]any{"name": "specific-name"}, + }, + want: true, + }, + { + name: "unstructured object with matching GVK but wrong name", + rule: NewFieldIgnoreRuleWithName[client.Object]( + newUnstructuredWithGVK("example.io", "v1", "MyCustomResource"), + "expected-name", + []string{"spec.someField"}, IgnoreScopeAlways), + manifest: map[string]any{ + "apiVersion": "example.io/v1", + "kind": "MyCustomResource", + "metadata": map[string]any{"name": "other-name"}, + }, + want: false, + }, + { + name: "unstructured object with non-matching kind", + rule: NewFieldIgnoreRule[client.Object]( + newUnstructuredWithGVK("example.io", "v1alpha1", "MyCustomResource"), + []string{"spec.someField"}, IgnoreScopeAlways), + manifest: map[string]any{ + "apiVersion": "example.io/v1alpha1", + "kind": "DifferentResource", + "metadata": map[string]any{"name": "test"}, + }, + want: false, + }, + { + name: "unstructured object with no GVK set", + rule: NewFieldIgnoreRule[client.Object]( + &unstructured.Unstructured{}, + []string{"spec.someField"}, IgnoreScopeAlways), + manifest: map[string]any{ + "apiVersion": "example.io/v1", + "kind": "MyCustomResource", + "metadata": map[string]any{"name": "test"}, + }, + want: false, + }, + { + name: "unstructured object with core group (no group prefix in apiVersion)", + rule: NewFieldIgnoreRule[client.Object]( + newUnstructuredWithGVK("", "v1", "ConfigMap"), + []string{"data"}, IgnoreScopeAlways), + manifest: map[string]any{ + "apiVersion": "v1", + "kind": "ConfigMap", + "metadata": map[string]any{"name": "test"}, + }, + want: true, + }, } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { - got := tc.rule.MatchesManifest(tc.manifest) + got := MatchesManifest(tc.rule, tc.manifest) if got != tc.want { t.Errorf("MatchesManifest() = %v, want %v", got, tc.want) } @@ -360,23 +425,15 @@ func TestMatchesManifest(t *testing.T) { } func TestRemoveFieldsFromManifest(t *testing.T) { - reconcileAndUpgradeRules := []UntypedFieldIgnoreRule{ - { - Group: "admissionregistration.k8s.io", - Version: "v1", - Kind: "ValidatingWebhookConfiguration", - Fields: []string{"webhooks[*].failurePolicy"}, - Scope: IgnoreScopeReconcileAndUpgrade, - }, + reconcileAndUpgradeRules := []GenericFieldIgnoreRule{ + NewFieldIgnoreRule[client.Object]( + &admissionv1.ValidatingWebhookConfiguration{}, + []string{"webhooks[*].failurePolicy"}, IgnoreScopeReconcileAndUpgrade), } - reconcileOnlyRules := []UntypedFieldIgnoreRule{ - { - Group: "admissionregistration.k8s.io", - Version: "v1", - Kind: "ValidatingWebhookConfiguration", - Fields: []string{"webhooks[*].failurePolicy"}, - Scope: IgnoreScopeReconcile, - }, + reconcileOnlyRules := []GenericFieldIgnoreRule{ + NewFieldIgnoreRule[client.Object]( + &admissionv1.ValidatingWebhookConfiguration{}, + []string{"webhooks[*].failurePolicy"}, IgnoreScopeReconcile), } webhookManifest := func() map[string]any { @@ -390,7 +447,7 @@ func TestRemoveFieldsFromManifest(t *testing.T) { tests := []struct { name string - rules []UntypedFieldIgnoreRule + rules []GenericFieldIgnoreRule manifest map[string]any isUpdate bool expected map[string]any @@ -458,7 +515,7 @@ func TestRemoveFieldsFromManifest(t *testing.T) { } func TestNewPredicateWithEmptyName(t *testing.T) { - rules := TypedRuleSet[*admissionv1.ValidatingWebhookConfiguration]{ + rules := RuleSet[*admissionv1.ValidatingWebhookConfiguration]{ {Fields: []string{"webhooks[*].failurePolicy"}}, } pred := rules.NewPredicate() @@ -517,7 +574,7 @@ func TestNewPredicateWithEmptyName(t *testing.T) { } func TestNewPredicate(t *testing.T) { - rules := TypedRuleSet[*admissionv1.ValidatingWebhookConfiguration]{ + rules := RuleSet[*admissionv1.ValidatingWebhookConfiguration]{ {Name: "istiod-istio-system-validator", Fields: []string{"webhooks[*].failurePolicy"}}, } pred := rules.NewPredicate() @@ -591,8 +648,14 @@ func TestNewPredicate(t *testing.T) { } } +func newUnstructuredWithGVK(group string, version string, kind string) *unstructured.Unstructured { + u := &unstructured.Unstructured{} + u.SetGroupVersionKind(schema.GroupVersionKind{Group: group, Version: version, Kind: kind}) + return u +} + func TestNewPredicateMutatingWebhookConfiguration(t *testing.T) { - rules := TypedRuleSet[*admissionv1.MutatingWebhookConfiguration]{ + rules := RuleSet[*admissionv1.MutatingWebhookConfiguration]{ {Fields: []string{"webhooks[*].clientConfig.caBundle"}}, } pred := rules.NewPredicate() @@ -646,27 +709,3 @@ func TestNewPredicateMutatingWebhookConfiguration(t *testing.T) { }) } } - -func TestIntoUntyped(t *testing.T) { - rules := TypedRuleSet[*admissionv1.ValidatingWebhookConfiguration]{ - {Name: "istiod-validator", Fields: []string{"webhooks[*].failurePolicy"}, Scope: IgnoreScopeReconcileAndUpgrade}, - } - - untyped := rules.IntoUntyped() - if len(untyped) != 1 { - t.Fatalf("expected 1 untyped rule, got %d", len(untyped)) - } - r := untyped[0] - if r.Group != "admissionregistration.k8s.io" || r.Version != "v1" || r.Kind != "ValidatingWebhookConfiguration" { - t.Errorf("GVK = %s/%s %s, want admissionregistration.k8s.io/v1 ValidatingWebhookConfiguration", r.Group, r.Version, r.Kind) - } - if r.Name != "istiod-validator" { - t.Errorf("Name = %q, want %q", r.Name, "istiod-validator") - } - if len(r.Fields) != 1 || r.Fields[0] != "webhooks[*].failurePolicy" { - t.Errorf("Fields = %v, want [webhooks[*].failurePolicy]", r.Fields) - } - if r.Scope != IgnoreScopeReconcileAndUpgrade { - t.Errorf("Scope = %q, want %q", r.Scope, IgnoreScopeReconcileAndUpgrade) - } -} diff --git a/pkg/helm/chartmanager.go b/pkg/helm/chartmanager.go index d5bd8efd89..48ddcccb89 100644 --- a/pkg/helm/chartmanager.go +++ b/pkg/helm/chartmanager.go @@ -41,7 +41,7 @@ type ChartManager struct { restClientGetter genericclioptions.RESTClientGetter driver string managedByValue string - fieldIgnoreRules []fieldignore.UntypedFieldIgnoreRule + fieldIgnoreRules []fieldignore.GenericFieldIgnoreRule } // ChartManagerOption is a functional option for configuring a ChartManager. @@ -59,7 +59,7 @@ func WithManagedByValue(v string) ChartManagerOption { // WithFieldIgnoreRules configures the field ignore rules that the post-renderer // uses to strip fields from rendered manifests. // See fieldignore.IgnoreScope for how the Scope field controls stripping behavior. -func WithFieldIgnoreRules(rules []fieldignore.UntypedFieldIgnoreRule) ChartManagerOption { +func WithFieldIgnoreRules(rules []fieldignore.GenericFieldIgnoreRule) ChartManagerOption { return func(cm *ChartManager) { cm.fieldIgnoreRules = rules } diff --git a/pkg/helm/postrenderer.go b/pkg/helm/postrenderer.go index 0f93970d5f..d28b46ac9f 100644 --- a/pkg/helm/postrenderer.go +++ b/pkg/helm/postrenderer.go @@ -39,7 +39,7 @@ const ( // It also removes fields from rendered manifests according to the provided FieldIgnoreRules. // See fieldignore.IgnoreScope for how the Scope field controls install vs. upgrade behavior. func NewHelmPostRenderer(ownerReference *metav1.OwnerReference, ownerNamespace string, isUpdate bool, - managedByValue string, fieldIgnoreRules []fieldignore.UntypedFieldIgnoreRule, + managedByValue string, fieldIgnoreRules []fieldignore.GenericFieldIgnoreRule, ) postrenderer.PostRenderer { return HelmPostRenderer{ ownerReference: ownerReference, @@ -55,7 +55,7 @@ type HelmPostRenderer struct { ownerNamespace string isUpdate bool managedByValue string - fieldIgnoreRules []fieldignore.UntypedFieldIgnoreRule + fieldIgnoreRules []fieldignore.GenericFieldIgnoreRule } var _ postrenderer.PostRenderer = HelmPostRenderer{} diff --git a/pkg/helm/postrenderer_test.go b/pkg/helm/postrenderer_test.go index 1d1afdd0ed..4e846ab815 100644 --- a/pkg/helm/postrenderer_test.go +++ b/pkg/helm/postrenderer_test.go @@ -21,48 +21,26 @@ import ( "github.com/google/go-cmp/cmp" "github.com/istio-ecosystem/sail-operator/pkg/constants" "github.com/istio-ecosystem/sail-operator/pkg/fieldignore" + admissionv1 "k8s.io/api/admissionregistration/v1" + corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/client" ) // defaultTestRules replicates the built-in field ignore rules used in production. -var defaultTestRules = []fieldignore.UntypedFieldIgnoreRule{ - { - Group: "admissionregistration.k8s.io", - Version: "v1", - Kind: "ValidatingWebhookConfiguration", - Fields: []string{"webhooks[*].failurePolicy"}, - Scope: fieldignore.IgnoreScopeReconcileAndUpgrade, - }, - { - Group: "admissionregistration.k8s.io", - Version: "v1", - Kind: "ValidatingWebhookConfiguration", - Fields: []string{"webhooks[*].clientConfig.caBundle"}, - }, - { - Group: "admissionregistration.k8s.io", - Version: "v1", - Kind: "MutatingWebhookConfiguration", - Fields: []string{"webhooks[*].clientConfig.caBundle"}, - }, - { - Group: "", - Version: "v1", - Kind: "ServiceAccount", - Fields: []string{"imagePullSecrets"}, - }, - { - Group: "", - Version: "v1", - Kind: "ServiceAccount", - Fields: []string{"automountServiceAccountToken"}, - }, - { - Group: "", - Version: "v1", - Kind: "ServiceAccount", - Fields: []string{"secrets"}, - }, +var defaultTestRules = []fieldignore.GenericFieldIgnoreRule{ + fieldignore.NewFieldIgnoreRule[client.Object](&admissionv1.ValidatingWebhookConfiguration{}, + []string{"webhooks[*].failurePolicy"}, fieldignore.IgnoreScopeReconcileAndUpgrade), + fieldignore.NewFieldIgnoreRule[client.Object](&admissionv1.ValidatingWebhookConfiguration{}, + []string{"webhooks[*].clientConfig.caBundle"}, fieldignore.IgnoreScopeAlways), + fieldignore.NewFieldIgnoreRule[client.Object](&admissionv1.MutatingWebhookConfiguration{}, + []string{"webhooks[*].clientConfig.caBundle"}, fieldignore.IgnoreScopeAlways), + fieldignore.NewFieldIgnoreRule[client.Object](&corev1.ServiceAccount{}, + []string{"imagePullSecrets"}, fieldignore.IgnoreScopeAlways), + fieldignore.NewFieldIgnoreRule[client.Object](&corev1.ServiceAccount{}, + []string{"automountServiceAccountToken"}, fieldignore.IgnoreScopeAlways), + fieldignore.NewFieldIgnoreRule[client.Object](&corev1.ServiceAccount{}, + []string{"secrets"}, fieldignore.IgnoreScopeAlways), } func TestHelmPostRenderer(t *testing.T) { @@ -71,7 +49,7 @@ func TestHelmPostRenderer(t *testing.T) { ownerReference *metav1.OwnerReference ownerNamespace string isUpdate bool - fieldIgnoreRules []fieldignore.UntypedFieldIgnoreRule + fieldIgnoreRules []fieldignore.GenericFieldIgnoreRule input string expected string }{ diff --git a/tests/integration/api/suite_test.go b/tests/integration/api/suite_test.go index d3965e1368..be303dc1c5 100644 --- a/tests/integration/api/suite_test.go +++ b/tests/integration/api/suite_test.go @@ -83,7 +83,7 @@ var _ = BeforeSuite(func() { } chartManager = helm.NewChartManager(mgr.GetConfig(), "", - helm.WithFieldIgnoreRules(fieldignore.IntoUntypedAll(fieldignore.DefaultRules))) + helm.WithFieldIgnoreRules(fieldignore.DefaultIgnoreRules)) operatorNs := &corev1.Namespace{ObjectMeta: v1.ObjectMeta{Name: operatorNamespace}} Expect(k8sClient.Create(context.TODO(), operatorNs)).To(Succeed())