Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

🐛 Make MachineSpec Version validation consistent with KCP #3147

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 9 additions & 2 deletions api/v1alpha3/machine_webhook.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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_\.+]*)?$`)
detiber marked this conversation as resolved.
Show resolved Hide resolved

// Default implements webhook.Defaulter so a webhook will be registered for the type
func (m *Machine) Default() {
if m.Labels == nil {
Expand All @@ -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
Expand Down Expand Up @@ -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"))
}
}
Expand Down
6 changes: 4 additions & 2 deletions api/v1alpha3/machine_webhook_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ func TestMachineDefault(t *testing.T) {
},
Spec: MachineSpec{
Bootstrap: Bootstrap{ConfigRef: &corev1.ObjectReference{}},
Version: pointer.StringPtr("1.17.5"),
},
}

Expand All @@ -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) {
Expand Down Expand Up @@ -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",
Expand Down
4 changes: 2 additions & 2 deletions 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 Expand Up @@ -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.
Expand Down
4 changes: 2 additions & 2 deletions 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 Expand Up @@ -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)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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 {
Expand All @@ -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
Expand Down Expand Up @@ -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"))
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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,
},
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
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