Implement temporary hack to prevent removal of pull secrets added by external controllers

controllers/istiocni/istiocni_controller.go

@@ -30,6 +30,7 @@ import (
 	"github.com/istio-ecosystem/sail-operator/pkg/helm"
 	"github.com/istio-ecosystem/sail-operator/pkg/istiovalues"
 	"github.com/istio-ecosystem/sail-operator/pkg/kube"
+	"github.com/istio-ecosystem/sail-operator/pkg/predicate"
 	"github.com/istio-ecosystem/sail-operator/pkg/reconciler"
 	"github.com/istio-ecosystem/sail-operator/pkg/validation"
 	appsv1 "k8s.io/api/apps/v1"
@@ -40,6 +41,7 @@ import (
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/types"
 	ctrl "sigs.k8s.io/controller-runtime"
+	"sigs.k8s.io/controller-runtime/pkg/builder"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/controller"
 	"sigs.k8s.io/controller-runtime/pkg/handler"
@@ -225,7 +227,11 @@ func (r *Reconciler) SetupWithManager(mgr ctrl.Manager) error {
 		Watches(&corev1.ConfigMap{}, ownedResourceHandler).
 		Watches(&appsv1.DaemonSet{}, ownedResourceHandler).
 		Watches(&corev1.ResourceQuota{}, ownedResourceHandler).
-		Watches(&corev1.ServiceAccount{}, 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())).
 
 		// TODO: only register NetAttachDef if the CRD is installed (may also need to watch for CRD creation)
 		// Owns(&multusv1.NetworkAttachmentDefinition{}).

controllers/istiorevision/istiorevision_controller.go

@@ -30,6 +30,7 @@ import (
 	"github.com/istio-ecosystem/sail-operator/pkg/errlist"
 	"github.com/istio-ecosystem/sail-operator/pkg/helm"
 	"github.com/istio-ecosystem/sail-operator/pkg/kube"
+	predicate2 "github.com/istio-ecosystem/sail-operator/pkg/predicate"
 	"github.com/istio-ecosystem/sail-operator/pkg/reconciler"
 	"github.com/istio-ecosystem/sail-operator/pkg/revision"
 	"github.com/istio-ecosystem/sail-operator/pkg/validation"
@@ -232,7 +233,11 @@ func (r *Reconciler) SetupWithManager(mgr ctrl.Manager) error {
 		Watches(&appsv1.Deployment{}, ownedResourceHandler). // we don't ignore the status here because we use it to calculate the IstioRevision status
 		Watches(&corev1.Endpoints{}, ownedResourceHandler).
 		Watches(&corev1.Service{}, ownedResourceHandler, builder.WithPredicates(ignoreStatusChange())).
-		Watches(&corev1.ServiceAccount{}, 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(predicate2.IgnoreUpdate())).
 		Watches(&rbacv1.Role{}, ownedResourceHandler).
 		Watches(&rbacv1.RoleBinding{}, ownedResourceHandler).
 		Watches(&policyv1.PodDisruptionBudget{}, ownedResourceHandler, builder.WithPredicates(ignoreStatusChange())).

pkg/predicate/predicate.go

@@ -0,0 +1,27 @@
+// 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 predicate
+
+import (
+	"sigs.k8s.io/controller-runtime/pkg/event"
+	"sigs.k8s.io/controller-runtime/pkg/predicate"
+)
+
+// ignoreUpdate returns a predicate that ignores update events.
+func IgnoreUpdate() predicate.Funcs {
+	return predicate.Funcs{
+		UpdateFunc: func(e event.UpdateEvent) bool { return false },
+	}
+}

tests/integration/api/istiocni_test.go

@@ -22,6 +22,7 @@ import (
 	"time"
 
 	"github.com/istio-ecosystem/sail-operator/api/v1alpha1"
+	"github.com/istio-ecosystem/sail-operator/pkg/enqueuelogger"
 	"github.com/istio-ecosystem/sail-operator/pkg/kube"
 	. "github.com/istio-ecosystem/sail-operator/pkg/test/util/ginkgo"
 	"github.com/istio-ecosystem/sail-operator/pkg/test/util/supportedversion"
@@ -43,6 +44,8 @@ var _ = Describe("IstioCNI", Ordered, func() {
 	SetDefaultEventuallyPollingInterval(time.Second)
 	SetDefaultEventuallyTimeout(30 * time.Second)
 
+	enqueuelogger.LogEnqueueEvents = true
+
 	ctx := context.Background()
 
 	namespace := &corev1.Namespace{
@@ -236,6 +239,32 @@ var _ = Describe("IstioCNI", Ordered, func() {
 					}).Should(Succeed())
 				})
 			})
+
+			It("skips reconcile when a pull secret is added to service account", func() {
+				waitForInFlightReconcileToFinish()
+
+				sa := &corev1.ServiceAccount{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:      "istio-cni",
+						Namespace: cniNamespace,
+					},
+				}
+				Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(sa), sa)).To(Succeed())
+
+				beforeCount := getIstioCNIReconcileCount(Default)
+
+				By("adding pull secret to ServiceAccount")
+				sa.ImagePullSecrets = append(sa.ImagePullSecrets, corev1.LocalObjectReference{Name: "other-pull-secret"})
+				Expect(k8sClient.Update(ctx, sa)).To(Succeed())
+
+				Consistently(func(g Gomega) {
+					afterCount := getIstioCNIReconcileCount(g)
+					g.Expect(afterCount).To(Equal(beforeCount))
+				}, 5*time.Second).Should(Succeed(), "IstioRevision was reconciled when it shouldn't have been")
+
+				Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(sa), sa)).To(Succeed())
+				Expect(sa.ImagePullSecrets).To(ContainElement(corev1.LocalObjectReference{Name: "other-pull-secret"}))
+			})
 		})
 
 		When("the resource is deleted", func() {

tests/integration/api/istiorevision_test.go

@@ -315,8 +315,8 @@ var _ = Describe("IstioRevision resource", Ordered, func() {
 	DescribeTable("reconciles owned resource",
 		func(obj client.Object, modify func(obj client.Object), validate func(g Gomega, obj client.Object)) {
 			By("on update", func() {
-				// ensure all in-flight reconcile operations finish before the test; unfortunately, sleeping seems to be the only option to achieve this
-				time.Sleep(5 * time.Second)
+				// ensure all in-flight reconcile operations finish before the test
+				waitForInFlightReconcileToFinish()
 
 				Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(obj), obj)).To(Succeed())
 
@@ -380,20 +380,18 @@ var _ = Describe("IstioRevision resource", Ordered, func() {
 
 	DescribeTable("skips reconcile when only the status of the owned resource is updated",
 		func(obj client.Object, modify func(obj client.Object)) {
-			// wait for the in-flight reconcile operations to finish
-			// unfortunately, I don't see a good way to do this other than by waiting
-			time.Sleep(5 * time.Second)
+			waitForInFlightReconcileToFinish()
 
 			Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(obj), obj)).To(Succeed())
 
-			beforeCount := getReconcileCount(Default)
+			beforeCount := getIstioRevisionReconcileCount(Default)
 
 			By("modifying object")
 			modify(obj)
 			Expect(k8sClient.Status().Update(ctx, obj)).To(Succeed())
 
 			Consistently(func(g Gomega) {
-				afterCount := getReconcileCount(g)
+				afterCount := getIstioRevisionReconcileCount(g)
 				g.Expect(afterCount).To(Equal(beforeCount))
 			}, 5*time.Second).Should(Succeed())
 		},
@@ -423,6 +421,34 @@ var _ = Describe("IstioRevision resource", Ordered, func() {
 		),
 	)
 
+	It("skips reconcile when a pull secret is added to service account", func() {
+		waitForInFlightReconcileToFinish()
+
+		sa := &corev1.ServiceAccount{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "istiod-" + revName,
+				Namespace: istioNamespace,
+			},
+		}
+		Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(sa), sa)).To(Succeed())
+
+		GinkgoWriter.Println("sa:", sa)
+
+		beforeCount := getIstioRevisionReconcileCount(Default)
+
+		By("adding pull secret to ServiceAccount")
+		sa.ImagePullSecrets = append(sa.ImagePullSecrets, corev1.LocalObjectReference{Name: "other-pull-secret"})
+		Expect(k8sClient.Update(ctx, sa)).To(Succeed())
+
+		Consistently(func(g Gomega) {
+			afterCount := getIstioRevisionReconcileCount(g)
+			g.Expect(afterCount).To(Equal(beforeCount))
+		}, 5*time.Second).Should(Succeed(), "IstioRevision was reconciled when it shouldn't have been")
+
+		Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(sa), sa)).To(Succeed())
+		Expect(sa.ImagePullSecrets).To(ContainElement(corev1.LocalObjectReference{Name: "other-pull-secret"}))
+	})
+
 	It("supports concurrent deployment of two control planes", func() {
 		rev2Name := revName + "2"
 		rev2Key := client.ObjectKey{Name: rev2Name}
@@ -466,7 +492,21 @@ var _ = Describe("IstioRevision resource", Ordered, func() {
 	})
 })
 
-func getReconcileCount(g Gomega) float64 {
+func waitForInFlightReconcileToFinish() {
+	// wait for the in-flight reconcile operations to finish
+	// unfortunately, I don't see a good way to do this other than by waiting
+	time.Sleep(5 * time.Second)
+}
+
+func getIstioRevisionReconcileCount(g Gomega) float64 {
+	return getReconcileCount(g, "istiorevision")
+}
+
+func getIstioCNIReconcileCount(g Gomega) float64 {
+	return getReconcileCount(g, "istiocni")
+}
+
+func getReconcileCount(g Gomega, controllerName string) float64 {
 	resp, err := http.Get("http://localhost:8080/metrics")
 	g.Expect(err).NotTo(HaveOccurred())
 	defer resp.Body.Close()
@@ -480,7 +520,7 @@ func getReconcileCount(g Gomega) float64 {
 	sum := float64(0)
 	for _, metric := range mf.Metric {
 		for _, l := range metric.Label {
-			if *l.Name == "controller" && *l.Value == "istiorevision" {
+			if *l.Name == "controller" && *l.Value == controllerName {
 				sum += metric.GetCounter().GetValue()
 			}
 		}