cli/cmd: rename --kubeconfig flag to --kubeconfig-file #602

invidian · 2020-06-09T18:41:45Z

This PR renames --kubeconfig flag to --kubeconfig-file to avoid automatic reading of KUBECONFIG environment variable by viper and adds explicit read of KUBECONFIG variable after cluster configuration in the hierarchy, when selecting kubeconfig file to use.

For the time being, there is no tests for this patch, as testing that is not trivial to implement (involves creating lokocfg configuration file in temporary directory, testing private method, and testing viper flags parsing.

Closes #595.

invidian · 2020-06-10T08:49:39Z

For the time being, there is no tests for this patch, as testing that is not trivial to implement (involves creating lokocfg configuration file in temporary directory, testing private method, and testing viper flags parsing.

It turns out, I introduced some other bugs to the implementation, so I decided to write tests for it, even though they are not great.

iaguis · 2020-06-18T11:52:28Z

cli/cmd/root.go

+		"Path to kubeconfig file, taken from the asset dir if not given or from KUBECONFIG "+
+			"environment variable and finally falls back to ~/.kube/config")


We can take this opportunity to improve the text a bit.

Suggested change

"Path to kubeconfig file, taken from the asset dir if not given or from KUBECONFIG "+

"environment variable and finally falls back to ~/.kube/config")

`Path to a kubeconfig file. If empty, the following precedence order is used: 1. cluster asset dir when a lokocfg file is present in the current directory 2. KUBECONFIG environment variable 3. "~/.kube/config"`)

iaguis · 2020-06-18T11:53:33Z

cli/cmd/utils.go

-// flag or environment variable. Then the asset directory of the cluster is searched
-// and finally the global default value is used. This cannot be done in Viper
-// because we need the other values from Viper to find the asset directory.
+// getKubeconfig finds the kubeconfig to be used. The preference is following:


Suggested change

// getKubeconfig finds the kubeconfig to be used. The preference is following:

// getKubeconfig finds the kubeconfig to be used. The precedence is the following:

iaguis · 2020-06-18T11:53:53Z

cli/cmd/utils.go

+// - --kubeconfig-file flag OR KUBECONFIG_FILE environent variable.
+// - Asset directory from cluster configuration.
+// - KUBECONFIG environment variable.
+// - ~/.kube/config path, which is kubectl default one.


Suggested change

// - ~/.kube/config path, which is kubectl default one.

// - ~/.kube/config path, which is the default for kubectl.

iaguis · 2020-06-18T11:56:17Z

cli/cmd/utils.go

+// assetsKubeconfigPath reads the lokocfg configuration and returns
+// path to kubeconfig file in it.


Suggested change

// assetsKubeconfigPath reads the lokocfg configuration and returns

// path to kubeconfig file in it.

// assetsKubeconfigPath reads the lokocfg configuration and returns

// the kubeconfig path defined in it.

iaguis · 2020-06-18T12:00:00Z

cli/cmd/utils_internal_test.go

@@ -0,0 +1,184 @@
+// Copyright 2020 The Lokomotive Authors


I'd like a test where both the KUBECONFIG flag is set and the lokocfg file is present to make sure it takes the file defined in the lokocfg. This is the most dangerous case IMO.

ipochi · 2020-06-18T12:27:13Z

@invidian @iaguis

I think that we should not use ~/.kube/config to fall back.
Having KUBECONFIG_FILE environment variable gives an irritating behavior to the user as lokoctl is not a replacement for kubectl.

Even after installing the component by setting the KUBECONFIG_FILE environment variable, a user still interfaces the cluster with kubectl or in some cases helm. In such a scenario kubectl or helm is pointing to KUBECONFIG env variable or ~/.kube/config and lokoctl to KUBECONFIG_FILE.

Consider the the simplest scenario :

Installed a component X with KUBECONFIG_FILE env var set.
Need to get some logs of the component via kubectl, now I need to set the KUBECONFIG to the Lokomotive managed cluster in order to perform operations.

