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 for cert-manager Integrationcharts/helm/checkmate/INSTALLATION.md, charts/helm/checkmate/templates/client-ingress.yaml, charts/helm/checkmate/templates/server-ingress.yaml, charts/helm/checkmate/values.yaml |
Adds conditional TLS blocks to client and server ingress templates, guarded by tls.enabled flags with corresponding secret names. Introduces TLS configuration defaults in values.yaml with commented cert-manager annotations. Includes installation documentation section covering TLS prerequisites, configuration, and verification steps. |
Estimated code review effort
🎯 1 (Trivial) | ⏱️ ~5–10 minutes
- Verification points: Confirm conditional template syntax is correct; verify secret names match between templates and default values; check that documentation accurately reflects the configuration options.
Poem
🐰 Hop through the tunnels, now locked and secure,
With TLS certificates, you can be sure!
cert-manager weaves its cryptographic spell,
Client and server in harmony dwell,
Encryption now blooms in our Helm garden so bright! 🔐✨
Pre-merge checks and finishing touches
❌ Failed checks (3 warnings)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Description check | The PR description clearly outlines the changes and purpose, but the required checklist items are not completed (all boxes remain unchecked) and no issue number is actually filled in after 'Fixes'. | Complete the PR checklist by checking relevant items and provide the actual issue number in the 'Fixes #' field instead of the template placeholder. | |
| Linked Issues check | The linked issue #123 is for 'Feat/forgot password' implementing recovery endpoints, which is completely unrelated to the TLS/HTTPS Helm chart changes in this PR. | Link the correct issue that covers Helm TLS/cert-manager support requirements, or update the PR to align with the forgot-password backend endpoints. | |
| Out of Scope Changes check | All changes are focused on TLS/HTTPS configuration for Helm charts and documentation; however, the linked issue is about forgot-password endpoints, making the entire PR appear out-of-scope relative to the stated objective. | Either link the correct Helm/TLS-related issue or confirm this PR is addressing the wrong objective and reassign to the appropriate issue. |
✅ Passed checks (2 passed)
| Check name | Status | Explanation |
|---|---|---|
| Title check | ✅ Passed | The PR title clearly and specifically describes the main change: adding TLS/HTTPS support with cert-manager integration, which aligns with the file modifications across Helm templates and documentation. |
| 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
Tip
📝 Customizable high-level summaries are now available in beta!
You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
- Provide your own instructions using the
high_level_summary_instructionssetting. - Format the summary however you like (bullet lists, tables, multi-section layouts, contributor stats, etc.).
- Use
high_level_summary_in_walkthroughto move the summary from the description to the walkthrough section.
Example instruction:
"Divide the high-level summary into five sections:
- 📝 Description — Summarize the main change in 50–60 words, explaining what was done.
- 📓 References — List relevant issues, discussions, documentation, or related PRs.
- 📦 Dependencies & Requirements — Mention any new/updated dependencies, environment variable changes, or configuration updates.
- 📊 Contributor Summary — Include a Markdown table showing contributions:
| Contributor | Lines Added | Lines Removed | Files Changed |- ✔️ Additional Notes — Add any extra reviewer context.
Keep each section concise (under 200 words) and use bullet or numbered lists for clarity."
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later.
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.
| # 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 |
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.