From ae4a43de6dd51176434820db011eff110a556391 Mon Sep 17 00:00:00 2001 From: Junhao Zou Date: Wed, 16 Jul 2025 11:01:37 +0800 Subject: [PATCH] Refactor: isolate flag registration to kube-apiserver to eliminate global state --- cmd/kube-apiserver/app/options/globalflags.go | 37 ----------- .../app/options/globalflags_test.go | 64 ------------------- cmd/kube-apiserver/app/server.go | 1 - pkg/kubeapiserver/options/admission.go | 1 + pkg/kubeapiserver/options/plugins.go | 8 +++ .../defaulttolerationseconds/admission.go | 64 +++++++++++++------ .../admission_test.go | 15 ++++- .../defaulttolerationseconds_test.go | 16 ++++- test/integration/node/lifecycle_test.go | 15 ++++- 9 files changed, 95 insertions(+), 126 deletions(-) delete mode 100644 cmd/kube-apiserver/app/options/globalflags.go delete mode 100644 cmd/kube-apiserver/app/options/globalflags_test.go diff --git a/cmd/kube-apiserver/app/options/globalflags.go b/cmd/kube-apiserver/app/options/globalflags.go deleted file mode 100644 index bef513605b5..00000000000 --- a/cmd/kube-apiserver/app/options/globalflags.go +++ /dev/null @@ -1,37 +0,0 @@ -/* -Copyright 2018 The Kubernetes Authors. - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -package options - -import ( - "github.com/spf13/pflag" - - "k8s.io/component-base/cli/globalflag" - - // ensure libs have a chance to globally register their flags - _ "k8s.io/apiserver/pkg/admission" -) - -// AddCustomGlobalFlags explicitly registers flags that internal packages register -// against the global flagsets from "flag". We do this in order to prevent -// unwanted flags from leaking into the kube-apiserver's flagset. -func AddCustomGlobalFlags(fs *pflag.FlagSet) { - // Lookup flags in global flag set and re-register the values with our flagset. - - // Adds flags from k8s.io/apiserver/pkg/admission. - globalflag.Register(fs, "default-not-ready-toleration-seconds") - globalflag.Register(fs, "default-unreachable-toleration-seconds") -} diff --git a/cmd/kube-apiserver/app/options/globalflags_test.go b/cmd/kube-apiserver/app/options/globalflags_test.go deleted file mode 100644 index 20d21c0f411..00000000000 --- a/cmd/kube-apiserver/app/options/globalflags_test.go +++ /dev/null @@ -1,64 +0,0 @@ -/* -Copyright 2018 The Kubernetes Authors. - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -package options - -import ( - "flag" - "reflect" - "sort" - "strings" - "testing" - - "github.com/spf13/pflag" - - cliflag "k8s.io/component-base/cli/flag" - "k8s.io/component-base/cli/globalflag" - "k8s.io/component-base/logs" -) - -func TestAddCustomGlobalFlags(t *testing.T) { - namedFlagSets := &cliflag.NamedFlagSets{} - - // Note that we will register all flags (including klog flags) into the same - // flag set. This allows us to test against all global flags from - // flags.CommandLine. - nfs := namedFlagSets.FlagSet("test") - nfs.SetNormalizeFunc(cliflag.WordSepNormalizeFunc) - globalflag.AddGlobalFlags(nfs, "test-cmd") - AddCustomGlobalFlags(nfs) - - actualFlag := []string{} - nfs.VisitAll(func(flag *pflag.Flag) { - actualFlag = append(actualFlag, flag.Name) - }) - - // Get all flags from flags.CommandLine, except flag `test.*`. - wantedFlag := []string{"help"} - pflag.CommandLine.AddGoFlagSet(flag.CommandLine) - logs.AddFlags(pflag.CommandLine) - normalizeFunc := nfs.GetNormalizeFunc() - pflag.VisitAll(func(flag *pflag.Flag) { - if !strings.Contains(flag.Name, "test.") { - wantedFlag = append(wantedFlag, string(normalizeFunc(nfs, flag.Name))) - } - }) - sort.Strings(wantedFlag) - - if !reflect.DeepEqual(wantedFlag, actualFlag) { - t.Errorf("[Default]: expected %+v, got %+v", wantedFlag, actualFlag) - } -} diff --git a/cmd/kube-apiserver/app/server.go b/cmd/kube-apiserver/app/server.go index 61ef5e90a6e..4dc9d1f3e89 100644 --- a/cmd/kube-apiserver/app/server.go +++ b/cmd/kube-apiserver/app/server.go @@ -132,7 +132,6 @@ cluster's shared state through which all other components interact.`, } verflag.AddFlags(namedFlagSets.FlagSet("global")) globalflag.AddGlobalFlags(namedFlagSets.FlagSet("global"), cmd.Name(), logs.SkipLoggingConfigurationFlags()) - options.AddCustomGlobalFlags(namedFlagSets.FlagSet("generic")) for _, f := range namedFlagSets.FlagSets { fs.AddFlagSet(f) } diff --git a/pkg/kubeapiserver/options/admission.go b/pkg/kubeapiserver/options/admission.go index c58c2e9080a..d70df9742a6 100644 --- a/pkg/kubeapiserver/options/admission.go +++ b/pkg/kubeapiserver/options/admission.go @@ -70,6 +70,7 @@ func (a *AdmissionOptions) AddFlags(fs *pflag.FlagSet) { if a == nil { return } + registerAllAdmissionPluginFlags(fs) fs.StringSliceVar(&a.PluginNames, "admission-control", a.PluginNames, ""+ "Admission is divided into two phases. "+ "In the first phase, only mutating admission plugins run. "+ diff --git a/pkg/kubeapiserver/options/plugins.go b/pkg/kubeapiserver/options/plugins.go index 0dccb55e2a4..d3bf0503c1e 100644 --- a/pkg/kubeapiserver/options/plugins.go +++ b/pkg/kubeapiserver/options/plugins.go @@ -20,6 +20,8 @@ package options // This should probably be part of some configuration fed into the build for a // given binary target. import ( + "github.com/spf13/pflag" + mutatingadmissionpolicy "k8s.io/apiserver/pkg/admission/plugin/policy/mutating" validatingadmissionpolicy "k8s.io/apiserver/pkg/admission/plugin/policy/validating" @@ -107,6 +109,12 @@ var AllOrderedPlugins = []string{ deny.PluginName, // AlwaysDeny } +// registerAllAdmissionPluginFlags registers legacy CLI flag options for admission plugins. +// No new plugins should use CLI flags to configure themselves. +func registerAllAdmissionPluginFlags(fs *pflag.FlagSet) { + defaulttolerationseconds.RegisterFlags(fs) +} + // RegisterAllAdmissionPlugins registers all admission plugins. // The order of registration is irrelevant, see AllOrderedPlugins for execution order. func RegisterAllAdmissionPlugins(plugins *admission.Plugins) { diff --git a/plugin/pkg/admission/defaulttolerationseconds/admission.go b/plugin/pkg/admission/defaulttolerationseconds/admission.go index a699d8d3178..08b22cdb9b1 100644 --- a/plugin/pkg/admission/defaulttolerationseconds/admission.go +++ b/plugin/pkg/admission/defaulttolerationseconds/admission.go @@ -18,13 +18,15 @@ package defaulttolerationseconds import ( "context" - "flag" "fmt" "io" + "github.com/spf13/pflag" v1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apiserver/pkg/admission" + "k8s.io/apiserver/pkg/admission/initializer" + "k8s.io/component-base/featuregate" api "k8s.io/kubernetes/pkg/apis/core" ) @@ -32,28 +34,18 @@ import ( const PluginName = "DefaultTolerationSeconds" var ( - defaultNotReadyTolerationSeconds = flag.Int64("default-not-ready-toleration-seconds", 300, + defaultNotReadyTolerationSeconds = int64(300) + defaultUnreachableTolerationSeconds = int64(300) +) + +func RegisterFlags(fs *pflag.FlagSet) { + fs.Int64Var(&defaultNotReadyTolerationSeconds, "default-not-ready-toleration-seconds", defaultNotReadyTolerationSeconds, "Indicates the tolerationSeconds of the toleration for notReady:NoExecute"+ " that is added by default to every pod that does not already have such a toleration.") - - defaultUnreachableTolerationSeconds = flag.Int64("default-unreachable-toleration-seconds", 300, + fs.Int64Var(&defaultUnreachableTolerationSeconds, "default-unreachable-toleration-seconds", defaultUnreachableTolerationSeconds, "Indicates the tolerationSeconds of the toleration for unreachable:NoExecute"+ " that is added by default to every pod that does not already have such a toleration.") - - notReadyToleration = api.Toleration{ - Key: v1.TaintNodeNotReady, - Operator: api.TolerationOpExists, - Effect: api.TaintEffectNoExecute, - TolerationSeconds: defaultNotReadyTolerationSeconds, - } - - unreachableToleration = api.Toleration{ - Key: v1.TaintNodeUnreachable, - Operator: api.TolerationOpExists, - Effect: api.TaintEffectNoExecute, - TolerationSeconds: defaultUnreachableTolerationSeconds, - } -) +} // Register registers a plugin func Register(plugins *admission.Plugins) { @@ -70,9 +62,12 @@ func Register(plugins *admission.Plugins) { // or `unreachable:NoExecute`, the plugin won't touch it. type Plugin struct { *admission.Handler + notReadyToleration api.Toleration + unreachableToleration api.Toleration } var _ admission.MutationInterface = &Plugin{} +var _ initializer.WantsFeatures = &Plugin{} // NewDefaultTolerationSeconds creates a new instance of the DefaultTolerationSeconds admission controller func NewDefaultTolerationSeconds() *Plugin { @@ -81,6 +76,33 @@ func NewDefaultTolerationSeconds() *Plugin { } } +// InspectFeatureGates runs after command-line flags have been parsed +func (p *Plugin) InspectFeatureGates(featureGates featuregate.FeatureGate) { + p.notReadyToleration = api.Toleration{ + Key: v1.TaintNodeNotReady, + Operator: api.TolerationOpExists, + Effect: api.TaintEffectNoExecute, + TolerationSeconds: &defaultNotReadyTolerationSeconds, + } + p.unreachableToleration = api.Toleration{ + Key: v1.TaintNodeUnreachable, + Operator: api.TolerationOpExists, + Effect: api.TaintEffectNoExecute, + TolerationSeconds: &defaultUnreachableTolerationSeconds, + } +} + +// ValidateInitialization validates the Plugin was initialized properly +func (p *Plugin) ValidateInitialization() error { + if p.notReadyToleration == (api.Toleration{}) { + return fmt.Errorf("%s was not initialized correctly, notReadyToleration is not set", PluginName) + } + if p.unreachableToleration == (api.Toleration{}) { + return fmt.Errorf("%s was not initialized correctly, unreachableToleration is not set", PluginName) + } + return nil +} + // Admit makes an admission decision based on the request attributes func (p *Plugin) Admit(ctx context.Context, attributes admission.Attributes, o admission.ObjectInterfaces) (err error) { if attributes.GetResource().GroupResource() != api.Resource("pods") { @@ -114,11 +136,11 @@ func (p *Plugin) Admit(ctx context.Context, attributes admission.Attributes, o a } if !toleratesNodeNotReady { - pod.Spec.Tolerations = append(pod.Spec.Tolerations, notReadyToleration) + pod.Spec.Tolerations = append(pod.Spec.Tolerations, p.notReadyToleration) } if !toleratesNodeUnreachable { - pod.Spec.Tolerations = append(pod.Spec.Tolerations, unreachableToleration) + pod.Spec.Tolerations = append(pod.Spec.Tolerations, p.unreachableToleration) } return nil diff --git a/plugin/pkg/admission/defaulttolerationseconds/admission_test.go b/plugin/pkg/admission/defaulttolerationseconds/admission_test.go index 4264dd97006..ca170305f55 100644 --- a/plugin/pkg/admission/defaulttolerationseconds/admission_test.go +++ b/plugin/pkg/admission/defaulttolerationseconds/admission_test.go @@ -22,6 +22,7 @@ import ( v1 "k8s.io/api/core/v1" "k8s.io/apiserver/pkg/admission" + "k8s.io/apiserver/pkg/admission/initializer" admissiontesting "k8s.io/apiserver/pkg/admission/testing" api "k8s.io/kubernetes/pkg/apis/core" "k8s.io/kubernetes/pkg/apis/core/helper" @@ -34,7 +35,11 @@ func TestForgivenessAdmission(t *testing.T) { return &s } - handler := admissiontesting.WithReinvocationTesting(t, NewDefaultTolerationSeconds()) + plugin, err := newHandlerForTest() + if err != nil { + t.Errorf("unexpected error initializing handler: %v", err) + } + handler := admissiontesting.WithReinvocationTesting(t, plugin) // NOTE: for anyone who want to modify this test, the order of tolerations matters! tests := []struct { description string @@ -291,3 +296,11 @@ func TestHandles(t *testing.T) { } } } + +// newHandlerForTest returns a handler configured for testing. +func newHandlerForTest() (*Plugin, error) { + handler := NewDefaultTolerationSeconds() + pluginInitializer := initializer.New(nil, nil, nil, nil, nil, nil, nil) + pluginInitializer.Initialize(handler) + return handler, admission.ValidateInitialization(handler) +} diff --git a/test/integration/defaulttolerationseconds/defaulttolerationseconds_test.go b/test/integration/defaulttolerationseconds/defaulttolerationseconds_test.go index 2b88cec8672..ccfe0f97a80 100644 --- a/test/integration/defaulttolerationseconds/defaulttolerationseconds_test.go +++ b/test/integration/defaulttolerationseconds/defaulttolerationseconds_test.go @@ -21,6 +21,8 @@ import ( v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apiserver/pkg/admission" + "k8s.io/apiserver/pkg/admission/initializer" "k8s.io/kubernetes/pkg/apis/core/helper" "k8s.io/kubernetes/pkg/controlplane" "k8s.io/kubernetes/plugin/pkg/admission/defaulttolerationseconds" @@ -30,10 +32,14 @@ import ( func TestAdmission(t *testing.T) { tCtx := ktesting.Init(t) + handler, err := newHandlerForTest() + if err != nil { + t.Errorf("unexpected error initializing handler: %v", err) + } client, _, tearDownFn := framework.StartTestServer(tCtx, t, framework.TestServerSetup{ ModifyServerConfig: func(cfg *controlplane.Config) { cfg.ControlPlane.Generic.EnableProfiling = true - cfg.ControlPlane.Generic.AdmissionControl = defaulttolerationseconds.NewDefaultTolerationSeconds() + cfg.ControlPlane.Generic.AdmissionControl = handler }, }) defer tearDownFn() @@ -100,3 +106,11 @@ func TestAdmission(t *testing.T) { t.Fatalf("unexpected tolerations: %v\n", updatedPod.Spec.Tolerations) } } + +// newHandlerForTest returns a handler configured for testing. +func newHandlerForTest() (*defaulttolerationseconds.Plugin, error) { + handler := defaulttolerationseconds.NewDefaultTolerationSeconds() + pluginInitializer := initializer.New(nil, nil, nil, nil, nil, nil, nil) + pluginInitializer.Initialize(handler) + return handler, admission.ValidateInitialization(handler) +} diff --git a/test/integration/node/lifecycle_test.go b/test/integration/node/lifecycle_test.go index 8c8b07fb450..e79f1fe5552 100644 --- a/test/integration/node/lifecycle_test.go +++ b/test/integration/node/lifecycle_test.go @@ -27,6 +27,7 @@ import ( "k8s.io/apimachinery/pkg/util/version" "k8s.io/apimachinery/pkg/util/wait" "k8s.io/apiserver/pkg/admission" + "k8s.io/apiserver/pkg/admission/initializer" "k8s.io/apiserver/pkg/util/feature" "k8s.io/client-go/informers" clientset "k8s.io/client-go/kubernetes" @@ -318,9 +319,13 @@ func TestTaintBasedEvictions(t *testing.T) { // Build admission chain handler. podTolerations := podtolerationrestriction.NewPodTolerationsPlugin(&pluginapi.Configuration{}) + defaultTolerationSeconds, err := newHandlerForTest() + if err != nil { + t.Errorf("unexpected error initializing handler: %v", err) + } admission := admission.NewChainHandler( podTolerations, - defaulttolerationseconds.NewDefaultTolerationSeconds(), + defaultTolerationSeconds, ) for _, test := range tests { t.Run(test.name, func(t *testing.T) { @@ -458,3 +463,11 @@ func TestTaintBasedEvictions(t *testing.T) { }) } } + +// newHandlerForTest returns a handler configured for testing. +func newHandlerForTest() (*defaulttolerationseconds.Plugin, error) { + handler := defaulttolerationseconds.NewDefaultTolerationSeconds() + pluginInitializer := initializer.New(nil, nil, nil, nil, nil, nil, nil) + pluginInitializer.Initialize(handler) + return handler, admission.ValidateInitialization(handler) +}