Merge pull request #168 from Fedosin/applied_spec_hash

🐛 Update provider components if (and only if) its spec has been changed

Author: k8s-ci-robot

internal/controller/genericprovider_controller.go

@@ -18,6 +18,8 @@ package controller
 
 import (
 	"context"
+	"crypto/sha256"
+	"encoding/json"
 	"errors"
 	"fmt"
 	"reflect"
@@ -44,6 +46,10 @@ type GenericProviderReconciler struct {
 	Config       *rest.Config
 }
 
+const (
+	appliedSpecHashAnnotation = "operator.cluster.x-k8s.io/applied-spec-hash"
+)
+
 func (r *GenericProviderReconciler) SetupWithManager(mgr ctrl.Manager, options controller.Options) error {
 	return ctrl.NewControllerManagedBy(mgr).
 		For(r.Provider).
@@ -106,7 +112,35 @@ func (r *GenericProviderReconciler) Reconcile(ctx context.Context, req reconcile
 		return r.reconcileDelete(ctx, typedProvider)
 	}
 
-	return r.reconcile(ctx, typedProvider, typedProviderList)
+	// Check if spec hash stays the same and don't go further in this case.
+	specHash, err := calculateHash(typedProvider.GetSpec())
+	if err != nil {
+		return ctrl.Result{}, err
+	}
+
+	if typedProvider.GetAnnotations()[appliedSpecHashAnnotation] == specHash {
+		log.Info("No changes detected, skipping further steps")
+
+		return ctrl.Result{}, nil
+	}
+
+	res, err := r.reconcile(ctx, typedProvider, typedProviderList)
+
+	annotations := typedProvider.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 {
+		annotations[appliedSpecHashAnnotation] = specHash
+	} else {
+		annotations[appliedSpecHashAnnotation] = ""
+	}
+
+	typedProvider.SetAnnotations(annotations)
+
+	return res, err
 }
 
 func patchProvider(ctx context.Context, provider genericprovider.GenericProvider, patchHelper *patch.Helper, options ...patch.Option) error {
@@ -227,3 +261,18 @@ func (r *GenericProviderReconciler) newGenericProviderList() (genericprovider.Ge
 		return nil, failedToCastInterfaceErr
 	}
 }
+
+func calculateHash(object interface{}) (string, error) {
+	jsonData, err := json.Marshal(object)
+	if err != nil {
+		return "", fmt.Errorf("cannot parse provider spec: %w", err)
+	}
+
+	hash := sha256.New()
+
+	if _, err = hash.Write(jsonData); err != nil {
+		return "", fmt.Errorf("cannot calculate provider spec hash: %w", err)
+	}
+
+	return fmt.Sprintf("%x", hash.Sum(nil)), nil
+}

internal/controller/genericprovider_controller_test.go

@@ -25,6 +25,7 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	utilruntime "k8s.io/apimachinery/pkg/util/runtime"
+	"k8s.io/utils/pointer"
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
 	clusterctlv1 "sigs.k8s.io/cluster-api/cmd/clusterctl/api/v1alpha3"
 	"sigs.k8s.io/controller-runtime/pkg/client"
@@ -462,6 +463,181 @@ func TestNewGenericProviderList(t *testing.T) {
 	}
 }
 
