From b8b19c66dc6fbd8481ceb2f74e21072e735c0812 Mon Sep 17 00:00:00 2001 From: Mateusz Gozdek Date: Thu, 24 Sep 2020 14:35:39 +0200 Subject: [PATCH 1/5] cli/cmd: consistently import logrus Signed-off-by: Mateusz Gozdek --- cli/cmd/cluster.go | 12 ++++++------ cli/cmd/utils.go | 8 ++++---- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/cli/cmd/cluster.go b/cli/cmd/cluster.go index fbb542e67..45db4c73c 100644 --- a/cli/cmd/cluster.go +++ b/cli/cmd/cluster.go @@ -18,7 +18,7 @@ import ( "fmt" "github.com/mitchellh/go-homedir" - "github.com/sirupsen/logrus" + log "github.com/sirupsen/logrus" "github.com/spf13/cobra" "helm.sh/helm/v3/pkg/action" "helm.sh/helm/v3/pkg/chart" @@ -43,7 +43,7 @@ func init() { // initialize does common initialization actions between cluster operations // and returns created objects to the caller for further use. -func initialize(contextLogger *logrus.Entry) (*terraform.Executor, platform.Platform, *config.Config, string) { +func initialize(contextLogger *log.Entry) (*terraform.Executor, platform.Platform, *config.Config, string) { lokoConfig, diags := getLokoConfig() if len(diags) > 0 { contextLogger.Fatal(diags) @@ -90,7 +90,7 @@ func initialize(contextLogger *logrus.Entry) (*terraform.Executor, platform.Plat // initializeTerraform initialized Terraform directory using given backend and platform // and returns configured executor. -func initializeTerraform(contextLogger *logrus.Entry, p platform.Platform, b backend.Backend) *terraform.Executor { +func initializeTerraform(contextLogger *log.Entry, p platform.Platform, b backend.Backend) *terraform.Executor { assetDir, err := homedir.Expand(p.Meta().AssetDir) if err != nil { contextLogger.Fatalf("Error expanding path: %v", err) @@ -131,7 +131,7 @@ func initializeTerraform(contextLogger *logrus.Entry, p platform.Platform, b bac // clusterExists determines if cluster has already been created by getting all // outputs from the Terraform. If there is any output defined, it means 'terraform apply' // run at least once. -func clusterExists(contextLogger *logrus.Entry, ex *terraform.Executor) bool { +func clusterExists(contextLogger *log.Entry, ex *terraform.Executor) bool { o := map[string]interface{}{} if err := ex.Output("", &o); err != nil { @@ -144,7 +144,7 @@ func clusterExists(contextLogger *logrus.Entry, ex *terraform.Executor) bool { type controlplaneUpdater struct { kubeconfig []byte assetDir string - contextLogger logrus.Entry + contextLogger log.Entry ex terraform.Executor } @@ -176,7 +176,7 @@ func (c controlplaneUpdater) getControlplaneValues(name string) (map[string]inte } func (c controlplaneUpdater) upgradeComponent(component, namespace string) { - contextLogger := c.contextLogger.WithFields(logrus.Fields{ + contextLogger := c.contextLogger.WithFields(log.Fields{ "action": "controlplane-upgrade", "component": component, }) diff --git a/cli/cmd/utils.go b/cli/cmd/utils.go index 6fbbbcec9..964cd9391 100644 --- a/cli/cmd/utils.go +++ b/cli/cmd/utils.go @@ -22,7 +22,7 @@ import ( "github.com/hashicorp/hcl/v2" "github.com/mitchellh/go-homedir" - "github.com/sirupsen/logrus" + log "github.com/sirupsen/logrus" "github.com/spf13/viper" "github.com/kinvolk/lokomotive/pkg/backend" @@ -86,7 +86,7 @@ func getConfiguredPlatform(lokoConfig *config.Config, require bool) (platform.Pl // getKubeconfig finds the right kubeconfig file to use for an action and returns it's content. // // If platform is required and user do not have it configured, an error is returned. -func getKubeconfig(contextLogger *logrus.Entry, lokoConfig *config.Config, platformRequired bool) ([]byte, error) { +func getKubeconfig(contextLogger *log.Entry, lokoConfig *config.Config, platformRequired bool) ([]byte, error) { sources, err := getKubeconfigSource(contextLogger, lokoConfig, platformRequired) if err != nil { return nil, fmt.Errorf("selecting kubeconfig source: %w", err) @@ -126,7 +126,7 @@ func getKubeconfig(contextLogger *logrus.Entry, lokoConfig *config.Config, platf // // - kubeconfig from ~/.kube/config file. // -func getKubeconfigSource(contextLogger *logrus.Entry, lokoConfig *config.Config, platformRequired bool) ([]string, error) { //nolint:lll +func getKubeconfigSource(contextLogger *log.Entry, lokoConfig *config.Config, platformRequired bool) ([]string, error) { //nolint:lll // Always try reading platform configuration. p, diags := getConfiguredPlatform(lokoConfig, platformRequired) if diags.HasErrors() { @@ -183,7 +183,7 @@ func getLokoConfig() (*config.Config, hcl.Diagnostics) { // readKubeconfigFromTerraformState initializes Terraform and // reads content of cluster kubeconfig file from the Terraform. -func readKubeconfigFromTerraformState(contextLogger *logrus.Entry) ([]byte, error) { +func readKubeconfigFromTerraformState(contextLogger *log.Entry) ([]byte, error) { contextLogger.Warn("Kubeconfig file not found in assets directory, pulling kubeconfig from " + "Terraform state, this might be slow. Run 'lokoctl cluster apply' to fix it.") From 112d745e7894c40db8e1ad7457cda35334a7c763 Mon Sep 17 00:00:00 2001 From: Mateusz Gozdek Date: Thu, 24 Sep 2020 15:46:31 +0200 Subject: [PATCH 2/5] cli/cmd: don't use contextLogger.Fatalf for cluster apply and destroy To remove a dependency on log.Entry to make moving this code around easier and to avoid hiding function complexity from logger. Refs #630 Signed-off-by: Mateusz Gozdek --- cli/cmd/cluster-apply.go | 28 ++++++++++++++++++---------- cli/cmd/cluster-destroy.go | 17 ++++++++++++++--- 2 files changed, 32 insertions(+), 13 deletions(-) diff --git a/cli/cmd/cluster-apply.go b/cli/cmd/cluster-apply.go index 99da2247d..384c0edf2 100644 --- a/cli/cmd/cluster-apply.go +++ b/cli/cmd/cluster-apply.go @@ -51,48 +51,54 @@ func init() { pf.BoolVarP(&upgradeKubelets, "upgrade-kubelets", "", false, "Experimentally upgrade self-hosted kubelets") } -//nolint:funlen func runClusterApply(cmd *cobra.Command, args []string) { contextLogger := log.WithFields(log.Fields{ "command": "lokoctl cluster apply", "args": args, }) + if err := clusterApply(contextLogger); err != nil { + contextLogger.Fatalf("Applying cluster failed: %v", err) + } +} + +//nolint:funlen +func clusterApply(contextLogger *log.Entry) error { ex, p, lokoConfig, assetDir := initialize(contextLogger) exists := clusterExists(contextLogger, ex) if exists && !confirm { // TODO: We could plan to a file and use it when installing. if err := ex.Plan(); err != nil { - contextLogger.Fatalf("Failed to reconcile cluster state: %v", err) + return fmt.Errorf("reconciling cluster state: %v", err) } if !askForConfirmation("Do you want to proceed with cluster apply?") { contextLogger.Println("Cluster apply cancelled") - return + return nil } } if err := p.Apply(ex); err != nil { - contextLogger.Fatalf("Error applying cluster: %v", err) + return fmt.Errorf("applying platform: %v", err) } fmt.Printf("\nYour configurations are stored in %s\n", assetDir) kubeconfig, err := getKubeconfig(contextLogger, lokoConfig, true) if err != nil { - contextLogger.Fatalf("Failed to get kubeconfig: %v", err) + return fmt.Errorf("getting kubeconfig: %v", err) } if err := verifyCluster(kubeconfig, p.Meta().ExpectedNodes); err != nil { - contextLogger.Fatalf("Verify cluster: %v", err) + return fmt.Errorf("verifying cluster: %v", err) } // Update all the pre installed namespaces with lokomotive specific label. // `lokomotive.kinvolk.io/name: `. if err := updateInstalledNamespaces(kubeconfig); err != nil { - contextLogger.Fatalf("Updating installed namespace: %v", err) + return fmt.Errorf("updating installed namespace: %v", err) } // Do controlplane upgrades only if cluster already exists and it is not a managed platform. @@ -122,12 +128,12 @@ func runClusterApply(cmd *cobra.Command, args []string) { if ph, ok := p.(platform.PlatformWithPostApplyHook); ok { if err := ph.PostApplyHook(kubeconfig); err != nil { - contextLogger.Fatalf("Running platform post install hook failed: %v", err) + return fmt.Errorf("running platform post install hook: %v", err) } } if skipComponents { - return + return nil } componentsToApply := []string{} @@ -139,9 +145,11 @@ func runClusterApply(cmd *cobra.Command, args []string) { if len(componentsToApply) > 0 { if err := applyComponents(lokoConfig, kubeconfig, componentsToApply...); err != nil { - contextLogger.Fatalf("Applying component configuration failed: %v", err) + return fmt.Errorf("applying component configuration: %v", err) } } + + return nil } func verifyCluster(kubeconfig []byte, expectedNodes int) error { diff --git a/cli/cmd/cluster-destroy.go b/cli/cmd/cluster-destroy.go index 416b0eab9..b8f635a43 100644 --- a/cli/cmd/cluster-destroy.go +++ b/cli/cmd/cluster-destroy.go @@ -15,6 +15,8 @@ package cmd import ( + "fmt" + log "github.com/sirupsen/logrus" "github.com/spf13/cobra" ) @@ -40,26 +42,35 @@ func runClusterDestroy(cmd *cobra.Command, args []string) { "args": args, }) + if err := clusterDestroy(contextLogger); err != nil { + contextLogger.Fatalf("Destroying cluster: %v", err) + } +} + +func clusterDestroy(contextLogger *log.Entry) error { ex, p, _, _ := initialize(contextLogger) if !clusterExists(contextLogger, ex) { contextLogger.Println("Cluster already destroyed, nothing to do") - return + return nil } if !confirm { confirmation := askForConfirmation("WARNING: This action cannot be undone. Do you really want to destroy the cluster?") if !confirmation { contextLogger.Println("Cluster destroy canceled") - return + + return nil } } if err := p.Destroy(ex); err != nil { - contextLogger.Fatalf("Error destroying cluster: %v", err) + return fmt.Errorf("destroying cluster: %v", err) } contextLogger.Println("Cluster destroyed successfully") contextLogger.Println("You can safely remove the assets directory now") + + return nil } From 4be26f031049c0e1380c7c2980557d54ea2a8be6 Mon Sep 17 00:00:00 2001 From: Mateusz Gozdek Date: Thu, 24 Sep 2020 15:47:15 +0200 Subject: [PATCH 3/5] cli/cmd/component-apply.go: simplify arguments handling a bit Signed-off-by: Mateusz Gozdek --- cli/cmd/component-apply.go | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/cli/cmd/component-apply.go b/cli/cmd/component-apply.go index 62170b427..15af519f1 100644 --- a/cli/cmd/component-apply.go +++ b/cli/cmd/component-apply.go @@ -66,10 +66,9 @@ func runApply(cmd *cobra.Command, args []string) { contextLogger.Fatal(diags) } - var componentsToApply []string - if len(args) > 0 { - componentsToApply = append(componentsToApply, args...) - } else { + componentsToApply := args + + if len(componentsToApply) == 0 { for _, component := range lokoConfig.RootConfig.Components { componentsToApply = append(componentsToApply, component.Name) } From 9a25a8e424b2c62a05c4486b675eb96645e04cab Mon Sep 17 00:00:00 2001 From: Mateusz Gozdek Date: Thu, 24 Sep 2020 16:18:09 +0200 Subject: [PATCH 4/5] cli/cmd: consistently use diags.HasErrors() for checking diagnostics Part of #994. Signed-off-by: Mateusz Gozdek --- cli/cmd/cluster.go | 2 +- cli/cmd/component-apply.go | 5 ++--- cli/cmd/component-delete.go | 2 +- 3 files changed, 4 insertions(+), 5 deletions(-) diff --git a/cli/cmd/cluster.go b/cli/cmd/cluster.go index 45db4c73c..f82fead16 100644 --- a/cli/cmd/cluster.go +++ b/cli/cmd/cluster.go @@ -45,7 +45,7 @@ func init() { // and returns created objects to the caller for further use. func initialize(contextLogger *log.Entry) (*terraform.Executor, platform.Platform, *config.Config, string) { lokoConfig, diags := getLokoConfig() - if len(diags) > 0 { + if diags.HasErrors() { contextLogger.Fatal(diags) } diff --git a/cli/cmd/component-apply.go b/cli/cmd/component-apply.go index 15af519f1..8a40b0987 100644 --- a/cli/cmd/component-apply.go +++ b/cli/cmd/component-apply.go @@ -62,12 +62,11 @@ func runApply(cmd *cobra.Command, args []string) { } lokoConfig, diags := getLokoConfig() - if len(diags) > 0 { + if diags.HasErrors() { contextLogger.Fatal(diags) } componentsToApply := args - if len(componentsToApply) == 0 { for _, component := range lokoConfig.RootConfig.Components { componentsToApply = append(componentsToApply, component.Name) @@ -96,7 +95,7 @@ func applyComponents(lokoConfig *config.Config, kubeconfig []byte, componentName componentConfigBody := lokoConfig.LoadComponentConfigBody(componentName) - if diags := component.LoadConfig(componentConfigBody, lokoConfig.EvalContext); len(diags) > 0 { + if diags := component.LoadConfig(componentConfigBody, lokoConfig.EvalContext); diags.HasErrors() { fmt.Printf("%v\n", diags) return diags } diff --git a/cli/cmd/component-delete.go b/cli/cmd/component-delete.go index dd4a56275..31a93b419 100644 --- a/cli/cmd/component-delete.go +++ b/cli/cmd/component-delete.go @@ -63,7 +63,7 @@ func runDelete(cmd *cobra.Command, args []string) { } lokoConfig, diags := getLokoConfig() - if len(diags) > 0 { + if diags.HasErrors() { contextLogger.Fatal(diags) } From 3ab2eca70d194763efbbfafa85e9ff69cb1ab73c Mon Sep 17 00:00:00 2001 From: Mateusz Gozdek Date: Fri, 25 Sep 2020 12:38:17 +0200 Subject: [PATCH 5/5] cli/cmd: make initialize return a struct Which will make it easier to return error if some part of initialization fails. Signed-off-by: Mateusz Gozdek --- cli/cmd/cluster-apply.go | 26 +++++++++++++------------- cli/cmd/cluster-destroy.go | 6 +++--- cli/cmd/cluster.go | 18 ++++++++++++++++-- cli/cmd/utils.go | 4 ++-- 4 files changed, 34 insertions(+), 20 deletions(-) diff --git a/cli/cmd/cluster-apply.go b/cli/cmd/cluster-apply.go index 384c0edf2..3c493f396 100644 --- a/cli/cmd/cluster-apply.go +++ b/cli/cmd/cluster-apply.go @@ -64,12 +64,12 @@ func runClusterApply(cmd *cobra.Command, args []string) { //nolint:funlen func clusterApply(contextLogger *log.Entry) error { - ex, p, lokoConfig, assetDir := initialize(contextLogger) + c := initialize(contextLogger) - exists := clusterExists(contextLogger, ex) + exists := clusterExists(contextLogger, &c.terraformExecutor) if exists && !confirm { // TODO: We could plan to a file and use it when installing. - if err := ex.Plan(); err != nil { + if err := c.terraformExecutor.Plan(); err != nil { return fmt.Errorf("reconciling cluster state: %v", err) } @@ -80,18 +80,18 @@ func clusterApply(contextLogger *log.Entry) error { } } - if err := p.Apply(ex); err != nil { + if err := c.platform.Apply(&c.terraformExecutor); err != nil { return fmt.Errorf("applying platform: %v", err) } - fmt.Printf("\nYour configurations are stored in %s\n", assetDir) + fmt.Printf("\nYour configurations are stored in %s\n", c.assetDir) - kubeconfig, err := getKubeconfig(contextLogger, lokoConfig, true) + kubeconfig, err := getKubeconfig(contextLogger, c.lokomotiveConfig, true) if err != nil { return fmt.Errorf("getting kubeconfig: %v", err) } - if err := verifyCluster(kubeconfig, p.Meta().ExpectedNodes); err != nil { + if err := verifyCluster(kubeconfig, c.platform.Meta().ExpectedNodes); err != nil { return fmt.Errorf("verifying cluster: %v", err) } @@ -102,14 +102,14 @@ func clusterApply(contextLogger *log.Entry) error { } // Do controlplane upgrades only if cluster already exists and it is not a managed platform. - if exists && !p.Meta().Managed { + if exists && !c.platform.Meta().Managed { fmt.Printf("\nEnsuring that cluster controlplane is up to date.\n") cu := controlplaneUpdater{ kubeconfig: kubeconfig, - assetDir: assetDir, + assetDir: c.assetDir, contextLogger: *contextLogger, - ex: *ex, + ex: c.terraformExecutor, } charts := platform.CommonControlPlaneCharts() @@ -126,7 +126,7 @@ func clusterApply(contextLogger *log.Entry) error { } } - if ph, ok := p.(platform.PlatformWithPostApplyHook); ok { + if ph, ok := c.platform.(platform.PlatformWithPostApplyHook); ok { if err := ph.PostApplyHook(kubeconfig); err != nil { return fmt.Errorf("running platform post install hook: %v", err) } @@ -137,14 +137,14 @@ func clusterApply(contextLogger *log.Entry) error { } componentsToApply := []string{} - for _, component := range lokoConfig.RootConfig.Components { + for _, component := range c.lokomotiveConfig.RootConfig.Components { componentsToApply = append(componentsToApply, component.Name) } contextLogger.Println("Applying component configuration") if len(componentsToApply) > 0 { - if err := applyComponents(lokoConfig, kubeconfig, componentsToApply...); err != nil { + if err := applyComponents(c.lokomotiveConfig, kubeconfig, componentsToApply...); err != nil { return fmt.Errorf("applying component configuration: %v", err) } } diff --git a/cli/cmd/cluster-destroy.go b/cli/cmd/cluster-destroy.go index b8f635a43..75295e710 100644 --- a/cli/cmd/cluster-destroy.go +++ b/cli/cmd/cluster-destroy.go @@ -48,9 +48,9 @@ func runClusterDestroy(cmd *cobra.Command, args []string) { } func clusterDestroy(contextLogger *log.Entry) error { - ex, p, _, _ := initialize(contextLogger) + c := initialize(contextLogger) - if !clusterExists(contextLogger, ex) { + if !clusterExists(contextLogger, &c.terraformExecutor) { contextLogger.Println("Cluster already destroyed, nothing to do") return nil @@ -65,7 +65,7 @@ func clusterDestroy(contextLogger *log.Entry) error { } } - if err := p.Destroy(ex); err != nil { + if err := c.platform.Destroy(&c.terraformExecutor); err != nil { return fmt.Errorf("destroying cluster: %v", err) } diff --git a/cli/cmd/cluster.go b/cli/cmd/cluster.go index f82fead16..5e70253ba 100644 --- a/cli/cmd/cluster.go +++ b/cli/cmd/cluster.go @@ -41,9 +41,18 @@ func init() { RootCmd.AddCommand(clusterCmd) } +// cluster is a temporary helper struct to aggregate objects which are used +// for managing the cluster and components. +type cluster struct { + terraformExecutor terraform.Executor + platform platform.Platform + lokomotiveConfig *config.Config + assetDir string +} + // initialize does common initialization actions between cluster operations // and returns created objects to the caller for further use. -func initialize(contextLogger *log.Entry) (*terraform.Executor, platform.Platform, *config.Config, string) { +func initialize(contextLogger *log.Entry) *cluster { lokoConfig, diags := getLokoConfig() if diags.HasErrors() { contextLogger.Fatal(diags) @@ -85,7 +94,12 @@ func initialize(contextLogger *log.Entry) (*terraform.Executor, platform.Platfor ex := initializeTerraform(contextLogger, p, b) - return ex, p, lokoConfig, assetDir + return &cluster{ + terraformExecutor: *ex, + platform: p, + lokomotiveConfig: lokoConfig, + assetDir: assetDir, + } } // initializeTerraform initialized Terraform directory using given backend and platform diff --git a/cli/cmd/utils.go b/cli/cmd/utils.go index 964cd9391..27c20253f 100644 --- a/cli/cmd/utils.go +++ b/cli/cmd/utils.go @@ -187,11 +187,11 @@ func readKubeconfigFromTerraformState(contextLogger *log.Entry) ([]byte, error) contextLogger.Warn("Kubeconfig file not found in assets directory, pulling kubeconfig from " + "Terraform state, this might be slow. Run 'lokoctl cluster apply' to fix it.") - ex, _, _, _ := initialize(contextLogger) //nolint:dogsled + c := initialize(contextLogger) kubeconfig := "" - if err := ex.Output(kubeconfigTerraformOutputKey, &kubeconfig); err != nil { + if err := c.terraformExecutor.Output(kubeconfigTerraformOutputKey, &kubeconfig); err != nil { return nil, fmt.Errorf("reading kubeconfig file content from Terraform state: %w", err) }