config refactoring: address review comments

This commit: - Removes the default value in cobra config flag - Adds a logic to set the default file under the config package Signed-off-by: Karen Almog <[email protected]>
k0sproject · Jan 21, 2022 · 4b1735e · 4b1735e
1 parent 8e556a4
commit 4b1735e
Show file tree

Hide file tree

Showing 15 changed files with 78 additions and 67 deletions.
diff --git a/cmd/backup/backup.go b/cmd/backup/backup.go
@@ -48,7 +48,10 @@ func NewBackupCmd() *cobra.Command {
 			}
 			return c.backup()
 		},
-		PreRunE: preRunValidateConfig,
+		PreRunE: func(c *cobra.Command, args []string) error {
+			cmdOpts := CmdOpts(config.GetCmdOpts())
+			return config.PreRunValidateConfig(cmdOpts.K0sVars)
+		},
 	}
 	cmd.Flags().StringVar(&savePath, "save-path", "", "destination directory path for backup assets, use '-' for stdout")
 	cmd.SilenceUsage = true
@@ -80,18 +83,7 @@ func (c *CmdOpts) backup() error {
 		if err != nil {
 			return err
 		}
-		return mgr.RunBackup(c.CfgFile, c.NodeConfig.Spec, c.K0sVars, savePath)
+		return mgr.RunBackup(c.NodeConfig.Spec, c.K0sVars, savePath)
 	}
 	return fmt.Errorf("backup command must be run on the controller node, have `%s`", status.Role)
 }
-
-func preRunValidateConfig(cmd *cobra.Command, args []string) error {
-	c := CmdOpts(config.GetCmdOpts())
-
-	loadingRules := config.ClientConfigLoadingRules{K0sVars: c.K0sVars}
-	_, err := loadingRules.ParseRuntimeConfig()
-	if err != nil {
-		return fmt.Errorf("failed to get config: %v", err)
-	}
-	return nil
-}
diff --git a/cmd/install/controller.go b/cmd/install/controller.go
@@ -46,7 +46,10 @@ With the controller subcommand you can setup a single node cluster by running:
 			}
 			return nil
 		},
-		PreRunE: preRunValidateConfig,
+		PreRunE: func(c *cobra.Command, args []string) error {
+			cmdOpts := CmdOpts(config.GetCmdOpts())
+			return config.PreRunValidateConfig(cmdOpts.K0sVars)
+		},
 	}
 	// append flags
 	cmd.PersistentFlags().AddFlagSet(config.GetPersistentFlagSet())

diff --git a/cmd/install/install.go b/cmd/install/install.go
@@ -89,14 +89,3 @@ func (c *CmdOpts) convertFileParamsToAbsolute() (err error) {
 	}
 	return nil
 }
-
-func preRunValidateConfig(_ *cobra.Command, _ []string) error {
-	c := CmdOpts(config.GetCmdOpts())
-
-	loadingRules := config.ClientConfigLoadingRules{K0sVars: c.K0sVars}
-	_, err := loadingRules.ParseRuntimeConfig()
-	if err != nil {
-		return fmt.Errorf("failed to get config: %v", err)
-	}
-	return nil
-}
diff --git a/cmd/install/worker.go b/cmd/install/worker.go
@@ -45,7 +45,6 @@ Windows flags like "--api-server", "--cidr-range" and "--cluster-dns" will be ig
 
 			return nil
 		},
-		PreRunE: preRunValidateConfig,
 	}
 	// append flags
 	cmd.PersistentFlags().AddFlagSet(config.GetPersistentFlagSet())

diff --git a/cmd/reset/reset.go b/cmd/reset/reset.go
@@ -41,7 +41,10 @@ func NewResetCmd() *cobra.Command {
 			c := CmdOpts(config.GetCmdOpts())
 			return c.reset()
 		},
-		PreRunE: preRunValidateConfig,
+		PreRunE: func(c *cobra.Command, args []string) error {
+			cmdOpts := CmdOpts(config.GetCmdOpts())
+			return config.PreRunValidateConfig(cmdOpts.K0sVars)
+		},
 	}
 	cmd.SilenceUsage = true
 	cmd.PersistentFlags().AddFlagSet(config.GetPersistentFlagSet())
@@ -72,14 +75,3 @@ func (c *CmdOpts) reset() error {
 
 	return err
 }
-
-func preRunValidateConfig(_ *cobra.Command, _ []string) error {
-	c := CmdOpts(config.GetCmdOpts())
-
-	loadingRules := config.ClientConfigLoadingRules{K0sVars: c.K0sVars}
-	_, err := loadingRules.ParseRuntimeConfig()
-	if err != nil {
-		return fmt.Errorf("failed to get config: %v", err)
-	}
-	return nil
-}
diff --git a/cmd/restore/restore.go b/cmd/restore/restore.go
@@ -58,7 +58,7 @@ func NewRestoreCmd() *cobra.Command {
 
 	cwd, err := os.Getwd()
 	if err != nil {
-		return nil
+		logrus.Fatal("failed to get local path")
 	}
 
 	restoredConfigPathDescription := fmt.Sprintf("Specify desired name and full path for the restored k0s.yaml file (default: %s/k0s_<archive timestamp>.yaml", cwd)

diff --git a/internal/testutil/runtime_config.go b/internal/testutil/runtime_config.go
@@ -64,7 +64,6 @@ func (c *ConfigGetter) FakeConfigFromFile() (*v1beta1.ClusterConfig, error) {
 	loadingRules := config.ClientConfigLoadingRules{
 		RuntimeConfigPath: RuntimeFakePath,
 		Nodeconfig:        c.NodeConfig,
-		CfgFileOverride:   c.cfgFilePath,
 		K0sVars:           c.k0sVars,
 	}
 	return loadingRules.Load()
@@ -87,7 +86,6 @@ func (c *ConfigGetter) FakeAPIConfig() (*v1beta1.ClusterConfig, error) {
 	loadingRules := config.ClientConfigLoadingRules{
 		RuntimeConfigPath: RuntimeFakePath,
 		Nodeconfig:        c.NodeConfig,
-		CfgFileOverride:   c.cfgFilePath,
 		APIClient:         client.K0sV1beta1(),
 		K0sVars:           c.k0sVars,
 	}

diff --git a/inttest/common/footloosesuite.go b/inttest/common/footloosesuite.go
@@ -564,7 +564,6 @@ func (s *FootlooseSuite) GetKubeClientConfig(node string, k0sKubeconfigArgs ...s
 	kubeConfigCmd := fmt.Sprintf("%s kubeconfig admin %s 2>/dev/null", s.K0sFullPath, strings.Join(k0sKubeconfigArgs, " "))
 	kubeConf, err := ssh.ExecWithOutput(kubeConfigCmd)
 	if err != nil {
-		fmt.Println(string(kubeConf))
 		return nil, err
 	}
 	cfg, err := clientcmd.Load([]byte(kubeConf))

diff --git a/inttest/k0sctl/k0sctl b/inttest/k0sctl/k0sctl
diff --git a/pkg/backup/manager.go b/pkg/backup/manager.go
@@ -44,8 +44,14 @@ type Manager struct {
 }
 
 // RunBackup backups cluster
-func (bm *Manager) RunBackup(cfgPath string, nodeSpec *v1beta1.ClusterSpec, vars constant.CfgVars, savePathDir string) error {
-	bm.discoverSteps(cfgPath, nodeSpec, vars, "backup", "")
+func (bm *Manager) RunBackup(nodeSpec *v1beta1.ClusterSpec, vars constant.CfgVars, savePathDir string) error {
+	configLoader := config.ClientConfigLoadingRules{}
+	_, err := configLoader.Load()
+	if err != nil {
+		return err
+	}
+
+	bm.discoverSteps(configLoader.RuntimeConfigPath, nodeSpec, vars, "backup", "")
 	defer os.RemoveAll(bm.tmpDir)
 	assets := make([]string, 0, len(bm.steps))
 

diff --git a/pkg/backup/util.go b/pkg/backup/util.go
@@ -44,7 +44,7 @@ func createArchive(archive io.Writer, files []string, baseDir string) error {
 	for _, file := range files {
 		err := addToArchive(tw, file, baseDir)
 		if err != nil {
-			return err
+			return fmt.Errorf("failed to add file to backup archive: %v", err)
 		}
 	}
 	return nil

diff --git a/pkg/config/cli.go b/pkg/config/cli.go
@@ -16,6 +16,7 @@ limitations under the License.
 package config
 
 import (
+	"fmt"
 	"os"
 	"path/filepath"
 	"strings"
@@ -189,7 +190,8 @@ func GetControllerFlags() *pflag.FlagSet {
 // it in multiple places
 func FileInputFlag() *pflag.FlagSet {
 	flagset := &pflag.FlagSet{}
-	flagset.StringVarP(&CfgFile, "config", "c", constant.K0sConfigPathDefault, "config file, use '-' to read the config from stdin")
+	descString := fmt.Sprintf("config file, use '-' to read the config from stdin (default \"%s\")", constant.K0sConfigPathDefault)
+	flagset.StringVarP(&CfgFile, "config", "c", "", descString)
 
 	return flagset
 }
@@ -202,8 +204,8 @@ func GetCmdOpts() CLIOptions {
 		K0sVars.DefaultStorageType = "kine"
 	}
 
-	// when a custom config file is used, verify that it can be opened
-	if CfgFile != constant.K0sConfigPathDefault {
+	// When CfgFile is set, verify the file can be opened
+	if CfgFile != "" {
 		_, err := os.Open(CfgFile)
 		if err != nil {
 			logrus.Fatalf("failed to load config file (%s): %v", CfgFile, err)
@@ -226,6 +228,14 @@ func GetCmdOpts() CLIOptions {
 	return opts
 }
 
+func PreRunValidateConfig(k0sVars constant.CfgVars) error {
+	loadingRules := ClientConfigLoadingRules{K0sVars: k0sVars}
+	_, err := loadingRules.ParseRuntimeConfig()
+	if err != nil {
+		return fmt.Errorf("failed to get config: %v", err)
+	}
+	return nil
+}
 func getNodeConfig(k0sVars constant.CfgVars) *v1beta1.ClusterConfig {
 	loadingRules := ClientConfigLoadingRules{Nodeconfig: true, K0sVars: k0sVars}
 	cfg, err := loadingRules.Load()

diff --git a/pkg/config/config.go b/pkg/config/config.go
@@ -71,9 +71,6 @@ type ClientConfigLoadingRules struct {
 	// this parameter is mainly used for testing purposes, to override the default location on local dev system
 	RuntimeConfigPath string
 
-	// CfgFileOverride is an optional field for overriding the CfgFile parameter from cobra. Used mainly for testing purposes.
-	CfgFileOverride string
-
 	// K0sVars is needed for fetching the right config from the API
 	K0sVars constant.CfgVars
 }
@@ -111,10 +108,6 @@ func (rules *ClientConfigLoadingRules) IsDefaultConfig() bool {
 }
 
 func (rules *ClientConfigLoadingRules) Load() (*v1beta1.ClusterConfig, error) {
-	if rules.CfgFileOverride != "" {
-		CfgFile = rules.CfgFileOverride
-	}
-
 	if rules.Nodeconfig {
 		return rules.fetchNodeConfig()
 	}

diff --git a/pkg/config/config_test.go b/pkg/config/config_test.go
@@ -144,10 +144,15 @@ spec:
 
 // Test using config from a yaml file
 func TestConfigFromDefaults(t *testing.T) {
-	CfgFile = constant.K0sConfigPathDefault // this path doesn't exist, so default values should be generated
 	defer os.Remove(configPathRuntimeTest)
 
+	CfgFile = ""
 	loadingRules := ClientConfigLoadingRules{RuntimeConfigPath: configPathRuntimeTest}
+	err := loadingRules.InitRuntimeConfig(constant.GetConfig(""))
+	if err != nil {
+		t.Fatalf("failed to initialize k0s config: %s", err.Error())
+	}
+
 	cfg, err := loadingRules.Load()
 	if err != nil {
 		t.Fatalf("failed to load config: %s", err.Error())
@@ -215,12 +220,12 @@ func TestNodeConfigWithAPIConfig(t *testing.T) {
 }
 
 func TestSingleNodeConfig(t *testing.T) {
-	CfgFile = constant.K0sConfigPathDefault // this path doesn't exist, so default values should be generated
 	defer os.Remove(configPathRuntimeTest)
 
 	loadingRules := ClientConfigLoadingRules{RuntimeConfigPath: configPathRuntimeTest, Nodeconfig: true}
 	k0sVars := constant.GetConfig("")
 	k0sVars.DefaultStorageType = "kine"
+	CfgFile = ""
 
 	err := loadingRules.InitRuntimeConfig(k0sVars)
 	if err != nil {
@@ -231,9 +236,11 @@ func TestSingleNodeConfig(t *testing.T) {
 	if err != nil {
 		t.Fatalf("failed to load config: %s", err.Error())
 	}
+
 	if cfg == nil {
 		t.Fatal("received an empty config! failing")
 	}
+
 	testCases := []struct {
 		name     string
 		got      string

diff --git a/pkg/config/file_config.go b/pkg/config/file_config.go
@@ -61,34 +61,57 @@ func (rules *ClientConfigLoadingRules) readRuntimeConfig() (clusterConfig *v1bet
 func (rules *ClientConfigLoadingRules) ParseRuntimeConfig() (*v1beta1.ClusterConfig, error) {
 	var cfg *v1beta1.ClusterConfig
 
+	var storage *v1beta1.StorageSpec
+	if rules.K0sVars.DefaultStorageType == "kine" {
+		storage = &v1beta1.StorageSpec{
+			Type: v1beta1.KineStorageType,
+			Kine: v1beta1.DefaultKineConfig(rules.K0sVars.DataDir),
+		}
+	}
 	if rules.RuntimeConfigPath == "" {
 		rules.RuntimeConfigPath = runtimeConfigPathDefault
 	}
 
-	// don't create the runtime config file, if it already exists
+	// If runtime config already exists, use it as the source of truth
 	if file.Exists(rules.RuntimeConfigPath) {
 		logrus.Debugf("runtime config found: using %s", rules.RuntimeConfigPath)
-		CfgFile = rules.RuntimeConfigPath
-	}
 
-	var storage *v1beta1.StorageSpec
-	if rules.K0sVars.DefaultStorageType == "kine" {
-		storage = &v1beta1.StorageSpec{
-			Type: v1beta1.KineStorageType,
-			Kine: v1beta1.DefaultKineConfig(rules.K0sVars.DataDir),
+		// read config from runtime config
+		f, err := os.Open(rules.RuntimeConfigPath)
+		if err != nil {
+			return nil, err
 		}
+		defer f.Close()
+
+		cfg, err = v1beta1.ConfigFromReader(f, storage)
+		if err != nil {
+			return nil, err
+		}
+		return cfg, nil
 	}
 
 	switch CfgFile {
 	// stdin input
 	case "-":
 		return v1beta1.ConfigFromReader(os.Stdin, storage)
-	default:
-		f, err := os.Open(CfgFile)
+	case "":
+		// if no config is set, look for config in the default location
+		// if it does not exist there either, generate default config
+		f, err := os.Open(constant.K0sConfigPathDefault)
 		if err != nil {
 			if os.IsNotExist(err) {
 				return rules.generateDefaults(storage), nil
 			}
+		}
+		defer f.Close()
+
+		cfg, err = v1beta1.ConfigFromReader(f, storage)
+		if err != nil {
+			return nil, err
+		}
+	default:
+		f, err := os.Open(CfgFile)
+		if err != nil {
 			return nil, err
 		}
 		defer f.Close()