From 67618d859c58c7f568415da7bb91cd5bba099d83 Mon Sep 17 00:00:00 2001 From: Cecile Robert-Michon Date: Mon, 8 Feb 2021 17:55:50 -0800 Subject: [PATCH] :seedling: Clean up kube version parsing --- api/v1alpha4/machine_webhook.go | 8 +- .../v1alpha4/kubeadm_control_plane_webhook.go | 15 +-- .../kubeadm_control_plane_webhook_test.go | 2 +- .../internal/workload_cluster_coredns.go | 8 +- exp/api/v1alpha3/conversion.go | 2 +- util/util.go | 41 ++---- util/util_test.go | 48 ------- util/version/version.go | 74 +++++++++++ util/version/version_test.go | 124 ++++++++++++++++++ 9 files changed, 223 insertions(+), 99 deletions(-) create mode 100644 util/version/version.go create mode 100644 util/version/version_test.go diff --git a/api/v1alpha4/machine_webhook.go b/api/v1alpha4/machine_webhook.go index e4250a35108c..6ed4881641c4 100644 --- a/api/v1alpha4/machine_webhook.go +++ b/api/v1alpha4/machine_webhook.go @@ -18,11 +18,11 @@ package v1alpha4 import ( "fmt" - "regexp" + "sigs.k8s.io/cluster-api/util/version" "strings" apierrors "k8s.io/apimachinery/pkg/api/errors" - runtime "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/validation/field" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/webhook" @@ -40,8 +40,6 @@ 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 { @@ -124,7 +122,7 @@ func (m *Machine) validate(old *Machine) error { } if m.Spec.Version != nil { - if !kubeSemver.MatchString(*m.Spec.Version) { + if !version.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/controlplane/kubeadm/api/v1alpha4/kubeadm_control_plane_webhook.go b/controlplane/kubeadm/api/v1alpha4/kubeadm_control_plane_webhook.go index 35a4897d9a51..bdd5cdb7e0d0 100644 --- a/controlplane/kubeadm/api/v1alpha4/kubeadm_control_plane_webhook.go +++ b/controlplane/kubeadm/api/v1alpha4/kubeadm_control_plane_webhook.go @@ -19,7 +19,6 @@ package v1alpha4 import ( "encoding/json" "fmt" - "regexp" "strings" "github.com/blang/semver" @@ -30,8 +29,8 @@ import ( "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/validation/field" kubeadmv1 "sigs.k8s.io/cluster-api/bootstrap/kubeadm/types/v1beta1" - "sigs.k8s.io/cluster-api/util" "sigs.k8s.io/cluster-api/util/container" + "sigs.k8s.io/cluster-api/util/version" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/webhook" ) @@ -48,8 +47,6 @@ 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 { @@ -268,7 +265,7 @@ func (in *KubeadmControlPlane) validateCommon() (allErrs field.ErrorList) { ) } - if !kubeSemver.MatchString(in.Spec.Version) { + if !version.KubeSemver.MatchString(in.Spec.Version) { allErrs = append(allErrs, field.Invalid(field.NewPath("spec", "version"), in.Spec.Version, "must be a valid semantic version")) } @@ -308,7 +305,7 @@ func (in *KubeadmControlPlane) validateCoreDNSVersion(prev *KubeadmControlPlane) return allErrs } - fromVersion, err := util.ParseMajorMinorPatch(prev.Spec.KubeadmConfigSpec.ClusterConfiguration.DNS.ImageTag) + fromVersion, err := version.ParseMajorMinorPatchTolerant(prev.Spec.KubeadmConfigSpec.ClusterConfiguration.DNS.ImageTag) if err != nil { allErrs = append(allErrs, field.InternalError( @@ -319,7 +316,7 @@ func (in *KubeadmControlPlane) validateCoreDNSVersion(prev *KubeadmControlPlane) return allErrs } - toVersion, err := util.ParseMajorMinorPatch(targetDNS.ImageTag) + toVersion, err := version.ParseMajorMinorPatchTolerant(targetDNS.ImageTag) if err != nil { allErrs = append(allErrs, field.Invalid( @@ -397,7 +394,7 @@ func (in *KubeadmControlPlane) validateEtcd(prev *KubeadmControlPlane) (allErrs } func (in *KubeadmControlPlane) validateVersion(previousVersion string) (allErrs field.ErrorList) { - fromVersion, err := util.ParseMajorMinorPatch(previousVersion) + fromVersion, err := version.ParseMajorMinorPatch(previousVersion) if err != nil { allErrs = append(allErrs, field.InternalError( @@ -408,7 +405,7 @@ func (in *KubeadmControlPlane) validateVersion(previousVersion string) (allErrs return allErrs } - toVersion, err := util.ParseMajorMinorPatch(in.Spec.Version) + toVersion, err := version.ParseMajorMinorPatch(in.Spec.Version) if err != nil { allErrs = append(allErrs, field.InternalError( diff --git a/controlplane/kubeadm/api/v1alpha4/kubeadm_control_plane_webhook_test.go b/controlplane/kubeadm/api/v1alpha4/kubeadm_control_plane_webhook_test.go index d8238b772ff0..183017c66465 100644 --- a/controlplane/kubeadm/api/v1alpha4/kubeadm_control_plane_webhook_test.go +++ b/controlplane/kubeadm/api/v1alpha4/kubeadm_control_plane_webhook_test.go @@ -133,7 +133,7 @@ func TestKubeadmControlPlaneValidateCreate(t *testing.T) { kcp: validVersion, }, { - name: "should succeed when given a valid semantic version without 'v'", + name: "should error when given a valid semantic version without 'v'", expectErr: true, kcp: invalidVersion2, }, diff --git a/controlplane/kubeadm/internal/workload_cluster_coredns.go b/controlplane/kubeadm/internal/workload_cluster_coredns.go index 5df278b431b2..4c41ff1f2ef9 100644 --- a/controlplane/kubeadm/internal/workload_cluster_coredns.go +++ b/controlplane/kubeadm/internal/workload_cluster_coredns.go @@ -19,6 +19,7 @@ package internal import ( "context" "fmt" + "sigs.k8s.io/cluster-api/util/version" "github.com/coredns/corefile-migration/migration" "github.com/pkg/errors" @@ -28,7 +29,6 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" kubeadmv1 "sigs.k8s.io/cluster-api/bootstrap/kubeadm/types/v1beta1" controlplanev1 "sigs.k8s.io/cluster-api/controlplane/kubeadm/api/v1alpha4" - "sigs.k8s.io/cluster-api/util" containerutil "sigs.k8s.io/cluster-api/util/container" "sigs.k8s.io/cluster-api/util/patch" ctrlclient "sigs.k8s.io/controller-runtime/pkg/client" @@ -298,7 +298,7 @@ func patchCoreDNSDeploymentImage(deployment *appsv1.Deployment, image string) { } func extractImageVersion(tag string) (string, error) { - ver, err := util.ParseMajorMinorPatch(tag) + ver, err := version.ParseMajorMinorPatchTolerant(tag) if err != nil { return "", err } @@ -309,11 +309,11 @@ func extractImageVersion(tag string) (string, error) { // Some of the checks come from // https://github.com/coredns/corefile-migration/blob/v1.0.6/migration/migrate.go#L414 func validateCoreDNSImageTag(fromTag, toTag string) error { - from, err := util.ParseMajorMinorPatch(fromTag) + from, err := version.ParseMajorMinorPatchTolerant(fromTag) if err != nil { return errors.Wrapf(err, "failed to parse CoreDNS current version %q", fromTag) } - to, err := util.ParseMajorMinorPatch(toTag) + to, err := version.ParseMajorMinorPatchTolerant(toTag) if err != nil { return errors.Wrapf(err, "failed to parse CoreDNS target version %q", toTag) } diff --git a/exp/api/v1alpha3/conversion.go b/exp/api/v1alpha3/conversion.go index 8c8847d04172..865a30e1695f 100644 --- a/exp/api/v1alpha3/conversion.go +++ b/exp/api/v1alpha3/conversion.go @@ -24,4 +24,4 @@ import ( // Convert_v1alpha3_MachinePoolSpec_To_v1alpha4_MachinePoolSpec is an autogenerated conversion function. func Convert_v1alpha3_MachinePoolSpec_To_v1alpha4_MachinePoolSpec(in *MachinePoolSpec, out *v1alpha4.MachinePoolSpec, s conversion.Scope) error { return autoConvert_v1alpha3_MachinePoolSpec_To_v1alpha4_MachinePoolSpec(in, out, s) -} \ No newline at end of file +} diff --git a/util/util.go b/util/util.go index 3bd63668cedb..be402af7195d 100644 --- a/util/util.go +++ b/util/util.go @@ -22,8 +22,6 @@ import ( "fmt" "math" "math/rand" - "regexp" - "strconv" "strings" "time" @@ -38,7 +36,7 @@ import ( "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" - "k8s.io/apimachinery/pkg/version" + k8sversion "k8s.io/apimachinery/pkg/version" "k8s.io/client-go/metadata" "k8s.io/client-go/rest" "k8s.io/klog/klogr" @@ -46,6 +44,7 @@ import ( "sigs.k8s.io/cluster-api/util/annotations" "sigs.k8s.io/cluster-api/util/container" "sigs.k8s.io/cluster-api/util/predicates" + "sigs.k8s.io/cluster-api/util/version" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/apiutil" @@ -67,35 +66,8 @@ var ( rnd = rand.New(rand.NewSource(time.Now().UnixNano())) ErrNoCluster = fmt.Errorf("no %q label present", clusterv1.ClusterLabelName) ErrUnstructuredFieldNotFound = fmt.Errorf("field not found") - kubeSemver = regexp.MustCompile(`^v?(0|[1-9][0-9]*)\.(0|[1-9][0-9]*)\.(0|[1-9][0-9]*)([-0-9a-zA-Z_\.+]*)?$`) ) -// ParseMajorMinorPatch returns a semver.Version from the string provided -// by looking only at major.minor.patch and stripping everything else out. -func ParseMajorMinorPatch(version string) (semver.Version, error) { - groups := kubeSemver.FindStringSubmatch(version) - if len(groups) < 4 { - return semver.Version{}, errors.Errorf("failed to parse major.minor.patch from %q", version) - } - major, err := strconv.ParseUint(groups[1], 10, 64) - if err != nil { - return semver.Version{}, errors.Wrapf(err, "failed to parse major version from %q", version) - } - minor, err := strconv.ParseUint(groups[2], 10, 64) - if err != nil { - return semver.Version{}, errors.Wrapf(err, "failed to parse minor version from %q", version) - } - patch, err := strconv.ParseUint(groups[3], 10, 64) - if err != nil { - return semver.Version{}, errors.Wrapf(err, "failed to parse patch version from %q", version) - } - return semver.Version{ - Major: major, - Minor: minor, - Patch: patch, - }, nil -} - // RandomString returns a random alphanumeric string. func RandomString(n int) string { result := make([]byte, n) @@ -128,6 +100,13 @@ func Ordinalize(n int) string { return fmt.Sprintf("%d%s", n, m[an%10]) } +// ParseMajorMinorPatch returns a semver.Version from the string provided +// by looking only at major.minor.patch and stripping everything else out. +// Deprecated: Please use the function in util/version +func ParseMajorMinorPatch(v string) (semver.Version, error) { + return version.ParseMajorMinorPatchTolerant(v) +} + // ModifyImageRepository takes an imageName (e.g., repository/image:tag), and returns an image name with updated repository // Deprecated: Please use the functions in util/container func ModifyImageRepository(imageName, repositoryName string) (string, error) { @@ -597,7 +576,7 @@ type KubeAwareAPIVersions []string func (k KubeAwareAPIVersions) Len() int { return len(k) } func (k KubeAwareAPIVersions) Swap(i, j int) { k[i], k[j] = k[j], k[i] } func (k KubeAwareAPIVersions) Less(i, j int) bool { - return version.CompareKubeAwareVersionStrings(k[i], k[j]) < 0 + return k8sversion.CompareKubeAwareVersionStrings(k[i], k[j]) < 0 } // MachinesByCreationTimestamp sorts a list of Machine by creation timestamp, using their names as a tie breaker. diff --git a/util/util_test.go b/util/util_test.go index 5b0e3377615d..fd389597b2e6 100644 --- a/util/util_test.go +++ b/util/util_test.go @@ -39,54 +39,6 @@ var ( ctx = ctrl.SetupSignalHandler() ) -func TestParseMajorMinorPatch(t *testing.T) { - g := NewWithT(t) - - var testcases = []struct { - name string - input string - output semver.Version - expectError bool - }{ - { - name: "should parse an OCI compliant string", - input: "v1.2.16_foo-1", - output: semver.Version{ - Major: 1, - Minor: 2, - Patch: 16, - }, - }, - { - name: "should parse a valid semver", - input: "v1.16.6+foobar-0", - output: semver.Version{ - Major: 1, - Minor: 16, - Patch: 6, - }, - }, - { - name: "should error if there is no patch version", - input: "v1.16+foobar-0", - expectError: true, - }, - { - name: "should error if there is no minor and patch", - input: "v1+foobar-0", - expectError: true, - }, - } - - for _, tc := range testcases { - t.Run(tc.name, func(t *testing.T) { - out, err := ParseMajorMinorPatch(tc.input) - g.Expect(err != nil).To(Equal(tc.expectError)) - g.Expect(out).To(Equal(tc.output)) - }) - } -} - func TestMachineToInfrastructureMapFunc(t *testing.T) { g := NewWithT(t) diff --git a/util/version/version.go b/util/version/version.go new file mode 100644 index 000000000000..6e8d9c79a648 --- /dev/null +++ b/util/version/version.go @@ -0,0 +1,74 @@ +/* +Copyright 2021 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package version + +import ( + "github.com/blang/semver" + "github.com/pkg/errors" + "regexp" + "strconv" +) + +var ( + // KubeSemver is the regex for Kubernetes versions. It requires the "v" prefix. + KubeSemver = regexp.MustCompile(`^v(0|[1-9][0-9]*)\.(0|[1-9][0-9]*)\.(0|[1-9][0-9]*)([-0-9a-zA-Z_\.+]*)?$`) + // KubeSemverTolerant is the regex for Kubernetes versions with an optional "v" prefix. + KubeSemverTolerant = regexp.MustCompile(`^v?(0|[1-9][0-9]*)\.(0|[1-9][0-9]*)\.(0|[1-9][0-9]*)([-0-9a-zA-Z_\.+]*)?$`) +) + +// ParseMajorMinorPatch returns a semver.Version from the string provided +// by looking only at major.minor.patch and stripping everything else out. +// It requires the version to have a "v" prefix. +func ParseMajorMinorPatch(version string) (semver.Version, error) { + return parseMajorMinorPatch(version, false) +} + +// ParseMajorMinorPatchTolerant returns a semver.Version from the string provided +// by looking only at major.minor.patch and stripping everything else out. +// It does not require the version to have a "v" prefix. +func ParseMajorMinorPatchTolerant(version string) (semver.Version, error) { + return parseMajorMinorPatch(version, true) +} + +// parseMajorMinorPatch returns a semver.Version from the string provided +// by looking only at major.minor.patch and stripping everything else out. +func parseMajorMinorPatch(version string, tolerant bool) (semver.Version, error) { + groups := KubeSemver.FindStringSubmatch(version) + if tolerant { + groups = KubeSemverTolerant.FindStringSubmatch(version) + } + if len(groups) < 4 { + return semver.Version{}, errors.Errorf("failed to parse major.minor.patch from %q", version) + } + major, err := strconv.ParseUint(groups[1], 10, 64) + if err != nil { + return semver.Version{}, errors.Wrapf(err, "failed to parse major version from %q", version) + } + minor, err := strconv.ParseUint(groups[2], 10, 64) + if err != nil { + return semver.Version{}, errors.Wrapf(err, "failed to parse minor version from %q", version) + } + patch, err := strconv.ParseUint(groups[3], 10, 64) + if err != nil { + return semver.Version{}, errors.Wrapf(err, "failed to parse patch version from %q", version) + } + return semver.Version{ + Major: major, + Minor: minor, + Patch: patch, + }, nil +} diff --git a/util/version/version_test.go b/util/version/version_test.go new file mode 100644 index 000000000000..40d5a163a823 --- /dev/null +++ b/util/version/version_test.go @@ -0,0 +1,124 @@ +/* +Copyright 2021 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package version + +import ( + "github.com/blang/semver" + . "github.com/onsi/gomega" + "testing" +) + +func TestParseMajorMinorPatch(t *testing.T) { + g := NewWithT(t) + + var testcases = []struct { + name string + input string + output semver.Version + expectError bool + }{ + { + name: "should parse an OCI compliant string", + input: "v1.2.16_foo-1", + output: semver.Version{ + Major: 1, + Minor: 2, + Patch: 16, + }, + }, + { + name: "should parse a valid semver", + input: "v1.16.6+foobar-0", + output: semver.Version{ + Major: 1, + Minor: 16, + Patch: 6, + }, + }, + { + name: "should error if there is no patch version", + input: "v1.16+foobar-0", + expectError: true, + }, + { + name: "should error if there is no minor and patch", + input: "v1+foobar-0", + expectError: true, + }, + { + name: "should error if there is no v prefix", + input: "1.4.7", + expectError: true, + }, + } + + for _, tc := range testcases { + t.Run(tc.name, func(t *testing.T) { + out, err := ParseMajorMinorPatch(tc.input) + g.Expect(err != nil).To(Equal(tc.expectError)) + g.Expect(out).To(Equal(tc.output)) + }) + } +} + +func TestParseMajorMinorPatchTolerant(t *testing.T) { + g := NewWithT(t) + + var testcases = []struct { + name string + input string + output semver.Version + expectError bool + }{ + { + name: "should parse an OCI compliant string", + input: "v1.2.16_foo-1", + output: semver.Version{ + Major: 1, + Minor: 2, + Patch: 16, + }, + }, + { + name: "should parse a valid semver with no v prefix", + input: "1.16.6+foobar-0", + output: semver.Version{ + Major: 1, + Minor: 16, + Patch: 6, + }, + }, + { + name: "should error if there is no patch version", + input: "1.16+foobar-0", + expectError: true, + }, + { + name: "should error if there is no minor and patch", + input: "1+foobar-0", + expectError: true, + }, + } + + for _, tc := range testcases { + t.Run(tc.name, func(t *testing.T) { + out, err := ParseMajorMinorPatchTolerant(tc.input) + g.Expect(err != nil).To(Equal(tc.expectError)) + g.Expect(out).To(Equal(tc.output)) + }) + } +}