Merge pull request #835 from Danil-Grigorev/secret-hash-by-default

🌱 Replace CM hash with secret hash

Author: k8s-ci-robot

internal/controller/genericprovider_controller.go

@@ -174,40 +174,8 @@ func (r *GenericProviderReconciler) Reconcile(ctx context.Context, req reconcile
 		}, nil
 	}
 
-	// Check if spec hash stays the same and don't go further in this case.
-	specHash, err := calculateHash(ctx, r.Client, r.Provider)
-	if err != nil {
-		return ctrl.Result{}, err
-	}
-
-	if r.Provider.GetAnnotations()[appliedSpecHashAnnotation] == specHash {
-		log.Info("No changes detected, skipping further steps")
-
-		return ctrl.Result{}, nil
-	}
-
 	res, err := r.reconcile(ctx)
 
-	annotations := r.Provider.GetAnnotations()
-	if annotations == nil {
-		annotations = map[string]string{}
-	}
-
-	// Set the spec hash annotation if reconciliation was successful or reset it otherwise.
-	if res.IsZero() && err == nil {
-		// Recalculate spec hash in case it was changed during reconciliation process.
-		specHash, err := calculateHash(ctx, r.Client, r.Provider)
-		if err != nil {
-			return ctrl.Result{}, err
-		}
-
-		annotations[appliedSpecHashAnnotation] = specHash
-	} else {
-		annotations[appliedSpecHashAnnotation] = ""
-	}
-
-	r.Provider.SetAnnotations(annotations)
-
 	return ctrl.Result{
 		Requeue:      res.Requeue,
 		RequeueAfter: res.RequeueAfter,
@@ -344,14 +312,6 @@ func providerHash(ctx context.Context, client client.Client, hash hash.Hash, pro
 	return nil
 }
 
-func calculateHash(ctx context.Context, k8sClient client.Client, provider genericprovider.GenericProvider) (string, error) {
-	hash := sha256.New()
-
-	err := providerHash(ctx, k8sClient, hash, provider)
-
-	return fmt.Sprintf("%x", hash.Sum(nil)), err
-}
-
 // ApplyFromCache applies provider configuration from cache and returns true if the cache did not change.
 func (p *PhaseReconciler) ApplyFromCache(ctx context.Context) (*Result, error) {
 	log := log.FromContext(ctx)

internal/controller/genericprovider_controller_test.go

@@ -17,7 +17,6 @@ limitations under the License.
 package controller
 
 import (
-	"context"
 	"testing"
 	"time"
 
@@ -745,14 +744,13 @@ func TestProviderConfigSecretChanges(t *testing.T) {
 			g.Expect(env.CreateAndWait(ctx, secret.DeepCopy())).To(Succeed())
 			objs = append(objs, secret)
 
-			initialHash, err := calculateHash(ctx, env.Client, provider)
-			g.Expect(err).ToNot(HaveOccurred())
-
 			t.Log("creating test provider", provider.GetName())
 			g.Expect(env.CreateAndWait(ctx, provider.DeepCopy())).To(Succeed())
 			objs = append(objs, provider)
 
-			g.Eventually(generateExpectedResultChecker(provider, initialHash, corev1.ConditionTrue), timeout).Should(BeEquivalentTo(true))
+			g.Eventually(generateExpectedResultChecker(provider, corev1.ConditionTrue, func(s string) bool { return s != "" }), timeout).Should(BeEquivalentTo(true))
+
+			initialHash := provider.GetAnnotations()[appliedSpecHashAnnotation]
 
 			g.Eventually(func() error {
 				if err := env.Client.Get(ctx, client.ObjectKeyFromObject(secret), secret); err != nil {
@@ -765,52 +763,32 @@ func TestProviderConfigSecretChanges(t *testing.T) {
 				return env.Client.Update(ctx, secret)
 			}).Should(Succeed())
 
-			var updatedDataHash string
-
 			if tc.expectSameHash {
-				g.Eventually(func() string {
-					updatedDataHash, err = calculateHash(ctx, env.Client, provider)
-					g.Expect(err).ToNot(HaveOccurred())
+				g.Consistently(func(g Gomega) {
+					err := env.Client.Get(ctx, client.ObjectKeyFromObject(provider), provider)
+					g.Expect(client.IgnoreNotFound(err)).ToNot(HaveOccurred())
 
-					return updatedDataHash
-				}, 15*time.Second).Should(Equal(initialHash))
+					g.Expect(provider.GetAnnotations()[appliedSpecHashAnnotation]).To(Equal(initialHash))
+				}, 15*time.Second).Should(Succeed())
 			} else {
-				g.Eventually(func() string {
-					updatedDataHash, err = calculateHash(ctx, env.Client, provider)
-					g.Expect(err).ToNot(HaveOccurred())
+				g.Eventually(func(g Gomega) {
+					err := env.Client.Get(ctx, client.ObjectKeyFromObject(provider), provider)
+					g.Expect(client.IgnoreNotFound(err)).ToNot(HaveOccurred())
 
-					return updatedDataHash
-				}, 15*time.Second).ShouldNot(Equal(initialHash))
+					g.Expect(provider.GetAnnotations()[appliedSpecHashAnnotation]).ToNot(Equal(initialHash))
+				}, 15*time.Second).ShouldNot(Succeed())
 			}
-
-			g.Eventually(func() error {
-				if err := env.Client.Get(ctx, client.ObjectKeyFromObject(provider), provider); err != nil {
-					return err
-				}
-
-				// Set a label to ensure that provider was changed
-				labels := provider.GetLabels()
-				if labels == nil {
-					labels = map[string]string{}
-				}
-				labels["my-label"] = "some-value"
-				provider.SetLabels(labels)
-				provider.SetManagedFields(nil)
-
-				return env.Client.Update(ctx, provider)
-			}).Should(Succeed())
-
-			g.Eventually(generateExpectedResultChecker(provider, updatedDataHash, corev1.ConditionTrue), timeout).Should(BeEquivalentTo(true))
 		})
 	}
 }
 
 func TestProviderSpecChanges(t *testing.T) {
 	testCases := []struct {
-		name        string
-		spec        operatorv1.ProviderSpec
-		updatedSpec operatorv1.ProviderSpec
-		expectError bool
+		name             string
+		spec             operatorv1.ProviderSpec
+		updatedSpec      operatorv1.ProviderSpec
+		expectHashChange bool
+		expectError      bool
 	}{
 		{
 			name: "same spec, hash annotation doesn't change",
@@ -860,9 +838,10 @@ func TestProviderSpecChanges(t *testing.T) {
 					},
 				},
 			},
+			expectHashChange: true,
 		},
 		{
-			name:        "upgrade to a non-existent version, hash annotation is empty",
+			name:        "upgrade to a non-existent version, hash annotation unchanged",
 			expectError: true,
 			spec: operatorv1.ProviderSpec{
 				Version: testCurrentVersion,
@@ -906,12 +885,6 @@ func TestProviderSpecChanges(t *testing.T) {
 			updatedProvider := provider.DeepCopy()
 			updatedProvider.SetSpec(tc.updatedSpec)
 
-			specHash, err := calculateHash(context.Background(), env.Client, provider)
-			g.Expect(err).ToNot(HaveOccurred())
-
-			updatedSpecHash, err := calculateHash(context.Background(), env.Client, updatedProvider)
-			g.Expect(err).ToNot(HaveOccurred())
-
 			namespace := "test-provider-spec-changes"
 
 			ns, err := env.CreateNamespace(ctx, namespace)
@@ -929,7 +902,11 @@ func TestProviderSpecChanges(t *testing.T) {
 				g.Expect(env.Cleanup(ctx, provider, dummyConfigMap(namespace))).To(Succeed())
 			}()
 
-			g.Eventually(generateExpectedResultChecker(provider, specHash, corev1.ConditionTrue), timeout).Should(BeEquivalentTo(true))
+			g.Eventually(generateExpectedResultChecker(provider, corev1.ConditionTrue, func(s string) bool {
+				return s != ""
+			}), timeout).Should(BeEquivalentTo(true))
+
+			currentHash := provider.GetAnnotations()[appliedSpecHashAnnotation]
 
 			g.Eventually(func() error {
 				if err := env.Client.Get(ctx, client.ObjectKeyFromObject(provider), provider); err != nil {
@@ -952,22 +929,28 @@ func TestProviderSpecChanges(t *testing.T) {
 			}).Should(Succeed())
 
 			if !tc.expectError {
-				g.Eventually(generateExpectedResultChecker(provider, updatedSpecHash, corev1.ConditionTrue), timeout).Should(BeEquivalentTo(true))
+				g.Eventually(generateExpectedResultChecker(provider, corev1.ConditionTrue, func(s string) bool {
+					if tc.expectHashChange {
+						return s != currentHash
+					}
+
+					return s == currentHash
+				}), timeout).Should(BeEquivalentTo(true))
 			} else {
-				g.Eventually(generateExpectedResultChecker(provider, "", corev1.ConditionFalse), timeout).Should(BeEquivalentTo(true))
+				g.Eventually(generateExpectedResultChecker(provider, corev1.ConditionFalse, func(s string) bool { return s == currentHash }), timeout).Should(BeEquivalentTo(true))
 			}
 		})
 	}
 }
 
-func generateExpectedResultChecker(provider genericprovider.GenericProvider, specHash string, condStatus corev1.ConditionStatus) func() bool {
+func generateExpectedResultChecker(provider genericprovider.GenericProvider, condStatus corev1.ConditionStatus, hashCheck func(string) bool) func() bool {
 	return func() bool {
 		if err := env.Get(ctx, client.ObjectKeyFromObject(provider), provider); err != nil {
 			return false
 		}
 
 		// In case of error we don't want the spec annotation to be updated
-		if provider.GetAnnotations()[appliedSpecHashAnnotation] != specHash {
+		if !hashCheck(provider.GetAnnotations()[appliedSpecHashAnnotation]) {
 			return false
 		}