Merge pull request #132401 from togettoyou/refactor-admission-plugin-flags

Refactor: isolate flag registration to kube-apiserver to eliminate global state

cmd/kube-apiserver/app/options/globalflags.go

@@ -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")
-}

cmd/kube-apiserver/app/options/globalflags_test.go

@@ -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)
-	}
-}

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)
 	}

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. "+

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) {

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

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)
+}

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)
+}

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)
+}