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

andyfeller commented 2 months ago

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.

https://github.com/spf13/cobra/blob/e94f6d0dd9a5e5738dca6bce03c4b1207ffbc0ec/command.go#L1305-L1332

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.

https://github.com/spf13/cobra/blob/e94f6d0dd9a5e5738dca6bce03c4b1207ffbc0ec/command.go#L790-L831

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 commented 2 months ago

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 commented 2 months ago

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. 🤔

spf13 / cobra