Dynamic webhook admission control plugin #46388

lavalamp · 2017-05-24T22:52:05Z

Unit tests pass.

Needs plumbing:

service resolver (depends on @wfender PR)
client cert (depends on ????)
hook source (depends on @caesarxuchao PR)

Also at least one thing will need to be renamed after Chao's PR merges.

Allow remote admission controllers to be dynamically added and removed by administrators.  External admission controllers make an HTTP POST containing details of the requested action which the service can approve or reject.

whitlockjc · 2017-05-25T17:39:59Z

plugin/pkg/admission/webhook/admission.go

+	wg.Wait()
+
+	if failCount > 0 {
+		return ErrWebhookRejected


Is this descriptive enough? An API caller has no idea why it was rejected and the only possible way to figure that out is by accessing the API server logs, which is likely not an option. Is there any good way to display the error(s) back to the user? (To me, it would be nice if all webhook responses with Status.Result.Reason were returned to the user that way they could possibly remedy the issue.)

I guess I can fix that, sure.

whitlockjc · 2017-05-25T17:41:04Z

plugin/pkg/admission/webhook/admission.go

+			// TODO: set KeyData/CertData
+		},
+		UserAgent: "kube-apiserver-admission",
+		Timeout:   30 * time.Second,


Any reason we cannot make this configurable? Might not be necessary for 1.0 but I can see this being useful in the future.

In the future. not needed for mvp I think.

smarterclayton · 2017-05-26T03:26:57Z

pkg/kubeapiserver/admission/initializer.go

