chore: relocate version change validation to preflight check

Previously, the check was executed at the end of reconciliation and
in multiple instances. By moving it to the beginning, the process is
halted when installed and desired versions match, streamlining the
reconciliation workflow.

Author: Fedosin

internal/controller/genericprovider_controller.go

@@ -108,12 +108,17 @@ func (r *GenericProviderReconciler) Reconcile(ctx context.Context, req reconcile
 	}
 
 	// Run preflight checks to ensure that we can proceed with given specification
-	res, err := preflightchecks.PreflightChecks(ctx, r.Client, typedProvider, typedProviderList)
+	res, halt, err := preflightchecks.PreflightChecks(ctx, r.Client, typedProvider, typedProviderList)
 	if !res.IsZero() || err != nil {
-		// the steps are sequential, so we must be complete before progressing.
 		return res, err
 	}
 
+	if halt {
+		log.Info("No changes detected, skipping further steps")
+
+		return res, nil
+	}
+
 	return r.reconcile(ctx, typedProvider, typedProviderList)
 }
 

internal/controller/phases.go

@@ -345,59 +345,15 @@ 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 {
-		return reconcile.Result{}, nil
-	}
-
 	log.Info("Upgrade 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")

internal/controller/preflightchecks/preflight_checks.go

@@ -54,7 +54,7 @@ var (
 )
 
 // PreflightChecks performs preflight checks before installing provider.
-func PreflightChecks(ctx context.Context, c client.Client, provider genericprovider.GenericProvider, providerList genericprovider.GenericProviderList) (ctrl.Result, error) {
+func PreflightChecks(ctx context.Context, c client.Client, provider genericprovider.GenericProvider, providerList genericprovider.GenericProviderList) (ctrl.Result, bool, error) {
 	log := ctrl.LoggerFrom(ctx)
 
 	log.Info("Performing preflight checks")
@@ -71,7 +71,7 @@ func PreflightChecks(ctx context.Context, c client.Client, provider genericprovi
 			emptyVersionMessage,
 		))
 
-		return ctrl.Result{}, fmt.Errorf("version can't be empty for provider %s", provider.GetName())
+		return ctrl.Result{}, true, fmt.Errorf("version can't be empty for provider %s", provider.GetName())
 	}
 
 	// Check that provider version contains a valid value.
@@ -84,13 +84,23 @@ func PreflightChecks(ctx context.Context, c client.Client, provider genericprovi
 			err.Error(),
 		))
 
-		return ctrl.Result{}, fmt.Errorf("version contains invalid value for provider %s", provider.GetName())
+		return ctrl.Result{}, true, fmt.Errorf("version contains invalid value for provider %s", provider.GetName())
+	}
+
+	// If desired and installed versions are the same - halt reconciliation
+	changed, err := versionChanged(provider)
+	if err != nil {
+		return ctrl.Result{}, true, fmt.Errorf("failed to parse installed version: %w", err)
+	}
+
+	if !changed {
+		return ctrl.Result{}, true, nil
 	}
 
 	// Check that if a predefined provider is being installed, and if it's not - ensure that FetchConfig is specified.
 	isPredefinedProvider, err := isPredefinedProvider(provider.GetName(), util.ClusterctlProviderType(provider))
 	if err != nil {
-		return ctrl.Result{}, fmt.Errorf("failed to generate a list of predefined providers: %w", err)
+		return ctrl.Result{}, true, fmt.Errorf("failed to generate a list of predefined providers: %w", err)
 	}
 
 	if !isPredefinedProvider {
@@ -102,7 +112,7 @@ func PreflightChecks(ctx context.Context, c client.Client, provider genericprovi
 				"Either Selector or URL must be provided for a not predefined provider",
 			))
 
-			return ctrl.Result{}, fmt.Errorf("either selector or URL must be provided for a not predefined provider %s", provider.GetName())
+			return ctrl.Result{}, true, fmt.Errorf("either selector or URL must be provided for a not predefined provider %s", provider.GetName())
 		}
 	}
 
@@ -115,7 +125,7 @@ func PreflightChecks(ctx context.Context, c client.Client, provider genericprovi
 			"Only one of Selector and URL must be provided, not both",
 		))
 
-		return ctrl.Result{}, fmt.Errorf("only one of Selector and URL must be provided for provider %s", provider.GetName())
+		return ctrl.Result{}, true, fmt.Errorf("only one of Selector and URL must be provided for provider %s", provider.GetName())
 	}
 
 	// Validate that provided github token works and has repository access.
