feat(qdrant): add TLS support via cert-manager - #2685

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.

Actionable comments posted: 1

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.

NOT LGTM — TLS scaffold is mostly clean (cert chain is well-shaped, leaf usages cover both server-auth and client-auth for p2p mTLS, tls.enabled tri-state via hasKey is sound), but the SAN list cannot match the short-form pod hostnames Qdrant uses for p2p bootstrap, and there's no restart trigger when cert-manager rotates the leaf — TLS will silently break at the one-year mark.

Business context: Add TLS to the managed Qdrant service for both client REST/gRPC (6333/6334) and inter-node p2p (6335) via a self-contained cert-manager chain, with tls.enabled falling back to external when unset.

Blockers

B1: SAN list does not cover Qdrant's short-form p2p peer hostnames

File: packages/apps/qdrant/templates/certmanager.yaml:61-68

Issue: When replicas > 1, the upstream Qdrant chart starts each pod with --bootstrap '<protocol>://<release>-0.<release>-headless:6335' and --uri '<protocol>://<release>-<N>.<release>-headless:6335' (see packages/system/qdrant/charts/qdrant/templates/configmap.yaml, initialize.sh). These URIs use the short form <release>-N.<release>-headless — no .<ns>.svc.<cluster> suffix. X.509 wildcards match exactly one DNS label at the leftmost position; *.<release>-headless.<ns>.svc.<cluster> cannot match <release>-0.<release>-headless (different label count).

