Merge pull request #1298 from apedriza/fix-cc-version-formats

Fix: support accepted formats for cluster version when using clsuterc…

Author: apedriza

api/k0smotron.io/v1beta1/k0smotroncluster_types.go

@@ -19,9 +19,10 @@ package v1beta1
 import (
 	"crypto/md5"
 	"fmt"
+	"strings"
+
 	"github.com/k0sproject/version"
 	"sigs.k8s.io/controller-runtime/pkg/webhook/admission"
-	"strings"
 
 	v1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/resource"
@@ -186,6 +187,9 @@ func (c *ClusterSpec) GetImage() string {
 		k0sVersion = DefaultK0SVersion
 	}
 
+	// Ensure any version with +k0s. is converted to -k0s. for image tagging
+	k0sVersion = strings.Replace(k0sVersion, "+k0s.", "-k0s.", 1)
+
 	if !strings.Contains(k0sVersion, "-k0s.") {
 		k0sVersion = fmt.Sprintf("%s-%s", k0sVersion, DefaultK0SSuffix)
 	}

api/k0smotron.io/v1beta1/k0smotroncluster_types_test.go

@@ -63,6 +63,14 @@ func TestClusterSpec_GetImage(t *testing.T) {
 			},
 			want: "foobar/k0s:v1.29.4-k0s.0",
 		},
