From 9828ad64da4e47e25a50e72257511c007d944818 Mon Sep 17 00:00:00 2001 From: Benjamin Elder Date: Wed, 5 Mar 2025 08:14:44 +0100 Subject: [PATCH 1/2] e2e framework WithFeatureGate adds [Feature:OffByDefault] (when passed a feature that is not Default) This allows using the regex filter to skip tests that do not work on a cluster without optional configuration, while moving tests to use WithFeatureGate without also setting WithFeature unless they have some additional configuration required. Co-authored-by: Patrick Ohly --- test/e2e/framework/ginkgowrapper.go | 52 ++++++++++++++----- .../framework/internal/unittests/bugs/bugs.go | 4 +- 2 files changed, 42 insertions(+), 14 deletions(-) diff --git a/test/e2e/framework/ginkgowrapper.go b/test/e2e/framework/ginkgowrapper.go index f7215eccb5c..2371f9330ec 100644 --- a/test/e2e/framework/ginkgowrapper.go +++ b/test/e2e/framework/ginkgowrapper.go @@ -208,9 +208,14 @@ func registerInSuite(ginkgoCall func(string, ...interface{}) bool, args []interf case label: fullLabel := strings.Join(arg.parts, ":") addLabel(fullLabel) - if arg.extraFeature != "" { - texts = append(texts, fmt.Sprintf("[%s]", arg.extraFeature)) - ginkgoArgs = append(ginkgoArgs, ginkgo.Label("Feature:"+arg.extraFeature)) + if arg.alphaBetaLevel != "" { + texts = append(texts, fmt.Sprintf("[%[1]s]", arg.alphaBetaLevel)) + ginkgoArgs = append(ginkgoArgs, ginkgo.Label("Feature:"+arg.alphaBetaLevel)) + } + if arg.offByDefault { + texts = append(texts, "[Feature:OffByDefault]") + // TODO: consider this once we have a plan to update the alpha/beta job filters + // ginkgoArgs = append(ginkgoArgs, ginkgo.Label("Feature:OffByDefault")) } if fullLabel == "Serial" { ginkgoArgs = append(ginkgoArgs, ginkgo.Serial) @@ -305,6 +310,12 @@ func validateText(location types.CodeLocation, text string, labels []string) { // Okay, was also set as label. continue } + // TODO: we currently only set this as a text value + // We should probably reflect it into labels, but that could break some + // existing jobs and we're still setting on an exact plan + if tag == "Feature:OffByDefault" { + continue + } if deprecatedTags.Has(tag) { recordTextBug(location, fmt.Sprintf("[%s] in plain text is deprecated and must be added through With%s instead", tag, tag)) } @@ -350,7 +361,8 @@ func withFeature(name Feature) interface{} { } // WithFeatureGate specifies that a certain test or group of tests depends on a -// feature gate being enabled. The return value must be passed as additional +// feature gate and the corresponding API group (if there is one) +// being enabled. The return value must be passed as additional // argument to [framework.It], [framework.Describe], [framework.Context]. // // The feature gate must be listed in @@ -359,9 +371,15 @@ func withFeature(name Feature) interface{} { // also need to be removed. // // [Alpha] resp. [Beta] get added to the test name automatically depending -// on the current stability level of the feature. Feature:Alpha resp. -// Feature:Beta get added to the Ginkgo labels because this is a special -// requirement for how the cluster needs to be configured. +// on the current stability level of the feature, to emulate historic +// usage of those tags. +// +// In addition, [Feature:Alpha] resp. [Feature:Beta] get added to support +// skipping a test with a dependency on an alpha or beta feature gate in +// jobs which use the traditional \[Feature:.*\] skip regular expression. +// +// For label filtering, Feature:Alpha resp. Feature:Beta get added to the +// Ginkgo labels. // // If the test can run in any cluster that has alpha resp. beta features and // API groups enabled, then annotating it with just WithFeatureGate is @@ -390,7 +408,8 @@ func withFeatureGate(featureGate featuregate.Feature) interface{} { } l := newLabel("FeatureGate", string(featureGate)) - l.extraFeature = level + l.offByDefault = !spec.Default + l.alphaBetaLevel = level return l } @@ -536,13 +555,19 @@ func withFlaky() interface{} { type label struct { // parts get concatenated with ":" to build the full label. parts []string - // extra is an optional feature name. It gets added as [] - // to the test name and as Feature: to the labels. - extraFeature string // explanation gets set for each label to help developers // who pass a label to a ginkgo function. They need to use // the corresponding framework function instead. explanation string + + // TODO: the fields below are only used for FeatureGates, we may want to refactor + + // alphaBetaLevel is "Alpha", "Beta" or empty for GA features + // It gets added as [] [Feature:] + // to the test name and as Feature: to the labels. + alphaBetaLevel string + // set based on featuregate default state + offByDefault bool } func newLabel(parts ...string) label { @@ -565,7 +590,10 @@ func TagsEqual(a, b interface{}) bool { if !ok { return false } - if al.extraFeature != bl.extraFeature { + if al.alphaBetaLevel != bl.alphaBetaLevel { + return false + } + if al.offByDefault != bl.offByDefault { return false } return slices.Equal(al.parts, bl.parts) diff --git a/test/e2e/framework/internal/unittests/bugs/bugs.go b/test/e2e/framework/internal/unittests/bugs/bugs.go index 7d818aa8e53..9147db39606 100644 --- a/test/e2e/framework/internal/unittests/bugs/bugs.go +++ b/test/e2e/framework/internal/unittests/bugs/bugs.go @@ -122,8 +122,8 @@ ERROR: some/relative/path/buggy.go:200: with spaces ` // Used by unittests/list-tests. It's sorted by test name, not source code location. ListTestsOutput = `The following spec names can be used with 'ginkgo run --focus/skip': - ../bugs/bugs.go:100: [sig-testing] abc space1 space2 [Feature:no-such-feature] [Feature:feature-foo] [Environment:no-such-env] [Environment:Linux] [FeatureGate:no-such-feature-gate] [FeatureGate:TestAlphaFeature] [Alpha] [FeatureGate:TestBetaFeature] [Beta] [FeatureGate:TestGAFeature] [Conformance] [NodeConformance] [Slow] [Serial] [Disruptive] [custom-label] xyz x [foo] should [bar] - ../bugs/bugs.go:95: [sig-testing] abc space1 space2 [Feature:no-such-feature] [Feature:feature-foo] [Environment:no-such-env] [Environment:Linux] [FeatureGate:no-such-feature-gate] [FeatureGate:TestAlphaFeature] [Alpha] [FeatureGate:TestBetaFeature] [Beta] [FeatureGate:TestGAFeature] [Conformance] [NodeConformance] [Slow] [Serial] [Disruptive] [custom-label] xyz y [foo] should [bar] + ../bugs/bugs.go:100: [sig-testing] abc space1 space2 [Feature:no-such-feature] [Feature:feature-foo] [Environment:no-such-env] [Environment:Linux] [FeatureGate:no-such-feature-gate] [Feature:OffByDefault] [FeatureGate:TestAlphaFeature] [Alpha] [Feature:OffByDefault] [FeatureGate:TestBetaFeature] [Beta] [Feature:OffByDefault] [FeatureGate:TestGAFeature] [Feature:OffByDefault] [Conformance] [NodeConformance] [Slow] [Serial] [Disruptive] [custom-label] xyz x [foo] should [bar] + ../bugs/bugs.go:95: [sig-testing] abc space1 space2 [Feature:no-such-feature] [Feature:feature-foo] [Environment:no-such-env] [Environment:Linux] [FeatureGate:no-such-feature-gate] [Feature:OffByDefault] [FeatureGate:TestAlphaFeature] [Alpha] [Feature:OffByDefault] [FeatureGate:TestBetaFeature] [Beta] [Feature:OffByDefault] [FeatureGate:TestGAFeature] [Feature:OffByDefault] [Conformance] [NodeConformance] [Slow] [Serial] [Disruptive] [custom-label] xyz y [foo] should [bar] ` From 628d107b61989fd13323f12ef751fecaa784019d Mon Sep 17 00:00:00 2001 From: Jordan Liggitt Date: Tue, 4 Mar 2025 15:35:12 -0500 Subject: [PATCH 2/2] Switch cluster trust bundle e2e tests to generic alpha feature + feature gates This relies on WithFeatureGate adding [Feature:OffByDefault]. Without that, the test would start to run in jobs which don't enable the feature. --- test/e2e/auth/projected_clustertrustbundle.go | 4 ++-- test/e2e/feature/feature.go | 6 ------ 2 files changed, 2 insertions(+), 8 deletions(-) diff --git a/test/e2e/auth/projected_clustertrustbundle.go b/test/e2e/auth/projected_clustertrustbundle.go index 76328621122..6b07a198410 100644 --- a/test/e2e/auth/projected_clustertrustbundle.go +++ b/test/e2e/auth/projected_clustertrustbundle.go @@ -39,7 +39,7 @@ import ( "k8s.io/apimachinery/pkg/util/uuid" "k8s.io/apimachinery/pkg/util/wait" podutil "k8s.io/kubernetes/pkg/api/v1/pod" - "k8s.io/kubernetes/test/e2e/feature" + "k8s.io/kubernetes/pkg/features" "k8s.io/kubernetes/test/e2e/framework" e2epodoutput "k8s.io/kubernetes/test/e2e/framework/pod/output" imageutils "k8s.io/kubernetes/test/utils/image" @@ -57,7 +57,7 @@ const ( noSignerKey = "no-signer" ) -var _ = SIGDescribe(feature.ClusterTrustBundle, feature.ClusterTrustBundleProjection, func() { +var _ = SIGDescribe(framework.WithFeatureGate(features.ClusterTrustBundle), framework.WithFeatureGate(features.ClusterTrustBundleProjection), func() { f := framework.NewDefaultFramework("projected-clustertrustbundle") f.NamespacePodSecurityLevel = admissionapi.LevelBaseline diff --git a/test/e2e/feature/feature.go b/test/e2e/feature/feature.go index 4352ea1dacf..3d351445164 100644 --- a/test/e2e/feature/feature.go +++ b/test/e2e/feature/feature.go @@ -60,12 +60,6 @@ var ( // Owner: sig-autoscaling ClusterSizeAutoscalingScaleUp = framework.WithFeature(framework.ValidFeatures.Add("ClusterSizeAutoscalingScaleUp")) - // TODO: document the feature (owning SIG, when to use this feature for a test) - ClusterTrustBundle = framework.WithFeature(framework.ValidFeatures.Add("ClusterTrustBundle")) - - // TODO: document the feature (owning SIG, when to use this feature for a test) - ClusterTrustBundleProjection = framework.WithFeature(framework.ValidFeatures.Add("ClusterTrustBundleProjection")) - // TODO: document the feature (owning SIG, when to use this feature for a test) ClusterUpgrade = framework.WithFeature(framework.ValidFeatures.Add("ClusterUpgrade"))