🐛 Make MachineSpec Version validation consistent with KCP

kubernetes-sigs · Jun 5, 2020 · 8a03ae7 · 8a03ae7
1 parent bfb2a3b
commit 8a03ae7
Show file tree

Hide file tree

Showing 10 changed files with 38 additions and 42 deletions.
diff --git a/api/v1alpha3/machine_webhook.go b/api/v1alpha3/machine_webhook.go
@@ -18,9 +18,9 @@ package v1alpha3
 
 import (
 	"fmt"
+	"regexp"
 	"strings"
 
-	"github.com/blang/semver"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	runtime "k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/util/validation/field"
@@ -40,6 +40,8 @@ func (m *Machine) SetupWebhookWithManager(mgr ctrl.Manager) error {
 var _ webhook.Validator = &Machine{}
 var _ webhook.Defaulter = &Machine{}
 
+var kubeSemver = regexp.MustCompile(`^v(0|[1-9][0-9]*)\.(0|[1-9][0-9]*)\.(0|[1-9][0-9]*)([-0-9a-zA-Z_\.+]*)?$`)
+
 // Default implements webhook.Defaulter so a webhook will be registered for the type
 func (m *Machine) Default() {
 	if m.Labels == nil {
@@ -54,6 +56,11 @@ func (m *Machine) Default() {
 	if len(m.Spec.InfrastructureRef.Namespace) == 0 {
 		m.Spec.InfrastructureRef.Namespace = m.Namespace
 	}
+
+	if m.Spec.Version != nil && !strings.HasPrefix(*m.Spec.Version, "v") {
+		normalizedVersion := "v" + *m.Spec.Version
+		m.Spec.Version = &normalizedVersion
+	}
 }
 
 // ValidateCreate implements webhook.Validator so a webhook will be registered for the type
@@ -117,7 +124,7 @@ func (m *Machine) validate(old *Machine) error {
 	}
 
 	if m.Spec.Version != nil {
-		if _, err := semver.Parse(strings.TrimPrefix(strings.TrimSpace(*m.Spec.Version), "v")); err != nil {
+		if !kubeSemver.MatchString(*m.Spec.Version) {
 			allErrs = append(allErrs, field.Invalid(field.NewPath("spec", "version"), *m.Spec.Version, "must be a valid semantic version"))
 		}
 	}

diff --git a/api/v1alpha3/machine_webhook_test.go b/api/v1alpha3/machine_webhook_test.go
@@ -35,6 +35,7 @@ func TestMachineDefault(t *testing.T) {
 		},
 		Spec: MachineSpec{
 			Bootstrap: Bootstrap{ConfigRef: &corev1.ObjectReference{}},
+			Version:   pointer.StringPtr("1.17.5"),
 		},
 	}
 
@@ -43,6 +44,7 @@ func TestMachineDefault(t *testing.T) {
 	g.Expect(m.Labels[ClusterLabelName]).To(Equal(m.Spec.ClusterName))
 	g.Expect(m.Spec.Bootstrap.ConfigRef.Namespace).To(Equal(m.Namespace))
 	g.Expect(m.Spec.InfrastructureRef.Namespace).To(Equal(m.Namespace))
+	g.Expect(*m.Spec.Version).To(Equal("v1.17.5"))
 }
 
 func TestMachineBootstrapValidation(t *testing.T) {
@@ -202,9 +204,9 @@ func TestMachineVersionValidation(t *testing.T) {
 			expectErr: false,
 		},
 		{
-			name:      "should succeed when given a valid semantic version without 'v'",
+			name:      "should return error when given a valid semantic version without 'v'",
 			version:   "1.17.2",
-			expectErr: false,
+			expectErr: true,
 		},
 		{
 			name:      "should return error when given an invalid semantic version",

diff --git a/controllers/machinedeployment_controller_test.go b/controllers/machinedeployment_controller_test.go
@@ -66,7 +66,7 @@ var _ = Describe("MachineDeployment Reconciler", func() {
 			"foo":                      "bar",
 			clusterv1.ClusterLabelName: testCluster.Name,
 		}
-		version := "1.10.3"
+		version := "v1.10.3"
 		deployment := &clusterv1.MachineDeployment{
 			ObjectMeta: metav1.ObjectMeta{
 				GenerateName: "md-",
@@ -207,7 +207,7 @@ var _ = Describe("MachineDeployment Reconciler", func() {
 
 		firstMachineSet := machineSets.Items[0]
 		Expect(*firstMachineSet.Spec.Replicas).To(BeEquivalentTo(2))
-		Expect(*firstMachineSet.Spec.Template.Spec.Version).To(BeEquivalentTo("1.10.3"))
+		Expect(*firstMachineSet.Spec.Template.Spec.Version).To(BeEquivalentTo("v1.10.3"))
 
 		//
 		// Delete firstMachineSet and expect Reconcile to be called to replace it.

diff --git a/controllers/machineset_controller_test.go b/controllers/machineset_controller_test.go
@@ -66,7 +66,7 @@ var _ = Describe("MachineSet Reconciler", func() {
 
 	It("Should reconcile a MachineSet", func() {
 		replicas := int32(2)
-		version := "1.14.2"
+		version := "v1.14.2"
 		instance := &clusterv1.MachineSet{
 			ObjectMeta: metav1.ObjectMeta{
 				GenerateName: "ms-",
@@ -236,7 +236,7 @@ var _ = Describe("MachineSet Reconciler", func() {
 			}
 
 			Expect(m.Spec.Version).ToNot(BeNil())
-			Expect(*m.Spec.Version).To(BeEquivalentTo("1.14.2"))
+			Expect(*m.Spec.Version).To(BeEquivalentTo("v1.14.2"))
 			fakeInfrastructureRefReady(m.Spec.InfrastructureRef, infraResource)
 			fakeMachineNodeRef(&m)
 		}

diff --git a/controlplane/kubeadm/api/v1alpha3/kubeadm_control_plane_types.go b/controlplane/kubeadm/api/v1alpha3/kubeadm_control_plane_types.go
@@ -41,8 +41,6 @@ type KubeadmControlPlaneSpec struct {
 	Replicas *int32 `json:"replicas,omitempty"`
 
 	// Version defines the desired Kubernetes version.
-	// +kubebuilder:validation:MinLength:=2
-	// +kubebuilder:validation:Pattern:=^v(0|[1-9][0-9]*)\.(0|[1-9][0-9]*)\.(0|[1-9][0-9]*)([-0-9a-zA-Z_\.+]*)?$
 	Version string `json:"version"`
 
 	// InfrastructureTemplate is a required reference to a custom resource

diff --git a/controlplane/kubeadm/api/v1alpha3/kubeadm_control_plane_types_test.go b/controlplane/kubeadm/api/v1alpha3/kubeadm_control_plane_types_test.go
@@ -51,22 +51,6 @@ var _ = Describe("KubeadmControlPlane", func() {
 				Namespace: "default",
 			}
 
-			// wrong version value
-			created = &KubeadmControlPlane{
-				ObjectMeta: metav1.ObjectMeta{
-					Name:      "foo",
-					Namespace: "default",
-				},
-				Spec: KubeadmControlPlaneSpec{
-					InfrastructureTemplate: corev1.ObjectReference{},
-					Version:                "1",
-					KubeadmConfigSpec:      cabpkv1.KubeadmConfigSpec{},
-				},
-			}
-
-			By("creating an API obj with wrong version")
-			Expect(k8sClient.Create(ctx, created)).NotTo(Succeed())
-
 			// missing field
 			created2 := map[string]interface{}{
 				"kind":       "KubeadmControlPlane",

diff --git a/controlplane/kubeadm/api/v1alpha3/kubeadm_control_plane_webhook.go b/controlplane/kubeadm/api/v1alpha3/kubeadm_control_plane_webhook.go
@@ -19,12 +19,12 @@ package v1alpha3
 import (
 	"encoding/json"
 	"fmt"
+	"regexp"
 	"strings"
 
 	"github.com/coredns/corefile-migration/migration"
 	kubeadmv1 "sigs.k8s.io/cluster-api/bootstrap/kubeadm/types/v1beta1"
 
-	"github.com/blang/semver"
 	jsonpatch "github.com/evanphx/json-patch"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/runtime"
@@ -47,6 +47,8 @@ func (in *KubeadmControlPlane) SetupWebhookWithManager(mgr ctrl.Manager) error {
 var _ webhook.Defaulter = &KubeadmControlPlane{}
 var _ webhook.Validator = &KubeadmControlPlane{}
 
+var kubeSemver = regexp.MustCompile(`^v(0|[1-9][0-9]*)\.(0|[1-9][0-9]*)\.(0|[1-9][0-9]*)([-0-9a-zA-Z_\.+]*)?$`)
+
 // Default implements webhook.Defaulter so a webhook will be registered for the type
 func (in *KubeadmControlPlane) Default() {
 	if in.Spec.Replicas == nil {
@@ -57,6 +59,10 @@ func (in *KubeadmControlPlane) Default() {
 	if in.Spec.InfrastructureTemplate.Namespace == "" {
 		in.Spec.InfrastructureTemplate.Namespace = in.Namespace
 	}
+
+	if !strings.HasPrefix(in.Spec.Version, "v") {
+		in.Spec.Version = "v" + in.Spec.Version
+	}
 }
 
 // ValidateCreate implements webhook.Validator so a webhook will be registered for the type
@@ -245,7 +251,7 @@ func (in *KubeadmControlPlane) validateCommon() (allErrs field.ErrorList) {
 		)
 	}
 
-	if _, err := semver.Parse(strings.TrimPrefix(strings.TrimSpace(in.Spec.Version), "v")); err != nil {
+	if !kubeSemver.MatchString(in.Spec.Version) {
 		allErrs = append(allErrs, field.Invalid(field.NewPath("spec", "version"), in.Spec.Version, "must be a valid semantic version"))
 	}
 

diff --git a/controlplane/kubeadm/api/v1alpha3/kubeadm_control_plane_webhook_test.go b/controlplane/kubeadm/api/v1alpha3/kubeadm_control_plane_webhook_test.go
@@ -38,11 +38,13 @@ func TestKubeadmControlPlaneDefault(t *testing.T) {
 		},
 		Spec: KubeadmControlPlaneSpec{
 			InfrastructureTemplate: corev1.ObjectReference{},
+			Version:                "1.18.3",
 		},
 	}
 	kcp.Default()
 
 	g.Expect(kcp.Spec.InfrastructureTemplate.Namespace).To(Equal(kcp.Namespace))
+	g.Expect(kcp.Spec.Version).To(Equal("v1.18.3"))
 }
 
 func TestKubeadmControlPlaneValidateCreate(t *testing.T) {
@@ -81,14 +83,14 @@ func TestKubeadmControlPlaneValidateCreate(t *testing.T) {
 		},
 	}
 
-	validVersion1 := valid.DeepCopy()
-	validVersion1.Spec.Version = "v1.16.6"
+	validVersion := valid.DeepCopy()
+	validVersion.Spec.Version = "v1.16.6"
 
-	validVersion2 := valid.DeepCopy()
-	validVersion2.Spec.Version = "1.16.6"
+	invalidVersion1 := valid.DeepCopy()
+	invalidVersion1.Spec.Version = "vv1.16.6"
 
-	invalidVersion := valid.DeepCopy()
-	invalidVersion.Spec.Version = "vv1.16.6"
+	invalidVersion2 := valid.DeepCopy()
+	invalidVersion2.Spec.Version = "1.16.6"
 
 	tests := []struct {
 		name      string
@@ -128,17 +130,17 @@ func TestKubeadmControlPlaneValidateCreate(t *testing.T) {
 		{
 			name:      "should succeed when given a valid semantic version with prepended 'v'",
 			expectErr: false,
-			kcp:       validVersion1,
+			kcp:       validVersion,
 		},
 		{
 			name:      "should succeed when given a valid semantic version without 'v'",
-			expectErr: false,
-			kcp:       validVersion2,
+			expectErr: true,
+			kcp:       invalidVersion2,
 		},
 		{
 			name:      "should return error when given an invalid semantic version",
 			expectErr: true,
-			kcp:       invalidVersion,
+			kcp:       invalidVersion1,
 		},
 	}
 

diff --git a/...rolplane/kubeadm/config/crd/bases/controlplane.cluster.x-k8s.io_kubeadmcontrolplanes.yaml b/...rolplane/kubeadm/config/crd/bases/controlplane.cluster.x-k8s.io_kubeadmcontrolplanes.yaml
@@ -1036,8 +1036,6 @@ spec:
                 type: string
               version:
                 description: Version defines the desired Kubernetes version.
-                minLength: 2
-                pattern: ^v(0|[1-9][0-9]*)\.(0|[1-9][0-9]*)\.(0|[1-9][0-9]*)([-0-9a-zA-Z_\.+]*)?$
                 type: string
             required:
             - infrastructureTemplate

diff --git a/test/e2e/common.go b/test/e2e/common.go
@@ -20,7 +20,6 @@ import (
 	"context"
 	"fmt"
 	"path/filepath"
-	"strings"
 
 	. "github.com/onsi/ginkgo"
 
@@ -94,7 +93,7 @@ func HaveValidVersion(version string) types.GomegaMatcher {
 type validVersionMatcher struct{ version string }
 
 func (m *validVersionMatcher) Match(actual interface{}) (success bool, err error) {
-	if _, err := semver.Parse(strings.TrimPrefix(strings.TrimSpace(m.version), "v")); err != nil {
+	if _, err := semver.ParseTolerant(m.version); err != nil {
 		return false, err
 	}
 	return true, nil