feat(opensearch)!: issue HTTP and Dashboards certificates from cert-manager - #2682

Code Review

This pull request introduces HTTP-layer TLS configuration for OpenSearch and Dashboards using cert-manager. It implements a tri-state logic for the tls.enabled field (auto-on when external access is enabled), adds corresponding cert-manager templates, and includes comprehensive unit and static tests. Feedback was provided regarding the placement of the TLS configuration section in values.yaml to prevent splitting the "Common parameters" table in the generated documentation.

Actionable comments posted: 2

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 `@hack/check-opensearch-readme.bats`:
- Around line 23-28: The test "topologySpreadPolicy is not in TLS configuration
section" uses `awk ... | grep -qv "topologySpreadPolicy"` which incorrectly
succeeds if any line differs; change the assertion to explicitly ensure the TLS
section does not contain the string by replacing the negative grep logic with an
explicit absence check (e.g., use the pipeline output piped into a command that
fails when "topologySpreadPolicy" is found, such as negating `grep -q
"topologySpreadPolicy"` or failing when `grep -q` matches). Update the command
in that test (the awk pipeline followed by the grep check) so it reliably fails
if `topologySpreadPolicy` appears in the TLS section.

In `@packages/apps/opensearch/README.md`:
- Around line 27-32: The README contains a duplicate heading "### Common
parameters" (first occurrence and a second before the table with
`topologySpreadPolicy` and `version`) which triggers MD024; rename the second
heading to a distinct title such as "### Common parameters (additional)" or "###
Additional parameters" so it no longer duplicates the earlier "### Common
parameters" heading, keeping the table and surrounding content unchanged.

---

