MLE-30962 : fix Webhook and add E2E tests#175
Draft
rwinieski wants to merge 2 commits into
Draft
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Adds admission webhook support (self-signed or cert-manager TLS) to the MarkLogic Operator Helm chart, wires a namespace-scope validating webhook into the manager, and extends CI + e2e-helm tests to validate webhook behavior (including cert-manager CA injection).
Changes:
- Introduces webhook TLS/resource templates (Service, Secret/Certificate, ValidatingWebhookConfiguration) plus Helm render-time validation.
- Adds manager-side namespace watch-scope validating admission handlers and corresponding unit/E2E tests.
- Extends Jenkins pipeline to run Helm namespace-scoped E2E tests in self-signed and optional cert-manager variants, with a run-only mode.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| test/e2e-helm/main_test.go | Installs cert-manager for cert-manager-mode tests; passes Helm values for webhook cert provider; teardown logic updates. |
| test/e2e-helm/10_webhook_namespace_validation_test.go | New E2E tests validating admission denial/allow behavior and cert-manager CA injection. |
| Jenkinsfile | Adds parameters and logic to run Helm namespace-scoped E2E tests (self-signed and cert-manager), including run-only mode. |
| internal/webhook/namespace_scope_validation.go | New validating admission handlers enforcing WATCH_NAMESPACE scope. |
| internal/webhook/namespace_scope_validation_test.go | Unit tests for watch-namespace parsing and admission decisions. |
| hack/helmify-post-process.sh | Post-helmify generation of webhook templates + values and deployment injections. |
| docs/WEBHOOK_VALIDATION_CERTS_DESIGN.md | Design doc describing webhook validation + certificate provider modes. |
| cmd/main.go | Adds webhook server options/flag and registers namespace scope validation webhook. |
| charts/marklogic-operator-kubernetes/values.yaml | Adds webhook configuration values (enablement, provider, cert settings). |
| charts/marklogic-operator-kubernetes/templates/webhook-validation.yaml | Adds webhook-specific Helm validation template (currently duplicates validation.yaml logic). |
| charts/marklogic-operator-kubernetes/templates/webhook-service.yaml | New Service exposing webhook port for admission calls. |
| charts/marklogic-operator-kubernetes/templates/webhook-certs.yaml | New self-signed Secret generation and cert-manager Certificate + ValidatingWebhookConfiguration resources. |
| charts/marklogic-operator-kubernetes/templates/validation.yaml | Adds webhook-related render-time validation checks. |
| charts/marklogic-operator-kubernetes/templates/manager-rbac.yaml | RBAC refactor for PVC/events verbs and duplication cleanup. |
| charts/marklogic-operator-kubernetes/templates/deployment.yaml | Mounts webhook cert Secret + enables webhook port + env var for namespace validation. |
Comments suppressed due to low confidence (1)
test/e2e-helm/main_test.go:347
- In cert-manager mode, uninstalling cert-manager before uninstalling the Helm release can cause
helm uninstallto fail (e.g., if the cert-manager CRDs are removed first, Helm may be unable to delete theCertificateresource), potentially leaving the cluster-scopedValidatingWebhookConfigurationbehind and impacting subsequent tests. Uninstall the Helm release first, then uninstall cert-manager.
testEnv.Finish(
func(ctx context.Context, cfg *envconf.Config) (context.Context, error) {
if webhookCerts == "certmanager" {
log.Printf("Uninstalling cert-manager...")
utils.UninstallCertManager()
}
return ctx, nil
},
// Uninstall the Helm release.
func(ctx context.Context, cfg *envconf.Config) (context.Context, error) {
log.Printf("Uninstalling Helm release %s", helmRelease)
// #nosec G204 — helmRelease and helmNS are package constants
cmd := exec.Command("helm", "uninstall", helmRelease, "--namespace", helmNS, "--ignore-not-found")
cmd.Stdout = os.Stdout
cmd.Stderr = os.Stderr
if err := cmd.Run(); err != nil {
log.Printf("Warning: helm uninstall failed: %v", err)
}
return ctx, nil
},
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
90
to
+107
| @@ -93,3 +101,7 @@ spec: | |||
| tolerations: {{- toYaml .Values.controllerManager.tolerations | nindent 8 }} | |||
| topologySpreadConstraints: {{- toYaml .Values.controllerManager.topologySpreadConstraints | |||
| | nindent 8 }} | |||
| volumes: | |||
| - name: webhook-server-cert | |||
| secret: | |||
| secretName: {{ .Values.webhook.certs.secretName }} | |||
Comment on lines
+328
to
+332
| func(ctx context.Context, cfg *envconf.Config) (context.Context, error) { | ||
| if webhookCerts == "certmanager" { | ||
| log.Printf("Uninstalling cert-manager...") | ||
| utils.UninstallCertManager() | ||
| } |
| stage('Run-Helm-NS-e2e-Tests') { | ||
| when { | ||
| expression { return params.VERIFY_HELM_NAMESPACE_SCOPED != false && params.E2E_SCOPE == 'cluster' } | ||
| expression { return params.RUN_ONLY_HELM_NS_E2E != false || ((params.VERIFY_HELM_NAMESPACE_SCOPED != false || params.VERIFY_HELM_NAMESPACE_SCOPED_CERT_MANAGER != false) && params.E2E_SCOPE == 'cluster') } |
Comment on lines
+15
to
+19
| {{- $ca := genCA (printf "%s-webhook-ca" (include "marklogic-operator-kubernetes.fullname" .)) $validity }} | ||
| {{- $cert := genSignedCert $serviceName nil (list (printf "%s.%s" $serviceName $namespace) (printf "%s.%s.svc" $serviceName $namespace) (printf "%s.%s.svc.cluster.local" $serviceName $namespace)) $validity $ca }} | ||
| {{- $tlsCrt = b64enc $cert.Cert }} | ||
| {{- $tlsKey = b64enc $cert.Key }} | ||
| {{- $caCrt = b64enc $ca.Cert }} |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request introduces support for configuring the MarkLogic Operator's admission webhook with either self-signed or cert-manager-provided TLS certificates, and enhances the Helm chart and CI pipeline to support flexible, validated webhook deployment. It also improves the Jenkins pipeline to allow targeted Helm namespace-scoped E2E test runs and adds robust validation for invalid configuration combinations. Several RBAC permissions are adjusted for clarity and correctness.
Helm chart and webhook enhancements:
webhook-certs.yaml,webhook-service.yaml) and values to control certificate provider, secret name, and related settings. The deployment now mounts the webhook cert secret and exposes the webhook port. [1] [2] [3] [4] [5] [6] [7]Jenkins pipeline improvements:
RBAC and permissions:
persistentvolumeclaimsandeventsto clarify verbs and avoid duplication, aligning with best practices. [1] [2] [3] [4] [5] [6]These changes make the webhook setup more robust, flexible, and CI-friendly, and ensure that invalid configurations are caught early.