IMO, we should just have a flag --kubeconfig-file in lokoctl without KUBECONFIG_FILE env var.

Can we perhaps use Extensions (not sure how extensions is used, I've never seen it in a kubeconfig file) in the kubeconfig file to specify the additional cluster information as managed by Lokomotive. If the KUBECONFIG or current context is not referring to the same then we print out a message to the user for confirmation before proceeding ?

invidian · 2020-06-18T12:29:55Z

I agree with Imran, that having this env variable is not really desired. Unfortunately, this is a side effect of the cobra. I'm not sure if we can disable env variable reading for just one flag.

iaguis · 2020-06-18T13:15:46Z

Just as a reminder, my main focus for this PR is fixing the following case (which happened to me twice with bad consequences):

A user runs lokoctl component apply from their cluster directory.
The user has $KUBECONFIG set to another cluster, probably not even managed by Lokomotive.
The components are applied to the cluster defined in $KUBECONFIG instead of the Lokomotive cluster (!)

I agree the $KUBECONFIG_FILE env var doesn't make sense. If it's not possible to disable it from cobra/viper I think just keeping it undocumented should be fine.

Regarding falling back to ~/.kube/config if there's no $KUBECONFIG and the user is not in a Lokomotive cluster directory I'm on the fence.

On the one hand kubectl falls back to ~/.kube/config so it makes sense if we do the same and commands to kubectl and lokoctl would apply to the same cluster. On the other hand this behavior might be dangerous and as a caution we might want to forbid it and ask the user to set $KUBECONFIG.

Since falling back to ~/.kube/config is already happening now, we can keep it and decide later if we want to change this behavior.

This also renames environment variable equivalent of old flag, which was KUBECONFIG to KUBECONFIG_FILE. This is to avoid prefering KUBECONFIG environment variable over cluster configuration, which is happening right now, as we treat --kubeconfig flag and KUBECONFIG environment variable as the same thing, implicitly via 'viper.AutomaticEnv()' call. The order of selecting kubeconfig file to use should be following: 1. --kubeconfig-file flag OR KUBECONFIG_FILE environment variable 2. cluster configuration in working directory 3. KUBECONFIG environment variable (currently not implemented) 4. ~/.kube/config file Part of #595. Signed-off-by: Mateusz Gozdek <mateusz@kinvolk.io>

After renaming --kubeconfig flag to --kubeconfig-file, we lost ability to use kubeconfig file defined by KUBECONFIG environment variable for installing components. This commit adds back this functionality and improves the code of getKubeconfig() function a bit. Closes #595 Signed-off-by: Mateusz Gozdek <mateusz@kinvolk.io>

Otherwise linter complains. Signed-off-by: Mateusz Gozdek <mateusz@kinvolk.io>

So resulting code is much easier to understand. Signed-off-by: Mateusz Gozdek <mateusz@kinvolk.io>

iaguis · 2020-06-18T13:51:01Z

Addressed my review since @invidian is out.

cli/cmd/utils_internal_test.go

To make sure we do it right, as otherwise it might be destructive. Signed-off-by: Mateusz Gozdek <mateusz@kinvolk.io>

invidian

LGTM

ipochi

Nice work @invidian @iaguis

lgtm

iaguis

lgtm

invidian force-pushed the invidian/rename-kubeconfig-flag branch from 4133c67 to 6f16108 Compare June 9, 2020 18:59

invidian requested review from johananl and surajssd as code owners June 9, 2020 18:59

invidian force-pushed the invidian/rename-kubeconfig-flag branch 2 times, most recently from d39d68b to ad712a7 Compare June 10, 2020 08:29

iaguis suggested changes Jun 18, 2020

View reviewed changes

invidian added 4 commits June 18, 2020 15:22

cli/cmd/root.go: handle binding flag errors

5024bf7

Otherwise linter complains. Signed-off-by: Mateusz Gozdek <mateusz@kinvolk.io>

cli/cmd: getKubeconfig: refactor into smaller functions

c61ba20

So resulting code is much easier to understand. Signed-off-by: Mateusz Gozdek <mateusz@kinvolk.io>

iaguis force-pushed the invidian/rename-kubeconfig-flag branch from ad712a7 to 0687e1e Compare June 18, 2020 13:50

iaguis requested a review from ipochi June 18, 2020 13:51

invidian commented Jun 18, 2020

View reviewed changes

cli/cmd/utils_internal_test.go Show resolved Hide resolved

cli/cmd/utils_internal_test.go Show resolved Hide resolved

cli/cmd/utils_internal_test.go Outdated Show resolved Hide resolved

iaguis force-pushed the invidian/rename-kubeconfig-flag branch from 0687e1e to ab7638a Compare June 18, 2020 14:03

cli/cmd: add tests for getKubeconfig

7470a24

To make sure we do it right, as otherwise it might be destructive. Signed-off-by: Mateusz Gozdek <mateusz@kinvolk.io>

iaguis force-pushed the invidian/rename-kubeconfig-flag branch from ab7638a to 7470a24 Compare June 18, 2020 14:04

invidian commented Jun 18, 2020

View reviewed changes

ipochi approved these changes Jun 18, 2020

View reviewed changes

iaguis approved these changes Jun 18, 2020

View reviewed changes

iaguis merged commit 0cbadba into master Jun 18, 2020

iaguis deleted the invidian/rename-kubeconfig-flag branch June 18, 2020 14:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cli/cmd: rename --kubeconfig flag to --kubeconfig-file #602

cli/cmd: rename --kubeconfig flag to --kubeconfig-file #602

invidian commented Jun 9, 2020

invidian commented Jun 10, 2020

iaguis Jun 18, 2020

iaguis Jun 18, 2020

iaguis Jun 18, 2020

iaguis Jun 18, 2020

iaguis Jun 18, 2020

ipochi commented Jun 18, 2020

invidian commented Jun 18, 2020

iaguis commented Jun 18, 2020 •

edited

Loading

iaguis commented Jun 18, 2020

invidian left a comment

ipochi left a comment

iaguis left a comment

		"Path to kubeconfig file, taken from the asset dir if not given or from KUBECONFIG "+
		"environment variable and finally falls back to ~/.kube/config")

	"Path to kubeconfig file, taken from the asset dir if not given or from KUBECONFIG "+
	"environment variable and finally falls back to ~/.kube/config")
	`Path to a kubeconfig file. If empty, the following precedence order is used: 1. cluster asset dir when a lokocfg file is present in the current directory 2. KUBECONFIG environment variable 3. "~/.kube/config"`)

	// getKubeconfig finds the kubeconfig to be used. The preference is following:
	// getKubeconfig finds the kubeconfig to be used. The precedence is the following:

	// - ~/.kube/config path, which is kubectl default one.
	// - ~/.kube/config path, which is the default for kubectl.

		// assetsKubeconfigPath reads the lokocfg configuration and returns
		// path to kubeconfig file in it.

cli/cmd: rename --kubeconfig flag to --kubeconfig-file #602

cli/cmd: rename --kubeconfig flag to --kubeconfig-file #602

Conversation

invidian commented Jun 9, 2020

invidian commented Jun 10, 2020

iaguis Jun 18, 2020

Choose a reason for hiding this comment

iaguis Jun 18, 2020

Choose a reason for hiding this comment

iaguis Jun 18, 2020

Choose a reason for hiding this comment

iaguis Jun 18, 2020

Choose a reason for hiding this comment

iaguis Jun 18, 2020

Choose a reason for hiding this comment

ipochi commented Jun 18, 2020

invidian commented Jun 18, 2020

iaguis commented Jun 18, 2020 • edited Loading

iaguis commented Jun 18, 2020

invidian left a comment

Choose a reason for hiding this comment

ipochi left a comment

Choose a reason for hiding this comment

iaguis left a comment

Choose a reason for hiding this comment

iaguis commented Jun 18, 2020 •

edited

Loading