Skip to content

Commit 8991e82

Browse files
k8s-ci-robotk8s-ci-robot
authored
Merge pull request kubernetes#125052 from p0lyn0mial/upstream-client-go-env-var-set
client-go/features: add Set method to the envvar impl
2 parents f4ea903 + a07654b commit 8991e82

File tree

3 files changed

+172
-36
lines changed

3 files changed

+172
-36
lines changed

‎staging/src/k8s.io/client-go/features/envvar.go‎

Lines changed: 56 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,10 @@ var _ Gates = &envVarFeatureGates{}
4747
//
4848
// Please note that environmental variables can only be set to the boolean value.
4949
// Incorrect values will be ignored and logged.
50+
//
51+
// Features can also be set directly via the Set method.
52+
// In that case, these features take precedence over
53+
// features set via environmental variables.
5054
func newEnvVarFeatureGates(features map[Feature]FeatureSpec) *envVarFeatureGates {
5155
known := map[Feature]FeatureSpec{}
5256
for name, spec := range features {
@@ -57,7 +61,8 @@ func newEnvVarFeatureGates(features map[Feature]FeatureSpec) *envVarFeatureGates
5761
callSiteName: naming.GetNameFromCallsite(internalPackages...),
5862
known: known,
5963
}
60-
fg.enabled.Store(map[Feature]bool{})
64+
fg.enabledViaEnvVar.Store(map[Feature]bool{})
65+
fg.enabledViaSetMethod = map[Feature]bool{}
6166

6267
return fg
6368
}
@@ -74,17 +79,34 @@ type envVarFeatureGates struct {
7479
// known holds known feature gates
7580
known map[Feature]FeatureSpec
7681

77-
// enabled holds a map[Feature]bool
82+
// enabledViaEnvVar holds a map[Feature]bool
7883
// with values explicitly set via env var
79-
enabled atomic.Value
84+
enabledViaEnvVar atomic.Value
85+
86+
// lockEnabledViaSetMethod protects enabledViaSetMethod
87+
lockEnabledViaSetMethod sync.RWMutex
88+
89+
// enabledViaSetMethod holds values explicitly set
90+
// via Set method, features stored in this map take
91+
// precedence over features stored in enabledViaEnvVar
92+
enabledViaSetMethod map[Feature]bool
8093

8194
// readEnvVars holds the boolean value which
8295
// indicates whether readEnvVarsOnce has been called.
8396
readEnvVars atomic.Bool
8497
}
8598

86-
// Enabled returns true if the key is enabled. If the key is not known, this call will panic.
99+
// Enabled returns true if the key is enabled. If the key is not known, this call will panic.
87100
func (f *envVarFeatureGates) Enabled(key Feature) bool {
101+
if v, ok := f.wasFeatureEnabledViaSetMethod(key); ok {
102+
// ensue that the state of all known features
103+
// is loaded from environment variables
104+
// on the first call to Enabled method.
105+
if !f.hasAlreadyReadEnvVar() {
106+
_ = f.getEnabledMapFromEnvVar()
107+
}
108+
return v
109+
}
88110
if v, ok := f.getEnabledMapFromEnvVar()[key]; ok {
89111
return v
90112
}
@@ -94,6 +116,26 @@ func (f *envVarFeatureGates) Enabled(key Feature) bool {
94116
panic(fmt.Errorf("feature %q is not registered in FeatureGates %q", key, f.callSiteName))
95117
}
96118

119+
// Set sets the given feature to the given value.
120+
//
121+
// Features set via this method take precedence over
122+
// the features set via environment variables.
123+
func (f *envVarFeatureGates) Set(featureName Feature, featureValue bool) error {
124+
feature, ok := f.known[featureName]
125+
if !ok {
126+
return fmt.Errorf("feature %q is not registered in FeatureGates %q", featureName, f.callSiteName)
127+
}
128+
if feature.LockToDefault && feature.Default != featureValue {
129+
return fmt.Errorf("cannot set feature gate %q to %v, feature is locked to %v", featureName, featureValue, feature.Default)
130+
}
131+
132+
f.lockEnabledViaSetMethod.Lock()
133+
defer f.lockEnabledViaSetMethod.Unlock()
134+
f.enabledViaSetMethod[featureName] = featureValue
135+
136+
return nil
137+
}
138+
97139
// getEnabledMapFromEnvVar will fill the enabled map on the first call.
98140
// This is the only time a known feature can be set to a value
99141
// read from the corresponding environmental variable.
@@ -119,7 +161,7 @@ func (f *envVarFeatureGates) getEnabledMapFromEnvVar() map[Feature]bool {
119161
featureGatesState[feature] = boolVal
120162
}
121163
}
122-
f.enabled.Store(featureGatesState)
164+
f.enabledViaEnvVar.Store(featureGatesState)
123165
f.readEnvVars.Store(true)
124166

125167
for feature, featureSpec := range f.known {
@@ -130,7 +172,15 @@ func (f *envVarFeatureGates) getEnabledMapFromEnvVar() map[Feature]bool {
130172
klog.V(1).InfoS("Feature gate default state", "feature", feature, "enabled", featureSpec.Default)
131173
}
132174
})
133-
return f.enabled.Load().(map[Feature]bool)
175+
return f.enabledViaEnvVar.Load().(map[Feature]bool)
176+
}
177+
178+
func (f *envVarFeatureGates) wasFeatureEnabledViaSetMethod(key Feature) (bool, bool) {
179+
f.lockEnabledViaSetMethod.RLock()
180+
defer f.lockEnabledViaSetMethod.RUnlock()
181+
182+
value, found := f.enabledViaSetMethod[key]
183+
return value, found
134184
}
135185

136186
func (f *envVarFeatureGates) hasAlreadyReadEnvVar() bool {

‎staging/src/k8s.io/client-go/features/envvar_test.go‎

Lines changed: 107 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -23,21 +23,21 @@ import (
2323
"github.com/stretchr/testify/require"
2424
)
2525

26+
var defaultTestFeatures = map[Feature]FeatureSpec{
27+
"TestAlpha": {
28+
Default: false,
29+
LockToDefault: false,
30+
PreRelease: "Alpha",
31+
},
32+
"TestBeta": {
33+
Default: true,
34+
LockToDefault: false,
35+
PreRelease: "Beta",
36+
},
37+
}
38+
2639
func TestEnvVarFeatureGates(t *testing.T) {
27-
defaultTestFeatures := map[Feature]FeatureSpec{
28-
"TestAlpha": {
29-
Default: false,
30-
LockToDefault: false,
31-
PreRelease: "Alpha",
32-
},
33-
"TestBeta": {
34-
Default: true,
35-
LockToDefault: false,
36-
PreRelease: "Beta",
37-
},
38-
}
3940
expectedDefaultFeaturesState := map[Feature]bool{"TestAlpha": false, "TestBeta": true}
40-
4141
copyExpectedStateMap := func(toCopy map[Feature]bool) map[Feature]bool {
4242
m := map[Feature]bool{}
4343
for k, v := range toCopy {
@@ -47,11 +47,14 @@ func TestEnvVarFeatureGates(t *testing.T) {
4747
}
4848

4949
scenarios := []struct {
50-
name string
51-
features map[Feature]FeatureSpec
52-
envVariables map[string]string
53-
expectedFeaturesState map[Feature]bool
54-
expectedInternalEnabledFeatureState map[Feature]bool
50+
name string
51+
features map[Feature]FeatureSpec
52+
envVariables map[string]string
53+
setMethodFeatures map[Feature]bool
54+
55+
expectedFeaturesState map[Feature]bool
56+
expectedInternalEnabledViaEnvVarFeatureState map[Feature]bool
57+
expectedInternalEnabledViaSetMethodFeatureState map[Feature]bool
5558
}{
5659
{
5760
name: "can add empty features",
@@ -76,7 +79,7 @@ func TestEnvVarFeatureGates(t *testing.T) {
7679
expectedDefaultFeaturesStateCopy["TestAlpha"] = true
7780
return expectedDefaultFeaturesStateCopy
7881
}(),
79-
expectedInternalEnabledFeatureState: map[Feature]bool{"TestAlpha": true},
82+
expectedInternalEnabledViaEnvVarFeatureState: map[Feature]bool{"TestAlpha": true},
8083
},
8184
{
8285
name: "incorrect env var value gets ignored",
@@ -111,9 +114,25 @@ func TestEnvVarFeatureGates(t *testing.T) {
111114
PreRelease: "Alpha",
112115
},
113116
},
114-
envVariables: map[string]string{"KUBE_FEATURE_TestAlpha": "True"},
115-
expectedFeaturesState: map[Feature]bool{"TestAlpha": true},
116-
expectedInternalEnabledFeatureState: map[Feature]bool{"TestAlpha": true},
117+
envVariables: map[string]string{"KUBE_FEATURE_TestAlpha": "True"},
118+
expectedFeaturesState: map[Feature]bool{"TestAlpha": true},
119+
expectedInternalEnabledViaEnvVarFeatureState: map[Feature]bool{"TestAlpha": true},
120+
},
121+
{
122+
name: "setting a feature via the Set method works",
123+
features: defaultTestFeatures,
124+
setMethodFeatures: map[Feature]bool{"TestAlpha": true},
125+
expectedFeaturesState: map[Feature]bool{"TestAlpha": true},
126+
expectedInternalEnabledViaSetMethodFeatureState: map[Feature]bool{"TestAlpha": true},
127+
},
128+
{
129+
name: "setting a feature via the Set method wins",
130+
features: defaultTestFeatures,
131+
setMethodFeatures: map[Feature]bool{"TestAlpha": false},
132+
envVariables: map[string]string{"KUBE_FEATURE_TestAlpha": "True"},
133+
expectedFeaturesState: map[Feature]bool{"TestAlpha": false},
134+
expectedInternalEnabledViaEnvVarFeatureState: map[Feature]bool{"TestAlpha": true},
135+
expectedInternalEnabledViaSetMethodFeatureState: map[Feature]bool{"TestAlpha": false},
117136
},
118137
}
119138
for _, scenario := range scenarios {
@@ -123,20 +142,33 @@ func TestEnvVarFeatureGates(t *testing.T) {
123142
}
124143
target := newEnvVarFeatureGates(scenario.features)
125144

145+
for k, v := range scenario.setMethodFeatures {
146+
err := target.Set(k, v)
147+
require.NoError(t, err)
148+
}
126149
for expectedFeature, expectedValue := range scenario.expectedFeaturesState {
127150
actualValue := target.Enabled(expectedFeature)
128151
require.Equal(t, actualValue, expectedValue, "expected feature=%v, to be=%v, not=%v", expectedFeature, expectedValue, actualValue)
129152
}
130153

131-
enabledInternalMap := target.enabled.Load().(map[Feature]bool)
132-
require.Len(t, enabledInternalMap, len(scenario.expectedInternalEnabledFeatureState))
133-
134-
for expectedFeature, expectedInternalPresence := range scenario.expectedInternalEnabledFeatureState {
135-
featureInternalValue, featureSet := enabledInternalMap[expectedFeature]
136-
require.Equal(t, expectedInternalPresence, featureSet, "feature %v present = %v, expected = %v", expectedFeature, featureSet, expectedInternalPresence)
154+
enabledViaEnvVarInternalMap := target.enabledViaEnvVar.Load().(map[Feature]bool)
155+
require.Len(t, enabledViaEnvVarInternalMap, len(scenario.expectedInternalEnabledViaEnvVarFeatureState))
156+
for expectedFeatureName, expectedFeatureValue := range scenario.expectedInternalEnabledViaEnvVarFeatureState {
157+
actualFeatureValue, wasExpectedFeatureFound := enabledViaEnvVarInternalMap[expectedFeatureName]
158+
if !wasExpectedFeatureFound {
159+
t.Errorf("feature %v has not been found in enabledViaEnvVarInternalMap", expectedFeatureName)
160+
}
161+
require.Equal(t, expectedFeatureValue, actualFeatureValue, "feature %v has incorrect value = %v, expected = %v", expectedFeatureName, actualFeatureValue, expectedFeatureValue)
162+
}
137163

138-
expectedFeatureInternalValue := scenario.expectedFeaturesState[expectedFeature]
139-
require.Equal(t, expectedFeatureInternalValue, featureInternalValue)
164+
enabledViaSetMethodInternalMap := target.enabledViaSetMethod
165+
require.Len(t, enabledViaSetMethodInternalMap, len(scenario.expectedInternalEnabledViaSetMethodFeatureState))
166+
for expectedFeatureName, expectedFeatureValue := range scenario.expectedInternalEnabledViaSetMethodFeatureState {
167+
actualFeatureValue, wasExpectedFeatureFound := enabledViaSetMethodInternalMap[expectedFeatureName]
168+
if !wasExpectedFeatureFound {
169+
t.Errorf("feature %v has not been found in enabledViaSetMethod", expectedFeatureName)
170+
}
171+
require.Equal(t, expectedFeatureValue, actualFeatureValue, "feature %v has incorrect value = %v, expected = %v", expectedFeatureName, actualFeatureValue, expectedFeatureValue)
140172
}
141173
})
142174
}
@@ -154,3 +186,48 @@ func TestHasAlreadyReadEnvVar(t *testing.T) {
154186
_ = target.getEnabledMapFromEnvVar()
155187
require.True(t, target.hasAlreadyReadEnvVar())
156188
}
189+
190+
func TestEnvVarFeatureGatesSetNegative(t *testing.T) {
191+
scenarios := []struct {
192+
name string
193+
features map[Feature]FeatureSpec
194+
featureName Feature
195+
featureValue bool
196+
197+
expectedErr func(string) error
198+
}{
199+
{
200+
name: "empty feature name returns an error",
201+
features: defaultTestFeatures,
202+
expectedErr: func(callSiteName string) error {
203+
return fmt.Errorf("feature %q is not registered in FeatureGates %q", "", callSiteName)
204+
},
205+
},
206+
{
207+
name: "setting unknown feature returns an error",
208+
features: defaultTestFeatures,
209+
featureName: "Unknown",
210+
expectedErr: func(callSiteName string) error {
211+
return fmt.Errorf("feature %q is not registered in FeatureGates %q", "Unknown", callSiteName)
212+
},
213+
},
214+
{
215+
name: "setting locked feature returns an error",
216+
features: map[Feature]FeatureSpec{"LockedFeature": {LockToDefault: true, Default: true}},
217+
featureName: "LockedFeature",
218+
featureValue: false,
219+
expectedErr: func(_ string) error {
220+
return fmt.Errorf("cannot set feature gate %q to %v, feature is locked to %v", "LockedFeature", false, true)
221+
},
222+
},
223+
}
224+
225+
for _, scenario := range scenarios {
226+
t.Run(scenario.name, func(t *testing.T) {
227+
target := newEnvVarFeatureGates(scenario.features)
228+
229+
err := target.Set(scenario.featureName, scenario.featureValue)
230+
require.Equal(t, scenario.expectedErr(target.callSiteName), err)
231+
})
232+
}
233+
}

‎staging/src/k8s.io/client-go/features/features_test.go‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,15 @@ func TestAddFeaturesToExistingFeatureGates(t *testing.T) {
3030
require.Equal(t, defaultKubernetesFeatureGates, fakeFeatureGates.specs)
3131
}
3232

33+
func TestReplaceFeatureGatesWithWarningIndicator(t *testing.T) {
34+
defaultFeatureGates := FeatureGates()
35+
require.Panics(t, func() { defaultFeatureGates.Enabled("Foo") }, "reading an unregistered feature gate Foo should panic")
36+
37+
if !replaceFeatureGatesWithWarningIndicator(defaultFeatureGates) {
38+
t.Error("replacing the default feature gates after reading a value hasn't produced a warning")
39+
}
40+
}
41+
3342
type fakeRegistry struct {
3443
specs map[Feature]FeatureSpec
3544
}

0 commit comments

Comments
 (0)