avoid unnecessary updates to CRDs and webhooks (#4561)

* avoid unnecessary updates to custom resource definitions

Signed-off-by: Steven Davidovitz <sdavidovitz@groq.com>

* avoid unnecessary updates to webhook configuration

Signed-off-by: Steven Davidovitz <sdavidovitz@groq.com>

---------

Signed-off-by: Steven Davidovitz <sdavidovitz@groq.com>
Co-authored-by: Gustavo Fernandes de Carvalho <17139678+gusfcarvalho@users.noreply.github.com>
Co-authored-by: Gergely Brautigam <182850+Skarlso@users.noreply.github.com>

pkg/controllers/crds/crds_controller.go

@@ -36,6 +36,7 @@ import (
 	"github.com/go-logr/logr"
 	corev1 "k8s.io/api/core/v1"
 	apiext "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
+	"k8s.io/apimachinery/pkg/api/equality"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/client-go/tools/record"
@@ -119,7 +120,7 @@ type CertInfo struct {
 func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
 	log := r.Log.WithValues("CustomResourceDefinition", req.NamespacedName)
 	if slices.Contains(r.CrdResources, req.NamespacedName.Name) {
-		err := r.updateCRD(ctx, req)
+		err := r.updateCRD(logr.NewContext(ctx, log), req)
 		if err != nil {
 			log.Error(err, "failed to inject conversion webhook")
 			r.readyStatusMapMu.Lock()
@@ -192,6 +193,7 @@ func (r *Reconciler) SetupWithManager(mgr ctrl.Manager, opts controller.Options)
 }
 
 func (r *Reconciler) updateCRD(ctx context.Context, req ctrl.Request) error {
+	log := logr.FromContextOrDiscard(ctx)
 	secret := corev1.Secret{}
 	secretName := types.NamespacedName{
 		Name:      r.SecretName,
@@ -205,6 +207,7 @@ func (r *Reconciler) updateCRD(ctx context.Context, req ctrl.Request) error {
 	if err := r.Get(ctx, req.NamespacedName, &updatedResource); err != nil {
 		return err
 	}
+	before := updatedResource.DeepCopyObject()
 
 	svc := types.NamespacedName{
 		Name:      r.SvcName,
@@ -227,7 +230,15 @@ func (r *Reconciler) updateCRD(ctx context.Context, req ctrl.Request) error {
 			return err
 		}
 	}
-	return r.Update(ctx, &updatedResource)
+	if !equality.Semantic.DeepEqual(before, &updatedResource) {
+		if err := r.Update(ctx, &updatedResource); err != nil {
+			return err
+		}
+		log.Info("updated crd")
+		return nil
+	}
+	log.V(1).Info("crd is unchanged")
+	return nil
 }
 
 func injectService(crd *apiext.CustomResourceDefinition, svc types.NamespacedName) error {

pkg/controllers/crds/crds_controller_test.go

@@ -95,21 +95,61 @@ func newCRD() apiextensionsv1.CustomResourceDefinition {
 }
 
 func TestUpdateCRD(t *testing.T) {
-	rec := newReconciler()
 	svc := newService()
 	secret := newSecret()
 	crd := newCRD()
 	c := client.NewClientBuilder().WithObjects(&svc, &secret, &crd).Build()
+
+	rec := newReconciler()
 	rec.Client = c
+
 	ctx := context.Background()
 	req := ctrl.Request{
 		NamespacedName: types.NamespacedName{
 			Name: "one",
 		},
 	}
+
 	err := rec.updateCRD(ctx, req)
 	if err != nil {
-		t.Errorf("Failed updating CRD: %v", err)
+		t.Fatalf("Failed updating CRD: %v", err)
+	}
+
+	var updatedCRD apiextensionsv1.CustomResourceDefinition
+	var updatedSecret corev1.Secret
+
+	err = c.Get(ctx, req.NamespacedName, &updatedCRD)
+	if err != nil {
+		t.Fatalf("Failed getting updated CRD: %v", err)
+	}
+
+	err = c.Get(ctx, types.NamespacedName{Name: secret.Name, Namespace: secret.Namespace}, &updatedSecret)
+	if err != nil {
+		t.Fatalf("Failed getting updated secret: %v", err)
+	}
+
+	if updatedCRD.Spec.Conversion.Webhook.ClientConfig.Service.Name != svc.Name {
+		t.Fatalf("Failed updating CRD webhook service name: expected %v, got %v", svc.Name, updatedCRD.Spec.Conversion.Webhook.ClientConfig.Service.Name)
+	}
+
+	if !bytes.Equal(updatedSecret.Data[caCertName], updatedCRD.Spec.Conversion.Webhook.ClientConfig.CABundle) {
+		t.Fatalf("Failed updating CRD webhook ca bundle: expected %v, got %v", string(updatedSecret.Data[caCertName]), string(updatedCRD.Spec.Conversion.Webhook.ClientConfig.CABundle))
+	}
+
+	err = rec.updateCRD(ctx, req)
+	if err != nil {
+		t.Fatalf("Failed updating CRD: %v", err)
+	}
+
+	resourceVersion := updatedCRD.ResourceVersion
+
+	err = c.Get(ctx, req.NamespacedName, &updatedCRD)
+	if err != nil {
+		t.Fatalf("Failed getting updated CRD: %v", err)
+	}
+
+	if updatedCRD.ResourceVersion != resourceVersion {
+		t.Errorf("expected no change in resource version: %v, got %v", resourceVersion, updatedCRD.ResourceVersion)
 	}
 }
 

pkg/controllers/webhookconfig/webhookconfig.go

@@ -26,6 +26,7 @@ import (
 	"github.com/go-logr/logr"
 	admissionregistration "k8s.io/api/admissionregistration/v1"
 	v1 "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/api/equality"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/types"
@@ -109,7 +110,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
 	}
 
 	log.Info("updating webhook config")
-	err = r.updateConfig(ctx, &cfg)
+	err = r.updateConfig(logr.NewContext(ctx, log), &cfg)
 	if err != nil {
 		log.Error(err, "could not update webhook config")
 		r.recorder.Eventf(&cfg, v1.EventTypeWarning, ReasonUpdateFailed, err.Error())
@@ -117,7 +118,6 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
 			RequeueAfter: time.Minute,
 		}, err
 	}
-	log.Info("updated webhook config")
 
 	// right now we only have one single
 	// webhook config we care about
@@ -172,6 +172,9 @@ func (r *Reconciler) ReadyCheck(_ *http.Request) error {
 
 // reads the ca cert and updates the webhook config.
 func (r *Reconciler) updateConfig(ctx context.Context, cfg *admissionregistration.ValidatingWebhookConfiguration) error {
+	log := logr.FromContextOrDiscard(ctx)
+	before := cfg.DeepCopyObject()
+
 	secret := v1.Secret{}
 	secretName := types.NamespacedName{
 		Name:      r.SecretName,
@@ -186,13 +189,21 @@ func (r *Reconciler) updateConfig(ctx context.Context, cfg *admissionregistratio
 	if !ok {
 		return errors.New(errCACertNotReady)
 	}
-	if err := r.inject(cfg, r.SvcName, r.SvcNamespace, crt); err != nil {
-		return err
+
+	r.inject(cfg, r.SvcName, r.SvcNamespace, crt)
+
+	if !equality.Semantic.DeepEqual(before, cfg) {
+		if err := r.Update(ctx, cfg); err != nil {
+			return err
+		}
+		log.Info("updated webhook config")
+		return nil
 	}
-	return r.Update(ctx, cfg)
+	log.V(1).Info("webhook config unchanged")
+	return nil
 }
 
-func (r *Reconciler) inject(cfg *admissionregistration.ValidatingWebhookConfiguration, svcName, svcNamespace string, certData []byte) error {
+func (r *Reconciler) inject(cfg *admissionregistration.ValidatingWebhookConfiguration, svcName, svcNamespace string, certData []byte) {
 	r.Log.Info("injecting ca certificate and service names", "cacrt", base64.StdEncoding.EncodeToString(certData), "name", cfg.Name)
 	for idx, w := range cfg.Webhooks {
 		if !strings.HasSuffix(w.Name, "external-secrets.io") {
@@ -204,5 +215,4 @@ func (r *Reconciler) inject(cfg *admissionregistration.ValidatingWebhookConfigur
 		cfg.Webhooks[idx].ClientConfig.Service.Namespace = svcNamespace
 		cfg.Webhooks[idx].ClientConfig.CABundle = certData
 	}
-	return nil
 }

pkg/controllers/webhookconfig/webhookconfig_test.go

@@ -170,7 +170,7 @@ var _ = Describe("ValidatingWebhookConfig reconcile", Ordered, func() {
 			Expect(err).ToNot(HaveOccurred())
 		}()
 		tc.assert = func() {
-
+			var resourceVersion string
 			Eventually(func() bool {
 				var vwc admissionregistration.ValidatingWebhookConfiguration
 				err := k8sClient.Get(context.Background(), types.NamespacedName{
@@ -193,11 +193,26 @@ var _ = Describe("ValidatingWebhookConfig reconcile", Ordered, func() {
 						return false
 					}
 				}
+				resourceVersion = vwc.ResourceVersion
 				return true
 			}).
 				WithTimeout(time.Second * 10).
 				WithPolling(time.Second).
 				Should(BeTrue())
+
+			// make sure no additional updates are made
+			Consistently(func() bool {
+				var vwc admissionregistration.ValidatingWebhookConfiguration
+				err := k8sClient.Get(context.Background(), types.NamespacedName{Name: tc.vwc.Name}, &vwc)
+				if err != nil {
+					return false
+				}
+				return vwc.ResourceVersion == resourceVersion
+			}).
+				WithTimeout(time.Second * 10).
+				WithPolling(time.Second).
+				Should(BeTrue())
+
 		}
 	}