From c983ae10cd05392586c9d54a3f12009b46ca31d2 Mon Sep 17 00:00:00 2001 From: Justin Klein Date: Sun, 4 Oct 2026 17:16:36 +0000 Subject: [PATCH] keycloak: the readiness check that is gated behind the metrics option Found while verifying the CI result rather than from the plan, and it changes a default: Keycloak's DATABASE health check is the one row of upstream's check table whose "Requires Metrics" column says yes. With metrics-enabled=false - the chart's default in its first draft - /health/ready answers 200 with the database gone, and the pod keeps taking logins it cannot serve. That is the urlaubsverwaltung shape a second time, and the generalisation is worth more than the fix: the dependency was not missing from a health GROUP this time, it was conditional on an option named for observability, which is a place nobody looks when reviewing a probe. So the rule grows past health groups - find the list of checks the endpoint performs, and find out what each one is conditional on. "This path includes the database" is a claim about configuration, not about the path. keycloak.md: metrics default is now true with that reasoning, a new trap section for it, and the measured install-test figure (71.8s from helm install to both pods Ready, 0 restarts, Gitea PR #87). Also corrected an overclaim of my own in the same page: two replicas both reaching Ready does NOT prove that jdbc-ping found the peer. Readiness reports one node's own health, so two nodes that never met are both Ready and both alone. The install-test rules out the louder half (a replica that cannot start or cannot share the database); the Infinispan view still has to be read out of the log, which is what step 2 of the first-install checklist is for. --- Lessons-Learned.md | 2 ++ keycloak.md | 31 +++++++++++++++++++++++++------ 2 files changed, 27 insertions(+), 6 deletions(-) diff --git a/Lessons-Learned.md b/Lessons-Learned.md index e1d36a7..ee16e70 100644 --- a/Lessons-Learned.md +++ b/Lessons-Learned.md @@ -18,6 +18,8 @@ Spring Boot's `readiness` group contains `readinessState` **alone**. The paths w **Now:** whatever aggregates behind a probe path — Spring health groups, a framework-assembled `/healthz` — is read off the **running** endpoint, not off the path name. The chart names the members explicitly, so the composition lives in the template rather than in an upstream default. See [§12.3](Chart-Style-Guide#12-values-that-must-not-lie). +**It recurred, and the second time it was caught before shipping — by reading the check list rather than the path.** [keycloak](keycloak) has three separate native endpoints, which looks like the problem being solved for you: `/health/live` without a database check, `/health/ready` with one. Except that upstream's table of available checks has a **"Requires Metrics"** column, and the *database* check is the single row that says yes — so with `metrics-enabled=false`, the chart's own default in its first draft, `/health/ready` would have answered 200 with the database gone. The dependency was not missing from a health *group* this time; it was gated behind an option named for **observability**, which is a place nobody looks when reviewing a probe. The chart now defaults metrics on and argues for it as a readiness setting. **So the rule generalises past health groups: find the list of checks the endpoint actually performs, and find out what each one is conditional on.** "This path includes the database" is a claim about configuration, not about the path. + ## The value that meant its own opposite `satisfactory` shipped `game.seasonalEvents`, passed straight into the image's `DISABLESEASONALEVENTS`. The default `true` switched the events **off** — in every install, for the chart's entire life — while the schema described the key as "true means events are enabled". diff --git a/keycloak.md b/keycloak.md index 68f7654..5150ba8 100644 --- a/keycloak.md +++ b/keycloak.md @@ -15,7 +15,7 @@ What it costs is the thing to know before the first upgrade: **Keycloak supports | Ingress | **two**: the public one and the admin hostname (switchable off). The admin one is **not** an access control — see [the first trap](#the-admin-hostname-is-not-an-access-control) | | External dependencies | an external PostgreSQL. Nothing else | | Secrets | 2 × `OnePasswordItem`, four fields in total | -| Network | ClusterIP Service on 8080 (`http`) and a **separate** ClusterIP Service on 9000 (`management`). The management port serves `/health/*` and optionally `/metrics` **unauthenticated** and is in no Ingress. JGroups uses 7800 pod-to-pod; no NetworkPolicy yet | +| Network | ClusterIP Service on 8080 (`http`) and a **separate** ClusterIP Service on 9000 (`management`). The management port serves `/health/*` and `/metrics` **unauthenticated** and is in no Ingress. JGroups uses 7800 pod-to-pod; no NetworkPolicy yet | | Disruption | `PodDisruptionBudget`, `minAvailable: 1` — the **first** in this repo, and now a convention ([style guide §11](Chart-Style-Guide#11-scaling)) | | Install-test | `ci/keycloak/` — fixtures + test values, **with two replicas** and a throwaway PostgreSQL | @@ -86,14 +86,14 @@ Host, port and database name are deliberately **not** in the vault — they are Ready means `/health/ready` answered 200, which means the database is reachable and the schema is in place. The first start is the slow one: it creates the whole schema, and with two replicas the second waits on the first's migration lock. If a pod sits in `Init`, read the init containers first — see [Why two init containers](#why-this-chart-has-two-init-containers). -2. **Check that the cluster really formed.** This is what the second replica is for, and a silent failure here leaves two independent islands that each believe they are the cluster: +2. **Check that the cluster really formed. No probe replaces this step and neither does the install-test.** A node that never found its peer is Ready, healthy and *alone* — readiness reports this node's own health — and two such nodes are two islands that each believe they are the cluster. ```bash kubectl -n keycloak logs deployment/keycloak-keycloak-deployment \ | grep -i "ISPN000094\|view accepted\|members" ``` - Look for a view with **two** members. One member per pod means `jdbc-ping` did not find the peer. + Look for a view with **two** members. One member per pod means `jdbc-ping` did not find the peer: check that both pods can reach each other on port 7800 and that they really share one database. 3. **Log in** at `https:///admin` with the bootstrap credentials from the vault item. @@ -199,7 +199,7 @@ Three practical consequences: | `keycloak.db.pool.initialSize` / `.minSize` | int | `2` / `2` | No upstream default. A few warm connections remove the first-request latency after an idle period. | | `keycloak.db.pool.maxSize` | int | `20` | **Deliberately not upstream's `100`**, and the sizes are **per replica**: two replicas at 100 would ask a default PostgreSQL (`max_connections = 100`) for twice what it has, and the failure arrives under load as *"too many clients already"* rather than at install time. Raise it together with the server's `max_connections`, and multiply by `keycloak.replicas` when you do. | | `keycloak.bootstrapAdmin.enabled` | bool | `true` | The temporary bootstrap administrator. **Switch it off once a permanent administrator exists** — see step 4 above. | -| `keycloak.metrics.enabled` | bool | `false` | **A build option.** Off because of the *port*, not the data: the management interface is unauthenticated here, and metrics add realm and client names to a surface nothing authenticates. Switch it on together with whatever scrapes it. | +| `keycloak.metrics.enabled` | bool | `true` | **A build option, and on for the *readiness probe* rather than for the metrics** — see [the readiness trap](#readiness-only-checks-the-database-while-metrics-are-enabled). The price: `/metrics` then answers on the management port, unauthenticated, carrying realm names, client ids and login counters. That port is ClusterIP-only and in no Ingress. Switching this off is a decision about whether readiness can fail. | | `keycloak.features` | list | `[]` | Extra feature flags (`--features`). **A build option.** A list and not a comma-separated string, so two entries cannot arrive as one unknown feature name — which Keycloak rejects at *build* time, inside an init container. | | `keycloak.cache.clusterName` | string | `null` | `--cache-embedded-cluster-name`; `null` means Keycloak's default `ISPN`. **Read [the trap](#a-shared-database-merges-two-clusters) before setting it.** | | `keycloak.ports.http` | int | `8080` | | @@ -263,6 +263,23 @@ Port 9000 carries `/health`, `/health/live`, `/health/ready`, `/health/started` Upstream's own alternative, `legacy-observability-interface=true`, moves health back onto the serving port where the Ingress already is. It is explicitly *"not recommended for security reasons"* and this chart does not offer it. +### Readiness only checks the database while metrics are enabled + +`keycloak.metrics.enabled` reads like an observability switch and is not only one. Upstream's table of available health checks carries a **"Requires Metrics"** column, and the row that says **yes** is the one that matters here: + +| Check | Description | Requires metrics | +|---|---|---| +| Database | "Returns the status of the database connection pool" | **yes** | +| Cluster | "Returns the status of the cluster (network partitions)" | no | +| Graceful shutdown | returns DOWN once pre-shutdown has started | no | +| Keycloak Initialized | status of server initialization | no | + +So with `metrics-enabled=false`, `/health/ready` answers **200 with the database gone**, and the pod keeps taking logins it cannot serve. + +That is exactly the shape [the readiness probe that could not fail](Lessons-Learned#the-readiness-probe-that-could-not-fail) cost this repo on `urlaubsverwaltung`: the probe *path* was right and the aggregate behind it contained nothing external. The difference is only where it hides — there in a Spring health group, here behind an option named for metrics. Which is why **this chart defaults `keycloak.metrics.enabled` to `true`, and argues for it as a readiness setting rather than a scraping one.** + +The price is real and is why the value still exists: `/metrics` then answers on the management port, unauthenticated, carrying realm names, client ids and login counters. That port is ClusterIP-only and in no Ingress, so the exposure is in-cluster — see [the trap below](#the-management-port-is-unauthenticated). **Switching it off is a decision about whether the readiness probe can fail at all**, and nothing else in this chart replaces it. + ### The three probes use three different paths on purpose | Probe | Path | Contains a database check? | @@ -315,9 +332,11 @@ This chart renders **no** NetworkPolicy. If one is ever added, it has to allow 7 `ci/keycloak/` carries `test-values.yaml`, `fixtures.yaml` (throwaway PostgreSQL plus the two Secrets the operator would materialize) and `settings.env` (`INSTALL_TIMEOUT=900`). -**The install-test runs with two replicas**, which is the single most valuable line in that fixture: the script waits for the Deployment's rollout, and a rollout is complete only when **both** pods are Ready — so cluster formation through `jdbc-ping` is inside the assertion rather than beside it. A one-replica test would be green without ever showing whether the second node is found, which is the mistake [§CI](CI-CD) was taught once already. +**The install-test runs with two replicas**, and the rollout is complete only when **both** pods are Ready — so a second replica that cannot start, deadlocks on the schema migration lock or fights the first over the database fails the run. Measured in Gitea PR #87: **71.8 seconds** from `helm install` to both pods Ready, 0 restarts. -It also closes three things a render cannot: that the two init containers work at all, that `readOnlyRootFilesystem: true` holds for the serving container with emptyDirs only at the three paths it needs, and that `start --optimized` accepts the pre-built installation instead of re-augmenting onto a read-only path. `metrics.enabled: true` in the fixture so the build container must produce an installation differing from the published image in **two** options rather than one — with metrics off, a build that silently did nothing would look identical. +**What that does *not* prove is that `jdbc-ping` found the peer**, and reading it that way is the mistake this paragraph exists to prevent. Readiness reports *this* node's health, so a node that never found its peer is Ready, healthy and alone — and two such nodes are two islands that each believe they are the cluster. The Infinispan view has to be read out of the log, which is what step 2 of the first-install checklist is for. So the test rules out the louder half and leaves the quiet half to a human. + +It also closes three things a render cannot: that the two init containers work at all, that `readOnlyRootFilesystem: true` holds for the serving container with emptyDirs only at the three paths it needs, and that `start --optimized` accepts the pre-built installation instead of re-augmenting onto a read-only path. And because `health-enabled` and `metrics-enabled` are build options, the fact that all three probe paths answer on port 9000 **is** the proof that the build container really did its work — a build that silently did nothing would leave them 404. **What CI cannot prove**: that `hostname-admin` serves the console and its resources cleanly on the second hostname. No ingress controller answers in the test cluster and nothing drives a browser. What it does check is that both Ingress objects are accepted, that the Service port they name exists, and that Keycloak accepts both hostname values as full URLs at start.