🐛 fix: resolve panic when multiple plugins define the same flag (e.g. edit with go/v4 + helm) (#5506)

fix: resolve panic when multiple plugins define the same flag (e.g. edit with go/v4 + helm)

- Merge duplicate flags instead of binding twice; sync parsed value to all plugins in PreRunE.
- Aggregate flag help as "For plugin (key): desc AND for plugin (key): desc".
- Return a clear error when the same flag name is used with different types (e.g. bool vs string).

Generated-by: Cursor/Claude

Author: camilamacedo86

pkg/cli/cmd_helpers.go

@@ -20,8 +20,10 @@ import (
 	"errors"
 	"fmt"
 	"os"
+	"strings"
 
 	"github.com/spf13/cobra"
+	"github.com/spf13/pflag"
 
 	"sigs.k8s.io/kubebuilder/v4/pkg/config"
 	"sigs.k8s.io/kubebuilder/v4/pkg/config/store"
@@ -115,8 +117,9 @@ func collectSubcommands(
 	}}
 }
 
-// applySubcommandHooks runs the initialization hooks and configures the commands pre-run,
-// run, and post-run hooks with the appropriate execution hooks.
+// applySubcommandHooks runs the initialization hooks and wires pre-run, run, and post-run for the command.
+// Used by init, create api, create webhook, and edit. When several plugins define the same flag,
+// one flag is shown and its value is synced to all plugins after parse.
 func (c *CLI) applySubcommandHooks(
 	cmd *cobra.Command,
 	subcommands []keySubcommandTuple,
@@ -143,18 +146,23 @@ func (c *CLI) applySubcommandHooks(
 		}
 	}
 
-	options := initializationHooks(cmd, subcommands, c.metadata())
+	result, err := initializationHooks(cmd, subcommands, c.metadata())
+	if err != nil {
+		cmdErr(cmd, err)
+		return
+	}
 
 	factory := executionHooksFactory{
-		fs:             c.fs,
-		store:          yamlstore.New(c.fs),
-		subcommands:    subcommands,
-		errorMessage:   errorMessage,
-		projectVersion: c.projectVersion,
-		pluginChain:    pluginChain,
-		cliVersion:     c.cliVersion,
+		fs:                  c.fs,
+		store:               yamlstore.New(c.fs),
+		subcommands:         subcommands,
+		errorMessage:        errorMessage,
+		projectVersion:      c.projectVersion,
+		pluginChain:         pluginChain,
+		cliVersion:          c.cliVersion,
+		duplicateFlagValues: result.duplicateFlagValues,
 	}
-	cmd.PreRunE = factory.preRunEFunc(options, createConfig)
+	cmd.PreRunE = factory.preRunEFunc(result.options, createConfig)
 	cmd.RunE = factory.runEFunc()
 	cmd.PostRunE = factory.postRunEFunc()
 }
@@ -166,12 +174,77 @@ func (c *CLI) appendPluginTable(cmd *cobra.Command, filter func(plugin.Plugin) b
 	cmd.Long = fmt.Sprintf("%s\n%s:\n\n%s\n", cmd.Long, title, pluginTable)
 }
 
