Skip to content

Commit

Permalink
🐛 CoreDNS validation should only look at major.minor.patch
Browse files Browse the repository at this point in the history 
Signed-off-by: Vince Prignano <[email protected]>
  • Loading branch information
@vincepri
vincepri committed Mar 17, 2020
1 parent 0e35461 commit 32e6e61
Show file tree
Hide file tree
Showing 4 changed files with 173 additions and 76 deletions.
60 changes: 34 additions & 26 deletions controlplane/kubeadm/internal/workload_cluster_coredns.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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"
)
Expand All @@ -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
Expand Down Expand Up @@ -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")
}

Expand Down Expand Up @@ -154,26 +159,28 @@ 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
}
toImageTag := imageRefTag.Tag()
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
}

Expand Down Expand Up @@ -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")
}
Expand Down Expand Up @@ -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
}
Expand All @@ -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())
Expand Down
111 changes: 61 additions & 50 deletions controlplane/kubeadm/internal/workload_cluster_coredns_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand All @@ -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)
Expand Down Expand Up @@ -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{
Expand All @@ -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
Expand Down Expand Up @@ -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))
Expand Down Expand Up @@ -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,
},
Expand Down
29 changes: 29 additions & 0 deletions util/util.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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)
Expand Down
Loading

0 comments on commit 32e6e61

Please sign in to comment.