Detecting and handling commands with uses / names that conflict with existing commands

[andyfeller]

Long time supporter, first time caller! 👋 Wanted to solicit maintainers' insights into how users avoid conflicting commands. I appreciate any guidance offered! ❤

Problem

As of v1.8.1, Command.AddCommand(cmds ...*Command) does not handle situations where a command added might conflict with the Use / Name of a previously added command, creating situations where commands can be overridden.

cobra/command.go

Lines 1305 to 1332 in e94f6d0

	// AddCommand adds one or more commands to this parent command.
	func (c *Command) AddCommand(cmds ...*Command) {
	for i, x := range cmds {
	if cmds[i] == c {
	panic("Command can't be a child of itself")
	}
	cmds[i].parent = c
	// update max lengths
	usageLen := len(x.Use)
	if usageLen > c.commandsMaxUseLen {
	c.commandsMaxUseLen = usageLen
	}
	commandPathLen := len(x.CommandPath())
	if commandPathLen > c.commandsMaxCommandPathLen {
	c.commandsMaxCommandPathLen = commandPathLen
	}
	nameLen := len(x.Name())
	if nameLen > c.commandsMaxNameLen {
	c.commandsMaxNameLen = nameLen
	}
	// If global normalization function exists, update all children
	if c.globNormFunc != nil {
	x.SetGlobalNormalizationFunc(c.globNormFunc)
	}
	c.commands = append(c.commands, x)
	c.commandsAreSorted = false
	}
	}

At first glance, it seems Command.Traverse(args []string) could be used to detect whether a command by name was previously added, however this searches too deeply.

cobra/command.go

Lines 790 to 831 in e94f6d0

	// Traverse the command tree to find the command, and parse args for
	// each parent.
	func (c *Command) Traverse(args []string) (*Command, []string, error) {
	flags := []string{}
	inFlag := false
	for i, arg := range args {
	switch {
	// A long flag with a space separated value
	case strings.HasPrefix(arg, "--") && !strings.Contains(arg, "="):
	// TODO: this isn't quite right, we should really check ahead for 'true' or 'false'
	inFlag = !hasNoOptDefVal(arg[2:], c.Flags())
	flags = append(flags, arg)
	continue
	// A short flag with a space separated value
	case strings.HasPrefix(arg, "-") && !strings.Contains(arg, "=") && len(arg) == 2 && !shortHasNoOptDefVal(arg[1:], c.Flags()):
	inFlag = true
	flags = append(flags, arg)
	continue
	// The value for a flag
	case inFlag:
	inFlag = false
	flags = append(flags, arg)
	continue
	// A flag without a value, or with an `=` separated value
	case isFlagArg(arg):
	flags = append(flags, arg)
	continue
	}
	cmd := c.findNext(arg)
	if cmd == nil {
	return c, args, nil
	}
	if err := c.ParseFlags(flags); err != nil {
	return nil, args, err
	}
	return cmd.Traverse(args[i+1:])
	}
	return c, args, nil
	}

This issue is aimed to understand how spf13/cobra expects multiple commands with overlap to behave, seeing if there are additional capabilities worth considering.

Background

The GitHub CLI has a static set of core commands we support while facilitating users being able to install and use extensions built by the community. These extensions are not specially namespaced, so it is possible for someone to create an extension as the same name of the core commandset to override them during execution.

https://github.com/cli/cli/blob/192f57ef429b4e89937c87dcbd330520730d4a3a/pkg/cmd/root/root.go#L172-L177

In this situation, a workaround would be iterating over the top-level commands looking for conflicting names:

	// Extensions
	em := f.ExtensionManager
	for _, e := range em.List() {
		var childCmd *cobra.Command
		for _, cc := range cmd.Commands() {
			if cc.Name() == e.Name() {
				childCmd = cc
				break
			}
		}
		if childCmd != nil {
			return nil, fmt.Errorf("cannot add extension %s as command by that name exists", e.Name())
		}
		extensionCmd := NewCmdExtension(io, em, e)
		cmd.AddCommand(extensionCmd)
	}

[marckhouzam]

Hi @andyfeller !

We can compare with kubectl’s approach. kubectl also allows users to define plugins and be named whatever they want. When starting, kubectl will generate it’s native commands first then it will attempt to create a cobra command for each plugin. However it first make sure that the name of the plug-in does not already exist as a native command when looking over each plugin it found.

If there’s a conflict, you can print an error message. kubectl also has a native plugin command, which will report a list of all conflicts and of valid plugins.

Helm on the other hand has a richer plug-in support which can block plug-ins from being installed if the name conflicts.

[andyfeller]

Thank you for taking the time and offering guidance, @marckhouzam! ❤ Let's see what kubectl and helm are doing to see how they might relate; feel free to correct any misinterpretations in my assessments below. 🙇

Digging into kubernetes/kubectl

