Merge pull request #189 from external-secrets/fix/add-resource-version

add synced resource version status

apis/externalsecrets/v1alpha1/externalsecret_types.go

@@ -161,6 +161,9 @@ type ExternalSecretStatus struct {
 	// the target secret updated
 	RefreshTime metav1.Time `json:"refreshTime,omitempty"`
 
+	// SyncedResourceVersion keeps track of the last synced version
+	SyncedResourceVersion string `json:"syncedResourceVersion,omitempty"`
+
 	// +optional
 	Conditions []ExternalSecretStatusCondition `json:"conditions,omitempty"`
 }

deploy/crds/external-secrets.io_externalsecrets.yaml

@@ -189,6 +189,10 @@ spec:
                 format: date-time
                 nullable: true
                 type: string
+              syncedResourceVersion:
+                description: SyncedResourceVersion keeps track of the last synced
+                  version
+                type: string
             type: object
         type: object
     served: true

pkg/controllers/externalsecret/externalsecret_controller.go

@@ -16,6 +16,9 @@ package externalsecret
 
 import (
 	"context"
+
+	// nolint
+	"crypto/md5"
 	"fmt"
 	"time"
 
@@ -72,13 +75,20 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
 		return ctrl.Result{}, nil
 	}
 
+	// patch status when done processing
+	p := client.MergeFrom(externalSecret.DeepCopy())
+	defer func() {
+		err = r.Status().Patch(ctx, &externalSecret, p)
+		if err != nil {
+			log.Error(err, "unable to patch status")
+		}
+	}()
+
 	store, err := r.getStore(ctx, &externalSecret)
 	if err != nil {
 		log.Error(err, "could not get store reference")
 		conditionSynced := NewExternalSecretCondition(esv1alpha1.ExternalSecretReady, corev1.ConditionFalse, esv1alpha1.ConditionReasonSecretSyncedError, err.Error())
 		SetExternalSecretCondition(&externalSecret, *conditionSynced)
-
-		err = r.Status().Update(ctx, &externalSecret)
 		syncCallsError.With(syncCallsMetricLabels).Inc()
 		return ctrl.Result{RequeueAfter: requeueAfter}, nil
 	}
@@ -103,10 +113,24 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
 		log.Error(err, "could not get provider client")
 		conditionSynced := NewExternalSecretCondition(esv1alpha1.ExternalSecretReady, corev1.ConditionFalse, esv1alpha1.ConditionReasonSecretSyncedError, err.Error())
 		SetExternalSecretCondition(&externalSecret, *conditionSynced)
-		err = r.Status().Update(ctx, &externalSecret)
 		syncCallsError.With(syncCallsMetricLabels).Inc()
 		return ctrl.Result{RequeueAfter: requeueAfter}, nil
 	}
+
+	refreshInt := time.Hour
+	if externalSecret.Spec.RefreshInterval != nil {
+		refreshInt = externalSecret.Spec.RefreshInterval.Duration
+	}
+
+	// refresh should be skipped if
+	// 1. resource generation hasn't changed
+	// 2. refresh interval is 0
+	// 3. if we're still within refresh-interval
+	if !shouldRefresh(externalSecret) {
+		log.V(1).Info("skipping refresh", "rv", getResourceVersion(externalSecret))
+		return ctrl.Result{RequeueAfter: refreshInt}, nil
+	}
+
 	secret := &corev1.Secret{
 		ObjectMeta: metav1.ObjectMeta{
 			Name:      externalSecret.Spec.Target.Name,
@@ -139,31 +163,19 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
 		log.Error(err, "could not reconcile ExternalSecret")
 		conditionSynced := NewExternalSecretCondition(esv1alpha1.ExternalSecretReady, corev1.ConditionFalse, esv1alpha1.ConditionReasonSecretSyncedError, err.Error())
 		SetExternalSecretCondition(&externalSecret, *conditionSynced)
-		err = r.Status().Update(ctx, &externalSecret)
-		if err != nil {
-			log.Error(err, "unable to update status")
-		}
 		syncCallsError.With(syncCallsMetricLabels).Inc()
 		return ctrl.Result{RequeueAfter: requeueAfter}, nil
 	}
 
-	dur := time.Hour
-	if externalSecret.Spec.RefreshInterval != nil {
-		dur = externalSecret.Spec.RefreshInterval.Duration
-	}
-
 	conditionSynced := NewExternalSecretCondition(esv1alpha1.ExternalSecretReady, corev1.ConditionTrue, esv1alpha1.ConditionReasonSecretSynced, "Secret was synced")
 	SetExternalSecretCondition(&externalSecret, *conditionSynced)
 	externalSecret.Status.RefreshTime = metav1.NewTime(time.Now())
-	err = r.Status().Update(ctx, &externalSecret)
-	if err != nil {
-		log.Error(err, "unable to update status")
-	}
-
+	externalSecret.Status.SyncedResourceVersion = getResourceVersion(externalSecret)
 	syncCallsTotal.With(syncCallsMetricLabels).Inc()
+	log.V(1).Info("reconciled secret")
 
 	return ctrl.Result{
-		RequeueAfter: dur,
+		RequeueAfter: refreshInt,
 	}, nil
 }
 
