From a5d0ae408bf39e0d27b9f5eb7927a208648f9143 Mon Sep 17 00:00:00 2001
From: Lucian Petrut <lpetrut@cloudbasesolutions.com>
Date: Tue, 24 Sep 2024 11:43:40 +0300
Subject: [PATCH] Return non-zero exit code in case of errors (#690)

* Return non-zero exit code in case of errors

At the moment, k8s and k8sd return 0 even if the command fails,
which is a problem especially when used inside scripts.

We'll ensure that a non-zero exit code is returned if the commands
fail.

* Update the cluster recovery command to use cobra "Run"

The cluster recovery command currently uses "RunE" and returns an
error in case of failures.

To stay consistent with other commands, we'll use "Run" and call
env.Exit as part of the command callback instead of returning the
errors.
---
 src/k8s/cmd/k8sd/k8sd.go                 |  2 +-
 src/k8s/cmd/k8sd/k8sd_cluster_recover.go | 27 ++++++++++++------------
 src/k8s/cmd/main.go                      | 18 +++++++++++++---
 3 files changed, 30 insertions(+), 17 deletions(-)

diff --git a/src/k8s/cmd/k8sd/k8sd.go b/src/k8s/cmd/k8sd/k8sd.go
index dedfafdf7..3fd28264a 100644
--- a/src/k8s/cmd/k8sd/k8sd.go
+++ b/src/k8s/cmd/k8sd/k8sd.go
@@ -91,7 +91,7 @@ func NewRootCmd(env cmdutil.ExecutionEnvironment) *cobra.Command {
 	addCommands(
 		cmd,
 		&cobra.Group{ID: "cluster", Title: "K8sd clustering commands:"},
-		newClusterRecoverCmd(),
+		newClusterRecoverCmd(env),
 	)
 
 	return cmd
diff --git a/src/k8s/cmd/k8sd/k8sd_cluster_recover.go b/src/k8s/cmd/k8sd/k8sd_cluster_recover.go
index 204dbbabd..661e0a646 100755
--- a/src/k8s/cmd/k8sd/k8sd_cluster_recover.go
+++ b/src/k8s/cmd/k8sd/k8sd_cluster_recover.go
@@ -24,6 +24,7 @@ import (
 	"golang.org/x/sys/unix"
 	"gopkg.in/yaml.v2"
 
+	cmdutil "github.com/canonical/k8s/cmd/util"
 	"github.com/canonical/k8s/pkg/log"
 	"github.com/canonical/k8s/pkg/utils"
 )
@@ -100,26 +101,28 @@ func logDebugf(format string, args ...interface{}) {
 
 }
 
-func newClusterRecoverCmd() *cobra.Command {
+func newClusterRecoverCmd(env cmdutil.ExecutionEnvironment) *cobra.Command {
 	cmd := &cobra.Command{
 		Use:   "cluster-recover",
 		Short: "Recover the cluster from this member if quorum is lost",
-		RunE: func(cmd *cobra.Command, args []string) error {
+		Run: func(cmd *cobra.Command, args []string) {
 			log.Configure(log.Options{
 				LogLevel:     rootCmdOpts.logLevel,
 				AddDirHeader: true,
 			})
 
 			if err := recoveryCmdPrechecks(cmd.Context()); err != nil {
-				return err
+				cmd.PrintErrf("Recovery precheck failed: %v\n", err)
+				env.Exit(1)
 			}
 
 			if clusterRecoverOpts.SkipK8sd {
-				cmd.Printf("Skipping k8sd recovery.")
+				cmd.Printf("Skipping k8sd recovery.\n")
 			} else {
 				k8sdTarballPath, err := recoverK8sd()
 				if err != nil {
-					return fmt.Errorf("failed to recover k8sd, error: %w", err)
+					cmd.PrintErrf("Failed to recover k8sd, error: %v\n", err)
+					env.Exit(1)
 				}
 				cmd.Printf("K8sd cluster changes applied.\n")
 				cmd.Printf("New database state saved to %s\n", k8sdTarballPath)
@@ -130,15 +133,15 @@ func newClusterRecoverCmd() *cobra.Command {
 			}
 
 			if clusterRecoverOpts.SkipK8sDqlite {
-				cmd.Printf("Skipping k8s-dqlite recovery.")
+				cmd.Printf("Skipping k8s-dqlite recovery.\n")
 			} else {
 				k8sDqlitePreRecoveryTarball, k8sDqlitePostRecoveryTarball, err := recoverK8sDqlite()
 				if err != nil {
-					return fmt.Errorf(
-						"failed to recover k8s-dqlite, error: %w, "+
-							"pre-recovery backup: %s",
-						err, k8sDqlitePreRecoveryTarball,
-					)
+					cmd.PrintErrf(
+						"Failed to recover k8s-dqlite, error: %v, "+
+							"pre-recovery backup: %s\n",
+						err, k8sDqlitePreRecoveryTarball)
+					env.Exit(1)
 				}
 				cmd.Printf("K8s-dqlite cluster changes applied.\n")
 				cmd.Printf("New database state saved to %s\n",
@@ -148,8 +151,6 @@ func newClusterRecoverCmd() *cobra.Command {
 					k8sDqlitePostRecoveryTarball, clusterRecoverOpts.K8sDqliteStateDir)
 				cmd.Printf("Pre-recovery database backup: %s\n\n", k8sDqlitePreRecoveryTarball)
 			}
-
-			return nil
 		},
 	}
 
diff --git a/src/k8s/cmd/main.go b/src/k8s/cmd/main.go
index 578449308..5e4b38bf1 100644
--- a/src/k8s/cmd/main.go
+++ b/src/k8s/cmd/main.go
@@ -30,14 +30,26 @@ func main() {
 
 	// choose command based on the binary name
 	base := filepath.Base(os.Args[0])
+	var err error
 	switch base {
 	case "k8s-apiserver-proxy":
-		k8s_apiserver_proxy.NewRootCmd(env).ExecuteContext(ctx)
+		err = k8s_apiserver_proxy.NewRootCmd(env).ExecuteContext(ctx)
 	case "k8sd":
-		k8sd.NewRootCmd(env).ExecuteContext(ctx)
+		err = k8sd.NewRootCmd(env).ExecuteContext(ctx)
 	case "k8s":
-		k8s.NewRootCmd(env).ExecuteContext(ctx)
+		err = k8s.NewRootCmd(env).ExecuteContext(ctx)
 	default:
 		panic(fmt.Errorf("invalid entrypoint name %q", base))
 	}
+
+	// Although k8s commands typically use Run instead of RunE and handle
+	// errors directly within the command execution, this acts as a safeguard in
+	// case any are overlooked.
+	//
+	// Furthermore, the Cobra framework may not invoke the "Run*" entry points
+	// at all in case of argument parsing errors, in which case we *need* to
+	// handle the errors here.
+	if err != nil {
+		env.Exit(1)
+	}
 }