kubeadm: handle stable-1 as the default version #69301

neolit123 · 2018-10-01T18:53:21Z

What this PR does / why we need it:
The default version in kubeadm is now stable-1. This will
pull a version from the stable-1.txt endpoint which might
end up being newer than the version of the client by a magnitude
of MINOR or even a MAJOR release.

To be able to prevent this scenario add the new helper function:
validateStableVersion()

This function determines if the remote version is newer than the
local client version and if that's the case it returns stable-X.xx
that conforms with the version of the client. If not it returns
the remote version.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes kubernetes/kubeadm#1141

Special notes for your reviewer:

the unit tests can illustrate what the function does.
NOTE: we need to backport this to 1.12 !!!

Release note:

kubeadm: fix a possible scenario where kubeadm can pull much newer control-plane images

/kind bug
/assign @kad @timothysc
@kubernetes/sig-cluster-lifecycle-pr-reviews

neolit123 · 2018-10-01T19:24:02Z

looks like the remote CI tests don't populate a client version properly when building the kubeadm binary....
"v0.0.0-master+$Format:%h$"

this would require a workaround.

neolit123 · 2018-10-01T20:10:50Z

/hold

neolit123 · 2018-10-01T20:13:17Z

cmd/kubeadm/app/util/version.go

+		if _, err := versionutil.ParseSemantic(pkgversion.Get().String()); err == nil {
+			// ignore the error as kubeadmVersion() cannot fail at this point
+			clientVersion, _ = kubeadmVersion(pkgversion.Get().String())
+		}


i'm not very happy with how this is turning out.
will revise tomorrow. the idea is to always get some sort of a client version: A) a valid one, B) empty string.

might as well remove the error check from kubeadmVersion() and adjust related sub-unit-tests.

timothysc · 2018-10-03T19:44:51Z

cmd/kubeadm/app/util/version.go

+// This is done to conform with "stable-X" and only allow remote versions from
+// the same Patch level release.
+func validateStableVersion(remoteVersion, clientVersion string) (string, error) {
+	const errorFormat = "validateStableVersion(): %s version error: %v"


Should really just use errors.wrap below, this is kind of weird.

timothysc · 2018-10-04T15:00:16Z

cmd/kubeadm/app/util/version.go

+	if verClient.Major() < verRemote.Major() ||
+		(verClient.Major() == verRemote.Major()) && verClient.Minor() < verRemote.Minor() {
+		estimatedRelease := fmt.Sprintf("stable-%d.%d", verClient.Major(), verClient.Minor())
+		glog.Infof("remote version is much newer: %s; falling back to: %s", remoteVersion, estimatedRelease)


This almost seems like a bug.
If a person has 1.11.1 download, they should be able to get the latest 1.11.y and it should just work, if we fallback that's almost unexpected behavior.

imo we need to switch back to stable-1.X in the 1.12 series bits.

If a person has 1.11.1 download, they should be able to get the latest 1.11.y

the above code should do just that.

for 1.11.1 it falls back to stable-1.11, which can be e.g. 1.11.5.

Ahh I see that now. K, I'm good.

timothysc

/approve

just fix up the error handling then lgtm

k8s-ci-robot · 2018-10-04T16:28:20Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: neolit123, timothysc

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~cmd/kubeadm/OWNERS~~ [timothysc]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

The default version in kubeadm is now `stable-1`. This will pull a version from the `stable-1.txt` endpoint which might end up being newer than the version of the client by a magnitude of MINOR or even a MAJOR release. To be able to prevent this scenario add the new helper function: validateStableVersion() This function determines if the remote version is newer than the local client version and if that's the case it returns `stable-X.xx` that conforms with the version of the client. If not it returns the remote version.

neolit123 · 2018-10-04T21:04:09Z

updated.
errors simplified to: fmt.Errorf("client version error: %v", err)

neolit123 · 2018-10-05T20:09:24Z

we possibly want this for .2 at least if not for .1.

timothysc

/lgtm

please use error.Wrap in subsequent PRs.

timothysc · 2018-10-10T15:04:30Z

cmd/kubeadm/app/util/version.go

+
+	verRemote, err := versionutil.ParseGeneric(remoteVersion)
+	if err != nil {
+		return "", fmt.Errorf("remote version error: %v", err)


grumble ... errors.Wrap

sure.
i don't like that it's not part of golang yet, but we can possibly SED them all at once.

neolit123 · 2018-10-10T15:30:11Z

/hold cancel

kad · 2018-10-10T15:58:56Z

cmd/kubeadm/app/util/version.go

+// the same Patch level release.
+func validateStableVersion(remoteVersion, clientVersion string) (string, error) {
+	if clientVersion == "" {
+		glog.Infof("could not obtain client version; using remote version: %s", remoteVersion)


glog should be used with verbosity level, isn't it ?

unless we want to always show the warning with file and line, etc, no?

I don't see that as a critical warning, but ok.

kad · 2018-10-10T16:05:38Z

cmd/kubeadm/app/util/version.go

+		return remoteVersion, nil
+	}
+
+	verRemote, err := versionutil.ParseGeneric(remoteVersion)


In theory, here we have content of remote file. right now, it is supposed to be exact version. but the idea of recursive validation was that if label 'stable-1' will be resolved to another label, e.g. 'stable-1.12', this function will try to resolve one more remote label. If this function will expect that remote content is always parsable version, it will break previous functionality.

thanks for the review but this was in flight for 10 days.
also just created the cherry pick for 1.12.

i see what you are saying but in my tests i wasn't able to reach such a breakage.
we do fall-back to stable-1.12, if stable-1 is much newer, recurse and it works fine.

now, if stable-1 returns a non-semantic version as contents, that's really a breakage with the endpoint from my perspective.

Sorry for delay with commenting on that, somehow screwed up notifications on this PR :(

Yes, that scenario is not covered by tests. It was previously implicitly working with nesting of the labels. Support for nested label resolution is not critical, but nice to have. I can do separate PR for that later on.

sounds good! thanks.

neolit123 · 2018-10-10T16:56:53Z

/retest

…301-origin-release-1.12 Automated cherry pick of #69301: kubeadm: handle stable-1 as the default version

k8s-ci-robot added the release-note-none Denotes a PR that doesn't merit a release note. label Oct 1, 2018

k8s-ci-robot assigned kad and timothysc Oct 1, 2018

k8s-ci-robot requested review from luxas and xiangpengzhao October 1, 2018 18:53

k8s-ci-robot added the area/kubeadm label Oct 1, 2018

neolit123 force-pushed the kubeadm-version-stable branch from ea1ec80 to 70bceaa Compare October 1, 2018 20:03

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 1, 2018

neolit123 commented Oct 1, 2018

View reviewed changes

neolit123 changed the title ~~kubeadm: handle stable-1 as the default version~~ [WIP]: kubeadm: handle stable-1 as the default version Oct 1, 2018

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 1, 2018

neolit123 force-pushed the kubeadm-version-stable branch from 70bceaa to 911ac67 Compare October 2, 2018 18:55

neolit123 changed the title ~~[WIP]: kubeadm: handle stable-1 as the default version~~ kubeadm: handle stable-1 as the default version Oct 4, 2018

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 4, 2018

timothysc reviewed Oct 4, 2018

View reviewed changes

timothysc approved these changes Oct 4, 2018

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 4, 2018

neolit123 force-pushed the kubeadm-version-stable branch from 911ac67 to 5054135 Compare October 4, 2018 21:03

timothysc approved these changes Oct 10, 2018

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 10, 2018

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 10, 2018

neolit123 mentioned this pull request Oct 10, 2018

Automated cherry pick of #69301: kubeadm: handle stable-1 as the default version #69629

Merged

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Oct 10, 2018

kad reviewed Oct 10, 2018

View reviewed changes

k8s-ci-robot merged commit 77742ea into kubernetes:master Oct 10, 2018

k8s-ci-robot added a commit that referenced this pull request Oct 11, 2018

Merge pull request #69629 from neolit123/automated-cherry-pick-of-#69…

b072a98

…301-origin-release-1.12 Automated cherry pick of #69301: kubeadm: handle stable-1 as the default version

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kubeadm: handle stable-1 as the default version #69301

kubeadm: handle stable-1 as the default version #69301

neolit123 commented Oct 1, 2018 •

edited

neolit123 commented Oct 1, 2018

neolit123 commented Oct 1, 2018

neolit123 Oct 1, 2018

timothysc Oct 3, 2018

neolit123 Oct 4, 2018

timothysc Oct 4, 2018

neolit123 Oct 4, 2018 •

edited

timothysc Oct 4, 2018

timothysc left a comment

k8s-ci-robot commented Oct 4, 2018

neolit123 commented Oct 4, 2018

neolit123 commented Oct 5, 2018

timothysc left a comment

timothysc Oct 10, 2018

neolit123 Oct 10, 2018

neolit123 commented Oct 10, 2018

kad Oct 10, 2018

neolit123 Oct 10, 2018

kad Oct 10, 2018

kad Oct 10, 2018

neolit123 Oct 10, 2018 •

edited

kad Oct 10, 2018

neolit123 Oct 10, 2018

neolit123 commented Oct 10, 2018

kubeadm: handle stable-1 as the default version #69301

kubeadm: handle stable-1 as the default version #69301

Conversation

neolit123 commented Oct 1, 2018 • edited

neolit123 commented Oct 1, 2018

neolit123 commented Oct 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neolit123 Oct 4, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timothysc left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Oct 4, 2018

neolit123 commented Oct 4, 2018

neolit123 commented Oct 5, 2018

timothysc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neolit123 commented Oct 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neolit123 Oct 10, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neolit123 commented Oct 10, 2018

neolit123 commented Oct 1, 2018 •

edited

neolit123 Oct 4, 2018 •

edited

neolit123 Oct 10, 2018 •

edited