@@ -175,6 +187,38 @@ func shouldProcessStore(store esv1alpha1.GenericStore, class string) bool {
 	return false
 }
 
+func getResourceVersion(es esv1alpha1.ExternalSecret) string {
+	return fmt.Sprintf("%d-%s", es.ObjectMeta.GetGeneration(), hashMeta(es.ObjectMeta))
+}
+
+func hashMeta(m metav1.ObjectMeta) string {
+	type meta struct {
+		annotations map[string]string
+		labels      map[string]string
+	}
+	h := md5.New() //nolint
+	_, _ = h.Write([]byte(fmt.Sprintf("%v", meta{
+		annotations: m.Annotations,
+		labels:      m.Labels,
+	})))
+	return fmt.Sprintf("%x", h.Sum(nil))
+}
+
+func shouldRefresh(es esv1alpha1.ExternalSecret) bool {
+	// refresh if resource version changed
+	if es.Status.SyncedResourceVersion != getResourceVersion(es) {
+		return true
+	}
+	// skip refresh if refresh interval is 0
+	if es.Spec.RefreshInterval == nil && es.Status.SyncedResourceVersion != "" {
+		return false
+	}
+	if es.Status.RefreshTime.IsZero() {
+		return true
+	}
+	return !es.Status.RefreshTime.Add(es.Spec.RefreshInterval.Duration).After(time.Now())
+}
+
 // we do not want to force-override the label/annotations
 // and only copy the necessary key/value pairs.
 func mergeTemplate(secret *corev1.Secret, externalSecret esv1alpha1.ExternalSecret) {

pkg/controllers/externalsecret/externalsecret_controller_test.go

@@ -37,7 +37,7 @@ import (
 var (
 	fakeProvider *fake.Client
 	metric       dto.Metric
-	timeout      = time.Second * 30
+	timeout      = time.Second * 10
 	interval     = time.Millisecond * 250
 )
 
@@ -162,7 +162,7 @@ var _ = Describe("ExternalSecret controller", func() {
 			Expect(externalSecretConditionShouldBe(ExternalSecretName, ExternalSecretNamespace, esv1alpha1.ExternalSecretReady, v1.ConditionTrue, 1.0)).To(BeTrue())
 			Eventually(func() bool {
 				Expect(syncCallsTotal.WithLabelValues(ExternalSecretName, ExternalSecretNamespace).Write(&metric)).To(Succeed())
-				return metric.GetCounter().GetValue() >= 2.0
+				return metric.GetCounter().GetValue() == 1.0
 			}, timeout, interval).Should(BeTrue())
 
 			// check value
@@ -208,7 +208,7 @@ var _ = Describe("ExternalSecret controller", func() {
 			Expect(externalSecretConditionShouldBe(ExternalSecretName, ExternalSecretNamespace, esv1alpha1.ExternalSecretReady, v1.ConditionTrue, 1.0)).To(BeTrue())
 			Eventually(func() bool {
 				Expect(syncCallsTotal.WithLabelValues(ExternalSecretName, ExternalSecretNamespace).Write(&metric)).To(Succeed())
-				return metric.GetCounter().GetValue() >= 2.0
+				return metric.GetCounter().GetValue() == 1.0
 			}, timeout, interval).Should(BeTrue())
 
 			// check values
@@ -330,7 +330,7 @@ var _ = Describe("ExternalSecret controller", func() {
 			Expect(externalSecretConditionShouldBe(ExternalSecretName, ExternalSecretNamespace, esv1alpha1.ExternalSecretReady, v1.ConditionTrue, 1.0)).To(BeTrue())
 			Eventually(func() bool {
 				Expect(syncCallsTotal.WithLabelValues(ExternalSecretName, ExternalSecretNamespace).Write(&metric)).To(Succeed())
-				return metric.GetCounter().GetValue() >= 2.0
+				return metric.GetCounter().GetValue() == 1.0
 			}, timeout, interval).Should(BeTrue())
 
 			// check values
@@ -373,7 +373,7 @@ var _ = Describe("ExternalSecret controller", func() {
 			Expect(externalSecretConditionShouldBe(ExternalSecretName, ExternalSecretNamespace, esv1alpha1.ExternalSecretReady, v1.ConditionTrue, 1.0)).To(BeTrue())
 			Eventually(func() bool {
 				Expect(syncCallsTotal.WithLabelValues(ExternalSecretName, ExternalSecretNamespace).Write(&metric)).To(Succeed())
-				return metric.GetCounter().GetValue() >= 2.0
+				return metric.GetCounter().GetValue() == 1.0
 			}, timeout, interval).Should(BeTrue())
 
 			// check values
@@ -539,6 +539,182 @@ var _ = Describe("ExternalSecret controller", func() {
 	)
 })
 
+var _ = Describe("ExternalSecret refresh logic", func() {
+	Context("secret refresh", func() {
+		It("should refresh when resource version does not match", func() {
+			Expect(shouldRefresh(esv1alpha1.ExternalSecret{
+				Status: esv1alpha1.ExternalSecretStatus{
+					SyncedResourceVersion: "some resource version",
+				},
+			})).To(BeTrue())
+		})
+
+		It("should refresh when labels change", func() {
+			es := esv1alpha1.ExternalSecret{
+				ObjectMeta: metav1.ObjectMeta{
+					Generation: 1,
+					Labels: map[string]string{
+						"foo": "bar",
+					},
+				},
+				Status: esv1alpha1.ExternalSecretStatus{},
+			}
+			es.Status.SyncedResourceVersion = getResourceVersion(es)
+			// this should not refresh, rv matches object
+			Expect(shouldRefresh(es)).To(BeFalse())
+
+			// change labels without changing the syncedResourceVersion and expect refresh
+			es.ObjectMeta.Labels["new"] = "w00t"
+			Expect(shouldRefresh(es)).To(BeTrue())
+		})
+
+		It("should refresh when annotations change", func() {
+			es := esv1alpha1.ExternalSecret{
+				ObjectMeta: metav1.ObjectMeta{
+					Generation: 1,
+					Annotations: map[string]string{
+						"foo": "bar",
+					},
+				},
+				Status: esv1alpha1.ExternalSecretStatus{},
+			}
+			es.Status.SyncedResourceVersion = getResourceVersion(es)
+			// this should not refresh, rv matches object
+			Expect(shouldRefresh(es)).To(BeFalse())
+
+			// change annotations without changing the syncedResourceVersion and expect refresh
+			es.ObjectMeta.Annotations["new"] = "w00t"
+			Expect(shouldRefresh(es)).To(BeTrue())
+		})
+
+		It("should refresh when generation has changed", func() {
+			es := esv1alpha1.ExternalSecret{
+				ObjectMeta: metav1.ObjectMeta{
+					Generation: 1,
+				},
+				Status: esv1alpha1.ExternalSecretStatus{},
+			}
+			es.Status.SyncedResourceVersion = getResourceVersion(es)
+			Expect(shouldRefresh(es)).To(BeFalse())
+
+			// update gen -> refresh
+			es.ObjectMeta.Generation = 2
+			Expect(shouldRefresh(es)).To(BeTrue())
+		})
+
+		It("should skip refresh when refreshInterval is nil", func() {
+			es := esv1alpha1.ExternalSecret{
+				ObjectMeta: metav1.ObjectMeta{
+					Generation: 1,
+				},
+				Spec: esv1alpha1.ExternalSecretSpec{
+					RefreshInterval: nil,
+				},
+				Status: esv1alpha1.ExternalSecretStatus{},
+			}
+			// resource version matches
+			es.Status.SyncedResourceVersion = getResourceVersion(es)
+			Expect(shouldRefresh(es)).To(BeFalse())
+		})
+
+		It("should refresh when refresh interval has passed", func() {
+			es := esv1alpha1.ExternalSecret{
+				ObjectMeta: metav1.ObjectMeta{
+					Generation: 1,
+				},
+				Spec: esv1alpha1.ExternalSecretSpec{
+					RefreshInterval: &metav1.Duration{Duration: time.Second},
+				},
+				Status: esv1alpha1.ExternalSecretStatus{
+					RefreshTime: metav1.NewTime(metav1.Now().Add(-time.Second * 5)),
+				},
+			}
+			// resource version matches
+			es.Status.SyncedResourceVersion = getResourceVersion(es)
+			Expect(shouldRefresh(es)).To(BeTrue())
+		})
+
+		It("should refresh when no refresh time was set", func() {
+			es := esv1alpha1.ExternalSecret{
+				ObjectMeta: metav1.ObjectMeta{
+					Generation: 1,
+				},
+				Spec: esv1alpha1.ExternalSecretSpec{
+					RefreshInterval: &metav1.Duration{Duration: time.Second},
+				},
+				Status: esv1alpha1.ExternalSecretStatus{},
+			}
+			// resource version matches
+			es.Status.SyncedResourceVersion = getResourceVersion(es)
+			Expect(shouldRefresh(es)).To(BeTrue())
+		})
+
+	})
+	Context("objectmeta hash", func() {
+		It("should produce different hashes for different k/v pairs", func() {
+			h1 := hashMeta(metav1.ObjectMeta{
+				Generation: 1,
+				Annotations: map[string]string{
+					"foo": "bar",
+				},
+			})
+			h2 := hashMeta(metav1.ObjectMeta{
+				Generation: 1,
+				Annotations: map[string]string{
+					"foo": "bing",
+				},
+			})
+			Expect(h1).ToNot(Equal(h2))
+		})
+
+		It("should produce different hashes for different generations but same label/annotations", func() {
+			h1 := hashMeta(metav1.ObjectMeta{
+				Generation: 1,
+				Annotations: map[string]string{
+					"foo": "bar",
+				},
+				Labels: map[string]string{
+					"foo": "bar",
+				},
+			})
+			h2 := hashMeta(metav1.ObjectMeta{
+				Generation: 2,
+				Annotations: map[string]string{
+					"foo": "bar",
+				},
+				Labels: map[string]string{
+					"foo": "bar",
+				},
+			})
+			Expect(h1).To(Equal(h2))
+		})
+
+		It("should produce the same hash for the same k/v pairs", func() {
+			h1 := hashMeta(metav1.ObjectMeta{
+				Generation: 1,
+			})
+			h2 := hashMeta(metav1.ObjectMeta{
+				Generation: 1,
+			})
+			Expect(h1).To(Equal(h2))
+
+			h1 = hashMeta(metav1.ObjectMeta{
+				Generation: 1,
+				Annotations: map[string]string{
+					"foo": "bar",
+				},
+			})
+			h2 = hashMeta(metav1.ObjectMeta{
+				Generation: 1,
+				Annotations: map[string]string{
+					"foo": "bar",
+				},
+			})
+			Expect(h1).To(Equal(h2))
+		})
+	})
+})
+
 // CreateNamespace creates a new namespace in the cluster.
 func CreateNamespace(baseName string, c client.Client) (string, error) {
 	genName := fmt.Sprintf("ctrl-test-%v", baseName)