Merge pull request #226 from jk-cluster/fix/urlaubsverwaltung-health-probes

urlaubsverwaltung: readiness must see the database, health must not lie (9.0.0+up6.7.0)

README.md

@@ -245,6 +245,7 @@ These guidelines are **binding for every current and future chart** in this repo
 
 - Every workload container offers an additive `env` value block, appended to the env entries the chart always sets.
 - Entries are rendered through `tpl` (so values may contain Helm templating) and support the `value`, `secretKeyRef` and `configMapKeyRef` forms (pattern: `template-chart/templates/xxx.deployment.yaml`).
+- **Additive means additive: an entry repeating a name the chart already sets does not replace it, and what the pair does depends on when it arrives.** At install both entries reach the pod and the later one — the user's — wins. On an *existing* release the same entry is lost: Kubernetes merges `env` by name, the pair collapses to the chart's own value, and `helm upgrade` still reports success. Measured on `urlaubsverwaltung` (helm 3.19.0, Kubernetes 1.35): `helm get manifest` shows the variable twice while the live Deployment shows it once, carrying the chart's value. The failure mode is silence, so a chart whose `NOTES.txt` tells operators to override one of its own variables has to say that the override only takes at install time — and that a green upgrade proves nothing, the value belongs read back off the Deployment.
 
 ### 11. Scaling
 
