Skip to content
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: use DefValue for the --kubeconfig flag #71874

Merged
merged 1 commit into from Mar 4, 2019

Conversation

@neolit123
Copy link
Member

neolit123 commented Dec 8, 2018

What this PR does / why we need it:

  • Rename FindExistingKubeConfig to FindKubeConfigFile

  • Cobra supports a DefValue option which can be used
    to differentiate between the cases where the user set a flag
    and when a flag was unset, while still adding a default value.
    Use this in options/generic.go for the kubeconfig flag.

  • Remove the FindKubeConfigFile() logic from reset and upgrade
    as these are node level kubeadm commands.

  • Default kubeconfig values to "" everywhere where FindKubeConfigFile
    is used. This allows to search for existing kubeconfig locations.

Which issue(s) this PR fixes:

Fixes kubernetes/kubeadm#1304

Special notes for your reviewer:
NONE

Does this PR introduce a user-facing change?:

kubeadm: fix a bug where standard kubeconfig paths were searched even if the user provided /etc/kubernetes/admin.conf explicitly for commands that accept --kubeconfig, like kubeadm token.
kubeadm: use the default kubeconfig (/etc/kubernetes/admin.conf) for "kubeadm reset" and "kubeadm upgrade" commands.

/assign @fabriziopandini
/cc @yagonobre
/cc @kubernetes/sig-cluster-lifecycle-pr-reviews
/priority important-longterm
/hold

@mauilion

This comment has been minimized.

Copy link
Contributor

mauilion commented Dec 8, 2018

+1 to KUBECONFIG precedence. This is what kubectl does so that's likely the expectation

@fabriziopandini

This comment has been minimized.

Copy link
Contributor

fabriziopandini commented Dec 8, 2018

As far as I'm concerned the main use case for kubeadm is to take care of the node that exist locally on the machine.
This is e.g. the case of kubeadm init, and if I remember well what was discussed with @luxas when implementing kiubeadm init phases we agreed that use only the kubeadm path for those commands in order to avoid the user mess up other machine by mistake

Kubeadm reset in my opinion should do the same (use only kubeadm default path)

The kubeconfig path should be considered only in few utilities command that the users might run on its own (without a node on the machine) e.g. kubeadm token or kubeadm config default, but IMO also in those cases kubeadm default path should go first: if there is a node one the machine, most probably this is the one I want to manage....

@neolit123

This comment has been minimized.

Copy link
Member Author

neolit123 commented Dec 8, 2018

^ we need to decide on the kubeconfig search precedence.
/assign @timothysc @luxas

  1. have the search order the same as kubectl - search the KUBECONFIG env var first
  2. search for the kubeadm default kubeconfig path first - /etc/kubernetes/admin.conf

EDIT: didn't see the reply from @timothysc

@neolit123

This comment has been minimized.

Copy link
Member Author

neolit123 commented Dec 8, 2018

@fabriziopandini the majority seem to suggest that we should search KUBECONFIG first.
@luxas do you have an opinion?

@rosti

This comment has been minimized.

Copy link
Member

rosti commented Dec 8, 2018

In my opinion, we should strive to reduce the confusion and act like kubectl. KUBECONFIG should take precedence over the default path.

@fabriziopandini

This comment has been minimized.

Copy link
Contributor

fabriziopandini commented Dec 9, 2018

@neolit123 I start getting the impression that changing this without a master plan in the background that considers all the kubeadm commands is potentially risky

Prior art: after discussion with @luxas and you when implementing #70763, using KUBECONFIG first was dropped because of the potential risks, and as a consequence the current behavior for kubeadm init and kubeadm init phases does not consider KUBECONFIG at all.

Now we are changing opinion for kebadm reset...
Might be better to discuss this at sig-level

@neolit123

This comment has been minimized.

Copy link
Member Author

neolit123 commented Dec 9, 2018

Prior art: after discussion with @luxas and you when implementing #70763, using KUBECONFIG first was dropped because of the potential risks, and as a consequence the current behavior for kubeadm init and kubeadm init phases does not consider KUBECONFIG at all.

i'm mostly indifferent about the search order myself.
but i see others have an opinion here.

Now we are changing opinion for kebadm reset...

the cross linked issue is about kubeadm token.

Might be better to discuss this at sig-level

we can also ask the #kubeadm channel with a vote.
but let's talk about it on wednesday.

@timothysc

This comment has been minimized.

Copy link
Member

timothysc commented Jan 22, 2019

Let's clear this one out this week.

@neolit123 neolit123 force-pushed the neolit123:fix-kubeconfig-path branch from 63c8fa0 to dba280c Feb 3, 2019