@@ -124,7 +134,7 @@ func PreflightChecks(ctx context.Context, c client.Client, provider genericprovi
 		key := types.NamespacedName{Namespace: provider.GetNamespace(), Name: provider.GetSpec().SecretName}
 
 		if err := c.Get(ctx, key, secret); err != nil {
-			return ctrl.Result{}, fmt.Errorf("failed to get providers secret: %w", err)
+			return ctrl.Result{}, true, fmt.Errorf("failed to get providers secret: %w", err)
 		}
 
 		if token, ok := secret.Data[configclient.GitHubTokenVariable]; ok {
@@ -139,13 +149,13 @@ func PreflightChecks(ctx context.Context, c client.Client, provider genericprovi
 					invalidGithubTokenMessage,
 				))
 
-				return ctrl.Result{}, fmt.Errorf("failed to validate provided github token: %w", err)
+				return ctrl.Result{}, true, fmt.Errorf("failed to validate provided github token: %w", err)
 			}
 		}
 	}
 
 	if err := c.List(ctx, providerList.GetObject()); err != nil {
-		return ctrl.Result{}, fmt.Errorf("failed to list providers: %w", err)
+		return ctrl.Result{}, true, fmt.Errorf("failed to list providers: %w", err)
 	}
 
 	// Check that no more than one instance of the provider is installed.
@@ -168,7 +178,7 @@ func PreflightChecks(ctx context.Context, c client.Client, provider genericprovi
 			preflightFalseCondition.Message = moreThanOneCoreProviderInstanceExistsMessage
 			conditions.Set(provider, preflightFalseCondition)
 
-			return ctrl.Result{}, fmt.Errorf("only one instance of CoreProvider is allowed")
+			return ctrl.Result{}, true, fmt.Errorf("only one instance of CoreProvider is allowed")
 		}
 
 		// For any other provider we should check that instances with similar name exist in any namespace
@@ -177,15 +187,15 @@ func PreflightChecks(ctx context.Context, c client.Client, provider genericprovi
 			log.Info(preflightFalseCondition.Message)
 			conditions.Set(provider, preflightFalseCondition)
 
-			return ctrl.Result{}, fmt.Errorf("only one %s provider is allowed in the cluster", p.GetName())
+			return ctrl.Result{}, true, fmt.Errorf("only one %s provider is allowed in the cluster", p.GetName())
 		}
 	}
 
 	// Wait for core provider to be ready before we install other providers.
 	if !util.IsCoreProvider(provider) {
 		ready, err := coreProviderIsReady(ctx, c)
 		if err != nil {
-			return ctrl.Result{}, fmt.Errorf("failed to get coreProvider ready condition: %w", err)
+			return ctrl.Result{}, true, fmt.Errorf("failed to get coreProvider ready condition: %w", err)
 		}
 
 		if !ready {
@@ -197,15 +207,15 @@ func PreflightChecks(ctx context.Context, c client.Client, provider genericprovi
 				waitingForCoreProviderReadyMessage,
 			))
 
-			return ctrl.Result{RequeueAfter: preflightFailedRequeueAfter}, nil
+			return ctrl.Result{RequeueAfter: preflightFailedRequeueAfter}, true, nil
 		}
 	}
 
 	conditions.Set(provider, conditions.TrueCondition(operatorv1.PreflightCheckCondition))
 
 	log.Info("Preflight checks passed")
 
-	return ctrl.Result{}, nil
+	return ctrl.Result{}, false, nil
 }
 
 // coreProviderIsReady returns true if the core provider is ready.
@@ -242,3 +252,26 @@ func isPredefinedProvider(providerName string, providerType clusterctlv1.Provide
 
 	return err == nil, nil
 }
+
+// versionChanged try to get installed version from provider status and decide if it has changed.
+func versionChanged(provider genericprovider.GenericProvider) (bool, error) {
+	installedVersion := provider.GetStatus().InstalledVersion
+	if installedVersion == nil {
+		return true, nil
+	}
+
+	currentVersion, err := version.ParseSemantic(*installedVersion)
+	if err != nil {
+		return false, err
+	}
+
+	res, err := currentVersion.Compare(provider.GetSpec().Version)
+	if err != nil {
+		return false, err
+	}
+
+	// we need to delete installed components if versions are different
+	versionChanged := res != 0
+
+	return versionChanged, nil
+}

internal/controller/preflightchecks/preflight_checks_test.go

