Merge pull request #2872 from jakobmoellersap/external-plugin-filter-flags

🐛 Add external plugin arg filter funcs and add filters for "--group", "--version", "--kind"

Author: k8s-ci-robot

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 {

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,
+		}))
 	}
 }