diff --git a/cmd/kubelet/app/BUILD b/cmd/kubelet/app/BUILD index 1eadc2c6e90..af2fcc09ad5 100644 --- a/cmd/kubelet/app/BUILD +++ b/cmd/kubelet/app/BUILD @@ -84,6 +84,7 @@ go_library( "//pkg/kubelet/apis/kubeletconfig:go_default_library", "//pkg/kubelet/apis/kubeletconfig/scheme:go_default_library", "//pkg/kubelet/apis/kubeletconfig/v1beta1:go_default_library", + "//pkg/kubelet/apis/kubeletconfig/validation:go_default_library", "//pkg/kubelet/cadvisor:go_default_library", "//pkg/kubelet/certificate:go_default_library", "//pkg/kubelet/certificate/bootstrap:go_default_library", diff --git a/cmd/kubelet/app/server.go b/cmd/kubelet/app/server.go index 7adfb77b7d2..2c0e47f99ed 100644 --- a/cmd/kubelet/app/server.go +++ b/cmd/kubelet/app/server.go @@ -68,6 +68,7 @@ import ( kubeletconfiginternal "k8s.io/kubernetes/pkg/kubelet/apis/kubeletconfig" kubeletscheme "k8s.io/kubernetes/pkg/kubelet/apis/kubeletconfig/scheme" kubeletconfigv1beta1 "k8s.io/kubernetes/pkg/kubelet/apis/kubeletconfig/v1beta1" + kubeletconfigvalidation "k8s.io/kubernetes/pkg/kubelet/apis/kubeletconfig/validation" "k8s.io/kubernetes/pkg/kubelet/cadvisor" kubeletcertificate "k8s.io/kubernetes/pkg/kubelet/certificate" "k8s.io/kubernetes/pkg/kubelet/certificate/bootstrap" @@ -198,43 +199,38 @@ HTTP server: The kubelet can also listen for HTTP and respond to a simple API } } - // TODO(#63305): always validate the combination of the local config file and flags, this is the fallback - // when the dynamic config controller tells us to use local config (this can be fixed alongside other validation fixes). + // We always validate the local configuration (command line + config file). + // This is the default "last-known-good" config for dynamic config, and must always remain valid. + if err := kubeletconfigvalidation.ValidateKubeletConfiguration(kubeletConfig); err != nil { + glog.Fatal(err) + } // use dynamic kubelet config, if enabled var kubeletConfigController *dynamickubeletconfig.Controller if dynamicConfigDir := kubeletFlags.DynamicConfigDir.Value(); len(dynamicConfigDir) > 0 { var dynamicKubeletConfig *kubeletconfiginternal.KubeletConfiguration - dynamicKubeletConfig, kubeletConfigController, err = BootstrapKubeletConfigController(dynamicConfigDir) + dynamicKubeletConfig, kubeletConfigController, err = BootstrapKubeletConfigController(dynamicConfigDir, + func(kc *kubeletconfiginternal.KubeletConfiguration) error { + // Here, we enforce flag precedence inside the controller, prior to the controller's validation sequence, + // so that we get a complete validation at the same point where we can decide to reject dynamic config. + // This fixes the flag-precedence component of issue #63305. + // See issue #56171 for general details on flag precedence. + return kubeletConfigFlagPrecedence(kc, args) + }) if err != nil { glog.Fatal(err) } // If we should just use our existing, local config, the controller will return a nil config if dynamicKubeletConfig != nil { kubeletConfig = dynamicKubeletConfig - // 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 + // Note: flag precedence was already enforced in the controller, prior to validation, + // by our above transform function. Now we simply update feature gates from the new config. if err := utilfeature.DefaultFeatureGate.SetFromMap(kubeletConfig.FeatureGates); err != nil { glog.Fatal(err) } } } - // TODO(#63305): need to reconcile that validation performed inside the dynamic config controller - // will happen against currently set feature gates, rather than future adjustments from combination of files - // and flags. There's a potential scenario where a valid config (because it sets new gates) is considered - // invalid against current gates (at least until --feature-gates flag is removed). - // We should validate against the combination of current feature gates, overrides from feature gates in the file, - // and overrides from feature gates set via flags, rather than currently set feature gates. - // Once the --feature-gates flag is removed, we should strictly validate against the combination of current - // feature gates and feature gates in the file (always need to validate against the combo, because feature-gates - // can layer between the file and dynamic config right now - though maybe we should change this). - // construct a KubeletServer from kubeletFlags and kubeletConfig kubeletServer := &options.KubeletServer{ KubeletFlags: *kubeletFlags, @@ -1108,7 +1104,7 @@ func parseResourceList(m map[string]string) (v1.ResourceList, error) { } // BootstrapKubeletConfigController constructs and bootstrap a configuration controller -func BootstrapKubeletConfigController(dynamicConfigDir string) (*kubeletconfiginternal.KubeletConfiguration, *dynamickubeletconfig.Controller, error) { +func BootstrapKubeletConfigController(dynamicConfigDir string, transform dynamickubeletconfig.TransformFunc) (*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") } @@ -1122,7 +1118,7 @@ func BootstrapKubeletConfigController(dynamicConfigDir string) (*kubeletconfigin 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(dir) + c := dynamickubeletconfig.NewController(dir, transform) kc, err := c.Bootstrap() if err != nil { return nil, nil, fmt.Errorf("failed to determine a valid configuration, error: %v", err) diff --git a/pkg/kubelet/apis/kubeletconfig/validation/validation.go b/pkg/kubelet/apis/kubeletconfig/validation/validation.go index e060ee46555..ab3bc4e14b4 100644 --- a/pkg/kubelet/apis/kubeletconfig/validation/validation.go +++ b/pkg/kubelet/apis/kubeletconfig/validation/validation.go @@ -31,6 +31,11 @@ import ( func ValidateKubeletConfiguration(kc *kubeletconfig.KubeletConfiguration) error { allErrors := []error{} + // Make a local copy of the global feature gates and combine it with the gates set by this configuration. + // This allows us to validate the config against the set of gates it will actually run against. + localFeatureGate := utilfeature.DefaultFeatureGate.DeepCopy() + localFeatureGate.SetFromMap(kc.FeatureGates) + if !kc.CgroupsPerQOS && len(kc.EnforceNodeAllocatable) > 0 { allErrors = append(allErrors, fmt.Errorf("invalid configuration: EnforceNodeAllocatable (--enforce-node-allocatable) is not supported unless CgroupsPerQOS (--cgroups-per-qos) feature is turned on")) } @@ -88,7 +93,7 @@ func ValidateKubeletConfiguration(kc *kubeletconfig.KubeletConfiguration) error if kc.RegistryPullQPS < 0 { allErrors = append(allErrors, fmt.Errorf("invalid configuration: RegistryPullQPS (--registry-qps) %v must not be a negative number", kc.RegistryPullQPS)) } - if kc.ServerTLSBootstrap && !utilfeature.DefaultFeatureGate.Enabled(features.RotateKubeletServerCertificate) { + if kc.ServerTLSBootstrap && !localFeatureGate.Enabled(features.RotateKubeletServerCertificate) { allErrors = append(allErrors, fmt.Errorf("invalid configuration: ServerTLSBootstrap %v requires feature gate RotateKubeletServerCertificate", kc.ServerTLSBootstrap)) } for _, val := range kc.EnforceNodeAllocatable { diff --git a/pkg/kubelet/kubeletconfig/controller.go b/pkg/kubelet/kubeletconfig/controller.go index f665745ab26..85b2ae2dab2 100644 --- a/pkg/kubelet/kubeletconfig/controller.go +++ b/pkg/kubelet/kubeletconfig/controller.go @@ -43,9 +43,22 @@ const ( configTrialDuration = 10 * time.Minute ) +// TransformFunc edits the KubeletConfiguration in-place, and returns an +// error if any of the transformations failed. +type TransformFunc func(kc *kubeletconfig.KubeletConfiguration) error + // Controller manages syncing dynamic Kubelet configurations // For more information, see the proposal: https://github.com/kubernetes/community/blob/master/contributors/design-proposals/node/dynamic-kubelet-configuration.md type Controller struct { + // transform applies an arbitrary transformation to config after loading, and before validation. + // This can be used, for example, to include config from flags before the controller's validation step. + // If transform returns an error, loadConfig will fail, and an InternalError will be reported. + // Be wary if using this function as an extension point, in most cases the controller should + // probably just be natively extended to do what you need. Injecting flag precedence transformations + // is something of an exception because the caller of this controller (cmd/) is aware of flags, but this + // controller's tree (pkg/) is not. + transform TransformFunc + // pendingConfigSource; write to this channel to indicate that the config source needs to be synced from the API server pendingConfigSource chan bool @@ -59,9 +72,17 @@ type Controller struct { checkpointStore store.Store } -// NewController constructs a new Controller object and returns it. Directory paths must be absolute. -func NewController(dynamicConfigDir string) *Controller { +// NewController constructs a new Controller object and returns it. The dynamicConfigDir +// path must be absolute. transform applies an arbitrary transformation to config after loading, and before validation. +// This can be used, for example, to include config from flags before the controller's validation step. +// If transform returns an error, loadConfig will fail, and an InternalError will be reported. +// Be wary if using this function as an extension point, in most cases the controller should +// probably just be natively extended to do what you need. Injecting flag precedence transformations +// is something of an exception because the caller of this controller (cmd/) is aware of flags, but this +// controller's tree (pkg/) is not. +func NewController(dynamicConfigDir string, transform TransformFunc) *Controller { return &Controller{ + transform: transform, // channels must have capacity at least 1, since we signal with non-blocking writes pendingConfigSource: make(chan bool, 1), configStatus: status.NewNodeConfigStatus(), @@ -71,6 +92,7 @@ func NewController(dynamicConfigDir string) *Controller { // Bootstrap attempts to return a valid KubeletConfiguration based on the configuration of the Controller, // or returns an error if no valid configuration could be produced. Bootstrap should be called synchronously before StartSync. +// If the pre-existing local configuration should be used, Bootstrap returns a nil config. func (cc *Controller) Bootstrap() (*kubeletconfig.KubeletConfiguration, error) { utillog.Infof("starting controller") @@ -194,6 +216,13 @@ func (cc *Controller) loadConfig(source checkpoint.RemoteConfigSource) (*kubelet if err != nil { return nil, status.LoadError, err } + // apply any required transformations to the KubeletConfiguration + if cc.transform != nil { + if err := cc.transform(kc); err != nil { + return nil, status.InternalError, err + } + } + // validate the result if err := validation.ValidateKubeletConfiguration(kc); err != nil { return nil, status.ValidateError, err } diff --git a/staging/src/k8s.io/apiserver/pkg/util/feature/feature_gate.go b/staging/src/k8s.io/apiserver/pkg/util/feature/feature_gate.go index 30687712489..fe35adc664e 100644 --- a/staging/src/k8s.io/apiserver/pkg/util/feature/feature_gate.go +++ b/staging/src/k8s.io/apiserver/pkg/util/feature/feature_gate.go @@ -88,6 +88,10 @@ type FeatureGate interface { Add(features map[Feature]FeatureSpec) error // KnownFeatures returns a slice of strings describing the FeatureGate's known features. KnownFeatures() []string + // DeepCopy returns a deep copy of the FeatureGate object, such that gates can be + // set on the copy without mutating the original. This is useful for validating + // config against potential feature gate changes before committing those changes. + DeepCopy() FeatureGate } // featureGate implements FeatureGate as well as pflag.Value for flag parsing. @@ -284,6 +288,10 @@ func (f *featureGate) Enabled(key Feature) bool { // AddFlag adds a flag for setting global feature gates to the specified FlagSet. func (f *featureGate) AddFlag(fs *pflag.FlagSet) { f.lock.Lock() + // TODO(mtaufen): Shouldn't we just close it on the first Set/SetFromMap instead? + // Not all components expose a feature gates flag using this AddFlag method, and + // in the future, all components will completely stop exposing a feature gates flag, + // in favor of componentconfig. f.closed = true f.lock.Unlock() @@ -306,3 +314,34 @@ func (f *featureGate) KnownFeatures() []string { sort.Strings(known) return known } + +// DeepCopy returns a deep copy of the FeatureGate object, such that gates can be +// set on the copy without mutating the original. This is useful for validating +// config against potential feature gate changes before committing those changes. +func (f *featureGate) DeepCopy() FeatureGate { + // Copy existing state. + known := map[Feature]FeatureSpec{} + for k, v := range f.known.Load().(map[Feature]FeatureSpec) { + known[k] = v + } + enabled := map[Feature]bool{} + for k, v := range f.enabled.Load().(map[Feature]bool) { + enabled[k] = v + } + + // Store copied state in new atomics. + knownValue := &atomic.Value{} + knownValue.Store(known) + enabledValue := &atomic.Value{} + enabledValue.Store(enabled) + + // Construct a new featureGate around the copied state. + // Note that specialFeatures is treated as immutable by convention, + // and we maintain the value of f.closed across the copy. + return &featureGate{ + special: specialFeatures, + known: knownValue, + enabled: enabledValue, + closed: f.closed, + } +}