From 8a03ae7d608f6dfd3befa7b08ee076253d8b804e Mon Sep 17 00:00:00 2001 From: Cecile Robert-Michon Date: Fri, 5 Jun 2020 15:00:31 -0700 Subject: [PATCH] =?UTF-8?q?=F0=9F=90=9B=20Make=20MachineSpec=20Version=20v?= =?UTF-8?q?alidation=20consistent=20with=20KCP?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- api/v1alpha3/machine_webhook.go | 11 ++++++++-- api/v1alpha3/machine_webhook_test.go | 6 +++-- .../machinedeployment_controller_test.go | 4 ++-- controllers/machineset_controller_test.go | 4 ++-- .../v1alpha3/kubeadm_control_plane_types.go | 2 -- .../kubeadm_control_plane_types_test.go | 16 -------------- .../v1alpha3/kubeadm_control_plane_webhook.go | 10 +++++++-- .../kubeadm_control_plane_webhook_test.go | 22 ++++++++++--------- ...cluster.x-k8s.io_kubeadmcontrolplanes.yaml | 2 -- test/e2e/common.go | 3 +-- 10 files changed, 38 insertions(+), 42 deletions(-) diff --git a/api/v1alpha3/machine_webhook.go b/api/v1alpha3/machine_webhook.go index f1bc587e581b..9b0c2d08d45d 100644 --- 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 index 5c29bbbe0c32..02dedf92b6b6 100644 --- 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 index 4df168676c73..98b3479254eb 100644 --- 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 index 9c94ccb6e539..e0ccddae8550 100644 --- 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 index 63690556aab2..f985772226b3 100644 --- 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 index 36e81284dff8..82890951dfe3 100644 --- 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 index 0b1ebfaea8e4..8cd653f0e15c 100644 --- 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 index 410d6a64f866..3afb02b1f591 100644 --- 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/controlplane/kubeadm/config/crd/bases/controlplane.cluster.x-k8s.io_kubeadmcontrolplanes.yaml b/controlplane/kubeadm/config/crd/bases/controlplane.cluster.x-k8s.io_kubeadmcontrolplanes.yaml index a12595b5fd25..ad1b756008d3 100644 --- a/controlplane/kubeadm/config/crd/bases/controlplane.cluster.x-k8s.io_kubeadmcontrolplanes.yaml +++ b/controlplane/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 index baad2876cca9..8ff9d98736b0 100644 --- 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