@@ -46,6 +46,7 @@ func TestPreflightChecks(t *testing.T) {
 		providerList      genericprovider.GenericProviderList
 		expectedCondition clusterv1.Condition
 		expectedError     bool
+		expectedHalt      bool
 	}{
 		{
 			name: "only one core provider exists, preflight check passed",
@@ -82,6 +83,7 @@ func TestPreflightChecks(t *testing.T) {
 		{
 			name:          "two core providers were created, preflight check failed",
 			expectedError: true,
+			expectedHalt:  true,
 			providers: []genericprovider.GenericProvider{
 				&genericprovider.CoreProviderWrapper{
 					CoreProvider: &operatorv1.CoreProvider{
@@ -132,6 +134,7 @@ func TestPreflightChecks(t *testing.T) {
 		{
 			name:          "two core providers in two different namespaces were created, preflight check failed",
 			expectedError: true,
+			expectedHalt:  true,
 			providers: []genericprovider.GenericProvider{
 				&genericprovider.CoreProviderWrapper{
 					CoreProvider: &operatorv1.CoreProvider{
@@ -387,6 +390,7 @@ func TestPreflightChecks(t *testing.T) {
 		{
 			name:          "two similar infra provider exist in different namespaces, preflight check failed",
 			expectedError: true,
+			expectedHalt:  true,
 			providers: []genericprovider.GenericProvider{
 				&genericprovider.InfrastructureProviderWrapper{
 					InfrastructureProvider: &operatorv1.InfrastructureProvider{
@@ -437,6 +441,7 @@ func TestPreflightChecks(t *testing.T) {
 		{
 			name:          "wrong version, preflight check failed",
 			expectedError: true,
+			expectedHalt:  true,
 			providers: []genericprovider.GenericProvider{
 				&genericprovider.InfrastructureProviderWrapper{
 					InfrastructureProvider: &operatorv1.InfrastructureProvider{
@@ -470,6 +475,7 @@ func TestPreflightChecks(t *testing.T) {
 		{
 			name:          "missing version, preflight check failed",
 			expectedError: true,
+			expectedHalt:  true,
 			providers: []genericprovider.GenericProvider{
 				&genericprovider.InfrastructureProviderWrapper{
 					InfrastructureProvider: &operatorv1.InfrastructureProvider{
@@ -501,6 +507,7 @@ func TestPreflightChecks(t *testing.T) {
 		{
 			name:          "incorrect fetchConfig, preflight check failed",
 			expectedError: true,
+			expectedHalt:  true,
 			providers: []genericprovider.GenericProvider{
 				&genericprovider.InfrastructureProviderWrapper{
 					InfrastructureProvider: &operatorv1.InfrastructureProvider{
@@ -569,6 +576,7 @@ func TestPreflightChecks(t *testing.T) {
 		{
 			name:          "custom Core Provider without fetch config, preflight check failed",
 			expectedError: true,
+			expectedHalt:  true,
 			providers: []genericprovider.GenericProvider{
 				&genericprovider.CoreProviderWrapper{
 					CoreProvider: &operatorv1.CoreProvider{
@@ -602,6 +610,7 @@ func TestPreflightChecks(t *testing.T) {
 		{
 			name:          "custom Core Provider with fetch config with empty values, preflight check failed",
 			expectedError: true,
+			expectedHalt:  true,
 			providers: []genericprovider.GenericProvider{
 				&genericprovider.CoreProviderWrapper{
 					CoreProvider: &operatorv1.CoreProvider{
@@ -648,13 +657,19 @@ func TestPreflightChecks(t *testing.T) {
 				gs.Expect(fakeclient.Create(context.Background(), c.GetObject())).To(Succeed())
 			}
 
-			_, err := PreflightChecks(context.Background(), fakeclient, tc.providers[0], tc.providerList)
+			_, halt, err := PreflightChecks(context.Background(), fakeclient, tc.providers[0], tc.providerList)
 			if tc.expectedError {
 				gs.Expect(err).To(HaveOccurred())
 			} else {
 				gs.Expect(err).ToNot(HaveOccurred())
 			}
 
+			if tc.expectedHalt {
+				gs.Expect(halt).To(BeTrue())
+			} else {
+				gs.Expect(halt).To(BeFalse())
+			}
+
 			// Check if proper condition is returned
 			gs.Expect(tc.providers[0].GetStatus().Conditions).To(HaveLen(1))
 			gs.Expect(tc.providers[0].GetStatus().Conditions[0].Type).To(Equal(tc.expectedCondition.Type))