Repository: kubernetes-sigs/kubebuilder (language: go)

Issue:
(Phase 2 Plugins): external plugin will rebind `--group`, `--version`, `--kind` flags 
### What broke? What's expected?

When running `kubebuilder create api` or `kubebuilder create webhook` with the `--plugins` flag set to use an external plugin, the external plugin flag binding logic will attempt to rebind the `--group`, `--version`, and `--kind` flags. These flags are already bound automatically by Kubebuilder.

The external plugin flag binding logic needs to be modified to ensure that we do NOT attempt to bind any flags that kubebuilder has already bound. This would likely need to be done here:
https://github.com/kubernetes-sigs/kubebuilder/blob/95f55fcf8b87397f7fdf97456b10ff70b31f728e/pkg/plugins/external/helpers.go#L158-L178

and here:
https://github.com/kubernetes-sigs/kubebuilder/blob/95f55fcf8b87397f7fdf97456b10ff70b31f728e/pkg/plugins/external/helpers.go#L180-L198

### Reproducing this issue

It is not trivial to reproduce this issue, and I only encountered it when creating a sample Phase 2 Plugin that can be used for e2e testing the Phase 2 Plugins implementation.

### KubeBuilder (CLI) Version

Version: main.version{KubeBuilderVersion:"v3.5.0-46-g60d0c6ee-dirty", KubernetesVendor:"unknown", GitCommit:"60d0c6ee9eeb1c07dee1eed88ba4004362ec42a7", BuildDate:"2022-07-06T20:56:11Z", GoOs:"linux", GoArch:"amd64"}

### PROJECT version

_No response_

### Plugin versions

_No response_

### Other versions

Go: go version go1.18.3 linux/amd64

### Extra Labels

_No response_


Discussion / hints:

Fix patch:
diff --git a/pkg/plugins/external/helpers.go b/pkg/plugins/external/helpers.go
index 7b6baa5822c..ca981912225 100644
--- a/pkg/plugins/external/helpers.go
+++ b/pkg/plugins/external/helpers.go
@@ -166,10 +166,7 @@ func bindAllFlags(fs *pflag.FlagSet, args []string) {
 			flag := strings.Replace(args[i], "--", "", 1)
 			// Check if the flag is a boolean flag
 			if isBooleanFlag(i, args) {
-				// --help is already a defined flag in the kubebuilder commands and has a description so we skip parsing it again
-				if flag != "help" {
-					_ = fs.Bool(flag, false, defaultFlagDescription)
-				}
+				_ = fs.Bool(flag, false, defaultFlagDescription)
 			} else {
 				_ = fs.String(flag, "", defaultFlagDescription)
 			}
@@ -197,6 +194,75 @@ func bindSpecificFlags(fs *pflag.FlagSet, flags []external.Flag) {
 	}
 }
 
