From 46d0e1d5aaad03d0b8bbfa07874b3de5fa5da76e Mon Sep 17 00:00:00 2001 From: Ben Luddy Date: Tue, 10 Oct 2023 16:00:33 -0400 Subject: [PATCH] Support overrides for registered feature defaults. This is to support the goal of enabling a feature by default for a single component only when the feature in question is consumed by multiple components. Overriden defaults are reflected in KnownFeatures and registered flag text. --- .../featuregate/feature_gate.go | 43 ++++++ .../featuregate/feature_gate_test.go | 138 ++++++++++++++++++ 2 files changed, 181 insertions(+) diff --git a/staging/src/k8s.io/component-base/featuregate/feature_gate.go b/staging/src/k8s.io/component-base/featuregate/feature_gate.go index a826b0e67ef..6884a3ee1f4 100644 --- a/staging/src/k8s.io/component-base/featuregate/feature_gate.go +++ b/staging/src/k8s.io/component-base/featuregate/feature_gate.go @@ -115,6 +115,17 @@ type MutableFeatureGate interface { GetAll() map[Feature]FeatureSpec // AddMetrics adds feature enablement metrics AddMetrics() + // OverrideDefault sets a local override for the registered default value of a named + // feature. If the feature has not been previously registered (e.g. by a call to Add), has a + // locked default, or if the gate has already registered itself with a FlagSet, a non-nil + // error is returned. + // + // When two or more components consume a common feature, one component can override its + // default at runtime in order to adopt new defaults before or after the other + // components. For example, a new feature can be evaluated with a limited blast radius by + // overriding its default to true for a limited number of components without simultaneously + // changing its default for all consuming components. + OverrideDefault(name Feature, override bool) error } // featureGate implements FeatureGate as well as pflag.Value for flag parsing. @@ -296,6 +307,38 @@ func (f *featureGate) Add(features map[Feature]FeatureSpec) error { return nil } +func (f *featureGate) OverrideDefault(name Feature, override bool) error { + f.lock.Lock() + defer f.lock.Unlock() + + if f.closed { + return fmt.Errorf("cannot override default for feature %q: gates already added to a flag set", name) + } + + known := map[Feature]FeatureSpec{} + for name, spec := range f.known.Load().(map[Feature]FeatureSpec) { + known[name] = spec + } + + spec, ok := known[name] + switch { + case !ok: + return fmt.Errorf("cannot override default: feature %q is not registered", name) + case spec.LockToDefault: + return fmt.Errorf("cannot override default: feature %q default is locked to %t", name, spec.Default) + case spec.PreRelease == Deprecated: + klog.Warningf("Overriding default of deprecated feature gate %s=%t. It will be removed in a future release.", name, override) + case spec.PreRelease == GA: + klog.Warningf("Overriding default of GA feature gate %s=%t. It will be removed in a future release.", name, override) + } + + spec.Default = override + known[name] = spec + f.known.Store(known) + + return nil +} + // GetAll returns a copy of the map of known feature names to feature specs. func (f *featureGate) GetAll() map[Feature]FeatureSpec { retval := map[Feature]FeatureSpec{} diff --git a/staging/src/k8s.io/component-base/featuregate/feature_gate_test.go b/staging/src/k8s.io/component-base/featuregate/feature_gate_test.go index c8537a6a514..063a5a36368 100644 --- a/staging/src/k8s.io/component-base/featuregate/feature_gate_test.go +++ b/staging/src/k8s.io/component-base/featuregate/feature_gate_test.go @@ -508,3 +508,141 @@ func TestFeatureGateString(t *testing.T) { }) } } + +func TestFeatureGateOverrideDefault(t *testing.T) { + t.Run("overrides take effect", func(t *testing.T) { + f := NewFeatureGate() + if err := f.Add(map[Feature]FeatureSpec{ + "TestFeature1": {Default: true}, + "TestFeature2": {Default: false}, + }); err != nil { + t.Fatal(err) + } + if err := f.OverrideDefault("TestFeature1", false); err != nil { + t.Fatal(err) + } + if err := f.OverrideDefault("TestFeature2", true); err != nil { + t.Fatal(err) + } + if f.Enabled("TestFeature1") { + t.Error("expected TestFeature1 to have effective default of false") + } + if !f.Enabled("TestFeature2") { + t.Error("expected TestFeature2 to have effective default of true") + } + }) + + t.Run("overrides are preserved across deep copies", func(t *testing.T) { + f := NewFeatureGate() + if err := f.Add(map[Feature]FeatureSpec{"TestFeature": {Default: false}}); err != nil { + t.Fatal(err) + } + if err := f.OverrideDefault("TestFeature", true); err != nil { + t.Fatal(err) + } + fcopy := f.DeepCopy() + if !fcopy.Enabled("TestFeature") { + t.Error("default override was not preserved by deep copy") + } + }) + + t.Run("reflected in known features", func(t *testing.T) { + f := NewFeatureGate() + if err := f.Add(map[Feature]FeatureSpec{"TestFeature": { + Default: false, + PreRelease: Alpha, + }}); err != nil { + t.Fatal(err) + } + if err := f.OverrideDefault("TestFeature", true); err != nil { + t.Fatal(err) + } + var found bool + for _, s := range f.KnownFeatures() { + if !strings.Contains(s, "TestFeature") { + continue + } + found = true + if !strings.Contains(s, "default=true") { + t.Errorf("expected override of default to be reflected in known feature description %q", s) + } + } + if !found { + t.Error("found no entry for TestFeature in known features") + } + }) + + t.Run("may not change default for specs with locked defaults", func(t *testing.T) { + f := NewFeatureGate() + if err := f.Add(map[Feature]FeatureSpec{ + "LockedFeature": { + Default: true, + LockToDefault: true, + }, + }); err != nil { + t.Fatal(err) + } + if f.OverrideDefault("LockedFeature", false) == nil { + t.Error("expected error when attempting to override the default for a feature with a locked default") + } + if f.OverrideDefault("LockedFeature", true) == nil { + t.Error("expected error when attempting to override the default for a feature with a locked default") + } + }) + + t.Run("does not supersede explicitly-set value", func(t *testing.T) { + f := NewFeatureGate() + if err := f.Add(map[Feature]FeatureSpec{"TestFeature": {Default: true}}); err != nil { + t.Fatal(err) + } + if err := f.OverrideDefault("TestFeature", false); err != nil { + t.Fatal(err) + } + if err := f.SetFromMap(map[string]bool{"TestFeature": true}); err != nil { + t.Fatal(err) + } + if !f.Enabled("TestFeature") { + t.Error("expected feature to be effectively enabled despite default override") + } + }) + + t.Run("prevents re-registration of feature spec after overriding default", func(t *testing.T) { + f := NewFeatureGate() + if err := f.Add(map[Feature]FeatureSpec{ + "TestFeature": { + Default: true, + PreRelease: Alpha, + }, + }); err != nil { + t.Fatal(err) + } + if err := f.OverrideDefault("TestFeature", false); err != nil { + t.Fatal(err) + } + if err := f.Add(map[Feature]FeatureSpec{ + "TestFeature": { + Default: true, + PreRelease: Alpha, + }, + }); err == nil { + t.Error("expected re-registration to return a non-nil error after overriding its default") + } + }) + + t.Run("does not allow override for an unknown feature", func(t *testing.T) { + f := NewFeatureGate() + if err := f.OverrideDefault("TestFeature", true); err == nil { + t.Error("expected an error to be returned in attempt to override default for unregistered feature") + } + }) + + t.Run("returns error if already added to flag set", func(t *testing.T) { + f := NewFeatureGate() + fs := pflag.NewFlagSet("test", pflag.ContinueOnError) + f.AddFlag(fs) + + if err := f.OverrideDefault("TestFeature", true); err == nil { + t.Error("expected a non-nil error to be returned") + } + }) +}