kubeadm:Clean up no longer used k8sVersion param #54982

xiangpengzhao · 2017-11-02T06:31:25Z

What this PR does / why we need it:
cleanup for kubeadm.

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

Special notes for your reviewer:
/cc @luxas

Release note:

NONE

~~kubeadm join now requires the --discovery-token-ca-cert-hash argument to be set, or the --discovery-token-unsafe-skip-ca-verification flag to be set for opting out of the CA pinning feature.~~

xiangpengzhao · 2017-11-08T15:05:02Z

/assign @krousey @luxas

luxas

Some cleanup comments

luxas · 2017-11-09T13:43:19Z

cmd/kubeadm/app/cmd/phases/bootstraptoken.go

@@ -119,10 +119,10 @@ func NewSubCmdNodeBootstrapTokenAutoApprove(kubeConfigFile *string) *cobra.Comma
 			client, err := kubeconfigutil.ClientSetFromFile(*kubeConfigFile)
 			kubeadmutil.CheckErr(err)

-			clusterVersion, err := getClusterVersion(client)


you can now remove this function completely as it's not needed anymore

luxas · 2017-11-09T13:43:34Z

cmd/kubeadm/app/cmd/phases/bootstraptoken.go

@@ -100,10 +100,10 @@ func NewSubCmdNodeBootstrapTokenPostCSRs(kubeConfigFile *string) *cobra.Command
 			client, err := kubeconfigutil.ClientSetFromFile(*kubeConfigFile)
 			kubeadmutil.CheckErr(err)

-			clusterVersion, err := getClusterVersion(client)


these two lines can now be removed

luxas · 2017-11-09T13:44:55Z

cmd/kubeadm/app/cmd/init.go

@@ -277,8 +277,7 @@ func (i *Init) Validate(cmd *cobra.Command) error {

 // Run executes master node provisioning, including certificates, needed static pod manifests, etc.
 func (i *Init) Run(out io.Writer) error {
-
-	k8sVersion, err := version.ParseSemantic(i.cfg.KubernetesVersion)


you can now remove this call completely

luxas · 2017-11-09T13:45:52Z

cmd/kubeadm/app/apis/kubeadm/validation/validation.go

-	if len(cfg.DiscoveryFile) == 0 && len(cfg.DiscoveryTokenCACertHashes) == 0 && !cfg.DiscoveryTokenUnsafeSkipCAVerification {
-		fmt.Println("[validation] WARNING: using token-based discovery without DiscoveryTokenCACertHashes can be unsafe (see https://kubernetes.io/docs/admin/kubeadm/#kubeadm-join).")
-		fmt.Println("[validation] WARNING: Pass --discovery-token-unsafe-skip-ca-verification to disable this warning. This warning will become an error in Kubernetes 1.9.")
+	if len(cfg.DiscoveryFile) == 0 && len(cfg.DiscoveryToken) != 0 && len(cfg.DiscoveryTokenCACertHashes) == 0 && !cfg.DiscoveryTokenUnsafeSkipCAVerification {


the logic seems to change here? why add len(cfg.DiscoveryToken) != 0 here?
cc @mattmoyer

IIUC, only when the conditions having len(cfg.DiscoveryToken) != 0 means "using token-based discovery" as is mentioned in the error message.

luxas · 2017-11-09T13:46:06Z

cmd/kubeadm/app/apis/kubeadm/validation/validation.go

-		fmt.Println("[validation] WARNING: using token-based discovery without DiscoveryTokenCACertHashes can be unsafe (see https://kubernetes.io/docs/admin/kubeadm/#kubeadm-join).")
-		fmt.Println("[validation] WARNING: Pass --discovery-token-unsafe-skip-ca-verification to disable this warning. This warning will become an error in Kubernetes 1.9.")
+	if len(cfg.DiscoveryFile) == 0 && len(cfg.DiscoveryToken) != 0 && len(cfg.DiscoveryTokenCACertHashes) == 0 && !cfg.DiscoveryTokenUnsafeSkipCAVerification {
+		allErrs = append(allErrs, field.Invalid(fldPath, "",


"discovery-token-unsafe-skip-ca-verification"

Thanks @luxas! Will address this and other comments tomorrow :)

@luxas other comments addressed. But I'm a bit confusing here now. What were you expecting here to be? allErrs = append(allErrs, field.Invalid(fldPath, "discovery-token-unsafe-skip-ca-verification", or something else?

luxas

/lgtm
/approve

luxas · 2017-11-10T14:07:06Z

/retest

luxas · 2017-11-10T14:09:22Z

/hold

We need to update the e2e tests first: https://github.com/kubernetes/kubernetes-anywhere/blob/master/phase1/gce/configure-vm-kubeadm.sh#L106 in order to get a green run of the kubeadm e2e CI presubmit job that now is failing: https://k8s-gubernator.appspot.com/build/kubernetes-jenkins/pr-logs/pull/54982/pull-kubernetes-e2e-kubeadm-gce/9008/

xiangpengzhao · 2017-11-10T16:06:47Z

@luxas PR kubernetes-retired/kubernetes-anywhere#483 sent. PTAL. Thanks!

mattmoyer · 2017-11-14T14:14:10Z

This overlaps with #55468 (pending approval).

xiangpengzhao · 2017-11-15T01:53:57Z

@mattmoyer thanks for pointing it out! ~~I will rebase after #55468 merged.~~[UPDATE: I remove the overlapped part in favor of #55468] But I guess #55468 also needs to wait for kubernetes-retired/kubernetes-anywhere#483 being merged?

cc @luxas

xiangpengzhao · 2017-11-15T15:28:13Z

/hold cancel

luxas

/lgtm

luxas · 2017-11-15T18:14:19Z

/approve no-issue

k8s-github-robot · 2017-11-15T18:15:05Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: luxas, xiangpengzhao

Associated issue requirement bypassed by: luxas

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

Needs approval from an approver in each of these OWNERS Files:

~~cmd/kubeadm/OWNERS~~ [luxas]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

k8s-github-robot · 2017-11-15T20:03:34Z

Automatic merge from submit-queue (batch tested with PRs 55648, 55274, 54982, 51955, 55639). If you want to cherry-pick this change to another branch, please follow the instructions here.

Automatic merge from submit-queue (batch tested with PRs 55839, 54495, 55884, 55983, 56069). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>. Add condition "len(cfg.DiscoveryToken) != 0" to ValidateArgSelection. **What this PR does / why we need it**: as per #54982 (comment) >only when the conditions having len(cfg.DiscoveryToken) != 0 means "using token-based discovery" as is mentioned in the error message. **Which issue(s) this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when PR gets merged)*: ref: #54982 **Special notes for your reviewer**: /cc @kubernetes/sig-cluster-lifecycle-pr-reviews **Release note**: ```release-note NONE ```

k8s-ci-robot requested a review from luxas November 2, 2017 06:31

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Nov 2, 2017

k8s-github-robot assigned dmmcquay and krousey Nov 2, 2017

xiangpengzhao force-pushed the cleanup-kubeadm branch from e311801 to f030dcb Compare November 2, 2017 12:44

k8s-ci-robot assigned luxas Nov 8, 2017

luxas reviewed Nov 9, 2017

View reviewed changes

xiangpengzhao force-pushed the cleanup-kubeadm branch from f030dcb to a04a94b Compare November 10, 2017 07:06

luxas approved these changes Nov 10, 2017

View reviewed changes

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

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 10, 2017

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 Nov 10, 2017

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

xiangpengzhao mentioned this pull request Nov 10, 2017

Set --discovery-token-unsafe-skip-ca-verification flag when --discovery-token-ca-cert-hash is not set kubernetes-retired/kubernetes-anywhere#483

Merged

xiangpengzhao force-pushed the cleanup-kubeadm branch from a04a94b to b8e7315 Compare November 15, 2017 15:23

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 15, 2017

xiangpengzhao changed the title ~~kubeadm: convert warning to error and remove unused version.~~ kubeadm:Clean up no longer used k8sVersion param Nov 15, 2017

Clean up no longer used k8sVersion param

b8e7315

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

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Nov 15, 2017

luxas approved these changes Nov 15, 2017

View reviewed changes

k8s-github-robot merged commit dab5dc6 into kubernetes:master Nov 15, 2017

xiangpengzhao deleted the cleanup-kubeadm branch November 15, 2017 22:32

xiangpengzhao mentioned this pull request Nov 20, 2017

Add condition "len(cfg.DiscoveryToken) != 0" to ValidateArgSelection. #56069

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kubeadm:Clean up no longer used k8sVersion param #54982

kubeadm:Clean up no longer used k8sVersion param #54982

xiangpengzhao commented Nov 2, 2017 •

edited by luxas

xiangpengzhao commented Nov 8, 2017

luxas left a comment

luxas Nov 9, 2017

luxas Nov 9, 2017

luxas Nov 9, 2017

luxas Nov 9, 2017

xiangpengzhao Nov 9, 2017 •

edited

luxas Nov 9, 2017

xiangpengzhao Nov 9, 2017

xiangpengzhao Nov 10, 2017

luxas left a comment

luxas commented Nov 10, 2017

luxas commented Nov 10, 2017

xiangpengzhao commented Nov 10, 2017

mattmoyer commented Nov 14, 2017

xiangpengzhao commented Nov 15, 2017 •

edited

xiangpengzhao commented Nov 15, 2017

luxas left a comment

luxas commented Nov 15, 2017

k8s-github-robot commented Nov 15, 2017

k8s-github-robot commented Nov 15, 2017

kubeadm:Clean up no longer used k8sVersion param #54982

kubeadm:Clean up no longer used k8sVersion param #54982

Conversation

xiangpengzhao commented Nov 2, 2017 • edited by luxas

xiangpengzhao commented Nov 8, 2017

luxas left a comment

Choose a reason for hiding this comment

luxas Nov 9, 2017

Choose a reason for hiding this comment

luxas Nov 9, 2017

Choose a reason for hiding this comment

luxas Nov 9, 2017

Choose a reason for hiding this comment

luxas Nov 9, 2017

Choose a reason for hiding this comment

xiangpengzhao Nov 9, 2017 • edited

Choose a reason for hiding this comment

luxas Nov 9, 2017

Choose a reason for hiding this comment

xiangpengzhao Nov 9, 2017

Choose a reason for hiding this comment

xiangpengzhao Nov 10, 2017

Choose a reason for hiding this comment

luxas left a comment

Choose a reason for hiding this comment

luxas commented Nov 10, 2017

luxas commented Nov 10, 2017

xiangpengzhao commented Nov 10, 2017

mattmoyer commented Nov 14, 2017

xiangpengzhao commented Nov 15, 2017 • edited

xiangpengzhao commented Nov 15, 2017

luxas left a comment

Choose a reason for hiding this comment

luxas commented Nov 15, 2017

k8s-github-robot commented Nov 15, 2017

k8s-github-robot commented Nov 15, 2017

xiangpengzhao commented Nov 2, 2017 •

edited by luxas

xiangpengzhao Nov 9, 2017 •

edited

xiangpengzhao commented Nov 15, 2017 •

edited