feat(helm): add TLS/HTTPS support with cert-manager integration #3077
Conversation
- Add TLS configuration to ingress templates (client & server) - Add tls.enabled and tls.secretName to values.yaml - Include comprehensive cert-manager documentation in INSTALLATION.md - Add example annotations for Let's Encrypt cluster issuer - Support both values.yaml and --set flag configuration methods - Backward compatible (TLS disabled by default) Enables automatic HTTPS certificate provisioning when cert-manager is installed in the cluster with appropriate ClusterIssuer configured.
web-engineer
requested review from
Br0wnHammer,
Owaiseimdad,
ajhollid and
karenvicent
as code owners
|
Note
|
| Cohort / File(s) | Summary |
|---|---|
TLS/HTTPS configuration & docscharts/helm/checkmate/INSTALLATION.md, charts/helm/checkmate/values.yaml |
Adds an INSTALLATION.md section explaining cert-manager prerequisites, TLS configuration (client/server ingress tls settings and example annotations), Helm --set alternative, and verification steps. Adds client.ingress.tls and server.ingress.tls blocks with enabled: false and default secretNames in values.yaml. |
Client Ingress template changecharts/helm/checkmate/templates/client-ingress.yaml |
Wraps a TLS stanza in a conditional (.Values.client.ingress.tls.enabled), setting hosts from .Values.client.ingress.host and secretName from .Values.client.ingress.tls.secretName. Existing ingress rules remain unchanged. |
Server Ingress template changecharts/helm/checkmate/templates/server-ingress.yaml |
Adds a conditional TLS stanza gated by .Values.server.ingress.tls.enabled, using .Values.server.ingress.host and .Values.server.ingress.tls.secretName. Leaves rules and paths intact. |
Estimated code review effort
🎯 2 (Simple) | ⏱️ ~10 minutes
- Review the Helm conditional block syntax in
client-ingress.yamlandserver-ingress.yaml. - Verify secretName defaults in
values.yamlmatch template references. - Check INSTALLATION.md examples/commands and any commented cert-manager annotations for correctness.
Poem
🐰 Through tunnels of YAML I bound and hop,
Secrets tucked safe in a TLS crop,
cert-manager whispers the keys in the night,
Ingresses gleam with encrypted light,
Helm charts snug — hop secure, don't stop! 🥕🔐
Pre-merge checks and finishing touches
❌ Failed checks (2 warnings)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Description check | The PR description includes a clear summary of changes and purpose, but the required checklist items are not completed (all boxes unchecked), violating submission requirements. | Complete the checklist by checking off all required items before submission, particularly local deployment and self-review confirmations. | |
| Linked Issues check | The PR changes (TLS/HTTPS support for Helm charts) are unrelated to the linked issue #123 (forgot password backend endpoints). | Link the correct issue related to TLS/HTTPS cert-manager support, or create a new issue if this PR addresses a different objective than #123. |
✅ Passed checks (3 passed)
| Check name | Status | Explanation |
|---|---|---|
| Title check | ✅ Passed | The pull request title clearly and concisely summarizes the main change: adding TLS/HTTPS support with cert-manager integration to Helm charts. |
| Out of Scope Changes check | ✅ Passed | All changes are focused on TLS/HTTPS configuration for Helm charts and are within scope of the stated PR objectives; no extraneous changes detected. |
| Docstring Coverage | ✅ Passed | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. |
✨ Finishing touches
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Post copyable unit tests in a comment
📜 Recent review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
charts/helm/checkmate/values.yaml(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- charts/helm/checkmate/values.yaml
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands and usage tips.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
charts/helm/checkmate/INSTALLATION.md (1)
96-112: Verification steps are practical but could mention pre-flight check for cert-manager.Consider adding a note to verify that cert-manager is running in the cluster before attempting TLS configuration (e.g.,
kubectl get deployment -n cert-manager), which would help users catch configuration issues earlier.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
charts/helm/checkmate/INSTALLATION.md(2 hunks)charts/helm/checkmate/templates/client-ingress.yaml(1 hunks)charts/helm/checkmate/templates/server-ingress.yaml(1 hunks)charts/helm/checkmate/values.yaml(2 hunks)
🔇 Additional comments (9)
charts/helm/checkmate/INSTALLATION.md (2)
46-112: Well-structured TLS documentation with clear prerequisites and examples.The new TLS/HTTPS section provides comprehensive guidance, including prerequisites, configuration examples (both YAML and --set approaches), and verification steps. The documentation appropriately positions TLS as optional and conditional on cert-manager installation.
82-94: Correct Helm --set syntax for cert-manager annotations.The annotation key escaping in the --set examples is correct for passing dotted keys via the Helm CLI.
charts/helm/checkmate/templates/client-ingress.yaml (2)
14-19: TLS block implementation is correct and properly gated.The Kubernetes Ingress TLS specification is correctly formatted with appropriate conditional gating. Hosts and secretName are properly extracted from values, and the YAML structure follows Kubernetes conventions.
1-31: Template structure preserved; TLS is cleanly integrated.The existing Ingress structure remains intact, with the TLS block cleanly inserted before the rules section. The dynamic annotation handling is preserved, allowing users to configure cert-manager annotations independently of TLS enablement.
charts/helm/checkmate/values.yaml (2)
10-16: Client TLS config is well-structured with sensible defaults.The
tls.enabled: falsedefault preserves backward compatibility, and the secretName convention is clear and consistent. The commented example annotation and explanatory comment are helpful for users enabling cert-manager integration.
27-33: Server TLS config mirrors client pattern; excellent consistency.The server TLS configuration follows the same structure as the client configuration, with distinct secretNames for each ingress. This consistency improves maintainability and user experience.
charts/helm/checkmate/templates/server-ingress.yaml (3)
21-26: TLS block mirrors client template pattern; implementation is correct.The server ingress TLS configuration correctly mirrors the client template, ensuring consistency across both ingress resources. The conditional gating, host reference, and secretName extraction are all correctly implemented.
1-38: Server ingress structure is sound; TLS integrated cleanly without disrupting rules.The new TLS block is properly positioned in the spec before the rules section. Pre-existing commented CORS annotations remain unchanged, and the /api/v1 path routing is unaffected by the TLS addition.
1-38:⚠️ PR description mismatch: References Fixes #123 (forgot-password) but contains TLS/cert-manager changes.The code changes are focused on TLS/HTTPS support with cert-manager integration. The PR description mentions "Fixes #123" (a forgot-password feature), which appears to be a copy-paste error or incorrect issue reference. Verify that issue #123 is indeed the TLS feature, or update the PR description and references to match the actual work.
Additionally, the PR checklist (testing, i18n, formatting, etc.) is noted as incomplete per the PR objectives. Ensure these verification steps are completed before merging.
|
This PR adds optional TLS sections to both ingress templates and introduces client.ingress.tls and server.ingress.tls fields in values.yaml. It’s backward compatible (off by default). Installation docs include cert-manager/Let’s Encrypt steps and validation commands. Happy to adjust defaults or add Helm schema if desired. |
![llamapreview[bot]](https://avatars.githubusercontent.com/in/1023541?s=60&v=4){kind=small}
There was a problem hiding this comment.
AI Code Review by LlamaPReview
🎯 TL;DR & Recommendation
Recommendation: Request Changes
This PR adds TLS support but introduces a critical architectural mismatch where the protocol remains HTTP when TLS is enabled, causing mixed-content issues and broken API calls. Documentation also omits the necessary protocol updates, leading to deployment failures.
📄 Documentation Diagram
This diagram documents the new TLS-enabled ingress workflow for Checkmate deployments.
sequenceDiagram
participant U as User
participant I as Ingress
participant S as Service
U->>I: HTTPS request
note over I: PR #35;3077: TLS configuration added
I->>S: HTTP request
S-->>I: HTTP response
I-->>U: HTTPS response
🌟 Strengths
- Backward compatible with TLS disabled by default.
- Comprehensive cert-manager integration documentation.
| Priority | File | Category | Impact Summary | Anchors |
|---|---|---|---|---|
| P1 | charts/helm/checkmate/values.yaml | Architecture | Architectural mismatch causes mixed-content issues | client-deployment.yaml, server-deployment.yaml |
| P1 | charts/helm/checkmate/INSTALLATION.md | Documentation | Documentation omits critical protocol update | - |
| P2 | charts/helm/.../client-ingress.yaml | Maintainability | Lacks validation for required annotations | - |
| P2 | charts/helm/checkmate/values.yaml | Maintainability | Hardcoded secret names risk conflicts | - |
| P2 | charts/helm/checkmate/INSTALLATION.md | Documentation | Incomplete --set flag examples mislead users | - |
🔍 Notable Themes
- Protocol Configuration Mismatch: Enabling TLS requires updating the protocol from http to https in values.yaml to avoid broken internal URLs and mixed-content errors.
- Documentation Gaps: Critical steps for protocol updates are missing in both values.yaml examples and --set flag instructions, increasing deployment failure risk.
📈 Risk Diagram
This diagram illustrates the risk of protocol mismatch when TLS is enabled without updating the protocol setting.
sequenceDiagram
participant U as User
participant I as Ingress
participant S as Service
U->>I: HTTPS request
I->>S: HTTP request (protocol: http)
note over I,S: R1(P1): Protocol mismatch causes mixed-content issues
S->>S: Generate URL with http
S-->>I: Response with http URLs
I-->>U: HTTPS response with mixed-content
💡 Have feedback? We'd love to hear it in our GitHub Discussions.
✨ This review was generated by LlamaPReview Advanced, which is free for all open-source projects. Learn more.
charts/helm/checkmate/values.yaml
Outdated
| # cert-manager.io/cluster-issuer: "letsencrypt-prod" | ||
| tls: | ||
| enabled: false | ||
| secretName: checkmate-client-tls |
There was a problem hiding this comment.
P1 | Confidence: High
The TLS configuration introduces a critical architectural mismatch. The related_context shows that both client and server deployments construct URLs using the protocol from values.yaml ({{ .Values.client.protocol }}://{{ .Values.client.ingress.host }} and {{ .Values.server.protocol }}://{{ .Values.server.ingress.host }}). When TLS is enabled on the ingress but the protocol remains "http", the application will generate incorrect internal URLs, causing mixed-content issues and broken API calls. This is a breaking change for existing deployments that enable TLS without updating the protocol values.
| secretName: checkmate-client-tls | |
| client: | |
| protocol: http # Change to https when TLS is enabled | |
| ingress: | |
| tls: | |
| enabled: false | |
| secretName: checkmate-client-tls | |
| server: | |
| protocol: http # Change to https when TLS is enabled | |
| ingress: | |
| tls: | |
| enabled: false | |
| secretName: checkmate-server-tls |
Evidence: path:charts/helm/checkmate/templates/client-deployment.yaml, path:charts/helm/checkmate/templates/server-deployment.yaml
| - cert-manager installed in your cluster | ||
| - A ClusterIssuer or Issuer configured (e.g., `letsencrypt-prod`) | ||
|
|
||
| ### Configuration |
There was a problem hiding this comment.
P1 | Confidence: High
The documentation fails to mention the critical requirement to update the protocol from "http" to "https" when enabling TLS. This omission will lead to deployment failures and broken application behavior, as the internal URL construction will remain HTTP while external access uses HTTPS.
Code Suggestion:
client:
protocol: https # ← MUST change from http to https
ingress:
tls:
enabled: true
secretName: checkmate-client-tls
server:
protocol: https # ← MUST change from http to https
ingress:
tls:
enabled: true
secretName: checkmate-server-tls| ### Alternative: Using --set flags | ||
There was a problem hiding this comment.
P2 | Confidence: High
The --set flag examples in documentation don't include the critical protocol update from http to https. This incomplete example will mislead users into creating broken deployments.
Code Suggestion:
helm install checkmate ./charts/helm/checkmate \
--set client.protocol=https \
--set server.protocol=https \
--set client.ingress.annotations.\"cert-manager\\.io/cluster-issuer\"=\"letsencrypt-prod\" \
--set client.ingress.tls.enabled=true \
--set client.ingress.tls.secretName=checkmate-client-tls \
--set server.ingress.annotations.\"cert-manager\\.io/cluster-issuer\"=\"letsencrypt-prod\" \
--set server.ingress.tls.enabled=true \
--set server.ingress.tls.secretName=checkmate-server-tls| {{- if .Values.client.ingress.tls.enabled }} | ||
| tls: | ||
| - hosts: | ||
| - {{ .Values.client.ingress.host }} | ||
| secretName: {{ .Values.client.ingress.tls.secretName }} | ||
| {{- end }} |
There was a problem hiding this comment.
P2 | Confidence: Medium
The TLS configuration lacks validation for required cert-manager annotations when TLS is enabled. Users might enable TLS without setting the necessary cert-manager.io/cluster-issuer annotation, leading to silent failures in certificate provisioning.
Code Suggestion:
{{- if .Values.client.ingress.tls.enabled }}
{{- if not (hasKey .Values.client.ingress.annotations "cert-manager.io/cluster-issuer") }}
{{- fail "cert-manager.io/cluster-issuer annotation is required when TLS is enabled" }}
{{- end }}
tls:
- hosts:
- {{ .Values.client.ingress.host }}
secretName: {{ .Values.client.ingress.tls.secretName }}
{{- end }}| tls: | ||
| enabled: false | ||
| secretName: checkmate-client-tls | ||
| # The secret will be automatically created by cert-manager when using the cert-manager.io/cluster-issuer annotation |
There was a problem hiding this comment.
P2 | Confidence: Medium
The hardcoded secret names (checkmate-client-tls, checkmate-server-tls) could cause conflicts in multi-tenant clusters where multiple Checkmate instances are deployed. Using a release-specific naming convention would be more robust.
| tls: | |
| enabled: false | |
| secretName: checkmate-client-tls | |
| # The secret will be automatically created by cert-manager when using the cert-manager.io/cluster-issuer annotation | |
| tls: | |
| enabled: false | |
| secretName: {{ .Release.Name }}-client-tls |
|
Updated: Commented out the |
1 participant
Enables automatic HTTPS certificate provisioning when cert-manager is installed in the cluster with appropriate ClusterIssuer configured.
(Please remove this line only before submitting your PR. Ensure that all relevant items are checked before submission.)
Describe your changes
Briefly describe the changes you made and their purpose.
Write your issue number after "Fixes "
Fixes #123
Please ensure all items are checked off before requesting a review. "Checked off" means you need to add an "x" character between brackets so they turn into checkmarks.
<div>Add</div>, use):npm run formatin server and client directories, which automatically formats your code.Summary by CodeRabbit
New Features
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.