-
Notifications
You must be signed in to change notification settings - Fork 112
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
Admission control webhooks for synchronous validation #596
Comments
https://kubernetes.slack.com/archives/C019ZRGUEJC/p1613568667090500 - a slack conversation between @imjasonh , @otaviof , and myself at the time of this comment some almost tl;dr level details in there :-) |
We currently do a lot of validations in advance on the Because a
but in terms of miss-configuration, there is not too much more benefit. Is my understanding that Tekton does not validate the If the webhook can block users from creating a Opinions? EDIT: My above comment is mainly around validating config via webhooks. I understand the webhook have other benefits, e.g. default values definition. |
Tekton validates Task definitions as well: $ cat task.yaml
apiVersion: tekton.dev/v1beta1
kind: Task
metadata:
name: test
spec:
steps:
$ kubectl apply -f task.yaml
Error from server (BadRequest): error when creating "task.yaml": admission webhook "validation.webhook.pipeline.tekton.dev" denied the request: validation failed: missing field(s): spec.steps And I think Shipwright should also synchronously validate Builds as well. |
@imjasonh I see. Yes, this will definitely be nice to have for both Build/BuildRun. I know Tekton have a controller to generate webhook certificates, is this something that you think we also need for the admission one? |
Tekton uses I honestly don't know much about how webhooks (certs especially) are configured for other frameworks like |
I'll dig up a non-knative reference and post it here. Technically speaking I think we could use the knative plumbing to build our cmd/webhook image/depoyment/etc. I used that to positive effect back when I was trying to get some RBAC related scenarios addressed in tekton directly vs. the keep it out of tekton OPA approach the community currently is on. I don't remember it having specific dependencies on the framework used for the controller/reconciler, but perhaps you are aware of something I'm forgetting @imjasonh But even if it is possible, it would be good to compare the pros and cons of both approaches and get to a consensus on what is desired choice. |
Yeah, I also think we could. I do think eventually we'd prefer to only rely on one framework, and not mix controller-runtime and knative/pkg. So given that, I think we should start by trying to run the webhook job without relying on knative/pkg. Gabe, any more guides/examples you could provide would be helpful there. 👍 |
I remember we had an initial discussion at the beginning, also cc @sbose78 , at that time, we try to avoid introducing the webhook to make shipwright build simple. Maybe we can discuss that again. I think for shipwright build, I don't know if we will introduce more parameters in future. But for the existing parameters, I am not sure if the default schema validation is ok for us, like this one: #474 |
@imjasonh after checking out a few options, I'm a bit torn :-). I still like the basic building blocs provided by k8s, as noted in this blog post: https://kubernetes.io/blog/2019/03/21/a-guide-to-kubernetes-admission-controllers Where the And your impl has the start/validate/mutate entrypoints and the flexibility/power integrating at a "lower level". That said, sure things like controller-runtime and knative supply some additional abstractions and functions (cert regen IIRC). I think controller runtime's are at https:/kubernetes-sigs/controller-runtime/tree/master/pkg/webhook |
Someone shared this during the community call yesterday and I forgot who it was now (sorry!) but it seems useful: https://book.kubebuilder.io/cronjob-tutorial/webhook-implementation.html |
Apologies to the non-Red Hatters, but we are having an impromptu deep dive on knative vs. operator-fw/OLM at https://coreos.slack.com/archives/C3VS0LV41/p1616609181255300 Between @imjasonh and myself we need to provide a end synopsis after it settles In the interim, some of those links @imjasonh asked for back in #596 (comment)
|
Only relying on k8s primitives certainly sounds nice, but then you end up building a lot of infra code that ends up taking up a lot of eng cycles. controller-runtime seems to not include cert (re)generation by default, and expects clients to generate and manage those themselves. knative/pkg's webhook package (here) does that automatically, which is really nice. The "downside" (if you can call it that) is that adopting knative/pkg for webhooks probably means we should adopt it for the controller framework too, which honestly wouldn't be that bad IMO. I'll try to scope out how much work that is too to get an idea how big an undertaking that would be, and if we want to do it. |
I think https://coreos.slack.com/archives/C3VS0LV41/p1616609181255300 does not work for me :) Looking forward for more information on what we think is the best approach for us. |
Kubernetes allows controllers to register as admission control webhooks, which get called synchronously when resources are created or updated, and can reject invalid objects from being created, or modify requests on-the-fly to set defaults for example.
Shipwright doesn't currently configure an admission control webhooks, so users can currently create invalid resources (e.g., a BuildRun that references a Build that doesn't exist). Shipwright's controller will notice these and correctly update them to a failed status, but that feedback isn't immediate to users.
The controller-runtime pkg/webhook package provides methods to register a webhook.
The text was updated successfully, but these errors were encountered: