Closes: #399 - Migrate away from bitnami/elasticsearch

[mgruner]

Which issue this PR fixes

• fixes Migrate away from bitnami/elasticsearch #399
• fixes Parameter fsGroupChangePolicy: Always causes very slow pod start #378
• also drops the now obsolete initJob.randomName setting name

Special notes for your reviewer

• Breaking change. Introduces dependency on eck-operator.

Checklist

• Chart Version bumped
• Upgrading instructions are documented in the zammad/README.md

[mgruner]

@monotek this is mostly a suggestion from Claude. Could you have a first look to see if this goes in the right direction? I need to do some manual testing.

[Copilot]

Pull request overview

Migrates the Zammad Helm chart’s bundled Elasticsearch deployment from the Bitnami subchart to Elastic’s official ECK-based eck-elasticsearch chart, updating chart wiring, documentation, and CI to reflect the new operator-managed setup.

Changes:

• Replaced the bitnami/elasticsearch dependency with elastic/eck-elasticsearch (breaking change; requires ECK operator + CRDs).
• Updated Helm templates/helpers to derive Elasticsearch host/user/secret references for the ECK-managed service/credentials.
• Updated README upgrade notes and CI validation to install the operator and validate CRD-backed manifests.

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
zammad/values.yaml	Replaced Bitnami Elasticsearch values with ECK Elasticsearch CR values and updated Elasticsearch config defaults/comments.
zammad/templates/job-init.yaml	Adjusted init job to source Elasticsearch password via helper-based secret key selection.
zammad/templates/configmap-init.yaml	Switched Elasticsearch URL/user composition to helper-based host/user resolution.
zammad/templates/_helpers.tpl	Added helpers for ECK host/user/secret name/key resolution used across templates.
zammad/README.md	Documented new ECK operator prerequisite and added upgrade notes for 16.x → 17.0.0.
zammad/ci/full-values.yaml	Removed now-irrelevant “existingSecret” config for bundled Elasticsearch in CI values.
zammad/Chart.yaml	Swapped chart dependency to eck-elasticsearch (aliased as elasticsearch) and bumped chart version to 17.0.0.
.github/workflows/ci.yaml	Installs ECK operator in CI before rendering/testing the chart.
.github/kubeconform.sh	Added CRD schema location to allow kubeconform validation of CRD-based kinds (ECK resources).
.github/ct.yaml	Added Elastic chart repo for chart-testing.
.editorconfig	Added shell indentation rules.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.

[mgruner]

@monotek can you take another look at this one, please? I will wait a bit with the merging, but your feedback would be very welcome, especially if you want some changes. I'd probably release this after the upcoming 7.1.3 security release.

@fl feel free to also review this, if you have time / tokens. :D

[monotek]

LGTM. Have not tested it though...

[fliebe92]

Had a look, here are my thoughts. Short version: I think the direction is right and this is in better shape than most subchart migrations. ECK is genuinely Elastic's supported path, routing the names through helpers is the correct shape, and the CI is doing real work here — the init script calls SearchIndexBackend.version, which forces an authenticated round-trip, so the green install-chart jobs actually prove that the elastic user, the generated password and the -es-http service wire up end to end. The upgrade section in the README is unusually thorough, thanks for that.

Most of what I found is documentation and robustness rather than architecture.

Things I'd fix before merging

1. The TLS instructions in the README don't work as written.

The README tells users to set selfSignedCertificate.disabled: false plus zammadConfig.elasticsearch.schema: https. But Zammad's es_ssl_verify setting defaults to true (db/seeds/settings.rb:3766), and nothing in the chart mounts ECK's CA cert (the <name>-es-http-certs-public secret) into the Zammad pods or turns verification off. Anyone following that advice will get an init job failing on certificate verification. I'd either wire the CA up properly or state plainly that HTTPS to the bundled Elasticsearch isn't supported yet.

2. The Elasticsearch version isn't pinned anywhere.

It comes from the subchart's own values.yaml default, which is 9.5.0 in eck-elasticsearch 0.20.0. That's fine today — Zammad accepts >= 7.8 and < 10 (lib/search_index_backend.rb:5-6) — and the failure mode is loud rather than silent, since the init job raises on an unsupported version. But the comment in updatecli.yaml asserts that chart patch releases only ever carry Elasticsearch patch updates, and that's an assumption about Elastic's release habits rather than a guarantee. I'd pin elasticsearch.version explicitly, or at minimum note the < 10 ceiling right next to that pattern so whoever raises it later knows the constraint.

3. No minimum ECK operator version is documented.

The operator's webhook validates the Elasticsearch version against its own supported range (pkg/controller/elasticsearch/validation/validations.go, supportedVersion). A cluster already running an older ECK for other workloads will reject a 9.5.0 resource. Chart 0.20.0 corresponds to operator 3.5.0 — the README should name a floor instead of just saying "install eck-operator".

4. Long release names will break.

ECK caps the Elasticsearch resource name at 36 characters (pkg/apis/elasticsearch/v1/name.go:83, MaxResourceNameLength), but zammad.elasticsearchName truncates at 63. Since the name is <release>-elasticsearch, any release name longer than 22 characters passes Helm and is then rejected by the admission webhook with a fairly opaque error. The bitnami chart had no such limit, so this is a real regression for anyone with a descriptive release name. Either use a shorter nameOverride to buy headroom, or fail early in the helper with a message that explains itself.

5. Small correctness nit in zammad.elasticsearchName.

The fallback is default "eck-elasticsearch" $es.nameOverride, but because of alias: elasticsearch, Helm rewrites the subchart's .Chart.Name to elasticsearch (pkg/chart/v2/util/dependencies.go:124), so the subchart's own fallback is elasticsearch, not eck-elasticsearch. It only bites someone who explicitly nulls nameOverride, but the helper comment claims it mirrors the subchart exactly, and it doesn't.

Honestly I'd drop the mirroring altogether: set fullnameOverride in our values.yaml and have the helpers read that single value. Then there's no upstream naming logic to stay in sync with, which is one less thing to silently rot on a subchart bump.

Worth documenting, not blocking

• Disabling HTTP TLS means the elastic superuser password crosses the cluster network in cleartext and is stored in Zammad's settings table. Still a net improvement over bitnami, which had no authentication at all, so I wouldn't block on it — but it deserves an explicit line in the README. Longer term, a scoped user via ECK's spec.auth.fileRealm would be nicer than handing Zammad a superuser.
• Leaving volumeClaimDeletePolicy unset gives us ECK's default of DeleteOnScaledownAndClusterDeletion, so disabling Elasticsearch or uninstalling the release now deletes the data volume, where the bitnami StatefulSet's PVCs survived. Low harm since the index rebuilds from PostgreSQL, but it's a behaviour change and the upgrade notes are the right place for it.
• The Elasticsearch container has a memory request but no CPU request, where bitnami's resourcesPreset: medium set one.

Two process points

The fsGroupChangePolicy change (#378) is unrelated to the Elasticsearch migration. It had its own PR (#427, now closed), it's a one-line low-risk default change, and folding it in here means people wait for a breaking major release to get it. I'd rather ship that separately in 17.x.

upgrade: true is still commented out in ct.yaml, so the 17 to 18 migration path — by far the riskiest part of this change — has no automated coverage. That's pre-existing, but this is the release where it matters most, so I'd want at least one manual 17 to 18 run against a populated instance before we ship.

One question

Requiring a cluster-wide operator with CRDs is a significant new burden for chart users: it needs cluster-admin to install, it can conflict with another team already running ECK at a different version, and it makes our chart no longer self-contained. The ECK operator is also Elastic License 2.0.

Given that PostgreSQL went to cloudpirates rather than to an operator, could we write down in #399 why a plain-StatefulSet Elasticsearch chart wasn't viable? I suspect the answer is simply "no well-maintained option exists post-bitnami", which is a perfectly good answer — but this is the decision people will push back on, so having the reasoning on record would help.

[mgruner]

1. The TLS instructions in the README don't work as written.

Improved / corrected docs to point out unsupported self-signed.

2. The Elasticsearch version isn't pinned anywhere.

That's on purpose. I don't want to manage this on my own.

3. No minimum ECK operator version is documented.

Added to docs.

4. Long release names will break.
5. Small correctness nit in zammad.elasticsearchName.

Improved in the code. Default nameOverride shortened to 'es', added explicit failure for unsupported long names.

Honestly I'd drop the mirroring altogether: set fullnameOverride in our values.yaml and have the helpers read that single value. Then there's no upstream naming logic to stay in sync with, which is one less thing to silently rot on a subchart bump.

Rejected. This would drop support for release-specific names.

• The Elasticsearch container has a memory request but no CPU request, where bitnami's resourcesPreset: medium set one.

Added request for cpu: 500m

Two process points

The fsGroupChangePolicy change (#378) is unrelated to the Elasticsearch migration. It had its own PR (#427, now closed), it's a one-line low-risk default change, and folding it in here means people wait for a breaking major release to get it. I'd rather ship that separately in 17.x.

Since it's a default value change, it would require its own major release. That's why I combined them here.

One question

Requiring a cluster-wide operator with CRDs is a significant new burden for chart users: it needs cluster-admin to install, it can conflict with another team already running ECK at a different version, and it makes our chart no longer self-contained. The ECK operator is also Elastic License 2.0.

Given that PostgreSQL went to cloudpirates rather than to an operator, could we write down in #399 why a plain-StatefulSet Elasticsearch chart wasn't viable? I suspect the answer is simply "no well-maintained option exists post-bitnami", which is a perfectly good answer — but this is the decision people will push back on, so having the reasoning on record would help.

Comment added.

[fliebe92]

Thanks, that's a good round of fixes — the fail-fast on the 36-character limit reads well, and the TLS section is much more honest now.

Conceded on fullnameOverride, you're right that it would drop release-specific names. And I'll drop the #378 point too — if a default value change needs its own major release anyway, combining them is clearly the lesser evil.

Two things though:

The cpu: 500m request doesn't seem to have made it into the push. At 158d14e, nodeSets[0].podTemplate.spec.containers[0].resources in values.yaml still only has memory: 1Gi for requests and limits. Left over in your working tree?

Fine with not pinning the ES version — that's your call to make, and the failure mode is a loudly failing init job rather than anything that damages data. But could you still add the Zammad-side constraint to the comment in .github/updatecli.yaml? It currently only says to check that the ECK operator supports the resulting version; the next person also needs to know that Zammad itself refuses anything >= 10 (lib/search_index_backend.rb:6). Since we're deliberately not pinning, that comment is the only thing standing between us and a surprise, so I think it earns the line.

Minor, and entirely up to you: with nameOverride: es, Helm's contains $base .Release.Name heuristic fires on any release name containing the substring "es" — zammad-helpdesk matches inside "desk", for example. Those installs end up with <release>-es-http rather than the <release>-es-es-http that the README and values.yaml now document. Nothing actually breaks, since our helper and the subchart use identical logic and stay in sync, so it may just be worth hedging the documented names with a "depending on your release name".

Also, did you see the volumeClaimDeletePolicy point from my last comment? ECK defaults to DeleteOnScaledownAndClusterDeletion, so uninstalling the release (or setting zammadConfig.elasticsearch.enabled: false) now deletes the Elasticsearch volume, where the bitnami PVCs survived. Harmless in practice since the index rebuilds from PostgreSQL, but it's a behaviour change that belongs in the upgrade notes.

[mgruner]

The cpu: 500m request doesn't seem to have made it into the push. At 158d14e, nodeSets[0].podTemplate.spec.containers[0].resources in values.yaml still only has memory: 1Gi for requests and limits. Left over in your working tree?

Just didn't push it - now it's there.

Fine with not pinning the ES version — that's your call to make, and the failure mode is a loudly failing init job rather than anything that damages data. But could you still add the Zammad-side constraint to the comment in .github/updatecli.yaml? It currently only says to check that the ECK operator supports the resulting version; the next person also needs to know that Zammad itself refuses anything >= 10 (lib/search_index_backend.rb:6). Since we're deliberately not pinning, that comment is the only thing standing between us and a surprise, so I think it earns the line.

I consider this obsolete. As soon as somebody tries that, CI will catch it, so we can avoid the redundancy.

Minor, and entirely up to you: with nameOverride: es, Helm's contains $base .Release.Name heuristic fires on any release name containing the substring "es" — zammad-helpdesk matches inside "desk", for example. Those installs end up with <release>-es-http rather than the <release>-es-es-http that the README and values.yaml now document. Nothing actually breaks, since our helper and the subchart use identical logic and stay in sync, so it may just be worth hedging the documented names with a "depending on your release name".

Added.

Also, did you see the volumeClaimDeletePolicy point from my last comment? ECK defaults to DeleteOnScaledownAndClusterDeletion, so uninstalling the release (or setting zammadConfig.elasticsearch.enabled: false) now deletes the Elasticsearch volume, where the bitnami PVCs survived. Harmless in practice since the index rebuilds from PostgreSQL, but it's a behaviour change that belongs in the upgrade notes.

Added.

[fliebe92]

Checked 74e1640, all good from my side — and thanks for the DeleteOnScaledownOnly escape hatch, that's a nicer answer than what I asked for.

Fair enough on the updatecli note. I verified the CI path does hold: a subchart bump marks the chart changed, ct does the real kind install, --wait blocks on the init job, and that job's SearchIndexBackend.version call raises on anything >= 10. Plus the 0.20.x pattern means the bot can only propose patches anyway. Withdrawn.

One leftover from my first comment that I think just got lost rather than rejected: there's still no note that the elastic password crosses the cluster network unencrypted, since HTTP TLS is off by default. A sentence in the auth bullet would cover it. Non-blocking either way.

Otherwise the only thing I'd still want before this leaves draft is one manual 17 → 18 run against a populated instance, since upgrade: true is still off in ct.yaml and that's the riskiest part of the change. Happy to approve once you've done that.

[mgruner]

One leftover from my first comment that I think just got lost rather than rejected: there's still no note that the elastic password crosses the cluster network unencrypted, since HTTP TLS is off by default. A sentence in the auth bullet would cover it. Non-blocking either way.

Added.

Otherwise the only thing I'd still want before this leaves draft is one manual 17 → 18 run against a populated instance, since upgrade: true is still off in ct.yaml and that's the riskiest part of the change. Happy to approve once you've done that.

Full manual update test with a long release name conducted successfully.

I added one more breaking change: removal of initJob.random name, which was obsolete by today's 17.0.1 release. Review this as well.

[fliebe92]

Checked 5aa2ba2. Plaintext note reads well — framing it against the old unauthenticated setup is the right comparison. And thanks for doing the upgrade run with a long release name, that exercises the fail-fast on the path that actually worried me.

The randomName removal is good, and I think it's more than cleanup: with randomName: false and a real helm upgrade, the constant Job name plus a changed pod template hits spec.template: field is immutable. The 300s TTL usually deleted the old Job first and masked it, but it was a trap. Your reading of the Argo case matches mine — revision is pinned at 1 there, so -init-1 was always just as static as -init, and only the Sync Hook annotation ever mattered.

LGTM. Only nit, take it or leave it: the "harmless one-time replacement" line holds unless someone upgrades while the init Job is still running, since Helm prunes the old one and a long migration would be cut off. Narrow enough that I wouldn't bother.