-
Notifications
You must be signed in to change notification settings - Fork 18
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
Allow configuration of an external webhook & associated certs #53
Allow configuration of an external webhook & associated certs #53
Conversation
/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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you tried running the end-to-end tests with the CERT_MAN_WEBHOOKS envvar set?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
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. |
8fb954d
to
4e43ca3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you resolve the unresolved comments please?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/approve
/lgtm
[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.