From d8a399f798a2c6b8f1e2c18cbb517d89de54b00c Mon Sep 17 00:00:00 2001 From: Tim Allclair Date: Fri, 31 Aug 2018 10:16:47 -0700 Subject: [PATCH] Hide & warn on GA & deprecated feature gates --- .../k8s.io/apiserver/pkg/util/feature/BUILD | 5 +- .../pkg/util/feature/feature_gate.go | 58 ++++++------------- .../pkg/util/feature/feature_gate_test.go | 33 ++++++++++- 3 files changed, 51 insertions(+), 45 deletions(-) diff --git a/staging/src/k8s.io/apiserver/pkg/util/feature/BUILD b/staging/src/k8s.io/apiserver/pkg/util/feature/BUILD index c2e48e2eb20..31274c71d86 100644 --- a/staging/src/k8s.io/apiserver/pkg/util/feature/BUILD +++ b/staging/src/k8s.io/apiserver/pkg/util/feature/BUILD @@ -10,7 +10,10 @@ go_test( name = "go_default_test", srcs = ["feature_gate_test.go"], embed = [":go_default_library"], - deps = ["//vendor/github.com/spf13/pflag:go_default_library"], + deps = [ + "//vendor/github.com/spf13/pflag:go_default_library", + "//vendor/github.com/stretchr/testify/assert:go_default_library", + ], ) go_library( 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 6b051a235a0..8847c1fb625 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 @@ -145,54 +145,24 @@ func NewFeatureGate() *featureGate { // Set parses a string of the form "key1=value1,key2=value2,..." into a // map[string]bool of known keys or returns an error. func (f *featureGate) Set(value string) error { - f.lock.Lock() - defer f.lock.Unlock() - - // 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 - } - + m := make(map[string]bool) for _, s := range strings.Split(value, ",") { if len(s) == 0 { continue } arr := strings.SplitN(s, "=", 2) - k := Feature(strings.TrimSpace(arr[0])) - featureSpec, ok := known[k] - if !ok { - return fmt.Errorf("unrecognized key: %s", k) - } + k := strings.TrimSpace(arr[0]) if len(arr) != 2 { return fmt.Errorf("missing bool value for %s", k) } v := strings.TrimSpace(arr[1]) boolValue, err := strconv.ParseBool(v) if err != nil { - return fmt.Errorf("invalid value of %s: %s, err: %v", k, v, err) - } - enabled[k] = boolValue - if boolValue && featureSpec.PreRelease == Deprecated { - glog.Warningf("enabling deprecated feature gate %s", k) - } - - // Handle "special" features like "all alpha gates" - if fn, found := f.special[k]; found { - fn(known, enabled, boolValue) + return fmt.Errorf("invalid value of %s=%s, err: %v", k, v, err) } + m[k] = boolValue } - - // Persist changes - f.known.Store(known) - f.enabled.Store(enabled) - - glog.V(1).Infof("feature gates: %v", enabled) - return nil + return f.SetFromMap(m) } // SetFromMap stores flag gates for known features from a map[string]bool or returns an error @@ -212,15 +182,21 @@ func (f *featureGate) SetFromMap(m map[string]bool) error { for k, v := range m { k := Feature(k) - _, ok := known[k] + featureSpec, ok := known[k] if !ok { - return fmt.Errorf("unrecognized key: %s", k) + return fmt.Errorf("unrecognized feature gate: %s", k) } enabled[k] = v // Handle "special" features like "all alpha gates" if fn, found := f.special[k]; found { fn(known, enabled, v) } + + if featureSpec.PreRelease == Deprecated { + glog.Warningf("Setting deprecated feature gate %s=%t. It will be removed in a future release.", k, v) + } else if featureSpec.PreRelease == GA { + glog.Warningf("Setting GA feature gate %s=%t. It will be removed in a future release.", k, v) + } } // Persist changes @@ -302,14 +278,14 @@ func (f *featureGate) AddFlag(fs *pflag.FlagSet) { } // KnownFeatures returns a slice of strings describing the FeatureGate's known features. +// Deprecated and GA features are hidden from the list. func (f *featureGate) KnownFeatures() []string { var known []string for k, v := range f.known.Load().(map[Feature]FeatureSpec) { - pre := "" - if v.PreRelease != GA { - pre = fmt.Sprintf("%s - ", v.PreRelease) + if v.PreRelease == GA || v.PreRelease == Deprecated { + continue } - known = append(known, fmt.Sprintf("%s=true|false (%sdefault=%t)", k, pre, v.Default)) + known = append(known, fmt.Sprintf("%s=true|false (%s - default=%t)", k, v.PreRelease, v.Default)) } sort.Strings(known) return known 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 f26f3cb88ed..14ec8694816 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 @@ -22,6 +22,7 @@ import ( "testing" "github.com/spf13/pflag" + "github.com/stretchr/testify/assert" ) func TestFeatureGateFlag(t *testing.T) { @@ -43,13 +44,13 @@ func TestFeatureGateFlag(t *testing.T) { }, }, { - arg: "fooBarBaz=maybeidk", + arg: "fooBarBaz=true", expect: map[Feature]bool{ allAlphaGate: false, testAlphaGate: false, testBetaGate: false, }, - parseError: "unrecognized key: fooBarBaz", + parseError: "unrecognized feature gate: fooBarBaz", }, { arg: "AllAlpha=false", @@ -190,6 +191,32 @@ func TestFeatureGateFlagDefaults(t *testing.T) { } } +func TestFeatureGateKnownFeatures(t *testing.T) { + // gates for testing + const ( + testAlphaGate Feature = "TestAlpha" + testBetaGate Feature = "TestBeta" + testGAGate Feature = "TestGA" + testDeprecatedGate Feature = "TestDeprecated" + ) + + // Don't parse the flag, assert defaults are used. + var f FeatureGate = NewFeatureGate() + f.Add(map[Feature]FeatureSpec{ + testAlphaGate: {Default: false, PreRelease: Alpha}, + testBetaGate: {Default: true, PreRelease: Beta}, + testGAGate: {Default: true, PreRelease: GA}, + testDeprecatedGate: {Default: false, PreRelease: Deprecated}, + }) + + known := strings.Join(f.KnownFeatures(), " ") + + assert.Contains(t, known, testAlphaGate) + assert.Contains(t, known, testBetaGate) + assert.NotContains(t, known, testGAGate) + assert.NotContains(t, known, testDeprecatedGate) +} + func TestFeatureGateSetFromMap(t *testing.T) { // gates for testing const testAlphaGate Feature = "TestAlpha" @@ -241,7 +268,7 @@ func TestFeatureGateSetFromMap(t *testing.T) { testAlphaGate: false, testBetaGate: false, }, - setmapError: "unrecognized key:", + setmapError: "unrecognized feature gate:", }, } for i, test := range tests {