+		{
+			name: "Image and version given with +k0s. suffix",
+			spec: &ClusterSpec{
+				Image:   "foobar/k0s",
+				Version: "v1.29.4+k0s.0",
+			},
+			want: "foobar/k0s:v1.29.4-k0s.0",
+		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {

internal/controller/bootstrap/worker_bootstrap_controller.go

@@ -130,9 +130,15 @@ func (r *Controller) Reconcile(ctx context.Context, req ctrl.Request) (res ctrl.
 	if config.Spec.Version == "" && machine.Spec.Version != nil {
 		config.Spec.Version = *machine.Spec.Version
 	}
+
 	// If the version does not contain the k0s suffix, append it.
-	if config.Spec.Version != "" && !strings.Contains(config.Spec.Version, "+k0s.") {
-		config.Spec.Version = fmt.Sprintf("%s+%s", config.Spec.Version, defaultK0sSuffix)
+	if config.Spec.Version != "" {
+		// When machine is created by CAPI, for example by using a clusterclass template, the version
+		// of the cluster may contain '-k0s.' instead of '+k0s.', so we need to replace it first.
+		config.Spec.Version = strings.Replace(config.Spec.Version, "-k0s.", "+k0s.", 1)
+		if !strings.Contains(config.Spec.Version, "+k0s.") {
+			config.Spec.Version = fmt.Sprintf("%s+%s", config.Spec.Version, defaultK0sSuffix)
+		}
 	}
 
 	// Lookup the cluster the config owner is associated with

internal/controller/controlplane/k0smotron_controlplane_controller.go

@@ -416,23 +416,6 @@ func ensureCertificates(ctx context.Context, cluster *clusterv1.Cluster, kcp *cp
 	return certificates.LookupOrGenerateCached(ctx, scope.secretCachingClient, scope.client, capiutil.ObjectKey(cluster), owner)
 }
 
-// FormatStatusVersion formats the status version to match the spec version format.
-// If spec.version doesn't contain "-k0s." suffix, it removes the suffix from status.version as well.
-func FormatStatusVersion(specVersion, statusVersion string) string {
-	specHasK0sSuffix := strings.Contains(specVersion, "-k0s.")
-
-	// Adjust status.version to match the format of spec.version
-	if !specHasK0sSuffix && strings.Contains(statusVersion, "-k0s.") {
-		// If spec.version doesn't have the -k0s. suffix, remove it from status.version as well
-		parts := strings.Split(statusVersion, "-k0s.")
-		if len(parts) > 0 {
-			return parts[0]
-		}
-	}
-
-	return statusVersion
-}
-
 func (c *K0smotronController) computeStatus(ctx context.Context, cluster *clusterv1.Cluster, kcp *cpv1beta1.K0smotronControlPlane, scope *kmcScope) error {
 	var kmc kapi.Cluster
 	err := c.Client.Get(ctx, types.NamespacedName{Name: cluster.Name, Namespace: cluster.Namespace}, &kmc)
@@ -462,8 +445,9 @@ func (c *K0smotronController) computeStatus(ctx context.Context, cluster *cluste
 	var updatedReplicas, readyReplicas, unavailableReplicas int
 
 	desiredVersionStr := kcp.Spec.Version
-	if !strings.Contains(desiredVersionStr, "-") {
-		desiredVersionStr = fmt.Sprintf("%s-%s", desiredVersionStr, kapi.DefaultK0SSuffix)
+	if !strings.Contains(desiredVersionStr, "-k0s.") && !strings.Contains(desiredVersionStr, "+k0s.") {
+		// Use default k0s suffix format when spec version does not contain either -k0s. or +k0s.
+		desiredVersionStr = fmt.Sprintf("%s+%s", desiredVersionStr, kapi.DefaultK0SSuffix)
 	}
 	desiredVersion, err := version.NewVersion(desiredVersionStr)
 	if err != nil {
@@ -488,7 +472,12 @@ func (c *K0smotronController) computeStatus(ctx context.Context, cluster *cluste
 			continue
 		}
 
-		currentVersion, err := scope.getComparableK0sVersionRunningInPod(ctx, &pod)
+		currentVersion, err := scope.getK0sVersionRunningInPod(ctx, &pod)
+		if err != nil {
+			return err
+		}
+		// Align version format in spec to the current version format for comparison. DO NOT modify version format in spec.
+		currentVersion, err = alignToSpecVersionFormat(desiredVersion, currentVersion)
 		if err != nil {
 			return err
 		}
@@ -507,9 +496,7 @@ func (c *K0smotronController) computeStatus(ctx context.Context, cluster *cluste
 	kcp.Status.UnavailableReplicas = int32(unavailableReplicas)
 
 	if kcp.Status.ReadyReplicas > 0 {
-		statusVersion := minimumVersion.String()
-		// Store the formatted version for Cluster API compatibility
-		kcp.Status.Version = FormatStatusVersion(kcp.Spec.Version, statusVersion)
+		kcp.Status.Version = minimumVersion.String()
 	}
 
 	c.computeAvailability(ctx, cluster, kcp)
@@ -519,14 +506,41 @@ func (c *K0smotronController) computeStatus(ctx context.Context, cluster *cluste
 	// Additionally, if the ControlPlaneReadyCondition is false (e.g., due to DNS resolution failures),
 	// we should also requeue to retry the connection.
 	if kcp.Status.UnavailableReplicas > 0 ||
-		FormatStatusVersion(kcp.Spec.Version, desiredVersion.String()) != kcp.Status.Version ||
+		desiredVersion.String() != kcp.Status.Version ||
 		!conditions.IsTrue(kcp, cpv1beta1.ControlPlaneReadyCondition) {
 		return ErrNotReady
 	}
 
 	return nil
 }
 
+// alignToSpecVersionFormat ensures that the currentVersion format matches the desiredVersion format.
+func alignToSpecVersionFormat(specVersion, currentVersion *version.Version) (*version.Version, error) {
+	specVersionUsesDefaultK0SSuffix := strings.Contains(specVersion.String(), "+k0s.")
+	currentVersionUsesDefaultK0SSuffix := strings.Contains(currentVersion.String(), "+k0s.")
+
+	isFormatAligned := (specVersionUsesDefaultK0SSuffix && currentVersionUsesDefaultK0SSuffix) ||
+		(!specVersionUsesDefaultK0SSuffix && !currentVersionUsesDefaultK0SSuffix)
+
+	if isFormatAligned {
+		return currentVersion, nil
+	}
+
+	currentVersionStr := currentVersion.String()
+	if !specVersionUsesDefaultK0SSuffix {
+		// Spec version format is like "vX.Y.Z-k0s.0".
+		// Current version format is like "vX.Y.Z+k0s.0".
+		// Convert currentVersion to match it.
+		currentVersionAlignedStr := strings.Replace(currentVersionStr, "+k0s.", "-k0s.", 1)
+		return version.NewVersion(currentVersionAlignedStr)
+	}
+	// Spec version format is like "vX.Y.Z+k0s.0".
+	// Current version format is like "vX.Y.Z-k0s.0".
+	// Convert currentVersion to match it.
+	currentVersionAlignedStr := strings.Replace(currentVersionStr, "-k0s.", "+k0s.", 1)
+	return version.NewVersion(currentVersionAlignedStr)
+}
+
 // computeAvailability checks if the control plane is ready by connecting to the API server
 // and checking if the control plane is initialized
 func (c *K0smotronController) computeAvailability(ctx context.Context, cluster *clusterv1.Cluster, kcp *cpv1beta1.K0smotronControlPlane) {
@@ -574,16 +588,12 @@ func (c *K0smotronController) computeAvailability(ctx context.Context, cluster *
 	})
 }
 
-func (scope *kmcScope) getComparableK0sVersionRunningInPod(ctx context.Context, pod *corev1.Pod) (*version.Version, error) {
+func (scope *kmcScope) getK0sVersionRunningInPod(ctx context.Context, pod *corev1.Pod) (*version.Version, error) {
 	currentVersionOutput, err := exec.PodExecCmdOutput(ctx, scope.clientSet, scope.restConfig, pod.GetName(), pod.GetNamespace(), "k0s version")
 	if err != nil {
 		return nil, err
 	}
 	currentVersionStr, _ := strings.CutSuffix(currentVersionOutput, "\n")
-	// In order to compare the version reported by the 'k0s version' command executed in the pod running
-	// the controlplane with the version declared in K0smotronControlPlane.spec this transformation is
-	// necessary to match their format.
-	currentVersionStr = strings.Replace(currentVersionStr, "+", "-", 1)
 	return version.NewVersion(currentVersionStr)
 }
 

internal/controller/controlplane/k0smotron_controlplane_controller_test.go

@@ -4,59 +4,11 @@ import (
 	"testing"
 
 	kapi "github.com/k0sproject/k0smotron/api/k0smotron.io/v1beta1"
-	"github.com/stretchr/testify/assert"
+	"github.com/k0sproject/version"
 	"github.com/stretchr/testify/require"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 )
 
-// TestFormatStatusVersion tests the formatStatusVersion function
-func TestFormatStatusVersion(t *testing.T) {
-	tests := []struct {
-		name           string
-		specVersion    string
-		statusVersion  string
-		expectedStatus string
-		description    string
-	}{
-		{
-			name:           "spec without k0s suffix",
-			specVersion:    "v1.33.1",
-			statusVersion:  "v1.33.1-k0s.0",
-			expectedStatus: "v1.33.1",
-			description:    "When spec.version doesn't have -k0s suffix, status.version should also not have it",
-		},
-		{
-			name:           "spec with k0s suffix",
-			specVersion:    "v1.33.1-k0s.0",
-			statusVersion:  "v1.33.1-k0s.0",
-			expectedStatus: "v1.33.1-k0s.0",
-			description:    "When spec.version has -k0s suffix, status.version should keep it",
-		},
-		{
-			name:           "spec with custom k0s suffix",
-			specVersion:    "v1.33.1-k0s.1",
-			statusVersion:  "v1.33.1-k0s.1",
-			expectedStatus: "v1.33.1-k0s.1",
-			description:    "When spec.version has custom -k0s suffix, status.version should keep it",
-		},
-		{
-			name:           "spec without suffix, status with different suffix",
-			specVersion:    "v1.33.1",
-			statusVersion:  "v1.33.1-k0s.1",
-			expectedStatus: "v1.33.1",
-			description:    "When spec.version doesn't have -k0s suffix, status.version should remove it even if it's different",
-		},
-	}
-
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			// Use the actual FormatStatusVersion function
-			result := FormatStatusVersion(tt.specVersion, tt.statusVersion)
-			assert.Equal(t, tt.expectedStatus, result, tt.description)
-		})
-	}
-}
-
 func TestIsClusterSpecSynced(t *testing.T) {
 	testCases := []struct {
 		name     string
@@ -247,3 +199,38 @@ func TestIsClusterSpecSynced(t *testing.T) {
 		})
 	}
 }
+
+func Test_alignToSpecVersionFormat(t *testing.T) {
+	tests := []struct {
+		name           string
+		specVersion    *version.Version
+		currentVersion *version.Version
+		want           *version.Version
+	}{
+		{
+			name:           "both versions have same format",
+			specVersion:    version.MustParse("v1.33.1-k0s.0"),
+			currentVersion: version.MustParse("v1.33.1-k0s.1"),
+			want:           version.MustParse("v1.33.1-k0s.1"),
+		},
+		{
+			name:           "versions does not have same format: spec with +k0s, current with -k0s",
+			specVersion:    version.MustParse("v1.33.1+k0s.0"),
+			currentVersion: version.MustParse("v1.33.1-k0s.1"),
+			want:           version.MustParse("v1.33.1+k0s.1"),
+		},
+		{
+			name:           "versions does not have same format: spec with -k0s, current with +k0s",
+			specVersion:    version.MustParse("v1.33.1-k0s.0"),
+			currentVersion: version.MustParse("v1.33.1+k0s.1"),
+			want:           version.MustParse("v1.33.1-k0s.1"),
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got, err := alignToSpecVersionFormat(tt.specVersion, tt.currentVersion)
+			require.NoError(t, err)
+			require.True(t, tt.want.Equal(got), "alignToSpecVersionFormat() = %v, want %v", got, tt.want)
+		})
+	}
+}

internal/controller/k0smotron.io/k0smotroncluster_webhook.go

@@ -76,8 +76,9 @@ func (c ClusterValidator) ValidateClusterSpec(kcs *km.ClusterSpec) (warnings adm
 // validateVersionSuffix checks if the version has a k0s suffix and returns a warning if it doesn't
 func (c ClusterValidator) validateVersionSuffix(version string) admission.Warnings {
 	warnings := admission.Warnings{}
-	if version != "" && !strings.Contains(version, "-k0s.") {
-		warnings = append(warnings, fmt.Sprintf("The specified version '%s' requires a k0s suffix (-k0s.<number>). Using '%s-k0s.0' instead.", version, version))
+	// When using CAPI clusterclass version can be specified with a +k0s. suffix.
+	if version != "" && (!strings.Contains(version, "-k0s.") && !strings.Contains(version, "+k0s.")) {
+		warnings = append(warnings, fmt.Sprintf("The specified version '%s' requires a k0s suffix (k0s.<number>). Using '%s-k0s.0' instead.", version, version))
 	}
 
 	return warnings

internal/controller/k0smotron.io/k0smotroncluster_webhook_test.go

@@ -0,0 +1,43 @@
+package k0smotronio
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/require"
+	"sigs.k8s.io/controller-runtime/pkg/webhook/admission"
+)
+
+func TestClusterValidator_validateVersionSuffix(t *testing.T) {
+	tests := []struct {
+		name    string
+		version string
+		want    admission.Warnings
+	}{
+		{
+			name:    "version without k0s suffix",
+			version: "v1.23.4",
+			want:    admission.Warnings{"The specified version 'v1.23.4' requires a k0s suffix (k0s.<number>). Using 'v1.23.4-k0s.0' instead."},
+		},
+		{
+			name:    "version with k0s suffix",
+			version: "v1.23.4-k0s.2",
+			want:    admission.Warnings{},
+		},
+		{
+			name:    "empty version",
+			version: "",
+			want:    admission.Warnings{},
+		},
+		{
+			name:    "version with +k0s. suffix",
+			version: "v1.23.4+k0s.2",
+			want:    admission.Warnings{},
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			var c ClusterValidator
+			require.Equal(t, tt.want, c.validateVersionSuffix(tt.version))
+		})
+	}
+}

inttest/capi-docker-clusterclass-k0smotron/capi_docker_clusterclass_k0smotron_test.go

@@ -113,7 +113,7 @@ func (s *CAPIDockerClusterClassK0smotronSuite) TestCAPIDocker() {
 			kcp.Status.UnavailableReplicas == 0 &&
 			kcp.Status.Ready &&
 			kcp.Status.UpdatedReplicas == 2 &&
-			kcp.Status.Version == "v1.27.2"
+			kcp.Status.Version == "v1.27.2+k0s.0"
 
 		return ready, nil
 	})