Merge pull request kubernetes#130079 from yongruilin/compatibility-remove-reset

[compatibility version] Avoid resetting config when adding flags

Author: k8s-ci-robot

cmd/kube-scheduler/app/testing/testserver.go

@@ -26,15 +26,17 @@ import (
 	"github.com/spf13/pflag"
 
 	"k8s.io/apimachinery/pkg/util/wait"
+	utilcompatibility "k8s.io/apiserver/pkg/util/compatibility"
+	utilfeature "k8s.io/apiserver/pkg/util/feature"
 	"k8s.io/client-go/kubernetes"
 	restclient "k8s.io/client-go/rest"
+	"k8s.io/component-base/compatibility"
 	"k8s.io/component-base/configz"
 	logsapi "k8s.io/component-base/logs/api/v1"
+	"k8s.io/klog/v2"
 	"k8s.io/kubernetes/cmd/kube-scheduler/app"
 	kubeschedulerconfig "k8s.io/kubernetes/cmd/kube-scheduler/app/config"
 	"k8s.io/kubernetes/cmd/kube-scheduler/app/options"
-
-	"k8s.io/klog/v2"
 )
 
 func init() {
@@ -98,6 +100,16 @@ func StartTestServer(ctx context.Context, customFlags []string) (result TestServ
 	fs := pflag.NewFlagSet("test", pflag.PanicOnError)
 
 	opts := options.NewOptions()
+	// set up new instance of ComponentGlobalsRegistry instead of using the DefaultComponentGlobalsRegistry to avoid contention in parallel tests.
+	featureGate := utilfeature.DefaultMutableFeatureGate.DeepCopy()
+	effectiveVersion := utilcompatibility.DefaultKubeEffectiveVersionForTest()
+	effectiveVersion.SetEmulationVersion(featureGate.EmulationVersion())
+	componentGlobalsRegistry := compatibility.NewComponentGlobalsRegistry()
+	if err := componentGlobalsRegistry.Register(compatibility.DefaultKubeComponent, effectiveVersion, featureGate); err != nil {
+		return result, err
+	}
+	opts.ComponentGlobalsRegistry = componentGlobalsRegistry
+
 	nfs := opts.Flags
 	for _, f := range nfs.FlagSets {
 		fs.AddFlagSet(f)
@@ -106,6 +118,20 @@ func StartTestServer(ctx context.Context, customFlags []string) (result TestServ
 	if err := opts.ComponentGlobalsRegistry.Set(); err != nil {
 		return result, err
 	}
+	// If the local ComponentGlobalsRegistry is changed by the flags,
+	// we need to copy the new feature values back to the DefaultFeatureGate because most feature checks still use the DefaultFeatureGate.
+	if !featureGate.EmulationVersion().EqualTo(utilfeature.DefaultMutableFeatureGate.EmulationVersion()) {
+		if err := utilfeature.DefaultMutableFeatureGate.SetEmulationVersion(effectiveVersion.EmulationVersion()); err != nil {
+			return result, err
+		}
+	}
+	for f := range utilfeature.DefaultMutableFeatureGate.GetAll() {
+		if featureGate.Enabled(f) != utilfeature.DefaultFeatureGate.Enabled(f) {
+			if err := utilfeature.DefaultMutableFeatureGate.Set(fmt.Sprintf("%s=%v", f, featureGate.Enabled(f))); err != nil {
+				return result, err
+			}
+		}
+	}
 
 	if opts.SecureServing.BindPort != 0 {
 		opts.SecureServing.Listener, opts.SecureServing.BindPort, err = createListenerOnFreePort()

staging/src/k8s.io/component-base/compatibility/registry.go

@@ -102,6 +102,9 @@ type componentGlobalsRegistry struct {
 	// When the `--feature-gates` flag is parsed, it would not take effect until Set() is called,
 	// because the emulation version needs to be set before the feature gate is set.
 	featureGatesConfig map[string][]string
+	// featureGatesConfigFlags stores a pointer to the flag value, allowing other commands
+	// to append to the feature gates configuration rather than overwriting it
+	featureGatesConfigFlags *cliflag.ColonSeparatedMultimapStringString
 	// set stores if the Set() function for the registry is already called.
 	set bool
 }
@@ -120,6 +123,7 @@ func (r *componentGlobalsRegistry) Reset() {
 	r.componentGlobals = make(map[string]*ComponentGlobals)
 	r.emulationVersionConfig = nil
 	r.featureGatesConfig = nil
+	r.featureGatesConfigFlags = nil
 	r.set = false
 }
 
@@ -226,19 +230,17 @@ func (r *componentGlobalsRegistry) AddFlags(fs *pflag.FlagSet) {
 			globals.featureGate.Close()
 		}
 	}
-	if r.emulationVersionConfig != nil || r.featureGatesConfig != nil {
-		klog.Warning("calling componentGlobalsRegistry.AddFlags more than once, the registry will be set by the latest flags")
-	}
-	r.emulationVersionConfig = []string{}
-	r.featureGatesConfig = make(map[string][]string)
 
 	fs.StringSliceVar(&r.emulationVersionConfig, "emulated-version", r.emulationVersionConfig, ""+
 		"The versions different components emulate their capabilities (APIs, features, ...) of.\n"+
 		"If set, the component will emulate the behavior of this version instead of the underlying binary version.\n"+
 		"Version format could only be major.minor, for example: '--emulated-version=wardle=1.2,kube=1.31'. Options are:\n"+strings.Join(r.unsafeVersionFlagOptions(true), "\n")+
 		"If the component is not specified, defaults to \"kube\"")
 
-	fs.Var(cliflag.NewColonSeparatedMultimapStringStringAllowDefaultEmptyKey(&r.featureGatesConfig), "feature-gates", "Comma-separated list of component:key=value pairs that describe feature gates for alpha/experimental features of different components.\n"+
+	if r.featureGatesConfigFlags == nil {
+		r.featureGatesConfigFlags = cliflag.NewColonSeparatedMultimapStringStringAllowDefaultEmptyKey(&r.featureGatesConfig)
+	}
+	fs.Var(r.featureGatesConfigFlags, "feature-gates", "Comma-separated list of component:key=value pairs that describe feature gates for alpha/experimental features of different components.\n"+
 		"If the component is not specified, defaults to \"kube\". This flag can be repeatedly invoked. For example: --feature-gates 'wardle:featureA=true,wardle:featureB=false' --feature-gates 'kube:featureC=true'"+
 		"Options are:\n"+strings.Join(r.unsafeKnownFeatures(), "\n"))
 }
@@ -415,7 +417,7 @@ func (r *componentGlobalsRegistry) SetEmulationVersionMapping(fromComponent, toC
 		return fmt.Errorf("EmulationVersion from %s to %s already exists", fromComponent, toComponent)
 	}
 	versionMapping[toComponent] = f
-	klog.V(klogLevel).Infof("setting the default EmulationVersion of %s based on mapping from the default EmulationVersion of %s", fromComponent, toComponent)
+	klog.V(klogLevel).Infof("setting the default EmulationVersion of %s based on mapping from the default EmulationVersion of %s", toComponent, fromComponent)
 	defaultFromVersion := r.componentGlobals[fromComponent].effectiveVersion.EmulationVersion()
 	emulationVersions, err := r.getFullEmulationVersionConfig(map[string]*version.Version{fromComponent: defaultFromVersion})
 	if err != nil {

staging/src/k8s.io/component-base/compatibility/registry_test.go

@@ -150,6 +150,7 @@ func TestVersionedFeatureGateFlags(t *testing.T) {
 func TestFlags(t *testing.T) {
 	tests := []struct {
 		name                         string
+		setupRegistry                func(r *componentGlobalsRegistry) error
 		flags                        []string
 		parseError                   string
 		expectedKubeEmulationVersion string
@@ -272,14 +273,46 @@ func TestFlags(t *testing.T) {
 			},
 			parseError: "component not registered: test3",
 		},
+		{
+			name: "feature gates config should accumulate across multiple flag sets",
+			setupRegistry: func(r *componentGlobalsRegistry) error {
+				fs := pflag.NewFlagSet("setupTestflag", pflag.ContinueOnError)
+				r.AddFlags(fs)
+				return fs.Parse([]string{"--feature-gates=test:commonC=true"})
+			},
+			flags: []string{
+				"--feature-gates=test:testA=true",
+			},
+			expectedTestFeatureValues: map[featuregate.Feature]bool{"testA": true, "commonC": true},
+		},
+		{
+			name: "feature gates config should be overridden when set multiple times one the same feature",
+			setupRegistry: func(r *componentGlobalsRegistry) error {
+				fs := pflag.NewFlagSet("setupTestflag", pflag.ContinueOnError)
+				r.AddFlags(fs)
+				return fs.Parse([]string{"--feature-gates=test:testA=false"})
+			},
+			flags: []string{
+				"--feature-gates=test:testA=true",
+			},
+			expectedTestFeatureValues: map[featuregate.Feature]bool{"testA": true},
+		},
 	}
 	for i, test := range tests {
 		t.Run(test.name, func(t *testing.T) {
 			fs := pflag.NewFlagSet("testflag", pflag.ContinueOnError)
 			r := testRegistry(t)
+			if test.setupRegistry != nil {
+				if err := test.setupRegistry(r); err != nil {
+					t.Fatalf("failed to setup registry: %v", err)
+				}
+			}
 			r.AddFlags(fs)
 			err := fs.Parse(test.flags)
 			if err == nil {
+				// AddFlags again to check whether there is no resetting on the config.
+				fs = pflag.NewFlagSet("testflag2", pflag.ContinueOnError)
+				r.AddFlags(fs)
 				err = r.Set()
 			}
 			if test.parseError != "" {

test/integration/examples/apiserver_test.go

@@ -48,6 +48,7 @@ import (
 	"k8s.io/apiserver/pkg/features"
 	"k8s.io/apiserver/pkg/server/dynamiccertificates"
 	genericapiserveroptions "k8s.io/apiserver/pkg/server/options"
+	utilcompatibility "k8s.io/apiserver/pkg/util/compatibility"
 	utilfeature "k8s.io/apiserver/pkg/util/feature"
 	client "k8s.io/client-go/kubernetes"
 	"k8s.io/client-go/rest"
@@ -263,6 +264,7 @@ func TestFrontProxyConfig(t *testing.T) {
 		testFrontProxyConfig(t, false)
 	})
 	t.Run("WithUID", func(t *testing.T) {
+		featuregatetesting.SetFeatureGateEmulationVersionDuringTest(t, utilfeature.DefaultFeatureGate, version.MajorMinor(1, 33))
 		featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.RemoteRequestHeaderUID, true)
 		testFrontProxyConfig(t, true)
 	})
@@ -277,7 +279,13 @@ func testFrontProxyConfig(t *testing.T, withUID bool) {
 	ctx, cancel := context.WithTimeout(context.Background(), 10*time.Minute)
 	t.Cleanup(cancel)
 
-	var extraKASFlags []string
+	// Set the emulation version for the kube-apiserver testserver by mapping
+	// the wardle version to the kube version.
+	wardleEmulationVersion := version.MustParse(wardleBinaryVersion)
+	kubeEmulationVersion := sampleserver.WardleVersionToKubeVersion(wardleEmulationVersion)
+	extraKASFlags := []string{
+		fmt.Sprintf("--emulated-version=kube=%s", kubeEmulationVersion.String()),
+	}
 	if withUID {
 		extraKASFlags = []string{"--requestheader-uid-headers=x-remote-uid"}
 	}
@@ -393,19 +401,27 @@ func (f roundTripperFunc) RoundTrip(req *http.Request) (*http.Response, error) {
 	return f(req)
 }
 
-func testAggregatedAPIServer(t *testing.T, setWardleFeatureGate, banFlunder bool, wardleBinaryVersion, wardleEmulationVersion string) {
+func testAggregatedAPIServer(t *testing.T, setWardleFeatureGate, banFlunder bool, wardleBinaryVersionRaw, wardleEmulationVersionRaw string) {
 	const testNamespace = "kube-wardle"
 
 	ctx, cancel := context.WithTimeout(context.Background(), 10*time.Minute)
 	t.Cleanup(cancel)
 
-	testKAS, wardleOptions, wardlePort := prepareAggregatedWardleAPIServer(ctx, t, testNamespace, wardleBinaryVersion, nil, false)
+	// set the emulation version for the kube-apiserver testserver by mapping
+	// the wardle version to the kube version.
+	wardleEmulationVersion := version.MustParse(wardleEmulationVersionRaw)
+	kubeEmulationVersion := sampleserver.WardleVersionToKubeVersion(wardleEmulationVersion)
+	extraKASFlags := []string{
+		fmt.Sprintf("--emulated-version=kube=%s", kubeEmulationVersion.String()),
+	}
+
+	testKAS, wardleOptions, wardlePort := prepareAggregatedWardleAPIServer(ctx, t, testNamespace, wardleBinaryVersionRaw, extraKASFlags, false)
 	kubeClientConfig := getKubeConfig(testKAS)
 
 	wardleCertDir, _ := os.MkdirTemp("", "test-integration-wardle-server")
 	defer os.RemoveAll(wardleCertDir)
 
-	directWardleClientConfig := runPreparedWardleServer(ctx, t, wardleOptions, wardleCertDir, wardlePort, setWardleFeatureGate, banFlunder, wardleEmulationVersion, kubeClientConfig, false)
+	directWardleClientConfig := runPreparedWardleServer(ctx, t, wardleOptions, wardleCertDir, wardlePort, setWardleFeatureGate, banFlunder, wardleEmulationVersionRaw, kubeClientConfig, false)
 
 	// now we're finally ready to test. These are what's run by default now
 	wardleDirectClient := client.NewForConfigOrDie(directWardleClientConfig)
@@ -699,7 +715,14 @@ func prepareAggregatedWardleAPIServer(ctx context.Context, t *testing.T, namespa
 		framework.SharedEtcd())
 	t.Cleanup(func() { testServer.TearDownFn() })
 
-	componentGlobalsRegistry := testServer.ServerOpts.Options.GenericServerRunOptions.ComponentGlobalsRegistry
+	// Create a new registry since the testServer's ComponentGlobalsRegistry is already Set(),
+	// and wardle server would try to Set() again in the test.
+	componentGlobalsRegistry := basecompatibility.NewComponentGlobalsRegistry()
+	_, _ = componentGlobalsRegistry.ComponentGlobalsOrRegister(
+		basecompatibility.DefaultKubeComponent,
+		utilcompatibility.DefaultKubeEffectiveVersionForTest(),
+		utilfeature.DefaultFeatureGate.DeepCopy(),
+	)
 	_, _ = componentGlobalsRegistry.ComponentGlobalsOrRegister(
 		apiserver.WardleComponentName, basecompatibility.NewEffectiveVersionFromString(wardleBinaryVersion, "", ""),
 		featuregate.NewVersionedFeatureGate(version.MustParse(wardleBinaryVersion)))