🐛 Make MachineSpec Version validation consistent with KCP Version val…

…idation
kubernetes-sigs · Jun 4, 2020 · 13320cd · 13320cd
1 parent e72c3ae
commit 13320cd
Show file tree

Hide file tree

Showing 10 changed files with 6 additions and 95 deletions.
diff --git a/api/v1alpha3/machine_types.go b/api/v1alpha3/machine_types.go
@@ -57,6 +57,7 @@ type MachineSpec struct {
 
 	// Version defines the desired Kubernetes version.
 	// This field is meant to be optionally used by bootstrap providers.
+	// +kubebuilder:validation:Pattern:=^v(0|[1-9][0-9]*)\.(0|[1-9][0-9]*)\.(0|[1-9][0-9]*)([-0-9a-zA-Z_\.+]*)?$
 	// +optional
 	Version *string `json:"version,omitempty"`
 

diff --git a/api/v1alpha3/machine_webhook.go b/api/v1alpha3/machine_webhook.go
@@ -18,9 +18,7 @@ package v1alpha3
 
 import (
 	"fmt"
-	"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"
@@ -116,12 +114,6 @@ 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 {
-			allErrs = append(allErrs, field.Invalid(field.NewPath("spec", "version"), *m.Spec.Version, "must be a valid semantic version"))
-		}
-	}
-
 	if len(allErrs) == 0 {
 		return nil
 	}

diff --git a/api/v1alpha3/machine_webhook_test.go b/api/v1alpha3/machine_webhook_test.go
@@ -189,58 +189,3 @@ func TestMachineClusterNameImmutable(t *testing.T) {
 		})
 	}
 }
-
-func TestMachineVersionValidation(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.17.2",
-			expectErr: false,
-		},
-		{
-			name:      "should succeed when given a valid semantic version without 'v'",
-			version:   "1.17.2",
-			expectErr: false,
-		},
-		{
-			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)
-
-			m := &Machine{
-				Spec: MachineSpec{
-					Version:   &tt.version,
-					Bootstrap: Bootstrap{ConfigRef: nil, DataSecretName: pointer.StringPtr("test")},
-				},
-			}
-
-			if tt.expectErr {
-				g.Expect(m.ValidateCreate()).NotTo(Succeed())
-				g.Expect(m.ValidateUpdate(m)).NotTo(Succeed())
-			} else {
-				g.Expect(m.ValidateCreate()).To(Succeed())
-				g.Expect(m.ValidateUpdate(m)).To(Succeed())
-			}
-		})
-	}
-}
diff --git a/config/crd/bases/cluster.x-k8s.io_machinedeployments.yaml b/config/crd/bases/cluster.x-k8s.io_machinedeployments.yaml
@@ -942,6 +942,7 @@ spec:
                       version:
                         description: Version defines the desired Kubernetes version.
                           This field is meant to be optionally used by bootstrap providers.
+                        pattern: ^v(0|[1-9][0-9]*)\.(0|[1-9][0-9]*)\.(0|[1-9][0-9]*)([-0-9a-zA-Z_\.+]*)?$
                         type: string
                     required:
                     - bootstrap

diff --git a/config/crd/bases/cluster.x-k8s.io_machines.yaml b/config/crd/bases/cluster.x-k8s.io_machines.yaml
@@ -529,6 +529,7 @@ spec:
               version:
                 description: Version defines the desired Kubernetes version. This
                   field is meant to be optionally used by bootstrap providers.
+                pattern: ^v(0|[1-9][0-9]*)\.(0|[1-9][0-9]*)\.(0|[1-9][0-9]*)([-0-9a-zA-Z_\.+]*)?$
                 type: string
             required:
             - bootstrap

diff --git a/config/crd/bases/cluster.x-k8s.io_machinesets.yaml b/config/crd/bases/cluster.x-k8s.io_machinesets.yaml
@@ -846,6 +846,7 @@ spec:
                       version:
                         description: Version defines the desired Kubernetes version.
                           This field is meant to be optionally used by bootstrap providers.
+                        pattern: ^v(0|[1-9][0-9]*)\.(0|[1-9][0-9]*)\.(0|[1-9][0-9]*)([-0-9a-zA-Z_\.+]*)?$
                         type: string
                     required:
                     - bootstrap

diff --git a/config/crd/bases/exp.cluster.x-k8s.io_machinepools.yaml b/config/crd/bases/exp.cluster.x-k8s.io_machinepools.yaml
@@ -353,6 +353,7 @@ spec:
                       version:
                         description: Version defines the desired Kubernetes version.
                           This field is meant to be optionally used by bootstrap providers.
+                        pattern: ^v(0|[1-9][0-9]*)\.(0|[1-9][0-9]*)\.(0|[1-9][0-9]*)([-0-9a-zA-Z_\.+]*)?$
                         type: string
                     required:
                     - bootstrap

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,10 @@ package v1alpha3
 import (
 	"encoding/json"
 	"fmt"
-	"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"
@@ -245,10 +243,6 @@ func (in *KubeadmControlPlane) validateCommon() (allErrs field.ErrorList) {
 		)
 	}
 
-	if _, err := semver.Parse(strings.TrimPrefix(strings.TrimSpace(in.Spec.Version), "v")); err != nil {
-		allErrs = append(allErrs, field.Invalid(field.NewPath("spec", "version"), in.Spec.Version, "must be a valid semantic version"))
-	}
-
 	allErrs = append(allErrs, in.validateCoreDNSImage()...)
 
 	return allErrs

diff --git a/controlplane/kubeadm/api/v1alpha3/kubeadm_control_plane_webhook_test.go b/controlplane/kubeadm/api/v1alpha3/kubeadm_control_plane_webhook_test.go
@@ -81,15 +81,6 @@ func TestKubeadmControlPlaneValidateCreate(t *testing.T) {
 		},
 	}
 
-	validVersion1 := valid.DeepCopy()
-	validVersion1.Spec.Version = "v1.16.6"
-
-	validVersion2 := valid.DeepCopy()
-	validVersion2.Spec.Version = "1.16.6"
-
-	invalidVersion := valid.DeepCopy()
-	invalidVersion.Spec.Version = "vv1.16.6"
-
 	tests := []struct {
 		name      string
 		expectErr bool
@@ -125,21 +116,6 @@ func TestKubeadmControlPlaneValidateCreate(t *testing.T) {
 			expectErr: false,
 			kcp:       evenReplicasExternalEtcd,
 		},
-		{
-			name:      "should succeed when given a valid semantic version with prepended 'v'",
-			expectErr: false,
-			kcp:       validVersion1,
-		},
-		{
-			name:      "should succeed when given a valid semantic version without 'v'",
-			expectErr: false,
-			kcp:       validVersion2,
-		},
-		{
-			name:      "should return error when given an invalid semantic version",
-			expectErr: true,
-			kcp:       invalidVersion,
-		},
 	}
 
 	for _, tt := range tests {

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