fix: cobra could throw error that will end up in json parser

Signed-off-by: Timur Tuktamyshev <[email protected]>

Author: timmilesdw

internal/controller/controller.go

@@ -78,7 +78,8 @@ func (c *HookController) ListHooksMeta() []pkg.HookMetadata {
 	return hooksmetas
 }
 
-var ErrHookIndexIsNotExists = errors.New("hook index is not exists")
+// TODO: fix typo, didn't fix now to not brake public API
+var ErrHookIndexIsNotExists = errors.New("hook index does not exist")
 
 func (c *HookController) RunHook(ctx context.Context, idx int) error {
 	hooks := c.registry.Hooks()

pkg/app/root.go

@@ -8,42 +8,53 @@ import (
 
 	"github.com/spf13/cobra"
 
+	"github.com/deckhouse/deckhouse/pkg/log"
+
 	"github.com/deckhouse/module-sdk/internal/controller"
 )
 
-func newCMD(controller *controller.HookController) *cmd {
+func newCMD(controller *controller.HookController, logger *log.Logger) *cmd {
 	return &cmd{
 		controller: controller,
+		logger:     logger,
 	}
 }
 
 type cmd struct {
 	controller *controller.HookController
+	logger     *log.Logger
 }
 
 // Execute adds all child commands to the root command and sets flags appropriately.
 // This is called by main.main(). It only needs to happen once to the rootCmd.
 func (c *cmd) Execute() {
-	rootCmd := c.rootCmd()
-	rootCmd.AddCommand(c.hooksCmd())
-	rootCmd.CompletionOptions.DisableDefaultCmd = true
+	rootCmd := c.buildCommand()
 
 	err := rootCmd.Execute()
 	if err != nil {
+		c.logger.Error("failed to execute root command", "error", err)
 		os.Exit(1)
 	}
 }
 
-// rootCmd represents the base command when called without any subcommands
-func (c *cmd) rootCmd() *cobra.Command {
-	return &cobra.Command{
+// buildCommand creates the complete command structure with all subcommands
+func (c *cmd) buildCommand() *cobra.Command {
+	rootCmd := &cobra.Command{
 		Use:   filepath.Base(os.Args[0]),
 		Short: "Binary with module hooks inside",
 		Long: `Inside this binary contains list of 
 precompiled hooks to use with corresponding module.`,
 	}
+	rootCmd.AddCommand(c.hooksCmd())
+	rootCmd.CompletionOptions.DisableDefaultCmd = true
+	// SilenceUsage and SilenceErrors prevent cobra from outputting
+	// "Error: ..." and "Usage: ..." which would break JSON output parsing
+	rootCmd.SilenceUsage = true
+	rootCmd.SilenceErrors = true
+	return rootCmd
 }
 
+// NOTE: all of the RunE errors and usage errors are silenced by SilenceUsage and SilenceErrors, to not cause "Invalid character 'E'" error
 func (c *cmd) hooksCmd() *cobra.Command {
 	hooksCmd := &cobra.Command{
 		Use:     "hooks",
@@ -67,77 +78,91 @@ func (c *cmd) hooksCmd() *cobra.Command {
 		},
 	})
 
-	hooksCmd.AddCommand(&cobra.Command{
+	configCmd := &cobra.Command{
 		Use:   "config",
 		Short: "Print hooks configs",
 		Long:  `Print list of hooks configs in json format`,
 		RunE: func(_ *cobra.Command, _ []string) error {
 			err := c.controller.PrintHookConfigs()
 			if err != nil {
+				c.logger.Error("can not print configs", "error", err)
 				return fmt.Errorf("can not print configs: %w", err)
 			}
 
 			return nil
 		},
-	})
+	}
+	hooksCmd.AddCommand(configCmd)
 
-	hooksCmd.AddCommand(&cobra.Command{
+	dumpCmd := &cobra.Command{
 		Use:    "dump",
 		Short:  "Dump hooks configs",
 		Long:   `Dump list of hooks configs in config.json file`,
 		Hidden: true,
 		RunE: func(_ *cobra.Command, _ []string) error {
 			err := c.controller.WriteHookConfigsInFile()
 			if err != nil {
+				c.logger.Error("can not write configs to file", "error", err)
 				return fmt.Errorf("can not write configs to file: %w", err)
 			}
 
 			fmt.Println("dump successfully")
 
 			return nil
 		},
-	})
+	}
+	hooksCmd.AddCommand(dumpCmd)
 
-	hooksCmd.AddCommand(&cobra.Command{
+	runCmd := &cobra.Command{
 		Use:    "run",
 		Short:  "Running hook",
 		Long:   `Run hook from binary registry`,
 		Hidden: true,
-		Args:   cobra.ExactArgs(1),
+		Args: func(cmd *cobra.Command, args []string) error {
+			if len(args) != 1 {
+				c.logger.Error("invalid number of arguments", "expected", 1, "received", len(args))
+
+				return fmt.Errorf("invalid number of arguments: expected 1, received %d", len(args))
+			}
+			return nil
+		},
 		RunE: func(cmd *cobra.Command, args []string) error {
 			ctx := cmd.Context()
 
 			idxRaw := args[0]
 			idx, err := strconv.Atoi(idxRaw)
 			if err != nil {
-				return fmt.Errorf("argument '%s' is not integer", idxRaw)
+				c.logger.Error("invalid argument", "argument", idxRaw, "error", err)
+
+				return fmt.Errorf("invalid argument: %w", err)
 			}
 
 			err = c.controller.RunHook(ctx, idx)
 			if err != nil {
-				return fmt.Errorf("run hook error: %w", err)
+				c.logger.Warn("hook shutdown", "error", err)
+				return fmt.Errorf("hook shutdown: %w", err)
 			}
 
 			return nil
 		},
-	})
+	}
+	hooksCmd.AddCommand(runCmd)
 
-	hooksCmd.AddCommand(&cobra.Command{
+	readyCmd := &cobra.Command{
 		Use:    "ready",
 		Short:  "Check readiness",
 		Long:   `Run readiness hook for module`,
 		Hidden: true,
-		RunE: func(cmd *cobra.Command, _ []string) error {
+		Run: func(cmd *cobra.Command, _ []string) {
 			ctx := cmd.Context()
 
 			err := c.controller.RunReadiness(ctx)
 			if err != nil {
-				return fmt.Errorf("run readiness hook error: %w", err)
+				c.logger.Warn("readiness hook shutdown", "error", err)
 			}
-
-			return nil
 		},
-	})
+	}
+	hooksCmd.AddCommand(readyCmd)
 
 	return hooksCmd
 }

pkg/app/root_test.go

@@ -0,0 +1,155 @@
+package app
+
+import (
+	"bytes"
+	"strings"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+
+	"github.com/deckhouse/deckhouse/pkg/log"
+
+	"github.com/deckhouse/module-sdk/internal/controller"
+)
+
+func Test_HooksRun_NoErrorOrUsageOutput(t *testing.T) {
+	tests := []struct {
+		name        string
+		args        []string
+		description string
+	}{
+		{
+			name:        "missing argument",
+			args:        []string{"hooks", "run"},
+			description: "should not output Error or Usage when argument is missing",
+		},
+		{
+			name:        "invalid argument format",
+			args:        []string{"hooks", "run", "not-a-number"},
+			description: "should not output Error or Usage when argument is not a number",
+		},
+		{
+			name:        "invalid hook index",
+			args:        []string{"hooks", "run", "999"},
+			description: "should not output Error or Usage when hook index is invalid",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			// Capture stdout and stderr
+			var stdout, stderr bytes.Buffer
+
+			// Create a mock controller
+			cfg := &controller.Config{
+				ModuleName: "test-module",
+				HookConfig: &controller.HookConfig{
+					BindingContextPath:    "in/binding_context.json",
+					ValuesPath:            "in/values_path.json",
+					ConfigValuesPath:      "in/config_values_path.json",
+					HookConfigPath:        "out/hook_config.json",
+					MetricsPath:           "out/metrics.json",
+					KubernetesPath:        "out/kubernetes.json",
+					ValuesJSONPath:        "out/values.json",
+					ConfigValuesJSONPath:  "out/config_values.json",
+					CreateFilesByYourself: true,
+				},
+				LogLevel: log.LevelFatal,
+			}
+
+			// Create logger that writes to buffer instead of stderr
+			logBuf := bytes.NewBuffer(nil)
+			logger := log.Default()
+			logger.SetOutput(logBuf)
+
+			hookController := controller.NewHookController(cfg, logger)
+
+			// Create command structure with our test logger
+			cmd := newCMD(hookController, logger)
+
+			// Build complete command structure
+			rootCmd := cmd.buildCommand()
+			rootCmd.SetOut(&stdout)
+			rootCmd.SetErr(&stderr)
+
+			// Set up test arguments
+			rootCmd.SetArgs(tt.args)
+
+			// Execute command
+			_ = rootCmd.Execute()
+
+			// Combine outputs (stdout, stderr, and log output)
+			output := stdout.String() + stderr.String() + logBuf.String()
+			// Check that Error: (from cobra) is not in output
+			// We check for "Error:" with capital E, which is how cobra outputs errors
+			// JSON logs may contain "error" field, but not "Error:" prefix from cobra
+			assert.NotContains(t, output, "Error:", tt.description+": should not contain 'Error:' from cobra")
+
+			// Check that Usage: (from cobra) is not in output
+			// We check for "Usage:" with capital U, which is how cobra outputs usage
+			assert.NotContains(t, output, "Usage:", tt.description+": should not contain 'Usage:' from cobra")
+		})
+	}
+}
+
+func Test_HooksConfig_NoErrorOrUsageOutput(t *testing.T) {
+	// Capture stdout and stderr
+	var stdout, stderr bytes.Buffer
+
+	// Create a mock controller
+	cfg := &controller.Config{
+		ModuleName: "test-module",
+		HookConfig: &controller.HookConfig{
+			BindingContextPath:    "in/binding_context.json",
+			ValuesPath:            "in/values_path.json",
+			ConfigValuesPath:      "in/config_values_path.json",
+			HookConfigPath:        "out/hook_config.json",
+			MetricsPath:           "out/metrics.json",
+			KubernetesPath:        "out/kubernetes.json",
+			ValuesJSONPath:        "out/values.json",
+			ConfigValuesJSONPath:  "out/config_values.json",
+			CreateFilesByYourself: true,
+		},
+		LogLevel: log.LevelFatal,
+	}
+
+	// Create logger that writes to buffer instead of stderr
+	logBuf := bytes.Buffer{}
+	logger := log.Default()
+	logger.SetOutput(&logBuf)
+
+	hookController := controller.NewHookController(cfg, logger)
+
+	// Create command structure with our test logger
+	cmd := newCMD(hookController, logger)
+
+	// Build complete command structure
+	rootCmd := cmd.buildCommand()
+	rootCmd.SetOut(&stdout)
+	rootCmd.SetErr(&stderr)
+
+	// Set up test arguments
+	rootCmd.SetArgs([]string{"hooks", "config"})
+
+	// Execute command
+	err := rootCmd.Execute()
+
+	// Combine outputs (stdout, stderr, and log output)
+	output := stdout.String() + stderr.String() + logBuf.String()
+
+	// Check that Error: (from cobra) is not in output
+	// We check for "Error:" with capital E, which is how cobra outputs errors
+	// JSON logs may contain "error" field, but not "Error:" prefix from cobra
+	assert.NotContains(t, output, "Error:", "config command should not contain 'Error:' from cobra")
+
+	// Check that Usage: (from cobra) is not in output
+	// We check for "Usage:" with capital U, which is how cobra outputs usage
+	assert.NotContains(t, output, "Usage:", "config command should not contain 'Usage:' from cobra")
+
+	// Check that output is valid JSON (if there are hooks and no error)
+	// Only check stdout for JSON, not stderr or logs
+	if err == nil && len(stdout.String()) > 0 {
+		require.True(t, strings.HasPrefix(strings.TrimSpace(stdout.String()), "{"), "config command should output valid JSON")
+	}
+}

pkg/app/run.go

@@ -36,7 +36,7 @@ func Run(opts ...RunConfigOption) {
 
 	controller := controller.NewHookController(remapConfigToControllerConfig(cfg), logger.Named("hook-controller").With("module", cfg.ModuleName))
 
-	c := newCMD(controller)
+	c := newCMD(controller, logger)
 
 	c.Execute()
 }