Evidence: configmap.yaml:initialize.sh builds the URI from qdrant.fullname + -headless — no FQDN suffix. The peer TLS client (Qdrant's tonic-based p2p) verifies the cert's SAN list against the literal URI hostname, not the post-resolution FQDN. Cert SAN list at certmanager.yaml:62-68 only covers <release>{,.<ns>.svc{,.<cluster>}}, the bare headless service plus its FQDN forms, and the wildcard *.<release>-headless.<ns>.svc.<cluster> — none of which match <release>-N.<release>-headless.

Impact: Multi-replica + TLS-enabled deployments fail to form a cluster — qdrant-1/qdrant-2/… each attempt the --bootstrap URI against qdrant-0.<release>-headless and the TLS handshake fails with a hostname-mismatch error. Single-replica deployments (the chart default) happen to work because qdrant-0's self-URI is never dialed.

Fix: Add SANs for each pod's short form. The chart already has .Values.replicas, so:

{{- range $i, $_ := until (int .Values.replicas) }}
- {{ printf "%s-%d.%s-headless" $.Release.Name $i $.Release.Name | quote }}
- {{ printf "%s-%d.%s-headless.%s.svc" $.Release.Name $i $.Release.Name $.Release.Namespace | quote }}
- {{ printf "%s-%d.%s-headless.%s.svc.%s" $.Release.Name $i $.Release.Name $.Release.Namespace $clusterDomain | quote }}
{{- end }}

(Scaling up later requires reissuing the cert; alternatively use a multi-label wildcard escape via the SVID/SPIFFE-style URI SAN, but the per-replica approach is simpler.)

B2: No restart trigger on cert-manager rotation — TLS breaks silently at the cert-expiry mark

File: packages/apps/qdrant/templates/qdrant.yaml:57-65 + templates/certmanager.yaml:47-48

Issue: cert-manager rotates the leaf every ~11 months (duration: 8760h, renewBefore: 720h). Qdrant reads tls.cert/tls.key/tls.ca_cert from /qdrant/tls/ at process start and does not watch the file for changes — neither actix-web (REST) nor tonic (gRPC) hot-reloads server certs in Qdrant's current configuration. After rotation:

1. kubelet syncs the new Secret content into the mounted volume (the on-disk file is fresh).
2. Qdrant keeps serving with the in-memory copy of the original cert.
3. At month-12, the original cert hits notAfter. Clients now reject the handshake with certificate has expired.

Nothing in the HelmRelease re-triggers a rolling restart when the Secret content changes — Flux's helm-controller hashes the chart artifact + HelmRelease values, not the contents of side-loaded Secrets.

Evidence: packages/system/qdrant/charts/qdrant/templates/statefulset.yaml carries checksum/config and checksum/secret annotations, but they hash configmap.yaml and secret.yaml (the API-key secret) — not the cert-manager-managed TLS Secret created by templates/certmanager.yaml. No reloader.stakater.com/* annotation is set on the pod template (cozystack ships stakater/reloader at packages/system/reloader/, so the operator is available cluster-wide).

Impact: A successfully-installed TLS-enabled Qdrant instance becomes unreachable to all clients exactly 12 months after install with no warning, until an operator manually restarts the StatefulSet. The renewBefore window helps cert-manager refresh on disk early, but does nothing because Qdrant never re-reads.

Fix: Pass an annotation that the reloader operator picks up:

{{- if $tlsEnabled }}
podAnnotations:
  reloader.stakater.com/auto: "true"
{{- end }}

…or pin it to the TLS secret specifically:

podAnnotations:
  secret.reloader.stakater.com/reload: "{{ .Release.Name }}-tls"

Upstream chart supports podAnnotations in statefulset.yaml (verified). With reloader installed cluster-side, a Secret content change will trigger a rolling restart and the new cert is loaded before the old one expires.

Non-blocking follow-ups

1. qdrant.yaml:57-69 — the additionalVolumes / additionalVolumeMounts block is unconditionally rewritten (overwriting any value coming from the cozystack-values Secret, including the explicit [] reset in the TLS-disabled branch). Tenants in the standard managed flow can't reach those keys via the ApplicationDefinition surface, so the practical regression is narrow, but the chart-level habit of overwriting upstream extension points instead of appending is worth flagging — if Qdrant's user values ever need an extra mount, this will silently drop it.

2. certmanager.yaml:28-29: CA privateKey only sets rotationPolicy: Never, leaving algorithm/size at cert-manager defaults (RSA-2048). Leaf cert has no privateKey block at all. Mariadb (#2680) and other sibling PRs pin RSA-4096 for the CA — non-uniform but not a blocker; RSA-2048 is still a sound choice for a tenant-scoped CA.

3. _helpers.tpl:1-13 — the tri-state implementation uses hasKey $tlsMap "enabled" and relies on values.schema.json rejecting null (since enabled is type: boolean, not ["boolean","null"]). That's correct given the current schema but tightly coupled — a future schema relaxation to null would let null reach the template as the string "<nil>" and silently flip TLS on (since neither "true" nor "false"). Worth either tightening the template (treat anything other than "true"/"false" as the external fallback) or pinning the schema with a comment that null is intentionally rejected.

LGTM. Each finding from my prior NOT LGTM on this branch is addressed in a dedicated commit:

• 8a426487c fix(qdrant): add per-replica short-form SANs for p2p TLS hostname verification — closes the SAN gap I flagged: initialize.sh connects peers via <release>-N.<release>-headless (no FQDN), so the per-replica short form {{ printf "%s-%d.%s-headless" $.Release.Name $i $.Release.Name }} now sits in the SAN list alongside the namespaced and FQDN variants and the wildcard.
• 3e59b8491 feat(qdrant): trigger pod restart on TLS Secret rotation via Reloader annotation — secret.reloader.stakater.com/reload: "{{ .Release.Name }}-tls" makes Stakater Reloader bounce pods when the leaf Secret rotates (cert-manager's rotationPolicy: Always). Solves the "cert rotated but pods still hold the old material" failure mode.
• 7544cdbf8 refactor(qdrant): switch CA and leaf private keys to ECDSA P-256 + 1192ae69c refactor(qdrant): use kindIs invalid pattern for tri-state tls.enabled detection + fc943ddda refactor(qdrant): extract TLS enablement logic into _helpers.tpl — modernization pass consistent with the rest of the batch.

Leaf usages [digital signature, key encipherment, server auth, client auth] ✓. dashboard RBAC exposes only <release>-apikey (no TLS Secret), so the B3 pattern doesn't apply here either. Clean.

LGTM — TLS via cert-manager is correctly wired for client REST/gRPC and inter-node p2p, both blockers from the earlier review (p2p short-form SANs and restart-on-rotation) are fixed and test-covered, and 58 helm-unittest cases pass.

Business context: adds TLS to the managed Qdrant service for client REST/gRPC (6333/6334) and inter-node p2p (6335) via a self-contained cert-manager chain in the tenant namespace, with tls.enabled falling back to external when unset.

Non-blocking follow-ups

1. p2p SANs are enumerated for the current replicas only (packages/apps/qdrant/templates/certmanager.yaml:75-79). On a replicas N→M change the Certificate re-renders and the StatefulSet scales in the same reconcile with no ordering guarantee (podManagementPolicy: Parallel), so a new pod can attempt p2p TLS before the reissued secret propagates and reloader restarts — a transient scale-up stall, not a permanent break; fresh installs are unaffected. Optional hardening: a short-form wildcard SAN "*.{{ .Release.Name }}-headless" covers future replica names without enumeration.
2. CA and leaf both use ECDSA P-256 (clean within this PR); a sibling cert-manager PR pins RSA-4096 for the CA. Worth a one-line confirmation that the cross-app divergence is intentional.
3. The tri-state relies on kindIs "invalid" to detect the omitted case while the schema keeps tls.enabled non-nullable (a literal null is rejected before rendering). Sound, but tightly coupled to the schema staying non-nullable — a brief comment pinning that intent would harden it against a future schema relaxation.