• When the root command is being instantiated, kubectl is building collections of cobra.Command groups

  	groups := templates.CommandGroups{
  		{
  			Message: "Basic Commands (Beginner):",
  			Commands: []*cobra.Command{
  				create.NewCmdCreate(f, o.IOStreams),
  				expose.NewCmdExposeService(f, o.IOStreams),
  				run.NewCmdRun(f, o.IOStreams),
  				set.NewCmdSet(f, o.IOStreams),
  			},
  		},
  		{
  			Message: "Basic Commands (Intermediate):",
  			Commands: []*cobra.Command{
  				explain.NewCmdExplain("kubectl", f, o.IOStreams),
  				getCmd,
  				edit.NewCmdEdit(f, o.IOStreams),
  				delete.NewCmdDelete(f, o.IOStreams),
  			},
  		},
  		{
  			Message: "Deploy Commands:",
  			Commands: []*cobra.Command{
  				rollout.NewCmdRollout(f, o.IOStreams),
  				scale.NewCmdScale(f, o.IOStreams),
  				autoscale.NewCmdAutoscale(f, o.IOStreams),
  			},
  		},
  		{
  			Message: "Cluster Management Commands:",
  			Commands: []*cobra.Command{
  				certificates.NewCmdCertificate(f, o.IOStreams),
  				clusterinfo.NewCmdClusterInfo(f, o.IOStreams),
  				top.NewCmdTop(f, o.IOStreams),
  				drain.NewCmdCordon(f, o.IOStreams),
  				drain.NewCmdUncordon(f, o.IOStreams),
  				drain.NewCmdDrain(f, o.IOStreams),
  				taint.NewCmdTaint(f, o.IOStreams),
  			},
  		},
  		{
  			Message: "Troubleshooting and Debugging Commands:",
  			Commands: []*cobra.Command{
  				describe.NewCmdDescribe("kubectl", f, o.IOStreams),
  				logs.NewCmdLogs(f, o.IOStreams),
  				attach.NewCmdAttach(f, o.IOStreams),
  				cmdexec.NewCmdExec(f, o.IOStreams),
  				portforward.NewCmdPortForward(f, o.IOStreams),
  				proxyCmd,
  				cp.NewCmdCp(f, o.IOStreams),
  				auth.NewCmdAuth(f, o.IOStreams),
  				debug.NewCmdDebug(f, o.IOStreams),
  				events.NewCmdEvents(f, o.IOStreams),
  			},
  		},
  		{
  			Message: "Advanced Commands:",
  			Commands: []*cobra.Command{
  				diff.NewCmdDiff(f, o.IOStreams),
  				apply.NewCmdApply("kubectl", f, o.IOStreams),
  				patch.NewCmdPatch(f, o.IOStreams),
  				replace.NewCmdReplace(f, o.IOStreams),
  				wait.NewCmdWait(f, o.IOStreams),
  				kustomize.NewCmdKustomize(o.IOStreams),
  			},
  		},
  		{
  			Message: "Settings Commands:",
  			Commands: []*cobra.Command{
  				label.NewCmdLabel(f, o.IOStreams),
  				annotate.NewCmdAnnotate("kubectl", f, o.IOStreams),
  				completion.NewCmdCompletion(o.IOStreams.Out, ""),
  			},
  		},
  	}
  	groups.Add(cmds)

• Adding commands to these CommandGroup iterates over kubectl struct to see if it has the Command already

• Plugins are grouped under their own CommandGroup

  func GetPluginCommandGroup(kubectl *cobra.Command) templates.CommandGroup {
  	// Find root level
  	return templates.CommandGroup{
  		Message:  i18n.T("Subcommands provided by plugins:"),
  		Commands: registerPluginCommands(kubectl, false),
  	}
  }

• kubectl processes plugin names to derive command / commandset information

  		// Plugins are named "kubectl-<name>" or with more - such as
  		// "kubectl-<name>-<subcmd1>..."
  		rawPluginArgs := strings.Split(plugin, "-")[1:]
  		pluginArgs := rawPluginArgs[:1]
  		if list {
  			pluginArgs = rawPluginArgs
  		}
  
  		// Iterate through all segments, for kubectl-my_plugin-sub_cmd, we will end up with
  		// two iterations: one for my_plugin and one for sub_cmd.
  		for _, arg := range pluginArgs {
  			// Underscores (_) in plugin's filename are replaced with dashes(-)
  			// e.g. foo_bar -> foo-bar
  			args = append(args, strings.ReplaceAll(arg, "_", "-"))
  		}

• The only time ☝ is checking for duplicate commands is when kubectl is trying to list plugins here

  		// In order to avoid that the same plugin command is added more than once,
  		// find the lowest command given args from the root command
  		parentCmd, remainingArgs, _ := kubectl.Find(args)
  		if parentCmd == nil {
  			parentCmd = kubectl
  		}
  
  		for _, remainingArg := range remainingArgs {
  			cmd := &cobra.Command{
  				Use: remainingArg,
  				// Add a description that will be shown with completion choices.
  				// Make each one different by including the plugin name to avoid
  				// all plugins being grouped in a single line during completion for zsh.
  				Short:              fmt.Sprintf(i18n.T("The command %s is a plugin installed by the user"), remainingArg),
  				DisableFlagParsing: true,
  				// Allow plugins to provide their own completion choices
  				ValidArgsFunction: pluginCompletion,
  				// A Run is required for it to be a valid command
  				Run: func(cmd *cobra.Command, args []string) {},
  			}
  			// Add the plugin command to the list of user defined commands
  			userDefinedCommands = append(userDefinedCommands, cmd)
  
  			if list {
  				parentCmd.AddCommand(cmd)
  				parentCmd = cmd
  			}
  		}
  	}
  
  	return userDefinedCommands

In kubectl case, are core commands protected from plugins because they are in their own group? 🤔

Digging into helm/helm

• When the root command is being instantiated, loadPlugins is called

• Apparently helm hasn't solved how to avoid collisions with existing commands as noted by the todo here

  		// TODO: Make sure a command with this name does not already exist.
  		baseCmd.AddCommand(c)

In helm case, I think they have a similar issue as gh. 🤔