TokenReview.authentication.k8s.io resource helpers for Wehbook Server

[christopherhein]

Would Controller Runtime be interested in being able to support non-AdmissionReview based HTTP handlers in the Webhook server package. I'm thinking of resources like authentication.k8s.io.TokenReview imagepolicy.k8s.io.ImageReview.

EG if I could use:

mgr.GetWebhookServer().Register("/tokenreview", &authentication.Webhook{Handler: &wh.TokenReview{}})

And it would then have something like:

import "k8s.io/api/authentication/v1"
// ...
type Request struct {
  v1.TokenReview
}

and my Handler func could be:

func (a *TokenReview) Handle(ctx context.Context, req authentication.Request) authentication.Response {
  // ... implement authentication request
  return authentication.Response{}
}

This would help to standardize the building of ImageReviews and TokenReviews for code bases like https://sigs.k8s.io/aws-iam-authenticator where it's currently just a normal go HTTP server implementation and where you need to add controllers (when we did) we just used client-go directly and we could have used CR easier.

[christopherhein]

/cc @pwittrock @vincepri @alvaroaleman

[coderanger]

We already also support conversion hooks, so this seems fine. I think we should probably keep it scoped to K8s-specific hooks just to avoid getting into generic web framework territory but these should work similarly to the existing hooks.

[christopherhein]

Yea, was thinking that as well, as long as it's exposed by core k8s APIs we could support it.

…and agree, avoiding the web framework side since you can always use a new Runnable.

[alvaroaleman]

We already also support conversion hooks, so this seems fine. I think we should probably keep it scoped to K8s-specific hooks just to avoid getting into generic web framework territory but these should work similarly to the existing hooks.

So right now, the signature allows you to register anything: Register(path string, hook http.Handler). IMHO that is fine?

Adding helpers to make writing other Kubernetes hook handlers easier sounds like a good idea to me 👍

[christopherhein]

So right now, the signature allows you to register anything: Register(path string, hook http.Handler). IMHO that is fine?

Yea I ended up implementing that for what I was doing but I'm basically maintaining a near replica of the admissions ServeHTTP func and felt like it could be upstreamed. Cool sounds like this might be of use.

[coderanger]

Also FWIW token review is probably more interesting than image review. Or at least I never really understand why anyone would use image review hooks over a validating webhook on Pods which you can already do nicely in c-r :)

[christopherhein]

Agreed, my use case is TokenReview especially with the new changes coming about for ServiceAccount tokens, I didn't know if adding ImageReview would make other use cases easier to enable.

[rfranzke]

We run an authorization webhook handler based on the controller-runtime's webhook server (https://github.com/gardener/gardener/blob/1cf5a18cc55b413b482fa3b27a100a11f4da3577/pkg/admissioncontroller/webhooks/auth/seed/handler.go#L67-L204), and this handler was heavily inspired from the admission handling in this repository. If you are interested I would be happy to contribute those helpers (it's very similar to TokenReview for authentication).

[coderanger]

Another webhook that sees some level of usage is the audit webhook system but that's usually a different kind of app so might be out of scope for us :)

[christopherhein]

@coderanger do you think it makes sense to make another issue to track doing the audit webhook support?

[christopherhein]

@rfranzke now that the first TokenReview is merged do you think we could add another issue to track the authorization webhook? Also is that something you're interested in adding?