From 978e6fbaa8e578990eb985b043de3392d04bd26b Mon Sep 17 00:00:00 2001 From: Justin Klein Date: Tue, 13 Oct 2026 11:03:36 +0000 Subject: [PATCH] refactor(stalwart): name the MTA-STS Ingress objects by index, per the corrected rule Maintainer's decision, and it reverses this chart's naming rather than the style guide's rule. The objects become -stalwart-mta-sts-ingress, -1, -2, ... in this repo's idiom ({{ if $i }}-{{ $i }}{{ end }}), the same as homeassistant's routes and keycloak's realm Ingresses, inlined in the template instead of behind a helper of its own. WHY IT GOES THIS WAY. This chart named by issuer, reading the style guide's index rule as resting on "a route holds no state and no certificate" and observing that these objects hold one. That reading was right and the rule's STATED REASON was wrong: the parallel #122 round named by index, cited the same rule as binding, and entered keycloak into it - whose realm Ingresses hold certificates too. Two charts did opposite things for one shape, and the rule had been extended onto cases its own precondition excluded. #125 is merged and published, so the reason is repaired and the naming unified on index; keycloak is not touched. What actually carries the index rule, in three steps that have different standing and are written as such in the template header and the style guide: cert-manager here runs WITHOUT --enable-certificate-owner-ref (MEASURED by the maintainer - absent from the Deployment's args and from the ControllerConfiguration its --config names, which carries only kind: ControllerConfiguration and enableGatewayAPI: true); by that default the Certificate is collected with its Ingress but the SECRET IS NOT (DOCUMENTED behaviour, not measured here), so a rename recreates the Certificate object against a surviving secret and issues nothing; and NO RENAME WAS APPLIED against a cluster. The promise therefore hangs on a cluster-level flag this repository does not control - with it on, a reorder would cost a fresh ACME request per renumbered object. UNCHANGED: the structure. One Ingress per distinct clusterIssuer, one rule per host, tls grouped by secret, and the abort when one secret spans two issuers - grouping is a cert-manager question and naming is a style question, and they are independent. TWO THINGS THE RENAME OBLIGES. 1. NOTES.txt now prints the mapping - index, object name, issuer, secrets and hosts per object, the way keycloak's does for its realms - because the name no longer carries the issuer and nothing else in the cluster says which object holds which. 2. The reordering hazard is documented as SHARPER than homeassistant's: there an index names a host, here it names an issuer GROUP derived from the list, so moving one entry can change the grouping and renumber objects that were not themselves moved. AN ASYMMETRY WITH keycloak, named in the template header: there both modes render one object per entry, so the index question arises twice. Here it arises on the Ingress side ONLY - Gateway mode renders one HTTPRoute carrying every hostname, because TLS is none of the route's business, so there is no index and no name to go stale. Version stays 1.3.0+upv0.16.25: nothing is published yet, so nothing is being renamed in a release. Re-measured after the rename, not assumed: three empty helm template diffs against origin/main (defaults, CI fixture, overlay), helm lint --strict clean three ways, every abort path still firing with the structural one still winning over the per-entry one, the schema still rejecting mtaSts.enabled, and the HTTPRoute render test still at 64 assertions over 22 charts - object names are not assertions. --- stalwart/templates/NOTES.txt | 39 ++++++++++++++--- stalwart/templates/_helpers.tpl | 40 ++++++++---------- stalwart/templates/stalwart.ingress.yaml | 54 +++++++++++++++++++++++- stalwart/values.yaml | 19 +++++++++ 4 files changed, 120 insertions(+), 32 deletions(-) diff --git a/stalwart/templates/NOTES.txt b/stalwart/templates/NOTES.txt index 377e691..42c7ae5 100644 --- a/stalwart/templates/NOTES.txt +++ b/stalwart/templates/NOTES.txt @@ -331,20 +331,45 @@ WEB UI AND JMAP — INGRESS MODE (ingress.enabled: true, httpRoute.enabled: fals Ingress resolves to a port that answers nothing. {{- if $mtaStsHosts }} -MTA-STS — {{ len $mtaStsHosts }} POLICY ROUTE(S), INGRESS MODE +{{- $mtaStsGroups := fromYaml (include "stalwart.mtaSts.ingressGroups" .) }} + +MTA-STS — {{ len $mtaStsHosts }} POLICY HOST(S) IN {{ len $mtaStsGroups.order }} INGRESS OBJECT(S), INGRESS MODE All of them → node group "{{ $ingressGroup }}", path {{ $mtaStsPath }} only (pathType Exact) -{{ range $entry := $mtaStsHosts }} - https://{{ $entry.host }}{{ $mtaStsPath }} - secret {{ $entry.tlsSecretName }}{{ if $entry.clusterIssuer }}, requested from the "{{ $entry.clusterIssuer }}" cluster issuer{{ else }}, NO cluster issuer set — this release does not request that certificate, it has to exist already{{ end }} + + ONE OBJECT PER CLUSTER ISSUER, NAMED BY INDEX — so the name does not say + which issuer it carries, and this is the mapping. Read it here rather than + guessing from the object names: +{{- range $i, $issuer := $mtaStsGroups.order }} +{{- $grp := index $mtaStsGroups.groups $issuer }} + + {{ printf "%s-mta-sts-ingress%s" $release (ternary "" (printf "-%d" $i) (eq $i 0)) }} + cluster issuer: {{ if $issuer }}"{{ $issuer }}" — cert-manager requests these certificates{{ else }}NONE — no annotation, so this release requests nothing; every secret below has to exist already{{ end }} + {{- range $secret := $grp.tlsOrder }} + secret {{ $secret }} + {{- range $host := index $grp.tls $secret }} + https://{{ $host }}{{ $mtaStsPath }} + {{- end }} + {{- end }} {{- end }} - One Ingress object PER CLUSTER ISSUER, not one per host: the cert-manager - issuer is an object-level ANNOTATION while the TLS secret is a per-entry - field, so one object cannot both request a certificate and leave another + Why one object per issuer rather than one for everything: the cert-manager + issuer is an object-level ANNOTATION while the TLS secret is a per-tls-entry + field, so a single object cannot both request a certificate and leave another host's secret alone. Hosts sharing a secret share one tls: entry, so one certificate with several names is requested once instead of once per host: kubectl -n {{ .Release.Namespace }} get ingress -l app={{ $release }} + AND MIND THE ORDER OF mtaSts.hosts. The index above runs over the issuer + GROUPS, which are derived from the list — so reordering entries can change + how many objects there are and which index carries which issuer, renumbering + objects that were not themselves moved. Kubernetes then deletes and recreates + them, which takes their Certificate objects with them. No certificate is + RE-ISSUED by that, because the Secrets survive it (cert-manager leaves them + alone unless it runs with --enable-certificate-owner-ref, which this cluster + does not set) — but that is a property of the CLUSTER, not of this chart. If + that flag is ever switched on, a reorder means a fresh ACME request per + renumbered object. + TWO THINGS THIS RELEASE DOES NOT DO, AND BOTH ARE SILENT WHEN MISSING: 1. IT DOES NOT CREATE THE DOMAIN. The domains live in Stalwart's data diff --git a/stalwart/templates/_helpers.tpl b/stalwart/templates/_helpers.tpl index 5da0287..08a314f 100644 --- a/stalwart/templates/_helpers.tpl +++ b/stalwart/templates/_helpers.tpl @@ -695,29 +695,23 @@ logged on this side. {{- end -}} {{/* -The name of the MTA-STS Ingress object for one clusterIssuer group. Input: dict -"root" "issuer" . +NOTE ON THE OBJECT NAMES, because this is where an earlier draft of this chart +disagreed with the rest of the repository and lost. -NOT AN INDEX, and the reason is the one exception the comparable precedent -names itself. homeassistant carries the entry's index in its HTTPRoute names -(Gitea #114, group 3) and writes down that reordering the list therefore -recreates rather than edits the objects - "harmless (a route holds no state or -certificate)". These objects DO hold a certificate: cert-manager owns the -Certificate from the Ingress, so a rename deletes one Certificate and creates -another. The Secret survives that - cert-manager does not delete it with the -Certificate - so no re-issue follows and nothing breaks, but it is churn on an -ACME account in return for a reordered list, and the issuer is a stable key -that costs nothing to use instead. +The MTA-STS Ingress objects are named BY INDEX over the issuer groups - +"-stalwart-mta-sts-ingress", then "-1", "-2" - inlined in +stalwart.ingress.yaml in the same idiom as homeassistant's routes and +keycloak's realm Ingresses ({{ if $i }}-{{ $i }}{{ end }}), rather than by a +helper of their own. -The issuer-less group takes the unsuffixed name, and that cannot collide with -an issuer's: a clusterIssuer name is never the empty string, so no suffix is -ever empty. +An earlier draft named them after the clusterIssuer, arguing that the style +guide's index rule rested on "a route holds no state and no certificate" and +that these objects hold one. The reading of the rule was right and the rule's +STATED REASON was wrong - that sentence has been corrected in the style guide +(section 4) rather than the naming of two charts, because keycloak's realm +Ingresses hold certificates too and are already published. What actually +carries the index rule is that THE SECRET SURVIVES a rename, so no certificate +is re-issued; the full three-step version, including which step is measured and +which is only documented behaviour, is in that section and in the header of +stalwart.ingress.yaml. */}} -{{- define "stalwart.mtaSts.ingressName" -}} -{{- $base := printf "%s-mta-sts" (include "stalwart.fullname" .root) -}} -{{- if .issuer -}} -{{- printf "%s-%s-ingress" $base .issuer -}} -{{- else -}} -{{- printf "%s-ingress" $base -}} -{{- end -}} -{{- end -}} diff --git a/stalwart/templates/stalwart.ingress.yaml b/stalwart/templates/stalwart.ingress.yaml index 56c882e..a740348 100644 --- a/stalwart/templates/stalwart.ingress.yaml +++ b/stalwart/templates/stalwart.ingress.yaml @@ -108,6 +108,56 @@ AND THE LIMIT THAT NO OBJECT HERE CAN FIX: this chart registers a ROUTE, it does not create a DOMAIN. Whether Stalwart answers for mta-sts. depends on existing in its data store, which is not in any chart value (see values.yaml, line 1 onwards, and the mtaSts block). + +THE NAMES ARE INDEXED OVER THE ISSUER GROUPS, AND THE INDEX IS ONLY AN INGRESS +QUESTION + +"-stalwart-mta-sts-ingress", then "-1", "-2" - the repo's idiom +({{ if $i }}-{{ $i }}{{ end }}), as homeassistant's routes and keycloak's realm +Ingresses use it (style guide section 4). The index runs over the ISSUER +GROUPS, not over mtaSts.hosts entries: the grouping is a separate decision, +made for cert-manager's sake and argued in stalwart.mtaSts.ingressGroups, and +it is untouched by how the objects are named. + +AN ASYMMETRY WITH keycloak WORTH KNOWING: there, both modes render one object +per entry, so the index question arises twice. HERE IT ARISES ONLY ON THIS +SIDE. Gateway mode renders exactly ONE HTTPRoute carrying every hostname, +because TLS is not involved there at all - no certificate, no secret, nothing +to split an object over - so there is no index and no name to go stale. The +whole of the naming question below is an Ingress-mode question. + +THE REORDERING HAZARD IS SHARPER HERE THAN AT homeassistant, so read this +before shuffling mtaSts.hosts. There, an index names a host, and reordering +changes which index serves which host. Here an index names an ISSUER GROUP +derived from the list, so reordering can change THE GROUPING ITSELF - how many +objects exist, and which index carries which issuer and which hosts. Moving +one entry can therefore renumber objects that were not themselves moved. What +makes it bearable rather than a reason to avoid indices: + + 1. MEASURED (by the maintainer, in jk-cluster, on this round): cert-manager + runs WITHOUT --enable-certificate-owner-ref - the flag appears neither in + the cert-manager Deployment's args nor in the ControllerConfiguration its + --config points at (which carries only kind: ControllerConfiguration and + enableGatewayAPI: true). So the controller's default applies. + 2. DOCUMENTED BEHAVIOUR of that default, not measured here: the Certificate + is garbage-collected with the Ingress it is owner-referenced from, but the + SECRET is not - cert-manager only adds an owner reference to the Secret + when that flag is on. A rename therefore deletes and recreates the + Certificate object while the Secret stays, so the new Certificate finds a + valid secret already present and ISSUES NOTHING. No ACME request, no rate + limit touched. + 3. NOT APPLIED: no rename was actually performed against a cluster. The two + steps above are the mechanism, not a trial of it. + +And note what the promise hangs on: a CLUSTER-LEVEL FLAG this repository does +not control. With --enable-certificate-owner-ref switched on, the Secret would +be collected with the Certificate and a reorder WOULD mean a re-issue for every +renumbered object. That is worth checking before anyone turns it on, and it is +not something a chart can defend itself against. + +Because the name no longer carries the issuer, NOTES.txt prints the mapping - +index, issuer and hosts per object - the way keycloak's does for its realms. +Without it, nothing in the cluster says which object holds which issuer. */ -}} {{- $httpRoute := .Values.httpRoute | default dict }} {{- if and .Values.ingress.enabled (not $httpRoute.enabled) }} @@ -121,13 +171,13 @@ values.yaml, line 1 onwards, and the mtaSts block). {{- end }} {{- $service := printf "%s-%s-http-service" (include "stalwart.fullname" .) $group }} {{- $path := include "stalwart.mtaSts.path" . }} -{{- range $issuer := $mtaSts.order }} +{{- range $i, $issuer := $mtaSts.order }} {{- $entry := index $mtaSts.groups $issuer }} --- apiVersion: networking.k8s.io/v1 kind: Ingress metadata: - name: {{ include "stalwart.mtaSts.ingressName" (dict "root" $ "issuer" $issuer) }} + name: "{{ include "stalwart.fullname" $ }}-mta-sts-ingress{{ if $i }}-{{ $i }}{{ end }}" labels: app: {{ include "stalwart.fullname" $ }} {{- if $issuer }} diff --git a/stalwart/values.yaml b/stalwart/values.yaml index c208c83..347d104 100644 --- a/stalwart/values.yaml +++ b/stalwart/values.yaml @@ -775,6 +775,25 @@ httpRoute: # argument, with the cert-manager behaviour it was measured # against, is in stalwart.mtaSts.ingressGroups in # _helpers.tpl. +# Named BY INDEX over those groups — +# -stalwart-mta-sts-ingress, then -1, -2 — in this +# repo's idiom (README/style guide section 4, as +# homeassistant's routes and keycloak's realm Ingresses do +# it). So the NAME DOES NOT SAY WHICH ISSUER it carries: +# NOTES.txt prints that mapping, and it is the only place the +# association is visible. +# REORDERING THIS LIST IS NOT FREE. The index runs over the +# issuer groups, which are derived from the list, so moving +# one entry can change how many objects exist and which index +# holds which issuer — renumbering objects that were not +# themselves moved. Kubernetes recreates them, which takes +# their Certificate objects along; nothing is RE-ISSUED, +# because the Secrets survive (cert-manager only +# owner-references a Secret with +# --enable-certificate-owner-ref, which this cluster does not +# set). That last part is a property of the CLUSTER and not +# of this chart: with that flag on, a reorder costs a fresh +# ACME request per renumbered object. # Gateway mode — ONE HTTPRoute with all the hostnames. Nothing differs per # host here (same path, same backend, no filter) and # spec.hostnames is a list, so one object says exactly what