Add support for built-in TLS#316
Open
Sigurs wants to merge 2 commits into
Open
Conversation
📝 WalkthroughWalkthroughBumped Meilisearch Helm chart to 0.30.0 and added built-in TLS support: new Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Service as K8s Service
participant Pod as Meilisearch Pod
participant Secret as TLS Secret
participant Prom as Prometheus (ServiceMonitor)
Client->>Service: HTTPS request
Service->>Pod: Forward to container port (HTTPS)
Pod->>Secret: Mount cert/key at tls.mountPath
Note over Pod: MEILI_SSL_CERT_PATH / MEILI_SSL_KEY_PATH env vars set
Prom->>Service: Scrape endpoint (ServiceMonitor)
alt TLS enabled
Prom->>Pod: HTTPS scrape using tlsConfig
else TLS disabled
Prom->>Pod: HTTP scrape
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@charts/meilisearch/templates/statefulset.yaml`:
- Around line 55-60: The OCSP secret mount is currently gated only on
.Values.tls.existingOcspSecret, so pods will mount tls-ocsp even when TLS is
disabled; update the template conditional to require both TLS is enabled and the
secret exists (i.e., check .Values.tls.enabled &&
.Values.tls.existingOcspSecret) where the tls-ocsp volume and the corresponding
volumeMounts are declared (the blocks referring to tls-ocsp and the secretName
using .Values.tls.existingOcspSecret), and make the same change for the other
occurrence (the second mount block around the volumeMounts section) so the OCSP
secret is only used when TLS is enabled.
- Around line 104-122: Update the Helm test hook in manifests/meilisearch.yaml
so it uses HTTPS and matches tls.enabled behavior: change the wget curl
invocation(s) (the test hook lines around the existing "wget meilisearch:7700")
to call the HTTPS URL (e.g. https://meilisearch:7700/) and handle certificate
verification consistently with the chart (either add
--no-check-certificate/--insecure or, preferably, point the client to the TLS
CA/cert mounted by the chart), and gate the change behind the same
.Values.tls.enabled conditional used in the StatefulSet so the test switches to
HTTPS only when TLS is enabled.
- Around line 49-53: When tls.enabled is true the template must fail if
tls.existingSecret is not provided; update the StatefulSet template
(statefulset.yaml) to use Helm's required function for
.Values.tls.existingSecret inside the conditional so the chart errors fast
instead of rendering an invalid secretName. For example, replace the raw
secretName reference with a required(...) call that includes a clear message
like "tls.existingSecret is required when tls.enabled is true" so Helm will
abort when the value is missing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5fc46014-c46e-439c-9b69-b4a51785df93
📒 Files selected for processing (6)
charts/meilisearch/Chart.yamlcharts/meilisearch/README.mdcharts/meilisearch/templates/serviceMonitor.yamlcharts/meilisearch/templates/statefulset.yamlcharts/meilisearch/values.yamlmanifests/meilisearch.yaml
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@charts/meilisearch/README.md`:
- Around line 137-138: Update the README entry for tls.existingSecret to remove
the hardcoded tls.crt/tls.key filenames and instead state that the secret must
contain keys matching tls.certFilename and tls.keyFilename (with defaults
tls.crt and tls.key); specifically edit the description for tls.existingSecret
and mention tls.certFilename and tls.keyFilename so users know the filenames are
configurable and the defaults are tls.crt and tls.key.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e1a8de38-aab4-49d4-97fc-2a44365a724a
📒 Files selected for processing (6)
charts/meilisearch/Chart.yamlcharts/meilisearch/README.mdcharts/meilisearch/templates/serviceMonitor.yamlcharts/meilisearch/templates/statefulset.yamlcharts/meilisearch/values.yamlmanifests/meilisearch.yaml
✅ Files skipped from review due to trivial changes (2)
- charts/meilisearch/Chart.yaml
- manifests/meilisearch.yaml
🚧 Files skipped from review as they are similar to previous changes (3)
- charts/meilisearch/templates/serviceMonitor.yaml
- charts/meilisearch/values.yaml
- charts/meilisearch/templates/statefulset.yaml
Comment on lines
+137
to
+138
| | tls.existingSecret | string | `""` | Name of existing Kubernetes TLS secret (must contain tls.crt and tls.key) | | ||
| | tls.keyFilename | string | `"tls.key"` | Key filename in the secret | |
There was a problem hiding this comment.
tls.existingSecret description conflicts with configurable filename keys
The description at Line 137 hardcodes tls.crt/tls.key, but Line 134 and Line 138 allow custom filenames. Please document this as “must contain keys matching tls.certFilename and tls.keyFilename (defaults: tls.crt, tls.key)” to avoid user misconfiguration.
Suggested doc fix
-| tls.existingSecret | string | `""` | Name of existing Kubernetes TLS secret (must contain tls.crt and tls.key) |
+| tls.existingSecret | string | `""` | Name of existing Kubernetes TLS secret (must contain keys matching tls.certFilename and tls.keyFilename; defaults: tls.crt and tls.key) |📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| | tls.existingSecret | string | `""` | Name of existing Kubernetes TLS secret (must contain tls.crt and tls.key) | | |
| | tls.keyFilename | string | `"tls.key"` | Key filename in the secret | | |
| | tls.existingSecret | string | `""` | Name of existing Kubernetes TLS secret (must contain keys matching tls.certFilename and tls.keyFilename; defaults: tls.crt and tls.key) | | |
| | tls.keyFilename | string | `"tls.key"` | Key filename in the secret | |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@charts/meilisearch/README.md` around lines 137 - 138, Update the README entry
for tls.existingSecret to remove the hardcoded tls.crt/tls.key filenames and
instead state that the secret must contain keys matching tls.certFilename and
tls.keyFilename (with defaults tls.crt and tls.key); specifically edit the
description for tls.existingSecret and mention tls.certFilename and
tls.keyFilename so users know the filenames are configurable and the defaults
are tls.crt and tls.key.
…gSecret when tls.enabled is set to true.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
charts/meilisearch/templates/statefulset.yaml (1)
58-63:⚠️ Potential issue | 🟠 MajorGate OCSP secret resources on TLS enablement as well.
Line 58 and Line 89 still mount/render OCSP resources when TLS is disabled. This can create an unnecessary secret dependency for TLS-off installs.
Suggested fix
- {{- if .Values.tls.existingOcspSecret }} + {{- if and .Values.tls.enabled .Values.tls.existingOcspSecret }} - name: tls-ocsp secret: secretName: {{ .Values.tls.existingOcspSecret }} defaultMode: 0400 {{- end }} ... - {{- if .Values.tls.existingOcspSecret }} + {{- if and .Values.tls.enabled .Values.tls.existingOcspSecret }} - name: tls-ocsp mountPath: {{ .Values.tls.mountPath }}/ocsp readOnly: true {{- end }}Also applies to: 89-93
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@charts/meilisearch/templates/statefulset.yaml` around lines 58 - 63, The OCSP secret mount is currently only gated by .Values.tls.existingOcspSecret and still renders when TLS is disabled; update both conditional blocks that reference tls-ocsp and .Values.tls.existingOcspSecret to require TLS to be enabled as well (e.g., change the condition to check both .Values.tls.enabled and .Values.tls.existingOcspSecret) so the tls-ocsp secret is only rendered/mounted when TLS is enabled; apply this change to the conditional around the tls-ocsp secret mount and the other OCSP-related block (both occurrences referencing tls-ocsp and .Values.tls.existingOcspSecret).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@charts/meilisearch/templates/statefulset.yaml`:
- Around line 58-63: The OCSP secret mount is currently only gated by
.Values.tls.existingOcspSecret and still renders when TLS is disabled; update
both conditional blocks that reference tls-ocsp and
.Values.tls.existingOcspSecret to require TLS to be enabled as well (e.g.,
change the condition to check both .Values.tls.enabled and
.Values.tls.existingOcspSecret) so the tls-ocsp secret is only rendered/mounted
when TLS is enabled; apply this change to the conditional around the tls-ocsp
secret mount and the other OCSP-related block (both occurrences referencing
tls-ocsp and .Values.tls.existingOcspSecret).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1d1823e4-76d8-4b11-8cd8-e714af485bc2
📒 Files selected for processing (3)
charts/meilisearch/templates/statefulset.yamlcharts/meilisearch/templates/tests/test-connection.yamlmanifests/meilisearch.yaml
✅ Files skipped from review due to trivial changes (1)
- manifests/meilisearch.yaml
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
1 participant
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.
Pull Request
Related issue
Fixes #315
What does this PR do?
Adds support to configure built-in TLS via the Helm chart while still leaving room for possible future implementations of certificate generation.
Simplest possible configuration - also used to test this:
Not my first time working on Helm charts, but AI was used to do quick edits around the templates and to speed up my work.
Looks like
helm-docshadn't been ran in a while.PR checklist
Please check if your PR fulfills the following requirements:
Thank you so much for contributing to Meilisearch!
Summary by CodeRabbit
New Features
Chores