From 69ebaaebca974e129a778e545d956a7b75e8399e Mon Sep 17 00:00:00 2001 From: Baek Date: Thu, 23 May 2024 16:30:28 +0000 Subject: [PATCH 1/3] featuregate: adds EtcdServer.FeatureEnabled interface. The interface can be used throughout the etcd server binary to check if the feature is enabled or not. Note that this commit also copies necessary FeatureGate interface from k8s component-base. Signed-off-by: Baek --- go.mod | 4 +-- go.sum | 4 +-- server/config/config.go | 4 +++ server/etcdserver/server.go | 6 ++++ server/internal/pkg/featuregate/doc.go | 17 +++++++++ .../internal/pkg/featuregate/feature_gate.go | 35 +++++++++++++++++++ 6 files changed, 66 insertions(+), 4 deletions(-) create mode 100644 server/internal/pkg/featuregate/doc.go create mode 100644 server/internal/pkg/featuregate/feature_gate.go diff --git a/go.mod b/go.mod index d5bd3cb7ebe..878ac5e1d85 100644 --- a/go.mod +++ b/go.mod @@ -1,6 +1,6 @@ module go.etcd.io/etcd/v3 -go 1.22 +go 1.22.0 toolchain go1.22.4 @@ -95,6 +95,6 @@ require ( google.golang.org/genproto/googleapis/rpc v0.0.0-20240520151616-dc85e6b867a5 // indirect gopkg.in/natefinch/lumberjack.v2 v2.2.1 // indirect gopkg.in/yaml.v3 v3.0.1 // indirect - sigs.k8s.io/json v0.0.0-20211020170558-c049b76a60c6 // indirect + sigs.k8s.io/json v0.0.0-20221116044647-bc3834ca7abd // indirect sigs.k8s.io/yaml v1.4.0 // indirect ) diff --git a/go.sum b/go.sum index b4f42c86d11..bbdd90c9679 100644 --- a/go.sum +++ b/go.sum @@ -250,7 +250,7 @@ gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA= gopkg.in/yaml.v3 v3.0.1/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= honnef.co/go/tools v0.0.0-20190102054323-c2f93a96b099/go.mod h1:rf3lG4BRIbNafJWhAfAdb/ePZxsR/4RtNHQocxwk9r4= honnef.co/go/tools v0.0.0-20190523083050-ea95bdfd59fc/go.mod h1:rf3lG4BRIbNafJWhAfAdb/ePZxsR/4RtNHQocxwk9r4= -sigs.k8s.io/json v0.0.0-20211020170558-c049b76a60c6 h1:fD1pz4yfdADVNfFmcP2aBEtudwUQ1AlLnRBALr33v3s= -sigs.k8s.io/json v0.0.0-20211020170558-c049b76a60c6/go.mod h1:p4QtZmO4uMYipTQNzagwnNoseA6OxSUutVw05NhYDRs= +sigs.k8s.io/json v0.0.0-20221116044647-bc3834ca7abd h1:EDPBXCAspyGV4jQlpZSudPeMmr1bNJefnuqLsRAsHZo= +sigs.k8s.io/json v0.0.0-20221116044647-bc3834ca7abd/go.mod h1:B8JuhiUyNFVKdsE8h686QcCxMaH6HrOAZj4vswFpcB0= sigs.k8s.io/yaml v1.4.0 h1:Mk1wCc2gy/F0THH0TAp1QYyJNzRm2KCLy3o5ASXVI5E= sigs.k8s.io/yaml v1.4.0/go.mod h1:Ejl7/uTz7PSA4eKMyQCUTnhZYNmLIl+5c2lQPGR2BPY= diff --git a/server/config/config.go b/server/config/config.go index d8edc514f45..bc767d4ca40 100644 --- a/server/config/config.go +++ b/server/config/config.go @@ -30,6 +30,7 @@ import ( "go.etcd.io/etcd/client/pkg/v3/types" "go.etcd.io/etcd/pkg/v3/netutil" "go.etcd.io/etcd/server/v3/etcdserver/api/v3discovery" + "go.etcd.io/etcd/server/v3/internal/pkg/featuregate" "go.etcd.io/etcd/server/v3/storage/datadir" ) @@ -207,6 +208,9 @@ type ServerConfig struct { // ExperimentalLocalAddress is the local IP address to use when communicating with a peer. ExperimentalLocalAddress string `json:"experimental-local-address"` + + // ServerFeatureGate is a server level feature gate + ServerFeatureGate featuregate.FeatureGate } // VerifyBootstrap sanity-checks the initial config for bootstrap case diff --git a/server/etcdserver/server.go b/server/etcdserver/server.go index 2eebb421de1..2e849a2ebd3 100644 --- a/server/etcdserver/server.go +++ b/server/etcdserver/server.go @@ -62,6 +62,7 @@ import ( "go.etcd.io/etcd/server/v3/etcdserver/errors" "go.etcd.io/etcd/server/v3/etcdserver/txn" serverversion "go.etcd.io/etcd/server/v3/etcdserver/version" + "go.etcd.io/etcd/server/v3/internal/pkg/featuregate" "go.etcd.io/etcd/server/v3/lease" "go.etcd.io/etcd/server/v3/lease/leasehttp" serverstorage "go.etcd.io/etcd/server/v3/storage" @@ -456,6 +457,11 @@ func (s *EtcdServer) Config() config.ServerConfig { return s.Cfg } +// FeatureEnabled returns true if the feature is enabled by the etcd server, false otherwise. +func (s *EtcdServer) FeatureEnabled(f featuregate.Feature) bool { + return s.Cfg.ServerFeatureGate.Enabled(f) +} + func tickToDur(ticks int, tickMs uint) string { return fmt.Sprintf("%v", time.Duration(ticks)*time.Duration(tickMs)*time.Millisecond) } diff --git a/server/internal/pkg/featuregate/doc.go b/server/internal/pkg/featuregate/doc.go new file mode 100644 index 00000000000..64d74ae51ed --- /dev/null +++ b/server/internal/pkg/featuregate/doc.go @@ -0,0 +1,17 @@ +// Copyright 2024 The etcd Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +// featuregate package is copied from k8s.io/component-base@v0.30.1 to avoid any potential circular dependency between k8s and etcd. +// Note that only necessary portions are copied to support etcd feature gate functionalities.s +package featuregate diff --git a/server/internal/pkg/featuregate/feature_gate.go b/server/internal/pkg/featuregate/feature_gate.go new file mode 100644 index 00000000000..a60c08c465b --- /dev/null +++ b/server/internal/pkg/featuregate/feature_gate.go @@ -0,0 +1,35 @@ +// Copyright 2024 The etcd Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package featuregate + +type Feature string + +// 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 +} From 60e3f454690bb0dac5af24ce00f14ab303565b17 Mon Sep 17 00:00:00 2001 From: Baek Date: Thu, 13 Jun 2024 22:46:54 +0000 Subject: [PATCH 2/3] Adds all feature_gate from component-base. We'll likely use most of the feature_gate package from component-base. Also this commit moves the pkg from server/internal/pkg to pkg/. Signed-off-by: Baek --- pkg/featuregate/feature_gate.go | 417 ++++++++++++ pkg/featuregate/feature_gate_test.go | 612 ++++++++++++++++++ server/config/config.go | 2 +- server/etcdserver/server.go | 2 +- server/internal/pkg/featuregate/doc.go | 17 - .../internal/pkg/featuregate/feature_gate.go | 35 - 6 files changed, 1031 insertions(+), 54 deletions(-) create mode 100644 pkg/featuregate/feature_gate.go create mode 100644 pkg/featuregate/feature_gate_test.go delete mode 100644 server/internal/pkg/featuregate/doc.go delete mode 100644 server/internal/pkg/featuregate/feature_gate.go diff --git a/pkg/featuregate/feature_gate.go b/pkg/featuregate/feature_gate.go new file mode 100644 index 00000000000..1c6f8f88704 --- /dev/null +++ b/pkg/featuregate/feature_gate.go @@ -0,0 +1,417 @@ +// Copyright 2024 The etcd Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +// featuregate package is copied from k8s.io/component-base@v0.30.1 to avoid any potential circular dependency between k8s and etcd. +package featuregate + +import ( + "context" + "fmt" + "sort" + "strconv" + "strings" + "sync" + "sync/atomic" + + "github.com/spf13/pflag" + + "k8s.io/apimachinery/pkg/util/naming" + "k8s.io/klog/v2" +) + +type Feature string + +const ( + flagName = "feature-gates" + + // allAlphaGate is a global toggle for alpha features. Per-feature key + // values override the default set by allAlphaGate. Examples: + // AllAlpha=false,NewFeature=true will result in newFeature=true + // AllAlpha=true,NewFeature=false will result in newFeature=false + allAlphaGate Feature = "AllAlpha" + + // allBetaGate is a global toggle for beta features. Per-feature key + // values override the default set by allBetaGate. Examples: + // AllBeta=false,NewFeature=true will result in NewFeature=true + // AllBeta=true,NewFeature=false will result in NewFeature=false + allBetaGate Feature = "AllBeta" +) + +var ( + // The generic features. + defaultFeatures = map[Feature]FeatureSpec{ + allAlphaGate: {Default: false, PreRelease: Alpha}, + allBetaGate: {Default: false, PreRelease: Beta}, + } + + // Special handling for a few gates. + specialFeatures = map[Feature]func(known map[Feature]FeatureSpec, enabled map[Feature]bool, val bool){ + allAlphaGate: setUnsetAlphaGates, + allBetaGate: setUnsetBetaGates, + } +) + +type FeatureSpec struct { + // Default is the default enablement state for the feature + Default bool + // LockToDefault indicates that the feature is locked to its default and cannot be changed + LockToDefault bool + // PreRelease indicates the maturity level of the feature + PreRelease prerelease +} + +type prerelease string + +const ( + // Values for PreRelease. + Alpha = prerelease("ALPHA") + Beta = prerelease("BETA") + GA = prerelease("") + + // Deprecated + Deprecated = prerelease("DEPRECATED") +) + +// 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 + // from a string like feature1=true,feature2=false,... + 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 + // Add adds features to the featureGate. + Add(features map[Feature]FeatureSpec) error + // GetAll returns a copy of the map of known feature names to feature specs. + 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. +type featureGate struct { + featureGateName string + + special map[Feature]func(map[Feature]FeatureSpec, map[Feature]bool, bool) + + // lock guards writes to known, enabled, and reads/writes of closed + lock sync.Mutex + // known holds a map[Feature]FeatureSpec + known atomic.Value + // enabled holds a map[Feature]bool + enabled atomic.Value + // closed is set to true when AddFlag is called, and prevents subsequent calls to Add + closed bool +} + +func setUnsetAlphaGates(known map[Feature]FeatureSpec, enabled map[Feature]bool, val bool) { + for k, v := range known { + if v.PreRelease == Alpha { + if _, found := enabled[k]; !found { + enabled[k] = val + } + } + } +} + +func setUnsetBetaGates(known map[Feature]FeatureSpec, enabled map[Feature]bool, val bool) { + for k, v := range known { + if v.PreRelease == Beta { + if _, found := enabled[k]; !found { + enabled[k] = val + } + } + } +} + +// Set, String, and Type implement pflag.Value +var _ pflag.Value = &featureGate{} + +// internalPackages are packages that ignored when creating a name for featureGates. These packages are in the common +// call chains, so they'd be unhelpful as names. +var internalPackages = []string{"k8s.io/component-base/featuregate/feature_gate.go"} + +func NewFeatureGate() *featureGate { + known := map[Feature]FeatureSpec{} + for k, v := range defaultFeatures { + known[k] = v + } + + f := &featureGate{ + featureGateName: naming.GetNameFromCallsite(internalPackages...), + special: specialFeatures, + } + f.known.Store(known) + f.enabled.Store(map[Feature]bool{}) + + return f +} + +// 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 { + m := 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]) + 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) + } + m[k] = boolValue + } + return f.SetFromMap(m) +} + +// SetFromMap stores flag gates for known features from a map[string]bool or returns an error +func (f *featureGate) SetFromMap(m map[string]bool) 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 + } + + for k, v := range m { + k := Feature(k) + featureSpec, ok := known[k] + if !ok { + return fmt.Errorf("unrecognized feature gate: %s", k) + } + if featureSpec.LockToDefault && featureSpec.Default != v { + return fmt.Errorf("cannot set feature gate %v to %v, feature is locked to %v", k, v, featureSpec.Default) + } + 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 { + klog.Warningf("Setting deprecated feature gate %s=%t. It will be removed in a future release.", k, v) + } else if featureSpec.PreRelease == GA { + klog.Warningf("Setting GA feature gate %s=%t. It will be removed in a future release.", k, v) + } + } + + // Persist changes + f.known.Store(known) + f.enabled.Store(enabled) + + klog.V(1).Infof("feature gates: %v", f.enabled) + return nil +} + +// String returns a string containing all enabled feature gates, formatted as "key1=value1,key2=value2,...". +func (f *featureGate) String() string { + pairs := []string{} + for k, v := range f.enabled.Load().(map[Feature]bool) { + pairs = append(pairs, fmt.Sprintf("%s=%t", k, v)) + } + sort.Strings(pairs) + return strings.Join(pairs, ",") +} + +func (f *featureGate) Type() string { + return "mapStringBool" +} + +// Add adds features to the featureGate. +func (f *featureGate) Add(features map[Feature]FeatureSpec) error { + f.lock.Lock() + defer f.lock.Unlock() + + if f.closed { + return fmt.Errorf("cannot add a feature gate after adding it to the flag set") + } + + // Copy existing state + known := map[Feature]FeatureSpec{} + for k, v := range f.known.Load().(map[Feature]FeatureSpec) { + known[k] = v + } + + for name, spec := range features { + if existingSpec, found := known[name]; found { + if existingSpec == spec { + continue + } + return fmt.Errorf("feature gate %q with different spec already exists: %v", name, existingSpec) + } + + known[name] = spec + } + + // Persist updated state + f.known.Store(known) + + 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{} + for k, v := range f.known.Load().(map[Feature]FeatureSpec) { + retval[k] = v + } + return retval +} + +// Enabled returns true if the key is enabled. If the key is not known, this call will panic. +func (f *featureGate) Enabled(key Feature) bool { + if v, ok := f.enabled.Load().(map[Feature]bool)[key]; ok { + return v + } + if v, ok := f.known.Load().(map[Feature]FeatureSpec)[key]; ok { + return v.Default + } + + panic(fmt.Errorf("feature %q is not registered in FeatureGate %q", key, f.featureGateName)) +} + +// AddFlag adds a flag for setting global feature gates to the specified FlagSet. +func (f *featureGate) AddFlag(fs *pflag.FlagSet) { + f.lock.Lock() + // TODO(mtaufen): Shouldn't we just close it on the first Set/SetFromMap instead? + // Not all components expose a feature gates flag using this AddFlag method, and + // in the future, all components will completely stop exposing a feature gates flag, + // in favor of componentconfig. + f.closed = true + f.lock.Unlock() + + known := f.KnownFeatures() + fs.Var(f, flagName, ""+ + "A set of key=value pairs that describe feature gates for alpha/experimental features. "+ + "Options are:\n"+strings.Join(known, "\n")) +} + +func (f *featureGate) AddMetrics() { + for feature, featureSpec := range f.GetAll() { + featuremetrics.RecordFeatureInfo(context.Background(), string(feature), string(featureSpec.PreRelease), f.Enabled(feature)) + } +} + +// 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) { + if v.PreRelease == GA || v.PreRelease == Deprecated { + continue + } + known = append(known, fmt.Sprintf("%s=true|false (%s - default=%t)", k, v.PreRelease, v.Default)) + } + sort.Strings(known) + return known +} + +// 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() MutableFeatureGate { + // 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 + } + + // Construct a new featureGate around the copied state. + // Note that specialFeatures is treated as immutable by convention, + // and we maintain the value of f.closed across the copy. + fg := &featureGate{ + special: specialFeatures, + closed: f.closed, + } + + fg.known.Store(known) + fg.enabled.Store(enabled) + + return fg +} diff --git a/pkg/featuregate/feature_gate_test.go b/pkg/featuregate/feature_gate_test.go new file mode 100644 index 00000000000..383bbb5a2f5 --- /dev/null +++ b/pkg/featuregate/feature_gate_test.go @@ -0,0 +1,612 @@ +// Copyright 2024 The etcd Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package featuregate + +import ( + "fmt" + "strings" + "testing" + + "github.com/spf13/pflag" + "github.com/stretchr/testify/assert" +) + +func TestFeatureGateFlag(t *testing.T) { + // gates for testing + const testAlphaGate Feature = "TestAlpha" + const testBetaGate Feature = "TestBeta" + + tests := []struct { + arg string + expect map[Feature]bool + parseError string + }{ + { + arg: "", + expect: map[Feature]bool{ + allAlphaGate: false, + allBetaGate: false, + testAlphaGate: false, + testBetaGate: false, + }, + }, + { + arg: "fooBarBaz=true", + expect: map[Feature]bool{ + allAlphaGate: false, + allBetaGate: false, + testAlphaGate: false, + testBetaGate: false, + }, + parseError: "unrecognized feature gate: fooBarBaz", + }, + { + arg: "AllAlpha=false", + expect: map[Feature]bool{ + allAlphaGate: false, + allBetaGate: false, + testAlphaGate: false, + testBetaGate: false, + }, + }, + { + arg: "AllAlpha=true", + expect: map[Feature]bool{ + allAlphaGate: true, + allBetaGate: false, + testAlphaGate: true, + testBetaGate: false, + }, + }, + { + arg: "AllAlpha=banana", + expect: map[Feature]bool{ + allAlphaGate: false, + allBetaGate: false, + testAlphaGate: false, + testBetaGate: false, + }, + parseError: "invalid value of AllAlpha", + }, + { + arg: "AllAlpha=false,TestAlpha=true", + expect: map[Feature]bool{ + allAlphaGate: false, + allBetaGate: false, + testAlphaGate: true, + testBetaGate: false, + }, + }, + { + arg: "TestAlpha=true,AllAlpha=false", + expect: map[Feature]bool{ + allAlphaGate: false, + allBetaGate: false, + testAlphaGate: true, + testBetaGate: false, + }, + }, + { + arg: "AllAlpha=true,TestAlpha=false", + expect: map[Feature]bool{ + allAlphaGate: true, + allBetaGate: false, + testAlphaGate: false, + testBetaGate: false, + }, + }, + { + arg: "TestAlpha=false,AllAlpha=true", + expect: map[Feature]bool{ + allAlphaGate: true, + allBetaGate: false, + testAlphaGate: false, + testBetaGate: false, + }, + }, + { + arg: "TestBeta=true,AllAlpha=false", + expect: map[Feature]bool{ + allAlphaGate: false, + allBetaGate: false, + testAlphaGate: false, + testBetaGate: true, + }, + }, + + { + arg: "AllBeta=false", + expect: map[Feature]bool{ + allAlphaGate: false, + allBetaGate: false, + testAlphaGate: false, + testBetaGate: false, + }, + }, + { + arg: "AllBeta=true", + expect: map[Feature]bool{ + allAlphaGate: false, + allBetaGate: true, + testAlphaGate: false, + testBetaGate: true, + }, + }, + { + arg: "AllBeta=banana", + expect: map[Feature]bool{ + allAlphaGate: false, + allBetaGate: false, + testAlphaGate: false, + testBetaGate: false, + }, + parseError: "invalid value of AllBeta", + }, + { + arg: "AllBeta=false,TestBeta=true", + expect: map[Feature]bool{ + allAlphaGate: false, + allBetaGate: false, + testAlphaGate: false, + testBetaGate: true, + }, + }, + { + arg: "TestBeta=true,AllBeta=false", + expect: map[Feature]bool{ + allAlphaGate: false, + allBetaGate: false, + testAlphaGate: false, + testBetaGate: true, + }, + }, + { + arg: "AllBeta=true,TestBeta=false", + expect: map[Feature]bool{ + allAlphaGate: false, + allBetaGate: true, + testAlphaGate: false, + testBetaGate: false, + }, + }, + { + arg: "TestBeta=false,AllBeta=true", + expect: map[Feature]bool{ + allAlphaGate: false, + allBetaGate: true, + testAlphaGate: false, + testBetaGate: false, + }, + }, + { + arg: "TestAlpha=true,AllBeta=false", + expect: map[Feature]bool{ + allAlphaGate: false, + allBetaGate: false, + testAlphaGate: true, + testBetaGate: false, + }, + }, + } + for i, test := range tests { + t.Run(test.arg, func(t *testing.T) { + fs := pflag.NewFlagSet("testfeaturegateflag", pflag.ContinueOnError) + f := NewFeatureGate() + f.Add(map[Feature]FeatureSpec{ + testAlphaGate: {Default: false, PreRelease: Alpha}, + testBetaGate: {Default: false, PreRelease: Beta}, + }) + f.AddFlag(fs) + + 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) + } + for k, v := range test.expect { + if actual := f.enabled.Load().(map[Feature]bool)[k]; actual != v { + t.Errorf("%d: expected %s=%v, Got %v", i, k, v, actual) + } + } + }) + } +} + +func TestFeatureGateOverride(t *testing.T) { + const testAlphaGate Feature = "TestAlpha" + const testBetaGate Feature = "TestBeta" + + // 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: false, PreRelease: Beta}, + }) + + f.Set("TestAlpha=true,TestBeta=true") + if f.Enabled(testAlphaGate) != true { + t.Errorf("Expected true") + } + if f.Enabled(testBetaGate) != true { + t.Errorf("Expected true") + } + + f.Set("TestAlpha=false") + if f.Enabled(testAlphaGate) != false { + t.Errorf("Expected false") + } + if f.Enabled(testBetaGate) != true { + t.Errorf("Expected true") + } +} + +func TestFeatureGateFlagDefaults(t *testing.T) { + // gates for testing + const testAlphaGate Feature = "TestAlpha" + const testBetaGate Feature = "TestBeta" + + // 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}, + }) + + if f.Enabled(testAlphaGate) != false { + t.Errorf("Expected false") + } + if f.Enabled(testBetaGate) != true { + t.Errorf("Expected true") + } +} + +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" + const testBetaGate Feature = "TestBeta" + const testLockedTrueGate Feature = "TestLockedTrue" + const testLockedFalseGate Feature = "TestLockedFalse" + + tests := []struct { + name string + setmap map[string]bool + expect map[Feature]bool + setmapError string + }{ + { + name: "set TestAlpha and TestBeta true", + setmap: map[string]bool{ + "TestAlpha": true, + "TestBeta": true, + }, + expect: map[Feature]bool{ + testAlphaGate: true, + testBetaGate: true, + }, + }, + { + name: "set TestBeta true", + setmap: map[string]bool{ + "TestBeta": true, + }, + expect: map[Feature]bool{ + testAlphaGate: false, + testBetaGate: true, + }, + }, + { + name: "set TestAlpha false", + setmap: map[string]bool{ + "TestAlpha": false, + }, + expect: map[Feature]bool{ + testAlphaGate: false, + testBetaGate: false, + }, + }, + { + name: "set TestInvaild true", + setmap: map[string]bool{ + "TestInvaild": true, + }, + expect: map[Feature]bool{ + testAlphaGate: false, + testBetaGate: false, + }, + setmapError: "unrecognized feature gate:", + }, + { + name: "set locked gates", + setmap: map[string]bool{ + "TestLockedTrue": true, + "TestLockedFalse": false, + }, + expect: map[Feature]bool{ + testAlphaGate: false, + testBetaGate: false, + }, + }, + { + name: "set locked gates", + setmap: map[string]bool{ + "TestLockedTrue": false, + }, + expect: map[Feature]bool{ + testAlphaGate: false, + testBetaGate: false, + }, + setmapError: "cannot set feature gate TestLockedTrue to false, feature is locked to true", + }, + { + name: "set locked gates", + setmap: map[string]bool{ + "TestLockedFalse": true, + }, + expect: map[Feature]bool{ + testAlphaGate: false, + testBetaGate: false, + }, + setmapError: "cannot set feature gate TestLockedFalse to true, feature is locked to false", + }, + } + for i, test := range tests { + t.Run(fmt.Sprintf("SetFromMap %s", test.name), func(t *testing.T) { + f := NewFeatureGate() + f.Add(map[Feature]FeatureSpec{ + testAlphaGate: {Default: false, PreRelease: Alpha}, + testBetaGate: {Default: false, PreRelease: Beta}, + testLockedTrueGate: {Default: true, PreRelease: GA, LockToDefault: true}, + testLockedFalseGate: {Default: false, PreRelease: GA, LockToDefault: true}, + }) + err := f.SetFromMap(test.setmap) + if test.setmapError != "" { + if err == nil { + t.Errorf("expected error, got none") + } else if !strings.Contains(err.Error(), test.setmapError) { + t.Errorf("%d: SetFromMap(%#v) Expected err:%v, Got err:%v", i, test.setmap, test.setmapError, err) + } + } else if err != nil { + t.Errorf("%d: SetFromMap(%#v) Expected success, Got err:%v", i, test.setmap, err) + } + for k, v := range test.expect { + if actual := f.Enabled(k); actual != v { + t.Errorf("%d: SetFromMap(%#v) Expected %s=%v, Got %s=%v", i, test.setmap, k, v, k, actual) + } + } + }) + } +} + +func TestFeatureGateMetrics(t *testing.T) { + // TODO(henrybear327): Add tests once feature gate metrics are added. +} + +func TestFeatureGateString(t *testing.T) { + // gates for testing + const testAlphaGate Feature = "TestAlpha" + const testBetaGate Feature = "TestBeta" + const testGAGate Feature = "TestGA" + + featuremap := map[Feature]FeatureSpec{ + testGAGate: {Default: true, PreRelease: GA}, + testAlphaGate: {Default: false, PreRelease: Alpha}, + testBetaGate: {Default: true, PreRelease: Beta}, + } + + tests := []struct { + setmap map[string]bool + expect string + }{ + { + setmap: map[string]bool{ + "TestAlpha": false, + }, + expect: "TestAlpha=false", + }, + { + setmap: map[string]bool{ + "TestAlpha": false, + "TestBeta": true, + }, + expect: "TestAlpha=false,TestBeta=true", + }, + { + setmap: map[string]bool{ + "TestGA": true, + "TestAlpha": false, + "TestBeta": true, + }, + expect: "TestAlpha=false,TestBeta=true,TestGA=true", + }, + } + for i, test := range tests { + t.Run(fmt.Sprintf("SetFromMap %s", test.expect), func(t *testing.T) { + f := NewFeatureGate() + f.Add(featuremap) + f.SetFromMap(test.setmap) + result := f.String() + if result != test.expect { + t.Errorf("%d: SetFromMap(%#v) Expected %s, Got %s", i, test.setmap, test.expect, result) + } + }) + } +} + +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") + } + }) +} diff --git a/server/config/config.go b/server/config/config.go index bc767d4ca40..f1cd47bffa5 100644 --- a/server/config/config.go +++ b/server/config/config.go @@ -28,9 +28,9 @@ import ( bolt "go.etcd.io/bbolt" "go.etcd.io/etcd/client/pkg/v3/transport" "go.etcd.io/etcd/client/pkg/v3/types" + "go.etcd.io/etcd/pkg/v3/featuregate" "go.etcd.io/etcd/pkg/v3/netutil" "go.etcd.io/etcd/server/v3/etcdserver/api/v3discovery" - "go.etcd.io/etcd/server/v3/internal/pkg/featuregate" "go.etcd.io/etcd/server/v3/storage/datadir" ) diff --git a/server/etcdserver/server.go b/server/etcdserver/server.go index 2e849a2ebd3..a6f25548ca1 100644 --- a/server/etcdserver/server.go +++ b/server/etcdserver/server.go @@ -39,6 +39,7 @@ import ( "go.etcd.io/etcd/client/pkg/v3/fileutil" "go.etcd.io/etcd/client/pkg/v3/types" "go.etcd.io/etcd/client/pkg/v3/verify" + "go.etcd.io/etcd/pkg/v3/featuregate" "go.etcd.io/etcd/pkg/v3/idutil" "go.etcd.io/etcd/pkg/v3/notify" "go.etcd.io/etcd/pkg/v3/pbutil" @@ -62,7 +63,6 @@ import ( "go.etcd.io/etcd/server/v3/etcdserver/errors" "go.etcd.io/etcd/server/v3/etcdserver/txn" serverversion "go.etcd.io/etcd/server/v3/etcdserver/version" - "go.etcd.io/etcd/server/v3/internal/pkg/featuregate" "go.etcd.io/etcd/server/v3/lease" "go.etcd.io/etcd/server/v3/lease/leasehttp" serverstorage "go.etcd.io/etcd/server/v3/storage" diff --git a/server/internal/pkg/featuregate/doc.go b/server/internal/pkg/featuregate/doc.go deleted file mode 100644 index 64d74ae51ed..00000000000 --- a/server/internal/pkg/featuregate/doc.go +++ /dev/null @@ -1,17 +0,0 @@ -// Copyright 2024 The etcd Authors -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -// featuregate package is copied from k8s.io/component-base@v0.30.1 to avoid any potential circular dependency between k8s and etcd. -// Note that only necessary portions are copied to support etcd feature gate functionalities.s -package featuregate diff --git a/server/internal/pkg/featuregate/feature_gate.go b/server/internal/pkg/featuregate/feature_gate.go deleted file mode 100644 index a60c08c465b..00000000000 --- a/server/internal/pkg/featuregate/feature_gate.go +++ /dev/null @@ -1,35 +0,0 @@ -// Copyright 2024 The etcd Authors -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package featuregate - -type Feature string - -// 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 -} From 14e15bcaca28c8b7aa58e74e1964f7f3646da182 Mon Sep 17 00:00:00 2001 From: Baek Date: Fri, 14 Jun 2024 06:22:55 +0000 Subject: [PATCH 3/3] Removes dependency on other k8s packages in featuregate pkg. The removed packages are: * k8s.io/apimachinery/pkg/util/naming * k8s.io/klog/v2 Do note that removing naming package necessitates adding feature gate name argument to featuregate.New(). Signed-off-by: Baek --- go.mod | 4 ++-- go.sum | 4 ++-- pkg/featuregate/feature_gate.go | 32 +++++++++++----------------- pkg/featuregate/feature_gate_test.go | 29 +++++++++++++------------ 4 files changed, 32 insertions(+), 37 deletions(-) diff --git a/go.mod b/go.mod index 878ac5e1d85..d5bd3cb7ebe 100644 --- a/go.mod +++ b/go.mod @@ -1,6 +1,6 @@ module go.etcd.io/etcd/v3 -go 1.22.0 +go 1.22 toolchain go1.22.4 @@ -95,6 +95,6 @@ require ( google.golang.org/genproto/googleapis/rpc v0.0.0-20240520151616-dc85e6b867a5 // indirect gopkg.in/natefinch/lumberjack.v2 v2.2.1 // indirect gopkg.in/yaml.v3 v3.0.1 // indirect - sigs.k8s.io/json v0.0.0-20221116044647-bc3834ca7abd // indirect + sigs.k8s.io/json v0.0.0-20211020170558-c049b76a60c6 // indirect sigs.k8s.io/yaml v1.4.0 // indirect ) diff --git a/go.sum b/go.sum index bbdd90c9679..b4f42c86d11 100644 --- a/go.sum +++ b/go.sum @@ -250,7 +250,7 @@ gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA= gopkg.in/yaml.v3 v3.0.1/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= honnef.co/go/tools v0.0.0-20190102054323-c2f93a96b099/go.mod h1:rf3lG4BRIbNafJWhAfAdb/ePZxsR/4RtNHQocxwk9r4= honnef.co/go/tools v0.0.0-20190523083050-ea95bdfd59fc/go.mod h1:rf3lG4BRIbNafJWhAfAdb/ePZxsR/4RtNHQocxwk9r4= -sigs.k8s.io/json v0.0.0-20221116044647-bc3834ca7abd h1:EDPBXCAspyGV4jQlpZSudPeMmr1bNJefnuqLsRAsHZo= -sigs.k8s.io/json v0.0.0-20221116044647-bc3834ca7abd/go.mod h1:B8JuhiUyNFVKdsE8h686QcCxMaH6HrOAZj4vswFpcB0= +sigs.k8s.io/json v0.0.0-20211020170558-c049b76a60c6 h1:fD1pz4yfdADVNfFmcP2aBEtudwUQ1AlLnRBALr33v3s= +sigs.k8s.io/json v0.0.0-20211020170558-c049b76a60c6/go.mod h1:p4QtZmO4uMYipTQNzagwnNoseA6OxSUutVw05NhYDRs= sigs.k8s.io/yaml v1.4.0 h1:Mk1wCc2gy/F0THH0TAp1QYyJNzRm2KCLy3o5ASXVI5E= sigs.k8s.io/yaml v1.4.0/go.mod h1:Ejl7/uTz7PSA4eKMyQCUTnhZYNmLIl+5c2lQPGR2BPY= diff --git a/pkg/featuregate/feature_gate.go b/pkg/featuregate/feature_gate.go index 1c6f8f88704..60c2ab114bb 100644 --- a/pkg/featuregate/feature_gate.go +++ b/pkg/featuregate/feature_gate.go @@ -12,11 +12,10 @@ // See the License for the specific language governing permissions and // limitations under the License. -// featuregate package is copied from k8s.io/component-base@v0.30.1 to avoid any potential circular dependency between k8s and etcd. +// Package featuregate is copied from k8s.io/component-base@v0.30.1 to avoid any potential circular dependency between k8s and etcd. package featuregate import ( - "context" "fmt" "sort" "strconv" @@ -25,9 +24,7 @@ import ( "sync/atomic" "github.com/spf13/pflag" - - "k8s.io/apimachinery/pkg/util/naming" - "k8s.io/klog/v2" + "go.uber.org/zap" ) type Feature string @@ -128,6 +125,8 @@ type MutableFeatureGate interface { // featureGate implements FeatureGate as well as pflag.Value for flag parsing. type featureGate struct { + lg *zap.Logger + featureGateName string special map[Feature]func(map[Feature]FeatureSpec, map[Feature]bool, bool) @@ -165,18 +164,15 @@ func setUnsetBetaGates(known map[Feature]FeatureSpec, enabled map[Feature]bool, // Set, String, and Type implement pflag.Value var _ pflag.Value = &featureGate{} -// internalPackages are packages that ignored when creating a name for featureGates. These packages are in the common -// call chains, so they'd be unhelpful as names. -var internalPackages = []string{"k8s.io/component-base/featuregate/feature_gate.go"} - -func NewFeatureGate() *featureGate { +func New(name string, lg *zap.Logger) *featureGate { known := map[Feature]FeatureSpec{} for k, v := range defaultFeatures { known[k] = v } f := &featureGate{ - featureGateName: naming.GetNameFromCallsite(internalPackages...), + lg: lg, + featureGateName: name, special: specialFeatures, } f.known.Store(known) @@ -239,9 +235,9 @@ func (f *featureGate) SetFromMap(m map[string]bool) error { } if featureSpec.PreRelease == Deprecated { - klog.Warningf("Setting deprecated feature gate %s=%t. It will be removed in a future release.", k, v) + f.lg.Warn(fmt.Sprintf("Setting deprecated feature gate %s=%t. It will be removed in a future release.", k, v)) } else if featureSpec.PreRelease == GA { - klog.Warningf("Setting GA feature gate %s=%t. It will be removed in a future release.", k, v) + f.lg.Warn(fmt.Sprintf("Setting GA feature gate %s=%t. It will be removed in a future release.", k, v)) } } @@ -249,7 +245,7 @@ func (f *featureGate) SetFromMap(m map[string]bool) error { f.known.Store(known) f.enabled.Store(enabled) - klog.V(1).Infof("feature gates: %v", f.enabled) + f.lg.Info(fmt.Sprintf("feature gates: %v", f.enabled)) return nil } @@ -319,9 +315,9 @@ func (f *featureGate) OverrideDefault(name Feature, override bool) error { 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) + f.lg.Warn(fmt.Sprintf("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) + f.lg.Warn(fmt.Sprintf("Overriding default of GA feature gate %s=%t. It will be removed in a future release.", name, override)) } spec.Default = override @@ -369,9 +365,7 @@ func (f *featureGate) AddFlag(fs *pflag.FlagSet) { } func (f *featureGate) AddMetrics() { - for feature, featureSpec := range f.GetAll() { - featuremetrics.RecordFeatureInfo(context.Background(), string(feature), string(featureSpec.PreRelease), f.Enabled(feature)) - } + // TODO(henrybear327): implement this. } // KnownFeatures returns a slice of strings describing the FeatureGate's known features. diff --git a/pkg/featuregate/feature_gate_test.go b/pkg/featuregate/feature_gate_test.go index 383bbb5a2f5..a5bdcf8bd4c 100644 --- a/pkg/featuregate/feature_gate_test.go +++ b/pkg/featuregate/feature_gate_test.go @@ -21,6 +21,7 @@ import ( "github.com/spf13/pflag" "github.com/stretchr/testify/assert" + "go.uber.org/zap/zaptest" ) func TestFeatureGateFlag(t *testing.T) { @@ -203,7 +204,7 @@ func TestFeatureGateFlag(t *testing.T) { for i, test := range tests { t.Run(test.arg, func(t *testing.T) { fs := pflag.NewFlagSet("testfeaturegateflag", pflag.ContinueOnError) - f := NewFeatureGate() + f := New("test", zaptest.NewLogger(t)) f.Add(map[Feature]FeatureSpec{ testAlphaGate: {Default: false, PreRelease: Alpha}, testBetaGate: {Default: false, PreRelease: Beta}, @@ -232,7 +233,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 = New("test", zaptest.NewLogger(t)) f.Add(map[Feature]FeatureSpec{ testAlphaGate: {Default: false, PreRelease: Alpha}, testBetaGate: {Default: false, PreRelease: Beta}, @@ -261,7 +262,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 = New("test", zaptest.NewLogger(t)) f.Add(map[Feature]FeatureSpec{ testAlphaGate: {Default: false, PreRelease: Alpha}, testBetaGate: {Default: true, PreRelease: Beta}, @@ -285,7 +286,7 @@ func TestFeatureGateKnownFeatures(t *testing.T) { ) // Don't parse the flag, assert defaults are used. - var f *featureGate = NewFeatureGate() + var f = New("test", zaptest.NewLogger(t)) f.Add(map[Feature]FeatureSpec{ testAlphaGate: {Default: false, PreRelease: Alpha}, testBetaGate: {Default: true, PreRelease: Beta}, @@ -392,7 +393,7 @@ func TestFeatureGateSetFromMap(t *testing.T) { } for i, test := range tests { t.Run(fmt.Sprintf("SetFromMap %s", test.name), func(t *testing.T) { - f := NewFeatureGate() + f := New("test", zaptest.NewLogger(t)) f.Add(map[Feature]FeatureSpec{ testAlphaGate: {Default: false, PreRelease: Alpha}, testBetaGate: {Default: false, PreRelease: Beta}, @@ -462,7 +463,7 @@ func TestFeatureGateString(t *testing.T) { } for i, test := range tests { t.Run(fmt.Sprintf("SetFromMap %s", test.expect), func(t *testing.T) { - f := NewFeatureGate() + f := New("test", zaptest.NewLogger(t)) f.Add(featuremap) f.SetFromMap(test.setmap) result := f.String() @@ -475,7 +476,7 @@ func TestFeatureGateString(t *testing.T) { func TestFeatureGateOverrideDefault(t *testing.T) { t.Run("overrides take effect", func(t *testing.T) { - f := NewFeatureGate() + f := New("test", zaptest.NewLogger(t)) if err := f.Add(map[Feature]FeatureSpec{ "TestFeature1": {Default: true}, "TestFeature2": {Default: false}, @@ -497,7 +498,7 @@ func TestFeatureGateOverrideDefault(t *testing.T) { }) t.Run("overrides are preserved across deep copies", func(t *testing.T) { - f := NewFeatureGate() + f := New("test", zaptest.NewLogger(t)) if err := f.Add(map[Feature]FeatureSpec{"TestFeature": {Default: false}}); err != nil { t.Fatal(err) } @@ -511,7 +512,7 @@ func TestFeatureGateOverrideDefault(t *testing.T) { }) t.Run("reflected in known features", func(t *testing.T) { - f := NewFeatureGate() + f := New("test", zaptest.NewLogger(t)) if err := f.Add(map[Feature]FeatureSpec{"TestFeature": { Default: false, PreRelease: Alpha, @@ -537,7 +538,7 @@ func TestFeatureGateOverrideDefault(t *testing.T) { }) t.Run("may not change default for specs with locked defaults", func(t *testing.T) { - f := NewFeatureGate() + f := New("test", zaptest.NewLogger(t)) if err := f.Add(map[Feature]FeatureSpec{ "LockedFeature": { Default: true, @@ -555,7 +556,7 @@ func TestFeatureGateOverrideDefault(t *testing.T) { }) t.Run("does not supersede explicitly-set value", func(t *testing.T) { - f := NewFeatureGate() + f := New("test", zaptest.NewLogger(t)) if err := f.Add(map[Feature]FeatureSpec{"TestFeature": {Default: true}}); err != nil { t.Fatal(err) } @@ -571,7 +572,7 @@ func TestFeatureGateOverrideDefault(t *testing.T) { }) t.Run("prevents re-registration of feature spec after overriding default", func(t *testing.T) { - f := NewFeatureGate() + f := New("test", zaptest.NewLogger(t)) if err := f.Add(map[Feature]FeatureSpec{ "TestFeature": { Default: true, @@ -594,14 +595,14 @@ func TestFeatureGateOverrideDefault(t *testing.T) { }) t.Run("does not allow override for an unknown feature", func(t *testing.T) { - f := NewFeatureGate() + f := New("test", zaptest.NewLogger(t)) 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() + f := New("test", zaptest.NewLogger(t)) fs := pflag.NewFlagSet("test", pflag.ContinueOnError) f.AddFlag(fs)