flag precedence redo #56995
+246
−151
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -30,10 +30,12 @@ import ( | |
| "path" | ||
| "path/filepath" | ||
| "strconv" | ||
| "strings" | ||
| "time" | ||
|
|
||
| "github.com/golang/glog" | ||
| "github.com/spf13/cobra" | ||
| "github.com/spf13/pflag" | ||
|
|
||
| "k8s.io/api/core/v1" | ||
| "k8s.io/apimachinery/pkg/api/resource" | ||
|
|
@@ -74,11 +76,13 @@ import ( | |
| dockerremote "k8s.io/kubernetes/pkg/kubelet/dockershim/remote" | ||
| "k8s.io/kubernetes/pkg/kubelet/eviction" | ||
| evictionapi "k8s.io/kubernetes/pkg/kubelet/eviction/api" | ||
| dynamickubeletconfig "k8s.io/kubernetes/pkg/kubelet/kubeletconfig" | ||
| "k8s.io/kubernetes/pkg/kubelet/kubeletconfig/configfiles" | ||
| "k8s.io/kubernetes/pkg/kubelet/server" | ||
| "k8s.io/kubernetes/pkg/kubelet/server/streaming" | ||
| kubetypes "k8s.io/kubernetes/pkg/kubelet/types" | ||
| "k8s.io/kubernetes/pkg/util/configz" | ||
| utilfs "k8s.io/kubernetes/pkg/util/filesystem" | ||
| "k8s.io/kubernetes/pkg/util/flock" | ||
| kubeio "k8s.io/kubernetes/pkg/util/io" | ||
| "k8s.io/kubernetes/pkg/util/mount" | ||
|
|
@@ -96,8 +100,9 @@ const ( | |
|
|
||
| // NewKubeletCommand creates a *cobra.Command object with default parameters | ||
| func NewKubeletCommand() *cobra.Command { | ||
| cleanFlagSet := pflag.NewFlagSet(componentKubelet, pflag.ContinueOnError) | ||
| kubeletFlags := options.NewKubeletFlags() | ||
| kubeletConfig, err := options.NewKubeletConfiguration() | ||
| // programmer error | ||
| if err != nil { | ||
| glog.Fatal(err) | ||
|
|
@@ -124,25 +129,79 @@ is checked every 20 seconds (also configurable with a flag). | |
|
|
||
| HTTP server: The kubelet can also listen for HTTP and respond to a simple API | ||
| (underspec'd currently) to submit a new manifest.`, | ||
| // The Kubelet has special flag parsing requirements to enforce flag precedence rules, | ||
| // so we do all our parsing manually in Run, below. | ||
| // DisableFlagParsing=true provides the full set of flags passed to the kubelet in the | ||
| // `args` arg to Run, without Cobra's interference. | ||
| 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 { | ||
| cmd.Usage() | ||
| glog.Fatal(err) | ||
| } | ||
|
|
||
| // short-circuit on help | ||
| help, err := cleanFlagSet.GetBool("help") | ||
| if err != nil { | ||
| glog.Fatal(`"help" flag is non-bool, programmer error, please correct`) | ||
| } | ||
| if help { | ||
| cmd.Help() | ||
| return | ||
| } | ||
|
|
||
| // short-circuit on verflag | ||
| verflag.PrintAndExitIfRequested() | ||
|
|
||
| // log args (separate lines, so we don't overflow line-length limits in logging infrastructure) | ||
| glog.V(2).Infof("kubelet flags: %s", strings.Join(args, "\n")) | ||
|
|
||
| // set feature gates from initial flags-based config | ||
| if err := utilfeature.DefaultFeatureGate.SetFromMap(kubeletConfig.FeatureGates); err != nil { | ||
| glog.Fatal(err) | ||
| } | ||
|
|
||
| // validate the initial KubeletFlags | ||
| if err := options.ValidateKubeletFlags(kubeletFlags); err != nil { | ||
| glog.Fatal(err) | ||
| } | ||
|
|
||
| // load kubelet config file, if provided | ||
| if configFile := kubeletFlags.KubeletConfigFile; len(configFile) > 0 { | ||
| kubeletConfig, err = loadConfigFile(configFile) | ||
|
There was a problem hiding this comment. BUG: This clobbers kubernetes/cmd/kubelet/app/options/options.go Lines 242 to 270 in cc76a75 There was a problem hiding this comment. That is intentional, not a bug. The v1beta1 defaults get reapplied to what is read from the config file. When reading from a config file, we have different and better / more secure defaults when running from a config file than from just the command line. There was a problem hiding this comment. That's what the godoc on applyLegacyDefaults was trying to express (these are defaults that only apply to the CLI flags and do not apply when running kubelet from a config file) There was a problem hiding this comment. ah thanks for clearing that up jordan! surprised you responded xD hope all is well |
||
| if err != nil { | ||
| glog.Fatal(err) | ||
| } | ||
| // 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. | ||
| if err := kubeletConfigFlagPrecedence(kubeletConfig, args); err != nil { | ||
| glog.Fatal(err) | ||
| } | ||
| // update feature gates based on new config | ||
| if err := utilfeature.DefaultFeatureGate.SetFromMap(kubeletConfig.FeatureGates); err != nil { | ||
| glog.Fatal(err) | ||
| } | ||
| } | ||
|
|
||
| // use dynamic kubelet config, if enabled | ||
| var kubeletConfigController *dynamickubeletconfig.Controller | ||
| if dynamicConfigDir := kubeletFlags.DynamicConfigDir.Value(); len(dynamicConfigDir) > 0 { | ||
| kubeletConfig, kubeletConfigController, err = BootstrapKubeletConfigController(kubeletConfig, dynamicConfigDir) | ||
| if err != nil { | ||
| glog.Fatal(err) | ||
| } | ||
| // 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. | ||
| if err := kubeletConfigFlagPrecedence(kubeletConfig, args); err != nil { | ||
| glog.Fatal(err) | ||
| } | ||
| // update feature gates based on new config | ||
| if err := utilfeature.DefaultFeatureGate.SetFromMap(kubeletConfig.FeatureGates); err != nil { | ||
| glog.Fatal(err) | ||
| } | ||
| } | ||
|
|
||
| // construct a KubeletServer from kubeletFlags and kubeletConfig | ||
|
|
@@ -174,13 +233,92 @@ HTTP server: The kubelet can also listen for HTTP and respond to a simple API | |
| }, | ||
| } | ||
|
|
||
| // keep cleanFlagSet separate, so Cobra doesn't pollute it with the global flags | ||
| kubeletFlags.AddFlags(cleanFlagSet) | ||
| options.AddKubeletConfigFlags(cleanFlagSet, kubeletConfig) | ||
| options.AddGlobalFlags(cleanFlagSet) | ||
| 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)) | ||
| }) | ||
|
|
||
| return cmd | ||
| } | ||
|
|
||
| // newFlagSetWithGlobals constructs a new pflag.FlagSet with global flags registered | ||
| // on it. | ||
| func newFlagSetWithGlobals() *pflag.FlagSet { | ||
| fs := pflag.NewFlagSet("", pflag.ExitOnError) | ||
| // set the normalize func, similar to k8s.io/apiserver/pkg/util/flag/flags.go:InitFlags | ||
| fs.SetNormalizeFunc(flag.WordSepNormalizeFunc) | ||
| // explicitly add flags from libs that register global flags | ||
| options.AddGlobalFlags(fs) | ||
| return fs | ||
| } | ||
|
|
||
| // newFakeFlagSet constructs a pflag.FlagSet with the same flags as fs, but where | ||
| // all values have noop Set implementations | ||
| func newFakeFlagSet(fs *pflag.FlagSet) *pflag.FlagSet { | ||
| ret := pflag.NewFlagSet("", pflag.ExitOnError) | ||
| ret.SetNormalizeFunc(fs.GetNormalizeFunc()) | ||
| fs.VisitAll(func(f *pflag.Flag) { | ||
| ret.VarP(flag.NoOp{}, f.Name, f.Shorthand, f.Usage) | ||
| }) | ||
| return ret | ||
| } | ||
|
|
||
| // kubeletConfigFlagPrecedence re-parses flags over the KubeletConfiguration 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 kubeletConfigFlagPrecedence(kc *kubeletconfiginternal.KubeletConfiguration, args []string) error { | ||
| // We use a throwaway kubeletFlags and a fake global flagset to avoid double-parses, | ||
| // as some Set implementations accumulate values from multiple flag invocations. | ||
| fs := newFakeFlagSet(newFlagSetWithGlobals()) | ||
| // register throwaway KubeletFlags | ||
| options.NewKubeletFlags().AddFlags(fs) | ||
| // register new KubeletConfiguration | ||
| options.AddKubeletConfigFlags(fs, kc) | ||
| // Remember original feature gates, so we can merge with flag gates later | ||
| original := kc.FeatureGates | ||
| // re-parse flags | ||
| if err := fs.Parse(args); err != nil { | ||
| return err | ||
| } | ||
| // Add back feature gates that were set in the original kc, but not in flags | ||
| for k, v := range original { | ||
| if _, ok := kc.FeatureGates[k]; !ok { | ||
| kc.FeatureGates[k] = v | ||
| } | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| func loadConfigFile(name string) (*kubeletconfiginternal.KubeletConfiguration, error) { | ||
| const errFmt = "failed to load Kubelet config file %s, error %v" | ||
| // compute absolute path based on current working dir | ||
| kubeletConfigFile, err := filepath.Abs(name) | ||
| if err != nil { | ||
| return nil, fmt.Errorf(errFmt, name, err) | ||
| } | ||
| loader, err := configfiles.NewFsLoader(utilfs.DefaultFs{}, kubeletConfigFile) | ||
| if err != nil { | ||
| return nil, fmt.Errorf(errFmt, name, err) | ||
| } | ||
| kc, err := loader.Load() | ||
| if err != nil { | ||
| return nil, fmt.Errorf(errFmt, name, err) | ||
| } | ||
| return kc, err | ||
| } | ||
|
|
||
| // UnsecuredDependencies returns a Dependencies suitable for being run, or an error if the server setup | ||
| // is not valid. It will not start any background processes, and does not include authentication/authorization | ||
| func UnsecuredDependencies(s *options.KubeletServer) (*kubelet.Dependencies, error) { | ||
|
|
@@ -438,9 +576,9 @@ func run(s *options.KubeletServer, kubeDeps *kubelet.Dependencies) (err error) { | |
| } | ||
| } | ||
|
|
||
| // If the kubelet config controller is available, and dynamic config is enabled, start the config and status sync loops | ||
| if utilfeature.DefaultFeatureGate.Enabled(features.DynamicKubeletConfig) && len(s.DynamicConfigDir.Value()) > 0 && | ||
| kubeDeps.KubeletConfigController != nil && !standaloneMode && !s.RunOnce { | ||
| kubeDeps.KubeletConfigController.StartSync(kubeDeps.KubeClient, kubeDeps.EventClient, string(nodeName)) | ||
| } | ||
|
|
||
|
|
@@ -916,35 +1054,26 @@ func parseResourceList(m map[string]string) (v1.ResourceList, error) { | |
|
|
||
| // BootstrapKubeletConfigController constructs and bootstrap a configuration controller | ||
| func BootstrapKubeletConfigController(defaultConfig *kubeletconfiginternal.KubeletConfiguration, | ||
| dynamicConfigDir string) (*kubeletconfiginternal.KubeletConfiguration, *dynamickubeletconfig.Controller, error) { | ||
| if !utilfeature.DefaultFeatureGate.Enabled(features.DynamicKubeletConfig) { | ||
| return nil, nil, fmt.Errorf("failed to bootstrap Kubelet config controller, you must enable the DynamicKubeletConfig feature gate") | ||
| } | ||
| if len(dynamicConfigDir) == 0 { | ||
| return nil, nil, fmt.Errorf("cannot bootstrap Kubelet config controller, --dynamic-config-dir was not provided") | ||
| } | ||
|
|
||
| // compute absolute path and bootstrap controller | ||
| dir, err := filepath.Abs(dynamicConfigDir) | ||
| if err != nil { | ||
| return nil, nil, fmt.Errorf("failed to get absolute path for --dynamic-config-dir=%s", dynamicConfigDir) | ||
| } | ||
| // get the latest KubeletConfiguration checkpoint from disk, or return the default config if no valid checkpoints exist | ||
| c := dynamickubeletconfig.NewController(defaultConfig, dir) | ||
| kc, err := c.Bootstrap() | ||
| if err != nil { | ||
| return nil, nil, fmt.Errorf("failed to determine a valid configuration, error: %v", err) | ||
| } | ||
| return kc, c, nil | ||
| } | ||
|
|
||
| // RunDockershim only starts the dockershim in current process. This is only used for cri validate testing purpose | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does it strip any parent command args (e.g.
hyperkube kubelet) or does it leave us with essentiallyos.Args[1:]?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it strips parent command args, based on my reading of the cobra internals, but I still have to write a small test program to confirm.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It strips parent command names.
Test program:
Output: