Skip to content
This repository has been archived by the owner on Apr 22, 2020. It is now read-only.

Don't pass true for bool type flag --discovery-token-unsafe-skip-ca-verification #488

Closed

Conversation

xiangpengzhao
Copy link
Contributor

Passing the flag means true, right? Seems like --discovery-token-unsafe-skip-ca-verification true is invalid.

xref: kubernetes/kubernetes#56091 [job failure] kubeadm-gce

Nov 23 05:13:24 ubuntu startupscript: kubeadm join --token "$KUBEADM_TOKEN" "$KUBEADM_MASTER_IP:443" --skip-preflight-checks --discovery-token-unsafe-skip-ca-verification true
Nov 23 05:13:24 ubuntu startupscript: + kubeadm join --token 043479.1a6ec0adafb593d0 10.128.0.2:443 --skip-preflight-checks --discovery-token-unsafe-skip-ca-verification true
Nov 23 05:13:25 ubuntu startupscript: Flag --skip-preflight-checks has been deprecated, it is now equivalent to --ignore-checks-errors=all
Nov 23 05:13:25 ubuntu startupscript: [kubeadm] WARNING: kubeadm is currently in beta
Nov 23 05:13:25 ubuntu startupscript: [preflight] Running pre-flight checks.
Nov 23 05:13:25 ubuntu startupscript: #011[WARNING FileExisting-crictl]: crictl not found in system path
Nov 23 05:13:25 ubuntu startupscript: [validation] WARNING: kubeadm doesn't fully support multiple API Servers yet
Nov 23 05:13:25 ubuntu startupscript: discovery: Invalid value: "true": address true: missing port in address
Nov 23 05:13:25 ubuntu startupscript: Finished running startup script /var/run/google.startup.script

@pipejakob @luxas @dims @spiffxp

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Nov 23, 2017
@dims
Copy link

dims commented Nov 23, 2017

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 23, 2017
@dims dims mentioned this pull request Nov 23, 2017
@xiangpengzhao
Copy link
Contributor Author

/priority critical-urgent
/priority failing-test

ping @pipejakob for merging.

@@ -1,4 +1,4 @@

# This is not meant to run on its own, but extends phase2/kubeadm/configure-vm-kubeadm.sh

kubeadm join --token "$KUBEADM_TOKEN" "$KUBEADM_MASTER_IP:443" --skip-preflight-checks --discovery-token-unsafe-skip-ca-verification true
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍
also, we should pass this flag ONLY if the kubeadm version is v1.8 or higher.
The versions below that didn't have this feature.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. 👍

@shashidharatd I'm not familiar with kubeadm version management/check in kubernetes-anywhere. Could you please help fix this? Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe can ues KUBEADM_VERSION, already have this variable in this file.
But KUBEADM_VERSION is a link like :gs://kubernetes-release-dev/bazel/v1.10.0-alpha.0.631+e7ad6e60081887/bin/linux/amd64/.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes but the variable is also used in openstack and seems like there it uses "stable" version from another link. It makes it complicated to check the version. And I can't test it out on an gce cloud :(

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep, its complicated. off hand what i could think of is to extract a semver from KUBEADM_VERSION and write a semver matching function and based on the condition use the right option for --discovery-token-unsafe-skip-ca-verification flag.

AFAIK, the KUBEADM_VERSION can be either one of 2 values.

  • stable
  • an URL pointing to GCS like the one prefixed with gs://

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shashidharatd sounds great! Thanks! BTW, do you have cycle to help implement this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@xiangpengzhao, i am giving it a try. will raise a PR once my GCE local testing pass.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shashidharatd cool, thanks!

@xiangpengzhao
Copy link
Contributor Author

/close
in favor of #491. Thanks @shashidharatd!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm Indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants