From 1d6db5924f4529431cd88bce20f04940681f0aa6 Mon Sep 17 00:00:00 2001 From: Jordan Liggitt Date: Tue, 20 Nov 2018 23:58:51 -0500 Subject: [PATCH] Tighten feature gate interface to split out mutating methods --- .../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/staging/src/k8s.io/apiserver/pkg/util/feature/feature_gate.go b/staging/src/k8s.io/apiserver/pkg/util/feature/feature_gate.go index a83dafd56a..a8bce27f3a 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 @@ -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/staging/src/k8s.io/apiserver/pkg/util/feature/feature_gate_test.go b/staging/src/k8s.io/apiserver/pkg/util/feature/feature_gate_test.go index 14ec869481..194ed1f072 100644 --- a/staging/src/k8s.io/apiserver/pkg/util/feature/feature_gate_test.go +++ b/staging/src/k8s.io/apiserver/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/staging/src/k8s.io/apiserver/pkg/util/feature/testing/feature_gate_testing.go b/staging/src/k8s.io/apiserver/pkg/util/feature/testing/feature_gate_testing.go index bcae2566bc..766a85a1d8 100644 --- a/staging/src/k8s.io/apiserver/pkg/util/feature/testing/feature_gate_testing.go +++ b/staging/src/k8s.io/apiserver/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) } } }