Kubelet config: Validate new config against future feature gates

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",

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)

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 {

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
 	}

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,
+	}
+}