Let webhook controller uses a local scheme that understand admissionReview #60995

caesarxuchao · 2018-03-09T19:30:08Z

An alternative to #60965.
Fix #60963.
Fix kubernetes/sample-apiserver#21.

Created a scheme that only understands admission/v1beta1 and use it to
encode/decode admissionReviews.

The webhook admission controller in a custom apiserver now works off-the-shelf.

cc @sttts

caesarxuchao · 2018-03-09T19:31:00Z

Created a PR to see if I missed any test.

sttts · 2018-03-09T19:32:30Z

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

@@ -130,10 +130,12 @@ func (a *MutatingWebhook) SetServiceResolver(sr config.ServiceResolver) {

 // SetScheme sets a serializer(NegotiatedSerializer) which is derived from the scheme
 func (a *MutatingWebhook) SetScheme(scheme *runtime.Scheme) {
+	admissionScheme := runtime.NewScheme()


We can set the client manager serializer statically then, can't we? We should. Not here.

Right. Let's wait for the test to finish?

I'll also test it with the sample apiserver to see if it fixes the problem.

Sure, am fine with the change if you move the code into the main constructor.

@deads2k can lgtm

sttts · 2018-03-09T19:58:20Z

@hzxuzhonghu this was even easier than expected. Ptal.

deads2k · 2018-03-09T20:03:47Z

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

@@ -130,10 +130,12 @@ func (a *MutatingWebhook) SetServiceResolver(sr config.ServiceResolver) {

 // SetScheme sets a serializer(NegotiatedSerializer) which is derived from the scheme
 func (a *MutatingWebhook) SetScheme(scheme *runtime.Scheme) {
+	admissionScheme := runtime.NewScheme()


I thought that the schemes we used had to be passed in so that the internal objects the admission plugin gets could be reliably converted to the external representations. Different schemes for different layers or something?

compare the converter below. The new scheme is about the versioned AdmissionReview with encoded payload objects.

caesarxuchao · 2018-03-09T20:16:58Z

Tests passed. Sending the commit that statically build the clientManager.

caesarxuchao · 2018-03-09T20:20:53Z

I'll squash before merging.

Got trouble setting up a test with the sample apiserver. Will seek for help in the sig slack.

deads2k · 2018-03-09T20:57:40Z

lgtm
/approve

Created a scheme that only understands admission/v1beta1 and use it to encode/decode admissionReviews. Also made the NegotiationSerializer setup static

caesarxuchao · 2018-03-09T22:31:42Z

Verified it solved kubernetes/sample-apiserver#21.

$ kubectl create -f ../flunders/01-flunder.yaml
Error from server: error when creating "../flunders/01-flunder.yaml": admission webhook "deny-unwanted-pod-container-name-and-label.k8s.io" denied the request: expect resource to be { v1 pods}

I used the e2e test webhook, which expected a pod, thus the error. The presence of the error proved that the apiserver was able to build an admissionReview and sent it to the webhook.

caesarxuchao · 2018-03-09T22:32:30Z

Squashed.

caesarxuchao · 2018-03-09T22:47:35Z

/retest

deads2k · 2018-03-09T23:25:19Z

/lgtm

k8s-ci-robot · 2018-03-09T23:25:26Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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:

~~staging/src/k8s.io/apiserver/OWNERS~~ [deads2k]

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

caesarxuchao · 2018-03-09T23:38:56Z

/release-note-none
/sig api-machinery

ash2k · 2018-03-11T22:09:48Z

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

@@ -54,8 +56,13 @@ func NewClientManager() (ClientManager, error) {
 	if err != nil {
 		return ClientManager{}, err
 	}
+	admissionScheme := runtime.NewScheme()
+	admissionv1beta1.AddToScheme(admissionScheme)


This can return an error

@ash2k Not necessary, because admissionv1beta1.AddToScheme will never return an error.

If it will never return an error why does the function return an error? :) Tomorrow the implementation will change and something will not be right. E.g. some other code would be added before this function is called and it will cause it to error out. The worst thing about such situations is that it will be impossible to see the problem directly.
Russian proverb: "Even a stick shoots once a year" :)

I agree that we should at least wrap those calls and trigger a panic. But this must be a pattern through the whole code base.

sttts · 2018-03-12T09:01:11Z

@deads2k can you label for 1.10?

deads2k · 2018-03-12T13:16:19Z

/status approved-for-milestone
/kind bug
/priority important-soon
/milestone v1.10

k8s-github-robot · 2018-03-12T13:16:28Z

[MILESTONENOTIFIER] Milestone Pull Request: Up-to-date for process

@caesarxuchao @deads2k @sttts

Pull Request Labels

sig/api-machinery: Pull Request will be escalated to these SIGs if needed.
priority/important-soon: Escalate to the pull request owners and SIG owner; move out of milestone after several unsuccessful escalation attempts.
kind/bug: Fixes a bug discovered during the current release.

Help

k8s-github-robot · 2018-03-12T13:17:00Z

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

k8s-github-robot · 2018-03-12T14:02:26Z

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

caesarxuchao · 2018-03-12T22:01:21Z

Moved to 1.9 milestone to request a cherry-pick.

k8s-github-robot · 2018-03-16T22:34:33Z

Commit found in the "release-1.9" branch appears to be this PR. Removing the "cherrypick-candidate" label. If this is an error find help to get your PR picked.

…#60995-upstream-release-1.9 Automatic merge from submit-queue. Automated cherry pick of #60995: Make admission webhooks work in custom apiservers. Cherry pick of #60995 on release-1.9. #60995: Make admission webhooks work in custom apiservers.

caesarxuchao self-assigned this Mar 9, 2018

k8s-ci-robot requested review from deads2k and sttts March 9, 2018 19:30

caesarxuchao changed the title ~~Let webhook controller uses a local scheme that understand admissionReview~~ [WIP] Let webhook controller uses a local scheme that understand admissionReview Mar 9, 2018

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 9, 2018

sttts reviewed Mar 9, 2018

View reviewed changes

deads2k reviewed Mar 9, 2018

View reviewed changes

caesarxuchao changed the title ~~[WIP] Let webhook controller uses a local scheme that understand admissionReview~~ Let webhook controller uses a local scheme that understand admissionReview Mar 9, 2018

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 Mar 9, 2018

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 9, 2018

Make admission webhooks work in custom apiservers.

3ab5160

Created a scheme that only understands admission/v1beta1 and use it to encode/decode admissionReviews. Also made the NegotiationSerializer setup static

caesarxuchao force-pushed the webhook-serializer branch from 6e9e4b3 to 3ab5160 Compare March 9, 2018 21:54

caesarxuchao assigned deads2k and sttts and unassigned caesarxuchao Mar 9, 2018

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

ash2k reviewed Mar 11, 2018

View reviewed changes

k8s-ci-robot added status/approved-for-milestone kind/bug Categorizes issue or PR as related to a bug. labels Mar 12, 2018

k8s-ci-robot added this to the v1.10 milestone Mar 12, 2018

k8s-ci-robot added the priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. label Mar 12, 2018

k8s-github-robot merged commit fd3cbc9 into kubernetes:master Mar 12, 2018

caesarxuchao mentioned this pull request Mar 12, 2018

Automated cherry pick of #60995: Make admission webhooks work in custom apiservers. #61066

Merged

caesarxuchao added the cherrypick-candidate label Mar 12, 2018

caesarxuchao modified the milestones: v1.10, v1.9 Mar 12, 2018

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Mar 13, 2018

sttts mentioned this pull request Mar 14, 2018

webhook-admission: quickfix of registering AdmissionReview in provided scheme #60965

Closed

k8s-github-robot removed the cherrypick-candidate label Mar 16, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Let webhook controller uses a local scheme that understand admissionReview #60995

Let webhook controller uses a local scheme that understand admissionReview #60995

caesarxuchao commented Mar 9, 2018 •

edited

Loading

caesarxuchao commented Mar 9, 2018

sttts Mar 9, 2018

caesarxuchao Mar 9, 2018

sttts Mar 9, 2018

sttts Mar 9, 2018

sttts commented Mar 9, 2018

deads2k Mar 9, 2018

sttts Mar 9, 2018

caesarxuchao commented Mar 9, 2018

caesarxuchao commented Mar 9, 2018

deads2k commented Mar 9, 2018

caesarxuchao commented Mar 9, 2018

caesarxuchao commented Mar 9, 2018

caesarxuchao commented Mar 9, 2018

deads2k commented Mar 9, 2018

k8s-ci-robot commented Mar 9, 2018

caesarxuchao commented Mar 9, 2018

ash2k Mar 11, 2018

hzxuzhonghu Mar 12, 2018

ash2k Mar 12, 2018

sttts Mar 12, 2018

sttts commented Mar 12, 2018

deads2k commented Mar 12, 2018

k8s-github-robot commented Mar 12, 2018

k8s-github-robot commented Mar 12, 2018

k8s-github-robot commented Mar 12, 2018

caesarxuchao commented Mar 12, 2018

k8s-github-robot commented Mar 16, 2018

Let webhook controller uses a local scheme that understand admissionReview #60995

Let webhook controller uses a local scheme that understand admissionReview #60995

Conversation

caesarxuchao commented Mar 9, 2018 • edited Loading

caesarxuchao commented Mar 9, 2018

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

sttts commented Mar 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

caesarxuchao commented Mar 9, 2018

caesarxuchao commented Mar 9, 2018

deads2k commented Mar 9, 2018

caesarxuchao commented Mar 9, 2018

caesarxuchao commented Mar 9, 2018

caesarxuchao commented Mar 9, 2018

deads2k commented Mar 9, 2018

k8s-ci-robot commented Mar 9, 2018

caesarxuchao commented Mar 9, 2018

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

sttts commented Mar 12, 2018

deads2k commented Mar 12, 2018

k8s-github-robot commented Mar 12, 2018

k8s-github-robot commented Mar 12, 2018

k8s-github-robot commented Mar 12, 2018

caesarxuchao commented Mar 12, 2018

k8s-github-robot commented Mar 16, 2018

caesarxuchao commented Mar 9, 2018 •

edited

Loading