@@ -115,4 +132,12 @@ func (i pluginInitializer) Initialize(plugin admission.Interface) {
 	if wants, ok := plugin.(WantsQuotaRegistry); ok {
 		wants.SetQuotaRegistry(i.quotaRegistry)
 	}
+
+	if wants, ok := plugin.(WantsServiceResolver); ok {
+		wants.SetQuotaRegistry(i.serviceResolver)


Pushed without running :)

caesarxuchao · 2017-05-26T00:08:45Z

plugin/pkg/admission/webhook/admission.go

+	"k8s.io/kubernetes/pkg/api"
+	admissionv1alpha1 "k8s.io/kubernetes/pkg/apis/admission/v1alpha1"
+
+	// install the clientgo admission API for use with api registry


s/clientgo//

caesarxuchao · 2017-05-26T18:57:35Z

plugin/pkg/admission/webhook/admission_test.go

+	defer testServer.Close()
+	serverURL, err := url.ParseRequestURI(testServer.URL)
+	if err != nil {
+		t.Fatalf("???? %v", err)


remove the "????"

caesarxuchao · 2017-05-26T20:39:43Z

plugin/pkg/admission/webhook/admission_test.go

+	rootCAs.AppendCertsFromPEM(caCert)
+	testServer := httptest.NewUnstartedServer(http.HandlerFunc(webhookHandler))
+	testServer.TLS = &tls.Config{
+		Certificates: []tls.Certificate{cert},


Is it correct to let the server use a cert generated from cleintCert and clientKey?

caesarxuchao · 2017-05-26T21:46:30Z

plugin/pkg/admission/webhook/certs_test.go

+Z0gPepfIFptDMl5wKWyw4o2XVSZ69Ur8tQQynoNyX/FTIx6tQ//hzg==
+-----END CERTIFICATE-----`)
+
+var badCAKey = []byte(`-----BEGIN RSA PRIVATE KEY-----


Forget to add a test that uses badCAKey?

caesarxuchao · 2017-05-26T21:57:35Z

plugin/pkg/admission/webhook/rules_test.go

+		"wildcard": {
+			rule: apiserver.RuleWithOperations{
+				Rule: apiserver.Rule{
+					APIGroups: []string{"*"},


The validation rule requires the APIGroups/APIVersions/Resources/Operations to be non-empty. Can you update these rules to be valid?

smarterclayton · 2017-05-30T16:20:53Z

cmd/kube-apiserver/app/options/options.go

-	fs.StringVar(&s.ProxyClientKeyFile, "proxy-client-key-file", s.ProxyClientKeyFile,
-		"client certificate key used to prove the identity of the aggragator or kube-apiserver when it proxies requests to a user api-server")
+	fs.StringVar(&s.ProxyClientCertFile, "proxy-client-cert-file", s.ProxyClientCertFile, ""+
+		"Client certificate used to prove the identity of the aggragator or kube-apiserver "+


smarterclayton · 2017-05-30T16:22:28Z

pkg/kubeapiserver/admission/initializer.go

-	cloudConfig    []byte
-	restMapper     meta.RESTMapper
-	quotaRegistry  quota.Registry
+type WantsServiceResolver interface {


Quick godoc on each

smarterclayton · 2017-05-30T16:24:21Z

plugin/pkg/admission/webhook/admission.go

+	if e.Reason != "" {
+		return fmt.Sprintf("rejected by webhook %q: %v", e.WebhookName, e.Reason)
+	}
+	return fmt.Sprintf("rejected by webhook %q without explanation", e.WebhookName)


rejected by admission webhook ... for both

smarterclayton · 2017-05-30T16:25:16Z

plugin/pkg/admission/webhook/admission.go

+	"k8s.io/kubernetes/pkg/api"
+	admissionv1alpha1 "k8s.io/kubernetes/pkg/apis/admission/v1alpha1"
+	"k8s.io/kubernetes/pkg/apis/admissionregistration"
+	adinit "k8s.io/kubernetes/pkg/kubeapiserver/admission"


weird prefix, I don't care that much but should be admissioninit

smarterclayton · 2017-05-30T16:26:02Z

plugin/pkg/admission/webhook/admission.go

+			defer wg.Done()
+			allowed, reason, err := a.callHook(hook, attr)
+			if err != nil {
+				glog.Warningf("Failed calling webhook %v: %v", hook.Name, err)


utilruntime.HandleError()

smarterclayton · 2017-05-30T16:26:48Z

plugin/pkg/admission/webhook/admission.go

+	wg.Wait()
+	close(errCh)
+
+	errs := []error{}


var errs []error avoids allocating if errors is empty.

smarterclayton · 2017-05-30T16:27:43Z

plugin/pkg/admission/webhook/admission.go

+		return fmt.Errorf("failed listing hooks: %v", err)
+	}
+
+	errCh := make(chan error, len(hooks))


wire a context through here so we can add cancelation in the future. context.TODO is fine.

file a follow up issue on admission that we need to be passing a context down into the admission chain.

smarterclayton · 2017-05-30T16:29:34Z

plugin/pkg/admission/webhook/admission.go

+	}
+
+	var statusCode int
+	if response.StatusCode(&statusCode); statusCode < 200 || statusCode >= 300 {


This is unnecessary and already handled by response.Error().

smarterclayton · 2017-05-30T16:30:03Z

plugin/pkg/admission/webhook/admission.go

+		return false, "", fmt.Errorf("error contacting webhook %q: %d", h.Name, statusCode)
+	}
+
+	if err := response.Into(&request); err != nil {


Fold this into 172.

smarterclayton · 2017-05-30T16:31:22Z

plugin/pkg/admission/webhook/admission.go

+
+	if !request.Status.Allowed {
+		if request.Status.Result != nil {
+			reason = string(request.Status.Result.Reason)


We should be returning the actual status back, and then converting it to an error. We shouldn't be transforming status -> string -> error.

I.e. the status field should be passed (if specified) otherwise we need to create a stub one which is generic admission failure. That status should then be wrapped in StatusErr{}. I think you could just get away with returning the error here - reason doesn't need to be separately split.

smarterclayton · 2017-05-30T16:32:22Z

plugin/pkg/admission/webhook/admission.go

+	}
+
+	// TODO: cache these instead of constructing one each time
+	cfg := &rest.Config{


Going to be really expensive to do this on every single admission call. Certainly can't afford to do it at scale. How high scale are you planning on using this at? I would say open a bug for this and we can fix it post dcut.

This is alpha. So I think high scale isn't super important. If this is too limiting then we can fix it as a bug.

smarterclayton · 2017-05-30T16:34:43Z

For e2e test we really need a way to start an image that has embedded code to act as a server. Do we have a small image we use in e2e that has go installed in it (so we can go run a file provided via env / shell arg)?

To properly register the types in the admission API group we need to create an "install" package and wire it up. This is required by the webhook admission controller being developed as part of kubernetes/community#132

As part of kubernetes/community#132, thsi commit adds a generic webhook admission controller. This plugin allows for a completely declarative approach for filtering/matching admission requests and for matching admission requests, calls out to an external webhook for handling admission requests.

lavalamp · 2017-05-31T19:22:12Z

@k8s-bot pull-kubernetes-e2e-gce-etcd3 test this

lavalamp · 2017-05-31T22:33:55Z

@k8s-bot pull-kubernetes-e2e-gce-etcd3 test this

caesarxuchao

lgtm. One nit.

caesarxuchao · 2017-05-31T22:42:05Z

plugin/pkg/admission/webhook/admission.go

+limitations under the License.
+*/
+
+// Package webhook checks a webhook for configured operation admission


nit: s/checks a webhook/checks webhooks

lavalamp · 2017-05-31T23:00:35Z

if the etcd3 test fails again I will fix the nit, otherwise I'll fix in a followup.

Also fix a bug in rest client

smarterclayton · 2017-05-31T23:57:36Z

/lgtm

May god have mercy on our souls

k8s-ci-robot · 2017-06-01T00:20:12Z

@lavalamp: The following test(s) failed:

Test name	Commit	Details	Rerun command
pull-kubernetes-cross	`c46e231`	link	`@k8s-bot pull-kubernetes-cross test this`

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-github-robot · 2017-06-01T01:02:31Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lavalamp, smarterclayton

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

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

~~cmd/kube-apiserver/OWNERS~~ [lavalamp,smarterclayton]
~~hack/OWNERS~~ [lavalamp,smarterclayton]
~~pkg/api/OWNERS~~ [lavalamp,smarterclayton]
~~pkg/kubeapiserver/OWNERS~~ [lavalamp,smarterclayton]
~~pkg/kubectl/OWNERS~~ [lavalamp,smarterclayton]
~~pkg/master/OWNERS~~ [lavalamp,smarterclayton]
~~plugin/OWNERS~~ [lavalamp,smarterclayton]
~~staging/src/k8s.io/apimachinery/pkg/OWNERS~~ [lavalamp,smarterclayton]
~~staging/src/k8s.io/client-go/OWNERS~~ [lavalamp,smarterclayton]
~~test/OWNERS~~ [lavalamp,smarterclayton]

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

smarterclayton · 2017-06-01T14:52:58Z

@k8s-bot pull-kubernetes-e2e-gce-etcd3 test this

caesarxuchao · 2017-06-01T23:12:30Z

pkg/kubeapiserver/admission/initializer.go

+
+// SetWebhookSource sets the webhook source-- admittedly this is probably
+// specific to the external admission hook plugin.
+func (i *PluginInitializer) SetWebhookSource(w WebhookSource) *PluginInitializer {


I'm going to remove SetWebhookSource. This should be a SetExternalClient, and we should initialize a webhookconfiguration manager in the method, like what Clayton did in b75238f#diff-3551f205184f7273b78e802da8fdc72fR93.

k8s-github-robot · 2017-06-03T06:37:41Z

Automatic merge from submit-queue (batch tested with PRs 46239, 46627, 46346, 46388, 46524)

Automatic merge from submit-queue (batch tested with PRs 47204, 46808, 47432, 47400, 47099) Make the generic webhook admission controller use the dynamic webhook config manager Based on #46672 and #46388. Only the last commit is unique. * removed `SetWebhookSource` from the PluginInitializer * implemented `SetExternalClientset` for the generic webhook admisson controller, initializing an ExternalWebhookConfigurationManager in the method.

Automatic merge from submit-queue (batch tested with PRs 46076, 43879, 44897, 46556, 46654) Use meta.v1 GroupVersionKind with json tags to generate OpenAPI spec We are using two different GVK struct in generation of OpenAPI extensions. This PR unify that and also add json tags to meta.v1 GVK to comply with json naming system in other serializations. Also the value of Action extension is now lowercase. ref: kubernetes/kubernetes#46388

Automatic merge from submit-queue (batch tested with PRs 46076, 43879, 44897, 46556, 46654) Use meta.v1 GroupVersionKind with json tags to generate OpenAPI spec We are using two different GVK struct in generation of OpenAPI extensions. This PR unify that and also add json tags to meta.v1 GVK to comply with json naming system in other serializations. Also the value of Action extension is now lowercase. ref: kubernetes/kubernetes#46388 Kubernetes-commit: e97b72296f36f0282fc9453fe288d249d906667b

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 24, 2017

k8s-github-robot assigned luxas and smarterclayton May 24, 2017

k8s-github-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. release-note-label-needed labels May 24, 2017

whitlockjc reviewed May 25, 2017

View reviewed changes

lavalamp mentioned this pull request May 25, 2017

Add initializer support to admission and uninitialized filtering to rest storage #36721

Merged

smarterclayton reviewed May 26, 2017

View reviewed changes

whitlockjc mentioned this pull request May 26, 2017

add generic webhook admission controller #46316

Closed

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

lavalamp force-pushed the whitlockjc-generic-webhook-admission branch from 61cbce5 to 374b5b7 Compare May 26, 2017 21:08

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

caesarxuchao reviewed May 26, 2017

View reviewed changes

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

mbohlool mentioned this pull request May 27, 2017

Use meta.v1 GroupVersionKind with json tags to generate OpenAPI spec #46556

Merged

smarterclayton reviewed May 30, 2017

View reviewed changes

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

smarterclayton reviewed May 30, 2017

View reviewed changes

lavalamp force-pushed the whitlockjc-generic-webhook-admission branch from 23e3a52 to 0ab9415 Compare May 30, 2017 18:30

whitlockjc added 4 commits May 31, 2017 11:41

add "install" package for admission API group

83d3d59

To properly register the types in the admission API group we need to create an "install" package and wire it up. This is required by the webhook admission controller being developed as part of kubernetes/community#132

add generated sources for admission API group "install" package

5375bc0

add generated sources for webhook admission plugin

f4403d2

lavalamp force-pushed the whitlockjc-generic-webhook-admission branch from 5d12dc1 to 04a7269 Compare May 31, 2017 18:42

caesarxuchao reviewed May 31, 2017

View reviewed changes

lavalamp added 2 commits May 31, 2017 16:38

Implement dynamic admission webhooks

d6e1140

Also fix a bug in rest client

generated changes

c46e231

lavalamp force-pushed the whitlockjc-generic-webhook-admission branch from 04a7269 to c46e231 Compare May 31, 2017 23:38

lavalamp unassigned luxas May 31, 2017

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

k8s-github-robot added the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Jun 1, 2017

smarterclayton added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. release-note-label-needed labels Jun 1, 2017

smarterclayton added this to the v1.7 milestone Jun 1, 2017

caesarxuchao reviewed Jun 1, 2017

View reviewed changes

caesarxuchao mentioned this pull request Jun 1, 2017

Make the generic webhook admission controller use the dynamic webhook config manager #46808

Merged

k8s-github-robot merged commit e837c3b into kubernetes:master Jun 3, 2017

Dynamic webhook admission control plugin #46388

Dynamic webhook admission control plugin #46388

Conversation

lavalamp commented May 24, 2017 • edited by smarterclayton Loading

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

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

smarterclayton commented May 30, 2017

lavalamp commented May 31, 2017

lavalamp commented May 31, 2017

caesarxuchao left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lavalamp commented May 31, 2017

smarterclayton commented May 31, 2017

k8s-ci-robot commented Jun 1, 2017 • edited Loading

k8s-github-robot commented Jun 1, 2017

smarterclayton commented Jun 1, 2017

Choose a reason for hiding this comment

k8s-github-robot commented Jun 3, 2017

lavalamp commented May 24, 2017 •

edited by smarterclayton

Loading

k8s-ci-robot commented Jun 1, 2017 •

edited

Loading