-// initializationHooks executes update metadata and bind flags plugin hooks.
+// initHooksResult holds the result of initializationHooks: resource options and
+// duplicate-flag values to sync after parse.
+type initHooksResult struct {
+	options             *resourceOptions
+	duplicateFlagValues map[string][]pflag.Value
+}
+
+// mergeFlagSetInto merges flags from src into dest using AddFlagSet. If a flag name already exists,
+// the flag is not added again; its Value is stored in duplicateValues for later sync and the existing
+// Usage is extended. Returns an error if the same flag name is used with a different value type.
+func mergeFlagSetInto(
+	dest *pflag.FlagSet,
+	src *pflag.FlagSet,
+	duplicateValues map[string][]pflag.Value,
+	pluginKey string,
+	firstPluginByFlag map[string]string,
+) error {
+	destNames := make(map[string]struct{})
+	dest.VisitAll(func(f *pflag.Flag) {
+		destNames[f.Name] = struct{}{}
+	})
+	dest.AddFlagSet(src)
+
+	var err error
+	src.VisitAll(func(flag *pflag.Flag) {
+		if err != nil {
+			return
+		}
+		existing := dest.Lookup(flag.Name)
+		if _, wasInDest := destNames[flag.Name]; !wasInDest {
+			firstPluginByFlag[flag.Name] = pluginKey
+			existing.Usage = "For plugin (" + pluginKey + "): " + strings.TrimSpace(flag.Usage)
+			return
+		}
+		if existing.Value.Type() != flag.Value.Type() {
+			firstKey := firstPluginByFlag[flag.Name]
+			err = fmt.Errorf(
+				"plugins %q and %q use the same flag name %q but expect different value types: one %s, other %s",
+				firstKey, pluginKey, flag.Name, existing.Value.Type(), flag.Value.Type(),
+			)
+			return
+		}
+		duplicateValues[flag.Name] = append(duplicateValues[flag.Name], flag.Value)
+		existing.Usage += " AND for plugin (" + pluginKey + "): " + strings.TrimSpace(flag.Usage)
+	})
+	return err
+}
+
+// syncDuplicateFlags copies the parsed value of each flag to all duplicate Values from merge.
+// Call after the command has parsed flags (e.g. at the start of PreRunE).
+func syncDuplicateFlags(flags *pflag.FlagSet, duplicateValues map[string][]pflag.Value) {
+	for name, values := range duplicateValues {
+		parsed := flags.Lookup(name)
+		if parsed == nil {
+			continue
+		}
+		srcVal := parsed.Value.String()
+		for _, v := range values {
+			_ = v.Set(srcVal)
+		}
+	}
+}
+
+// initializationHooks runs update-metadata and bind-flags hooks. When multiple plugins bind the same
+// flag, one flag is used and its value is synced to all after parse; usage text is aggregated.
+// Returns an error if the same flag name is used with different value types (e.g. bool vs string).
 func initializationHooks(
 	cmd *cobra.Command,
 	subcommands []keySubcommandTuple,
 	meta plugin.CLIMetadata,
-) *resourceOptions {
+) (*initHooksResult, error) {
 	// Update metadata hook.
 	subcmdMeta := plugin.SubcommandMetadata{
 		Description: cmd.Long,
@@ -197,14 +270,21 @@ func initializationHooks(
 		options = bindResourceFlags(cmd.Flags())
 	}
 
-	// Bind flags hook.
+	// Bind flags hook: each plugin binds to a temporary FlagSet, then we merge into the command so
+	// duplicate names do not panic; values are synced after parse and help text is aggregated.
+	duplicateValues := make(map[string][]pflag.Value)
+	firstPluginByFlag := make(map[string]string)
 	for _, tuple := range subcommands {
 		if subcommand, hasFlags := tuple.subcommand.(plugin.HasFlags); hasFlags {
-			subcommand.BindFlags(cmd.Flags())
+			tmpSet := pflag.NewFlagSet(cmd.Name(), pflag.ExitOnError)
+			subcommand.BindFlags(tmpSet)
+			if err := mergeFlagSetInto(cmd.Flags(), tmpSet, duplicateValues, tuple.key, firstPluginByFlag); err != nil {
+				return nil, err
+			}
 		}
 	}
 
-	return options
+	return &initHooksResult{options: options, duplicateFlagValues: duplicateValues}, nil
 }
 
 type executionHooksFactory struct {
@@ -223,6 +303,8 @@ type executionHooksFactory struct {
 	pluginChain []string
 	// cliVersion is the version of the CLI.
 	cliVersion string
+	// duplicateFlagValues maps flag names to Values to sync from the parsed flag in PreRunE.
+	duplicateFlagValues map[string][]pflag.Value
 }
 
 func (factory *executionHooksFactory) forEach(cb func(subcommand plugin.Subcommand) error, errorMessage string) error {
@@ -319,7 +401,10 @@ func (factory *executionHooksFactory) preRunEFunc(
 	options *resourceOptions,
 	createConfig bool,
 ) func(*cobra.Command, []string) error {
-	return func(*cobra.Command, []string) error {
+	return func(cmd *cobra.Command, _ []string) error {
+		if len(factory.duplicateFlagValues) > 0 {
+			syncDuplicateFlags(cmd.Flags(), factory.duplicateFlagValues)
+		}
 		if createConfig {
 			// Check if a project configuration is already present.
 			if err := factory.store.Load(); err == nil || !errors.Is(err, os.ErrNotExist) {

pkg/cli/cmd_helpers_test.go

@@ -22,6 +22,7 @@ import (
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
 	"github.com/spf13/cobra"
+	"github.com/spf13/pflag"
 
 	"sigs.k8s.io/kubebuilder/v4/pkg/config"
 	"sigs.k8s.io/kubebuilder/v4/pkg/machinery"
@@ -259,6 +260,183 @@ var _ = Describe("cmd_helpers", func() {
 			Expect(result).To(BeEmpty())
 		})
 	})
+
+	Context("duplicate flag handling (mergeFlagSetInto, syncDuplicateFlags)", func() {
+		It("should not panic when merging two FlagSets that define the same flag name (same type)", func() {
+			dest := pflag.NewFlagSet("dest", pflag.ExitOnError)
+			src := pflag.NewFlagSet("src", pflag.ExitOnError)
+			duplicateValues := make(map[string][]pflag.Value)
+			firstPluginByFlag := make(map[string]string)
+
+			var destBool bool
+			var srcBool bool
+			dest.BoolVar(&destBool, "force", false, "overwrite files (plugin A)")
+			src.BoolVar(&srcBool, "force", false, "regenerate all files (plugin B)")
+
+			err := mergeFlagSetInto(dest, src, duplicateValues, "pluginB/v1", firstPluginByFlag)
+			Expect(err).NotTo(HaveOccurred())
+			Expect(dest.Lookup("force")).NotTo(BeNil())
+			Expect(duplicateValues["force"]).To(HaveLen(1))
+		})
+
+		It("should aggregate help text as For plugin (key): desc AND for plugin (key): desc", func() {
+			dest := pflag.NewFlagSet("dest", pflag.ExitOnError)
+			src := pflag.NewFlagSet("src", pflag.ExitOnError)
+			duplicateValues := make(map[string][]pflag.Value)
+			firstPluginByFlag := make(map[string]string)
+
+			var a, b bool
+			dest.BoolVar(&a, "force", false, "overwrite files (plugin A)")
+			src.BoolVar(&b, "force", false, "regenerate all files (plugin B)")
+
+			err := mergeFlagSetInto(dest, src, duplicateValues, "pluginB/v1", firstPluginByFlag)
+			Expect(err).NotTo(HaveOccurred())
+
+			flag := dest.Lookup("force")
+			Expect(flag).NotTo(BeNil())
+			Expect(flag.Usage).To(ContainSubstring("overwrite files (plugin A)"))
+			Expect(flag.Usage).To(ContainSubstring("AND for plugin (pluginB/v1):"))
+			Expect(flag.Usage).To(ContainSubstring("regenerate all files (plugin B)"))
+		})
+
+		It("should prefix first plugin with For plugin (key): when both flags merged via mergeFlagSetInto", func() {
+			dest := pflag.NewFlagSet("dest", pflag.ExitOnError)
+			pluginA := pflag.NewFlagSet("a", pflag.ExitOnError)
+			pluginB := pflag.NewFlagSet("b", pflag.ExitOnError)
+			duplicateValues := make(map[string][]pflag.Value)
+			firstPluginByFlag := make(map[string]string)
+
+			var a, b bool
+			pluginA.BoolVar(&a, "force", false, "overwrite files (plugin A)")
+			pluginB.BoolVar(&b, "force", false, "regenerate all files (plugin B)")
+
+			Expect(mergeFlagSetInto(dest, pluginA, duplicateValues, "pluginA/v1", firstPluginByFlag)).NotTo(HaveOccurred())
+			Expect(mergeFlagSetInto(dest, pluginB, duplicateValues, "pluginB/v1", firstPluginByFlag)).NotTo(HaveOccurred())
+
+			flag := dest.Lookup("force")
+			Expect(flag).NotTo(BeNil())
+			Expect(flag.Usage).To(Equal(
+				"For plugin (pluginA/v1): overwrite files (plugin A) AND for plugin (pluginB/v1): regenerate all files (plugin B)"))
+		})
+
+		It("should show full plugin keys in aggregated usage", func() {
+			dest := pflag.NewFlagSet("dest", pflag.ExitOnError)
+			goPlugin := pflag.NewFlagSet("go", pflag.ExitOnError)
+			helmPlugin := pflag.NewFlagSet("helm", pflag.ExitOnError)
+			duplicateValues := make(map[string][]pflag.Value)
+			firstPluginByFlag := make(map[string]string)
+
+			var a, b bool
+			goPlugin.BoolVar(&a, "force", false, "overwrite scaffolded files to apply changes (manual edits may be lost)")
+			helmPlugin.BoolVar(&b, "force", false, "if true, regenerates all the files")
+
+			Expect(mergeFlagSetInto(dest, goPlugin, duplicateValues, "base.go.kubebuilder.io/v4", firstPluginByFlag)).
+				NotTo(HaveOccurred())
+			Expect(mergeFlagSetInto(dest, helmPlugin, duplicateValues, "helm.kubebuilder.io/v2-alpha", firstPluginByFlag)).
+				NotTo(HaveOccurred())
+
+			flag := dest.Lookup("force")
+			Expect(flag).NotTo(BeNil())
+			expectedUsage := "For plugin (base.go.kubebuilder.io/v4): overwrite scaffolded files to apply changes " +
+				"(manual edits may be lost) AND for plugin (helm.kubebuilder.io/v2-alpha): if true, regenerates all the files"
+			Expect(flag.Usage).To(Equal(expectedUsage))
+		})
+
+		It("should return error when same flag name is bound with different value types", func() {
+			dest := pflag.NewFlagSet("dest", pflag.ExitOnError)
+			src := pflag.NewFlagSet("src", pflag.ExitOnError)
+			duplicateValues := make(map[string][]pflag.Value)
+			firstPluginByFlag := make(map[string]string)
+			firstPluginByFlag["flag"] = "pluginA/v1" // dest already has this flag from a previous plugin
+
+			var a bool
+			var b string
+			dest.BoolVar(&a, "flag", false, "bool usage (plugin A)")
+			src.StringVar(&b, "flag", "", "string usage (plugin B)")
+
+			err := mergeFlagSetInto(dest, src, duplicateValues, "pluginB/v1", firstPluginByFlag)
+			Expect(err).To(HaveOccurred())
+			Expect(err.Error()).To(ContainSubstring("same flag name"))
+			Expect(err.Error()).To(ContainSubstring("different value types"))
+			Expect(err.Error()).To(ContainSubstring("flag"))
+			Expect(err.Error()).To(ContainSubstring("bool"))
+			Expect(err.Error()).To(ContainSubstring("string"))
+			Expect(err.Error()).To(ContainSubstring("pluginA/v1"))
+			Expect(err.Error()).To(ContainSubstring("pluginB/v1"))
+		})
+
+		It("should sync parsed value to duplicate Values after syncDuplicateFlags", func() {
+			flags := pflag.NewFlagSet("cmd", pflag.ExitOnError)
+			var mainVal, dupVal bool
+			flags.BoolVar(&mainVal, "force", false, "usage")
+			tmpFS := pflag.NewFlagSet("", pflag.ExitOnError)
+			tmpFS.BoolVar(&dupVal, "force", false, "")
+			duplicateValues := map[string][]pflag.Value{
+				"force": {tmpFS.Lookup("force").Value},
+			}
+
+			Expect(flags.Parse([]string{"--force", "true"})).NotTo(HaveOccurred())
+			Expect(mainVal).To(BeTrue())
+			Expect(dupVal).To(BeFalse())
+
+			syncDuplicateFlags(flags, duplicateValues)
+			Expect(dupVal).To(BeTrue())
+		})
+
+		It("should give all plugins in the chain the same value for a shared flag (e.g. --force)", func() {
+			cmdFlags := pflag.NewFlagSet("edit", pflag.ExitOnError)
+			pluginA := pflag.NewFlagSet("pluginA", pflag.ExitOnError)
+			pluginB := pflag.NewFlagSet("pluginB", pflag.ExitOnError)
+			var forceA, forceB bool
+			pluginA.BoolVar(&forceA, "force", false, "plugin A force")
+			pluginB.BoolVar(&forceB, "force", false, "plugin B force")
+
+			duplicateValues := make(map[string][]pflag.Value)
+			firstPluginByFlag := make(map[string]string)
+			Expect(mergeFlagSetInto(cmdFlags, pluginA, duplicateValues, "pluginA/v1", firstPluginByFlag)).NotTo(HaveOccurred())
+			Expect(mergeFlagSetInto(cmdFlags, pluginB, duplicateValues, "pluginB/v1", firstPluginByFlag)).NotTo(HaveOccurred())
+
+			Expect(cmdFlags.Parse([]string{"--force", "true"})).NotTo(HaveOccurred())
+			syncDuplicateFlags(cmdFlags, duplicateValues)
+			Expect(forceA).To(BeTrue(), "plugin A must receive the value passed by the user")
+			Expect(forceB).To(BeTrue(), "plugin B must receive the same value as the command")
+		})
+
+		It("should sync string flag value to duplicate Values", func() {
+			flags := pflag.NewFlagSet("cmd", pflag.ExitOnError)
+			var mainVal, dupVal string
+			flags.StringVar(&mainVal, "name", "", "name usage")
+			tmpFS := pflag.NewFlagSet("", pflag.ExitOnError)
+			tmpFS.StringVar(&dupVal, "name", "", "")
+			duplicateValues := map[string][]pflag.Value{
+				"name": {tmpFS.Lookup("name").Value},
+			}
+
+			Expect(flags.Parse([]string{"--name", "foo"})).NotTo(HaveOccurred())
+			syncDuplicateFlags(flags, duplicateValues)
+			Expect(dupVal).To(Equal("foo"))
+		})
+
+		It("applies merge and sync for any subcommand (init, api, webhook, edit), not only edit", func() {
+			cmd := &cobra.Command{Use: "api"}
+			pluginA := &mockSubcommandWithForceFlag{}
+			pluginB := &mockSubcommandWithForceFlag{}
+			tuples := []keySubcommandTuple{
+				{key: "pluginA.kubebuilder.io/v1", subcommand: pluginA},
+				{key: "pluginB.kubebuilder.io/v1", subcommand: pluginB},
+			}
+			meta := plugin.CLIMetadata{}
+
+			result, err := initializationHooks(cmd, tuples, meta)
+			Expect(err).NotTo(HaveOccurred())
+			Expect(result.duplicateFlagValues["force"]).To(HaveLen(1), "second plugin's Value recorded as duplicate")
+
+			Expect(cmd.ParseFlags([]string{"--force", "true"})).NotTo(HaveOccurred())
+			syncDuplicateFlags(cmd.Flags(), result.duplicateFlagValues)
+			Expect(pluginA.Force).To(BeTrue(), "first plugin (flag on command) receives value")
+			Expect(pluginB.Force).To(BeTrue(), "second plugin (duplicate) receives same value after sync")
+		})
+	})
 })
 
 type mockTestSubcommand struct{}
@@ -267,6 +445,19 @@ func (m *mockTestSubcommand) Scaffold(machinery.Filesystem) error {
 	return nil
 }
 
+// mockSubcommandWithForceFlag implements Subcommand and HasFlags for tests with a shared flag.
+type mockSubcommandWithForceFlag struct {
+	Force bool
+}
+
+func (m *mockSubcommandWithForceFlag) Scaffold(machinery.Filesystem) error {
+	return nil
+}
+
+func (m *mockSubcommandWithForceFlag) BindFlags(flags *pflag.FlagSet) {
+	flags.BoolVar(&m.Force, "force", false, "force usage")
+}
+
 type mockPluginWithSubcommand struct {
 	name                     string
 	supportedProjectVersions []config.Version