Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

remove global variable dependency from admission plugins #84813

Merged
merged 2 commits into from Nov 12, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions cmd/kube-apiserver/app/aggregator.go
Expand Up @@ -38,6 +38,7 @@ import (
genericapiserver "k8s.io/apiserver/pkg/server"
"k8s.io/apiserver/pkg/server/healthz"
genericoptions "k8s.io/apiserver/pkg/server/options"
"k8s.io/apiserver/pkg/util/feature"
utilfeature "k8s.io/apiserver/pkg/util/feature"
kubeexternalinformers "k8s.io/client-go/informers"
"k8s.io/client-go/tools/cache"
Expand Down Expand Up @@ -73,6 +74,7 @@ func createAggregatorConfig(
&genericConfig,
externalInformers,
genericConfig.LoopbackClientConfig,
feature.DefaultFeatureGate,
pluginInitializers...)
if err != nil {
return nil, err
Expand Down
2 changes: 2 additions & 0 deletions cmd/kube-apiserver/app/apiextensions.go
Expand Up @@ -30,6 +30,7 @@ import (
"k8s.io/apiserver/pkg/features"
genericapiserver "k8s.io/apiserver/pkg/server"
genericoptions "k8s.io/apiserver/pkg/server/options"
"k8s.io/apiserver/pkg/util/feature"
utilfeature "k8s.io/apiserver/pkg/util/feature"
"k8s.io/apiserver/pkg/util/webhook"
kubeexternalinformers "k8s.io/client-go/informers"
Expand Down Expand Up @@ -57,6 +58,7 @@ func createAPIExtensionsConfig(
&genericConfig,
externalInformers,
genericConfig.LoopbackClientConfig,
feature.DefaultFeatureGate,
pluginInitializers...)
if err != nil {
return nil, err
Expand Down
2 changes: 2 additions & 0 deletions cmd/kube-apiserver/app/server.go
Expand Up @@ -48,6 +48,7 @@ import (
serveroptions "k8s.io/apiserver/pkg/server/options"
serverstorage "k8s.io/apiserver/pkg/server/storage"
"k8s.io/apiserver/pkg/storage/etcd3/preflight"
"k8s.io/apiserver/pkg/util/feature"
utilfeature "k8s.io/apiserver/pkg/util/feature"
"k8s.io/apiserver/pkg/util/term"
"k8s.io/apiserver/pkg/util/webhook"
Expand Down Expand Up @@ -511,6 +512,7 @@ func buildGenericConfig(
genericConfig,
versionedInformers,
kubeClientConfig,
feature.DefaultFeatureGate,
pluginInitializers...)
if err != nil {
lastErr = fmt.Errorf("failed to initialize admission: %v", err)
Expand Down
1 change: 1 addition & 0 deletions pkg/kubeapiserver/options/BUILD
Expand Up @@ -64,6 +64,7 @@ go_library(
"//staging/src/k8s.io/client-go/informers:go_default_library",
"//staging/src/k8s.io/client-go/rest:go_default_library",
"//staging/src/k8s.io/component-base/cli/flag:go_default_library",
"//staging/src/k8s.io/component-base/featuregate:go_default_library",
"//vendor/github.com/spf13/pflag:go_default_library",
"//vendor/k8s.io/klog:go_default_library",
],
Expand Down
4 changes: 3 additions & 1 deletion pkg/kubeapiserver/options/admission.go
Expand Up @@ -28,6 +28,7 @@ import (
genericoptions "k8s.io/apiserver/pkg/server/options"
"k8s.io/client-go/informers"
"k8s.io/client-go/rest"
"k8s.io/component-base/featuregate"
)

// AdmissionOptions holds the admission options.
Expand Down Expand Up @@ -107,6 +108,7 @@ func (a *AdmissionOptions) ApplyTo(
c *server.Config,
informers informers.SharedInformerFactory,
kubeAPIServerClientConfig *rest.Config,
features featuregate.FeatureGate,
pluginInitializers ...admission.PluginInitializer,
) error {
if a == nil {
Expand All @@ -118,7 +120,7 @@ func (a *AdmissionOptions) ApplyTo(
a.GenericAdmission.EnablePlugins, a.GenericAdmission.DisablePlugins = computePluginNames(a.PluginNames, a.GenericAdmission.RecommendedPluginOrder)
}

return a.GenericAdmission.ApplyTo(c, informers, kubeAPIServerClientConfig, pluginInitializers...)
return a.GenericAdmission.ApplyTo(c, informers, kubeAPIServerClientConfig, features, pluginInitializers...)
}

// explicitly disable all plugins that are not in the enabled list
Expand Down
2 changes: 1 addition & 1 deletion plugin/pkg/admission/gc/gc_admission_test.go
Expand Up @@ -101,7 +101,7 @@ func newGCPermissionsEnforcement() (*gcPermissionsEnforcement, error) {
whiteList: whiteList,
}

genericPluginInitializer := initializer.New(nil, nil, fakeAuthorizer{})
genericPluginInitializer := initializer.New(nil, nil, fakeAuthorizer{}, nil)
fakeDiscoveryClient := &fakediscovery.FakeDiscovery{Fake: &coretesting.Fake{}}
fakeDiscoveryClient.Resources = []*metav1.APIResourceList{
{
Expand Down
2 changes: 1 addition & 1 deletion plugin/pkg/admission/limitranger/admission_test.go
Expand Up @@ -790,7 +790,7 @@ func newHandlerForTest(c clientset.Interface) (*LimitRanger, informers.SharedInf
if err != nil {
return nil, f, err
}
pluginInitializer := genericadmissioninitializer.New(c, f, nil)
pluginInitializer := genericadmissioninitializer.New(c, f, nil, nil)
pluginInitializer.Initialize(handler)
err = admission.ValidateInitialization(handler)
return handler, f, err
Expand Down
Expand Up @@ -41,7 +41,7 @@ import (
func newHandlerForTest(c clientset.Interface) (admission.MutationInterface, informers.SharedInformerFactory, error) {
f := informers.NewSharedInformerFactory(c, 5*time.Minute)
handler := NewProvision()
pluginInitializer := genericadmissioninitializer.New(c, f, nil)
pluginInitializer := genericadmissioninitializer.New(c, f, nil, nil)
pluginInitializer.Initialize(handler)
err := admission.ValidateInitialization(handler)
return handler, f, err
Expand Down
2 changes: 1 addition & 1 deletion plugin/pkg/admission/namespace/exists/admission_test.go
Expand Up @@ -39,7 +39,7 @@ import (
func newHandlerForTest(c kubernetes.Interface) (admission.ValidationInterface, informers.SharedInformerFactory, error) {
f := informers.NewSharedInformerFactory(c, 5*time.Minute)
handler := NewExists()
pluginInitializer := genericadmissioninitializer.New(c, f, nil)
pluginInitializer := genericadmissioninitializer.New(c, f, nil, nil)
pluginInitializer.Initialize(handler)
err := admission.ValidateInitialization(handler)
return handler, f, err
Expand Down
2 changes: 1 addition & 1 deletion plugin/pkg/admission/noderestriction/BUILD
Expand Up @@ -29,7 +29,6 @@ go_library(
"//staging/src/k8s.io/apimachinery/pkg/util/sets:go_default_library",
"//staging/src/k8s.io/apiserver/pkg/admission:go_default_library",
"//staging/src/k8s.io/apiserver/pkg/admission/initializer:go_default_library",
"//staging/src/k8s.io/apiserver/pkg/util/feature:go_default_library",
"//staging/src/k8s.io/client-go/informers:go_default_library",
"//staging/src/k8s.io/client-go/listers/core/v1:go_default_library",
"//staging/src/k8s.io/component-base/featuregate:go_default_library",
Expand All @@ -54,6 +53,7 @@ go_test(
"//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library",
"//staging/src/k8s.io/apimachinery/pkg/runtime/schema:go_default_library",
"//staging/src/k8s.io/apimachinery/pkg/types:go_default_library",
"//staging/src/k8s.io/apimachinery/pkg/util/runtime:go_default_library",
"//staging/src/k8s.io/apimachinery/pkg/util/sets:go_default_library",
"//staging/src/k8s.io/apiserver/pkg/admission:go_default_library",
"//staging/src/k8s.io/apiserver/pkg/authentication/user:go_default_library",
Expand Down
26 changes: 17 additions & 9 deletions plugin/pkg/admission/noderestriction/admission.go
Expand Up @@ -31,7 +31,6 @@ import (
"k8s.io/apimachinery/pkg/util/sets"
"k8s.io/apiserver/pkg/admission"
apiserveradmission "k8s.io/apiserver/pkg/admission/initializer"
utilfeature "k8s.io/apiserver/pkg/util/feature"
"k8s.io/client-go/informers"
corev1lister "k8s.io/client-go/listers/core/v1"
"k8s.io/component-base/featuregate"
Expand Down Expand Up @@ -63,7 +62,6 @@ func NewPlugin(nodeIdentifier nodeidentifier.NodeIdentifier) *Plugin {
return &Plugin{
Handler: admission.NewHandler(admission.Create, admission.Update, admission.Delete),
nodeIdentifier: nodeIdentifier,
features: utilfeature.DefaultFeatureGate,
}
}

Expand All @@ -72,15 +70,25 @@ type Plugin struct {
*admission.Handler
nodeIdentifier nodeidentifier.NodeIdentifier
podsGetter corev1lister.PodLister
// allows overriding for testing
features featuregate.FeatureGate

tokenRequestEnabled bool
csiNodeInfoEnabled bool
expandPersistentVolumesEnabled bool
}

var (
_ = admission.Interface(&Plugin{})
_ = apiserveradmission.WantsExternalKubeInformerFactory(&Plugin{})
_ admission.Interface = &Plugin{}
_ apiserveradmission.WantsExternalKubeInformerFactory = &Plugin{}
_ apiserveradmission.WantsFeatures = &Plugin{}
)

// InspectFeatureGates allows setting bools without taking a dep on a global variable
func (p *Plugin) InspectFeatureGates(featureGates featuregate.FeatureGate) {
p.tokenRequestEnabled = featureGates.Enabled(features.TokenRequest)
p.csiNodeInfoEnabled = featureGates.Enabled(features.CSINodeInfo)
p.expandPersistentVolumesEnabled = featureGates.Enabled(features.ExpandPersistentVolumes)
}

// SetExternalKubeInformerFactory registers an informer factory into Plugin
func (p *Plugin) SetExternalKubeInformerFactory(f informers.SharedInformerFactory) {
p.podsGetter = f.Core().V1().Pods().Lister()
Expand Down Expand Up @@ -145,7 +153,7 @@ func (p *Plugin) Admit(ctx context.Context, a admission.Attributes, o admission.
}

case svcacctResource:
if p.features.Enabled(features.TokenRequest) {
if p.tokenRequestEnabled {
return p.admitServiceAccount(nodeName, a)
}
return nil
Expand All @@ -154,7 +162,7 @@ func (p *Plugin) Admit(ctx context.Context, a admission.Attributes, o admission.
return p.admitLease(nodeName, a)

case csiNodeResource:
if p.features.Enabled(features.CSINodeInfo) {
if p.csiNodeInfoEnabled {
return p.admitCSINode(nodeName, a)
}
return admission.NewForbidden(a, fmt.Errorf("disabled by feature gates %s", features.CSINodeInfo))
Expand Down Expand Up @@ -294,7 +302,7 @@ func (p *Plugin) admitPodEviction(nodeName string, a admission.Attributes) error
func (p *Plugin) admitPVCStatus(nodeName string, a admission.Attributes) error {
switch a.GetOperation() {
case admission.Update:
if !p.features.Enabled(features.ExpandPersistentVolumes) {
if !p.expandPersistentVolumesEnabled {
return admission.NewForbidden(a, fmt.Errorf("node %q is not allowed to update persistentvolumeclaim metadata", nodeName))
}

Expand Down
26 changes: 14 additions & 12 deletions plugin/pkg/admission/noderestriction/admission_test.go
Expand Up @@ -28,6 +28,7 @@ import (
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime/schema"
"k8s.io/apimachinery/pkg/types"
utilruntime "k8s.io/apimachinery/pkg/util/runtime"
"k8s.io/apimachinery/pkg/util/sets"
"k8s.io/apiserver/pkg/admission"
"k8s.io/apiserver/pkg/authentication/user"
Expand All @@ -53,18 +54,19 @@ var (
)

func init() {
if err := trEnabledFeature.Add(map[featuregate.Feature]featuregate.FeatureSpec{features.TokenRequest: {Default: true}}); err != nil {
panic(err)
}
if err := trDisabledFeature.Add(map[featuregate.Feature]featuregate.FeatureSpec{features.TokenRequest: {Default: false}}); err != nil {
panic(err)
}
if err := csiNodeInfoEnabledFeature.Add(map[featuregate.Feature]featuregate.FeatureSpec{features.CSINodeInfo: {Default: true}}); err != nil {
panic(err)
}
if err := csiNodeInfoDisabledFeature.Add(map[featuregate.Feature]featuregate.FeatureSpec{features.CSINodeInfo: {Default: false}}); err != nil {
panic(err)
// all features need to be set on all featuregates for the tests. We set everything and then then the if's below override it.
relevantFeatures := map[featuregate.Feature]featuregate.FeatureSpec{
features.TokenRequest: {Default: false},
features.CSINodeInfo: {Default: false},
features.ExpandPersistentVolumes: {Default: false},
}
liggitt marked this conversation as resolved.
Show resolved Hide resolved
utilruntime.Must(trEnabledFeature.Add(relevantFeatures))
utilruntime.Must(trDisabledFeature.Add(relevantFeatures))
utilruntime.Must(csiNodeInfoEnabledFeature.Add(relevantFeatures))
utilruntime.Must(csiNodeInfoDisabledFeature.Add(relevantFeatures))

utilruntime.Must(trEnabledFeature.SetFromMap(map[string]bool{string(features.TokenRequest): true}))
utilruntime.Must(csiNodeInfoEnabledFeature.SetFromMap(map[string]bool{string(features.CSINodeInfo): true}))
}

func makeTestPod(namespace, name, node string, mirror bool) (*api.Pod, *corev1.Pod) {
Expand Down Expand Up @@ -1233,7 +1235,7 @@ func Test_nodePlugin_Admit(t *testing.T) {
t.Run(tt.name, func(t *testing.T) {
c := NewPlugin(nodeidentifier.NewDefaultNodeIdentifier())
if tt.features != nil {
c.features = tt.features
c.InspectFeatureGates(tt.features)
}
c.podsGetter = tt.podsGetter
err := c.Admit(context.TODO(), tt.attributes, nil)
Expand Down
2 changes: 1 addition & 1 deletion plugin/pkg/admission/podnodeselector/admission_test.go
Expand Up @@ -198,7 +198,7 @@ func TestHandles(t *testing.T) {
func newHandlerForTest(c kubernetes.Interface) (*Plugin, informers.SharedInformerFactory, error) {
f := informers.NewSharedInformerFactory(c, 5*time.Minute)
handler := NewPodNodeSelector(nil)
pluginInitializer := genericadmissioninitializer.New(c, f, nil)
pluginInitializer := genericadmissioninitializer.New(c, f, nil, nil)
pluginInitializer.Initialize(handler)
err := admission.ValidateInitialization(handler)
return handler, f, err
Expand Down
Expand Up @@ -356,7 +356,7 @@ func newHandlerForTest(c kubernetes.Interface) (*Plugin, informers.SharedInforme
return nil, nil, err
}
handler := NewPodTolerationsPlugin(pluginConfig)
pluginInitializer := genericadmissioninitializer.New(c, f, nil)
pluginInitializer := genericadmissioninitializer.New(c, f, nil, nil)
pluginInitializer.Initialize(handler)
err = admission.ValidateInitialization(handler)
return handler, f, err
Expand Down
3 changes: 0 additions & 3 deletions plugin/pkg/admission/serviceaccount/BUILD
Expand Up @@ -27,7 +27,6 @@ go_library(
"//staging/src/k8s.io/apiserver/pkg/admission:go_default_library",
"//staging/src/k8s.io/apiserver/pkg/admission/initializer:go_default_library",
"//staging/src/k8s.io/apiserver/pkg/storage/names:go_default_library",
"//staging/src/k8s.io/apiserver/pkg/util/feature:go_default_library",
"//staging/src/k8s.io/client-go/informers:go_default_library",
"//staging/src/k8s.io/client-go/kubernetes:go_default_library",
"//staging/src/k8s.io/client-go/listers/core/v1:go_default_library",
Expand All @@ -42,7 +41,6 @@ go_test(
deps = [
"//pkg/apis/core:go_default_library",
"//pkg/controller:go_default_library",
"//pkg/features:go_default_library",
"//pkg/kubelet/types:go_default_library",
"//staging/src/k8s.io/api/core/v1:go_default_library",
"//staging/src/k8s.io/apimachinery/pkg/api/errors:go_default_library",
Expand All @@ -54,7 +52,6 @@ go_test(
"//staging/src/k8s.io/client-go/kubernetes/fake:go_default_library",
"//staging/src/k8s.io/client-go/listers/core/v1:go_default_library",
"//staging/src/k8s.io/client-go/tools/cache:go_default_library",
"//staging/src/k8s.io/component-base/featuregate:go_default_library",
"//vendor/github.com/stretchr/testify/assert:go_default_library",
],
)
Expand Down
21 changes: 12 additions & 9 deletions plugin/pkg/admission/serviceaccount/admission.go
Expand Up @@ -34,14 +34,13 @@ import (
"k8s.io/apiserver/pkg/admission"
genericadmissioninitializer "k8s.io/apiserver/pkg/admission/initializer"
"k8s.io/apiserver/pkg/storage/names"
utilfeature "k8s.io/apiserver/pkg/util/feature"
"k8s.io/client-go/informers"
"k8s.io/client-go/kubernetes"
corev1listers "k8s.io/client-go/listers/core/v1"
"k8s.io/component-base/featuregate"
podutil "k8s.io/kubernetes/pkg/api/pod"
api "k8s.io/kubernetes/pkg/apis/core"
kubefeatures "k8s.io/kubernetes/pkg/features"
"k8s.io/kubernetes/pkg/features"
"k8s.io/kubernetes/pkg/serviceaccount"
)

Expand Down Expand Up @@ -92,11 +91,12 @@ type Plugin struct {

generateName func(string) string

featureGate featuregate.FeatureGate
boundServiceAccountTokenVolume bool
}

var _ admission.MutationInterface = &Plugin{}
var _ admission.ValidationInterface = &Plugin{}
var _ genericadmissioninitializer.WantsFeatures = &Plugin{}
var _ = genericadmissioninitializer.WantsExternalKubeClientSet(&Plugin{})
var _ = genericadmissioninitializer.WantsExternalKubeInformerFactory(&Plugin{})

Expand All @@ -117,11 +117,14 @@ func NewServiceAccount() *Plugin {
RequireAPIToken: true,

generateName: names.SimpleNameGenerator.GenerateName,

featureGate: utilfeature.DefaultFeatureGate,
}
}

// InspectFeatureGates allows setting bools without taking a dep on a global variable
func (s *Plugin) InspectFeatureGates(featureGates featuregate.FeatureGate) {
s.boundServiceAccountTokenVolume = featureGates.Enabled(features.BoundServiceAccountTokenVolume)
}

// SetExternalKubeClientSet sets the client for the plugin
func (s *Plugin) SetExternalKubeClientSet(cl kubernetes.Interface) {
s.client = cl
Expand Down Expand Up @@ -443,8 +446,8 @@ func (s *Plugin) mountServiceAccountToken(serviceAccount *corev1.ServiceAccount,
allVolumeNames := sets.NewString()
for _, volume := range pod.Spec.Volumes {
allVolumeNames.Insert(volume.Name)
if (!s.featureGate.Enabled(kubefeatures.BoundServiceAccountTokenVolume) && volume.Secret != nil && volume.Secret.SecretName == serviceAccountToken) ||
(s.featureGate.Enabled(kubefeatures.BoundServiceAccountTokenVolume) && strings.HasPrefix(volume.Name, ServiceAccountVolumeName+"-")) {
if (!s.boundServiceAccountTokenVolume && volume.Secret != nil && volume.Secret.SecretName == serviceAccountToken) ||
(s.boundServiceAccountTokenVolume && strings.HasPrefix(volume.Name, ServiceAccountVolumeName+"-")) {
tokenVolumeName = volume.Name
hasTokenVolume = true
break
Expand All @@ -453,7 +456,7 @@ func (s *Plugin) mountServiceAccountToken(serviceAccount *corev1.ServiceAccount,

// Determine a volume name for the ServiceAccountTokenSecret in case we need it
if len(tokenVolumeName) == 0 {
if s.featureGate.Enabled(kubefeatures.BoundServiceAccountTokenVolume) {
if s.boundServiceAccountTokenVolume {
tokenVolumeName = s.generateName(ServiceAccountVolumeName + "-")
} else {
// Try naming the volume the same as the serviceAccountToken, and uniquify if needed
Expand Down Expand Up @@ -510,7 +513,7 @@ func (s *Plugin) mountServiceAccountToken(serviceAccount *corev1.ServiceAccount,
}

func (s *Plugin) createVolume(tokenVolumeName, secretName string) api.Volume {
if s.featureGate.Enabled(kubefeatures.BoundServiceAccountTokenVolume) {
if s.boundServiceAccountTokenVolume {
return api.Volume{
Name: tokenVolumeName,
VolumeSource: api.VolumeSource{
Expand Down