+func TestProviderSpecChanges(t *testing.T) {
+	testCases := []struct {
+		name        string
+		spec        operatorv1.ProviderSpec
+		updatedSpec operatorv1.ProviderSpec
+		expectError bool
+	}{
+		{
+			name: "same spec, hash annotation doesn't change",
+			spec: operatorv1.ProviderSpec{
+				Version: testCurrentVersion,
+				FetchConfig: &operatorv1.FetchConfiguration{
+					Selector: &metav1.LabelSelector{
+						MatchLabels: map[string]string{
+							"test": "dummy-config",
+						},
+					},
+				},
+			},
+			updatedSpec: operatorv1.ProviderSpec{
+				Version: testCurrentVersion,
+				FetchConfig: &operatorv1.FetchConfiguration{
+					Selector: &metav1.LabelSelector{
+						MatchLabels: map[string]string{
+							"test": "dummy-config",
+						},
+					},
+				},
+			},
+		},
+		{
+			name: "add more replicas, hash annotation is updated",
+			spec: operatorv1.ProviderSpec{
+				Version: testCurrentVersion,
+				FetchConfig: &operatorv1.FetchConfiguration{
+					Selector: &metav1.LabelSelector{
+						MatchLabels: map[string]string{
+							"test": "dummy-config",
+						},
+					},
+				},
+			},
+			updatedSpec: operatorv1.ProviderSpec{
+				Version: testCurrentVersion,
+				Deployment: &operatorv1.DeploymentSpec{
+					Replicas: pointer.Int(2),
+				},
+				FetchConfig: &operatorv1.FetchConfiguration{
+					Selector: &metav1.LabelSelector{
+						MatchLabels: map[string]string{
+							"test": "dummy-config",
+						},
+					},
+				},
+			},
+		},
+		{
+			name:        "upgrade to a non-existent version, hash annotation is empty",
+			expectError: true,
+			spec: operatorv1.ProviderSpec{
+				Version: testCurrentVersion,
+				FetchConfig: &operatorv1.FetchConfiguration{
+					Selector: &metav1.LabelSelector{
+						MatchLabels: map[string]string{
+							"test": "dummy-config",
+						},
+					},
+				},
+			},
+			updatedSpec: operatorv1.ProviderSpec{
+				Version: "10000.0.0-NONEXISTENT",
+				Deployment: &operatorv1.DeploymentSpec{
+					Replicas: pointer.Int(2),
+				},
+				FetchConfig: &operatorv1.FetchConfiguration{
+					Selector: &metav1.LabelSelector{
+						MatchLabels: map[string]string{
+							"test": "dummy-config",
+						},
+					},
+				},
+			},
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			g := NewWithT(t)
+
+			specHash, err := calculateHash(tc.spec)
+			g.Expect(err).ToNot(HaveOccurred())
+
+			updatedSpecHash, err := calculateHash(tc.spec)
+			g.Expect(err).ToNot(HaveOccurred())
+
+			provider := &genericprovider.CoreProviderWrapper{
+				CoreProvider: &operatorv1.CoreProvider{
+					ObjectMeta: metav1.ObjectMeta{
+						Name: "cluster-api",
+					},
+					Spec: operatorv1.CoreProviderSpec{
+						ProviderSpec: tc.spec,
+					},
+				},
+			}
+
+			namespace := "test-provider-spec-changes"
+
+			t.Log("Ensure namespace exists", namespace)
+			g.Expect(env.EnsureNamespaceExists(ctx, namespace)).To(Succeed())
+
+			g.Expect(env.CreateAndWait(ctx, dummyConfigMap(namespace, testCurrentVersion))).To(Succeed())
+
+			provider.SetNamespace(namespace)
+			t.Log("creating test provider", provider.GetName())
+			g.Expect(env.CreateAndWait(ctx, provider.GetObject())).To(Succeed())
+
+			g.Eventually(generateExpectedResultChecker(provider, specHash, corev1.ConditionTrue), timeout).Should(BeEquivalentTo(true))
+
+			// Change provider spec
+			provider.SetSpec(tc.updatedSpec)
+
+			// 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)
+
+			g.Expect(env.Client.Update(ctx, provider.GetObject())).To(Succeed())
+
+			if !tc.expectError {
+				g.Eventually(generateExpectedResultChecker(provider, updatedSpecHash, corev1.ConditionTrue), timeout).Should(BeEquivalentTo(true))
+			} else {
+				g.Eventually(generateExpectedResultChecker(provider, "", corev1.ConditionFalse), timeout).Should(BeEquivalentTo(true))
+			}
+
+			// Clean up
+			objs := []client.Object{provider.GetObject()}
+			objs = append(objs, &corev1.ConfigMap{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      testCurrentVersion,
+					Namespace: namespace,
+				},
+			})
+
+			g.Expect(env.CleanupAndWait(ctx, objs...)).To(Succeed())
+		})
+	}
+}
+
+func generateExpectedResultChecker(provider *genericprovider.CoreProviderWrapper, specHash string, condStatus corev1.ConditionStatus) func() bool {
+	return func() bool {
+		if err := env.Get(ctx, client.ObjectKeyFromObject(provider.GetObject()), provider.GetObject()); err != nil {
+			return false
+		}
+
+		// In case of error we don't want the spec annotation to be updated
+		if provider.GetAnnotations()[appliedSpecHashAnnotation] != specHash {
+			return false
+		}
+
+		for _, cond := range provider.GetStatus().Conditions {
+			if cond.Type == operatorv1.ProviderInstalledCondition {
+				if cond.Status == condStatus {
+					return true
+				}
+			}
+		}
+
+		return false
+	}
+}
+
 func setupScheme() *runtime.Scheme {
 	scheme := runtime.NewScheme()
 	utilruntime.Must(corev1.AddToScheme(scheme))

internal/controller/phases.go

@@ -353,59 +353,20 @@ func (p *phaseReconciler) fetch(ctx context.Context) (reconcile.Result, error) {
 func (p *phaseReconciler) preInstall(ctx context.Context) (reconcile.Result, error) {
 	log := ctrl.LoggerFrom(ctx)
 
-	needPreDelete, err := p.versionChanged()
-	if err != nil {
-		return reconcile.Result{}, wrapPhaseError(err, "failed getting clusterctl Provider version")
-	}
-
-	// we need to delete existing components only if their version changes and something has already been installed.
-	if !needPreDelete || p.provider.GetStatus().InstalledVersion == nil {
+	// Nothing to do if it's a fresh installation.
+	if p.provider.GetStatus().InstalledVersion == nil {
 		return reconcile.Result{}, nil
 	}
 
-	log.Info("Upgrade detected, deleting existing components")
+	log.Info("Changes detected, deleting existing components")
 
 	return p.delete(ctx)
 }
 
-// versionChanged try to get installed version from provider status and decide if it has changed.
-func (p *phaseReconciler) versionChanged() (bool, error) {
-	installedVersion := p.provider.GetStatus().InstalledVersion
-	if installedVersion == nil {
-		return true, nil
-	}
-
-	currentVersion, err := versionutil.ParseSemantic(*installedVersion)
-	if err != nil {
-		return false, err
-	}
-
-	res, err := currentVersion.Compare(p.components.Version())
-	if err != nil {
-		return false, err
-	}
-
-	// we need to delete installed components if versions are different
-	versionChanged := res != 0
-
-	return versionChanged, nil
-}
-
 // install installs the provider components using clusterctl library.
 func (p *phaseReconciler) install(ctx context.Context) (reconcile.Result, error) {
 	log := ctrl.LoggerFrom(ctx)
 
-	versionChanged, err := p.versionChanged()
-	if err != nil {
-		return reconcile.Result{}, wrapPhaseError(err, "failed getting clusterctl Provider version")
-	}
-
-	// skip installation if the version hasn't changed.
-	if !versionChanged {
-		log.Info("Provider already installed")
-		return reconcile.Result{}, nil
-	}
-
 	clusterClient := p.newClusterClient()
 
 	log.Info("Installing provider")