@neolit123 neolit123 changed the title kubeadm: fix a bug in the search logic for kubeconfig files kubeadm: don't search for a kubeconfig file on reset Feb 3, 2019

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Feb 3, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: neolit123

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:

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

@neolit123

This comment has been minimized.

Copy link
Member Author

neolit123 commented Feb 3, 2019

/remove-kind bug
/kind cleanup

updated this PR to only remove the search precedence on reset.
other locations where precedence is used:

app/cmd/config.go:86:	kubeConfigFile = cmdutil.FindExistingKubeConfig(kubeConfigFile)
app/cmd/token.go:124:			kubeConfigFile = cmdutil.FindExistingKubeConfig(kubeConfigFile)
app/cmd/token.go:151:			kubeConfigFile = cmdutil.FindExistingKubeConfig(kubeConfigFile)
app/cmd/token.go:175:			kubeConfigFile = cmdutil.FindExistingKubeConfig(kubeConfigFile)
app/cmd/upgrade/upgrade.go:65:	flags.kubeConfigPath = cmdutil.FindExistingKubeConfig(flags.kubeConfigPath)
app/cmd/alpha/selfhosting.go:122:			kubeConfigFile = cmdutil.FindExistingKubeConfig(kubeConfigFile)
app/cmd/alpha/kubelet.go:151:			kubeConfigFile = cmdutil.FindExistingKubeConfig(kubeConfigFile)
app/cmd/alpha/certs.go:171:		kubeConfigPath := cmdutil.FindExistingKubeConfig(cfg.kubeconfigPath)

search precedence might make sense for these.

current precedence for the above commands:

  • explicit path
  • KUBECONFIG env variable paths
  • user home directory ~/.kube/...
  • default kubeadm path etc/kubernetes/admin.conf

this issue is about the precedence not accepting the user provided flag if it's the default value:
kubernetes/kubeadm#1304
this PR fixes that.

ping @fabriziopandini and @timothysc for LGTM.

@k8s-ci-robot k8s-ci-robot removed the kind/bug label Feb 3, 2019

@neolit123

This comment has been minimized.

Copy link
Member Author

neolit123 commented Feb 3, 2019

/retest

@neolit123

This comment has been minimized.

Copy link
Member Author

neolit123 commented Feb 3, 2019

/kind bug
/retest

@k8s-ci-robot k8s-ci-robot added kind/bug and removed kind/bug labels Feb 3, 2019

@MalloZup
Copy link
Contributor

MalloZup left a comment

+1

