Add namespace selector to admission webhook #54727

caesarxuchao · 2017-10-27T23:07:43Z

Implementing the design.

Added the NamespaceSelector field to the webhook configuration API
Let the webhook plugin respect the NamespaceSelector
Added unit test and e2e test

cc @kubernetes/sig-api-machinery-api-reviews

Added namespaceSelector to externalAdmissionWebhook configuration to allow applying webhooks only to objects in the namespaces that have matching labels.

caesarxuchao · 2017-10-30T16:59:51Z

/retest

caesarxuchao · 2017-10-31T20:24:06Z

staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/admission_test.go

 // TestAdmit tests that GenericAdmissionWebhook#Admit works as expected
 func TestAdmit(t *testing.T) {
 	scheme := runtime.NewScheme()
 	v1alpha1.AddToScheme(scheme)
-	api.AddToScheme(scheme)


Note that "api" was just an inappropriate alias.

caesarxuchao · 2017-10-31T23:34:46Z

staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/admission.go

+	if err != nil {
+		return false, apierrors.NewInternalError(err)
+	}
+	namespace, err := a.namespaceLister.Get(namespaceName)


Admittedly, the cache namespace can be stale. We tolerate the delay in other plugins like podtolerationrestriction, so I think it's ok.

caesarxuchao 20 hours ago Member
Admittedly, the cache namespace can be stale. We tolerate the delay in other plugins like podtolerationrestriction, so I think it's ok.

Agree

cheftako · 2017-11-01T00:08:13Z

pkg/apis/admissionregistration/types.go

@@ -165,6 +165,32 @@ type ExternalAdmissionHook struct {
 	// allowed values are Ignore or Fail. Defaults to Ignore.
 	// +optional
 	FailurePolicy *FailurePolicyType
+
+	// Selects namespaces that are subjected to this webhook.


We should probably reword this. It might be better to be slightly repetitive with the description so it is clear that it applies to both selectors. Something like "Selects whether to run the webhook on an object based on whether the namespace for that object is/is not labeled with the associated key and the values equals any of the provided values. To run the webhook on any object whose namespace was not associated with 'runlevel' of '0' or '1'; you would set the selector as follows."

"If instead you wanted to only run the the webhook on any objects whose namespace was associated with the 'environment' of 'prod' or 'staging'; you would set the selector as follows."

whether the namespace for that object is/is not labeled with the associated key and the values equals any of the provided values.

This is not accurate, because selector also supports operations like "if key exists/not exists".

I'll just say "whether the namespace for that object matches the selector".

Also, is it intentional to use the past tense?

Updated the comments in types.go, PTAL. Thank you.

cheftako · 2017-11-01T00:08:46Z

pkg/apis/admissionregistration/types.go

+	// matching, so you can use this selector as a whitelist or a blacklist.
+	// For example, to apply the webhook to all namespaces except for those have
+	// labels with key "runlevel" and value equal to "0" or "1":
+	// metav1.LabelSelctor{MatchExpressions: []LabelSelectorRequirement{


metav1.LabelSelector

cheftako · 2017-11-01T00:08:55Z

pkg/apis/admissionregistration/types.go

+	// }}
+	// As another example, to only apply the webhook to the namespaces that have
+	// labels with key "environment" and value equal to "prod" or "staging":
+	// metav1.LabelSelctor{MatchExpressions: []LabelSelectorRequirement{


metav1.LabelSelector

deads2k · 2017-11-01T19:29:40Z

staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/admission.go

+	a.namespaceLister = namespaceInformer.Lister()
+	a.SetReadyFunc(namespaceInformer.Informer().HasSynced)
+}
+
 // Validator holds Validate functions, which are responsible for validation of initialized shared resources
 // and should be implemented on admission plugins
 func (a *GenericAdmissionWebhook) Validate() error {


Add requirement for namespace lister.

deads2k · 2017-11-01T19:30:17Z

staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/admission.go

+// whether the namespace is exempted from the webhook.
+func (a *GenericAdmissionWebhook) exemptNamespace(h *v1alpha1.ExternalAdmissionHook, attr admission.Attributes) (bool, error) {
+	namespaceName := attr.GetNamespace()
+	if namespaceName == "" {


len(namespaceName) == 0

deads2k · 2017-11-01T19:35:09Z

staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/admission.go

+	namespaceName := attr.GetNamespace()
+	if namespaceName == "" {
+		// if the request is root scoped, it is not exempted.
+		return false, nil


This is a neat side-effect. A downed admission plugin could prevent the node that would run the admission webhook from running.

I don't think we need to resolve it for beta, but given the switch to a DNS name, perhaps admission plugins that can mess with nodes should be hosted in a way that allows off cluster access.

I think this can be solved with the run level concept, like by reserving some nodes for run level 0 items.

deads2k · 2017-11-01T19:37:09Z

staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/admission.go

@@ -246,7 +259,42 @@ func (a *GenericAdmissionWebhook) Admit(attr admission.Attributes) error {
 	return errs[0]
 }

+// whether the namespace is exempted from the webhook.
+func (a *GenericAdmissionWebhook) exemptNamespace(h *v1alpha1.ExternalAdmissionHook, attr admission.Attributes) (bool, error) {


Doesn't this fail when it restricts namespaces and I'm trying to create a namespace?

Good point. I updated the code to use the labels of the namespace object itself if the attr.Object is a namespace.

deads2k · 2017-11-01T19:37:56Z

staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/admission.go

+	if err != nil && !apierrors.IsNotFound(err) {
+		return false, apierrors.NewInternalError(err)
+	}
+	if err != nil && apierrors.IsNotFound(err) {


Just need if apierrors.IsNotFound(err)

cheftako · 2017-11-09T01:26:49Z

test/images/webhook/main.go

@@ -115,12 +157,20 @@ func serve(w http.ResponseWriter, r *http.Request) {
 	}
 }

+func servePods(w http.ResponseWriter, r *http.Request) {
+	serve(w, r, "pods")


Maybe pass the function rather than a resource name. You would then get to lose the switch above.

caesarxuchao · 2017-11-09T20:29:24Z

@cheftako comments addressed. PTAL. Thank you.

cheftako

/lgtm

k8s-github-robot · 2017-11-09T22:52:03Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: caesarxuchao, cheftako, lavalamp

No associated issue. Update pull-request body to add a reference to an issue, or get approval with /approve no-issue

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

Needs approval from an approver in each of these OWNERS Files:

~~api/OWNERS~~ [lavalamp]
~~docs/api-reference/OWNERS~~ [lavalamp]
~~pkg/apis/OWNERS~~ [lavalamp]
~~staging/src/k8s.io/api/OWNERS~~ [lavalamp]
~~test/OWNERS~~ [lavalamp]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

caesarxuchao · 2017-11-09T23:03:47Z

Applying the approval label based on #54727 (comment).

caesarxuchao · 2017-11-10T21:02:21Z

Adding back the lgtm after rebase.

business logic in webhook plugin and unit test add a e2e test for namespace selector

caesarxuchao · 2017-11-10T21:44:50Z

Rebased again.

caesarxuchao · 2017-11-10T22:30:54Z

/retest

k8s-github-robot · 2017-11-11T15:02:41Z

/test all [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2017-11-11T15:50:31Z

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

caesarxuchao self-assigned this Oct 27, 2017

k8s-github-robot added the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label Oct 27, 2017

cmluciano changed the title ~~[WIP] Add amespace selector to admission webhook~~ [WIP] Add namespace selector to admission webhook Oct 30, 2017

caesarxuchao force-pushed the namespaceSelector branch from 31c2b58 to fb1ea5a Compare October 31, 2017 01:17

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 31, 2017

caesarxuchao force-pushed the namespaceSelector branch from fb1ea5a to b6d075f Compare October 31, 2017 01:26

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 31, 2017

caesarxuchao commented Oct 31, 2017

View reviewed changes

caesarxuchao force-pushed the namespaceSelector branch 2 times, most recently from de768ee to 75730c5 Compare October 31, 2017 23:28

caesarxuchao changed the title ~~[WIP] Add namespace selector to admission webhook~~ Add namespace selector to admission webhook Oct 31, 2017

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 31, 2017

caesarxuchao assigned deads2k, cheftako and lavalamp and unassigned caesarxuchao and deads2k Oct 31, 2017

caesarxuchao commented Oct 31, 2017

View reviewed changes

cheftako reviewed Nov 1, 2017

View reviewed changes

deads2k reviewed Nov 1, 2017

View reviewed changes

cheftako reviewed Nov 9, 2017

View reviewed changes

caesarxuchao force-pushed the namespaceSelector branch from 7bcda79 to 1aad3a6 Compare November 9, 2017 20:28

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Nov 9, 2017

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 9, 2017

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

cheftako approved these changes Nov 9, 2017

View reviewed changes

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Nov 9, 2017

caesarxuchao added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 9, 2017

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 10, 2017

caesarxuchao force-pushed the namespaceSelector branch from 1aad3a6 to 7ac8a38 Compare November 10, 2017 19:06

k8s-github-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Nov 10, 2017

caesarxuchao force-pushed the namespaceSelector branch from 7ac8a38 to 3e19bcd Compare November 10, 2017 19:37

caesarxuchao added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 10, 2017

Chao Xu added 2 commits November 10, 2017 13:40

add NamespaceSelector to the api

7006d22

business logic in webhook plugin and unit test add a e2e test for namespace selector

generated

2f83748

caesarxuchao force-pushed the namespaceSelector branch from 3e19bcd to 2f83748 Compare November 10, 2017 21:44

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 10, 2017

caesarxuchao added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 10, 2017

k8s-github-robot merged commit e52e793 into kubernetes:master Nov 11, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add namespace selector to admission webhook #54727

Add namespace selector to admission webhook #54727

caesarxuchao commented Oct 27, 2017 •

edited

caesarxuchao commented Oct 30, 2017

caesarxuchao Oct 31, 2017

caesarxuchao Oct 31, 2017

deads2k Nov 1, 2017

cheftako Nov 1, 2017

caesarxuchao Nov 3, 2017

cheftako Nov 1, 2017

cheftako Nov 1, 2017

deads2k Nov 1, 2017

deads2k Nov 1, 2017

deads2k Nov 1, 2017

lavalamp Nov 3, 2017

deads2k Nov 1, 2017

caesarxuchao Nov 3, 2017

deads2k Nov 1, 2017

cheftako Nov 9, 2017

caesarxuchao Nov 9, 2017

caesarxuchao commented Nov 9, 2017

cheftako left a comment

k8s-github-robot commented Nov 9, 2017

caesarxuchao commented Nov 9, 2017

caesarxuchao commented Nov 10, 2017

caesarxuchao commented Nov 10, 2017

caesarxuchao commented Nov 10, 2017

k8s-github-robot commented Nov 11, 2017

k8s-github-robot commented Nov 11, 2017

Add namespace selector to admission webhook #54727

Add namespace selector to admission webhook #54727

Conversation

caesarxuchao commented Oct 27, 2017 • edited

caesarxuchao commented Oct 30, 2017

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

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

caesarxuchao commented Nov 9, 2017

cheftako left a comment

Choose a reason for hiding this comment

k8s-github-robot commented Nov 9, 2017

caesarxuchao commented Nov 9, 2017

caesarxuchao commented Nov 10, 2017

caesarxuchao commented Nov 10, 2017

caesarxuchao commented Nov 10, 2017

k8s-github-robot commented Nov 11, 2017

k8s-github-robot commented Nov 11, 2017

caesarxuchao commented Oct 27, 2017 •

edited