Nitpick comments:
In `@packages/apps/opensearch/tests/tls_test.yaml`:
- Around line 29-249: Add a new failedTemplate test case to this tls_test.yaml
suite that sets a release name longer than 47 characters (e.g. a 48-char string)
and asserts a failedTemplate with an errorMessage indicating the
release-name-length guard (for example: "release name must be at most 47
characters" or the existing guard message). Place it alongside the other TLS
cases (near the top-level tests block) and use the same structure as other cases
(set: release: name: "<48-char-string>" and asserts: - failedTemplate:
errorMessage: "<expected message>"). Ensure the assertion uses the existing
failedTemplate symbol so the CI will catch regressions to the 47-char guard.

NOT LGTM — external: true silently auto-enables a chart-managed HTTP TLS mode that the chart itself documents as breaking the operator's securityadmin job, so security-config reconciliation (users/roles/audit) stops applying on external deployments; this is documented, not fixed or gated.

Business context: add cert-manager-backed TLS for the OpenSearch HTTP API (9200) and Dashboards (5601) in the tenant namespace, leaving operator-managed transport mTLS (9300) untouched.

Blockers

B1: external: true auto-enables an HTTP TLS mode that breaks securityadmin reconciliation
File: packages/apps/opensearch/templates/_tls.tpl:13, packages/apps/opensearch/templates/opensearch.yaml:33-38
Issue: when tls.enabled is unset, the helper turns chart-managed HTTP TLS on whenever external: true. This switches the HTTP listener from the operator-generated cert to a cert-manager leaf signed by <release>-http-ca. The operator's securityadmin job connects to the HTTP listener with an admin cert and a CA bundle rooted in the transport CA (/certs/ca.crt), which does not include <release>-http-ca. The -icl/-nhnv flags relax cluster-name and hostname checks only, not CA-chain verification, so the handshake fails, the job exits non-zero, and <release>-security-config (users/roles/audit/password policies) is never applied or refreshed.
Evidence: opensearch.yaml:35 sets the HTTP secret to <release>-http-cert when TLS is on; certmanager.yaml:51-77 issues that leaf from <release>-http-ca, distinct from the operator transport CA <release>-ca. The chart's own KNOWN ISSUE / KNOWN LIMITATION comments (certmanager.yaml:18-23, values.yaml:101-110) acknowledge that securityadmin "cannot verify the HTTP TLS chain". The latest commit (a6ee81b) adds a workaround (tls.enabled: false) to the docs but does not change the auto-on default.
Impact: an existing external OpenSearch deployment silently loses security-config reconciliation upon upgrade, with no error surfaced to the operator the platform targets; auto-on via external makes a documented-broken mode the default for external deployments.
Fix (pick one): (a) do not auto-enable chart-managed HTTP TLS from external — require an explicit tls.enabled: true opt-in, keeping operator-managed certs as the external default until the cross-trust gap is resolved; (b) overlay <release>-http-ca's ca.crt into the admin job's trust bundle; or (c) sign the HTTP cert from the operator transport CA.

Non-blocking follow-ups

1. The cert-manager leaf secret opensearch-<name>-http-cert (including the server private key tls.key) is surfaced to the tenant via the cozyrds secrets.include list (packages/system/opensearch-rd/cozyrds/opensearch.yaml:35-36). Namespace-scoped, not a cross-tenant escalation — but clients only need ca.crt/the public cert to trust the endpoint; exposing the private key is unnecessary. Compare kafka-rd which exposes a CA-only secret. Prefer a CA/public bundle.
2. The CA cert usages include key encipherment (packages/apps/opensearch/templates/certmanager.yaml:57-60), a no-op for a CA; the conventional profile is [signing, cert sign, crl sign]. Cosmetic.

Actionable comments posted: 2

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/opensearch/values.schema.json`:
- Around line 146-147: The description for tls.enabled in values.schema.json
incorrectly mentions "null" as a tri-state value while the schema enforces
"type": "boolean"; update the source annotation in values.yaml for the
tls.enabled field to say "unset/omitted" (not "null") and then regenerate the
JSON schema using the generator (cozyvalues-gen / run make generate) so
values.schema.json is rebuilt from values.yaml rather than editing
values.schema.json manually.

In `@packages/apps/opensearch/values.yaml`:
- Around line 101-109: The workaround text is misleading about operator-managed
HTTP TLS: clarify that when using external: true (external deployment), leaving
tls.enabled unset enables chart-managed TLS, so to use operator-managed HTTP TLS
in external mode users must explicitly set tls.enabled: false; update the
paragraph to replace the suggestion "keep tls.enabled unset (operator-managed
mode)" with guidance "set tls.enabled: false for operator-managed HTTP TLS when
external: true" and mention the involved keys tls.enabled and external to make
the required configuration change explicit.

---

Outside diff comments:
In `@packages/system/opensearch-rd/cozyrds/opensearch.yaml`:
- Line 29: The keysOrder array is missing a nested entry for the tls.enabled
property, causing inconsistent UI ordering; update the keysOrder array (the
keysOrder declaration) to include ["spec","tls","enabled"] immediately after
["spec","tls"] so the tls object’s enabled field is explicitly ordered like the
other nested properties (e.g., follow the pattern used for ["spec","images"] ->
["spec","images","opensearch"] and ["spec","dashboards"] ->
["spec","dashboards","enabled"]).

LGTM. My prior concern about the securityadmin cross-trust limitation is now explicitly documented:

• 30d25a616 docs(opensearch): document securityadmin cross-trust limitation in values.yaml — the chart's leaf cert is signed by the chart's own CA, but the OpenSearch operator runs securityadmin against the HTTP listener using the transport CA that the operator manages separately. The -nhnv flag passed by the operator's securityadmin job skips hostname verification but still validates the chain, so a chart CA ≠ transport CA means the bootstrap apply may fail until the operator's transport CA is overlaid into the chart's trust. This is now called out at the values doc level so operators know what they're signing up for.
• af76d5f19 test(opensearch): add failedTemplate case for the 47-char release-name guard + 15f97fc61 fix(hack): correct negative grep assertion in opensearch README bats test — the chart-side guard against names too long for the operator's StatefulSet-named-as-resource-name convention is now positive- and negative-tested.
• ab33611ea refactor(opensearch): switch CA and leaf private keys to ECDSA P-256 + 43d7801e2 refactor(opensearch): drop null from tls.enabled schema, use kindIs invalid pattern — same modernization as the rest of the batch.

Leaf usages [server auth, signing, key encipherment] — signing is cert-manager's alias for digital signature (pkg/api/util/usages.go maps both to KeyUsageDigitalSignature). No client auth because the chart cert serves only the HTTP listener; transport-side mTLS is operator-owned with the separate transport CA. dashboard RBAC exposes only <release>-credentials, no TLS Secret leak. Clean.

Verdict

NOT LGTM as of 76bb7cba3 — two things to settle, and one of them is a name that becomes expensive to change the moment this merges.

What was actually missing, which is not what the title says

Worth getting this straight first, because the title sent me looking for the wrong thing. TLS was not missing. On the merge base the chart already sets spec.security.tls.http.generate: true and spec.dashboards.tls: {enable: true, generate: true}, and templates/security.yaml already advertises https:// URIs in the credentials Secret. Both listeners were already serving TLS.

What was missing is a certificate anyone could verify. The cert came from the operator's own private CA, which no tenant can obtain, and its FQDN SAN was built from the operator's DNS_BASE=cluster.local while the Secret advertises <name>.<ns>.svc.cozy.local. So the URI the chart hands a tenant has never verified — not against a CA they could fetch, and not against a name in the SAN. This PR replaces the issuer with a per-release cert-manager CA, puts the real cluster domain in the SAN, and publishes the anchor as <release>.tenant-ca on the same contract the rest of the epic uses.

That is a better change than the title claims, and I would rather it said so. feat(opensearch)!: add TLS support for HTTP API and Dashboards describes adding a feature that was already there; this is an issuer change that makes the existing TLS verifiable. The release note opens the same way. Both worth correcting, because the next person deciding whether this affects them will read the title first.

Three things ride along that are not TLS and are not in the title either: setVMMaxMapCount flipping to false, drainDataNodes becoming conditional on replicas, and a Dashboards external-Service selector fix for a selector that matched no pod. The release note covers the first two.

Blocker 1: merge order, and it just got longer

The hard dependency on #4185 is real and this diff says so in three places. packages/system/opensearch-operator/values.yaml sets no manager.dnsBase, so the vendored default cluster.local applies. This PR closes the CA mismatch that securityadmin trips over, but the job still resolves a name composed from DNS_BASE, so the security configuration — users, roles, audit policies — still does not apply. Merged alone this ships a change whose headline benefit does not work.

The sequencing matters in the other direction too: when #4185 moves DNS_BASE, the operator-issued SANs move with it, and that is exactly the tls.enabled: false path here.

As of today #4185 is in rework at the author's own request, so this one waits on that rather than on review.

Blocker 2: tls.enabled names a switch that does not switch TLS

In this chart tls.enabled: false does not turn TLS off. It selects the operator's issuer — the else branch renders generate: true and the listener still serves TLS. So the field name states the opposite of what the field does, on a user-facing API.

This is not a general style objection; the fleet already made this call and wrote down the reasoning. packages/apps/mariadb/values.yaml:87 names its equivalent field issuer and says why, in as many words: "Named for the issuer because TLS is served under both, unlike the similarly-spelled tls.enabled in some other charts, which does switch TLS on and off." That comment exists to stop exactly this spelling from spreading, and this chart would become the counterexample it warns about. tls.issuer: operator | cert-manager matches mariadb precisely and reads true.

I am asking for this before merge rather than as a follow-up because the field is already in the ApplicationDefinition schema and there is a terraform-provider issue filed against it. Renaming afterwards costs a schema migration plus a second provider change; renaming now costs a sed.

The rest of the upgrade story, which is well handled

I went looking for the usual upgrade traps and did not find them. No Secret type changes, because every cert-manager Secret lands on a new name and none collides with an existing object — so no immutable-type wedge. No Service port change, no connection-string change (the URIs were already https:// and the leaf SAN covers the advertised name), no value renamed. The default flip is scoped: external: true with tls unset moves to the chart-managed CA, external: false does not move, and an explicit tls.enabled: false does not move.

Two things every release gets regardless of TLS, and the note covers both: the setVMMaxMapCount change rolls every pod once, and for external: true releases the external-dns name moves from the in-cluster domain to <release>.<tenant-host>, upsert-only, so the old record is orphaned rather than removed.

One omission in the note: the new release-name cap. opensearch.validateReleaseName fails the render for external plus dashboards beyond 43 characters. Such a release would previously have failed at apply on an invalid Service name, so it is not a new class of breakage — but it now withholds every object in the release, which is worth a line.

As for whether an existing client keeps working: only one that skipped verification, and given the SAN could not have matched the advertised name, that is every client that worked at all. Worth stating plainly in the note rather than leaving a reader to work out that the old path was never verifiable.

Notes, not blocking

Certificate lifetime is ten years with no renewBefore, on the CA and both leaves, against redis and mariadb's five-year CA and one-year leaf. The justification in certmanager.yaml is sound — nothing reloads the certificate, and at operator 2.8.0 the caSecret branch mounts with SubPath, which the kubelet never refreshes — and I could not find a hole in it. But the outcome is a ten-year server private key whose only rotation path is the manual delete-and-restart in the README, and that should be a decision somebody made rather than something that slipped through.

Inferring TLS from external is a deliberate departure from a rule a sibling wrote down: packages/apps/redis/values.yaml:98 says TLS is opt-in and not inferred from external. The practical effect here is milder than it sounds — flipping external swaps the CA rather than turning encryption on — and the reason is stated. Still worth one line saying the two charts differ on purpose.

secrets.exclude stays empty in the ApplicationDefinition while redis carries four entries and mariadb thirteen, both added under this epic as a named backstop behind the label-only include. There are three enumerable key-bearing Secrets here and none is listed. Not universal across the fleet, so a nit — but note that hack/check-opensearch-rd-secrets.bats pins the empty list, so adding entries later means editing the test too.

The chart's own LoadBalancer Service names are in no SAN, so a client dialling <name>-external by in-cluster name fails hostname verification. The operator's certificate did not cover it either, so no regression, and the e2e sidesteps it with an explicit -servername.

The cross-release Secret collision documented in certmanager.yaml:49-60 deserves its own issue rather than a comment: templates/users.yaml renders <release>-user-<username> with the username tenant-supplied and unconstrained by the schema, so release x with a user named http-server collides with release x-user. Both objects are Helm-owned, so the realistic outcome is the second release wedging on ownership rather than key disclosure — but it is cross-release and tenant-triggerable, which is the combination worth tracking.

drainDataNodes: {{ gt (int .Values.replicas) 1 }} counts total nodes rather than data nodes, so nodeRoles.data: false with three replicas still asks for a drain. Harmless, but the expression does not say what the comment above it means.

On comment volume: these are WHY comments and they satisfy the letter of AGENTS.md, but the dot-in-the-Secret-name argument alone is restated in certmanager.yaml, opensearch.yaml, values.yaml, the README and the bats file. certmanager.yaml is 341 lines of which roughly 190 are comment. Collapsing that to one canonical place would cut the cost of reading this diff more than anything else in it. Related: the DNS-label footnote now attached to dashboards.enabled ships into the ApplicationDefinition and becomes help text under an on/off toggle in the dashboard UI — that belongs in the README.

Checked and clean

The vendored-chart rule does not apply: packages/apps/opensearch/charts/cozy-lib is a symlink to the in-repo library chart, and the only file touched under packages/system/opensearch-operator/ is that package's own template directory, not its vendored charts/. Generated artifacts are consistent — the chart's values.schema.json is byte-identical to the ApplicationDefinition's openAPISchema, keysOrder gained ["spec","tls"] in the right place, the README table matches, and the deepcopy is regenerated. The package Makefile has a test: target, so hack/helm-unit-tests.sh actually discovers the suite — 11 suites, 140 tests, green here — and the assertions are non-vacuous: reverting the TLS templates turns the upgrade suite red on the notExists generate and Secret-name assertions. The chainsaw suite follows the repo conventions and its last step is the one that earns it, reading the certificate the listener actually serves and fingerprint-matching it against the chart's leaf. Tenant boundary holds: the anchor reaches the tenant only through the label selector, the dashboard Role grants the credentials Secret by name, and the e2e asserts the negative — the key-bearing Secrets are not served as tenant secrets.

Fix the field name and I have no objection to the rest; the merge still waits on #4185.

LGTM as of 76c9ece4a.

Both blockers are closed. #4185 is merged and this branch sits directly on its merge commit, e89617d75, which is the merge-base here, so the securityadmin job now resolves on the platform domain rather than cluster.local. And tls.enabled is tls.issuer with operator and cert-manager, carrying the same reasoning mariadb wrote down, so the field no longer names a switch that does not switch TLS off. The title says what the change does now too.

The version floor is the part I checked hardest, since it decides what an upgrade does to a release nobody is watching. _tls.tpl resolves the tri-state the way the comment says: explicit value obeyed, absent one follows external, and below 2.0.0 an explicitly requested cert-manager fails the render while an inferred one degrades to operator. That asymmetry is right. Someone who asked for a per-release CA should hear that they cannot have it on that version; someone who only asked for external access should keep the release they already have.

Two things I had flagged last time I am withdrawing. The certificate lifetime now carries the argument rather than just the number, including that dropping renewBefore does not stop reissue since cert-manager still defaults to duration/3, and that a 30-day cycle was a promise nothing here could honour. And secrets.exclude being empty reads differently now that include is an allowlist: one entry by resourceNames for the credentials Secret and one label selector that matches the key-free projection by design, so there is no broad label include for an exclude list to backstop.

What I would still like to know is how the release-name bound was confirmed. The guard withholds every object in the release, and its premise is that a release past the bound was already stuck: the operator composes the Dashboards Service past the 63-character label limit, fails on every pass, and the reconcilers queued behind Dashboards have not run since the release was created. If that holds, the guard turns a silent stall into a message, which is worth a lot. If it does not, the guard turns a working release into a failed one. The operator's Go source is not vendored here, only its chart, so I cannot check dashboards.Reconcile ordering at 2.8.0 from the tree, and the chainsaw suite does not exercise a name past either bound. A line saying how you established it, or one e2e case at 42 characters, would close it.

One follow-up worth its own issue rather than more prose in values.yaml. Changing external, the tenant host or the cluster domain reissues both certificates while the pods go on serving the old material, with no error anywhere, and the fix is a manual delete plus restart on each side. Nothing in the chart detects that state: there is no checksum annotation over the certificate, so nothing rolls the pods and nothing reports the mismatch. The documentation is thorough about it, which is the right first move, but a condition that is invisible until a client fails verification will be found by the client.

The suite is 12 files and 144 tests, green here, and the generated artifacts line up: values.schema.json and the ApplicationDefinition's openAPISchema are identical. Nothing under any charts/ directory is touched, so the vendored-chart rule does not come into it.