make webhook admission kind of work #16861

deads2k · 2017-10-13T15:40:50Z

This is done to support openshift/kubernetes-namespace-reservation#3 . It contains multiple fixes needed to make webhooks run at all. In addition, it changes validation rules and admission handling until we get changes like kubernetes/kubernetes#53826 into the API.

This affects handling and compatibility of an alpha feature.

@dgoodwin @abhgupta be ready for upgrade pain in this area.
@bparees I turned this on in cluster-up. Surgery was relatively minor

bparees · 2017-10-13T15:52:05Z

cluster up changes lgtm.

deads2k · 2017-10-13T16:28:27Z

/retest

dgoodwin · 2017-10-13T17:23:28Z

Upgrade pain just in the sense that we'll need a build deployed with this change before we can get anything operational?

deads2k · 2017-10-13T17:23:57Z

Upgrade pain just in the sense that we'll need a build deployed with this change before we can get anything operational?

Alpha fields are alpha. The API will change and your webhook admission will either disappear or start failing.

deads2k · 2017-10-13T19:59:43Z

/retest

sttts · 2017-10-16T07:10:27Z

pkg/cmd/server/origin/admission/plugin_initializer.go

@@ -144,6 +145,14 @@ func NewPluginInitializer(
 		cloudConfig,
 		restMapper,
 		quotaRegistry)
+	// upstream broke this, so we can't use their mechanism.  We need to get an actual client cert and practically speaking privileged loopback will always have one


broke what?

broke what?

They wire to the wrong client cert: kubernetes/kubernetes#53828 . Also, it's built such that using the "normal" constructor produces a plugin initializer that will panic on a "normal" admission chain.

That link helps. Next time I read your PR description.

sttts · 2017-10-16T07:12:12Z

vendor/k8s.io/kubernetes/plugin/pkg/admission/webhook/admission.go

@@ -45,6 +45,8 @@ import (
 	"k8s.io/kubernetes/pkg/kubeapiserver/admission/configuration"

 	// install the clientgo admission API for use with api registry
+	"strings"


Automatic merge from submit-queue (batch tested with PRs 16867, 16231). remove unnecessary anonymous function There is an upstream replacement for this function. This cleans up the last usages. blocks #16861 /kind bug

sttts · 2017-10-16T17:04:54Z

test/integration/router/router_http_server.go

@@ -19,6 +19,8 @@ import (
 	kapi "k8s.io/kubernetes/pkg/api"

 	"github.com/openshift/origin/pkg/cmd/util"
+
+	_ "github.com/openshift/origin/pkg/api/install"


how is this related to the PR?

how is this related to the PR?

if the APIs aren't installed, their fake apiserver fails.

sttts · 2017-10-16T17:07:32Z

vendor/k8s.io/kubernetes/plugin/pkg/admission/webhook/admission.go

 	response := &admissionv1alpha1.AdmissionReview{}
-	if err := client.Post().Context(ctx).Body(&request).Do().Into(response); err != nil {


did this work upstream?

sttts · 2017-10-16T17:18:44Z

Commented on the upstream PRs.

…have a valid CN; fix a fuzzer

…to work without modifying api

deads2k · 2017-10-16T17:41:16Z

comments addressed

deads2k · 2017-10-16T20:12:45Z

/retest

openshift-bot · 2017-10-16T23:52:02Z

/retest

Please review the full test history for this PR and help us cut down flakes.

sttts · 2017-10-17T05:37:36Z

/lgtm

openshift-merge-robot · 2017-10-17T05:37:42Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

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

~~OWNERS~~ [deads2k]

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

openshift-bot · 2017-10-17T07:56:05Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-merge-robot · 2017-10-17T11:07:33Z

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

openshift-merge-robot · 2017-10-17T11:26:14Z

Automatic merge from submit-queue (batch tested with PRs 16861, 16438).

openshift-ci-robot · 2017-10-17T12:41:02Z

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

Test name	Commit	Details	Rerun command
ci/openshift-jenkins/extended_conformance_install_update	`b8e3c19`	link	`/test extended_conformance_install_update`

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.

caesarxuchao · 2017-10-17T23:59:01Z

pkg/cmd/server/origin/admission/plugin_initializer.go

@@ -144,6 +145,14 @@ func NewPluginInitializer(
 		cloudConfig,
 		restMapper,
 		quotaRegistry)
+	// upstream broke this, so we can't use their mechanism.  We need to get an actual client cert and practically speaking privileged loopback will always have one
+	kubePluginInitializer.SetClientCert(privilegedLoopbackConfig.TLSClientConfig.CertData, privilegedLoopbackConfig.TLSClientConfig.KeyData)


Where can a webhook find the CA that signs this cert?

Where can a webhook find the CA that signs this cert?

This cert is signed by the client CA for the apiservers. Since the our webhook apiservers are run as aggregated API servers, they automatically trust that CA (though a user could wire it differently). That means that normal cluster authentication and authorization (rbac) rules apply.

deads2k added the kind/bug Categorizes issue or PR as related to a bug. label Oct 13, 2017

deads2k assigned mfojtik and sttts Oct 13, 2017

openshift-merge-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 13, 2017

openshift-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Oct 13, 2017

deads2k mentioned this pull request Oct 13, 2017

remove unnecessary anonymous function #16867

Merged

deads2k force-pushed the cli-08-clusterup-admission branch from f568af2 to 6060039 Compare October 13, 2017 18:10

openshift-merge-robot added the vendor-update Touching vendor dir or related files label Oct 14, 2017

sttts reviewed Oct 16, 2017

View reviewed changes

deads2k force-pushed the cli-08-clusterup-admission branch 3 times, most recently from b9fcab6 to 3d06084 Compare October 16, 2017 13:53

sttts reviewed Oct 16, 2017

View reviewed changes

deads2k force-pushed the cli-08-clusterup-admission branch from 3d06084 to 329cf75 Compare October 16, 2017 17:39

deads2k added 6 commits October 16, 2017 13:41

UPSTREAM: 52673: default service resolver for webhook admission

d7cdcba

turn admission webhooks on in cluster up

0db40a6

UPSTREAM: 53823: allow fail close webhook admission

ea12582

UPSTREAM: 50476: fix the webhook unit test; the server cert needs to …

099f600

…have a valid CN; fix a fuzzer

UPSTREAM: 53896: decode admission responses into a fresh object

e8061fe

UPSTREAM: <drop>: drop in 1.9 rebase. Shims enough admission webhook …

b8e3c19

…to work without modifying api

deads2k force-pushed the cli-08-clusterup-admission branch from 329cf75 to b8e3c19 Compare October 16, 2017 17:41

deads2k added the lgtm Indicates that a PR is ready to be merged. label Oct 16, 2017

openshift-merge-robot merged commit 20db538 into openshift:master Oct 17, 2017

caesarxuchao reviewed Oct 17, 2017

View reviewed changes

deads2k mentioned this pull request Oct 23, 2017

oc cluster up fails on latest master #16995

Closed

deads2k deleted the cli-08-clusterup-admission branch January 24, 2018 14:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

make webhook admission kind of work #16861

make webhook admission kind of work #16861

deads2k commented Oct 13, 2017

bparees commented Oct 13, 2017

deads2k commented Oct 13, 2017

dgoodwin commented Oct 13, 2017

deads2k commented Oct 13, 2017

deads2k commented Oct 13, 2017

sttts Oct 16, 2017

deads2k Oct 16, 2017

sttts Oct 16, 2017

sttts Oct 16, 2017

sttts Oct 16, 2017

deads2k Oct 16, 2017

sttts Oct 16, 2017

sttts commented Oct 16, 2017

deads2k commented Oct 16, 2017

deads2k commented Oct 16, 2017

openshift-bot commented Oct 16, 2017

sttts commented Oct 17, 2017

openshift-merge-robot commented Oct 17, 2017

openshift-bot commented Oct 17, 2017

openshift-merge-robot commented Oct 17, 2017

openshift-merge-robot commented Oct 17, 2017

openshift-ci-robot commented Oct 17, 2017

caesarxuchao Oct 17, 2017

deads2k Oct 18, 2017

		response := &admissionv1alpha1.AdmissionReview{}
		if err := client.Post().Context(ctx).Body(&request).Do().Into(response); err != nil {

make webhook admission kind of work #16861

make webhook admission kind of work #16861

Conversation

deads2k commented Oct 13, 2017

bparees commented Oct 13, 2017

deads2k commented Oct 13, 2017

dgoodwin commented Oct 13, 2017

deads2k commented Oct 13, 2017

deads2k commented Oct 13, 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

sttts commented Oct 16, 2017

deads2k commented Oct 16, 2017

deads2k commented Oct 16, 2017

openshift-bot commented Oct 16, 2017

sttts commented Oct 17, 2017

openshift-merge-robot commented Oct 17, 2017

openshift-bot commented Oct 17, 2017

openshift-merge-robot commented Oct 17, 2017

openshift-merge-robot commented Oct 17, 2017

openshift-ci-robot commented Oct 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment