Skip to content

Commit

Permalink
simplify configfile loading logic
Browse files Browse the repository at this point in the history 
Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com>
  • Loading branch information
@inteon
inteon committed Aug 17, 2023
1 parent 6d31ed2 commit 0429a47
Show file tree
Hide file tree
Showing 7 changed files with 516 additions and 364 deletions.
57 changes: 0 additions & 57 deletions cmd/controller/app/controller_test.go
View file

This file was deleted.

5 changes: 0 additions & 5 deletions cmd/controller/app/options/options.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
209 changes: 75 additions & 134 deletions cmd/controller/app/start.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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 {
Expand All @@ -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 webhook 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 webhook 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
}

0 comments on commit 0429a47

Please sign in to comment.