From 929728cb37942e1c7c769dbfde12aa9e009d96e8 Mon Sep 17 00:00:00 2001
From: abhinavnagaraj <abhinav@spectrocloud.com>
Date: Wed, 6 Oct 2021 18:12:09 -0700
Subject: [PATCH] normalize MachineSet version validation

---
 api/v1beta1/machineset_webhook.go      | 13 +++++
 api/v1beta1/machineset_webhook_test.go | 68 +++++++++++++++++++++++++-
 2 files changed, 80 insertions(+), 1 deletion(-)

diff --git a/api/v1beta1/machineset_webhook.go b/api/v1beta1/machineset_webhook.go
index 5785ef2c55c5..ba4b093d4ecf 100644
--- a/api/v1beta1/machineset_webhook.go
+++ b/api/v1beta1/machineset_webhook.go
@@ -18,12 +18,14 @@ package v1beta1
 
 import (
 	"fmt"
+	"strings"
 
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/labels"
 	runtime "k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/util/validation/field"
+	"sigs.k8s.io/cluster-api/util/version"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/webhook"
 )
@@ -64,6 +66,11 @@ func (m *MachineSet) Default() {
 		m.Spec.Selector.MatchLabels[MachineSetLabelName] = m.Name
 		m.Spec.Template.Labels[MachineSetLabelName] = m.Name
 	}
+
+	if m.Spec.Template.Spec.Version != nil && !strings.HasPrefix(*m.Spec.Template.Spec.Version, "v") {
+		normalizedVersion := "v" + *m.Spec.Template.Spec.Version
+		m.Spec.Template.Spec.Version = &normalizedVersion
+	}
 }
 
 // ValidateCreate implements webhook.Validator so a webhook will be registered for the type.
@@ -111,6 +118,12 @@ func (m *MachineSet) validate(old *MachineSet) error {
 		)
 	}
 
+	if m.Spec.Template.Spec.Version != nil {
+		if !version.KubeSemver.MatchString(*m.Spec.Template.Spec.Version) {
+			allErrs = append(allErrs, field.Invalid(field.NewPath("spec", "template", "spec", "version"), *m.Spec.Template.Spec.Version, "must be a valid semantic version"))
+		}
+	}
+
 	if len(allErrs) == 0 {
 		return nil
 	}
diff --git a/api/v1beta1/machineset_webhook_test.go b/api/v1beta1/machineset_webhook_test.go
index 28025e17bc14..bb033d363ed8 100644
--- a/api/v1beta1/machineset_webhook_test.go
+++ b/api/v1beta1/machineset_webhook_test.go
@@ -20,8 +20,8 @@ import (
 	"testing"
 
 	. "github.com/onsi/gomega"
-
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/utils/pointer"
 	utildefaulting "sigs.k8s.io/cluster-api/util/defaulting"
 )
 
@@ -31,6 +31,13 @@ func TestMachineSetDefault(t *testing.T) {
 		ObjectMeta: metav1.ObjectMeta{
 			Name: "test-ms",
 		},
+		Spec: MachineSetSpec{
+			Template: MachineTemplateSpec{
+				Spec: MachineSpec{
+					Version: pointer.String("1.19.10"),
+				},
+			},
+		},
 	}
 	t.Run("for MachineSet", utildefaulting.DefaultValidateTest(ms))
 	ms.Default()
@@ -39,6 +46,7 @@ func TestMachineSetDefault(t *testing.T) {
 	g.Expect(ms.Spec.DeletePolicy).To(Equal(string(RandomMachineSetDeletePolicy)))
 	g.Expect(ms.Spec.Selector.MatchLabels).To(HaveKeyWithValue(MachineSetLabelName, "test-ms"))
 	g.Expect(ms.Spec.Template.Labels).To(HaveKeyWithValue(MachineSetLabelName, "test-ms"))
+	g.Expect(*ms.Spec.Template.Spec.Version).To(Equal("v1.19.10"))
 }
 
 func TestMachineSetLabelSelectorMatchValidation(t *testing.T) {
@@ -151,3 +159,61 @@ func TestMachineSetClusterNameImmutable(t *testing.T) {
 		})
 	}
 }
+
+func TestMachineSetVersionValidation(t *testing.T) {
+	tests := []struct {
+		name      string
+		version   string
+		expectErr bool
+	}{
+		{
+			name:      "should succeed when given a valid semantic version with prepended 'v'",
+			version:   "v1.19.2",
+			expectErr: false,
+		},
+		{
+			name:      "should return error when given a valid semantic version without 'v'",
+			version:   "1.19.2",
+			expectErr: true,
+		},
+		{
+			name:      "should return error when given an invalid semantic version",
+			version:   "1",
+			expectErr: true,
+		},
+		{
+			name:      "should return error when given an invalid semantic version",
+			version:   "v1",
+			expectErr: true,
+		},
+		{
+			name:      "should return error when given an invalid semantic version",
+			version:   "wrong_version",
+			expectErr: true,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			g := NewWithT(t)
+
+			md := &MachineSet{
+				Spec: MachineSetSpec{
+					Template: MachineTemplateSpec{
+						Spec: MachineSpec{
+							Version: pointer.String(tt.version),
+						},
+					},
+				},
+			}
+
+			if tt.expectErr {
+				g.Expect(md.ValidateCreate()).NotTo(Succeed())
+				g.Expect(md.ValidateUpdate(md)).NotTo(Succeed())
+			} else {
+				g.Expect(md.ValidateCreate()).To(Succeed())
+				g.Expect(md.ValidateUpdate(md)).To(Succeed())
+			}
+		})
+	}
+}