@@ -270,6 +271,8 @@ Reference implementation: `template-chart/values.schema.json` and `template-char
 
 **3. Do not hang liveness on an external dependency where a dependency-free path exists.** `zipline` deliberately probes `/` for liveness and `/api/healthcheck` only for readiness and startup, so a database outage cannot restart the pod. Where no such path exists — `outline` returns 500 on `/` when the database is down — all three probes may share the endpoint, but the reason belongs in a comment on the helper, or the next reader will "fix" the inconsistency.
 
+  The other half of the same rule: readiness **must** hang on the dependency, and a separate path is not proof that it does. `urlaubsverwaltung` probed `/actuator/health/liveness` and `/actuator/health/readiness` for two years and still stayed 1/1 Ready through a total database outage (#218), because Spring Boot's `readiness` group contains `readinessState` **alone** — the split paths were right, the group content was empty of anything external. Whatever aggregates behind a probe path (Spring health groups, a `/healthz` a framework assembles for you) has to be read off the **running** endpoint, not off the path name; the chart names the members explicitly (`MANAGEMENT_ENDPOINT_HEALTH_GROUP_READINESS_INCLUDE=readinessState,db`) so the composition is in the template rather than in an upstream default.
+
 **4. A fallback reproduces *this chart's* defaults, not the repo standard.** Five charts deviate from the hardening baseline on purpose. A "cleaned up" standard default is not tidying, it is an outage: `calibre-web-automated` must keep `readOnlyRootFilesystem: false`, because in read-only mode the image disables its PUID/PGID remapping and the runtime uid silently moves from 1000 to 911 — **making existing volume data inaccessible**. The same applies to non-standard values that look like mistakes: `satisfactory` keeps `limitFactor: 2` (that is what holds the memory limit at the published 8Gi) and `fsGroupChangePolicy: OnRootMismatch` (which avoids a recursive chown of multi-GB game files on every start).
 
 **5. A probe default carries the probe's context, not just its timings.** Two different symptoms, both from real charts: `teamspeak-server` addresses its TCP port by the name `file-transfer` because the voice service is UDP and cannot be probed — a default without that name leaves the workload **with no working probe at all**. `patchmon` sends a `Host` header because the server answers **403** to any request whose Host is neither loopback nor a CORS_ORIGIN host — a default without it means the pod never becomes Ready. Port names, `Host` headers and `exec` commands belong in the fallback; where the context is dynamic, pass the root context in and rebuild it (patchmon derives the header from `ingress.domain`).

urlaubsverwaltung/Chart.yaml

@@ -4,5 +4,5 @@ description: A Helm chart to deploy urlaubsverwaltung on Kubernetes
 icon: https://raw.githubusercontent.com/urlaubsverwaltung/urlaubsverwaltung/main/src/main/resources/static/favicons/apple-touch-icon.png
 type: application
 
-version: 8.1.0+up6.7.0
+version: 9.0.0+up6.7.0
 appVersion: 6.7.0

urlaubsverwaltung/templates/NOTES.txt

@@ -199,33 +199,89 @@ THE MEMORY LIMIT SIZES THE JAVA HEAP — IT IS NOT JUST A CEILING
 
     kubectl -n {{ .Release.Namespace }} logs deployment/{{ .Release.Name }}-{{ .Chart.Name }}-deployment | head -1
 
-DO NOT HEALTH-CHECK THIS RELEASE ON /actuator/health
-  That endpoint aggregates every health indicator, and one of them is the mail
-  sender pointed at localhost:1025 by this chart (see below). Nothing listens
-  there, so the aggregate answers 503 DOWN on a completely healthy release,
-  for the lifetime of that release:
+THE THREE HEALTH PATHS AND WHAT EACH ONE ANSWERS FOR
+  This chart sets two health settings in the deployment template, and they are
+  what makes the three paths mean different things:
 
-    {"components":{ ... "mail":{"status":"DOWN"} ... },"status":"DOWN"}
+    MANAGEMENT_ENDPOINT_HEALTH_GROUP_READINESS_INCLUDE=readinessState,db
+    MANAGEMENT_HEALTH_MAIL_ENABLED=false
 
-  Everything else in it — db, ping, diskSpace, ssl — is UP. The probes this
-  chart configures deliberately use the /actuator/health/liveness and
-  /actuator/health/readiness GROUPS instead, which contain only the
-  application's lifecycle state and answer 200. Any external monitor you point
-  at this release has to use the same two paths, or it will page you forever
-  about a mail server you never configured.
+  /actuator/health            everything the release actually runs: db,
+                              diskSpace, ping, ssl and the two lifecycle
+                              states. 200 UP on a healthy release. The mail
+                              indicator is NOT in it — see the mail section.
+    → point external monitoring here. This is the only path that also fails on
+      diskSpace or ssl.
 
-WHAT A READY POD DOES NOT PROVE
-  Those two groups report lifecycle state and nothing else — the datasource is
-  not in them. Measured here: with the database scaled away underneath a
-  running pod, both groups kept answering 200, the pod stayed 1/1 Ready and was
-  never restarted. The database is verified exactly once, at boot, by the
-  migration.
+  /actuator/health/readiness  readinessState AND db. Goes DOWN (503) on a
+                              database outage, which takes the pod out of Ready
+                              and stops traffic reaching it.
+    → the readinessProbe.
 
-  Know the resulting picture in advance: during a database outage the pod stays
-  Ready, its endpoint keeps receiving traffic{{ if .Values.ingress.enabled }}, the Ingress keeps routing to
-  it{{ end }}, and users get application errors while Kubernetes reports everything
-  healthy. Nothing in the pod's status or restart count points at the database.
-  Look there first anyway.
+  /actuator/health/liveness   livenessState alone, deliberately WITHOUT db.
+                              Stays 200 through a database outage.
+    → the livenessProbe and the startupProbe.
+
+  The split is the point: a database outage has to keep traffic away, but
+  restarting the pod does not bring the database back. Adding db to the
+  liveness group would turn every outage into a crash loop on top of the
+  outage. Measured on this chart, with the database scaled to zero underneath a
+  running pod: readiness 503, liveness 200, pod 0/1 Ready, restart count 0 —
+  the pod left Ready about 20 seconds after the database went away.
+
+  The other half of that measurement matters just as much: when the database
+  came back, the pod returned to Ready by itself within about five seconds,
+  still with restart count 0. Nothing had to be rolled, deleted or scaled by
+  hand. That is what leaving the liveness group alone buys — the outage is
+  ridden out and then simply over.
+
+  How readiness actually fails is worth knowing before you tune it. The db
+  indicator does not answer 503 quickly — it waits out the JDBC connection
+  timeout first, measured at 30 seconds against a database whose Service has no
+  endpoints at all. readinessProbe.timeoutSeconds is 5, so the kubelet gives up
+  long before the answer arrives and logs
+
+    Readiness probe failed: ... context deadline exceeded
+
+  rather than a 503. The pod leaves Ready either way, and faster; the 503 is
+  what a client with patience sees. Raising timeoutSeconds past the JDBC
+  timeout would swap the timeout for the 503 and delay the reaction — there is
+  no reason to.
+
+WHAT A READY POD DOES AND DOES NOT PROVE
+  Ready now means the pod can reach the database — that is checked on every
+  readiness period, not once at boot. It still does not mean the identity
+  provider is reachable: OIDC is resolved once at startup (see below) and no
+  health indicator watches it afterwards, so a provider that goes down after
+  the rollout leaves a Ready pod on which nobody can log in.
+
+  Ready also proves reachability, not correctness: the db indicator validates a
+  connection, it never looks at the schema. Measured here — a database that came
+  back EMPTY put the pod straight back to Ready with /actuator/health at 200,
+  while not one table existed in it. A restored-but-wrong database reads as
+  healthy, so a probe going green after a database incident is not on its own
+  evidence that the data came back with it.
+
+  A database outage now shows up in the pod's status: READY drops to 0/1 while
+  STATUS stays Running and RESTARTS stays put. That combination — running, not
+  restarting, not Ready — is the signature of a dependency outage rather than
+  of a broken release. The reason is in the endpoint, not in the log:
+
+    kubectl -n {{ .Release.Namespace }} get pod -l app={{ .Release.Name }}-{{ .Chart.Name }}-deployment
+    kubectl -n {{ .Release.Namespace }} port-forward svc/{{ .Release.Name }}-{{ .Chart.Name }}-service 8080:80
+    curl -s localhost:8080/actuator/health
+
+  The body names components only when show-details is on; the chart does not
+  set it, so the aggregate answers {"status":"DOWN","groups":[...]} and the
+  status code carries the message. To see WHICH component is down, add
+
+    urlaubsverwaltung.env:
+      - name: MANAGEMENT_ENDPOINT_HEALTH_SHOW_DETAILS
+        value: always
+
+  and know what that publishes to anyone who can reach the endpoint: the
+  database product, the container's filesystem path and free bytes, the
+  certificate chains, and the stack-trace class of whatever failed.
 
   Note also that there is no shell in this image: "kubectl exec" cannot open
   one, and neither can a debug sidecar sharing the process namespace run the
@@ -241,25 +297,61 @@ MAIL AND THE APPLICATION URL ARE PLACEHOLDERS UNTIL YOU SET THEM
     UV_MAIL_APPLICATIONURL = http://localhost:8080
 
   Nothing in this pod listens on localhost:1025, so no notification mail leaves
-  the release and the mail health indicator stays DOWN. UV_MAIL_APPLICATIONURL
-  is what the app puts into the links inside those mails — it is NOT derived
-  from ingress.domain. Left as it is, every link points at the reader's own
-  machine.
+  the release. UV_MAIL_APPLICATIONURL is what the app puts into the links
+  inside those mails — it is NOT derived from ingress.domain. Left as it is,
+  every link points at the reader's own machine.
 
-urlaubsverwaltung.env IS ADDITIVE ONLY, AND A COLLISION BREAKS THE UPGRADE
+  BECAUSE that wiring is a placeholder, the chart sets
+
+    MANAGEMENT_HEALTH_MAIL_ENABLED=false
+
+  and the mail indicator is gone from /actuator/health. Otherwise the aggregate
+  would answer 503 DOWN on a perfectly healthy release for its whole lifetime,
+  and any monitor pointed at it would page forever about a mail server nobody
+  configured.
+
+  THE PART THAT BITES LATER: the day you configure real mail delivery, switch
+  the indicator back on in the SAME change —
+
+    urlaubsverwaltung.env:
+      - name: MANAGEMENT_HEALTH_MAIL_ENABLED
+        value: "true"
+
+  Left at false it does not report a false alarm, it reports nothing: the
+  endpoint answers 200 while the mail server is unreachable and notifications
+  are silently not delivered. The setting is only honest while there is no mail
+  server to be honest about.
+
+  Note that this variable repeats a name the chart already sets, so per the rule
+  below it only takes effect when it goes in at INSTALL time. Put into an
+  already installed release it does nothing at all, and says so nowhere: helm
+  reports a successful upgrade while the container keeps "false".
+
+urlaubsverwaltung.env IS ADDITIVE ONLY, AND A COLLISION IS LOST ON UPGRADE
   The entries are appended AFTER the chart's own list{{ if $envNames }} (this release appends:
   {{ join ", " $envNames }}){{ end }}. An entry that repeats a name the
   chart already sets does not replace it — it renders the variable twice, and
-  that has a sharp edge: adding such a name to an ALREADY INSTALLED release
-  makes the next upgrade fail in the API server, before anything changes, with
+  what happens to that pair depends on WHEN it first appears:
 
-    UPGRADE FAILED: cannot patch "{{ .Release.Name }}-{{ .Chart.Name }}-deployment" ...
-    The order in patch list ... doesn't match $setElementOrder list
+  AT INSTALL both entries reach the pod and the later one — yours — wins in the
+  container. The API server accepts it with a warning:
 
-  because env entries are merged by name and a duplicate name breaks that
-  ordering. A fresh install with the same values goes through, so the failure
-  looks arbitrary until you know it. Treat the variables above as fixed for the
-  life of a release: to really change one, reinstall.
+    Warning: spec.template.spec.containers[0].env[N]: hides previous definition
+    of "NAME", which may be dropped when using apply
+
+  AT UPGRADE that warning comes true. Kubernetes merges env by name, so the pair
+  collapses back to ONE entry and the surviving value is the CHART's, not yours.
+  Nothing fails: helm prints "Release has been upgraded", the revision counts up,
+  and the override is simply gone. Measured on this chart (helm 3.19.0,
+  Kubernetes 1.35): "helm get manifest" shows the variable twice while the live
+  Deployment shows it once, with the chart's value.
+
+  So the trap is silence, not an error. Treat the variables above as fixed for
+  the life of a release: to really change one, reinstall. And never read a green
+  upgrade as proof that an override took effect — read it off the object:
+
+    kubectl -n {{ .Release.Namespace }} get deployment {{ .Release.Name }}-{{ .Chart.Name }}-deployment \
+      -o jsonpath='{.spec.template.spec.containers[0].env}'
 
   Anything sensitive belongs in valueFrom.secretKeyRef rather than a plain
   value: a value is stored in the pod spec and readable by anyone who can read

urlaubsverwaltung/templates/urlaubsverwaltung.deployment.yaml

@@ -50,6 +50,32 @@ spec:
                 secretKeyRef:
                   name: {{ .Release.Name }}-{{ .Chart.Name }}-db-secret
                   key: password
+            # Health indicators and probe groups
+            #
+            # Spring Boot creates the "liveness" and "readiness" groups by
+            # itself here (measured: /actuator/health answers
+            # {"groups":["liveness","readiness"]} with nothing configured), but
+            # the readiness group ships with readinessState ALONE. Without the
+            # line below a total database outage leaves /actuator/health/readiness
+            # at 200 and the pod 1/1 Ready while every request fails (#218).
+            # Naming the group's members REPLACES the default, so readinessState
+            # has to be repeated. "db" is the datasource indicator's real name,
+            # read off the running endpoint.
+            - name: MANAGEMENT_ENDPOINT_HEALTH_GROUP_READINESS_INCLUDE
+              value: "readinessState,db"
+            # The liveness group is deliberately NOT extended: a database
+            # outage has to keep traffic away, but restarting the pod does not
+            # bring the database back — it would only add a crash loop to the
+            # outage.
+            #
+            # The mail indicator is switched off because the mail wiring below
+            # points at localhost:1025, where nothing listens. Left on, it holds
+            # the aggregate /actuator/health at 503 DOWN for the whole life of a
+            # healthy release (#217). Anyone who later configures real mail
+            # delivery has to switch it back on via urlaubsverwaltung.env, or
+            # the endpoint stays blind to a broken mail server.
+            - name: MANAGEMENT_HEALTH_MAIL_ENABLED
+              value: "false"
             # Mail
             - name: UV_MAIL_FROM
               value: urlaubsverwaltung@example.org

urlaubsverwaltung/values.yaml

@@ -45,6 +45,42 @@ urlaubsverwaltung:
       capabilities:
         drop:
           - ALL
+  # HEALTH INDICATORS — TWO SETTINGS THE CHART FIXES IN THE TEMPLATE
+  #
+  # The chart sets these two as plain environment variables, not as values,
+  # because urlaubsverwaltung.env below already overrides anything:
+  #
+  #   MANAGEMENT_ENDPOINT_HEALTH_GROUP_READINESS_INCLUDE=readinessState,db
+  #     puts the datasource into the readiness group, so a database outage
+  #     takes the pod out of Ready instead of leaving it serving errors (#218).
+  #     The liveness group stays untouched on purpose — an outage must keep
+  #     traffic away, not restart the pod in a loop.
+  #   MANAGEMENT_HEALTH_MAIL_ENABLED=false
+  #     removes the mail indicator, which otherwise pins the aggregate
+  #     /actuator/health at 503 DOWN forever, because the chart's mail wiring
+  #     points at localhost:1025 where nothing listens (#217).
+  #
+  # THE CONSEQUENCE TO KNOW BEFORE IT BITES: whoever later configures real mail
+  # delivery (SPRING_MAIL_HOST/SPRING_MAIL_PORT via urlaubsverwaltung.env) MUST
+  # switch the indicator back on in the same change:
+  #
+  #   env:
+  #     - name: MANAGEMENT_HEALTH_MAIL_ENABLED
+  #       value: "true"
+  #
+  # Left at false, /actuator/health keeps answering 200 while the configured
+  # mail server is unreachable and no notification mail is delivered. The
+  # setting trades a permanently false alarm for silence, and silence is only
+  # correct as long as there is nothing to report about.
+  #
+  # That override repeats a name the chart already sets, and env is additive, so
+  # it renders the variable twice. At INSTALL time both entries reach the pod and
+  # the later one — yours — wins in the container. Added to an ALREADY INSTALLED
+  # release it is lost instead: Kubernetes merges env by name, the pair collapses
+  # to the chart's own entry, and helm still reports a successful upgrade. The
+  # override does not fail loudly, it fails silently. See the "env IS ADDITIVE
+  # ONLY" section of NOTES.txt — the switch has to go in at install time, and
+  # after any such change the result belongs verified on the live Deployment.
   livenessProbe:
     httpGet:
       port: 8080