DM-55959: add CertManager support to Helm chart - #1096
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The new cert-manager templates should be aligned with chart conventions (labels and czar enablement gating) and avoid a createIssuer/issuerRef.kind mismatch that can produce an invalid issuer reference.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds optional cert-manager integration to the Qserv Helm chart so deployments can have cert-manager manage the TLS secret used by the czar HTTP frontend / SSL proxy.
Changes:
- Introduces
certManagerconfiguration invalues.yaml(enablement, issuer reference, dnsNames). - Adds a templated
Issuer(optional) andCertificate(createsczar-certsecret when enabled).
File summaries
| File | Description |
|---|---|
deploy/helm/values.yaml |
Adds certManager values used to enable/parameterize cert-manager resources. |
deploy/helm/templates/czar-issuer.yaml |
New template to optionally create a self-signed namespaced Issuer. |
deploy/helm/templates/czar-certificate.yaml |
New template to create a Certificate targeting the czar-cert secret. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
3d46b45 to
82b8f6f
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Enabling cert-manager with the provided defaults can render an invalid Certificate (empty dnsNames) and can render an invalid issuerRef when bring-your-own issuer is selected without a required name, so the chart should fail-fast with clear validation.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Lite
82b8f6f to
6d78e34
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The new Helm templates have rendering/formatting errors (not on a map and whitespace-trimming that can break YAML) that can cause helm template/install to fail when enabled.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Lite
6d78e34 to
8eba38f
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Enabling cert-manager with the current default dnsNames: [] can render an invalid Certificate, leading to a broken Helm install/upgrade unless the chart validates this configuration.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Lite
|
Go home copilot you're drunk |
Adds chart support for optional per-deployment use of CertManager to manage frontend SSL cert