+func filterFlags(flags []external.Flag, externalFlagFilters []externalFlagFilterFunc) []external.Flag {
+	filteredFlags := []external.Flag{}
+	for _, flag := range flags {
+		ok := true
+		for _, filter := range externalFlagFilters {
+			if !filter(flag) {
+				ok = false
+				break
+			}
+		}
+		if ok {
+			filteredFlags = append(filteredFlags, flag)
+		}
+	}
+	return filteredFlags
+}
+
+func filterArgs(args []string, argFilters []argFilterFunc) []string {
+	filteredArgs := []string{}
+	for _, arg := range args {
+		ok := true
+		for _, filter := range argFilters {
+			if !filter(arg) {
+				ok = false
+				break
+			}
+		}
+		if ok {
+			filteredArgs = append(filteredArgs, arg)
+		}
+	}
+	return filteredArgs
+}
+
+type (
+	externalFlagFilterFunc func(flag external.Flag) bool
+	argFilterFunc          func(arg string) bool
+)
+
+var (
+	// see gvkArgFilter
+	gvkFlagFilter = func(flag external.Flag) bool {
+		return gvkArgFilter(flag.Name)
+	}
+	// gvkFlagFilter filters out any flag named "group", "version", "kind" as
+	// they are already bound by kubebuilder
+	gvkArgFilter = func(arg string) bool {
+		arg = strings.Replace(arg, "--", "", 1)
+		for _, invalidFlagName := range []string{
+			"group", "version", "kind",
+		} {
+			if arg == invalidFlagName {
+				return false
+			}
+		}
+		return true
+	}
+
+	// see helpArgFilter
+	helpFlagFilter = func(flag external.Flag) bool {
+		return helpArgFilter(flag.Name)
+	}
+	// helpArgFilter filters out any flag named "help" as its already bound
+	helpArgFilter = func(arg string) bool {
+		arg = strings.Replace(arg, "--", "", 1)
+		return !(arg == "help")
+	}
+)
+
 func bindExternalPluginFlags(fs *pflag.FlagSet, subcommand string, path string, args []string) {
 	req := external.PluginRequest{
 		APIVersion: defaultAPIVersion,
@@ -208,10 +274,20 @@ func bindExternalPluginFlags(fs *pflag.FlagSet, subcommand string, path string,
 	// If it returns an error, parse all flags passed by the user and let
 	// the external plugin return an unknown flag error.
 	flags, err := getExternalPluginFlags(req, path)
+
+	// Filter Flags based on a set of filters that we do not want.
+	// can be used to filter out non-overridable flags or other
+	// criteria by creating your own filterFlagFunc
 	if err != nil {
-		bindAllFlags(fs, args)
+		bindAllFlags(fs, filterArgs(args, []argFilterFunc{
+			gvkArgFilter,
+			helpArgFilter,
+		}))
 	} else {
-		bindSpecificFlags(fs, flags)
+		bindSpecificFlags(fs, filterFlags(flags, []externalFlagFilterFunc{
+			gvkFlagFilter,
+			helpFlagFilter,
+		}))
 	}
 }
 


Test patch:
diff --git a/pkg/plugins/external/external_test.go b/pkg/plugins/external/external_test.go
index 40f35055c17..8a3ec3699c9 100644
--- a/pkg/plugins/external/external_test.go
+++ b/pkg/plugins/external/external_test.go
@@ -100,8 +100,10 @@ func (m *mockValidMEOutputGetter) GetExecOutput(req []byte, path string) ([]byte
 	return json.Marshal(response)
 }
 
-const externalPlugin = "myexternalplugin.sh"
-const floatVal = "float"
+const (
+	externalPlugin = "myexternalplugin.sh"
+	floatVal       = "float"
+)
 
 var _ = Describe("Run external plugin using Scaffold", func() {
 	Context("with valid mock values", func() {
@@ -136,7 +138,6 @@ var _ = Describe("Run external plugin using Scaffold", func() {
 			Expect(err).To(BeNil())
 
 			args = []string{"--domain", "example.com"}
-
 		})
 
 		AfterEach(func() {
@@ -185,7 +186,6 @@ var _ = Describe("Run external plugin using Scaffold", func() {
 			err = c.Scaffold(fs)
 			Expect(err).To(BeNil())
 		})
-
 	})
 
 	Context("with invalid mock values of GetExecOutput() and GetCurrentDir()", func() {
@@ -204,7 +204,6 @@ var _ = Describe("Run external plugin using Scaffold", func() {
 
 			pluginFileName = externalPlugin
 			args = []string{"--domain", "example.com"}
-
 		})
 
 		It("should return error upon running init subcommand on the external plugin", func() {
@@ -241,7 +240,6 @@ var _ = Describe("Run external plugin using Scaffold", func() {
 			err = e.Scaffold(fs)
 			Expect(err).NotTo(BeNil())
 			Expect(err.Error()).To(ContainSubstring("error getting current directory"))
-
 		})
 
 		It("should return error upon running create api subcommand on the external plugin", func() {
@@ -278,7 +276,6 @@ var _ = Describe("Run external plugin using Scaffold", func() {
 			err = c.Scaffold(fs)
 			Expect(err).NotTo(BeNil())
 			Expect(err.Error()).To(ContainSubstring("error getting current directory"))
-
 		})
 	})
 
@@ -437,14 +434,50 @@ var _ = Describe("Run external plugin using Scaffold", func() {
 
 			checkFlagset()
 		})
+	})
 
+	Context("Flag Parsing Filter Functions", func() {
+		It("gvk(Arg/Flag)Filter should filter out (--)group, (--)version, (--)kind", func() {
+			for _, toBeFiltered := range []string{
+				"group", "version", "kind",
+			} {
+				Expect(gvkArgFilter("--" + toBeFiltered)).To(BeFalse())
+				Expect(gvkArgFilter(toBeFiltered)).To(BeFalse())
+				Expect(gvkFlagFilter(external.Flag{Name: "--" + toBeFiltered})).To(BeFalse())
+				Expect(gvkFlagFilter(external.Flag{Name: "--" + toBeFiltered})).To(BeFalse())
+			}
+			Expect(gvkArgFilter("somerandomflag")).To(BeTrue())
+			Expect(gvkFlagFilter(external.Flag{Name: "somerandomflag"})).To(BeTrue())
+		})
+
+		It("helpArgFilter should filter out (--)help", func() {
+			Expect(helpArgFilter("--help")).To(BeFalse())
+			Expect(helpArgFilter("help")).To(BeFalse())
+			Expect(helpArgFilter("somerandomflag")).To(BeTrue())
+			Expect(helpFlagFilter(external.Flag{Name: "--help"})).To(BeFalse())
+			Expect(helpFlagFilter(external.Flag{Name: "help"})).To(BeFalse())
+			Expect(helpFlagFilter(external.Flag{Name: "somerandomflag"})).To(BeTrue())
+		})
 	})
 
 	Context("Flag Parsing Helper Functions", func() {
 		var (
-			fs    *pflag.FlagSet
-			args  = []string{"--domain", "something.com", "--boolean", "--another", "flag", "--help"}
-			flags []external.Flag
+			fs   *pflag.FlagSet
+			args = []string{
+				"--domain", "something.com",
+				"--boolean",
+				"--another", "flag",
+				"--help",
+				"--group", "somegroup",
+				"--kind", "somekind",
+				"--version", "someversion",
+			}
+			forbidden = []string{
+				"help", "group", "kind", "version",
+			}
+			flags               []external.Flag
+			argFilters          []argFilterFunc
+			externalFlagFilters []externalFlagFilterFunc
 		)
 
 		BeforeEach(func() {
@@ -454,6 +487,13 @@ var _ = Describe("Run external plugin using Scaffold", func() {
 
 			flags = make([]external.Flag, len(flagsToAppend))
 			copy(flags, flagsToAppend)
+
+			argFilters = []argFilterFunc{
+				gvkArgFilter, helpArgFilter,
+			}
+			externalFlagFilters = []externalFlagFilterFunc{
+				gvkFlagFilter, helpFlagFilter,
+			}
 		})
 
 		It("isBooleanFlag should return true if boolean flag provided at index", func() {
@@ -464,11 +504,11 @@ var _ = Describe("Run external plugin using Scaffold", func() {
 			Expect(isBooleanFlag(0, args)).To(BeFalse())
 		})
 
-		It("bindAllFlags should bind all flags except for `--help`", func() {
+		It("bindAllFlags should bind all flags", func() {
 			usage := "Kubebuilder could not validate this flag with the external plugin. " +
 				"Consult the external plugin documentation for more information."
 
-			bindAllFlags(fs, args)
+			bindAllFlags(fs, filterArgs(args, argFilters))
 			Expect(fs.HasFlags()).To(BeTrue())
 			Expect(fs.Lookup("domain")).NotTo(BeNil())
 			Expect(fs.Lookup("domain").Value.Type()).To(Equal("string"))
@@ -479,15 +519,20 @@ var _ = Describe("Run external plugin using Scaffold", func() {
 			Expect(fs.Lookup("another")).NotTo(BeNil())
 			Expect(fs.Lookup("another").Value.Type()).To(Equal("string"))
 			Expect(fs.Lookup("another").Usage).To(Equal(usage))
-			Expect(fs.Lookup("help")).To(BeNil())
+
+			By("bindAllFlags not have bound any forbidden flag after filtering")
+			for i := range forbidden {
+				Expect(fs.Lookup(forbidden[i])).To(BeNil())
+			}
 		})
 
 		It("bindSpecificFlags should bind all flags in given []Flag", func() {
-			bindSpecificFlags(fs, flags)
+			filteredFlags := filterFlags(flags, externalFlagFilters)
+			bindSpecificFlags(fs, filteredFlags)
 
 			Expect(fs.HasFlags()).To(BeTrue())
 
-			for _, flag := range flags {
+			for _, flag := range filteredFlags {
 				Expect(fs.Lookup(flag.Name)).NotTo(BeNil())
 				// we parse floats as float64 Go type so this check will account for that
 				if flag.Type != floatVal {
@@ -498,6 +543,11 @@ var _ = Describe("Run external plugin using Scaffold", func() {
 				Expect(fs.Lookup(flag.Name).Usage).To(Equal(flag.Usage))
 				Expect(fs.Lookup(flag.Name).DefValue).To(Equal(flag.Default))
 			}
+
+			By("bindSpecificFlags not have bound any forbidden flag after filtering")
+			for i := range forbidden {
+				Expect(fs.Lookup(forbidden[i])).To(BeNil())
+			}
 		})
 	})
 
@@ -629,7 +679,6 @@ var _ = Describe("Run external plugin using Scaffold", func() {
 			checkMetadata()
 		})
 	})
-
 })
 
 func getFlags() []external.Flag {