HACK, UNTESTED: suppert external gate providers

Author: thockin

gate_example.go

@@ -0,0 +1,43 @@
+package main
+
+import (
+	"fmt"
+
+	"github.com/spf13/pflag"
+	"k8s.io/utils/feature"
+)
+
+var (
+	libGates = &feature.GateSet{}
+	f1       = libGates.Add(&feature.Gate{
+		Name:    "Feature1",
+		Default: false,
+		Release: feature.Alpha,
+	})
+	f2 = libGates.Add(&feature.Gate{
+		Name:    "Feature2",
+		Default: true,
+		Release: feature.Beta,
+	})
+	f3 = libGates.Add(&feature.Gate{
+		Name:          "Feature3",
+		Default:       true,
+		Release:       feature.GA,
+		LockToDefault: true,
+	})
+	f4 = &feature.Gate{
+		Name:    "Feature4",
+		Default: true,
+		Release: feature.Beta,
+	}
+)
+
+func main() {
+	libGates.EnablePFlagControl("gates", pflag.CommandLine)
+	libGates.EnableEnvControl("GATE_")
+	pflag.Parse()
+	fmt.Println(feature.Enabled(f1))
+	fmt.Println(feature.Enabled(f2))
+	fmt.Println(feature.Enabled(f3))
+	fmt.Println(feature.Enabled(f4))
+}

staging/src/k8s.io/apiserver/pkg/server/options/server_run_options.go

@@ -340,5 +340,6 @@ func (s *ServerRunOptions) AddUniversalFlags(fs *pflag.FlagSet) {
 		"This option, if set, represents the maximum amount of grace period the apiserver will wait "+
 		"for active watch request(s) to drain during the graceful server shutdown window.")
 
-	s.FeatureGates.AddFlags("new-feature-gates", fs)
+	s.FeatureGates.EnableFlagControl("new-feature-gates", fs)
+	s.FeatureGates.EnableEnvControl("KUBE_FEATURE_")
 }

staging/src/k8s.io/utils/feature/feature.go

