Test webhook2 #33
Test webhook2 #33
Conversation
There was a problem hiding this comment.
@zekemorton I don't see the obvious answer so I'll tell you how I'd work on this - if you look in other operator examples (production) you can find a bunch that have webhooks, and compare what they do to what we do in detail. E.g.,:
- the config files
- the commands to start (e.g the Makefile)
- how they debug / look at logs for the webhook (this could be very helpful to debug)
- how they register / interact with it
The reason I'm worried about api issues is that after creating one (and seeing the minicluster print that out) I don't see any logs:
$ kubectl get validatingwebhookconfigurations
No resources found
So perhaps debugging that outside of the operator makes sense first - if the API has changed it could be that we aren't actually creating it - so maybe kubebuilder / the operator print that it's created but kubernetes is like whut? 😆
| Copyright 2022. | ||
|
|
||
| Licensed under the Apache License, Version 2.0 (the "License"); | ||
| you may not use this file except in compliance with the License. | ||
| You may obtain a copy of the License at | ||
|
|
||
| http://www.apache.org/licenses/LICENSE-2.0 | ||
|
|
||
| Unless required by applicable law or agreed to in writing, software | ||
| distributed under the License is distributed on an "AS IS" BASIS, | ||
| WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| See the License for the specific language governing permissions and | ||
| limitations under the License. | ||
| */ |
There was a problem hiding this comment.
We will eventually want our LLNL header here, although after we re-write most of the code so it's our own!
|
|
||
| if err := (&api.MiniCluster{}).SetupWebhookWithManager(mgr); err != nil { | ||
| setupLog.Error(err, "unable to create webhook", "webhook", "MiniCluster") | ||
| return "MiniCluster", err |
There was a problem hiding this comment.
Hmm - so this core.go function is specifically to create controllers. Either we rename it to be about creating resources, or we move the webhook creation to be outside of it, back as its own function from main.go (probably ideal). The error message in main.go that this returns from is referencing a controller failed creation, which could be confusing if it turns out it was a webhook error.
There was a problem hiding this comment.
Thats good to know, I think the operator sdk originally puts this in main.go, I can't remember why exactly I moved it here
There was a problem hiding this comment.
Oh I think it would be ok in this file, but maybe in a separate function just to init the webhooks!
| @@ -107,7 +107,6 @@ func main() { | |||
| setupLog.Error(err, "Unable to create controller", "controller", failedCtrl) | |||
| os.Exit(1) | |||
| } | |||
| //+kubebuilder:scaffold:builder | |||
There was a problem hiding this comment.
Did you mean to delete this line?
There was a problem hiding this comment.
No, I may have deleted it with the the changes that the operator made to main.go
| @@ -107,7 +107,6 @@ func main() { | |||
| setupLog.Error(err, "Unable to create controller", "controller", failedCtrl) | |||
| os.Exit(1) | |||
There was a problem hiding this comment.
So one pattern I'm seeing in some operators is defining the webhook server explicitly: https://github.com/kubernetes-sigs/controller-runtime/blob/c83076e9f79258f23bcb200cf2391cf4ade939cf/pkg/manager/manager.go#L245-L248. I think it's intended for external / custom ones, but could be something to try for here. E.g.,
webhookServer := webhook.Server{
CertDir: "/tmp/k8s-webhook-server/serving-certs",
CertName: "tls.crt",
KeyName: "tls.key",
}
mgr, err := ctrl.NewManager(ctrl.GetConfigOrDie(), ctrl.Options{
Scheme: scheme,
MetricsBindAddress: metricsAddr,
Port: 9443,
HealthProbeBindAddress: probeAddr,
LeaderElection: enableLeaderElection,
LeaderElectionID: "14dde902.flux-framework.org",
WebhookServer: &webhookServer,
...but that shouldn't cause the error we see here. Going to keep looking.
There was a problem hiding this comment.
I'm worried it's related to some deprecation https://kubernetes.io/docs/reference/kubernetes-api/extend-resources/validating-webhook-configuration-v1/.
|
@zekemorton I did some more digging and found a few things:
I found a fairly good description here kubernetes-sigs/kubebuilder#400 (comment) and I have some gut feelings, below! This would add quite a bit of complexity to set up, especially for local development, and I’m not sure the benefits of “a different way of validation” are worth it. E.g, to give a new developer user to the operator all these custom steps to manually create certs and possibly a server - is that worth any benefit? Is it worth needing to handle another set of APIs that can change under us and add bugs? I think the main goal was pretty error messages in the console, but most of these workflows (at least in the next year) will be run by us, and we can access logs directly. We also get a fair amount of validation out of the box. For example, we get validation of our CRD with kubebuilder - in my recent PR I forgot to make the post start exec optional and it yelled at me when I forgot it. And since we have a lot of work to do with respect to the core operator and workflows, I want to suggest that we hold off on this until it’s either absolutely necessary or we are finishing up a production product (and it would be a nice to have). If you are excited about working on it by all means keep debugging, but my spidey senses are thinking soon when we have lots of workflows to figure out it won’t be the best use of our time. Let me know what you think! |
|
@vsoch I agree. This isn't the most time sensitive feature, and if it is add more headache than its worth, I think we should just put it in the back log until it's necessary. |
|
@zekemorton are you interested in trying this out again in the new year? If not (with your permission) I'd like to give a shot, and at least get some insight to why stuffs weren't working. No rush to get back to me on this - it's more just curiously. Happy Holidays! |
|
Thanks for following up on this @vsoch! I'd be happy to take a look at it again at some point, but feel free to give it a shot if you find the time! |
|
okay sounds good! I just did a big amount of work in #61 since we need that for some experiments soon, and I should have time after that (soon). |
Opening a PR from @zekemorton so I can add little comments as I am testing things out!