Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

⚠️ webhook redesign for generic case #323

Merged
merged 19 commits into from
Feb 21, 2019

Conversation

DirectXMan12
Copy link
Contributor

@DirectXMan12 DirectXMan12 commented Feb 11, 2019

While implementing a webhook, I found some tech debt and rough corners around the webhook code. This is an attempt to smooth some of that over.

@k8s-ci-robot 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 Feb 11, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: DirectXMan12

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:

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

@DirectXMan12
Copy link
Contributor Author

cc @mengqiy

@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Feb 11, 2019
@DirectXMan12
Copy link
Contributor Author

(built on top of #300 )

@DirectXMan12 DirectXMan12 changed the title [WIP] ⚠️ webhook refactor and redesign [WIP] ⚠️ webhook refactor and redesign (for generic webhooks) Feb 11, 2019
@DirectXMan12 DirectXMan12 changed the title [WIP] ⚠️ webhook refactor and redesign (for generic webhooks) [WIP] ⚠️ webhook redesign for generic case Feb 11, 2019
@DirectXMan12
Copy link
Contributor Author

/assign @mengqiy

Copy link
Contributor

@droot droot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love this change. I have couple of comments. I will take another look from the perspective of adding conversion-webhook-support tomorrow.

example/main.go Outdated
os.Exit(1)
}
hookServer.Register("/mutate-pods", webhook.Admission{&podAnnotator{}})
hookServer.Register("/validate-pods", webhook.Admission{&podValidator{}})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks very neat, easier to follow. This means path is now decoupled from the webhook definition.

// podAnnotator adds an annotation to every incoming pods.
func (a *podAnnotator) Handle(ctx context.Context, req types.Request) types.Response {
func (a *podAnnotator) Handle(ctx context.Context, req admission.Request) admission.Response {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 on readability admission.{Request, Response} as http.{Request, Response}

)

var admissionv1beta1scheme = runtime.NewScheme()
var admissionv1beta1schemecodecs = serializer.NewCodecFactory(admissionv1beta1scheme)
var admissionScheme = runtime.NewScheme()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this scheme needs user defined types ? (basically, should we be injected from the manager ?)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This decoder only need to understand AdmissionReviewRequest. So IMO no need to be injected.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it.

func (hs multiMutating) Handle(ctx context.Context, req Request) Response {
patches := []jsonpatch.JsonPatchOperation{}
for _, handler := range hs {
resp := handler.Handle(ctx, req)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like these handlers are independent, so can probably be invoked concurrently ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe, but I almost want to deprecate this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe, but I almost want to deprecate this.

Reasons?

func instrumentedHook(path string, hookRaw http.Handler) http.Handler {
return http.HandlerFunc(func(resp http.ResponseWriter, req *http.Request) {
startTS := time.Now()
defer metrics.RequestLatency.WithLabelValues(path).Observe(time.Now().Sub(startTS).Seconds())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to wrap it with func{} otherwise the args time.Now().Sub(startTS).Seconds() will be evaluated immediately reporting almost no latency :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch!
The deferred call's arguments are evaluated immediately, but the function call is not executed until the surrounding function returns.
Ref: https://tour.golang.org/flowcontrol/12

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ack, yep

Copy link
Member

@mengqiy mengqiy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A lot of improvement!

I have several comment inlined.

example/main.go Outdated
Port: 9876,
CertDir: "/tmp/cert",
})
if err != nil {
entryLog.Error(err, "unable to create a new webhook server")
os.Exit(1)
}
mgr.Add(as)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this will work, since mgr.Add() actually does the deps injection.
It should be done after registering all the handlers.
Thus, I proposed using Complete method to finish the setup.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Start does the deps injection, really. Add just records the setFields method for later.

type Server struct {
// Name is the name of server
Name string
// TODO(directxman12): should we make the mux configurable?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a use case in your head that the user may want to configure mux?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you want to serve on an existing handler more easily. It's not clear that there's a concrete usecase, but I can imagine someone saying something at some points.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(e.g. your comment below about Handle)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(e.g. your comment below about Handle)

We probably should make webhookMux field configurable.

registry map[string]Webhook
// webhookMux is the multiplexer that handles different webhooks.
webhookMux *http.ServeMux
// hooks keep track of all registered webhooks for dependency injection,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: do we want to use hooks or webhooks here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

comment still doesn't match field name

}

return nil
}

// Handle registers a http.Handler for the given pattern.
func (s *Server) Handle(pattern string, handler http.Handler) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The original intend was to give the user flexibility to add additional handlers. e.g. statusz, healthz etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right, but they shouldn't be doing it on the webhook server. They should do it elsewhere, or we should let people pass in their own mux.

example/mutatingwebhook.go Show resolved Hide resolved
example/validatingwebhook.go Show resolved Hide resolved
pkg/webhook/server.go Show resolved Hide resolved
pkg/webhook/server.go Show resolved Hide resolved
@@ -98,14 +89,6 @@ func (wh *Webhook) ServeHTTP(w http.ResponseWriter, r *http.Request) {
}

func (wh *Webhook) writeResponse(w io.Writer, response Response) {
if response.Result.Code != 0 {
if response.Result.Code == http.StatusOK {
metrics.TotalRequests.WithLabelValues(wh.Path, "true").Inc()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems metrics TotalRequests has been dropped.
Is it not useful?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not clear how useful it is (accepting or rejecting is a normal occurrence -- you really probably want to see errors and error codes if you're trying to alert, etc), and it was difficult to work in the new code structure. There may be a way to implement it generically, but I figured it was fine for a follow-up PR + the scream test.

@DirectXMan12 DirectXMan12 changed the title [WIP] ⚠️ webhook redesign for generic case ⚠️ webhook redesign for generic case Feb 20, 2019
@k8s-ci-robot 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 Feb 20, 2019
@DirectXMan12
Copy link
Contributor Author

@mengqiy this is ready for more review. I'll write a better description tomorrow. Still want to follow it up with a better story around generating patches and returning errors, perhaps.

@DirectXMan12 DirectXMan12 force-pushed the tmp/webhook-decouple branch 2 times, most recently from a25f015 to 8623a9b Compare February 20, 2019 04:22
@droot
Copy link
Contributor

droot commented Feb 20, 2019

Looks good to me. Will defer it to @mengqiy for the final LGTM.

Copy link
Member

@mengqiy mengqiy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ha, 20 commits now.
Overall looks good.

move webhook self installer to CT as generator

The 1st commit should be gone after the rebase. Why it is still there?

@@ -34,6 +36,21 @@ func NewDecoder(scheme *runtime.Scheme) (*Decoder, error) {

// Decode decodes the inlined object in the AdmissionRequest into the passed-in runtime.Object.
func (d *Decoder) Decode(req Request, into runtime.Object) error {
// NB(directxman12): there's a bug/weird interaction between decoders and
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like a workaround until apimachinery folks fix the bug.
We can probably link to the upstream issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ack, need to file the issue

Handler: admission.HandlerFunc(func(ctx context.Context, req AdmissionRequest) AdmissionResponse {
return Patched("some changes",
JSONPatchOp{Operation: "add", Path: "/metadata/annotations/access", Value: "granted"},
JSONPatchOp{Operation: "add", Path: "/metadata/annotations/access", Value: "granted"},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is strange to have a duplicate JSONPatch in the example, isn't it?

// podAnnotator implements inject.Client.
// A client will be automatically injected.
var _ inject.Client = &podAnnotator{}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why removing this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't really add much, and necessitates an extra import. I can add it back in if we really want it, but I think the comment suffices here.

@@ -22,7 +22,8 @@ import (
"reflect"

"k8s.io/apimachinery/pkg/api/errors"
"k8s.io/apimachinery/pkg/apis/meta/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

duplicate package?

@@ -0,0 +1,105 @@
/*
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe everything under pkg/internal/webhookgenerator is rebase artifacts and should not exist.

invoked bool
fn func(context.Context, Request) Response
decoder *Decoder
injectedString string
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it actually tested anywhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep, pkg/webhook/admission/webhook_test.go

This makes sure to run go vet on the examples and the alias file, and
turns goimports back on.
We had goimports turned off.  When turned back on, it complained about a
bunch of stuff.  This fixes that.
The example was still using the old list options arguments style (second
argument instead of last argument), which made it fail to compile.

This also makes sure we run go test on the whole project, instead of
just pkg/...
The webhook server logic was not handling injection correctly -- it
implemented injection for a couple of specific fields, rather than
receiving and dealing with an injectfunc.  This fixes that.

As a side effect, it also removes the handle to manager and client from
the webhook server, since those aren't used by the server, and should
thus be injected from the manager.
This flattens down the webhook package structure, removing dedicated
`types` packages in favor of having things in the relevant places.  To
do this, the inject interface definitions for admission were also moved
to the admission controller location, but they actually make some amount
of sense living there.

It also renames and restructures a couple of the types for code clarity (e.g.
WebhookTypeMutating to MutatingWebhook) or to follow the convention of
other types in CR (e.g. Making wrapped types from core Kubernetes nested
fields).
We don't have anywhere that we actually make use of decoder as an
interface, so switch to just have a concrete implementation.
This extracts out the multi-handler support into separate interfaces.
That simplifies the code, makes it easier to test directly, and allows
us to drop the now-extraneous Type field.  This simultaneously removes
the builder (for now) since it doesn't actually make anything simpler.

Along the way, we also refactor some common functionality out into a
helper on reponse itself as opposed to being run by different bits of
the webhook.
This cleans up tests to more closely follow CR style, favoring
behavior-style tests as opposed to pure unit tests.
This renames a few ambigously named fields in the webhook server,
removes some extraneous fields and methods, and removes the unnecessary
constructor for Server.
It was only being used for registering metrics, and that can be done
with the path instead.
This removes the path functionality from admission webhooks, since it's
not strictly needed.  Relevant metrics are moved up to the webhook
server, and path is added to register.  This also introduces a couple of
helpers that should make code a bit clearer when writing admission
webhooks (Allowed, Denied, Patched), and an alias file to avoid
importing both webhook and webhook/admission.
This ensures that decoders (which are no longer constructed in the
manager) are properly injected into webhook handlers.  The webhook
itself receives a scheme, which it uses to construct a decoder.
Due to a weird interaction between the unstructured decoder (which
demands APIVersion and Kind fields) and the API server (which fails to
set those fields on the embedded object in admission requests), we can't
use the normal decoders, nor can we call json.Unmarshal directly on the
unstructured (since it implements an unmarshaller that calls back into
the decoder).

This detects unstructured objects, and does the right thing for them.
This moves the examples to an actual Go example file, so that we can be
sure that they actually compile.
It's not used anywhere any more.
It's the same name used in alias.go, and it produces less-verbose code
without loss of readability.
The were incorrect (would not have compiled), are mostly duplicates
of examples/*webhook.go, and can't be easily checked for being
up-to-date.
This gives each webhook a log, so that serving errors can be tied to a
particuluar webhook.
@mengqiy
Copy link
Member

mengqiy commented Feb 21, 2019

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 21, 2019
Nothing was missing from the tree (due to transitive deps), just missing
from being required by Gopkg.lock.
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 21, 2019
@mengqiy
Copy link
Member

mengqiy commented Feb 21, 2019

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 21, 2019
@k8s-ci-robot k8s-ci-robot merged commit 58a08d8 into kubernetes-sigs:master Feb 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants