Simplify flag and configfile parsing & add tests

cmd/controller/app/options/options.go

@@ -50,11 +50,6 @@ func (f *ControllerFlags) AddFlags(fs *pflag.FlagSet) {
 	fs.StringVar(&f.Config, "config", "", "Path to a file containing a ControllerConfiguration object used to configure the controller")
 }
 
-func ValidateControllerFlags(f *ControllerFlags) error {
-	// No validation needed today
-	return nil
-}
-
 func NewControllerConfiguration() (*config.ControllerConfiguration, error) {
 	scheme, _, err := configscheme.NewSchemeAndCodecs()
 	if err != nil {

cmd/controller/app/start.go

@@ -23,8 +23,6 @@ import (
 	"path/filepath"
 
 	"github.com/spf13/cobra"
-	"github.com/spf13/pflag"
-	cliflag "k8s.io/component-base/cli/flag"
 
 	"github.com/cert-manager/cert-manager/controller-binary/app/options"
 	config "github.com/cert-manager/cert-manager/internal/apis/config/controller"
@@ -54,12 +52,20 @@ const componentController = "controller"
 func NewServerCommand(stopCh <-chan struct{}) *cobra.Command {
 	ctx := cmdutil.ContextWithStopCh(context.Background(), stopCh)
 	log := logf.Log
+	ctx = logf.NewContext(ctx, log)
 
-	ctx = logf.NewContext(ctx, log, componentController)
+	return newServerCommand(ctx, func(ctx context.Context, cfg *config.ControllerConfiguration) error {
+		return Run(cfg, ctx.Done())
+	}, os.Args[1:])
+}
+
+func newServerCommand(
+	ctx context.Context,
+	run func(context.Context, *config.ControllerConfiguration) error,
+	allArgs []string,
+) *cobra.Command {
+	log := logf.FromContext(ctx, componentController)
 
-	cleanFlagSet := pflag.NewFlagSet(componentController, pflag.ContinueOnError)
-	// Replaces all instances of `_` in flag names with `-`
-	cleanFlagSet.SetNormalizeFunc(cliflag.WordSepNormalizeFunc)
 	controllerFlags := options.NewControllerFlags()
 	controllerConfig, err := options.NewControllerConfiguration()
 	if err != nil {
@@ -76,148 +82,83 @@ TLS certificates from various issuing sources.
 
 It will ensure certificates are valid and up to date periodically, and attempt
 to renew certificates at an appropriate time before expiry.`,
-		// The controller has special flag parsing requirements to handle precedence of providing
-		// configuration via versioned configuration files and flag values.
-		// Setting DisableFlagParsing=true prevents Cobra from interfering with flag parsing
-		// at all, and instead we handle it all in the RunE below.
-		DisableFlagParsing: true,
-		Run: func(cmd *cobra.Command, args []string) {
-			// initial flag parse, since we disable cobra's flag parsing
-			if err := cleanFlagSet.Parse(args); err != nil {
-				log.Error(err, "Failed to parse controller flag")
-				cmd.Usage()
-				os.Exit(1)
-			}
-
-			// check if there are non-flag arguments in the command line
-			cmds := cleanFlagSet.Args()
-			if len(cmds) > 0 {
-				log.Error(nil, "Unknown command", "command", cmds[0])
-				cmd.Usage()
-				os.Exit(1)
-			}
-
-			// short-circuit on help
-			help, err := cleanFlagSet.GetBool("help")
-			if err != nil {
-				log.Info(`"help" flag is non-bool, programmer error, please correct`)
-				os.Exit(1)
-			}
-			if help {
-				cmd.Help()
-				return
-			}
-
-			// set feature gates from initial flags-based config
-			if err := utilfeature.DefaultMutableFeatureGate.SetFromMap(controllerConfig.FeatureGates); err != nil {
-				log.Error(err, "Failed to set feature gates from initial flags-based config")
-				os.Exit(1)
-			}
-
-			if err := logf.ValidateAndApply(&controllerConfig.Logging); err != nil {
-				log.Error(err, "Failed to validate controller flags")
-				os.Exit(1)
-			}
-
-			if err := options.ValidateControllerFlags(controllerFlags); err != nil {
-				log.Error(err, "Failed to validate controller flags")
-				os.Exit(1)
-			}
 
-			if configFile := controllerFlags.Config; len(configFile) > 0 {
-				controllerConfig, err = loadConfigFile(configFile)
-				if err != nil {
-					log.Error(err, "Failed to load controller config file", "path", configFile)
-					os.Exit(1)
-				}
-
-				if err := controllerConfigFlagPrecedence(controllerConfig, args); err != nil {
-					log.Error(err, "Failed to merge flags with config file values")
-					os.Exit(1)
-				}
-				// update feature gates based on new config
-				if err := utilfeature.DefaultMutableFeatureGate.SetFromMap(controllerConfig.FeatureGates); err != nil {
-					log.Error(err, "Failed to set feature gates from config file")
-					os.Exit(1)
-				}
-
-				if err := logf.ValidateAndApply(&controllerConfig.Logging); err != nil {
-					log.Error(err, "Failed to validate controller flags")
-					os.Exit(1)
-				}
+		RunE: func(cmd *cobra.Command, args []string) error {
+			if err := loadConfigFromFile(
+				cmd, allArgs, controllerFlags.Config, controllerConfig,
+				func() error {
+					if err := logf.ValidateAndApply(&controllerConfig.Logging); err != nil {
+						return fmt.Errorf("failed to validate controller logging flags: %w", err)
+					}
+
+					// set feature gates from initial flags-based config
+					if err := utilfeature.DefaultMutableFeatureGate.SetFromMap(controllerConfig.FeatureGates); err != nil {
+						return fmt.Errorf("failed to set feature gates from initial flags-based config: %w", err)
+					}
+
+					return nil
+				},
+			); err != nil {
+				return err
 			}
 
-			// Start the controller
-			if err := Run(controllerConfig, stopCh); err != nil {
-				log.Error(err, "Failed to run the controller")
-				os.Exit(1)
-			}
+			return run(ctx, controllerConfig)
 		},
 	}
 
-	controllerFlags.AddFlags(cleanFlagSet)
-	options.AddConfigFlags(cleanFlagSet, controllerConfig)
-
-	cleanFlagSet.BoolP("help", "h", false, fmt.Sprintf("help for %s", cmd.Name()))
+	controllerFlags.AddFlags(cmd.Flags())
+	options.AddConfigFlags(cmd.Flags(), controllerConfig)
 
-	// ugly, but necessary, because Cobra's default UsageFunc and HelpFunc pollute the flagset with global flags
-	const usageFmt = "Usage:\n  %s\n\nFlags:\n%s"
-	cmd.SetUsageFunc(func(cmd *cobra.Command) error {
-		fmt.Fprintf(cmd.OutOrStderr(), usageFmt, cmd.UseLine(), cleanFlagSet.FlagUsagesWrapped(2))
-		return nil
-	})
-	cmd.SetHelpFunc(func(cmd *cobra.Command, args []string) {
-		fmt.Fprintf(cmd.OutOrStdout(), "%s\n\n"+usageFmt, cmd.Long, cmd.UseLine(), cleanFlagSet.FlagUsagesWrapped(2))
-	})
+	// explicitly set provided args in case it does not equal os.Args[:1],
+	// eg. when running tests
+	cmd.SetArgs(allArgs)
 
 	return cmd
 }
 
-// newFakeFlagSet constructs a pflag.FlagSet with the same flags as fs, but where
-// all values have noop Set implementations
-func newFakeFlagSet() *pflag.FlagSet {
-	fs := pflag.NewFlagSet("", pflag.ExitOnError)
-
-	// set the normalize func, similar to k8s.io/component-base/cli//flags.go:InitFlags
-	fs.SetNormalizeFunc(cliflag.WordSepNormalizeFunc)
-
-	return fs
-}
-
-// controllerConfigFlagPrecedence re-parses flags over the ControllerConfiguration object.
-// We must enforce flag precedence by re-parsing the command line into the new object.
-// This is necessary to preserve backwards-compatibility across binary upgrades.
-// See issue #56171 for more details.
-func controllerConfigFlagPrecedence(cfg *config.ControllerConfiguration, args []string) error {
-	// We use a throwaway controllerFlags and a fake global flagset to avoid double-parses,
-	// as some Set implementations accumulate values from multiple flag invocations.
-	fs := newFakeFlagSet()
-	// register throwaway KubeletFlags
-	options.NewControllerFlags().AddFlags(fs)
-	// register new ControllerConfiguration
-	options.AddConfigFlags(fs, cfg)
-	// re-parse flags
-	if err := fs.Parse(args); err != nil {
+func loadConfigFromFile(
+	cmd *cobra.Command,
+	allArgs []string,
+	configFilePath string,
+	cfg *config.ControllerConfiguration,
+	fn func() error,
+) error {
+	if err := fn(); err != nil {
 		return err
 	}
-	return nil
-}
 
-func loadConfigFile(name string) (*config.ControllerConfiguration, error) {
-	const errFmt = "failed to load controller config file %s, error %v"
-	// compute absolute path based on current working dir
-	controllerConfigFile, err := filepath.Abs(name)
-	if err != nil {
-		return nil, fmt.Errorf(errFmt, name, err)
-	}
-	controllerConfig := controllerconfigfile.New()
-	loader, err := configfile.NewConfigurationFSLoader(nil, controllerConfigFile)
-	if err != nil {
-		return nil, fmt.Errorf(errFmt, name, err)
-	}
-	if err := loader.Load(controllerConfig); err != nil {
-		return nil, fmt.Errorf(errFmt, name, err)
+	if len(configFilePath) > 0 {
+		// compute absolute path based on current working dir
+		controllerConfigFile, err := filepath.Abs(configFilePath)
+		if err != nil {
+			return fmt.Errorf("failed to load config file %s, error %v", configFilePath, err)
+		}
+
+		loader, err := configfile.NewConfigurationFSLoader(nil, controllerConfigFile)
+		if err != nil {
+			return fmt.Errorf("failed to load config file %s, error %v", configFilePath, err)
+		}
+
+		controllerConfigFromFile := controllerconfigfile.New()
+		if err := loader.Load(controllerConfigFromFile); err != nil {
+			return fmt.Errorf("failed to load config file %s, error %v", configFilePath, err)
+		}
+
+		controllerConfigFromFile.Config.DeepCopyInto(cfg)
+
+		_, args, err := cmd.Root().Find(allArgs)
+		if err != nil {
+			return fmt.Errorf("failed to re-parse flags: %w", err)
+		}
+
+		if err := cmd.ParseFlags(args); err != nil {
+			return fmt.Errorf("failed to re-parse flags: %w", err)
+		}
+
+		if err := fn(); err != nil {
+			return err
+		}
 	}
 
-	return controllerConfig.Config, nil
+	return nil
 }