Merge pull request #407 from Fedosin/no_downgrades

✨ Add a preflight check to prevent provider downgrades
kubernetes-sigs · Feb 6, 2024 · 630ec4b · 630ec4b
2 parents f5d8954 + a22f799
commit 630ec4b
Show file tree

Hide file tree

Showing 3 changed files with 147 additions and 11 deletions.
diff --git a/api/v1alpha2/conditions_consts.go b/api/v1alpha2/conditions_consts.go
@@ -61,6 +61,9 @@ const (
 
 	// NoDeploymentAvailableConditionReason documents that there is no Available condition for provider deployment yet.
 	NoDeploymentAvailableConditionReason = "NoDeploymentAvailableConditionReason"
+
+	// UnsupportedProviderDowngradeReason documents that the provider downgrade is not supported.
+	UnsupportedProviderDowngradeReason = "UnsupportedProviderDowngradeReason"
 )
 
 const (

diff --git a/internal/controller/preflight_checks.go b/internal/controller/preflight_checks.go
@@ -48,6 +48,7 @@ var (
 	invalidGithubTokenMessage                    = "Invalid github token, please check your github token value and its permissions" //nolint:gosec
 	waitingForCoreProviderReadyMessage           = "Waiting for the core provider to be installed."
 	incorrectCoreProviderNameMessage             = "Incorrect CoreProvider name: %s. It should be %s"
+	unsupportedProviderDowngradeMessage          = "Downgrade is not supported for provider %s"
 )
 
 // preflightChecks performs preflight checks before installing provider.
@@ -58,18 +59,10 @@ func preflightChecks(ctx context.Context, c client.Client, provider genericprovi
 
 	spec := provider.GetSpec()
 
-	// Check that provider version contains a valid value if it's not empty.
 	if spec.Version != "" {
-		if _, err := version.ParseSemantic(spec.Version); err != nil {
-			log.Info("Version contains invalid value")
-			conditions.Set(provider, conditions.FalseCondition(
-				operatorv1.PreflightCheckCondition,
-				operatorv1.IncorrectVersionFormatReason,
-				clusterv1.ConditionSeverityError,
-				err.Error(),
-			))
-
-			return ctrl.Result{}, fmt.Errorf("version contains invalid value for provider %q", provider.GetName())
+		// Check that the provider version is supported.
+		if err := checkProviderVersion(ctx, spec.Version, provider); err != nil {
+			return ctrl.Result{}, err
 		}
 	}
 
@@ -208,6 +201,46 @@ func preflightChecks(ctx context.Context, c client.Client, provider genericprovi
 	return ctrl.Result{}, nil
 }
 
+// checkProviderVersion verifies that target and installed provider versions are correct.
+func checkProviderVersion(ctx context.Context, providerVersion string, provider genericprovider.GenericProvider) error {
+	log := ctrl.LoggerFrom(ctx)
+
+	// Check that provider version contains a valid value if it's not empty.
+	targetVersion, err := version.ParseSemantic(providerVersion)
+	if err != nil {
+		log.Info("Version contains invalid value")
+		conditions.Set(provider, conditions.FalseCondition(
+			operatorv1.PreflightCheckCondition,
+			operatorv1.IncorrectVersionFormatReason,
+			clusterv1.ConditionSeverityError,
+			err.Error(),
+		))
+
+		return fmt.Errorf("version contains invalid value for provider %q", provider.GetName())
+	}
+
+	// Cluster API doesn't support downgrades by design. We need to report that for the user.
+	if provider.GetStatus().InstalledVersion != nil && *provider.GetStatus().InstalledVersion != "" {
+		installedVersion, err := version.ParseSemantic(*provider.GetStatus().InstalledVersion)
+		if err != nil {
+			return fmt.Errorf("installed version contains invalid value for provider %q", provider.GetName())
+		}
+
+		if targetVersion.Major() < installedVersion.Major() || targetVersion.Major() == installedVersion.Major() && targetVersion.Minor() < installedVersion.Minor() {
+			conditions.Set(provider, conditions.FalseCondition(
+				operatorv1.PreflightCheckCondition,
+				operatorv1.UnsupportedProviderDowngradeReason,
+				clusterv1.ConditionSeverityError,
+				fmt.Sprintf(unsupportedProviderDowngradeMessage, provider.GetName(), configclient.ClusterAPIProviderName),
+			))
+
+			return fmt.Errorf("downgrade is not supported for provider %q", provider.GetName())
+		}
+	}
+
+	return nil
+}
+
 // coreProviderIsReady returns true if the core provider is ready.
 func coreProviderIsReady(ctx context.Context, c client.Client) (bool, error) {
 	cpl := &operatorv1.CoreProviderList{}

diff --git a/internal/controller/preflight_checks_test.go b/internal/controller/preflight_checks_test.go
@@ -623,3 +623,103 @@ func TestPreflightChecks(t *testing.T) {
 		})
 	}
 }
+
+func TestPreflightChecksUpgradesDowngrades(t *testing.T) {
+	testCases := []struct {
+		name                    string
+		installedVersion        string
+		targetVersion           string
+		expectedConditionStatus corev1.ConditionStatus
+		expectedError           bool
+	}{
+		{
+			name:                    "upgrade core provider major version",
+			expectedConditionStatus: corev1.ConditionTrue,
+			installedVersion:        "v1.9.0",
+			targetVersion:           "v2.0.0",
+		},
+		{
+			name:                    "upgrade core provider minor version",
+			expectedConditionStatus: corev1.ConditionTrue,
+			installedVersion:        "v1.9.0",
+			targetVersion:           "v1.10.0",
+		},
+		{
+			name:                    "downgrade core provider major version",
+			expectedConditionStatus: corev1.ConditionFalse,
+			installedVersion:        "v2.0.0",
+			targetVersion:           "v1.9.0",
+			expectedError:           true,
+		},
+		{
+			name:                    "downgrade core provider minor version",
+			expectedConditionStatus: corev1.ConditionFalse,
+			installedVersion:        "v1.10.0",
+			targetVersion:           "v1.9.0",
+			expectedError:           true,
+		},
+		{
+			name:                    "downgrade core provider patch version",
+			expectedConditionStatus: corev1.ConditionTrue,
+			installedVersion:        "v1.10.1",
+			targetVersion:           "v1.10.0",
+		},
+		{
+			name:                    "same version",
+			expectedConditionStatus: corev1.ConditionTrue,
+			installedVersion:        "v1.10.0",
+			targetVersion:           "v1.10.0",
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			gs := NewWithT(t)
+
+			provider := &operatorv1.CoreProvider{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "cluster-api",
+					Namespace: "provider-test-ns-1",
+				},
+				TypeMeta: metav1.TypeMeta{
+					Kind:       "CoreProvider",
+					APIVersion: "operator.cluster.x-k8s.io/v1alpha1",
+				},
+				Spec: operatorv1.CoreProviderSpec{
+					ProviderSpec: operatorv1.ProviderSpec{
+						Version: tc.targetVersion,
+						FetchConfig: &operatorv1.FetchConfiguration{
+							URL: "https://example.com",
+						},
+					},
+				},
+				Status: operatorv1.CoreProviderStatus{
+					ProviderStatus: operatorv1.ProviderStatus{
+						InstalledVersion: &tc.installedVersion,
+					},
+				},
+			}
+
+			fakeclient := fake.NewClientBuilder().WithObjects().Build()
+
+			gs.Expect(fakeclient.Create(ctx, provider)).To(Succeed())
+
+			_, err := preflightChecks(context.Background(), fakeclient, provider, &operatorv1.CoreProviderList{})
+			if tc.expectedError {
+				gs.Expect(err).To(HaveOccurred())
+			} else {
+				gs.Expect(err).ToNot(HaveOccurred())
+			}
+
+			// Check if proper condition is returned
+			gs.Expect(provider.GetStatus().Conditions).To(HaveLen(1))
+			gs.Expect(provider.GetStatus().Conditions[0].Type).To(Equal(operatorv1.PreflightCheckCondition))
+			gs.Expect(provider.GetStatus().Conditions[0].Status).To(Equal(tc.expectedConditionStatus))
+
+			if tc.expectedConditionStatus == corev1.ConditionFalse {
+				gs.Expect(provider.GetStatus().Conditions[0].Reason).To(Equal(operatorv1.UnsupportedProviderDowngradeReason))
+				gs.Expect(provider.GetStatus().Conditions[0].Severity).To(Equal(clusterv1.ConditionSeverityError))
+			}
+		})
+	}
+}