simplify configfile loading logic & directly call validation method

Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com>

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/options/options_test.go

@@ -17,15 +17,13 @@ limitations under the License.
 package options
 
 import (
-	"strings"
 	"testing"
 
 	"k8s.io/apimachinery/pkg/util/sets"
 
 	//"github.com/cert-manager/cert-manager/controller-binary/app/options"
 	config "github.com/cert-manager/cert-manager/internal/apis/config/controller"
 	defaults "github.com/cert-manager/cert-manager/internal/apis/config/controller/v1alpha1"
-	"github.com/cert-manager/cert-manager/internal/apis/config/controller/validation"
 )
 
 func TestEnabledControllers(t *testing.T) {
@@ -69,43 +67,3 @@ func TestEnabledControllers(t *testing.T) {
 		})
 	}
 }
-
-func TestValidate(t *testing.T) {
-	tests := map[string]struct {
-		DNS01RecursiveServers []string
-		expError              string
-	}{
-		"if valid dns servers with ip address and port, return no errors": {
-			DNS01RecursiveServers: []string{"192.168.0.1:53", "10.0.0.1:5353"},
-			expError:              "",
-		},
-		"if valid DNS servers with DoH server addresses including https prefix, return no errors": {
-			DNS01RecursiveServers: []string{"https://dns.example.com", "https://doh.server"},
-			expError:              "",
-		},
-		"if invalid DNS server format due to missing https prefix, return 'invalid DNS server' error": {
-			DNS01RecursiveServers: []string{"dns.example.com"},
-			expError:              "invalid DNS server",
-		},
-		"if invalid DNS server format due to invalid IP address length and no port, return 'invalid DNS server' error": {
-			DNS01RecursiveServers: []string{"192.168.0.1.53"},
-			expError:              "invalid DNS server",
-		},
-	}
-
-	for name, test := range tests {
-		t.Run(name, func(t *testing.T) {
-			o, _ := NewControllerConfiguration()
-			o.ACMEDNS01Config.RecursiveNameservers = test.DNS01RecursiveServers
-
-			err := validation.ValidateControllerConfiguration(o)
-			if test.expError != "" {
-				if err == nil || !strings.Contains(err.Error(), test.expError) {
-					t.Errorf("expected error containing '%s', but got: %v", test.expError, err)
-				}
-			} else if err != nil {
-				t.Errorf("unexpected error: %v", err)
-			}
-		})
-	}
-}

cmd/controller/app/start.go

@@ -23,11 +23,10 @@ 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"
+	"github.com/cert-manager/cert-manager/internal/apis/config/controller/validation"
 	cmdutil "github.com/cert-manager/cert-manager/internal/cmd/util"
 
 	_ "github.com/cert-manager/cert-manager/pkg/controller/acmechallenges"
@@ -57,9 +56,6 @@ func NewServerCommand(stopCh <-chan struct{}) *cobra.Command {
 
 	ctx = logf.NewContext(ctx, log, 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,130 +72,65 @@ 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
+		RunE: func(cmd *cobra.Command, args []string) 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 {
-				log.Error(err, "Failed to set feature gates from initial flags-based config")
-				os.Exit(1)
-			}
-
-			if err := options.ValidateControllerFlags(controllerFlags); err != nil {
-				log.Error(err, "Failed to validate controller flags")
-				os.Exit(1)
+				return fmt.Errorf("failed to set feature gates from initial flags-based config: %w", err)
 			}
 
 			if configFile := controllerFlags.Config; len(configFile) > 0 {
-				controllerConfig, err = loadConfigFile(configFile)
+				controllerConfigFromFile, err := loadConfigFile(configFile)
 				if err != nil {
-					log.Error(err, "Failed to load controller config file", "path", configFile)
-					os.Exit(1)
+					return fmt.Errorf("failed to load controller config file %s: %w", configFile, err)
+				}
+
+				controllerConfigFromFile.DeepCopyInto(controllerConfig)
+
+				_, args, err := cmd.Root().Find(os.Args[1:])
+				if err != nil {
+					return fmt.Errorf("failed to re-parse flags: %w", err)
+				}
+
+				if err := cmd.Flags().Parse(args); err != nil {
+					return fmt.Errorf("failed to re-parse flags: %w", err)
 				}
 
-				if err := controllerConfigFlagPrecedence(controllerConfig, args); err != nil {
-					log.Error(err, "Failed to merge flags with config file values")
-					os.Exit(1)
+				if err := logf.ValidateAndApply(&controllerConfig.Logging); err != nil {
+					return fmt.Errorf("failed to validate controller logging flags: %w", err)
 				}
+
 				// 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)
+					return fmt.Errorf("failed to set feature gates from config file: %w", err)
 				}
 			}
 
-			// Start the controller
-			if err := Run(controllerConfig, stopCh); err != nil {
-				log.Error(err, "Failed to run the controller")
-				os.Exit(1)
+			if err := validation.ValidateControllerConfiguration(controllerConfig); err != nil {
+				return fmt.Errorf("failed to validate controller configuration: %w", err)
 			}
+
+			return Run(controllerConfig, stopCh)
 		},
 	}
 
-	controllerFlags.AddFlags(cleanFlagSet)
-	options.AddConfigFlags(cleanFlagSet, controllerConfig)
-
-	cleanFlagSet.BoolP("help", "h", false, fmt.Sprintf("help for %s", cmd.Name()))
-
-	// 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))
-	})
+	controllerFlags.AddFlags(cmd.Flags())
+	options.AddConfigFlags(cmd.Flags(), controllerConfig)
 
 	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 {
-		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 {