Show resolved Hide resolved cmd/kubeadm/app/cmd/util/cmdutil.go
// FindExistingKubeConfig returns the location of a kubeconfig file.
// If the user passed the --kubeconfig flag respect it, otherwise search for existing
// kubeconfig files in default locations.
func FindExistingKubeConfig(flags *pflag.FlagSet) string {

This comment has been minimized.

@rosti

rosti Feb 4, 2019

Member

I dislike the idea of having FindExistingKubeConfig take FlagSet as an argument. This needs to retain the file path string and just return it if it's not empty. Then we simply need to ensure, the file path is not defaulted anywhere.

This comment has been minimized.

@neolit123

neolit123 Feb 4, 2019

Author Member

if we check over a string, there is no way to determine if the user set the flag or if it was defaulted to a value by cobra.

in the linked issue the user was passing: --kubeconfig=/etc/kubernetes/admin.conf and it still used the KUBECONFIG env.

flags.Changed() solves this problem.

This comment has been minimized.

@rosti

rosti Feb 5, 2019

Member

We must not default the flag in Cobra. We have to make sure it's defaulted to an empty string.

This comment has been minimized.

@rosti

rosti Feb 5, 2019

Member

This is kind of the case of the cri-socket flag.

This comment has been minimized.

@neolit123

neolit123 Feb 5, 2019

Author Member

here is an example of what happens in kubeoconfig.go:

kubeConfigFile := constants.GetAdminKubeConfigPath()
cmd := &cobra.Command{
Use: "config",
Short: "Manage configuration for a kubeadm cluster persisted in a ConfigMap in the cluster.",
Long: fmt.Sprintf(dedent.Dedent(`
There is a ConfigMap in the %s namespace called %q that kubeadm uses to store internal configuration about the
cluster. kubeadm CLI v1.8.0+ automatically creates this ConfigMap with the config used with 'kubeadm init', but if you
initialized your cluster using kubeadm v1.7.x or lower, you must use the 'config upload' command to create this
ConfigMap. This is required so that 'kubeadm upgrade' can configure your upgraded cluster correctly.
`), metav1.NamespaceSystem, constants.KubeadmConfigConfigMap),
// Without this callback, if a user runs just the "upload"
// command without a subcommand, or with an invalid subcommand,
// cobra will print usage information, but still exit cleanly.
// We want to return an error code in these cases so that the
// user knows that their command was invalid.
RunE: cmdutil.SubCmdRunE("config"),
}
options.AddKubeConfigFlag(cmd.PersistentFlags(), &kubeConfigFile)

  • we default a local variable to /etc/kubernetes/....
  • then use options.. to add the flag.
  • at this point the default becomes /etc/kubernetes/...

how should this be handled instead?
the CRI flag passes "" as the default.

cmdutil.AddCRISocketFlag(cmd.PersistentFlags(), &criSocketPath)

problem is we want to show /etc/kubernetes... as the default kubeconfig path when the user types kubeadm... --help.

This comment has been minimized.

@rosti

rosti Feb 6, 2019

Member

pflag supports special field in its Flag struct called DefValue (see here). It's used solely for printing the default value for usage information and not for initialization. You can overwrite that just after flag insertion with something along those lines:

func AddKubeConfigFlag(fs *pflag.FlagSet, kubeConfigFile *string, defaultKubeConfigFile string) {
	fs.StringVar(kubeConfigFile, KubeconfigPath, "", "The kubeconfig file to use when talking to the cluster. If the flag is not set, a set of standard locations are searched for an existing KubeConfig file.")
	fs.Lookup(KubeconfigPath).DefValue = defaultKubeConfigFile
}

Then the invocation will be something along those lines:

kubeConfigPath := ""
// ....
options.AddKubeConfigFlag(cmd.Flags(), &kubeConfigPath, constants.GetAdminKubeConfigPath())

In theory this should work, but I did not test it.

P.S. You need the Lookup call, because StringVar does not return a *pflag.Flag.

This comment has been minimized.

@neolit123

neolit123 Feb 6, 2019

Author Member

pflag supports special field in its Flag struct called DefValue

ok i didn't know this existed.
will give this a test in the following days.

@neolit123 neolit123 force-pushed the neolit123:fix-kubeconfig-path branch from 639dae9 to 5a2b834 Feb 16, 2019

@k8s-ci-robot k8s-ci-robot added size/M and removed size/XS labels Feb 16, 2019

@neolit123 neolit123 changed the title kubeadm: don't search for a kubeconfig file on reset kubeadm: use DefValue for the --kubeconfig flag Feb 16, 2019

if _, err := os.Stat(kubeConfigFile); !os.IsNotExist(err) {
client, err = getClientset(kubeConfigFile, false)
kubeadmutil.CheckErr(err)
klog.V(1).Infof("[reset] loaded client set from kubeconfig file: %s", kubeConfigFile)
} else {
klog.V(1).Infof("[reset] could not get client set from missing kubeconfig file: %s", kubeConfigFile)

This comment has been minimized.

@neolit123

neolit123 Feb 16, 2019

Author Member

added some V(1) output for this..

@neolit123

This comment has been minimized.

Copy link
Member Author

neolit123 commented Feb 16, 2019

@fabriziopandini @rosti @bart0sh
updated the PR to follow the last suggestion by @rosti for Cobra's DefValue.
the OP includes details of what this PR is doing as a summary.

summary of how kubeconfig loading works currently:

  • init (phases), join (phases), reset & upgrade, use the default path only.
    the way this is done - always preset the kubeconfig to the default value before adding the flag using AddKubeConfigFlag. don't call FindKubeConfigFile.
  • for other commands if a variable with no value is passed to AddKubeConfigFlag( this value will end up in FindKubeConfigFile and default locations will be searched. however if the user sets a value for that flag the value will reach FindKubeConfigFile and the function will act as a no-op.

i don't think i can improve the logic more here, so let's try to settle on a decision for 1.14.
also, we need to consider what implication this has on existing setups and what else to reflect in the release notes.

return rules.GetDefaultFilename()
file = rules.GetDefaultFilename()
klog.V(1).Infof("Using kubeconfig file: %s", file)
return file
}

This comment has been minimized.

@bart0sh

bart0sh Feb 16, 2019

Contributor

I still find It confusing that this machinery can return path to the file that doesn't exist. Should we check it here and return an error or at least warn user somehow?

This comment has been minimized.

@neolit123

neolit123 Feb 16, 2019

Author Member

this value will eventually reach a function that tries to open this file and an error will be thrown if the file is missing.

This comment has been minimized.

@bart0sh

bart0sh Feb 16, 2019

Contributor

The point is that possible error is not handled immediately. We (intentionally?) leave error handling for future processing which may or not may happen. This is generally fragile and error prone approach in my opinion.

This comment has been minimized.

@neolit123

neolit123 Feb 16, 2019

Author Member

please show examples in the source code where error handling is delayed for kubeconfig.

This comment has been minimized.

@bart0sh

bart0sh Feb 16, 2019

Contributor

Sure. Almost any place where FindExistingConfig function is called, e.g. https://github.com/kubernetes/kubernetes/blob/master/cmd/kubeadm/app/cmd/alpha/certs.go#L171
The potential error (returned path to not existing config) is not processed in the FindExistingConfig function. It's left unprocessed in a hope that it will be processed in kubeconfigutil.ClientSetFromFile function.

This comment has been minimized.

@bart0sh

bart0sh Feb 16, 2019

Contributor

up to you. In my opinion it would make sense to catch potential errors as soon as possible. Current code doesn't do it, leaving error processing to LoadFromFile, ClientSetFromFile, getClientset and other functions. That processing may or may not happen. I hope it happens in current Kubeadm code. However, looking at the calls of FindExistingKubeConfig in app/cmd/upgrade/upgrade.go and a couple of similar places I would't be surprised if it doesn't happen at all.

This comment has been minimized.

@bart0sh

bart0sh Feb 18, 2019

Contributor

@fabriziopandini, @rosti What do you guys think about this? I can explain it in more details if needed.

This comment has been minimized.

@rosti

rosti Feb 19, 2019

Member

Depends on the use cases, which TBH, I did not check in their entirety. If we always load that file after we call this function and not do anything else with it prior to that, then we are OK. Otherwise, it will not be very safe.
If we don't need an extra check, let's not have it, but instead, we can rename the func to something, that implies getting a config file path, rather than verifying it exists. In that case words like "find" and "existing" should not be included in the function name.

Considering this, how about s/FindKibeConfigFile/GetKubeConfigPath?

This comment has been minimized.

@neolit123

neolit123 Feb 19, 2019

Author Member

the rename SGTM will update.

This comment has been minimized.

@neolit123

neolit123 Feb 19, 2019

Author Member

i just had a look and all calls to (the newly renamed) GetKubeConfigPath results in a kubeconfig string that is always passed to ClientSetFromFile.

which to me confirms this:

If we always load that file after we call this function and not do anything else with it prior to that, then we are OK. Otherwise, it will not be very safe.

return rules.GetDefaultFilename()
file = rules.GetDefaultFilename()
klog.V(1).Infof("Using kubeconfig file: %s", file)
return file
}

This comment has been minimized.

@rosti

rosti Feb 19, 2019

Member

Depends on the use cases, which TBH, I did not check in their entirety. If we always load that file after we call this function and not do anything else with it prior to that, then we are OK. Otherwise, it will not be very safe.
If we don't need an extra check, let's not have it, but instead, we can rename the func to something, that implies getting a config file path, rather than verifying it exists. In that case words like "find" and "existing" should not be included in the function name.

Considering this, how about s/FindKibeConfigFile/GetKubeConfigPath?

Show resolved Hide resolved cmd/kubeadm/app/cmd/options/generic.go
kubeadm: use DefValue for the --kubeconfig flag
- Rename FindExistingKubeConfig to GetKubeConfigPath

- Cobra supports a DefValue option which can be used
to differentiate between the cases where the user set a flag
and when a flag was unset, while still adding a default value.
Use this in options/generic.go for the kubeconfig flag.

- Remove the GetKubeConfigPath() logic from `reset` and `upgrade`
as these are node level kubeadm commands.

- Default kubeconfig values to "" everywhere where GetKubeConfigPath
is used. This allows to search for existing kubeconfig locations.

@neolit123 neolit123 force-pushed the neolit123:fix-kubeconfig-path branch from 5a2b834 to 363c778 Feb 19, 2019

@neolit123

This comment has been minimized.

Copy link
Member Author

neolit123 commented Feb 27, 2019

@rosti @bart0sh @timothysc @fabriziopandini et al
code freeze is coming soon. are we blocking on this for 1.14?

@bart0sh

This comment has been minimized.

Copy link
Contributor

bart0sh commented Feb 28, 2019

/lgtm

@neolit123

This comment has been minimized.

Copy link
Member Author

neolit123 commented Mar 2, 2019

this PR is approved and has thumbs up by @bart0sh and @rosti
final call on reviews before /hold cancel this weekend.

@neolit123

This comment has been minimized.

Copy link
Member Author

neolit123 commented Mar 4, 2019

/hold cancel

@k8s-ci-robot k8s-ci-robot merged commit 37f0138 into kubernetes:master Mar 4, 2019

17 checks passed

cla/linuxfoundation neolit123 authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-godeps Skipped
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped
pull-kubernetes-local-e2e-containerized Context retired without replacement.
Details
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
pull-publishing-bot-validate Skipped
tide In merge pool.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.