remove global variable dependency from admission plugins #84813

deads2k · 2019-11-05T19:54:26Z

Adds an admission plugin initializer that passes a set of FeatureGates to the admission plugins during initialization that can then be used to set bool values and avoid coupling global variables together.

func (a *admissionPlugin) InspectFeatureGates(features featuregate.FeatureGate){
    a.myFeatureIsOn = features.Enabled("my-feature")
}

/kind cleanup
/priority important-soon
@kubernetes/sig-api-machinery-pr-reviews

@ravisantoshgudimetla this is how to remove the global dep from admission plugins

NONE

liggitt · 2019-11-05T20:46:24Z

cmd/kube-apiserver/app/aggregator.go

@@ -73,6 +74,7 @@ func createAggregatorConfig(
 		&genericConfig,
 		externalInformers,
 		genericConfig.LoopbackClientConfig,
+		feature.DefaultMutableFeatureGate,


pass in feature.DefaultFeatureGate, not the mutable one? (comment applies everywhere)

liggitt · 2019-11-05T20:52:56Z

staging/src/k8s.io/apiserver/pkg/server/options/recommended.go

@@ -65,6 +68,7 @@ func NewRecommendedOptions(prefix string, codec runtime.Codec, processInfo *Proc
 		Audit:                      NewAuditOptions(),
 		Features:                   NewFeatureOptions(),
 		CoreAPI:                    NewCoreAPIOptions(),
+		FeatureGate:                featuregate.NewFeatureGate(), // empty default features


Trying to think what the implications of this are... having multiple feature gate instances floating around seems bad. Are we overriding this with our existing util featureGate buckets anywhere?

Trying to think what the implications of this are... having multiple feature gate instances floating around seems bad. Are we overriding this with our existing util featureGate buckets anywhere?

kube-apiserver does. I could choose to reference the on in the apiserver package.

fixing the feature gate impl to error if an unregistered feature is interrogated seems like a good path forward. #84865

it is a little strange that the default here will not have the generic apiserver features registered

it is a little strange that the default here will not have the generic apiserver features registered

to my knowledge, our recommend options don't include featuregates

kubernetes/staging/src/k8s.io/apiserver/pkg/features/kube_features.go

Lines 157 to 167 in 62f66ea

APIResponseCompression: {Default: true, PreRelease: featuregate.Beta},

APIListChunking: {Default: true, PreRelease: featuregate.Beta},

DryRun: {Default: true, PreRelease: featuregate.Beta},

RemainingItemCount: {Default: true, PreRelease: featuregate.Beta},

ServerSideApply: {Default: true, PreRelease: featuregate.Beta},

StorageVersionHash: {Default: true, PreRelease: featuregate.Beta},

WinOverlay: {Default: false, PreRelease: featuregate.Alpha},

WinDSR: {Default: false, PreRelease: featuregate.Alpha},

WatchBookmark: {Default: true, PreRelease: featuregate.GA, LockToDefault: true},

RequestManagement: {Default: false, PreRelease: featuregate.Alpha},

RemoveSelfLink: {Default: false, PreRelease: featuregate.Alpha},

those?

We have them defined, but we don't actually expose a way to wire them to options in the genericapiserver. Or at least I don't remember them in recommendedoptions

they're not wired to flags, but the API server still checks them in handlers, etc, so if they're not registered, I'm pretty sure the API server wouldn't run

yue9944882 · 2019-11-06T13:02:36Z

/retest

deads2k

asdf

deads2k · 2019-11-08T13:22:07Z

Updated to use the featuregate defined in apiserver that is abused by so many others because it sits high in the stack. Left a comment for future developers to ponder their choices.

liggitt · 2019-11-08T13:54:54Z

/approve

one unit test failure, lgtm otherwise

k8s-ci-robot · 2019-11-08T13:55:41Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, liggitt

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~cmd/kube-apiserver/OWNERS~~ [deads2k,liggitt]
~~pkg/kubeapiserver/OWNERS~~ [deads2k,liggitt]
~~plugin/pkg/admission/OWNERS~~ [deads2k,liggitt]
~~staging/src/k8s.io/apiserver/OWNERS~~ [deads2k,liggitt]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

liggitt · 2019-11-08T16:39:26Z

/lgtm

liggitt · 2019-11-08T18:17:43Z

/lgtm

plugin/pkg/admission/noderestriction/admission_test.go

fejta-bot · 2019-11-08T22:26:41Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

k8s-ci-robot · 2019-11-12T13:11:44Z

New changes are detected. LGTM label has been removed.

deads2k · 2019-11-12T13:12:06Z

updated per comment, retagging.

deads2k · 2019-11-12T14:04:25Z

/retest

liggitt · 2019-11-12T14:12:11Z

diff -Naupr -x _output /home/prow/go/src/k8s.io/kubernetes/plugin/pkg/admission/noderestriction/BUILD /tmp/verify-bazel.qguOOd/go/src/k8s.io/kubernetes/plugin/pkg/admission/noderestriction/BUILD
--- /home/prow/go/src/k8s.io/kubernetes/plugin/pkg/admission/noderestriction/BUILD	2019-11-12 13:14:11.728674466 +0000
+++ /tmp/verify-bazel.qguOOd/go/src/k8s.io/kubernetes/plugin/pkg/admission/noderestriction/BUILD	2019-11-12 13:15:34.733658705 +0000
@@ -53,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",

Run ./hack/update-bazel.sh

k8s-ci-robot · 2019-11-12T15:55:21Z

New changes are detected. LGTM label has been removed.

k8s-ci-robot · 2019-11-12T16:08:50Z

@deads2k: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-kubernetes-node-e2e-containerd	`83f6f27`	link	`/test pull-kubernetes-node-e2e-containerd`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

k8s-ci-robot requested review from apelisse and derekwaynecarr November 5, 2019 19:55

k8s-ci-robot added area/apiserver sig/auth Categorizes an issue or PR as relevant to SIG Auth. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Nov 5, 2019

liggitt reviewed Nov 5, 2019

View reviewed changes

deads2k commented Nov 8, 2019

View reviewed changes

deads2k force-pushed the admission-feature-gates branch 2 times, most recently from aa596d9 to 08eca7f Compare November 8, 2019 13:21

deads2k force-pushed the admission-feature-gates branch from 08eca7f to 03a14f4 Compare November 8, 2019 16:23

k8s-ci-robot assigned liggitt Nov 8, 2019

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 8, 2019

add featuregate inspection as admission plugin initializer

675c2fb

deads2k force-pushed the admission-feature-gates branch from 03a14f4 to 01ac0cc Compare November 8, 2019 18:12

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 8, 2019

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 8, 2019

liggitt reviewed Nov 8, 2019

View reviewed changes

plugin/pkg/admission/noderestriction/admission_test.go Show resolved Hide resolved

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 8, 2019

liggitt added this to the v1.17 milestone Nov 10, 2019

deads2k force-pushed the admission-feature-gates branch from 01ac0cc to 131512c Compare November 12, 2019 13:11

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 12, 2019

deads2k added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Nov 12, 2019

liggitt mentioned this pull request Nov 12, 2019

Relax namespace restriction for critical pods #76310

Merged

remove global variable dep in admission

83f6f27

deads2k force-pushed the admission-feature-gates branch from 131512c to 83f6f27 Compare November 12, 2019 15:55

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 12, 2019

deads2k added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 12, 2019

k8s-ci-robot merged commit 94efa98 into kubernetes:master Nov 12, 2019

liggitt mentioned this pull request Nov 12, 2019

CSI Migration phase 2: disable probing of in-tree plugins #83098

Merged

liggitt removed the sig/auth Categorizes an issue or PR as relevant to SIG Auth. label Feb 18, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

remove global variable dependency from admission plugins #84813

remove global variable dependency from admission plugins #84813

deads2k commented Nov 5, 2019

liggitt Nov 5, 2019

liggitt Nov 5, 2019

deads2k Nov 6, 2019

liggitt Nov 6, 2019

liggitt Nov 6, 2019

deads2k Nov 7, 2019

liggitt Nov 7, 2019

deads2k Nov 8, 2019

liggitt Nov 8, 2019 •

edited

yue9944882 commented Nov 6, 2019

deads2k left a comment

deads2k commented Nov 8, 2019

liggitt commented Nov 8, 2019

k8s-ci-robot commented Nov 8, 2019

liggitt commented Nov 8, 2019

liggitt commented Nov 8, 2019

fejta-bot commented Nov 8, 2019

k8s-ci-robot commented Nov 12, 2019

deads2k commented Nov 12, 2019

deads2k commented Nov 12, 2019

liggitt commented Nov 12, 2019

k8s-ci-robot commented Nov 12, 2019

k8s-ci-robot commented Nov 12, 2019 •

edited

	APIResponseCompression: {Default: true, PreRelease: featuregate.Beta},
	APIListChunking: {Default: true, PreRelease: featuregate.Beta},
	DryRun: {Default: true, PreRelease: featuregate.Beta},
	RemainingItemCount: {Default: true, PreRelease: featuregate.Beta},
	ServerSideApply: {Default: true, PreRelease: featuregate.Beta},
	StorageVersionHash: {Default: true, PreRelease: featuregate.Beta},
	WinOverlay: {Default: false, PreRelease: featuregate.Alpha},
	WinDSR: {Default: false, PreRelease: featuregate.Alpha},
	WatchBookmark: {Default: true, PreRelease: featuregate.GA, LockToDefault: true},
	RequestManagement: {Default: false, PreRelease: featuregate.Alpha},
	RemoveSelfLink: {Default: false, PreRelease: featuregate.Alpha},

remove global variable dependency from admission plugins #84813

remove global variable dependency from admission plugins #84813

Conversation

deads2k commented Nov 5, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liggitt Nov 8, 2019 • edited

Choose a reason for hiding this comment

yue9944882 commented Nov 6, 2019

deads2k left a comment

Choose a reason for hiding this comment

deads2k commented Nov 8, 2019

liggitt commented Nov 8, 2019

k8s-ci-robot commented Nov 8, 2019

liggitt commented Nov 8, 2019

liggitt commented Nov 8, 2019

fejta-bot commented Nov 8, 2019

k8s-ci-robot commented Nov 12, 2019

deads2k commented Nov 12, 2019

deads2k commented Nov 12, 2019

liggitt commented Nov 12, 2019

k8s-ci-robot commented Nov 12, 2019

k8s-ci-robot commented Nov 12, 2019 • edited

liggitt Nov 8, 2019 •

edited

k8s-ci-robot commented Nov 12, 2019 •

edited