feat(qdrant): add TLS support via cert-manager#2685
Conversation
- Extend QdrantSpec with tls.enabled tri-state bool and expose it via values.yaml and JSON schema - Update cozyrds integration to pass TLS config to the operator - Add test: Makefile target and TLS parameter documentation to README Signed-off-by: Arsolitt <arsolitt@gmail.com>
- Add certmanager.yaml to provision cert-manager Certificate and Issuer for p2p and client TLS - Extend qdrant.yaml to mount TLS secrets and configure Qdrant with TLS settings when enabled - TLS activation follows a tri-state: explicit true/false or inherit from cluster external setting Signed-off-by: Arsolitt <arsolitt@gmail.com>
- Add helm-unittest suite for cert-manager resources covering all TLS mode combinations - Add helm-unittest suite for Qdrant StatefulSet verifying TLS volume mounts and p2p behavior - Cover explicit enabled/disabled and external-inherited cases; document p2p standalone behavior Signed-off-by: Arsolitt <arsolitt@gmail.com>
📝 WalkthroughWalkthroughThis PR introduces explicit TLS configuration for Qdrant via a new ChangesQdrant TLS Configuration and Certificate Management
🎯 3 (Moderate) | ⏱️ ~20 minutes
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
github-actions
Bot
added
size/XL
Signed-off-by: Arsolitt <arsolitt@gmail.com>
Arsolitt
requested review from
IvanHunters,
androndo,
kvaps,
lexfrei,
lllamnyp,
myasnikovdaniil and
sircthulhu
as code owners
dosubot
Bot
added
the
lifecycle/active
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the Qdrant managed service by integrating cert-manager for automated TLS certificate management. It provides a robust configuration mechanism that defaults to secure-by-default behavior based on external access requirements, while allowing explicit overrides. The changes include updates to the API types, Helm templates, and schema definitions, accompanied by thorough unit testing to ensure correct rendering of certificates and service configurations. Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces TLS support for the Qdrant application by adding a tls configuration field to the API and Helm values. The changes include a new certmanager.yaml template for certificate generation, updates to the qdrant.yaml HelmRelease to configure TLS and volume mounts, and the addition of comprehensive unit tests. Feedback focuses on improving template maintainability by extracting TLS enablement logic into a common Helm helper and simplifying the volume mount logic by removing redundant empty list assignments.
| {{- $tlsMap := default (dict) .Values.tls -}} | ||
| {{- $tlsEnabled := false -}} | ||
| {{- if hasKey $tlsMap "enabled" -}} | ||
| {{- $tlsEnabled = index $tlsMap "enabled" -}} | ||
| {{- else -}} | ||
| {{- $tlsEnabled = .Values.external | default false -}} | ||
| {{- end -}} |
There was a problem hiding this comment.
This logic to determine if TLS is enabled is also present in templates/certmanager.yaml. To improve maintainability and adhere to the DRY (Don't Repeat Yourself) principle, this logic should be extracted into a common helper template.
You can create a new file packages/apps/qdrant/templates/_helpers.tpl with the following content:
{{/*
Determines if TLS should be enabled based on .Values.
*/}}
{{- define "qdrant.tls.enabled" . -}}
{{- $tlsMap := .Values.tls | default dict -}}
{{- if hasKey $tlsMap "enabled" -}}
{{- $tlsMap.enabled -}}
{{- else -}}
{{- .Values.external | default false -}}
{{- end -}}
{{- end -}}
Then, you can replace this block (and the similar one in certmanager.yaml) with a single line:
{{- $tlsEnabled := include "qdrant.tls.enabled" . | toBool -}}
| {{- if $tlsEnabled }} | ||
| additionalVolumes: | ||
| - name: tls | ||
| secret: | ||
| secretName: {{ .Release.Name }}-tls | ||
| additionalVolumeMounts: | ||
| - name: tls | ||
| mountPath: /qdrant/tls | ||
| readOnly: true | ||
| {{- else }} | ||
| additionalVolumes: [] | ||
| additionalVolumeMounts: [] | ||
| {{- end }} |
There was a problem hiding this comment.
This if/else block can be simplified. Since the upstream Qdrant chart likely defines empty lists as defaults for additionalVolumes and additionalVolumeMounts, the else block which sets them to empty lists is redundant. You can remove it and only keep the if $tlsEnabled block. This makes the template cleaner and more concise.
{{- if $tlsEnabled }}
additionalVolumes:
- name: tls
secret:
secretName: {{ .Release.Name }}-tls
additionalVolumeMounts:
- name: tls
mountPath: /qdrant/tls
readOnly: true
{{- end }}![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/apps/qdrant/tests/qdrant_test.yaml (1)
160-247: ⚡ Quick winAdd explicit
tls.enabled: nullinheritance test cases.The suite validates unset/true/false, but the nullable mode (
null) is not covered. Adding these assertions will lock in tri-state behavior and prevent regressions.Suggested test additions
+ # (e) external: false, tls.enabled: null -> TLS OFF (null inherits) + - it: "(e) TLS is OFF when external is false and tls.enabled is null" + release: + name: test-qdrant + namespace: tenant-test + set: + external: false + tls: + enabled: null + asserts: + - equal: + path: spec.values.qdrant.config.service.enable_tls + value: false + - equal: + path: spec.values.qdrant.config.cluster.p2p.enable_tls + value: false + + # (f) external: true, tls.enabled: null -> TLS ON (null inherits) + - it: "(f) TLS is ON when external is true and tls.enabled is null" + release: + name: test-qdrant + namespace: tenant-test + set: + external: true + tls: + enabled: null + asserts: + - equal: + path: spec.values.qdrant.config.service.enable_tls + value: true + - equal: + path: spec.values.qdrant.config.cluster.p2p.enable_tls + value: true🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/apps/qdrant/tests/qdrant_test.yaml` around lines 160 - 247, Add two canonical tri-state tests that assert nullable inheritance when tls.enabled is explicitly null: one where set: { external: false, tls: { enabled: null } } should result in spec.values.qdrant.config.service.enable_tls and spec.values.qdrant.config.cluster.p2p.enable_tls being false and spec.values.qdrant.additionalVolumes/additionalVolumeMounts being empty, and another where set: { external: true, tls: { enabled: null } } should result in those enable_tls fields being true and spec.values.qdrant.additionalVolumes containing the tls secret (secretName: test-qdrant-tls); mirror the style and assertions used in the existing cases "(a)"–"(d)" to ensure tri-state null behavior is covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/apps/qdrant/templates/qdrant.yaml`:
- Around line 1-7: The template treats tls.enabled present-but-null as an
explicit value; change the assignment logic for $tlsEnabled so null falls back
to .Values.external (and then to false) instead of being used as-is. Replace the
hasKey/index branch with a single safe default expression that uses index
$tlsMap "enabled" but falls back to .Values.external (and then false) when
nil—for example use Helm's default function: set $tlsEnabled = default
(.Values.external | default false) (index $tlsMap "enabled") so null behaves
like unset.
---
Nitpick comments:
In `@packages/apps/qdrant/tests/qdrant_test.yaml`:
- Around line 160-247: Add two canonical tri-state tests that assert nullable
inheritance when tls.enabled is explicitly null: one where set: { external:
false, tls: { enabled: null } } should result in
spec.values.qdrant.config.service.enable_tls and
spec.values.qdrant.config.cluster.p2p.enable_tls being false and
spec.values.qdrant.additionalVolumes/additionalVolumeMounts being empty, and
another where set: { external: true, tls: { enabled: null } } should result in
those enable_tls fields being true and spec.values.qdrant.additionalVolumes
containing the tls secret (secretName: test-qdrant-tls); mirror the style and
assertions used in the existing cases "(a)"–"(d)" to ensure tri-state null
behavior is covered.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 18212de2-150e-43bd-9deb-b98a92dbbbe6
📒 Files selected for processing (11)
api/apps/v1alpha1/qdrant/types.goapi/apps/v1alpha1/qdrant/zz_generated.deepcopy.gopackages/apps/qdrant/Makefilepackages/apps/qdrant/README.mdpackages/apps/qdrant/templates/certmanager.yamlpackages/apps/qdrant/templates/qdrant.yamlpackages/apps/qdrant/tests/certmanager_test.yamlpackages/apps/qdrant/tests/qdrant_test.yamlpackages/apps/qdrant/values.schema.jsonpackages/apps/qdrant/values.yamlpackages/system/qdrant-rd/cozyrds/qdrant.yaml
| {{- $tlsMap := default (dict) .Values.tls -}} | ||
| {{- $tlsEnabled := false -}} | ||
| {{- if hasKey $tlsMap "enabled" -}} | ||
| {{- $tlsEnabled = index $tlsMap "enabled" -}} | ||
| {{- else -}} | ||
| {{- $tlsEnabled = .Values.external | default false -}} | ||
| {{- end -}} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
cd packages/apps/qdrant
cat > /tmp/qdrant-values-null.yaml <<'YAML'
external: true
tls:
enabled: null
YAML
# Verify rendered value under null is inherited from external (expected: true)
helm template test-qdrant . -f /tmp/qdrant-values-null.yaml --show-only templates/qdrant.yaml \
| rg -n -C2 'enable_tls|additionalVolumes|additionalVolumeMounts'Repository: cozystack/cozystack
Length of output: 107
🏁 Script executed:
# First, examine the qdrant template file
cd packages/apps/qdrant
find . -name "qdrant.yaml" -type fRepository: cozystack/cozystack
Length of output: 87
🏁 Script executed:
# Read the qdrant template file with line numbers
cat -n packages/apps/qdrant/templates/qdrant.yamlRepository: cozystack/cozystack
Length of output: 2571
🏁 Script executed:
# Check the values structure in this chart
ls -la packages/apps/qdrant/
cat packages/apps/qdrant/values.yaml | head -50Repository: cozystack/cozystack
Length of output: 1904
🏁 Script executed:
# Check the values.yaml to see default tls structure
cat packages/apps/qdrant/values.yamlRepository: cozystack/cozystack
Length of output: 2415
🏁 Script executed:
# Look for values.schema.json to understand the expected types
cat packages/apps/qdrant/values.schema.jsonRepository: cozystack/cozystack
Length of output: 3741
🌐 Web query:
Helm Go template null handling render "{{ null }}" output YAML
💡 Result:
In Helm templates (Go text/template), a value that is actually nil (“null” in YAML/values) does not reliably render as the literal string "null". Instead, Go/Helm ends up rendering a blank string (sometimes as a single space) when nil is expanded to text. This is why you may see YAML lines like key: instead of key: null, or weird whitespace. [1][2] What to do instead (robust patterns) 1) If you want to omit the field when the value is null/nil, guard with a nil check: {{- with .Values.someField }}{{ . }}{{- end }} or (if you need “only nil” semantics): {{- if kindIs "invalid" .Values.someField -}}...{{- end -}} Helm users commonly use with/else or explicit nil checks to avoid consuming nil values. [3] 2) If you are converting to YAML with toYaml, be aware that empty YAML scalars are parsed as Go nil, and toYaml will serialize nil as null according to YAML rules. In other words: this behavior comes from the input (empty scalars become nil) plus YAML serialization choice, not from the literal template string "{{ null }}". So to prevent inserting null, ensure the source values do not contain nils (e.g., set fields to {} or "" as appropriate), or prune/omit nils before calling toYaml. [4] 3) If you truly want the literal token null in the rendered output, don’t pass nil—pass the string "null": value: {{ "null" }} Because nil is treated as a nil value and stringification of nil yields an empty/blank result rather than the token null. [1][2] Practical mapping to your exact symptom - If your template contains something like ...: {{ null }} and the output is blank/space, that’s consistent with Go template nil rendering behavior (nil expands to whitespace/empty, not the text "null"). [2] - If your output instead contains null values inside YAML blocks generated by toYaml, then those nils were introduced earlier by YAML parsing of empty scalars in values, and toYaml is faithfully re-serializing nil as YAML null. [4] If you share a minimal snippet of your chart template + the relevant part of values.yaml, I can point to the exact fix (omit field vs force string "null" vs prune before toYaml).
Citations:
- 1: <no value> should be considered empty helm/helm#13487
- 2: https://go.dev/issue/58326
- 3: Issues while linting or templating with IF statement when Value is NIL helm/helm#8585
- 4: https://stackoverflow.com/questions/75400822/pass-an-entire-yaml-from-values-to-templates-without-filling-empty-fields-with-n
Handle tls.enabled: null as inherit-from-external, not as explicit override.
The values schema documents a tri-state contract: tls.enabled should inherit from .Values.external when unset or null, but the current code only checks key presence with hasKey. When tls.enabled is explicitly set to null, it passes the hasKey check, and index assigns nil to $tlsEnabled. This causes lines 53 and 56 to render enable_tls: (blank YAML), which violates the boolean contract and breaks Qdrant configuration.
Suggested fix
{{- $tlsMap := default (dict) .Values.tls -}}
{{- $tlsEnabled := false -}}
-{{- if hasKey $tlsMap "enabled" -}}
- {{- $tlsEnabled = index $tlsMap "enabled" -}}
-{{- else -}}
+{{- $tlsRaw := index $tlsMap "enabled" -}}
+{{- if and (hasKey $tlsMap "enabled") (ne $tlsRaw nil) -}}
+ {{- $tlsEnabled = $tlsRaw -}}
+{{- else -}}
{{- $tlsEnabled = .Values.external | default false -}}
{{- end -}}📝 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.
| {{- $tlsMap := default (dict) .Values.tls -}} | |
| {{- $tlsEnabled := false -}} | |
| {{- if hasKey $tlsMap "enabled" -}} | |
| {{- $tlsEnabled = index $tlsMap "enabled" -}} | |
| {{- else -}} | |
| {{- $tlsEnabled = .Values.external | default false -}} | |
| {{- end -}} | |
| {{- $tlsMap := default (dict) .Values.tls -}} | |
| {{- $tlsEnabled := false -}} | |
| {{- $tlsRaw := index $tlsMap "enabled" -}} | |
| {{- if and (hasKey $tlsMap "enabled") (ne $tlsRaw nil) -}} | |
| {{- $tlsEnabled = $tlsRaw -}} | |
| {{- else -}} | |
| {{- $tlsEnabled = .Values.external | default false -}} | |
| {{- end -}} |
🧰 Tools
🪛 YAMLlint (1.38.0)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/apps/qdrant/templates/qdrant.yaml` around lines 1 - 7, The template
treats tls.enabled present-but-null as an explicit value; change the assignment
logic for $tlsEnabled so null falls back to .Values.external (and then to false)
instead of being used as-is. Replace the hasKey/index branch with a single safe
default expression that uses index $tlsMap "enabled" but falls back to
.Values.external (and then false) when nil—for example use Helm's default
function: set $tlsEnabled = default (.Values.external | default false) (index
$tlsMap "enabled") so null behaves like unset.
1 participant
What this PR does
Adds TLS support to the Qdrant managed service via cert-manager.
tls.enabledfield with tri-state semantics: when unset, defaults to the value ofexternal(auto-on for externally published services, off for cluster-internal). Explicittls.enabledalways wins..ns.svc,.ns.svc.<cluster-domain>) plus the external hostname whenexternal: true.Verified end-to-end on a sandbox cluster: cert chain reaches Ready, TLS handshake against the tenant service succeeds with the chart CA bundle.
Release note
Summary by CodeRabbit
New Features
Documentation
Tests
Chores