From b310554dfd60c5308297cc409488f7435eca7cb2 Mon Sep 17 00:00:00 2001 From: atlas-bot Date: Sat, 26 Sep 2026 00:32:26 +0000 Subject: [PATCH] feat(monitoring): alert when an ExternalSecret stops syncing (REN-103) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Merge B of REN-103. #994 made the series exist; this is the rule built on them. Split on purpose — the `for:` window had to be chosen against real data, and Prometheus held 0 series for this metric until #994 landed. Two corrections to what REN-100 specified, both found only once there was data or source to read: * `exported_namespace`, not `namespace`. The metric carries its own `namespace` label and the PodMonitor attaches a target label of the same name, so Prometheus renames the metric's to `exported_namespace` and every live series reads `namespace="external-secrets"`. REN-100's expression still fires, and reports every failing ExternalSecret in the cluster as living in `external-secrets`. Found by Argus against the live data. * `max`, not `sum`. Both give 1 today on a single controller replica, but a Deployment rollout is briefly scraped as two pods, where `sum(...) == 1` evaluates to 2 and silently stops matching. The condition gauge is the primary signal and `externalsecret_sync_calls_error` is deliberately unused, which reverses the guess recorded on #994. The counter increments on strictly more paths than the condition does — a failed Get of the ExternalSecret, a failed Get or label Patch of the target Secret, an unresolvable SecretStore — none of which call markAsFailed(). It is the noisier signal, not the quieter one. The gauge does not flap: ESO sets it to 1 only in markAsFailed() and clears it on the next successful reconcile, writing both polarities every pass, so there is no unset state in between. `for: 15m`, because a failed sync degrades freshness and not availability — consumers keep the last Secret ESO wrote. Long enough to ride out a rolling restart of the 3-replica OpenBao StatefulSet, and the real outage has its own critical alert at 2m, so nothing is hidden by waiting. Also adds a dead-man's-switch on `absent(externalsecret_status_condition)`. Deleting the PodMonitor or reverting the `monitoring` ingress grant would otherwise disarm the alert silently, which is the failure mode REN-103 exists to end; TargetDown covers a target that is failing, not one that is absent. Verified with promtool 2.53.2: `check rules` passes, and the committed `-test.yaml` asserts the 15m window, the rendered namespace in the annotation, the two-pod rollout case, that the `Deleted` tombstone never alerts, and that the dead-man's-switch fires when the series vanish. Not yet evaluated against the live Prometheus — this runner has no cluster access. Co-Authored-By: Claude Opus 5 --- .../apps/monitoring-rules/kustomization.yaml | 4 + .../prometheusrule-external-secrets-test.yaml | 125 +++++++++++++++ .../prometheusrule-external-secrets.yaml | 144 ++++++++++++++++++ 3 files changed, 273 insertions(+) create mode 100644 kubernetes/apps/monitoring-rules/prometheusrule-external-secrets-test.yaml create mode 100644 kubernetes/apps/monitoring-rules/prometheusrule-external-secrets.yaml diff --git a/kubernetes/apps/monitoring-rules/kustomization.yaml b/kubernetes/apps/monitoring-rules/kustomization.yaml index f7e01483..bdc470de 100644 --- a/kubernetes/apps/monitoring-rules/kustomization.yaml +++ b/kubernetes/apps/monitoring-rules/kustomization.yaml @@ -52,6 +52,10 @@ resources: # infrastructure/network-policies/external-secrets-ns.yaml in the same change, # or it scrapes nothing. See the file. - external-secrets-podmonitor.yaml + # The alert built on what that PodMonitor collects. Split from it on purpose: + # the `for:` window had to be chosen against real series, and Prometheus held + # none for this metric until the scrape landed. + - prometheusrule-external-secrets.yaml # Flux's own control plane. The controllers were never scraped, so nothing # about Flux's behaviour was queryable. Scrape only: per-object readiness is # not on this endpoint at the pinned controller versions and needs diff --git a/kubernetes/apps/monitoring-rules/prometheusrule-external-secrets-test.yaml b/kubernetes/apps/monitoring-rules/prometheusrule-external-secrets-test.yaml new file mode 100644 index 00000000..d8eb77f5 --- /dev/null +++ b/kubernetes/apps/monitoring-rules/prometheusrule-external-secrets-test.yaml @@ -0,0 +1,125 @@ +# promtool test rules for prometheusrule-external-secrets.yaml. +# +# NOT A KUBERNETES MANIFEST, despite living in kubernetes/. Same placement +# argument as prometheusrule-ceph-test.yaml: a test kept in a separate tests/ +# tree is one nobody edits in the same diff as the rule. It is not listed in +# kustomization.yaml, so kustomize never builds it and Flux never applies it. +# +# INERT ON master UNTIL REN-96 LANDS. The `promql-test` step of +# .woodpecker/ci-manifests-test.yml, and the scripts/ci/ extractor whose naming +# convention `rule_files:` below depends on, are still on an unmerged branch. +# Until then this is run by hand — the output recorded on the pull request came +# from promtool 2.53.2 against the spec extracted with: +# +# awk 'f{sub(/^ /,""); print} /^spec:$/{f=1}' \ +# kubernetes/apps/monitoring-rules/prometheusrule-external-secrets.yaml \ +# > prometheusrule-external-secrets.rules.yaml +# promtool test rules kubernetes/apps/monitoring-rules/prometheusrule-external-secrets-test.yaml +# +# WHAT IT IS FOR. Every claim in the rule file that a reviewer would otherwise +# have to take on trust is asserted here instead: that the tombstone series does +# not alert, that `for: 15m` really is 15m, that the annotation names the +# ExternalSecret's own namespace and not the controller's, and that `max` +# survives a two-pod scrape where `sum` does not. +# +# THE REJECTED FORM, for the record. REN-100 and ESO's shipped dashboard both +# use `sum(...) by (namespace, name) == 1`. Run against the identical input +# series below, that expression produces exactly one alert, +# {namespace="external-secrets", name="gitea-db"} — the wrong namespace on the +# one it catches, and nothing at all for monitoring/loki-basic-auth, whose two +# scraped pods sum to 2 and stop matching `== 1`. Both defects, one expression. +rule_files: + - prometheusrule-external-secrets.rules.yaml + +evaluation_interval: 1m + +tests: + # Series shapes copied from the live cluster after #994 landed: the metric's + # own `namespace` has already been shadowed to `exported_namespace` by the + # PodMonitor's target label, and `namespace` is the controller pod's. + - interval: 1m + input_series: + # A healthy ExternalSecret. ESO writes BOTH polarities of the Ready + # condition on every reconcile, the inactive one at 0 — which is why the + # rule needs `== 1` and not a bare selector. + - series: 'externalsecret_status_condition{condition="Ready",status="True",name="uptime-robot-api-key",exported_namespace="uptime-monitor",namespace="external-secrets",job="monitoring/external-secrets",pod="external-secrets-56c649799b-t65gf"}' + values: '1+0x60' + - series: 'externalsecret_status_condition{condition="Ready",status="False",name="uptime-robot-api-key",exported_namespace="uptime-monitor",namespace="external-secrets",job="monitoring/external-secrets",pod="external-secrets-56c649799b-t65gf"}' + values: '0+0x60' + + # Starts failing at t=10m and stays failed. + - series: 'externalsecret_status_condition{condition="Ready",status="False",name="gitea-db",exported_namespace="gitea",namespace="external-secrets",job="monitoring/external-secrets",pod="external-secrets-56c649799b-t65gf"}' + values: '0+0x9 1+0x50' + - series: 'externalsecret_status_condition{condition="Ready",status="True",name="gitea-db",exported_namespace="gitea",namespace="external-secrets",job="monitoring/external-secrets",pod="external-secrets-56c649799b-t65gf"}' + values: '1+0x9 0+0x50' + + # Tombstone left behind for an ExternalSecret that no longer exists. Held + # at 1 forever until the controller restarts, so a rule that matched + # `condition!="Ready"` or ignored `status` would page on it every day. + - series: 'externalsecret_status_condition{condition="Deleted",status="False",name="wiki-index-basic-auth",exported_namespace="ai",namespace="external-secrets",job="monitoring/external-secrets",pod="external-secrets-56c649799b-t65gf"}' + values: '1+0x60' + + # The same failing ExternalSecret reported by TWO controller pods, as + # happens for the seconds of a Deployment rollout when Prometheus still + # has the old target and already has the new one. + - series: 'externalsecret_status_condition{condition="Ready",status="False",name="loki-basic-auth",exported_namespace="monitoring",namespace="external-secrets",job="monitoring/external-secrets",pod="external-secrets-56c649799b-t65gf"}' + values: '0+0x9 1+0x50' + - series: 'externalsecret_status_condition{condition="Ready",status="False",name="loki-basic-auth",exported_namespace="monitoring",namespace="external-secrets",job="monitoring/external-secrets",pod="external-secrets-7d4f9c6b88-qq2xv"}' + values: '0+0x9 1+0x50' + + alert_rule_test: + # 5m into the failure: pending, not firing. Asserts the window is real — + # a `for:` accidentally set to 0 passes every other case here and fails + # only this one. + - eval_time: 15m + alertname: ExternalSecretSyncFailing + exp_alerts: [] + + # 20m into the failure. Both failing ExternalSecrets fire, ONCE each; the + # healthy one and the tombstone stay silent. The annotations are asserted + # in full because the whole point of REN-103's label finding is that the + # text names `gitea`, not `external-secrets`. + - eval_time: 30m + alertname: ExternalSecretSyncFailing + exp_alerts: + - exp_labels: + severity: warning + exported_namespace: gitea + name: gitea-db + exp_annotations: + summary: "ExternalSecret gitea/gitea-db is not syncing" + description: "ESO has been unable to sync this ExternalSecret for 15 minutes. The Secret it manages still holds its last successfully synced value, so consumers keep working — what breaks is that a rotation at the provider no longer propagates, which surfaces much later as a credential that stopped being accepted. Get the reason with `kubectl -n gitea get externalsecret gitea-db -o jsonpath='{.status.conditions}'`; the common causes are a key that was renamed or removed in OpenBao, a ClusterSecretStore that cannot authenticate, and a target Secret owned by something else." + - exp_labels: + severity: warning + exported_namespace: monitoring + name: loki-basic-auth + exp_annotations: + summary: "ExternalSecret monitoring/loki-basic-auth is not syncing" + description: "ESO has been unable to sync this ExternalSecret for 15 minutes. The Secret it manages still holds its last successfully synced value, so consumers keep working — what breaks is that a rotation at the provider no longer propagates, which surfaces much later as a credential that stopped being accepted. Get the reason with `kubectl -n monitoring get externalsecret loki-basic-auth -o jsonpath='{.status.conditions}'`; the common causes are a key that was renamed or removed in OpenBao, a ClusterSecretStore that cannot authenticate, and a target Secret owned by something else." + + # Metrics are present, so the dead-man's-switch stays quiet. + - eval_time: 30m + alertname: ExternalSecretMetricsMissing + exp_alerts: [] + + # The scrape disappears entirely: PodMonitor deleted, selector broken by a + # chart bump, or the `monitoring` ingress grant reverted out of + # external-secrets-ns.yaml. ExternalSecretSyncFailing cannot fire on series + # that do not exist — this is the case that made REN-103 necessary, so it is + # the one the dead-man's-switch has to cover. + - interval: 1m + input_series: + - series: 'up{job="monitoring/external-secrets"}' + values: '0+0x60' + alert_rule_test: + - eval_time: 30m + alertname: ExternalSecretSyncFailing + exp_alerts: [] + - eval_time: 30m + alertname: ExternalSecretMetricsMissing + exp_alerts: + - exp_labels: + severity: warning + exp_annotations: + summary: "No ExternalSecret metrics are reaching Prometheus" + description: "`externalsecret_status_condition` has no series. Either the ESO controller is gone or its scrape stopped being collected, and ExternalSecretSyncFailing is blind for as long as this is true. Check the PodMonitor `monitoring/external-secrets` still selects the controller pods, and that external-secrets-ns.yaml still admits `monitoring` on TCP/8080." diff --git a/kubernetes/apps/monitoring-rules/prometheusrule-external-secrets.yaml b/kubernetes/apps/monitoring-rules/prometheusrule-external-secrets.yaml new file mode 100644 index 00000000..16b8098c --- /dev/null +++ b/kubernetes/apps/monitoring-rules/prometheusrule-external-secrets.yaml @@ -0,0 +1,144 @@ +# Alert when an ExternalSecret stops syncing. +# +# This is the second half of REN-103. The first half (#994) added the PodMonitor +# and the `monitoring` ingress grant that make the series exist at all; before +# that, `externalsecret_status_condition` held 0 series and a key that silently +# stopped refreshing was invisible to Prometheus, to Alertmanager and to anyone +# looking at a dashboard. The alert is split out because its `for:` window had to +# be chosen against real data rather than guessed. +# +# `exported_namespace`, NOT `namespace`. The metric carries its own `namespace` +# label (the ExternalSecret's), and the PodMonitor attaches a target label of the +# same name (the controller pod's, always `external-secrets`). Prometheus resolves +# that collision by renaming the metric's label to `exported_namespace`, so on +# every one of the ~105 live series `namespace="external-secrets"`. Grouping by +# `namespace` would therefore still fire correctly and still be useless: every +# failing ExternalSecret in the cluster would be reported as living in +# `external-secrets`. ESO's own shipped Grafana dashboard does group by +# `namespace` — that is right for the chart's ServiceMonitor, which scrapes ESO +# through its own Service and injects no conflicting target label, and wrong here. +# This is a general trap for the PodMonitor-instead-of-ServiceMonitor trade this +# repo keeps making: any metric carrying its own `namespace`, `pod` or `container` +# label gets it shadowed. +# +# `honorLabels: true` on the PodMonitor endpoint would keep the clean label name +# instead, and is deliberately not used: it drops the target labels, so nothing +# records which controller replica reported, and it would make this the only +# monitor in the directory doing so. +# +# A consequence worth knowing rather than fixing: aggregating by +# `exported_namespace` leaves these alerts with no `namespace` label at all, and +# Alertmanager's route groups on ['alertname', 'cluster', 'namespace']. So every +# failing ExternalSecret arrives in ONE email listing all of them, each named by +# its own annotation, instead of one email per namespace. For a fleet this size +# that is the better shape. +apiVersion: monitoring.coreos.com/v1 +kind: PrometheusRule +metadata: + name: external-secrets + namespace: monitoring + labels: + # kube-prometheus-stack's ruleSelector matches on this label; without it the + # rule is created but never loaded into Prometheus. + release: kube-prometheus-stack +spec: + groups: + - name: external-secrets.rules + rules: + # `== 1` IS LOAD-BEARING, not defensive. ESO writes BOTH polarities of the + # Ready condition on every reconcile — the true one at 1 and the opposite + # at 0 (esmetrics.UpdateExternalSecretCondition) — so + # `{condition="Ready",status="False"}` is present with value 0 for every + # healthy ExternalSecret. A bare selector would match all 52 of them. + # + # `max`, not the `sum` the vendor dashboard uses. Both give 1 today, + # because ESO runs a single controller replica. During a Deployment + # rollout Prometheus can briefly scrape the old and the new pod at once, + # and `sum(...) == 1` would then evaluate to 2 and silently STOP matching + # — the alert would resolve itself in the middle of a failure. `max` is 0 + # or 1 whatever the replica count. + # + # The gauge, not `externalsecret_sync_calls_error`. The counter was the + # other candidate, on the theory that a counter cannot flap the way a + # condition gauge might. Reading the controller says the opposite on both + # halves: the counter increments on strictly MORE paths than the condition + # does — a failed Get of the ExternalSecret itself, a failed Get or label + # Patch of the target Secret, an unresolvable SecretStore — and none of + # those call markAsFailed(), so none of them set Ready=False. A cache blip + # or an apiserver hiccup moves the counter and nothing else. It is the + # noisier signal, not the quieter one. And the gauge does + # not flap, because it is set to 1 only by markAsFailed() and cleared by + # the next successful reconcile. There is no unset state in between. + - alert: ExternalSecretSyncFailing + expr: >- + max by (exported_namespace, name) ( + externalsecret_status_condition{condition="Ready",status="False"} + ) == 1 + # 15m, and the reason it can be this relaxed is that a failed sync is + # not an outage: consumers keep using the last Secret ESO wrote, so what + # degrades is freshness, not availability. Nothing here needs catching + # inside a minute. + # + # It needs to be at least this long because the provider-error path + # returns the error to controller-runtime, which retries with + # exponential backoff rather than waiting out `refreshInterval`. A + # momentary OpenBao blip is therefore cleared within seconds and never + # reaches 15m, while a rolling restart of the 3-replica OpenBao + # StatefulSet is ridden out entirely — and that outage has its own + # alert (OpenBaoQuorumLost, critical at 2m), so nothing is hidden by + # waiting. 15m of continuous Ready=False means repeated failure. + for: 15m + labels: + severity: warning + annotations: + summary: "ExternalSecret {{ $labels.exported_namespace }}/{{ $labels.name }} is not syncing" + description: >- + ESO has been unable to sync this ExternalSecret for 15 minutes. The + Secret it manages still holds its last successfully synced value, so + consumers keep working — what breaks is that a rotation at the + provider no longer propagates, which surfaces much later as a + credential that stopped being accepted. Get the reason with + `kubectl -n {{ $labels.exported_namespace }} get externalsecret + {{ $labels.name }} -o jsonpath='{.status.conditions}'`; the common + causes are a key that was renamed or removed in OpenBao, a + ClusterSecretStore that cannot authenticate, and a target Secret + owned by something else. + + # Dead-man's-switch, mirroring ExternalDNSMetricsMissing and + # OpenBaoMetricsMissing. The alert above can only fire on series that + # exist, so deleting the PodMonitor, reverting the `monitoring` ingress + # grant in infrastructure/network-policies/external-secrets-ns.yaml, or + # bumping the ESO chart onto a metrics port that no longer matches would + # all disarm it silently — which is the exact failure mode REN-103 exists + # to end. TargetDown does not cover this: it reports a target that exists + # and is failing, not the absence of a target. + - alert: ExternalSecretMetricsMissing + expr: absent(externalsecret_status_condition) + for: 15m + labels: + severity: warning + annotations: + summary: "No ExternalSecret metrics are reaching Prometheus" + description: >- + `externalsecret_status_condition` has no series. Either the ESO + controller is gone or its scrape stopped being collected, and + ExternalSecretSyncFailing is blind for as long as this is true. + Check the PodMonitor `monitoring/external-secrets` still selects the + controller pods, and that external-secrets-ns.yaml still admits + `monitoring` on TCP/8080. + + # NOT covered here, deliberately, so nobody assumes it is: + # + # * `condition="Deleted"`. In chart 2.11.0 the only writer of that + # condition is the NotFound branch of Reconcile, which fabricates + # `Deleted=False` purely to make UpdateExternalSecretCondition drop + # the object's Ready series. It is a tombstone for an ExternalSecret + # that no longer exists, not a health state, and `Deleted=True` is + # never emitted at all — a rule on it would be dead code. The one such + # series in the cluster today (`ai/wiki-index-basic-auth`) is leftover + # process state for an ExternalSecret that moved namespace; it clears + # when the controller next restarts. + # * An ExternalSecret that exists in the cluster but was never + # reconciled emits no series at all, so nothing above can see it. That + # needs object-existence data — kube-state-metrics custom resource + # state — rather than ESO's own metrics, and is not in this change.