Allow configuration of an external webhook & associated certs #53
Conversation
Catalin-Stratulat-Ericsson
requested review from
liamfallon,
efiacor and
kushnaidu
and removed request for
kushnaidu
Catalin-Stratulat-Ericsson
requested review from
henderiw and
kushnaidu
and removed request for
s3wong and
tliron
Catalin-Stratulat-Ericsson
assigned Catalin-Stratulat-Ericsson, liamfallon and efiacor and unassigned kushnaidu
|
/assign @tliron |
|
|
||
| cert, err := tls.LoadX509KeyPair(certFile, keyFile) | ||
| // load the certificate from the secret and update when secret cert data changes e.g. | ||
| func loadCertificate(certPath, keyPath string) (tls.Certificate, error) { |
There was a problem hiding this comment.
Might be a good idea to extract out the cert/file handling stuff out to it's own go mod/file and add some test around it.
Also, the reloading is non trivial and could do with some test also.
There was a problem hiding this comment.
- yes i agree i will amend the previous comments first then begin drafting some test cases for this & separation to a new file.
There was a problem hiding this comment.
I also suggest to add test to test
- if the interaction with the cert manager works as expected
- and that the webhook still works properly with the cert manager-provided certificates
There was a problem hiding this comment.
i think E2E tests such as what you mentioned need to be added as a separate PR after the introduction of this one.
I have created a separate issue noting this here https://github.com/nephio-project/nephio/issues/742.
let me know if i have captured your request in that issue correctly and if you are happy with having this as a separate PR?
pkg/apiserver/webhooks.go
Outdated
| if err != nil { | ||
| return err | ||
| } | ||
| if err := createValidatingWebhook(ctx, webhookNs, caBytes); err != nil { |
There was a problem hiding this comment.
I think createValidatingWebhook() should still be called and the ValidatingWebhookConfiguration resource should still be created, even if you are using cert-manager for serving the webhook. The webhook should still be registered, shouldn't it?
There was a problem hiding this comment.
Have you tried running the end-to-end tests with the CERT_MAN_WEBHOOKS envvar set?
There was a problem hiding this comment.
- as for the last point im not too sure, the validating web-hook in the "USE_CERT_MAN_FOR_WEBHOOK" operation mode will be externally defined and applied in a yaml file with the rest of the package resources. this validating webhook will require an annotation to inject the ca bundle from the cert manager certificate. without that annotation the ca bundle not be updated and the certificate will eventually expire. I have a dummy "Cert-manager porch pkg" here "https://github.com/Catalin-Stratulat-Ericsson/porch-tls" which can help a little in the explanation. as this will be a "separate" porch pkg not the default as far as i understand
pkg/apiserver/webhooks.go
Outdated
| if err != nil { | ||
| return err | ||
| } | ||
| if err := createValidatingWebhook(ctx, webhookNs, caBytes); err != nil { |
There was a problem hiding this comment.
Have you tried running the end-to-end tests with the CERT_MAN_WEBHOOKS envvar set?
pkg/apiserver/webhooks.go
Outdated
| if useCertManWebhook { | ||
| _, err := loadCertificate(certFile, keyFile) | ||
| if err != nil { | ||
| klog.Errorf("failed to load certificate: %v", err) | ||
| } | ||
| // Start watching the certificate files for changes | ||
| // watch for changes in directory where secret is mounted | ||
| go watchCertificates(certStorageDir, certFile, keyFile) | ||
|
|
||
| klog.Infoln("Starting webhook server") | ||
| http.HandleFunc(serverEndpoint, validateDeletion) | ||
| server := http.Server{ | ||
| Addr: fmt.Sprintf(":%d", webhookServicePort), | ||
| TLSConfig: &tls.Config{ | ||
| GetCertificate: getCertificate, | ||
| }, | ||
| } | ||
| go func() { | ||
| err = server.ListenAndServeTLS("", "") | ||
| if err != nil { | ||
| klog.Errorf("could not start server: %v", err) | ||
| } | ||
| }() | ||
| return err | ||
|
|
||
| } else { | ||
| cert, err := tls.LoadX509KeyPair(certFile, keyFile) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| klog.Infoln("Starting webhook server") | ||
| http.HandleFunc(serverEndpoint, validateDeletion) | ||
| server := http.Server{ | ||
| Addr: fmt.Sprintf(":%d", webhookServicePort), | ||
| TLSConfig: &tls.Config{ | ||
| Certificates: []tls.Certificate{cert}, | ||
| }, | ||
| } | ||
| go func() { | ||
| err = server.ListenAndServeTLS("", "") | ||
| if err != nil { | ||
| klog.Errorf("could not start server: %v", err) | ||
| } | ||
| }() | ||
| return err | ||
| } | ||
| } |
There was a problem hiding this comment.
| if useCertManWebhook { | |
| _, err := loadCertificate(certFile, keyFile) | |
| if err != nil { | |
| klog.Errorf("failed to load certificate: %v", err) | |
| } | |
| // Start watching the certificate files for changes | |
| // watch for changes in directory where secret is mounted | |
| go watchCertificates(certStorageDir, certFile, keyFile) | |
| klog.Infoln("Starting webhook server") | |
| http.HandleFunc(serverEndpoint, validateDeletion) | |
| server := http.Server{ | |
| Addr: fmt.Sprintf(":%d", webhookServicePort), | |
| TLSConfig: &tls.Config{ | |
| GetCertificate: getCertificate, | |
| }, | |
| } | |
| go func() { | |
| err = server.ListenAndServeTLS("", "") | |
| if err != nil { | |
| klog.Errorf("could not start server: %v", err) | |
| } | |
| }() | |
| return err | |
| } else { | |
| cert, err := tls.LoadX509KeyPair(certFile, keyFile) | |
| if err != nil { | |
| return err | |
| } | |
| klog.Infoln("Starting webhook server") | |
| http.HandleFunc(serverEndpoint, validateDeletion) | |
| server := http.Server{ | |
| Addr: fmt.Sprintf(":%d", webhookServicePort), | |
| TLSConfig: &tls.Config{ | |
| Certificates: []tls.Certificate{cert}, | |
| }, | |
| } | |
| go func() { | |
| err = server.ListenAndServeTLS("", "") | |
| if err != nil { | |
| klog.Errorf("could not start server: %v", err) | |
| } | |
| }() | |
| return err | |
| } | |
| } | |
| var tlsConfig tls.Config | |
| if useCertManWebhook { | |
| // load the cert for the first time | |
| _, err := loadCertificate(certFile, keyFile) | |
| if err != nil { | |
| klog.Errorf("failed to load certificate: %v", err) | |
| } | |
| // Start watching the certificate files for changes | |
| // watch for changes in directory where secret is mounted | |
| go watchCertificates(certStorageDir, certFile, keyFile) | |
| tlsConfig.GetCertificate = getCertificate | |
| } else { | |
| cert, err := tls.LoadX509KeyPair(certFile, keyFile) | |
| if err != nil { | |
| return err | |
| } | |
| tlsConfig.Certificates = []tls.Certificate{cert} | |
| } | |
| var err error | |
| klog.Infoln("Starting webhook server") | |
| http.HandleFunc(serverEndpoint, validateDeletion) | |
| server := http.Server{ | |
| Addr: fmt.Sprintf(":%d", webhookServicePort), | |
| TLSConfig: &tlsConfig, | |
| } | |
| go func() { | |
| err = server.ListenAndServeTLS("", "") | |
| if err != nil { | |
| klog.Errorf("could not start server: %v", err) | |
| } | |
| }() | |
| return err |
This comment was marked as outdated.
This comment was marked as outdated.
|
@Catalin-Stratulat-Ericsson Have you tried removing the offending commit(s) by force-pushing into the branch? It would be nice to save the discussion and reviews here |
efiacor
force-pushed
the
porch-cert-manager-webhook
branch
from
9242885 to
98efbce
Compare
|
/retest |
|
/retest |
|
@Catalin-Stratulat-Ericsson My #57 caused a merge conflict with this PR. I am absolutely willing to help to rebase you code to my changes regarding how setup the validation webhook. |
Catalin-Stratulat-Ericsson
force-pushed
the
porch-cert-manager-webhook
branch
from
8fb954d to
4e43ca3
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Catalin-Stratulat-Ericsson, efiacor 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 |
This change is related to nephio-project/nephio#554
This change introduces a new environment variable, when present in the porch server and set to true porch will no longer create its own tls certificates for webhooks from the code but expects those to be created by a cert manager issuer and certificate yaml configuration along with the validationwebhookconfiguration.
These will all be provided by a separate porch pkg.