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

Dynamic type webhooks #62

Merged
merged 4 commits into from
May 15, 2020
Merged

Dynamic type webhooks #62

merged 4 commits into from
May 15, 2020

Conversation

slok
Copy link
Owner

@slok slok commented May 12, 2020

Fixes #36

This PR adds the ability to use dynamic webhooks. This webhooks infer the type of the request based on the received review JSON data. If the type can't be inferred because is unknown then it will fallback to runtime.Unstructured object type.

We consider unknown types (core objects that are not tracked by the webhook kubernetes libraries and CRDs).

These dynamic webhooks will be activated when the webhook factory receives a configuration without the Obj field set.

The webhooks have been refactored internally to use a internal ObjectCreator that is the one responsible to create the object from the received raw JSON, so we have 2, a Static and Dynamic one.

These webhooks should be also very helpful on multitype webhooks or webhooks that mutate/validate metadata fields like labels.

slok added 4 commits May 11, 2020 12:44
Signed-off-by: Xabier Larrakoetxea <[email protected]>
Signed-off-by: Xabier Larrakoetxea <[email protected]>
Signed-off-by: Xabier Larrakoetxea <[email protected]>
@slok slok self-assigned this May 12, 2020
@turkenh
Copy link

turkenh commented May 15, 2020

Looking forward for this PR.
Is there an ETA?

@slok
Copy link
Owner Author

slok commented May 15, 2020

Looking forward for this PR.
Is there an ETA?

Hey @turkenh Sorry fo not merging it in 3 days, I normally merge once I make the PR and passes the CI, in this case I wanted to test it a little bit more, so I was setting up a production webhook with so I could test it for a while in a production environment, I've had the webhook ~32h hours mutating multiple types and seems to be ok, in mutation and in resource consumption, so I'll merge into master.

If you start using it and detect problems or bugs, please open an issue so I can check and fix it.

Thanks!

Edit: I'll make a new release this weekend. Meanwhile, you can use master

@slok slok merged commit 823fc44 into master May 15, 2020
@slok slok deleted the dynamic branch May 15, 2020 10:19
@turkenh
Copy link

turkenh commented May 15, 2020

@slok thanks a lot!

Testing with a validating webhook for DELETE requests and getting the following error:

Error from server (InternalError): Internal error occurred: failed calling webhook "delete-validating-webhooks": an error on the server ("{\"response\":{\"uid\":\"3c1de137-8371-4ba8-a6d1-3bd6aeae511f\",\"allowed\":false,\"status\":{\"metadata\":{},\"status\":\"Failure\",\"message\":\"unexpected end of JSON input\"}}}") has prevented the request from succeeding

Any ideas?

@turkenh
Copy link

turkenh commented May 15, 2020

Could this be related to not having request body for delete requests?
Related: #41

if !ok {
err := fmt.Errorf("could not type assert metav1.Object to runtime.Object")
// Create a new object from the raw type.
runtimeObj, err := w.objectCreator.NewObject(ar.Request.Object.Raw)
Copy link

Choose a reason for hiding this comment

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

Should we use ar.Request.oldObject.Raw is it is a delete operation?

@turkenh
Copy link

turkenh commented May 15, 2020

Just tested and same webhook works for UPDATE requests.

@slok
Copy link
Owner Author

slok commented May 15, 2020

Yes, is related with #41.

I opened an issue #63

I'll try fixing this in the following days! many thanks for the feedback!!

@slok
Copy link
Owner Author

slok commented May 16, 2020

@turkenh I just merged the fix in #64.

Could you please, test the DELETE operation on your validation webhook?

Let's see if now it doesn't fail on unmarshaling, you can access the object in the webhook, validate the deletion object, and see if it works correctly all the process.

Thanks!

@turkenh
Copy link

turkenh commented May 18, 2020

@slok yes, I can verify the issue is fixed on my side and I can now use dynamic type webhook with DELETE operation.

Thanks a lot for quick fix!

@slok
Copy link
Owner Author

slok commented May 18, 2020

Awesome!

I'll publish a release this week with these changes.

Many thanks for your help and feedback!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Same webhook for different kinds of resource
2 participants