-
Notifications
You must be signed in to change notification settings - Fork 38.7k
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
kubeadm: handle stable-1 as the default version #69301
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -78,6 +78,10 @@ func KubernetesReleaseVersion(version string) (string, error) { | |
|
||
// kubeReleaseLabelRegex matches labels such as: latest, latest-1, latest-1.10 | ||
if kubeReleaseLabelRegex.MatchString(versionLabel) { | ||
var clientVersion string | ||
// Try to obtain a client version. | ||
clientVersion, _ = kubeadmVersion(pkgversion.Get().String()) | ||
// Fetch version from the internet. | ||
url := fmt.Sprintf("%s/%s.txt", bucketURL, versionLabel) | ||
body, err := fetchFromURL(url, getReleaseVersionTimeout) | ||
if err != nil { | ||
|
@@ -87,11 +91,13 @@ func KubernetesReleaseVersion(version string) (string, error) { | |
} | ||
// Handle air-gapped environments by falling back to the client version. | ||
glog.Infof("could not fetch a Kubernetes version from the internet: %v", err) | ||
body, err = kubeadmVersion(pkgversion.Get().String()) | ||
if err != nil { | ||
return "", err | ||
} | ||
glog.Infof("falling back to the local client version: %s", body) | ||
glog.Infof("falling back to the local client version: %s", clientVersion) | ||
return KubernetesReleaseVersion(clientVersion) | ||
} | ||
// both the client and the remote version are obtained; validate them and pick a stable version | ||
body, err = validateStableVersion(body, clientVersion) | ||
if err != nil { | ||
return "", err | ||
} | ||
// Re-validate received version and return. | ||
return KubernetesReleaseVersion(body) | ||
|
@@ -204,3 +210,31 @@ func kubeadmVersion(info string) (string, error) { | |
vStr := fmt.Sprintf("v%d.%d.%d%s", v.Major(), v.Minor(), patch, pre) | ||
return vStr, nil | ||
} | ||
|
||
// Validate if the remote version is one Minor release newer than the client version. | ||
// 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) { | ||
if clientVersion == "" { | ||
glog.Infof("could not obtain client version; using remote version: %s", remoteVersion) | ||
return remoteVersion, nil | ||
} | ||
|
||
verRemote, err := versionutil.ParseGeneric(remoteVersion) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. thanks for the review but this was in flight for 10 days. i see what you are saying but in my tests i wasn't able to reach such a breakage. now, if There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sounds good! thanks. |
||
if err != nil { | ||
return "", fmt.Errorf("remote version error: %v", err) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. grumble ... errors.Wrap There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sure. |
||
} | ||
verClient, err := versionutil.ParseGeneric(clientVersion) | ||
if err != nil { | ||
return "", fmt.Errorf("client version error: %v", err) | ||
} | ||
// If the remote Major version is bigger or if the Major versions are the same, | ||
// but the remote Minor is bigger use the client version release. This handles Major bumps too. | ||
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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This almost seems like a bug. imo we need to switch back to stable-1.X in the 1.12 series bits. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
the above code should do just that. for 1.11.1 it falls back to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ahh I see that now. K, I'm good. |
||
return estimatedRelease, nil | ||
} | ||
return remoteVersion, nil | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
glog should be used with verbosity level, isn't it ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unless we want to always show the warning with file and line, etc, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see that as a critical warning, but ok.