Skip to content

Commit

Permalink
🐛 Make MachineSpec Version validation consistent with KCP Version val…
Browse files Browse the repository at this point in the history
…idation
  • Loading branch information
Cecile Robert-Michon committed Jun 4, 2020
1 parent e72c3ae commit 85c5468
Show file tree
Hide file tree
Showing 12 changed files with 8 additions and 97 deletions.
1 change: 1 addition & 0 deletions api/v1alpha3/machine_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"`

Expand Down
8 changes: 0 additions & 8 deletions api/v1alpha3/machine_webhook.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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
}
Expand Down
55 changes: 0 additions & 55 deletions api/v1alpha3/machine_webhook_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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())
}
})
}
}
1 change: 1 addition & 0 deletions config/crd/bases/cluster.x-k8s.io_machinedeployments.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
1 change: 1 addition & 0 deletions config/crd/bases/cluster.x-k8s.io_machines.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
1 change: 1 addition & 0 deletions config/crd/bases/cluster.x-k8s.io_machinesets.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
1 change: 1 addition & 0 deletions config/crd/bases/exp.cluster.x-k8s.io_machinepools.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion controllers/machinedeployment_controller_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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-",
Expand Down
2 changes: 1 addition & 1 deletion controllers/machineset_controller_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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-",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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 {
Expand Down
3 changes: 1 addition & 2 deletions test/e2e/common.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ import (
"context"
"fmt"
"path/filepath"
"strings"

. "github.com/onsi/ginkgo"

Expand Down Expand Up @@ -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
Expand Down

0 comments on commit 85c5468

Please sign in to comment.