From 32e6e61a254c170095c85816fb0ea0f6d575bb89 Mon Sep 17 00:00:00 2001 From: Vince Prignano Date: Tue, 17 Mar 2020 14:27:03 -0700 Subject: [PATCH] :bug: CoreDNS validation should only look at major.minor.patch Signed-off-by: Vince Prignano --- .../internal/workload_cluster_coredns.go | 60 ++++++---- .../internal/workload_cluster_coredns_test.go | 111 ++++++++++-------- util/util.go | 29 +++++ util/util_test.go | 49 ++++++++ 4 files changed, 173 insertions(+), 76 deletions(-) diff --git a/controlplane/kubeadm/internal/workload_cluster_coredns.go b/controlplane/kubeadm/internal/workload_cluster_coredns.go index 5a0aa2ba9b00..dbc71d55deeb 100644 --- a/controlplane/kubeadm/internal/workload_cluster_coredns.go +++ b/controlplane/kubeadm/internal/workload_cluster_coredns.go @@ -20,7 +20,6 @@ import ( "context" "fmt" - "github.com/blang/semver" "github.com/coredns/corefile-migration/migration" "github.com/docker/distribution/reference" "github.com/pkg/errors" @@ -30,6 +29,7 @@ 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/v1alpha3" + "sigs.k8s.io/cluster-api/util" "sigs.k8s.io/cluster-api/util/patch" ctrlclient "sigs.k8s.io/controller-runtime/pkg/client" ) @@ -52,12 +52,17 @@ func (c *CoreDNSMigrator) Migrate(fromCoreDNSVersion, toCoreDNSVersion, corefile } type coreDNSInfo struct { - Corefile string - Deployment *appsv1.Deployment - FromParsedVersion string - ToParsedVersion string - FromImage string - ToImage string + Corefile string + Deployment *appsv1.Deployment + + FromImageTag string + ToImageTag string + + CurrentMajorMinorPatch string + TargetMajorMinorPatch string + + FromImage string + ToImage string } // UpdateCoreDNS updates the kubeadm configmap, coredns corefile and coredns @@ -90,7 +95,7 @@ func (w *Workload) UpdateCoreDNS(ctx context.Context, kcp *controlplanev1.Kubead } // Validate the image tag. - if err := validateCoreDNSImageTag(info.FromParsedVersion, kcp.Spec.KubeadmConfigSpec.ClusterConfiguration.DNS.ImageTag); err != nil { + if err := validateCoreDNSImageTag(info.FromImageTag, info.ToImageTag); err != nil { return errors.Wrapf(err, "failed to validate CoreDNS") } @@ -154,7 +159,7 @@ func (w *Workload) getCoreDNSInfo(ctx context.Context, dns *kubeadmv1.DNS) (*cor if !ok { return nil, errors.Errorf("failed to update coredns deployment: does not have a valid image tag: %q", container.Image) } - fromParsedVersion, err := extractImageVersion(imageRefTag.Tag()) + currentMajorMinorPatch, err := extractImageVersion(imageRefTag.Tag()) if err != nil { return nil, err } @@ -162,18 +167,20 @@ func (w *Workload) getCoreDNSInfo(ctx context.Context, dns *kubeadmv1.DNS) (*cor if dns.ImageTag != "" { toImageTag = dns.ImageTag } - toParsedVersion, err := extractImageVersion(toImageTag) + targetMajorMinorPatch, err := extractImageVersion(toImageTag) if err != nil { return nil, err } return &coreDNSInfo{ - Corefile: corefile, - Deployment: deployment, - FromParsedVersion: fromParsedVersion, - ToParsedVersion: toParsedVersion, - FromImage: container.Image, - ToImage: fmt.Sprintf("%s:%s", toImageRepository, toImageTag), + Corefile: corefile, + Deployment: deployment, + CurrentMajorMinorPatch: currentMajorMinorPatch, + TargetMajorMinorPatch: targetMajorMinorPatch, + FromImageTag: imageRefTag.Tag(), + ToImageTag: toImageTag, + FromImage: container.Image, + ToImage: fmt.Sprintf("%s:%s", toImageRepository, toImageTag), }, nil } @@ -215,7 +222,7 @@ func (w *Workload) updateCoreDNSImageInfoInKubeadmConfigMap(ctx context.Context, func (w *Workload) updateCoreDNSCorefile(ctx context.Context, info *coreDNSInfo) error { // Run the CoreDNS migration tool first because if it cannot migrate the // corefile, then there's no point in continuing further. - updatedCorefile, err := w.CoreDNSMigrator.Migrate(info.FromParsedVersion, info.ToParsedVersion, info.Corefile, false) + updatedCorefile, err := w.CoreDNSMigrator.Migrate(info.CurrentMajorMinorPatch, info.TargetMajorMinorPatch, info.Corefile, false) if err != nil { return errors.Wrap(err, "unable to migrate CoreDNS corefile") } @@ -283,7 +290,7 @@ func patchCoreDNSDeploymentImage(deployment *appsv1.Deployment, image string) { } func extractImageVersion(tag string) (string, error) { - ver, err := semver.Parse(tag) + ver, err := util.ParseMajorMinorPatch(tag) if err != nil { return "", err } @@ -293,19 +300,20 @@ func extractImageVersion(tag string) (string, error) { // validateCoreDNSImageTag returns error if the versions don't meet requirements. // Some of the checks come from // https://github.com/coredns/corefile-migration/blob/v1.0.6/migration/migrate.go#L414 -func validateCoreDNSImageTag(fromVersion, toVersion string) error { - from, err := semver.Parse(fromVersion) +func validateCoreDNSImageTag(fromTag, toTag string) error { + from, err := util.ParseMajorMinorPatch(fromTag) if err != nil { - return errors.Wrapf(err, "failed to semver parse %q", fromVersion) + return errors.Wrapf(err, "failed to parse CoreDNS current version %q", fromTag) } - to, err := semver.Parse(toVersion) + to, err := util.ParseMajorMinorPatch(toTag) if err != nil { - return errors.Wrapf(err, "failed to semver parse %q", toVersion) + return errors.Wrapf(err, "failed to parse CoreDNS target version %q", toTag) } - if from.Compare(to) >= 0 { - return fmt.Errorf("toVersion %q must be greater than fromVersion %q", to.String(), from.String()) + // make sure that the version we're upgrading to is greater than the current one, + // or if they're the same version, the raw tags should be different (e.g. allow from `v1.17.4-somevendor.0` to `v1.17.4-somevendor.1`). + if x := from.Compare(to); x > 0 || (x == 0 && fromTag == toTag) { + return fmt.Errorf("toVersion %q must be greater than fromVersion %q", toTag, fromTag) } - // check if the from version is even in the list of coredns versions if _, ok := migration.Versions[fmt.Sprintf("%d.%d.%d", from.Major, from.Minor, from.Patch)]; !ok { return fmt.Errorf("fromVersion %q is not a compatible coredns version", from.String()) diff --git a/controlplane/kubeadm/internal/workload_cluster_coredns_test.go b/controlplane/kubeadm/internal/workload_cluster_coredns_test.go index 6c2eff5165de..52e5eb724abc 100644 --- a/controlplane/kubeadm/internal/workload_cluster_coredns_test.go +++ b/controlplane/kubeadm/internal/workload_cluster_coredns_test.go @@ -52,35 +52,44 @@ func TestValidateCoreDNSImageTag(t *testing.T) { toVer: "1.6.3", expectErrSubStr: "not a compatible coredns version", }, - { - name: "toVer is not a valid coredns version format", - fromVer: "1.6.1", - toVer: "v1.6.2", - expectErrSubStr: "failed to semver parse", - }, { name: "toVer is not a valid semver", fromVer: "1.5.1", toVer: "foobar", - expectErrSubStr: "failed to semver parse", + expectErrSubStr: "failed to parse CoreDNS target version", }, { name: "fromVer is not a valid semver", fromVer: "foobar", toVer: "1.6.1", - expectErrSubStr: "failed to semver parse", + expectErrSubStr: "failed to parse CoreDNS current version", }, { - name: "fromVer is equal to toVer, return false because there's no need to upgrade", - fromVer: "1.6.5", - toVer: "1.6.5", - expectErrSubStr: "must be greater than", + name: "fromVer is equal to toVer, but different patch versions", + fromVer: "1.6.5_foobar.1", + toVer: "1.6.5_foobar.2", + }, + { + name: "fromVer is equal to toVer", + fromVer: "1.6.5_foobar.1", + toVer: "1.6.5_foobar.1", + expectErrSubStr: "must be greater", }, { name: "fromVer is lower but has meta", fromVer: "1.6.5-foobar.1", toVer: "1.7.5", }, + { + name: "fromVer is lower and has meta and leading v", + fromVer: "v1.6.5-foobar.1", + toVer: "1.7.5", + }, + { + name: "fromVer is lower, toVer has meta and leading v", + fromVer: "1.6.5-foobar.1", + toVer: "v1.7.5_foobar.1", + }, } for _, tt := range tests { @@ -156,10 +165,10 @@ func TestUpdateCoreDNSCorefile(t *testing.T) { } info := &coreDNSInfo{ - Corefile: "updated-core-file", - Deployment: depl, - FromParsedVersion: "1.6.2", - ToParsedVersion: "1.7.2", + Corefile: "updated-core-file", + Deployment: depl, + CurrentMajorMinorPatch: "1.6.2", + TargetMajorMinorPatch: "1.7.2", } err := w.updateCoreDNSCorefile(context.TODO(), info) @@ -188,10 +197,10 @@ func TestUpdateCoreDNSCorefile(t *testing.T) { } info := &coreDNSInfo{ - Corefile: originalCorefile, - Deployment: depl, - FromParsedVersion: currentImageTag, - ToParsedVersion: "1.7.2", + Corefile: originalCorefile, + Deployment: depl, + CurrentMajorMinorPatch: currentImageTag, + TargetMajorMinorPatch: "1.7.2", } err := w.updateCoreDNSCorefile(context.TODO(), info) @@ -218,10 +227,10 @@ func TestUpdateCoreDNSCorefile(t *testing.T) { } info := &coreDNSInfo{ - Corefile: originalCorefile, - Deployment: depl, - FromParsedVersion: currentImageTag, - ToParsedVersion: "1.7.2", + Corefile: originalCorefile, + Deployment: depl, + CurrentMajorMinorPatch: currentImageTag, + TargetMajorMinorPatch: "1.7.2", } err := w.updateCoreDNSCorefile(context.TODO(), info) @@ -305,7 +314,7 @@ func TestGetCoreDNSInfo(t *testing.T) { noTagContainerDepl.Spec.Template.Spec.Containers[0].Image = "k8s.gcr.io/coredns" badSemverContainerDepl := depl.DeepCopy() - badSemverContainerDepl.Spec.Template.Spec.Containers[0].Image = "k8s.gcr.io/coredns:v1.6.2" + badSemverContainerDepl.Spec.Template.Spec.Containers[0].Image = "k8s.gcr.io/coredns:v1X6.2" dns := &kubeadmv1.DNS{ ImageMeta: kubeadmv1.ImageMeta{ @@ -314,7 +323,7 @@ func TestGetCoreDNSInfo(t *testing.T) { }, } badImgTagDNS := dns.DeepCopy() - badImgTagDNS.ImageTag = "v1.7.2-foobar.1" + badImgTagDNS.ImageTag = "v1X6.2-foobar.1" tests := []struct { name string @@ -399,12 +408,14 @@ func TestGetCoreDNSInfo(t *testing.T) { } g.Expect(err).ToNot(HaveOccurred()) expectedInfo := &coreDNSInfo{ - Corefile: expectedCorefile, - Deployment: actualDepl, - FromParsedVersion: "1.6.2", - ToParsedVersion: "1.7.2", - FromImage: expectedImage, - ToImage: "myrepo/coredns:1.7.2-foobar.1", + Corefile: expectedCorefile, + Deployment: actualDepl, + CurrentMajorMinorPatch: "1.6.2", + TargetMajorMinorPatch: "1.7.2", + FromImage: expectedImage, + ToImage: "myrepo/coredns:1.7.2-foobar.1", + FromImageTag: "1.6.2", + ToImageTag: "1.7.2-foobar.1", } g.Expect(actualInfo).To(Equal(expectedInfo)) @@ -553,36 +564,36 @@ func TestUpdateCoreDNSDeployment(t *testing.T) { name: "patches coredns deployment successfully", objs: []runtime.Object{depl}, info: &coreDNSInfo{ - Deployment: depl.DeepCopy(), - Corefile: "updated-core-file", - FromImage: "k8s.gcr.io/coredns:1.6.2", - ToImage: "myrepo/mycoredns:1.7.2-foobar.1", - FromParsedVersion: "1.6.2", - ToParsedVersion: "1.7.2", + Deployment: depl.DeepCopy(), + Corefile: "updated-core-file", + FromImage: "k8s.gcr.io/coredns:1.6.2", + ToImage: "myrepo/mycoredns:1.7.2-foobar.1", + CurrentMajorMinorPatch: "1.6.2", + TargetMajorMinorPatch: "1.7.2", }, }, { name: "returns error if patch fails", objs: []runtime.Object{}, info: &coreDNSInfo{ - Deployment: depl.DeepCopy(), - Corefile: "updated-core-file", - FromImage: "k8s.gcr.io/coredns:1.6.2", - ToImage: "myrepo/mycoredns:1.7.2-foobar.1", - FromParsedVersion: "1.6.2", - ToParsedVersion: "1.7.2", + Deployment: depl.DeepCopy(), + Corefile: "updated-core-file", + FromImage: "k8s.gcr.io/coredns:1.6.2", + ToImage: "myrepo/mycoredns:1.7.2-foobar.1", + CurrentMajorMinorPatch: "1.6.2", + TargetMajorMinorPatch: "1.7.2", }, expectErr: true, }, { name: "deployment is nil for some reason", info: &coreDNSInfo{ - Deployment: nil, - Corefile: "updated-core-file", - FromImage: "k8s.gcr.io/coredns:1.6.2", - ToImage: "myrepo/mycoredns:1.7.2-foobar.1", - FromParsedVersion: "1.6.2", - ToParsedVersion: "1.7.2", + Deployment: nil, + Corefile: "updated-core-file", + FromImage: "k8s.gcr.io/coredns:1.6.2", + ToImage: "myrepo/mycoredns:1.7.2-foobar.1", + CurrentMajorMinorPatch: "1.6.2", + TargetMajorMinorPatch: "1.7.2", }, expectErr: true, }, diff --git a/util/util.go b/util/util.go index bb55790fe603..edd72100303f 100644 --- a/util/util.go +++ b/util/util.go @@ -22,9 +22,11 @@ import ( "fmt" "math/rand" "regexp" + "strconv" "strings" "time" + "github.com/blang/semver" "github.com/docker/distribution/reference" "github.com/pkg/errors" v1 "k8s.io/api/core/v1" @@ -53,8 +55,35 @@ var ( ErrNoCluster = fmt.Errorf("no %q label present", clusterv1.ClusterLabelName) ErrUnstructuredFieldNotFound = fmt.Errorf("field not found") ociTagAllowedChars = regexp.MustCompile(`[^-a-zA-Z0-9_\.]`) + 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) diff --git a/util/util_test.go b/util/util_test.go index 89f3a66fb24b..8fc9e94b6905 100644 --- a/util/util_test.go +++ b/util/util_test.go @@ -20,6 +20,7 @@ import ( "context" "testing" + "github.com/blang/semver" . "github.com/onsi/gomega" corev1 "k8s.io/api/core/v1" @@ -34,6 +35,54 @@ import ( "sigs.k8s.io/controller-runtime/pkg/reconcile" ) +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)