Webhook enhancements #543
Webhook enhancements #543
Conversation
pkg/webhook/certs.go
Outdated
| } | ||
|
|
||
| func (c *certProvider) Start() { | ||
| stopChannel := make(chan interface{}) |
There was a problem hiding this comment.
Looks like you can initialize this field in NewCertProvider, and then use case <-c.stopChannel below. Ditto for ticker.
pkg/webhook/webhook.go
Outdated
| if userConfig.webhookNamespaceSelector == "" { | ||
| glog.Fatal("webhook-namespace-selector must be set when webhook-fail-on-error is true.") | ||
| } else { | ||
| kv := strings.SplitN(userConfig.webhookNamespaceSelector, "=", 2) |
There was a problem hiding this comment.
I think we should also support specification of multiple selector entries, e.g., key1=val1,key2=val2.
| func (c *certProvider) tlsConfig() *tls.Config { | ||
| return &tls.Config{ | ||
| GetCertificate: func(ch *tls.ClientHelloInfo) (*tls.Certificate, error) { | ||
| return c.currentCert, nil |
There was a problem hiding this comment.
Do we need to use a mutex to guard currentCert, which may be read in tlsConfig and written in updateCert?
There was a problem hiding this comment.
It's probably a good idea, since this behavior is technically undefined (although AFAICT it's safe on all modern architectures).
pkg/webhook/webhook.go
Outdated
| } | ||
| if userConfig.webhookFailOnError { | ||
| if userConfig.webhookNamespaceSelector == "" { | ||
| glog.Fatal("webhook-namespace-selector must be set when webhook-fail-on-error is true.") |
There was a problem hiding this comment.
Remind me why is this a requirement?
There was a problem hiding this comment.
Because the operator going down would cause a cluster-wide outage. And if the operator pod were rescheduled, it would required manual intervention (because it would have a circular dependency on itself to start up).
There was a problem hiding this comment.
Got it. Instead of using glog.Fatal, please return an error instead. The caller of New will do glog.Fatal on the return error anyway.
|
@khogeland can you address the remaining comments? It looks good otherwise. |
|
@liyinan926 Addressed your above comments |
| @@ -254,61 +348,47 @@ func (wh *WebHook) selfDeregistration(webhookConfigName string) error { | |||
| func mutatePods( | |||
| review *admissionv1beta1.AdmissionReview, | |||
| lister crdlisters.SparkApplicationLister, | |||
| sparkJobNs string) *admissionv1beta1.AdmissionResponse { | |||
| sparkJobNs string) (*admissionv1beta1.AdmissionResponse, error) { | |||
| if review.Request.Resource != podResource { | |||
There was a problem hiding this comment.
Looks like we don't need this check here any more as the same check is added above.
| w.WriteHeader(code) | ||
| _, err = w.Write(resp) | ||
| if err != nil { | ||
| glog.Errorf("Failed to write response body: %v", err) |
There was a problem hiding this comment.
Nit: lower case for error messages.
| glog.Error(err) | ||
| reviewResponse = toAdmissionResponse(err) | ||
| internalError(w, err) | ||
| return | ||
| } else { |
There was a problem hiding this comment.
Nit: this else is no longer needed with the return above.
| if userConfig.webhookFailOnError { | ||
| if userConfig.webhookNamespaceSelector == "" { | ||
| return nil, fmt.Errorf("webhook-namespace-selector must be set when webhook-fail-on-error is true.") | ||
| } else { |
There was a problem hiding this comment.
Nit: this else is no longer needed with the return above.
* Cert configuration and reloading * Add support for strict webhook error handling * Improve webhook error handling * Don't deregister the webhook when failure policy is strict * standard error message capitalization * have the webhook parse its own configuration from flags * clean up cert provider code * Add explanation for skipping deregistration * Tests and fixes
Bundled to avoid merge conflicts
Subsumes #515 #516 #517: