From b497d28d4300cbe70a7d5ad37b11cd7783a1d646 Mon Sep 17 00:00:00 2001 From: "Lubomir I. Ivanov" Date: Tue, 3 Sep 2024 18:50:28 +0300 Subject: [PATCH] kubeadm: better error handling for unknown phases and commands If an unknown command or a phase is called consistently return the same error. If a command that has subcommands is called return an error. To achieve the above add a new util function RequireSubcommand() that sets NoArgs and RunE for regular commands or a "phase" command. Remove MacroCommandLongDescription and just return an error that a subcommand is required from the phase runner. Fix minor comments capitalization. Perform other minor fixes in util/error.go. --- cmd/kubeadm/app/cmd/certs.go | 7 ++--- cmd/kubeadm/app/cmd/config.go | 11 ++------ cmd/kubeadm/app/cmd/phases/init/addons.go | 1 - cmd/kubeadm/app/cmd/phases/init/certs.go | 1 - .../app/cmd/phases/init/controlplane.go | 1 - cmd/kubeadm/app/cmd/phases/init/etcd.go | 1 - cmd/kubeadm/app/cmd/phases/init/kubeconfig.go | 1 - .../app/cmd/phases/init/uploadconfig.go | 1 - cmd/kubeadm/app/cmd/phases/workflow/runner.go | 8 ++++-- cmd/kubeadm/app/cmd/token.go | 8 +----- cmd/kubeadm/app/cmd/upgrade/upgrade.go | 2 +- cmd/kubeadm/app/cmd/util/cmdutil.go | 28 ++++++++----------- cmd/kubeadm/app/cmd/util/documentation.go | 5 ---- cmd/kubeadm/app/util/error.go | 9 ++---- 14 files changed, 28 insertions(+), 56 deletions(-) diff --git a/cmd/kubeadm/app/cmd/certs.go b/cmd/kubeadm/app/cmd/certs.go index 3c01ee33194..47a4330861c 100644 --- a/cmd/kubeadm/app/cmd/certs.go +++ b/cmd/kubeadm/app/cmd/certs.go @@ -104,9 +104,9 @@ func newCmdCertsUtility(out io.Writer) *cobra.Command { cmd := &cobra.Command{ Use: "certs", Aliases: []string{"certificates"}, - Short: "Commands related to handling kubernetes certificates", - Run: cmdutil.SubCmdRun(), + Short: "Commands related to handling Kubernetes certificates", } + cmdutil.RequireSubcommand(cmd) cmd.AddCommand(newCmdCertsRenewal(out)) cmd.AddCommand(newCmdCertsExpiration(out, kubeadmconstants.KubernetesDir)) @@ -215,9 +215,8 @@ func newCmdCertsRenewal(out io.Writer) *cobra.Command { cmd := &cobra.Command{ Use: "renew", Short: "Renew certificates for a Kubernetes cluster", - Long: cmdutil.MacroCommandLongDescription, - Run: cmdutil.SubCmdRun(), } + cmdutil.RequireSubcommand(cmd) cmd.AddCommand(getRenewSubCommands(out, kubeadmconstants.KubernetesDir)...) diff --git a/cmd/kubeadm/app/cmd/config.go b/cmd/kubeadm/app/cmd/config.go index 3816bdc6546..71ac8c47f1d 100644 --- a/cmd/kubeadm/app/cmd/config.go +++ b/cmd/kubeadm/app/cmd/config.go @@ -62,13 +62,8 @@ func newCmdConfig(out io.Writer) *cobra.Command { initialized your cluster using kubeadm v1.7.x or lower, you must use the 'kubeadm init phase upload-config' command to create this ConfigMap. This is required so that 'kubeadm upgrade' can configure your upgraded cluster correctly. `), metav1.NamespaceSystem, constants.KubeadmConfigConfigMap), - // Without this callback, if a user runs just the "upload" - // command without a subcommand, or with an invalid subcommand, - // cobra will print usage information, but still exit cleanly. - // We want to return an error code in these cases so that the - // user knows that their command was invalid. - Run: cmdutil.SubCmdRun(), } + cmdutil.RequireSubcommand(cmd) options.AddKubeConfigFlag(cmd.PersistentFlags(), &kubeConfigFile) @@ -88,8 +83,8 @@ func newCmdConfigPrint(out io.Writer) *cobra.Command { Long: dedent.Dedent(` This command prints configurations for subcommands provided. For details, see: https://pkg.go.dev/k8s.io/kubernetes/cmd/kubeadm/app/apis/kubeadm#section-directories`), - Run: cmdutil.SubCmdRun(), } + cmdutil.RequireSubcommand(cmd) cmd.AddCommand(newCmdConfigPrintInitDefaults(out)) cmd.AddCommand(newCmdConfigPrintJoinDefaults(out)) cmd.AddCommand(newCmdConfigPrintResetDefaults(out)) @@ -360,8 +355,8 @@ func newCmdConfigImages(out io.Writer) *cobra.Command { cmd := &cobra.Command{ Use: "images", Short: "Interact with container images used by kubeadm", - Run: cmdutil.SubCmdRun(), } + cmdutil.RequireSubcommand(cmd) cmd.AddCommand(newCmdConfigImagesList(out, nil)) cmd.AddCommand(newCmdConfigImagesPull()) return cmd diff --git a/cmd/kubeadm/app/cmd/phases/init/addons.go b/cmd/kubeadm/app/cmd/phases/init/addons.go index cd54fe702ad..879731be29d 100644 --- a/cmd/kubeadm/app/cmd/phases/init/addons.go +++ b/cmd/kubeadm/app/cmd/phases/init/addons.go @@ -56,7 +56,6 @@ func NewAddonPhase() workflow.Phase { return workflow.Phase{ Name: "addon", Short: "Install required addons for passing conformance tests", - Long: cmdutil.MacroCommandLongDescription, Phases: []workflow.Phase{ { Name: "all", diff --git a/cmd/kubeadm/app/cmd/phases/init/certs.go b/cmd/kubeadm/app/cmd/phases/init/certs.go index ce2549a9e6f..a9343c47d5e 100644 --- a/cmd/kubeadm/app/cmd/phases/init/certs.go +++ b/cmd/kubeadm/app/cmd/phases/init/certs.go @@ -57,7 +57,6 @@ func NewCertsPhase() workflow.Phase { Short: "Certificate generation", Phases: newCertSubPhases(), Run: runCerts, - Long: cmdutil.MacroCommandLongDescription, } } diff --git a/cmd/kubeadm/app/cmd/phases/init/controlplane.go b/cmd/kubeadm/app/cmd/phases/init/controlplane.go index e69194e60d1..3920afe6706 100644 --- a/cmd/kubeadm/app/cmd/phases/init/controlplane.go +++ b/cmd/kubeadm/app/cmd/phases/init/controlplane.go @@ -66,7 +66,6 @@ func NewControlPlanePhase() workflow.Phase { phase := workflow.Phase{ Name: "control-plane", Short: "Generate all static Pod manifest files necessary to establish the control plane", - Long: cmdutil.MacroCommandLongDescription, Phases: []workflow.Phase{ { Name: "all", diff --git a/cmd/kubeadm/app/cmd/phases/init/etcd.go b/cmd/kubeadm/app/cmd/phases/init/etcd.go index 2dc6814e9cf..46615b501c1 100644 --- a/cmd/kubeadm/app/cmd/phases/init/etcd.go +++ b/cmd/kubeadm/app/cmd/phases/init/etcd.go @@ -47,7 +47,6 @@ func NewEtcdPhase() workflow.Phase { phase := workflow.Phase{ Name: "etcd", Short: "Generate static Pod manifest file for local etcd", - Long: cmdutil.MacroCommandLongDescription, Phases: []workflow.Phase{ newEtcdLocalSubPhase(), }, diff --git a/cmd/kubeadm/app/cmd/phases/init/kubeconfig.go b/cmd/kubeadm/app/cmd/phases/init/kubeconfig.go index 7e255b29fbb..42a4c5476d3 100644 --- a/cmd/kubeadm/app/cmd/phases/init/kubeconfig.go +++ b/cmd/kubeadm/app/cmd/phases/init/kubeconfig.go @@ -74,7 +74,6 @@ func NewKubeConfigPhase() workflow.Phase { return workflow.Phase{ Name: "kubeconfig", Short: "Generate all kubeconfig files necessary to establish the control plane and the admin kubeconfig file", - Long: cmdutil.MacroCommandLongDescription, Phases: []workflow.Phase{ { Name: "all", diff --git a/cmd/kubeadm/app/cmd/phases/init/uploadconfig.go b/cmd/kubeadm/app/cmd/phases/init/uploadconfig.go index 5b04e18192b..93719d98df7 100644 --- a/cmd/kubeadm/app/cmd/phases/init/uploadconfig.go +++ b/cmd/kubeadm/app/cmd/phases/init/uploadconfig.go @@ -65,7 +65,6 @@ func NewUploadConfigPhase() workflow.Phase { Name: "upload-config", Aliases: []string{"uploadconfig"}, Short: "Upload the kubeadm and kubelet configuration to a ConfigMap", - Long: cmdutil.MacroCommandLongDescription, Phases: []workflow.Phase{ { Name: "all", diff --git a/cmd/kubeadm/app/cmd/phases/workflow/runner.go b/cmd/kubeadm/app/cmd/phases/workflow/runner.go index 07e65f86d5e..63f7e79e821 100644 --- a/cmd/kubeadm/app/cmd/phases/workflow/runner.go +++ b/cmd/kubeadm/app/cmd/phases/workflow/runner.go @@ -23,6 +23,8 @@ import ( "github.com/pkg/errors" "github.com/spf13/cobra" "github.com/spf13/pflag" + + cmdutil "k8s.io/kubernetes/cmd/kubeadm/app/cmd/util" ) // phaseSeparator defines the separator to be used when concatenating nested @@ -332,8 +334,9 @@ func (e *Runner) BindToCommand(cmd *cobra.Command) { // adds the phases subcommand phaseCommand := &cobra.Command{ Use: "phase", - Short: fmt.Sprintf("Use this command to invoke single phase of the %s workflow", cmd.Name()), + Short: fmt.Sprintf("Use this command to invoke single phase of the %q workflow", cmd.Name()), } + cmdutil.RequireSubcommand(phaseCommand) cmd.AddCommand(phaseCommand) @@ -363,7 +366,8 @@ func (e *Runner) BindToCommand(cmd *cobra.Command) { RunE: func(cmd *cobra.Command, args []string) error { // if the phase has subphases, print the help and exits if len(p.Phases) > 0 { - return cmd.Help() + _ = cmd.Help() + return cmdutil.ErrorSubcommandRequired } // overrides the command triggering the Runner using the phaseCmd diff --git a/cmd/kubeadm/app/cmd/token.go b/cmd/kubeadm/app/cmd/token.go index 6b665549fb6..e4070ae0337 100644 --- a/cmd/kubeadm/app/cmd/token.go +++ b/cmd/kubeadm/app/cmd/token.go @@ -77,14 +77,8 @@ func newCmdToken(out io.Writer, errW io.Writer) *cobra.Command { You can read more about bootstrap tokens here: https://kubernetes.io/docs/admin/bootstrap-tokens/ `), - - // Without this callback, if a user runs just the "token" - // command without a subcommand, or with an invalid subcommand, - // cobra will print usage information, but still exit cleanly. - // We want to return an error code in these cases so that the - // user knows that their command was invalid. - Run: cmdutil.SubCmdRun(), } + cmdutil.RequireSubcommand(tokenCmd) options.AddKubeConfigFlag(tokenCmd.PersistentFlags(), &kubeConfigFile) tokenCmd.PersistentFlags().BoolVar(&dryRun, diff --git a/cmd/kubeadm/app/cmd/upgrade/upgrade.go b/cmd/kubeadm/app/cmd/upgrade/upgrade.go index 1145196557c..8e71f5df2a2 100644 --- a/cmd/kubeadm/app/cmd/upgrade/upgrade.go +++ b/cmd/kubeadm/app/cmd/upgrade/upgrade.go @@ -56,8 +56,8 @@ func NewCmdUpgrade(out io.Writer) *cobra.Command { cmd := &cobra.Command{ Use: "upgrade", Short: "Upgrade your cluster smoothly to a newer version with this command", - Run: cmdutil.SubCmdRun(), } + cmdutil.RequireSubcommand(cmd) cmd.AddCommand(newCmdApply(flags)) cmd.AddCommand(newCmdPlan(flags)) diff --git a/cmd/kubeadm/app/cmd/util/cmdutil.go b/cmd/kubeadm/app/cmd/util/cmdutil.go index de898102a5f..67148ffb927 100644 --- a/cmd/kubeadm/app/cmd/util/cmdutil.go +++ b/cmd/kubeadm/app/cmd/util/cmdutil.go @@ -34,27 +34,23 @@ import ( "k8s.io/kubernetes/cmd/kubeadm/app/cmd/options" kubeadmconstants "k8s.io/kubernetes/cmd/kubeadm/app/constants" - kubeadmutil "k8s.io/kubernetes/cmd/kubeadm/app/util" "k8s.io/kubernetes/cmd/kubeadm/app/util/apiclient" kubeconfigutil "k8s.io/kubernetes/cmd/kubeadm/app/util/kubeconfig" ) -// SubCmdRun returns a function that handles a case where a subcommand must be specified -// Without this callback, if a user runs just the command without a subcommand, -// or with an invalid subcommand, cobra will print usage information, but still exit cleanly. -func SubCmdRun() func(c *cobra.Command, args []string) { - return func(c *cobra.Command, args []string) { - if len(args) > 0 { - kubeadmutil.CheckErr(usageErrorf(c, "invalid subcommand %q", strings.Join(args, " "))) - } - c.Help() - kubeadmutil.CheckErr(kubeadmutil.ErrExit) - } -} +// ErrorSubcommandRequired is an error returned when a parent command cannot be executed. +// It starts with a new line so that it's separated from previous information like a Help() screen. +var ErrorSubcommandRequired = errors.New("\nerror: subcommand is required") -func usageErrorf(c *cobra.Command, format string, args ...interface{}) error { - msg := fmt.Sprintf(format, args...) - return errors.Errorf("%s\nSee '%s -h' for help and examples", msg, c.CommandPath()) +// RequireSubcommand can be used to set an empty Run function and NoArgs on a Cobra command. +// This handles a case where a subcommand must be specified for a parent command 'c'. +// If no subcommand is specified the CLI exist with an error. +func RequireSubcommand(c *cobra.Command) { + c.RunE = func(c *cobra.Command, args []string) error { + _ = c.Help() + return ErrorSubcommandRequired + } + c.Args = cobra.NoArgs } // ValidateExactArgNumber validates that the required top-level arguments are specified diff --git a/cmd/kubeadm/app/cmd/util/documentation.go b/cmd/kubeadm/app/cmd/util/documentation.go index 29c4c4b3f6e..9545a81f653 100644 --- a/cmd/kubeadm/app/cmd/util/documentation.go +++ b/cmd/kubeadm/app/cmd/util/documentation.go @@ -25,11 +25,6 @@ var ( AlphaDisclaimer = ` Alpha Disclaimer: this command is currently alpha. ` - - // MacroCommandLongDescription provide a standard description for "macro" commands - MacroCommandLongDescription = LongDesc(` - This command is not meant to be run on its own. See list of available subcommands. - `) ) // LongDesc is designed to help with producing better long command line descriptions in code. diff --git a/cmd/kubeadm/app/util/error.go b/cmd/kubeadm/app/util/error.go index 17271f024d3..fa9cacdc7c6 100644 --- a/cmd/kubeadm/app/util/error.go +++ b/cmd/kubeadm/app/util/error.go @@ -38,8 +38,6 @@ const ( ) var ( - // ErrInvalidSubCommandMsg is an error message returned on invalid subcommands - ErrInvalidSubCommandMsg = "invalid subcommand" // ErrExit is an error returned when kubeadm is about to exit ErrExit = errors.New("exit") ) @@ -80,10 +78,10 @@ func checkErr(err error, handleErr func(string, int)) { } msg := fmt.Sprintf("%s\nTo see the stack trace of this error execute with --v=5 or higher", err.Error()) - // check if the verbosity level in klog is high enough and print a stack trace. + // Check if the verbosity level in klog is high enough and print a stack trace. f := flag.CommandLine.Lookup("v") if f != nil { - // assume that the "v" flag contains a parseable Int32 as per klog's "Level" type alias, + // Assume that the "v" flag contains a parsable Int32 as per klog's "Level" type alias, // thus no error from ParseInt is handled here. if v, e := strconv.ParseInt(f.Value.String(), 10, 32); e == nil { // https://git.k8s.io/community/contributors/devel/sig-instrumentation/logging.md @@ -97,15 +95,12 @@ func checkErr(err error, handleErr func(string, int)) { switch { case err == ErrExit: handleErr("", DefaultErrorExitCode) - case strings.Contains(err.Error(), ErrInvalidSubCommandMsg): - handleErr(err.Error(), DefaultErrorExitCode) default: switch err.(type) { case preflightError: handleErr(msg, PreFlightExitCode) case errorsutil.Aggregate: handleErr(msg, ValidationExitCode) - default: handleErr(msg, DefaultErrorExitCode) }