fix: Make the gateway certificate alerts able to fire - #437
Conversation
Every metric in internal/controller registered against prometheus.DefaultRegisterer, which the manager never serves. The manager exposes controller-runtime's own registry at /metrics, so none of these families has ever reached Prometheus. The cost is not only absence. Two alerts shipped in this repo's own telemetry bundle, GatewayListenerCertUnusable and GatewayListenerCertExpiringSoon, select on nso_gateway_listener_cert_* and are deployed to production today, where they can never fire. A customer's HTTPS listener can be withheld for an unusable certificate with no signal at all. Worse, three plain gauges do reach production, and they lie. The extension server serves DefaultRegisterer itself, and it links this package, so nso_location_source_total, nso_location_published_total and nso_location_retained_total are exported from every edge cell at a constant 0. The location publisher runs in the manager, not there, so the values are placeholders that read as a real measurement. After this change they are exported once, by the process that sets them. Key changes: - Register every internal/controller metric with promauto.With(ctrlmetrics.Registry) so the manager's /metrics serves them - Correct the package comment, which asserted the opposite and is presumably why this went unnoticed - Add TestControllerMetricsAreServedByTheManager, which gathers from the registry the manager actually serves and fails on the pre-fix registration internal/extensionserver keeps its own metrics on DefaultRegisterer, which it registers and serves itself. Fixes #359 Claude-Session: https://claude.ai/code/session_011XoSfWG6XKn8ckkRAAGiCP
GatewayReconciler is multicluster: upstream gateways live in per-project control planes, and 129 of 131 production gateways sit in namespace "default". Labelling the five gateway certificate and programmed metrics by namespace+name alone collapses two projects that happen to share a gateway name onto one series, and clearListenerCertMetrics then deletes the other project's series on every reconcile via DeletePartialMatch. The alerts built on these series could silently stop firing for the collided project. Thread the upstream cluster name through as a project label on gatewayProgrammedTotal, gatewayListenerCertWithheld, gatewayListenerCertGatingTotal, gatewayListenerCertExpiryTime and gatewayListenerCertManaged, matching the existing metricLabelProject convention used by missingAllocationsTotal. Surface the label in the GatewayListenerCertUnusable and GatewayListenerCertExpiringSoon alert annotations so on-call can see which project a firing alert belongs to. Claude-Session: https://claude.ai/code/session_011XoSfWG6XKn8ckkRAAGiCP
The registry-coverage test named every metric by hand, so a metric added later with a bare promauto.New* call (registering against prometheus.DefaultRegisterer instead of ctrlmetrics.Registry) stayed invisible to it. Add a second test that gathers prometheus.DefaultGatherer directly and fails on any "nso_" family that isn't "nso_extension_", catching that mistake regardless of which metric it is. Also drop the three location metrics from the hand-maintained name list: draft PR #413 deletes them, and naming them here would make that PR fail to build against this branch. Claude-Session: https://claude.ai/code/session_011XoSfWG6XKn8ckkRAAGiCP
The package comment said prometheus.DefaultRegisterer is also the registry controller-runtime exposes via sigs.k8s.io/controller-runtime/pkg/metrics. It is not: the manager serves ctrlmetrics.Registry, and the extension server is a separate binary that serves DefaultRegisterer itself via promhttp.Handler(). A metric registered here was never invisible because of that false claim, but the claim is exactly what this branch's controller-side fix exists to correct, so leaving it uncorrected here would contradict it. Claude-Session: https://claude.ai/code/session_011XoSfWG6XKn8ckkRAAGiCP
locationSourceTotal, locationPublishedTotal and locationRetainedTotal are plain gauges set only by the leader-elected location publisher, while production runs 3 replicas of the manager. A non-leader exports each of them at a constant 0 from the registry the manager serves, so sum() or a bare selector against these metrics undercounts by however many non-leader replicas are running. Say so in the Help text so the gauge's query semantics travel with it into Grafana and any new PromQL, without restructuring the publisher. Claude-Session: https://claude.ai/code/session_011XoSfWG6XKn8ckkRAAGiCP
ecv
changed the title
|
Parking this. It is finished and reviewed, but it adds alerts, and the current goal is a channel at zero before any new alert is created. Releasing it turns two certificate alerts from never-firing to roughly ten to twenty firing at once, all long-standing, almost all demo, lab or test projects. Nothing decays while it waits: the bug is a registration line, the fix does not rot, and no enterprise workload is dark today. Unparking needs two things first. The exclusion for gateways already dark, https://github.com/datum-cloud/infra/issues/4997, so the release does not arrive as a page wave. And staging back on main, https://github.com/datum-cloud/infra/issues/4998, so there is somewhere to soak it. Both are recorded as blockers on #359. Marking as draft so it is not merged by accident. |
|
Correcting the severity I gave earlier in this thread. Both certificate alerts carry warning severity, and the Alertmanager configuration has no alertname matcher for either, so they route to the warning channel rather than to on-call. The rollout still puts ten to twenty entries in that channel at once, which is what matters against a zero-alert goal, but it is channel noise and not a page wave. The issue body is corrected. |
|
Unpark conditions, updated. The alert-side guard is no longer one of them. An attempt to gate the certificate alert on recent breakage is closed, https://github.com/datum-cloud/infra/pull/5003, because no standing gate can hold back listeners that were already dark without also muting listeners that are dark from creation. The operator records the managed and withheld gauges in the same loop iteration, so a born-broken listener never accumulates a healthy sample, and born-broken is the main shape this alert reports. So the wave gets a one-time answer rather than a rule change. Before releasing this:
Two review findings on this branch also bear on the release. The metrics carry a project label as of the fixes here, so the silence should be scoped by project rather than by gateway name, which collides across projects. And the location gauges will move from sixteen edge cells to the control plane replicas, where the non-leaders report zero, so read them with max(). |
1 participant
Summary
Alerts that tell on-call when a customer's HTTPS listener has gone dark, or its certificate is about to expire, could never fire. The certificate health measurements they depend on were recorded somewhere the alerting system never reads, so the alerts had nothing to evaluate.
A related set of measurements had the opposite problem: it reported a flat zero from every edge location instead of a real value from the one place that actually measures it. Both are fixed, and the underlying measurements now separate customers who happen to reuse the same gateway name, so one customer's certificate status can no longer overwrite another's.
Test plan
Fixes #359
Related to https://github.com/datum-cloud/infra/issues/4005
https://claude.ai/code/session_011XoSfWG6XKn8ckkRAAGiCP