@@ -24,7 +24,6 @@ import (
 	"sort"
 	"strconv"
 	"strings"
-	"sync"
 
 	"github.com/spf13/pflag"
 	utilruntime "k8s.io/apimachinery/pkg/util/runtime"
@@ -36,7 +35,8 @@ import (
 // GateSet is a list of feature gates.  All Gates in a GateSet must have unique
 // names.
 type GateSet struct {
-	gateMap map[string]*Gate
+	gateMap   map[string]*Gate
+	providers []GateProvider
 }
 
 // Add adds the specified Gate to the GateSet.
@@ -54,16 +54,23 @@ func (gs *GateSet) Add(gate *Gate) *Gate {
 		//FIXME: what should we do?
 	}
 	gs.gateMap[gate.Name] = gate
+	gate.set = gs
 	return gate
 }
 
-// Merge adds all of the Gates in other to this GateSet.
+// Merge moves all of the Gates in other to this GateSet.
 func (gs *GateSet) Merge(other *GateSet) {
 	for _, v := range other.gateMap {
 		gs.Add(v)
+		//FIXME: remove from other?
 	}
 }
 
+type GateProvider interface {
+	//FIXME: comments, (val, found) return
+	Get(name string) (bool, bool)
+}
+
 const (
 	// allAlphaGate is a global toggle for alpha features. Per-feature key
 	// values override the default set by allAlphaGate. Examples:
@@ -78,12 +85,11 @@ const (
 	allBetaGate = "AllBeta"
 )
 
-// AddFlags adds gate-related flags to the specified FlagSet.
-// FIXME: how are gates exposed today?
-func (gs *GateSet) AddFlags(flagName string, fs *pflag.FlagSet) {
-	for _, gate := range gs.gateMap {
-		gate.readEnvVar()
-	}
+// FIXME: comments
+// FIXME: We could move all the flag and env stuff out of this lib, and make
+// FIXME:   callers do it, but it's considerably less useful.
+// FIXME: merge with existing gates flag and config
+func (gs *GateSet) EnablePFlagControl(flagName string, fs *pflag.FlagSet) {
 	helpStrings := []string{
 		fmt.Sprintf("%s=true|false (%s - default=%t)", allAlphaGate, Alpha, false),
 		fmt.Sprintf("%s=true|false (%s - default=%t)", allBetaGate, Beta, false),
@@ -94,14 +100,31 @@ func (gs *GateSet) AddFlags(flagName string, fs *pflag.FlagSet) {
 	sort.Strings(helpStrings)
 
 	//FIXME: nicer to have a --list-feature-gates flag?
-	fs.Var(&flagValue{gs}, flagName, ""+
+	fv := &flagValue{gates: gs}
+	fs.Var(fv, flagName, ""+
 		"A set of key=value pairs that describe feature gates for alpha/experimental features. "+
 		"Options are:\n"+strings.Join(helpStrings, "\n"))
+
+	gs.providers = append(gs.providers, fv)
 }
 
-// flagValue implements pflag.Value in terms of a GateSet.
+// flagValue implements pflag.Value and GateProvider.
 type flagValue struct {
-	gates *GateSet
+	gates  *GateSet
+	values map[string]bool
+}
+
+// Implement GateProvider.
+func (fv *flagValue) Get(name string) (bool, bool) {
+	val, found := fv.values[name]
+	return val, found
+}
+
+func (fv *flagValue) setOne(k string, v bool) {
+	if fv.values == nil {
+		fv.values = map[string]bool{}
+	}
+	fv.values[k] = v
 }
 
 // Set parses a string of the form "key1=value1,key2=value2,..." into a
@@ -124,43 +147,42 @@ func (fv *flagValue) Set(value string) error {
 		}
 		m[k] = boolValue
 	}
-	return fv.setFromMap(m)
-}
 
-func (fv *flagValue) setFromMap(m map[string]bool) error {
 	for name, enabled := range m {
 		switch {
 		case name == allAlphaGate:
 			for _, gate := range fv.gates.gateMap {
 				if gate.Release == Alpha {
-					gate.Set(enabled)
+					fv.setOne(gate.Name, enabled)
 				}
 			}
 			continue
 		case name == allBetaGate:
 			for _, gate := range fv.gates.gateMap {
 				if gate.Release == Beta {
-					gate.Set(enabled)
+					fv.setOne(gate.Name, enabled)
 				}
 			}
 			continue
 		}
 
+		// FIXME: do we need this?  Env won't fail with error unless we scan all vars
 		gate, exists := fv.gates.gateMap[name]
 		if !exists {
 			return fmt.Errorf("unrecognized feature gate: %q", name)
 		}
 		if gate.LockToDefault && gate.Default != enabled {
 			return fmt.Errorf("cannot set feature gate %q to %t, feature is locked to %t", name, enabled, gate.Default)
 		}
-		gate.Set(enabled)
+		fv.setOne(gate.Name, enabled)
 	}
 
 	return nil
 }
 
 // String returns a string containing all enabled feature gates, formatted as "key1=value1,key2=value2,...".
 func (fv *flagValue) String() string {
+	//FIXME: should this list all gates or just ones that are set?
 	pairs := []string{}
 	for _, gate := range fv.gates.gateMap {
 		pairs = append(pairs, fmt.Sprintf("%s=%t", gate.Name, Enabled(gate)))
@@ -176,14 +198,76 @@ func (fv *flagValue) Type() string {
 	return "mapStringBool"
 }
 
+func (gs *GateSet) EnableEnvControl(prefix string) {
+	//FIXME: make sure all env vars with this prefix are known gates?
+	gs.providers = append(gs.providers, &envProvider{prefix: prefix})
+}
+
+// envProvider implements GateProvider.
+type envProvider struct {
+	prefix string
+	values map[string]bool
+}
+
+// Implement GateProvider.
+func (ep *envProvider) Get(name string) (bool, bool) {
+	if ep.values == nil {
+		ep.values = map[string]bool{}
+	}
+	key := fmt.Sprintf("%s%s", ep.prefix, name)
+	strVal, found := os.LookupEnv(key)
+	if !found {
+		return false, false
+	}
+
+	boolVal, err := strconv.ParseBool(strVal)
+	if err != nil {
+		//FIXME: blech
+		utilruntime.HandleError(fmt.Errorf("cannot set feature gate %q to %q: %v", key, strVal, err))
+	}
+	/* FIXME: check if gate is allowed to be overridden?
+	if g.LockToDefault {
+		if boolVal != g.Default {
+			utilruntime.HandleError(fmt.Errorf("cannot set feature gate %q to %q: feature is locked to %t", feature, strVal, g.Default))
+		}
+	}
+	*/
+	return boolVal, true
+}
+
+// FIXME: comments
+func (gs *GateSet) EnableExternalControl(provider GateProvider) {
+	gs.providers = append(gs.providers, provider)
+}
+
+// FIXME: comments
+func (gs *GateSet) Enabled(name string) (enabled bool, found bool) {
+	foundAny := false
+	for _, provider := range gs.providers {
+		en, found := provider.Get(name)
+		if !found {
+			continue
+		}
+		foundAny = true
+		if en {
+			return true, true
+		}
+	}
+	if foundAny {
+		// We didn't early-return so we must have found a `false`
+		return false, true
+	}
+	return false, false
+}
+
 // Gate is a single feature gate definition.
 type Gate struct {
-	Name            string
-	Default         bool
-	Release         StabilityLevel
-	LockToDefault   bool
-	enabled         *bool
-	readEnvVarsOnce sync.Once
+	Name          string
+	Default       bool
+	Release       StabilityLevel
+	LockToDefault bool
+	forcedValue   *bool
+	set           *GateSet
 }
 
 // StabilityLevel indicates which phase of the feature lifecycle a given gate is
@@ -198,30 +282,9 @@ const (
 )
 
 // Set sets the value of this gate.
+// FIXME: comments regardless of other gate config sources
 func (g *Gate) Set(val bool) {
-	g.enabled = &val
-}
-
-// readEnvVar looks for an environment variable for this gate and (if found)
-
-func (g *Gate) readEnvVar() {
-	g.readEnvVarsOnce.Do(func() {
-		feature := g.Name
-		strVal, found := os.LookupEnv(fmt.Sprintf("KUBE_FEATURE_%s", feature))
-		if !found {
-			return
-		}
-
-		if boolVal, err := strconv.ParseBool(strVal); err != nil {
-			utilruntime.HandleError(fmt.Errorf("cannot set feature gate %q to %q: %v", feature, strVal, err))
-		} else if g.LockToDefault {
-			if boolVal != g.Default {
-				utilruntime.HandleError(fmt.Errorf("cannot set feature gate %q to %q: feature is locked to %t", feature, strVal, g.Default))
-			}
-		} else {
-			g.Set(boolVal)
-		}
-	})
+	g.forcedValue = &val
 }
 
 // MergeGateSets combines a list of GateSets into a new GateSet.
@@ -236,11 +299,21 @@ func MergeGateSets(gatesets ...*GateSet) *GateSet {
 // FIXME: This could be a function here or a method on Gate, but let's not do both?
 // Enabled returns whether the specified Gate is enabled.
 func Enabled(g *Gate) bool {
-	g.readEnvVar()
-	if g.enabled == nil {
+	// If some code called Set(), we always respect that.
+	if g.forcedValue != nil {
+		return *(g.forcedValue)
+	}
+	// If the value is locked or this gate is not in a set, use the default.
+	if g.LockToDefault || g.set == nil {
 		return g.Default
 	}
-	return *g.enabled
+	// Otherwise let the set's configured providers have an opinion.
+	en, found := g.set.Enabled(g.Name)
+	if found {
+		return en
+	}
+	// Otherwise, use the default.
+	return g.Default
 }
 
 // testHelper is the locally-used subset of testing.T needed in SetForTesting.
@@ -254,10 +327,10 @@ type testHelper interface {
 func SetForTesting(g *Gate, t testHelper, enabled bool) func() {
 	t.Helper()
 
-	prev := g.enabled
-	g.enabled = &enabled
+	prev := g.forcedValue
+	g.forcedValue = &enabled
 
 	return func() {
-		g.enabled = prev
+		g.forcedValue = prev
 	}
 }