Skip to content

Commit

Permalink
Add Validation to versioned feature specs.
Browse files Browse the repository at this point in the history 
Co-authored-by: Jordan Liggitt <liggitt@google.com>
Co-authored-by: Siyuan Zhang <sizhang@google.com>

Signed-off-by: Siyuan Zhang <sizhang@google.com>

Kubernetes-commit: 00dab9dffa6a35fbfaad4ebcdd17be00f40e423c
  • Loading branch information
@siyuanfoundation @k8s-publishing-bot
siyuanfoundation authored and k8s-publishing-bot committed Dec 3, 2024
1 parent b7fbd0d commit 2234523
Show file tree
Hide file tree
Showing 4 changed files with 225 additions and 28 deletions.
40 changes: 36 additions & 4 deletions featuregate/feature_gate.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Expand Down Expand Up @@ -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
}

Expand Down
195 changes: 179 additions & 16 deletions featuregate/feature_gate_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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")
}
})
}
}
13 changes: 7 additions & 6 deletions featuregate/registry_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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 {
Expand All @@ -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 {
Expand Down
Loading

0 comments on commit 2234523

Please sign in to comment.