From e5cdab222a8e84f1e4a178332c9d94c34cea4f83 Mon Sep 17 00:00:00 2001 From: Jordan Liggitt Date: Tue, 20 Nov 2018 23:58:51 -0500 Subject: [PATCH 1/3] Tighten feature gate interface to split out mutating methods Kubernetes-commit: 1d6db5924f4529431cd88bce20f04940681f0aa6 --- pkg/util/feature/feature_gate.go | 37 +++++++++++++------ pkg/util/feature/feature_gate_test.go | 6 +-- .../feature/testing/feature_gate_testing.go | 12 +++--- 3 files changed, 34 insertions(+), 21 deletions(-) diff --git a/pkg/util/feature/feature_gate.go b/pkg/util/feature/feature_gate.go index a83dafd56..a8bce27f3 100644 --- a/pkg/util/feature/feature_gate.go +++ b/pkg/util/feature/feature_gate.go @@ -51,8 +51,15 @@ var ( allAlphaGate: setUnsetAlphaGates, } + // DefaultMutableFeatureGate is a mutable version of DefaultFeatureGate. + // Only top-level commands/options setup and the k8s.io/apiserver/pkg/util/feature/testing package should make use of this. + // Tests that need to modify feature gates for the duration of their test should use: + // defer utilfeaturetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features., )() + DefaultMutableFeatureGate MutableFeatureGate = NewFeatureGate() + // DefaultFeatureGate is a shared global FeatureGate. - DefaultFeatureGate FeatureGate = NewFeatureGate() + // Top-level commands/options setup that needs to modify this feature gate should use DefaultMutableFeatureGate. + DefaultFeatureGate FeatureGate = DefaultMutableFeatureGate ) type FeatureSpec struct { @@ -72,9 +79,23 @@ const ( Deprecated = prerelease("DEPRECATED") ) -// FeatureGate parses and stores flag gates for known features from -// a string like feature1=true,feature2=false,... +// FeatureGate indicates whether a given feature is enabled or not type FeatureGate interface { + // Enabled returns true if the key is enabled. + Enabled(key Feature) bool + // 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() MutableFeatureGate +} + +// MutableFeatureGate parses and stores flag gates for known features from +// a string like feature1=true,feature2=false,... +type MutableFeatureGate interface { + FeatureGate + // AddFlag adds a flag for setting global feature gates to the specified FlagSet. AddFlag(fs *pflag.FlagSet) // Set parses and stores flag gates for known features @@ -82,16 +103,8 @@ type FeatureGate interface { Set(value string) error // SetFromMap stores flag gates for known features from a map[string]bool or returns an error SetFromMap(m map[string]bool) error - // Enabled returns true if the key is enabled. - Enabled(key Feature) bool // Add adds features to the featureGate. 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. @@ -294,7 +307,7 @@ func (f *featureGate) 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. -func (f *featureGate) DeepCopy() FeatureGate { +func (f *featureGate) DeepCopy() MutableFeatureGate { // Copy existing state. known := map[Feature]FeatureSpec{} for k, v := range f.known.Load().(map[Feature]FeatureSpec) { diff --git a/pkg/util/feature/feature_gate_test.go b/pkg/util/feature/feature_gate_test.go index 14ec86948..194ed1f07 100644 --- a/pkg/util/feature/feature_gate_test.go +++ b/pkg/util/feature/feature_gate_test.go @@ -148,7 +148,7 @@ func TestFeatureGateOverride(t *testing.T) { const testBetaGate Feature = "TestBeta" // Don't parse the flag, assert defaults are used. - var f FeatureGate = NewFeatureGate() + var f *featureGate = NewFeatureGate() f.Add(map[Feature]FeatureSpec{ testAlphaGate: {Default: false, PreRelease: Alpha}, testBetaGate: {Default: false, PreRelease: Beta}, @@ -177,7 +177,7 @@ func TestFeatureGateFlagDefaults(t *testing.T) { const testBetaGate Feature = "TestBeta" // Don't parse the flag, assert defaults are used. - var f FeatureGate = NewFeatureGate() + var f *featureGate = NewFeatureGate() f.Add(map[Feature]FeatureSpec{ testAlphaGate: {Default: false, PreRelease: Alpha}, testBetaGate: {Default: true, PreRelease: Beta}, @@ -201,7 +201,7 @@ func TestFeatureGateKnownFeatures(t *testing.T) { ) // Don't parse the flag, assert defaults are used. - var f FeatureGate = NewFeatureGate() + var f *featureGate = NewFeatureGate() f.Add(map[Feature]FeatureSpec{ testAlphaGate: {Default: false, PreRelease: Alpha}, testBetaGate: {Default: true, PreRelease: Beta}, diff --git a/pkg/util/feature/testing/feature_gate_testing.go b/pkg/util/feature/testing/feature_gate_testing.go index bcae2566b..766a85a1d 100644 --- a/pkg/util/feature/testing/feature_gate_testing.go +++ b/pkg/util/feature/testing/feature_gate_testing.go @@ -69,16 +69,16 @@ func VerifyFeatureGatesUnchanged(gate feature.FeatureGate, tests func() int) { // Example use: // // defer utilfeaturetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features., true)() -func SetFeatureGateDuringTest(t *testing.T, gate feature.FeatureGate, feature feature.Feature, value bool) func() { - originalValue := gate.Enabled(feature) +func SetFeatureGateDuringTest(t *testing.T, gate feature.FeatureGate, f feature.Feature, value bool) func() { + originalValue := gate.Enabled(f) - if err := gate.Set(fmt.Sprintf("%s=%v", feature, value)); err != nil { - t.Errorf("error setting %s=%v: %v", feature, value, err) + if err := gate.(feature.MutableFeatureGate).Set(fmt.Sprintf("%s=%v", f, value)); err != nil { + t.Errorf("error setting %s=%v: %v", f, value, err) } return func() { - if err := gate.Set(fmt.Sprintf("%s=%v", feature, originalValue)); err != nil { - t.Errorf("error restoring %s=%v: %v", feature, originalValue, err) + if err := gate.(feature.MutableFeatureGate).Set(fmt.Sprintf("%s=%v", f, originalValue)); err != nil { + t.Errorf("error restoring %s=%v: %v", f, originalValue, err) } } } From d294e6b5b46397acabf498fd341428d6c0beba28 Mon Sep 17 00:00:00 2001 From: Jordan Liggitt Date: Tue, 20 Nov 2018 23:59:52 -0500 Subject: [PATCH 2/3] Update non-test code to use DefaultMutableFeatureGate Kubernetes-commit: d440ecdd3b41a4fc4a207195e1bb976422d6d35e --- pkg/admission/plugin/initialization/initialization.go | 2 +- pkg/features/kube_features.go | 2 +- pkg/server/options/server_run_options.go | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/admission/plugin/initialization/initialization.go b/pkg/admission/plugin/initialization/initialization.go index d4d184a57..8219b797f 100644 --- a/pkg/admission/plugin/initialization/initialization.go +++ b/pkg/admission/plugin/initialization/initialization.go @@ -85,7 +85,7 @@ func (i *initializer) ValidateInitialization() error { } if !utilfeature.DefaultFeatureGate.Enabled(features.Initializers) { - if err := utilfeature.DefaultFeatureGate.Set(string(features.Initializers) + "=true"); err != nil { + if err := utilfeature.DefaultMutableFeatureGate.Set(string(features.Initializers) + "=true"); err != nil { klog.Errorf("error enabling Initializers feature as part of admission plugin setup: %v", err) } else { klog.Infof("enabled Initializers feature as part of admission plugin setup") diff --git a/pkg/features/kube_features.go b/pkg/features/kube_features.go index 924182568..88d949c4d 100644 --- a/pkg/features/kube_features.go +++ b/pkg/features/kube_features.go @@ -91,7 +91,7 @@ const ( ) func init() { - utilfeature.DefaultFeatureGate.Add(defaultKubernetesFeatureGates) + utilfeature.DefaultMutableFeatureGate.Add(defaultKubernetesFeatureGates) } // defaultKubernetesFeatureGates consists of all known Kubernetes-specific feature keys. diff --git a/pkg/server/options/server_run_options.go b/pkg/server/options/server_run_options.go index fccb24e03..2884f853f 100644 --- a/pkg/server/options/server_run_options.go +++ b/pkg/server/options/server_run_options.go @@ -154,5 +154,5 @@ func (s *ServerRunOptions) AddUniversalFlags(fs *pflag.FlagSet) { "handler, which picks a randomized value above this number as the connection timeout, "+ "to spread out load.") - utilfeature.DefaultFeatureGate.AddFlag(fs) + utilfeature.DefaultMutableFeatureGate.AddFlag(fs) } From 2433950ca14a8d0545c931feed8931b488e6669c Mon Sep 17 00:00:00 2001 From: Jordan Liggitt Date: Wed, 21 Nov 2018 10:44:02 -0500 Subject: [PATCH 3/3] drop VerifyFeatureGatesUnchanged Kubernetes-commit: 2498ca760695ad03d4b400ab1e9766799f4d7975 --- .../feature/testing/feature_gate_testing.go | 40 ------------------- 1 file changed, 40 deletions(-) diff --git a/pkg/util/feature/testing/feature_gate_testing.go b/pkg/util/feature/testing/feature_gate_testing.go index 766a85a1d..6b0a5fe8d 100644 --- a/pkg/util/feature/testing/feature_gate_testing.go +++ b/pkg/util/feature/testing/feature_gate_testing.go @@ -18,51 +18,11 @@ package testing import ( "fmt" - "os" - "strings" "testing" "k8s.io/apiserver/pkg/util/feature" ) -// VerifyFeatureGatesUnchanged ensures the provided gate does not change any values when tests() are completed. -// Intended to be placed into unit test packages that mess with feature gates. -// -// Example use: -// -// import ( -// "testing" -// -// utilfeature "k8s.io/apiserver/pkg/util/feature" -// utilfeaturetesting "k8s.io/apiserver/pkg/util/feature/testing" -// _ "k8s.io/kubernetes/pkg/features" -// ) -// -// func TestMain(m *testing.M) { -// utilfeaturetesting.VerifyFeatureGatesUnchanged(utilfeature.DefaultFeatureGate, m.Run) -// } -func VerifyFeatureGatesUnchanged(gate feature.FeatureGate, tests func() int) { - originalGates := gate.DeepCopy() - originalSet := fmt.Sprint(gate) - - rc := tests() - - finalSet := fmt.Sprint(gate) - if finalSet != originalSet { - for _, kv := range strings.Split(finalSet, ",") { - k := strings.Split(kv, "=")[0] - if originalGates.Enabled(feature.Feature(k)) != gate.Enabled(feature.Feature(k)) { - fmt.Println(fmt.Sprintf("VerifyFeatureGatesUnchanged: mutated %s feature gate from %v to %v", k, originalGates.Enabled(feature.Feature(k)), gate.Enabled(feature.Feature(k)))) - rc = 1 - } - } - } - - if rc != 0 { - os.Exit(rc) - } -} - // SetFeatureGateDuringTest sets the specified gate to the specified value, and returns a function that restores the original value. // Failures to set or restore cause the test to fail. //