Merge pull request #31140 from thockin/feature-gate-fixes

Automatic merge from submit-queue

Make 'allAlpha' a special feature gate

Rather than making all caller check both allAlpha and their own flag, make `allAlpha` set all of the alpha gates explicitly.

This is hard to test because of the globalness.  I will follow this commit with a new one to add some way to test, but I wanted to float this design

pkg/util/config/feature_gate.go

@@ -31,48 +31,78 @@ const (
 
 	// All known feature keys
 	// To add a new feature, define a key for it below and add
-	// a featureInfo entry to knownFeatures.
+	// a featureSpec entry to knownFeatures.
 
-	// allAlpha is a global toggle for alpha features. Per feature key
-	// values override the default set by allAlpha.
-	// e.g. allAlpha=false,newFeature=true will result in newFeature=true
-	allAlpha = "allAlpha"
+	// allAlphaGate is a global toggle for alpha features. Per-feature key
+	// values override the default set by allAlphaGate, if they come later in the
+	// specification of gates. Examples:
+	//   AllAlpha=false,NewFeature=true  will result in newFeature=true
+	//   AllAlpha=true,NewFeature=false  will result in newFeature=false
+	allAlphaGate = "AllAlpha"
 )
 
 var (
-	// DefaultFeatureGate is a shared global FeatureGate.
-	DefaultFeatureGate = &featureGate{}
-
-	// Default values for recorded features.
-	knownFeatures = map[string]featureInfo{
-		allAlpha: {false, alpha},
+	// Default values for recorded features.  Every new feature gate should be
+	// represented here.
+	knownFeatures = map[string]featureSpec{
+		allAlphaGate: {false, alpha},
 	}
 
+	// Special handling for a few gates.
+	specialFeatures = map[string]func(f *featureGate, val bool){
+		allAlphaGate: setUnsetAlphaGates,
+	}
+
+	// DefaultFeatureGate is a shared global FeatureGate.
+	DefaultFeatureGate = &featureGate{
+		known:   knownFeatures,
+		special: specialFeatures,
+	}
+)
+
+type featureSpec struct {
+	enabled    bool
+	prerelease prerelease
+}
+
+type prerelease string
+
+const (
+	// Values for prerelease.
 	alpha = prerelease("ALPHA")
 	beta  = prerelease("BETA")
 	ga    = prerelease("")
 )
 
-type prerelease string
-
-type featureInfo struct {
-	enabled    bool
-	prerelease prerelease
-}
-
 // FeatureGate parses and stores flag gates for known features from
 // a string like feature1=true,feature2=false,...
 type FeatureGate interface {
 	AddFlag(fs *pflag.FlagSet)
-	// owner: @jlowdermilk
-	// alpha: v1.4
-	AllAlpha() bool
+
+	// Every feature gate should add method here following this template:
+	//
+	// // owner: @username
+	// // alpha: v1.4
+	// MyFeature() bool
+
 	// TODO: Define accessors for each non-API alpha feature.
 }
 
 // featureGate implements FeatureGate as well as pflag.Value for flag parsing.
 type featureGate struct {
-	features map[string]bool
+	known   map[string]featureSpec
+	special map[string]func(*featureGate, bool)
+	enabled map[string]bool
+}
+
+func setUnsetAlphaGates(f *featureGate, val bool) {
+	for k, v := range f.known {
+		if v.prerelease == alpha {
+			if _, found := f.enabled[k]; !found {
+				f.enabled[k] = val
+			}
+		}
+	}
 }
 
 // Set, String, and Type implement pflag.Value
@@ -80,14 +110,14 @@ type featureGate struct {
 // 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.features = make(map[string]bool)
+	f.enabled = make(map[string]bool)
 	for _, s := range strings.Split(value, ",") {
 		if len(s) == 0 {
 			continue
 		}
 		arr := strings.SplitN(s, "=", 2)
 		k := strings.TrimSpace(arr[0])
-		_, ok := knownFeatures[k]
+		_, ok := f.known[k]
 		if !ok {
 			return fmt.Errorf("unrecognized key: %s", k)
 		}
@@ -99,15 +129,21 @@ func (f *featureGate) Set(value string) error {
 		if err != nil {
 			return fmt.Errorf("invalid value of %s: %s, err: %v", k, v, err)
 		}
-		f.features[k] = boolValue
+		f.enabled[k] = boolValue
+
+		// Handle "special" features like "all alpha gates"
+		if fn, found := f.special[k]; found {
+			fn(f, boolValue)
+		}
 	}
-	glog.Infof("feature gates: %v", f.features)
+
+	glog.Infof("feature gates: %v", f.enabled)
 	return nil
 }
 
 func (f *featureGate) String() string {
 	pairs := []string{}
-	for k, v := range f.features {
+	for k, v := range f.enabled {
 		pairs = append(pairs, fmt.Sprintf("%s=%t", k, v))
 	}
 	sort.Strings(pairs)
@@ -118,18 +154,12 @@ func (f *featureGate) Type() string {
 	return "mapStringBool"
 }
 
-// AllAlpha returns value for allAlpha.
-func (f *featureGate) AllAlpha() bool {
-	return f.lookup(allAlpha)
-}
-
 func (f *featureGate) lookup(key string) bool {
-	defaultValue := knownFeatures[key].enabled
-	if f.features == nil {
-		panic(fmt.Sprintf("--%s has not been parsed", flagName))
-	}
-	if v, ok := f.features[key]; ok {
-		return v
+	defaultValue := f.known[key].enabled
+	if f.enabled != nil {
+		if v, ok := f.enabled[key]; ok {
+			return v
+		}
 	}
 	return defaultValue
 
@@ -138,7 +168,7 @@ func (f *featureGate) lookup(key string) bool {
 // AddFlag adds a flag for setting global feature gates to the specified FlagSet.
 func (f *featureGate) AddFlag(fs *pflag.FlagSet) {
 	var known []string
-	for k, v := range knownFeatures {
+	for k, v := range f.known {
 		pre := ""
 		if v.prerelease != ga {
 			pre = fmt.Sprintf("%s - ", v.prerelease)

pkg/util/config/feature_gate_test.go

@@ -25,32 +25,135 @@ import (
 )
 
 func TestFeatureGateFlag(t *testing.T) {
+	// gates for testing
+	const testAlphaGate = "TestAlpha"
+	const testBetaGate = "TestBeta"
+
 	tests := []struct {
 		arg        string
-		allAlpha   bool
-		parseError error
+		expect     map[string]bool
+		parseError string
 	}{
-		{fmt.Sprintf("--%s=fooBarBaz=maybeidk", flagName), false, fmt.Errorf("unrecognized key: fooBarBaz")},
-		{fmt.Sprintf("--%s=", flagName), false, nil},
-		{fmt.Sprintf("--%s=allAlpha=false", flagName), false, nil},
-		{fmt.Sprintf("--%s=allAlpha=true", flagName), true, nil},
-		{fmt.Sprintf("--%s=allAlpha=banana", flagName), false, fmt.Errorf("invalid value of allAlpha")},
+		{
+			arg: "",
+			expect: map[string]bool{
+				allAlphaGate:  false,
+				testAlphaGate: false,
+				testBetaGate:  false,
+			},
+		},
+		{
+			arg: "fooBarBaz=maybeidk",
+			expect: map[string]bool{
+				allAlphaGate:  false,
+				testAlphaGate: false,
+				testBetaGate:  false,
+			},
+			parseError: "unrecognized key: fooBarBaz",
+		},
+		{
+			arg: "AllAlpha=false",
+			expect: map[string]bool{
+				allAlphaGate:  false,
+				testAlphaGate: false,
+				testBetaGate:  false,
+			},
+		},
+		{
+			arg: "AllAlpha=true",
+			expect: map[string]bool{
+				allAlphaGate:  true,
+				testAlphaGate: true,
+				testBetaGate:  false,
+			},
+		},
+		{
+			arg: "AllAlpha=banana",
+			expect: map[string]bool{
+				allAlphaGate:  false,
+				testAlphaGate: false,
+				testBetaGate:  false,
+			},
+			parseError: "invalid value of AllAlpha",
+		},
+		{
+			arg: "AllAlpha=false,TestAlpha=true",
+			expect: map[string]bool{
+				allAlphaGate:  false,
+				testAlphaGate: true,
+				testBetaGate:  false,
+			},
+		},
+		{
+			arg: "TestAlpha=true,AllAlpha=false",
+			expect: map[string]bool{
+				allAlphaGate:  false,
+				testAlphaGate: true,
+				testBetaGate:  false,
+			},
+		},
+		{
+			arg: "AllAlpha=true,TestAlpha=false",
+			expect: map[string]bool{
+				allAlphaGate:  true,
+				testAlphaGate: false,
+				testBetaGate:  false,
+			},
+		},
+		{
+			arg: "TestAlpha=false,AllAlpha=true",
+			expect: map[string]bool{
+				allAlphaGate:  true,
+				testAlphaGate: false,
+				testBetaGate:  false,
+			},
+		},
+		{
+			arg: "TestBeta=true,AllAlpha=false",
+			expect: map[string]bool{
+				allAlphaGate:  false,
+				testAlphaGate: false,
+				testBetaGate:  true,
+			},
+		},
 	}
 	for i, test := range tests {
 		fs := pflag.NewFlagSet("testfeaturegateflag", pflag.ContinueOnError)
-		f := &featureGate{}
+		f := DefaultFeatureGate
+		f.known[testAlphaGate] = featureSpec{false, alpha}
+		f.known[testBetaGate] = featureSpec{false, beta}
 		f.AddFlag(fs)
 
-		err := fs.Parse([]string{test.arg})
-		if test.parseError != nil {
-			if !strings.Contains(err.Error(), test.parseError.Error()) {
+		err := fs.Parse([]string{fmt.Sprintf("--%s=%s", flagName, test.arg)})
+		if test.parseError != "" {
+			if !strings.Contains(err.Error(), test.parseError) {
 				t.Errorf("%d: Parse() Expected %v, Got %v", i, test.parseError, err)
 			}
 		} else if err != nil {
 			t.Errorf("%d: Parse() Expected nil, Got %v", i, err)
 		}
-		if alpha := f.AllAlpha(); alpha != test.allAlpha {
-			t.Errorf("%d: AlphaEnabled() expected %v, Got %v", i, test.allAlpha, alpha)
+		for k, v := range test.expect {
+			if f.enabled[k] != v {
+				t.Errorf("%d: expected %s=%v, Got %v", i, k, v, f.enabled[k])
+			}
 		}
 	}
 }
+
+func TestFeatureGateFlagDefaults(t *testing.T) {
+	// gates for testing
+	const testAlphaGate = "TestAlpha"
+	const testBetaGate = "TestBeta"
+
+	// Don't parse the flag, assert defaults are used.
+	f := DefaultFeatureGate
+	f.known[testAlphaGate] = featureSpec{false, alpha}
+	f.known[testBetaGate] = featureSpec{true, beta}
+
+	if f.lookup(testAlphaGate) != false {
+		t.Errorf("Expected false")
+	}
+	if f.lookup(testBetaGate) != true {
+		t.Errorf("Expected true")
+	}
+}