Skip to content
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

Merged
merged 6 commits into from
Jun 7, 2024

Conversation

Catalin-Stratulat-Ericsson
Copy link
Contributor

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.

  1. Since the ValidationWebhookConfiguration is now a yaml file config which will be in its own separate porch package points 1 and 3 in issue 554 are handled.
  2. if were running with cert manager certificates the certificate gets mounted as a secret to the porch-server pod and when updated by cert manager it is detected and the new one is used by the porch server. point 2 in issue 554.
  3. the cert manager certificate and issuer resources are configurable in their own yaml file.

@liamfallon
Copy link
Member

/assign @tliron

pkg/apiserver/apiserver.go Outdated Show resolved Hide resolved
pkg/apiserver/apiserver.go Outdated Show resolved Hide resolved
pkg/apiserver/apiserver.go Outdated Show resolved Hide resolved
pkg/apiserver/webhooks.go Outdated Show resolved Hide resolved
pkg/apiserver/webhooks.go Show resolved Hide resolved
pkg/apiserver/webhooks.go Outdated Show resolved Hide resolved
pkg/apiserver/webhooks.go Outdated Show resolved Hide resolved
pkg/apiserver/webhooks.go Outdated Show resolved Hide resolved

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) {
Copy link
Collaborator

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.

Copy link
Contributor Author

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.

Copy link
Collaborator

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

Copy link
Contributor Author

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/apiserver.go Outdated Show resolved Hide resolved
if err != nil {
return err
}
if err := createValidatingWebhook(ctx, webhookNs, caBytes); err != nil {
Copy link
Collaborator

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?

Copy link
Collaborator

@kispaljr kispaljr May 28, 2024

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?

Copy link
Contributor Author

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

if err != nil {
return err
}
if err := createValidatingWebhook(ctx, webhookNs, caBytes); err != nil {
Copy link
Collaborator

@kispaljr kispaljr May 28, 2024

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?

Comment on lines 319 to 382
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
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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

Copy link

linux-foundation-easycla bot commented May 28, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

@Catalin-Stratulat-Ericsson

This comment was marked as outdated.

@kispaljr
Copy link
Collaborator

@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

@liamfallon
Copy link
Member

/retest

pkg/apiserver/webhooks.go Outdated Show resolved Hide resolved
pkg/apiserver/webhooks.go Outdated Show resolved Hide resolved
pkg/apiserver/webhooks.go Outdated Show resolved Hide resolved
pkg/apiserver/webhooks.go Outdated Show resolved Hide resolved
@liamfallon
Copy link
Member

/retest

@kispaljr
Copy link
Collaborator

kispaljr commented Jun 5, 2024

@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.

Copy link
Member

@liamfallon liamfallon left a 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?

Copy link
Collaborator

@efiacor efiacor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/approve
/lgtm

@nephio-prow nephio-prow bot added the lgtm label Jun 7, 2024
Copy link
Contributor

nephio-prow bot commented Jun 7, 2024

[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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants