Add warning when --certificate-key is set and --control-plane is not. #83661

jfbai · 2019-10-09T06:27:23Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

Print warning when --certificate-key is set and --control-plane is not to notify users what happens.

For example, when users try to download certs via kubeadm join phase control-plane-prepare download-certs <ip>:<port> --certificate-key <key> --discovery-token <token> --discovery-token-unsafe -skip-ca-verification, this command exits with no error and the certs will not be downloaded successfully, because --control-plane is not set. I was confused for hours and got to know the cause via reading source code. So, it would be helpful for users to print a warning.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

None

Does this PR introduce a user-facing change?:

None

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

None

/sig cluster-lifecycle
/assign @rosti
/assign @neolit123

k8s-ci-robot · 2019-10-09T06:27:31Z

Hi @jfbai. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

rosti

Thanks @jfbai !
Indeed this is a known UX problem. Unfortunately, I don't think, that this is the correct way of solving it.
The best way, in my opinion, is to just force JoinConfiguration.ControlPlane to be non-nil in case some of the control plane phases of join are invoked manually (as is the case here).
@fabriziopandini @neolit123 @ereslibre WDYT?

neolit123 · 2019-10-09T11:25:44Z

cmd/kubeadm/app/cmd/join.go

@@ -342,6 +342,9 @@ func newJoinData(cmd *cobra.Command, args []string, opt *joinOptions, out io.Wri

 	// if not joining a control plane, unset the ControlPlane object
 	if !opt.controlPlane {
+		if len(opt.externalcfg.ControlPlane.CertificateKey) > 0 {
+			klog.Warningf("[preflight] WARNING: %s will be ignored when %s is not set.", options.CertificateKey, options.ControlPlane)
+		}


long term, we should not bind more logic related to flags and flags mixtures.
some day, ideally, we should move all flags to configuration-only and configuration patching.

neolit123 · 2019-10-09T11:26:05Z

cmd/kubeadm/app/cmd/join.go

@@ -342,6 +342,9 @@ func newJoinData(cmd *cobra.Command, args []string, opt *joinOptions, out io.Wri

 	// if not joining a control plane, unset the ControlPlane object
 	if !opt.controlPlane {
+		if len(opt.externalcfg.ControlPlane.CertificateKey) > 0 {


i think ControlPlane can be nil here?

Yes it can.

If --control-plane is false, we are ignoring all the configurations under opt.externalcfg.ControlPlane (bot certificateKey and localAPIEndpoint)

So IMO the check and the message should be made more generic e.g

if !opt.controlPlane { if opt.externalcfg.ControlPlane!=nil { klog.Warningf("[preflight] WARNING: JoinControlPane.controlPlane settings will be ignored when %s flag is not set.", options.ControlPlane) } }

neolit123 · 2019-10-09T11:31:21Z

@rosti

The best way, in my opinion, is to just force JoinConfiguration.ControlPlane to be non-nil in case some of the control plane phases of join are invoked manually (as is the case here).

i would prefer a different solution instead of the flag mixture warning.
by forcing ControlPlane to non-nil are you suggesting that if certificate key is passed we should:

create a JoinControlPlane object and assign it to JoinConfiguration.ControlPlane
populate JoinConfiguration.ControlPlane.CertificateKey
pupulate JoinConfiguration.ControlPlane.LocalAPIEndpoint from the default net interface

this would make the --control-plane flag redundant if --certificate-key is provided?

yastij · 2019-10-09T12:43:32Z

cmd/kubeadm/app/cmd/join.go

@@ -342,6 +342,9 @@ func newJoinData(cmd *cobra.Command, args []string, opt *joinOptions, out io.Wri

 	// if not joining a control plane, unset the ControlPlane object
 	if !opt.controlPlane {
+		if len(opt.externalcfg.ControlPlane.CertificateKey) > 0 {


Yes it can.

yastij · 2019-10-09T12:44:47Z

cmd/kubeadm/app/cmd/join.go

@@ -342,6 +342,9 @@ func newJoinData(cmd *cobra.Command, args []string, opt *joinOptions, out io.Wri

 	// if not joining a control plane, unset the ControlPlane object
 	if !opt.controlPlane {
+		if len(opt.externalcfg.ControlPlane.CertificateKey) > 0 {
+			klog.Warningf("[preflight] WARNING: %s will be ignored when %s is not set.", options.CertificateKey, options.ControlPlane)
+		}


yastij · 2019-10-09T12:46:48Z

cmd/kubeadm/app/cmd/join.go

@@ -342,6 +342,9 @@ func newJoinData(cmd *cobra.Command, args []string, opt *joinOptions, out io.Wri

 	// if not joining a control plane, unset the ControlPlane object
 	if !opt.controlPlane {
+		if len(opt.externalcfg.ControlPlane.CertificateKey) > 0 {
+			klog.Warningf("[preflight] WARNING: %s will be ignored when %s is not set.", options.CertificateKey, options.ControlPlane)


If we think that we should force --control-plane when providing cert. key, I think we should error here.

Sorry, I am a bit confused. Do we prefer to force ControlPlane to non-nil or error and exits?

i will add this for a discussion for our next kubeadm meeting and we will hopefully have a good answer in the next couple of days.

Thanks a lot. :)

neolit123 · 2019-10-09T16:48:16Z

/assign @fabriziopandini

fabriziopandini

@jfbai thanks for this contribution
I think that while we get agreement on a more generalized solution (sew issue kubernetes/kubeadm#1442 for a similar use case), I think that giving a warning to users is a good idea.

I only added a suggestion to make the check/the warning more generic, then lgtm from my side

fabriziopandini · 2019-10-13T08:26:19Z

cmd/kubeadm/app/cmd/join.go

@@ -342,6 +342,9 @@ func newJoinData(cmd *cobra.Command, args []string, opt *joinOptions, out io.Wri

 	// if not joining a control plane, unset the ControlPlane object
 	if !opt.controlPlane {
+		if len(opt.externalcfg.ControlPlane.CertificateKey) > 0 {


If --control-plane is false, we are ignoring all the configurations under opt.externalcfg.ControlPlane (bot certificateKey and localAPIEndpoint)

So IMO the check and the message should be made more generic e.g

if !opt.controlPlane { if opt.externalcfg.ControlPlane!=nil { klog.Warningf("[preflight] WARNING: JoinControlPane.controlPlane settings will be ignored when %s flag is not set.", options.ControlPlane) } }

jfbai · 2019-10-13T09:12:42Z

@jfbai thanks for this contribution
I think that while we get agreement on a more generalized solution (sew issue kubernetes/kubeadm#1442 for a similar use case), I think that giving a warning to users is a good idea.

I only added a suggestion to make the check/the warning more generic, then lgtm from my side

@fabriziopandini Thanks a lot and your comment has been fixed. :)

fabriziopandini · 2019-10-13T11:10:02Z

Great!
/approve
I'll wait one or two days for letting everyone in this loop take s look at the last commit

k8s-ci-robot · 2019-10-13T11:10:44Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fabriziopandini, jfbai

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~~ [fabriziopandini]

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

fabriziopandini · 2019-10-13T11:12:24Z

/ok-to-test

fabriziopandini · 2019-10-13T11:14:57Z

/priority important-soon

jfbai · 2019-10-13T11:55:41Z

/test pull-kubernetes-kubemark-e2e-gce-big

neolit123 · 2019-10-13T12:30:24Z

/lgtm
/retest

jfbai · 2019-10-13T13:41:47Z

@neolit123 I fixed the gofmt, could you please help add LGTM again? thanks a lot. :)

neolit123 · 2019-10-13T13:58:15Z

/lgtm

jfbai · 2019-10-13T14:15:24Z

/test pull-kubernetes-integration

Add warning when --certificate-key is set and --control-plane is not.

k8s-ci-robot assigned neolit123 and rosti Oct 9, 2019

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. area/kubeadm labels Oct 9, 2019

k8s-ci-robot requested review from yagonobre and yastij October 9, 2019 06:29

rosti reviewed Oct 9, 2019

View reviewed changes

neolit123 reviewed Oct 9, 2019

View reviewed changes

yastij reviewed Oct 9, 2019

View reviewed changes

k8s-ci-robot assigned fabriziopandini Oct 9, 2019

fabriziopandini suggested changes Oct 13, 2019

View reviewed changes

jfbai force-pushed the fix-kubeadm-join branch from 62c9765 to 0aa88af Compare October 13, 2019 09:11

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

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 13, 2019

k8s-ci-robot added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Oct 13, 2019

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

Add warning when --certificate-key is set and --control-plane is not.

6dbf154

jfbai force-pushed the fix-kubeadm-join branch from 0aa88af to 6dbf154 Compare October 13, 2019 13:39

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 13, 2019

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

k8s-ci-robot merged commit f0e1d39 into kubernetes:master Oct 13, 2019

k8s-ci-robot added this to the v1.17 milestone Oct 13, 2019

jfbai deleted the fix-kubeadm-join branch October 14, 2019 02:24

ohsewon pushed a commit to ohsewon/kubernetes that referenced this pull request Oct 16, 2019

Merge pull request kubernetes#83661 from jfbai/fix-kubeadm-join

18af5c6

Add warning when --certificate-key is set and --control-plane is not.

jfbai mentioned this pull request Oct 28, 2019

REQUEST: New membership for @jfbai kubernetes/org#1346

Closed

6 tasks

neolit123 mentioned this pull request Mar 18, 2020

misleading warning on control-plane join kubernetes/kubeadm#2065

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add warning when --certificate-key is set and --control-plane is not. #83661

Add warning when --certificate-key is set and --control-plane is not. #83661

jfbai commented Oct 9, 2019 •

edited

Loading

k8s-ci-robot commented Oct 9, 2019

rosti left a comment

neolit123 Oct 9, 2019

yastij Oct 9, 2019

neolit123 Oct 9, 2019

yastij Oct 9, 2019

fabriziopandini Oct 13, 2019

neolit123 commented Oct 9, 2019

yastij Oct 9, 2019

yastij Oct 9, 2019

yastij Oct 9, 2019

jfbai Oct 9, 2019

neolit123 Oct 9, 2019

jfbai Oct 10, 2019

neolit123 commented Oct 9, 2019

fabriziopandini left a comment

fabriziopandini Oct 13, 2019

jfbai commented Oct 13, 2019

fabriziopandini commented Oct 13, 2019

k8s-ci-robot commented Oct 13, 2019

fabriziopandini commented Oct 13, 2019

fabriziopandini commented Oct 13, 2019

jfbai commented Oct 13, 2019

neolit123 commented Oct 13, 2019

jfbai commented Oct 13, 2019

neolit123 commented Oct 13, 2019

jfbai commented Oct 13, 2019

Add warning when --certificate-key is set and --control-plane is not. #83661

Add warning when --certificate-key is set and --control-plane is not. #83661

Conversation

jfbai commented Oct 9, 2019 • edited Loading

k8s-ci-robot commented Oct 9, 2019

rosti left a comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neolit123 commented Oct 9, 2019

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neolit123 commented Oct 9, 2019

fabriziopandini left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jfbai commented Oct 13, 2019

fabriziopandini commented Oct 13, 2019

k8s-ci-robot commented Oct 13, 2019

fabriziopandini commented Oct 13, 2019

fabriziopandini commented Oct 13, 2019

jfbai commented Oct 13, 2019

neolit123 commented Oct 13, 2019

jfbai commented Oct 13, 2019

neolit123 commented Oct 13, 2019

jfbai commented Oct 13, 2019

jfbai commented Oct 9, 2019 •

edited

Loading