Add Validation to versioned feature specs.

Co-authored-by: Jordan Liggitt <liggitt@google.com>
Co-authored-by: Siyuan Zhang <sizhang@google.com>

Signed-off-by: Siyuan Zhang <sizhang@google.com>
This commit is contained in:
Siyuan Zhang 2024-12-03 18:48:07 +00:00
parent 107be8fecc
commit 00dab9dffa
10 changed files with 261 additions and 32 deletions

View File

@ -747,8 +747,8 @@ func TestEmulatedVersion(t *testing.T) {
fg := featuregate.NewVersionedFeatureGate(version.MustParse("1.32"))
utilruntime.Must(fg.AddVersioned(map[featuregate.Feature]featuregate.VersionedSpecs{
"kubeA": {
{Version: version.MustParse("1.32"), Default: true, LockToDefault: true, PreRelease: featuregate.GA},
{Version: version.MustParse("1.30"), Default: false, PreRelease: featuregate.Beta},
{Version: version.MustParse("1.32"), Default: true, LockToDefault: true, PreRelease: featuregate.GA},
},
"kubeB": {
{Version: version.MustParse("1.31"), Default: false, PreRelease: featuregate.Alpha},

View File

@ -29,6 +29,7 @@ import (
"github.com/google/go-cmp/cmp"
"github.com/spf13/pflag"
v1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
@ -446,8 +447,8 @@ leaderElection:
fg := feature.DefaultFeatureGate.DeepCopy()
utilruntime.Must(fg.AddVersioned(map[featuregate.Feature]featuregate.VersionedSpecs{
"kubeA": {
{Version: version.MustParse("1.32"), Default: true, LockToDefault: true, PreRelease: featuregate.GA},
{Version: version.MustParse("1.30"), Default: false, PreRelease: featuregate.Beta},
{Version: version.MustParse("1.32"), Default: true, LockToDefault: true, PreRelease: featuregate.GA},
},
"kubeB": {
{Version: version.MustParse("1.31"), Default: false, PreRelease: featuregate.Alpha},

View File

@ -37,18 +37,22 @@ import (
// Entries are alphabetized.
var defaultVersionedKubernetesFeatureGates = map[featuregate.Feature]featuregate.VersionedSpecs{
AllowDNSOnlyNodeCSR: {
{Version: version.MustParse("1.0"), Default: true, PreRelease: featuregate.GA},
{Version: version.MustParse("1.31"), Default: false, PreRelease: featuregate.Deprecated},
},
AllowInsecureKubeletCertificateSigningRequests: {
{Version: version.MustParse("1.0"), Default: true, PreRelease: featuregate.GA},
{Version: version.MustParse("1.31"), Default: false, PreRelease: featuregate.Deprecated},
},
AllowOverwriteTerminationGracePeriodSeconds: {
{Version: version.MustParse("1.0"), Default: true, PreRelease: featuregate.GA},
{Version: version.MustParse("1.32"), Default: false, PreRelease: featuregate.Deprecated},
},
AllowServiceLBStatusOnNonLB: {
{Version: version.MustParse("1.0"), Default: true, PreRelease: featuregate.GA},
{Version: version.MustParse("1.29"), Default: false, PreRelease: featuregate.Deprecated},
{Version: version.MustParse("1.32"), Default: false, PreRelease: featuregate.Deprecated, LockToDefault: true}, // remove in 1.35
},
@ -291,6 +295,7 @@ var defaultVersionedKubernetesFeatureGates = map[featuregate.Feature]featuregate
},
genericfeatures.KMSv1: {
{Version: version.MustParse("1.0"), Default: true, PreRelease: featuregate.GA},
{Version: version.MustParse("1.28"), Default: true, PreRelease: featuregate.Deprecated},
{Version: version.MustParse("1.29"), Default: false, PreRelease: featuregate.Deprecated},
},
@ -474,6 +479,7 @@ var defaultVersionedKubernetesFeatureGates = map[featuregate.Feature]featuregate
},
KubeletRegistrationGetOnExistsOnly: {
{Version: version.MustParse("1.0"), Default: true, PreRelease: featuregate.GA},
{Version: version.MustParse("1.32"), Default: false, PreRelease: featuregate.Deprecated},
},

View File

@ -356,6 +356,7 @@ var defaultVersionedKubernetesFeatureGates = map[featuregate.Feature]featuregate
},
KMSv1: {
{Version: version.MustParse("1.0"), Default: true, PreRelease: featuregate.GA},
{Version: version.MustParse("1.28"), Default: true, PreRelease: featuregate.Deprecated},
{Version: version.MustParse("1.29"), Default: false, PreRelease: featuregate.Deprecated},
},

View File

@ -318,7 +318,6 @@ func (f *featureGate) unsafeSetFromMap(enabled map[Feature]bool, m map[string]bo
// Copy existing state
known := map[Feature]VersionedSpecs{}
for k, v := range f.known.Load().(map[Feature]VersionedSpecs) {
sort.Sort(v)
known[k] = v
}
@ -421,19 +420,52 @@ func (f *featureGate) AddVersioned(features map[Feature]VersionedSpecs) error {
if f.closed {
return fmt.Errorf("cannot add a feature gate after adding it to the flag set")
}
// Copy existing state
known := f.GetAllVersioned()
for name, specs := range features {
sort.Sort(specs)
if existingSpec, found := known[name]; found {
sort.Sort(existingSpec)
if reflect.DeepEqual(existingSpec, specs) {
continue
}
return fmt.Errorf("feature gate %q with different spec already exists: %v", name, existingSpec)
}
// Validate new specs are well-formed
var lastVersion *version.Version
var wasBeta, wasGA, wasDeprecated bool
for i, spec := range specs {
if spec.Version == nil {
return fmt.Errorf("feature %q did not provide a version", name)
}
if len(spec.Version.Components()) != 2 {
return fmt.Errorf("feature %q specified patch version: %s", name, spec.Version.String())
}
// gates that begin as deprecated must indicate their prior state
if i == 0 && spec.PreRelease == Deprecated && spec.Version.Minor() != 0 {
return fmt.Errorf("feature %q introduced as deprecated must provide a 1.0 entry indicating initial state", name)
}
if i > 0 {
// versions must strictly increase
if !lastVersion.LessThan(spec.Version) {
return fmt.Errorf("feature %q lists version transitions in non-increasing order (%s <= %s)", name, spec.Version, lastVersion)
}
// stability must not regress from ga --> {beta,alpha} or beta --> alpha, and
// Deprecated state must be the terminal state
switch {
case spec.PreRelease != Deprecated && wasDeprecated:
return fmt.Errorf("deprecated feature %q must not resurrect from its terminal state", name)
case spec.PreRelease == Alpha && (wasBeta || wasGA):
return fmt.Errorf("feature %q regresses stability from more stable level to %s in %s", name, spec.PreRelease, spec.Version)
case spec.PreRelease == Beta && wasGA:
return fmt.Errorf("feature %q regresses stability from more stable level to %s in %s", name, spec.PreRelease, spec.Version)
}
}
lastVersion = spec.Version
wasBeta = wasBeta || spec.PreRelease == Beta
wasGA = wasGA || spec.PreRelease == GA
wasDeprecated = wasDeprecated || spec.PreRelease == Deprecated
}
known[name] = specs
}

View File

@ -1017,20 +1017,20 @@ func TestVersionedFeatureGateFlag(t *testing.T) {
}
err := f.AddVersioned(map[Feature]VersionedSpecs{
testGAGate: {
{Version: version.MustParse("1.29"), Default: true, PreRelease: GA},
{Version: version.MustParse("1.28"), Default: false, PreRelease: Beta},
{Version: version.MustParse("1.27"), Default: false, PreRelease: Alpha},
{Version: version.MustParse("1.28"), Default: false, PreRelease: Beta},
{Version: version.MustParse("1.29"), Default: true, PreRelease: GA},
},
testAlphaGate: {
{Version: version.MustParse("1.29"), Default: false, PreRelease: Alpha},
},
testBetaGate: {
{Version: version.MustParse("1.29"), Default: false, PreRelease: Beta},
{Version: version.MustParse("1.28"), Default: false, PreRelease: Alpha},
{Version: version.MustParse("1.29"), Default: false, PreRelease: Beta},
},
testLockedFalseGate: {
{Version: version.MustParse("1.29"), Default: false, PreRelease: GA, LockToDefault: true},
{Version: version.MustParse("1.28"), Default: false, PreRelease: GA},
{Version: version.MustParse("1.29"), Default: false, PreRelease: GA, LockToDefault: true},
},
})
require.NoError(t, err)
@ -1076,8 +1076,8 @@ func TestVersionedFeatureGateOverride(t *testing.T) {
{Version: version.MustParse("1.29"), Default: false, PreRelease: Alpha},
},
testBetaGate: {
{Version: version.MustParse("1.29"), Default: false, PreRelease: Beta},
{Version: version.MustParse("1.28"), Default: false, PreRelease: Alpha},
{Version: version.MustParse("1.29"), Default: false, PreRelease: Beta},
},
})
require.NoError(t, err)
@ -1127,17 +1127,17 @@ func TestVersionedFeatureGateFlagDefaults(t *testing.T) {
err := f.AddVersioned(map[Feature]VersionedSpecs{
testGAGate: {
{Version: version.MustParse("1.29"), Default: true, PreRelease: GA},
{Version: version.MustParse("1.27"), Default: true, PreRelease: Beta},
{Version: version.MustParse("1.25"), Default: true, PreRelease: Alpha},
{Version: version.MustParse("1.27"), Default: true, PreRelease: Beta},
{Version: version.MustParse("1.29"), Default: true, PreRelease: GA},
},
testAlphaGate: {
{Version: version.MustParse("1.29"), Default: false, PreRelease: Alpha},
},
testBetaGate: {
{Version: version.MustParse("1.29"), Default: true, PreRelease: Beta},
{Version: version.MustParse("1.28"), Default: false, PreRelease: Beta},
{Version: version.MustParse("1.26"), Default: false, PreRelease: Alpha},
{Version: version.MustParse("1.28"), Default: false, PreRelease: Beta},
{Version: version.MustParse("1.29"), Default: true, PreRelease: Beta},
},
})
require.NoError(t, err)
@ -1207,8 +1207,8 @@ func TestVersionedFeatureGateKnownFeatures(t *testing.T) {
{Version: version.MustParse("1.28"), Default: false, PreRelease: Beta},
},
testDeprecatedGate: {
{Version: version.MustParse("1.28"), Default: true, PreRelease: Deprecated},
{Version: version.MustParse("1.26"), Default: false, PreRelease: Alpha},
{Version: version.MustParse("1.28"), Default: true, PreRelease: Deprecated},
},
})
require.NoError(t, err)
@ -1264,12 +1264,12 @@ func TestVersionedFeatureGateMetrics(t *testing.T) {
{Version: version.MustParse("1.29"), Default: true, PreRelease: Beta},
},
testBetaGate: {
{Version: version.MustParse("1.28"), Default: true, PreRelease: Beta},
{Version: version.MustParse("1.27"), Default: false, PreRelease: Alpha},
{Version: version.MustParse("1.28"), Default: true, PreRelease: Beta},
},
testBetaDisabled: {
{Version: version.MustParse("1.28"), Default: true, PreRelease: Beta},
{Version: version.MustParse("1.27"), Default: false, PreRelease: Alpha},
{Version: version.MustParse("1.28"), Default: true, PreRelease: Beta},
},
})
require.NoError(t, err)
@ -1528,9 +1528,10 @@ func TestVersionedFeatureGateOverrideDefault(t *testing.T) {
}
func TestFeatureSpecAtEmulationVersion(t *testing.T) {
specs := VersionedSpecs{{Version: version.MustParse("1.29"), Default: true, PreRelease: GA},
{Version: version.MustParse("1.28"), Default: false, PreRelease: Beta},
specs := VersionedSpecs{
{Version: version.MustParse("1.25"), Default: false, PreRelease: Alpha},
{Version: version.MustParse("1.28"), Default: false, PreRelease: Beta},
{Version: version.MustParse("1.29"), Default: true, PreRelease: GA},
}
sort.Sort(specs)
tests := []struct {
@ -1660,8 +1661,8 @@ func TestExplicitlySet(t *testing.T) {
{Version: version.MustParse("1.29"), Default: false, PreRelease: Alpha},
},
testBetaGate: {
{Version: version.MustParse("1.29"), Default: false, PreRelease: Beta},
{Version: version.MustParse("1.28"), Default: false, PreRelease: Alpha},
{Version: version.MustParse("1.29"), Default: false, PreRelease: Beta},
},
})
require.NoError(t, err)
@ -1699,8 +1700,8 @@ func TestResetFeatureValueToDefault(t *testing.T) {
{Version: version.MustParse("1.29"), Default: false, PreRelease: Alpha},
},
testBetaGate: {
{Version: version.MustParse("1.29"), Default: true, PreRelease: Beta},
{Version: version.MustParse("1.28"), Default: false, PreRelease: Alpha},
{Version: version.MustParse("1.29"), Default: true, PreRelease: Beta},
},
})
require.NoError(t, err)
@ -1742,3 +1743,165 @@ func TestResetFeatureValueToDefault(t *testing.T) {
assert.False(t, f.Enabled("TestAlpha"))
assert.False(t, f.Enabled("TestBeta"))
}
func TestAddVersioned(t *testing.T) {
// gates for testing
const testAGate Feature = "TestA"
const testBGate Feature = "TestB"
tests := []struct {
name string
expectError bool
features map[Feature]VersionedSpecs
}{
{
name: "normal progression",
features: map[Feature]VersionedSpecs{
testAGate: {
{Version: version.MustParse("1.29"), Default: false, PreRelease: Alpha},
{Version: version.MustParse("1.30"), Default: false, PreRelease: Beta},
{Version: version.MustParse("1.31"), Default: true, PreRelease: Beta},
{Version: version.MustParse("1.32"), Default: true, PreRelease: GA},
{Version: version.MustParse("1.33"), Default: false, PreRelease: Deprecated},
},
testBGate: {
{Version: version.MustParse("1.27"), Default: false, PreRelease: Alpha},
},
},
},
{
name: "conflicting specs",
expectError: true,
features: map[Feature]VersionedSpecs{
testBGate: {
{Version: version.MustParse("1.28"), Default: false, PreRelease: Alpha},
},
},
},
{
name: "deprecated feature with no prior state",
expectError: true,
features: map[Feature]VersionedSpecs{
testAGate: {
{Version: version.MustParse("1.29"), Default: false, PreRelease: Deprecated},
},
},
},
{
name: "deprecated feature with prior state",
features: map[Feature]VersionedSpecs{
testAGate: {
{Version: version.MustParse("1.0"), Default: true, PreRelease: GA},
{Version: version.MustParse("1.29"), Default: false, PreRelease: Deprecated},
},
},
},
{
name: "duplicate version",
expectError: true,
features: map[Feature]VersionedSpecs{
testAGate: {
{Version: version.MustParse("1.29"), Default: false, PreRelease: Alpha},
{Version: version.MustParse("1.29"), Default: true, PreRelease: Beta},
},
},
},
{
name: "decreasing version",
expectError: true,
features: map[Feature]VersionedSpecs{
testAGate: {
{Version: version.MustParse("1.30"), Default: false, PreRelease: Beta},
{Version: version.MustParse("1.29"), Default: true, PreRelease: Beta},
},
},
},
{
name: "Beta to Alpha",
expectError: true,
features: map[Feature]VersionedSpecs{
testAGate: {
{Version: version.MustParse("1.29"), Default: true, PreRelease: Beta},
{Version: version.MustParse("1.30"), Default: false, PreRelease: Alpha},
},
},
},
{
name: "GA to Alpha",
expectError: true,
features: map[Feature]VersionedSpecs{
testAGate: {
{Version: version.MustParse("1.29"), Default: true, PreRelease: GA},
{Version: version.MustParse("1.30"), Default: false, PreRelease: Alpha},
},
},
},
{
name: "GA to Beta",
expectError: true,
features: map[Feature]VersionedSpecs{
testAGate: {
{Version: version.MustParse("1.29"), Default: true, PreRelease: GA},
{Version: version.MustParse("1.30"), Default: true, PreRelease: Beta},
},
},
},
{
name: "Alpha to Deprecated to Beta",
expectError: true,
features: map[Feature]VersionedSpecs{
testAGate: {
{Version: version.MustParse("1.29"), Default: false, PreRelease: Alpha},
{Version: version.MustParse("1.30"), Default: false, PreRelease: Deprecated},
{Version: version.MustParse("1.31"), Default: true, PreRelease: Beta},
},
},
},
{
name: "Deprecated to Deprecated",
features: map[Feature]VersionedSpecs{
testAGate: {
{Version: version.MustParse("1.29"), Default: false, PreRelease: Alpha},
{Version: version.MustParse("1.30"), Default: true, PreRelease: Deprecated},
{Version: version.MustParse("1.31"), Default: false, PreRelease: Deprecated},
},
},
},
{
name: "always Deprecated",
features: map[Feature]VersionedSpecs{
testAGate: {
{Version: version.MustParse("1.0"), Default: false, PreRelease: Deprecated},
},
},
},
{
name: "patch version",
expectError: true,
features: map[Feature]VersionedSpecs{
testAGate: {
{Version: version.MustParse("1.29.1"), Default: false, PreRelease: Alpha},
{Version: version.MustParse("1.30"), Default: false, PreRelease: Beta},
},
},
},
}
for _, test := range tests {
t.Run(fmt.Sprintf("AddVersioned-%s", test.name), func(t *testing.T) {
f := NewFeatureGate()
err := f.AddVersioned(map[Feature]VersionedSpecs{
testBGate: {
{Version: version.MustParse("1.27"), Default: false, PreRelease: Alpha},
},
})
require.NoError(t, err)
err = f.AddVersioned(test.features)
if err != nil && !test.expectError {
t.Errorf("expected no errors, error found %+v", err)
}
if err == nil && test.expectError {
t.Errorf("expected errors, no errors found")
}
})
}
}

View File

@ -22,6 +22,7 @@ import (
"testing"
"github.com/spf13/pflag"
utilruntime "k8s.io/apimachinery/pkg/util/runtime"
"k8s.io/apimachinery/pkg/util/version"
baseversion "k8s.io/component-base/version"
@ -60,16 +61,16 @@ func testRegistry(t *testing.T) *componentGlobalsRegistry {
fgKube := NewVersionedFeatureGate(version.MustParse("0.0"))
err := fgKube.AddVersioned(map[Feature]VersionedSpecs{
"kubeA": {
{Version: version.MustParse("1.31"), Default: true, LockToDefault: true, PreRelease: GA},
{Version: version.MustParse("1.28"), Default: false, PreRelease: Beta},
{Version: version.MustParse("1.27"), Default: false, PreRelease: Alpha},
{Version: version.MustParse("1.28"), Default: false, PreRelease: Beta},
{Version: version.MustParse("1.31"), Default: true, LockToDefault: true, PreRelease: GA},
},
"kubeB": {
{Version: version.MustParse("1.30"), Default: false, PreRelease: Alpha},
},
"commonC": {
{Version: version.MustParse("1.29"), Default: true, PreRelease: Beta},
{Version: version.MustParse("1.27"), Default: false, PreRelease: Alpha},
{Version: version.MustParse("1.29"), Default: true, PreRelease: Beta},
},
})
if err != nil {
@ -80,16 +81,16 @@ func testRegistry(t *testing.T) *componentGlobalsRegistry {
fgTest := NewVersionedFeatureGate(version.MustParse("0.0"))
err = fgTest.AddVersioned(map[Feature]VersionedSpecs{
"testA": {
{Version: version.MustParse("2.10"), Default: true, PreRelease: GA},
{Version: version.MustParse("2.8"), Default: false, PreRelease: Beta},
{Version: version.MustParse("2.7"), Default: false, PreRelease: Alpha},
{Version: version.MustParse("2.8"), Default: false, PreRelease: Beta},
{Version: version.MustParse("2.10"), Default: true, PreRelease: GA},
},
"testB": {
{Version: version.MustParse("2.9"), Default: false, PreRelease: Alpha},
},
"commonC": {
{Version: version.MustParse("2.9"), Default: true, PreRelease: Beta},
{Version: version.MustParse("2.7"), Default: false, PreRelease: Alpha},
{Version: version.MustParse("2.9"), Default: true, PreRelease: Beta},
},
})
if err != nil {

View File

@ -21,6 +21,7 @@ import (
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"k8s.io/apimachinery/pkg/util/version"
"k8s.io/component-base/featuregate"
)
@ -173,15 +174,15 @@ func TestSpecialGatesVersioned(t *gotest.T) {
{Version: version.MustParse("1.31"), Default: true, PreRelease: featuregate.Beta},
},
"beta_default_on_set_off": {
{Version: version.MustParse("1.31"), Default: true, PreRelease: featuregate.Beta},
{Version: version.MustParse("1.27"), Default: false, PreRelease: featuregate.Alpha},
{Version: version.MustParse("1.31"), Default: true, PreRelease: featuregate.Beta},
},
"beta_default_off": {
{Version: version.MustParse("1.27"), Default: false, PreRelease: featuregate.Beta},
},
"beta_default_off_set_on": {
{Version: version.MustParse("1.31"), Default: false, PreRelease: featuregate.Beta},
{Version: version.MustParse("1.27"), Default: false, PreRelease: featuregate.Alpha},
{Version: version.MustParse("1.31"), Default: false, PreRelease: featuregate.Beta},
},
})
require.NoError(t, err)

View File

@ -143,9 +143,9 @@ func NewCommandStartWardleServer(ctx context.Context, defaults *WardleServerOpti
// These specifications, together with the effective version, determine if the feature is enabled.
utilruntime.Must(wardleFeatureGate.AddVersioned(map[featuregate.Feature]featuregate.VersionedSpecs{
"BanFlunder": {
{Version: version.MustParse("1.2"), Default: true, PreRelease: featuregate.GA, LockToDefault: true},
{Version: version.MustParse("1.1"), Default: true, PreRelease: featuregate.Beta},
{Version: version.MustParse("1.0"), Default: false, PreRelease: featuregate.Alpha},
{Version: version.MustParse("1.1"), Default: true, PreRelease: featuregate.Beta},
{Version: version.MustParse("1.2"), Default: true, PreRelease: featuregate.GA, LockToDefault: true},
},
}))

View File

@ -28,18 +28,30 @@
version: "1.30"
- name: AllowDNSOnlyNodeCSR
versionedSpecs:
- default: true
lockToDefault: false
preRelease: GA
version: "1.0"
- default: false
lockToDefault: false
preRelease: Deprecated
version: "1.31"
- name: AllowInsecureKubeletCertificateSigningRequests
versionedSpecs:
- default: true
lockToDefault: false
preRelease: GA
version: "1.0"
- default: false
lockToDefault: false
preRelease: Deprecated
version: "1.31"
- name: AllowOverwriteTerminationGracePeriodSeconds
versionedSpecs:
- default: true
lockToDefault: false
preRelease: GA
version: "1.0"
- default: false
lockToDefault: false
preRelease: Deprecated
@ -52,6 +64,10 @@
version: "1.33"
- name: AllowServiceLBStatusOnNonLB
versionedSpecs:
- default: true
lockToDefault: false
preRelease: GA
version: "1.0"
- default: false
lockToDefault: false
preRelease: Deprecated
@ -620,6 +636,10 @@
version: "1.31"
- name: KMSv1
versionedSpecs:
- default: true
lockToDefault: false
preRelease: GA
version: "1.0"
- default: true
lockToDefault: false
preRelease: Deprecated
@ -670,6 +690,10 @@
version: "1.27"
- name: KubeletRegistrationGetOnExistsOnly
versionedSpecs:
- default: true
lockToDefault: false
preRelease: GA
version: "1.0"
- default: false
lockToDefault: false
preRelease: Deprecated