fix(uptime-monitor): fail only on the ESO reasons that never retry (REN-106) #999

Merged

Ghost merged 2 commits from atlas/ren-106-narrow-eso-failed into master 2026-09-26 08:10:59 +00:00

Conversation 2 Commits 2 Files Changed 1

+76 -3

Ghost commented 2026-09-26 00:28:45 +00:00

Copy Link

Copy Source

Closes REN-106.

What changed

One expression in kubernetes/clusters/homelab/uptime-monitor.yaml, plus the comment that explains it:

-failed: "... c.type == 'Ready' && c.status == 'False'"
+failed: "... c.type == 'Ready' && c.status == 'False' && (c.reason == 'SecretOwnedByOther' || c.reason == 'SecretImmutable')"

current is unchanged. No other file is touched.

Why not the reason REN-106 asked for

REN-106 asked for c.reason == 'SecretSyncedError', mirroring wordpress.yaml, and asked me to confirm the string against the running ESO before shipping. I did, and the confirmation says that string is the wrong one — it would keep the flap and drop the terminal states.

Running ESO is v2.11.0 (kube_pod_container_info{namespace="external-secrets"} → ghcr.io/external-secrets/external-secrets:v2.11.0; the chart range >=2.7.0 <3.0.0 resolves there today). On the Secret target path, pkg/controllers/externalsecret/externalsecret_controller.go at that tag sets Ready=False with exactly three reasons:

reason	set on	ESO's own return	retrying?
SecretSyncedError	provider read failed (GetProviderSecretData), template/update failed, orphan cleanup failed	returns the error → controller-runtime requeues with backoff	yes
SecretOwnedByOther	target Secret owned by something else	returns nil, source comment: "cant be fixed by retrying"	no
SecretImmutable	target Secret immutable, needs update	returns nil, same comment	no

SecretMissing and SecretDeleted are markAsDone reasons — they go on Ready=True, not False.

So SecretSyncedError is the single string covering both an OpenBao blip or token renewal and a key that was never populated (the REN-100 case). It cannot separate transient from terminal, because ESO does not separate them there. Narrowing to it would have kept the only flap-prone reason and dropped the only two that cannot flap — the inverse of the intent. wordpress.yaml works because MariaDB's reason == 'Failed' really is terminal-only; ESO has no equivalent string.

The line the split does fall on is whether ESO is still trying. This change uses that: fail immediately when ESO has given up; wait out the timeout while it is still retrying. A SecretSyncedError now matches neither expression, so Flux reports in-progress and keeps polling until the existing timeout: 5m, then fails the Kustomization — which is the for: window REN-106 correctly says Flux health checks lack. Worst case a genuinely stuck sync is reported ~5m later instead of immediately; a blip that clears inside 5m never produces an error event, so it never reaches the owner's channel.

Verification — offline, not applied

Stated plainly: I have no write path to the cluster and did not apply this. The uptime-monitor ExternalSecret does not exist in the cluster yet either (no externalsecret_status_condition series for namespace="uptime-monitor"; 52 ExternalSecrets are Ready=True and none is this one), so there is currently nothing live to observe it against. Alertmanager currently holds one alert, Watchdog, so no Flux failure is in flight while this merges.

What I did verify:

1. ESO version and reason strings — version from kube-state-metrics above; reasons read from the v2.11.0 tag, not from memory or from the card.
2. Flux evaluation order — fluxcd/pkg runtime/cel/status_evaluator.go: Current is the only required expression, Failed and InProgress are optional, evaluation order is inProgress → failed → current, and "if none of the expressions are true, we return status InProgress". That is what makes dropping SecretSyncedError out of failed a dampening rather than a hole. (kustomize-controller running here is v1.9.5.)
3. Truth table, both expressions evaluated against representative statuses (offline, with the cel-js package — a JS CEL implementation, not the cel-go Flux uses; it agrees on has()/exists() for these expressions but it is a stand-in, not the real evaluator):

freshly applied, no status at all                failed=false current=false -> InProgress
status present, conditions absent                failed=false current=false -> InProgress
Ready=True  SecretSynced (normal)                failed=false current=true  -> Current
Ready=True  SecretMissing (CreationPolicy)       failed=false current=true  -> Current
Ready=False SecretSyncedError (blip / REN-100)   failed=false current=false -> InProgress
Ready=False SecretOwnedByOther (no retry)        failed=true  current=false -> Failed
Ready=False SecretImmutable (no retry)           failed=true  current=false -> Failed
Deleted=False + Ready=True (two conditions)      failed=false current=true  -> Current

4. CI (ci-manifests-test.yml) builds every kustomization and lints the tree on this PR; that is the check that the file still parses and overlays.

Blast radius and revert

Unchanged from #993: one leaf Kustomization that nothing dependsOn, currently reconciling one ExternalSecret. The change can only make this Kustomization fail later (5m instead of instantly) or not at all for transient reasons; it cannot make anything else fail, and it touches no workload. Revert is git revert 16c0c0a7, which restores the broad failed expression and nothing else.

If you would rather not carry a two-reason failed at all, the alternative with the same dampening property is to drop failed entirely and keep the guarded current — everything then fails at the 5m timeout. I kept the two non-retryable reasons because failing fast there is free: ESO has already stopped, so there is nothing to wait for. Say the word and I will cut it to current only.

#995 — resolved

Superseded: #995 merged as 1c00e8bf and carries no healthCheckExprs of its own, so there is exactly one ExternalSecret health check on master (git grep -l over kubernetes/clusters/homelab returns only uptime-monitor.yaml). Nothing to reconcile between the two, and this branch merges cleanly into current master — checked with git merge-tree, which keeps the REN-6 wording #995 added to the comment block just above the expressions.

Addendum 5f335847 — the third status shape

Comment-only. git diff -U0 on that commit has no non-# line and the two CEL expressions from 16c0c0a7 are byte-identical. Pushed on top rather than force-pushed, so the review already done still applies.

It answers the review request to make the third status shape — no Ready condition — explicit. The census behind that request is right; the object behind it is not:

• ESO v2.11.0 builds the Deleted condition on a throwaway &esv1.ExternalSecret{} in the IsNotFound branch of Reconcile (pkg/controllers/externalsecret/externalsecret_controller.go:194), carrying upstream's own note "this does not actually set the condition on the ExternalSecret, because it does not exist". esmetrics.go:105 uses that branch to delete the object's condition="Ready" series. The series is a deletion artifact that outlives the object until the ESO pod restarts.
• ai/wiki-index-basic-auth is that artifact. Retired from git in 419cbed3 (chore(wiki-index): retire the Python service, 2026-09-24 14:13Z); the only remaining git hit for the string is the OpenBao key apps/ai/wiki-index-basic-auth that wiki-api-basic-auth reads. Its target Secret is absent from kube_secret_info{namespace="ai"} while all six live siblings have both a Secret and a Ready series; changes(externalsecret_sync_calls_total[30m]) is 0 for it; and the ESO pod started 2026-09-23T18:35:10Z, before the prune.
• On a live object ESO writes conditions of type Ready only — SetExternalSecretCondition is reached from markAsDone / markAsFailed and nowhere else. "conditions present, no Ready" is not a shape the API can hold.

So three shapes, all decided, and the file now says so: no conditions yet → in-progress (that is what the has() guard buys), Ready=True → current, Ready=False → failed on the two non-retrying reasons, in-progress otherwise. Nothing hangs indefinitely: timeout: 5m ends the wait either way.

Still no inProgress expression, deliberately. The CRD in flux-system/gotk-components.yaml (kustomize-controller v1.9.5) requires only apiVersion and current, and StatusEvaluator.Evaluate in fluxcd/pkg runtime/cel orders the checks inProgress → failed → current, returning in-progress when none match. An inProgress naming shapes 1 and 3 gives the same verdict on all three — a no-op with one more expression to get wrong. If you would rather see it spelled out regardless, it is one line and I will add it.

Two corrections to 16c0c0a7's own comment, both now in the file:

• Ready=True also covers reasons SecretDeleted and SecretMissing, which ESO reports from markAsDone with no target Secret behind them. Unreachable for this object — it takes the defaults (Retain/Owner), where a vanished provider key is a SecretSyncedError — but whoever later sets deletionPolicy: Delete/Merge or creationPolicy: Merge reopens REN-100 straight through current.
• SecretSyncedError is not a clean "still retrying" signal: the ErrSecretSetCtrlRef site returns nil under that same reason. That is why failed is an allowlist of two reasons and not != 'SecretSyncedError'. The generic-target path has its own reason, ResourceSyncedError, unused here and left to the in-progress path.

Verification. Offline for everything about the expressions: ESO v2.11.0 source read locally, the Flux evaluator source, the CRD in this repo, and a shape-by-shape table. No CEL engine was run — there is no Go toolchain in my environment to run one — so the table is reasoning, not execution. Live reads for the facts about the cluster, through the Prometheus proxy grant: ESO image v2.11.0, 52 Ready=True ExternalSecrets, the one ghost series, kube_secret_info, the sync counters, the ESO pod start time. Not applied: I have no cluster write path, and the uptime-monitor ExternalSecret does not exist in the cluster yet.

Revert. git revert 5f335847 drops this addendum and keeps the narrowing. git revert 5f335847 16c0c0a7 returns the file to 69fef47b.

---

Addendum 3 — this is no longer a dormant path (2026-09-26, verifiable from git alone)

The body above says the target ExternalSecret "does not exist in the cluster yet". That was true when written and is no longer true. Correcting it here rather than editing it away:

• #995 merged as 1c00e8bf, carrying kubernetes/apps/uptime-monitor/externalsecret.yaml (uptime-monitor/uptime-robot-api-key, added in c40c337e).
• kubernetes/apps/uptime-monitor/kustomization.yaml on master lists it, and kubernetes/clusters/homelab/uptime-monitor.yaml points spec.path at that directory with wait: true and the broad failed expression.
• So the object the expression governs is now applied by Flux from master. The undampened Kustomization-Failed -> flux-failures Alert -> Alertmanager -> owner path described in REN-106 is live, not hypothetical.

Two numbers that set the exposure: the ExternalSecret's refreshInterval is 1h, and the Kustomization's interval is 10m. Every refresh is an opportunity for a transient Ready=False/SecretSyncedError (an OpenBao blip or a token renewal), and a reconcile lands within 10 minutes of one. Under the expression on master today, any such blip caught by a reconcile emails the owner with no for: window. Under this PR it falls through to in-progress and is only reported if it is still unresolved at timeout: 5m.

Nothing else on master needs reconciling: git grep confirms kubernetes/clusters/homelab/uptime-monitor.yaml is the only file with an external-secrets.io/v1 healthCheckExprs entry, so #995 did indeed land without one of its own and there is no divergent copy to fix.

No new live reads in this addendum. The Prometheus proxy grant used for the earlier addenda is not present in this run, so the ESO-version and condition-shape evidence above stands as previously recorded and was not re-taken. Everything asserted in this section is checkable with git show origin/master:<path> and git grep.

Mergeability. master has advanced to 5f8b6ca0 since this branch was cut; Gitea still reports this PR mergeable with no conflicts. Revert path is unchanged.

🤖 Generated with Claude Code

Closes REN-106. ## What changed One expression in `kubernetes/clusters/homelab/uptime-monitor.yaml`, plus the comment that explains it: ```diff -failed: "... c.type == 'Ready' && c.status == 'False'" +failed: "... c.type == 'Ready' && c.status == 'False' && (c.reason == 'SecretOwnedByOther' || c.reason == 'SecretImmutable')" ``` `current` is unchanged. No other file is touched. ## Why not the reason REN-106 asked for REN-106 asked for `c.reason == 'SecretSyncedError'`, mirroring `wordpress.yaml`, and asked me to confirm the string against the running ESO before shipping. I did, and the confirmation says that string is the wrong one — it would keep the flap and drop the terminal states. Running ESO is **v2.11.0** (`kube_pod_container_info{namespace="external-secrets"}` → `ghcr.io/external-secrets/external-secrets:v2.11.0`; the chart range `>=2.7.0 <3.0.0` resolves there today). On the Secret target path, `pkg/controllers/externalsecret/externalsecret_controller.go` at that tag sets `Ready=False` with exactly three reasons: | reason | set on | ESO's own return | retrying? | |---|---|---|---| | `SecretSyncedError` | provider read failed (`GetProviderSecretData`), template/update failed, orphan cleanup failed | returns the error → controller-runtime requeues with backoff | **yes** | | `SecretOwnedByOther` | target Secret owned by something else | returns `nil`, source comment: *"cant be fixed by retrying"* | no | | `SecretImmutable` | target Secret immutable, needs update | returns `nil`, same comment | no | `SecretMissing` and `SecretDeleted` are `markAsDone` reasons — they go on `Ready=True`, not `False`. So `SecretSyncedError` is the single string covering both an OpenBao blip or token renewal *and* a key that was never populated (the REN-100 case). It cannot separate transient from terminal, because ESO does not separate them there. Narrowing to it would have kept the only flap-prone reason and dropped the only two that cannot flap — the inverse of the intent. `wordpress.yaml` works because MariaDB's `reason == 'Failed'` really is terminal-only; ESO has no equivalent string. The line the split does fall on is whether ESO is still trying. This change uses that: **fail immediately when ESO has given up; wait out the timeout while it is still retrying.** A `SecretSyncedError` now matches neither expression, so Flux reports in-progress and keeps polling until the existing `timeout: 5m`, then fails the Kustomization — which is the `for:` window REN-106 correctly says Flux health checks lack. Worst case a genuinely stuck sync is reported ~5m later instead of immediately; a blip that clears inside 5m never produces an error event, so it never reaches the owner's channel. ## Verification — offline, not applied Stated plainly: I have no write path to the cluster and did not apply this. The `uptime-monitor` ExternalSecret does not exist in the cluster yet either (no `externalsecret_status_condition` series for `namespace="uptime-monitor"`; 52 ExternalSecrets are `Ready=True` and none is this one), so there is currently nothing live to observe it against. Alertmanager currently holds one alert, `Watchdog`, so no Flux failure is in flight while this merges. What I did verify: 1. **ESO version and reason strings** — version from kube-state-metrics above; reasons read from the v2.11.0 tag, not from memory or from the card. 2. **Flux evaluation order** — `fluxcd/pkg` `runtime/cel/status_evaluator.go`: `Current` is the only required expression, `Failed` and `InProgress` are optional, evaluation order is inProgress → failed → current, and **"if none of the expressions are true, we return status InProgress"**. That is what makes dropping `SecretSyncedError` out of `failed` a dampening rather than a hole. (kustomize-controller running here is v1.9.5.) 3. **Truth table**, both expressions evaluated against representative statuses (offline, with the `cel-js` package — a JS CEL implementation, not the cel-go Flux uses; it agrees on `has()`/`exists()` for these expressions but it is a stand-in, not the real evaluator): ``` freshly applied, no status at all failed=false current=false -> InProgress status present, conditions absent failed=false current=false -> InProgress Ready=True SecretSynced (normal) failed=false current=true -> Current Ready=True SecretMissing (CreationPolicy) failed=false current=true -> Current Ready=False SecretSyncedError (blip / REN-100) failed=false current=false -> InProgress Ready=False SecretOwnedByOther (no retry) failed=true current=false -> Failed Ready=False SecretImmutable (no retry) failed=true current=false -> Failed Deleted=False + Ready=True (two conditions) failed=false current=true -> Current ``` 4. CI (`ci-manifests-test.yml`) builds every kustomization and lints the tree on this PR; that is the check that the file still parses and overlays. ## Blast radius and revert Unchanged from #993: one leaf Kustomization that nothing `dependsOn`, currently reconciling one ExternalSecret. The change can only make this Kustomization fail *later* (5m instead of instantly) or not at all for transient reasons; it cannot make anything else fail, and it touches no workload. Revert is `git revert 16c0c0a7`, which restores the broad `failed` expression and nothing else. If you would rather not carry a two-reason `failed` at all, the alternative with the same dampening property is to drop `failed` entirely and keep the guarded `current` — everything then fails at the 5m timeout. I kept the two non-retryable reasons because failing fast there is free: ESO has already stopped, so there is nothing to wait for. Say the word and I will cut it to `current` only. ## #995 — resolved Superseded: #995 merged as `1c00e8bf` and carries **no** `healthCheckExprs` of its own, so there is exactly one ExternalSecret health check on `master` (`git grep -l` over `kubernetes/clusters/homelab` returns only `uptime-monitor.yaml`). Nothing to reconcile between the two, and this branch merges cleanly into current `master` — checked with `git merge-tree`, which keeps the REN-6 wording #995 added to the comment block just above the expressions. ## Addendum `5f335847` — the third status shape **Comment-only.** `git diff -U0` on that commit has no non-`#` line and the two CEL expressions from `16c0c0a7` are byte-identical. Pushed on top rather than force-pushed, so the review already done still applies. It answers the review request to make the third status shape — no `Ready` condition — explicit. The census behind that request is right; the object behind it is not: - ESO v2.11.0 builds the `Deleted` condition on a **throwaway** `&esv1.ExternalSecret{}` in the `IsNotFound` branch of `Reconcile` (`pkg/controllers/externalsecret/externalsecret_controller.go:194`), carrying upstream's own note *"this does not actually set the condition on the ExternalSecret, because it does not exist"*. `esmetrics.go:105` uses that branch to **delete** the object's `condition="Ready"` series. The series is a deletion artifact that outlives the object until the ESO pod restarts. - `ai/wiki-index-basic-auth` is that artifact. Retired from git in `419cbed3` (*chore(wiki-index): retire the Python service*, 2026-09-24 14:13Z); the only remaining git hit for the string is the OpenBao **key** `apps/ai/wiki-index-basic-auth` that `wiki-api-basic-auth` reads. Its target Secret is absent from `kube_secret_info{namespace="ai"}` while all six live siblings have both a Secret and a `Ready` series; `changes(externalsecret_sync_calls_total[30m])` is `0` for it; and the ESO pod started `2026-09-23T18:35:10Z`, before the prune. - On a live object ESO writes conditions of type `Ready` only — `SetExternalSecretCondition` is reached from `markAsDone` / `markAsFailed` and nowhere else. "conditions present, no `Ready`" is not a shape the API can hold. So three shapes, all decided, and the file now says so: no conditions yet → in-progress (that is what the `has()` guard buys), `Ready=True` → current, `Ready=False` → failed on the two non-retrying reasons, in-progress otherwise. Nothing hangs indefinitely: `timeout: 5m` ends the wait either way. **Still no `inProgress` expression, deliberately.** The CRD in `flux-system/gotk-components.yaml` (kustomize-controller v1.9.5) requires only `apiVersion` and `current`, and `StatusEvaluator.Evaluate` in `fluxcd/pkg` `runtime/cel` orders the checks inProgress → failed → current, returning in-progress when none match. An `inProgress` naming shapes 1 and 3 gives the same verdict on all three — a no-op with one more expression to get wrong. If you would rather see it spelled out regardless, it is one line and I will add it. Two corrections to `16c0c0a7`'s own comment, both now in the file: - `Ready=True` also covers reasons `SecretDeleted` and `SecretMissing`, which ESO reports from `markAsDone` with **no target Secret behind them**. Unreachable for this object — it takes the defaults (Retain/Owner), where a vanished provider key is a `SecretSyncedError` — but whoever later sets `deletionPolicy: Delete`/`Merge` or `creationPolicy: Merge` reopens REN-100 straight through `current`. - `SecretSyncedError` is not a clean "still retrying" signal: the `ErrSecretSetCtrlRef` site returns `nil` under that same reason. That is why `failed` is an allowlist of two reasons and not `!= 'SecretSyncedError'`. The generic-target path has its own reason, `ResourceSyncedError`, unused here and left to the in-progress path. **Verification.** Offline for everything about the expressions: ESO v2.11.0 source read locally, the Flux evaluator source, the CRD in this repo, and a shape-by-shape table. No CEL engine was run — there is no Go toolchain in my environment to run one — so the table is reasoning, not execution. Live reads for the facts about the cluster, through the Prometheus proxy grant: ESO image `v2.11.0`, 52 `Ready=True` ExternalSecrets, the one ghost series, `kube_secret_info`, the sync counters, the ESO pod start time. **Not applied**: I have no cluster write path, and the `uptime-monitor` ExternalSecret does not exist in the cluster yet. **Revert.** `git revert 5f335847` drops this addendum and keeps the narrowing. `git revert 5f335847 16c0c0a7` returns the file to `69fef47b`. --- ## Addendum 3 — this is no longer a dormant path (2026-09-26, verifiable from git alone) The body above says the target ExternalSecret *"does not exist in the cluster yet"*. That was true when written and is **no longer true**. Correcting it here rather than editing it away: - `#995` merged as `1c00e8bf`, carrying `kubernetes/apps/uptime-monitor/externalsecret.yaml` (`uptime-monitor/uptime-robot-api-key`, added in `c40c337e`). - `kubernetes/apps/uptime-monitor/kustomization.yaml` on `master` lists it, and `kubernetes/clusters/homelab/uptime-monitor.yaml` points `spec.path` at that directory with `wait: true` and the broad `failed` expression. - So the object the expression governs is now applied by Flux from `master`. The undampened Kustomization-Failed -> `flux-failures` Alert -> Alertmanager -> owner path described in REN-106 is live, not hypothetical. Two numbers that set the exposure: the ExternalSecret's `refreshInterval` is **1h**, and the Kustomization's `interval` is **10m**. Every refresh is an opportunity for a transient `Ready=False`/`SecretSyncedError` (an OpenBao blip or a token renewal), and a reconcile lands within 10 minutes of one. Under the expression on `master` today, any such blip caught by a reconcile emails the owner with no `for:` window. Under this PR it falls through to in-progress and is only reported if it is still unresolved at `timeout: 5m`. Nothing else on `master` needs reconciling: `git grep` confirms `kubernetes/clusters/homelab/uptime-monitor.yaml` is the **only** file with an `external-secrets.io/v1` `healthCheckExprs` entry, so `#995` did indeed land without one of its own and there is no divergent copy to fix. **No new live reads in this addendum.** The Prometheus proxy grant used for the earlier addenda is not present in this run, so the ESO-version and condition-shape evidence above stands as previously recorded and was not re-taken. Everything asserted in *this* section is checkable with `git show origin/master:<path>` and `git grep`. **Mergeability.** `master` has advanced to `5f8b6ca0` since this branch was cut; Gitea still reports this PR mergeable with no conflicts. Revert path is unchanged. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

Ghost added 1 commit 2026-09-26 00:28:45 +00:00

fix(uptime-monitor): fail only on the ESO reasons that never retry (REN-106) ... 16c0c0a7ca

The healthCheckExprs block merged in #993 treated any `Ready=False` on the
ExternalSecret as Failed. The `flux-failures` Alert forwards every Kustomization
error event to Alertmanager and Flux health checks have no `for:` window, so a
single unlucky poll mailed the owner about something that was about to fix
itself.

REN-106 asked for `reason == 'SecretSyncedError'`, matching wordpress.yaml.
Checked against the ESO the cluster runs (v2.11.0) that is the wrong reason:
`SecretSyncedError` is the reason ESO sets when it returns an error and keeps
retrying — an OpenBao blip, a token renewal and a not-yet-populated key all
produce it. `SecretOwnedByOther` and `SecretImmutable` are the two reasons ESO
returns nil on, i.e. where it has stopped retrying.

So `failed` now names those two, and `SecretSyncedError` falls through to
in-progress and fails the Kustomization at the existing `timeout: 5m`, which
becomes the missing `for:` window. Detection of a genuinely stuck sync moves
from immediate to ~5m; a transient blip no longer reaches the owner at all.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Ghost requested review from Ghost 2026-09-26 00:28:49 +00:00

review-bot reviewed 2026-09-26 00:36:39 +00:00

review-bot left a comment

Member

Copy Link

Copy Source

🤖 Automated review — model claude-sonnet-5 (complexity: normal), commit 16c0c0a7cad934b0a75e64bebb6fc418281d83b5.

Advisory review by the review-bot pipeline. It never approves; a human merges. Push new commits to get a fresh review.

---

Summary

Single-expression change to kubernetes/clusters/homelab/uptime-monitor.yaml: the failed CEL health-check expression for the ExternalSecret target now only matches Ready=False when reason is SecretOwnedByOther or SecretImmutable, instead of any Ready=False. The accompanying comment (~60 lines) documents the reasoning in detail. The change is small, self-contained, and consistent with the repo's existing healthCheckExprs conventions.

No blocking issues found.

🟡 Suggestions

• External claims are not independently verifiable in this review environment. The PR body and the new comment (uptime-monitor.yaml:74-109) make specific claims about ESO v2.11.0's source (pkg/controllers/externalsecret/externalsecret_controller.go): that exactly three reasons set Ready=False on the Secret-target path, that SecretOwnedByOther/SecretImmutable return nil (no requeue) while SecretSyncedError returns an error (requeue with backoff), and a Flux evaluator claim about fluxcd/pkg runtime/cel. This review had no outbound network access, so these could not be fetched and checked against the actual ESO/Flux source. What I could verify locally:

  • The chart version bound >=2.7.0 <3.0.0 in kubernetes/infrastructure/external-secrets/helmrelease.yaml:23 matches the PR's citation.
  • The flux-failures Alert (kubernetes/infrastructure/flux-notifications/alerts.yaml:29-49) does forward eventSeverity: error from every Kustomization (name: '*') to Alertmanager, supporting the "undampened failures reach the owner immediately" claim.
  • The expression's own CEL is syntactically sound: && binds tighter than || in CEL, and the explicit parens around (c.reason == 'SecretOwnedByOther' || c.reason == 'SecretImmutable') remove any ambiguity anyway.
  • The pattern matches the existing convention in wordpress.yaml:28 (c.reason == 'Failed', no extra has() guard on reason), so this isn't introducing a new risk shape.

  I can't fault the PR for this — it explicitly discloses that it verified offline and that the target ExternalSecret doesn't exist in-cluster yet, so there's nothing live to check now. But since the correctness of this fix hinges entirely on the ESO reason/retry claims being accurate, it's worth a human (or a follow-up check once the uptime-monitor ExternalSecret actually exists) confirming behavior against a live Ready=False SecretSyncedError and a live SecretOwnedByOther/SecretImmutable condition before fully trusting the dampening behavior in production.

Tests

No automated test covers CEL expression correctness — consistent with the rest of the repo (wordpress.yaml, infrastructure.yaml's healthCheckExprs blocks have no test coverage either). ci-manifests-test.yml's kustomize-build step (confirmed by reading the file) validates that the manifest still parses and overlays via kubectl kustomize, but that does not exercise CEL semantics — the PR body is careful to only claim the former, not the latter. Given the target ExternalSecret isn't live in the cluster yet, there's no way to add a live-behavior test right now; a manual verification once it exists (or in a scratch cluster) would be the natural follow-up, as noted above.

Recommendation

Looks reasonable — no blocking issues; comment above about verifying the ESO source-level claims once feasible is optional/deferred, not a merge blocker.

🤖 **Automated review** — model `claude-sonnet-5` (complexity: normal), commit `16c0c0a7cad934b0a75e64bebb6fc418281d83b5`. > Advisory review by the `review-bot` pipeline. It never approves; a human merges. Push new commits to get a fresh review. --- ## Summary Single-expression change to `kubernetes/clusters/homelab/uptime-monitor.yaml`: the `failed` CEL health-check expression for the `ExternalSecret` target now only matches `Ready=False` when `reason` is `SecretOwnedByOther` or `SecretImmutable`, instead of any `Ready=False`. The accompanying comment (~60 lines) documents the reasoning in detail. The change is small, self-contained, and consistent with the repo's existing `healthCheckExprs` conventions. No blocking issues found. ## 🟡 Suggestions - **External claims are not independently verifiable in this review environment.** The PR body and the new comment (`uptime-monitor.yaml:74-109`) make specific claims about ESO v2.11.0's source (`pkg/controllers/externalsecret/externalsecret_controller.go`): that exactly three reasons set `Ready=False` on the Secret-target path, that `SecretOwnedByOther`/`SecretImmutable` return `nil` (no requeue) while `SecretSyncedError` returns an error (requeue with backoff), and a Flux evaluator claim about `fluxcd/pkg runtime/cel`. This review had no outbound network access, so these could not be fetched and checked against the actual ESO/Flux source. What I *could* verify locally: - The chart version bound `>=2.7.0 <3.0.0` in `kubernetes/infrastructure/external-secrets/helmrelease.yaml:23` matches the PR's citation. - The `flux-failures` Alert (`kubernetes/infrastructure/flux-notifications/alerts.yaml:29-49`) does forward `eventSeverity: error` from every `Kustomization` (`name: '*'`) to Alertmanager, supporting the "undampened failures reach the owner immediately" claim. - The expression's own CEL is syntactically sound: `&&` binds tighter than `||` in CEL, and the explicit parens around `(c.reason == 'SecretOwnedByOther' || c.reason == 'SecretImmutable')` remove any ambiguity anyway. - The pattern matches the existing convention in `wordpress.yaml:28` (`c.reason == 'Failed'`, no extra `has()` guard on `reason`), so this isn't introducing a new risk shape. I can't fault the PR for this — it explicitly discloses that it verified offline and that the target `ExternalSecret` doesn't exist in-cluster yet, so there's nothing live to check now. But since the correctness of this fix hinges entirely on the ESO reason/retry claims being accurate, it's worth a human (or a follow-up check once the `uptime-monitor` ExternalSecret actually exists) confirming behavior against a live `Ready=False` `SecretSyncedError` and a live `SecretOwnedByOther`/`SecretImmutable` condition before fully trusting the dampening behavior in production. ## Tests No automated test covers CEL expression correctness — consistent with the rest of the repo (`wordpress.yaml`, `infrastructure.yaml`'s `healthCheckExprs` blocks have no test coverage either). `ci-manifests-test.yml`'s `kustomize-build` step (confirmed by reading the file) validates that the manifest still parses and overlays via `kubectl kustomize`, but that does not exercise CEL semantics — the PR body is careful to only claim the former, not the latter. Given the target `ExternalSecret` isn't live in the cluster yet, there's no way to add a live-behavior test right now; a manual verification once it exists (or in a scratch cluster) would be the natural follow-up, as noted above. ## Recommendation Looks reasonable — no blocking issues; comment above about verifying the ESO source-level claims once feasible is optional/deferred, not a merge blocker. <!-- pr-review:sha=16c0c0a7cad934b0a75e64bebb6fc418281d83b5 -->

Ghost added 1 commit 2026-09-26 03:09:01 +00:00

docs(uptime-monitor): give the third ExternalSecret status shape a verdict (REN-106) ... 5f335847a3

Comment-only. The narrowing in 16c0c0a7 is unchanged; this records what the
two expressions do with the shape neither of them mentions — an ExternalSecret
with no `Ready` condition.

Review of REN-106 asked for the third state to be made explicit, on the
strength of a metrics census showing `ai/wiki-index-basic-auth` with only a
`Deleted=False` condition. That series is real; the object behind it is not.
ESO v2.11.0 builds the `Deleted` condition on a throwaway object in the
`IsNotFound` branch of Reconcile, purely to retire the object's metric series,
and never persists it — so the series outlives the object until the ESO pod
restarts. That ExternalSecret was retired from git in 419cbed3, its target
Secret is gone from `ai`, and its sync counter is frozen while its six live
siblings advance. On a live object ESO writes conditions of type `Ready` only.

What that leaves is three shapes, all decided: no conditions yet (in-progress,
bounded by `timeout: 5m`), `Ready=True` (current), `Ready=False` (failed for
the two non-retrying reasons, in-progress otherwise). Flux evaluates
inProgress -> failed -> current and falls back to in-progress, so an explicit
`inProgress` expression would be a no-op on all three; the reasoning is written
down instead of encoded twice.

Also noted: `Ready=True` covers reasons `SecretDeleted` and `SecretMissing`,
where ESO reports healthy with no Secret behind it. Unreachable under this
object's default Retain/Owner policies, and a trap for whoever changes them.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Ghost approved these changes 2026-09-26 08:10:51 +00:00

Ghost left a comment

Copy Link

Copy Source

APPROVED — commit 5f335847a36f43100ae93d816f399de264877923

Reviewed in a private worktree at that SHA (git rev-parse HEAD = 5f335847a3…), diffed against origin/master = f96f04cd.

What I checked

Scope. git diff origin/master...5f335847 touches one file, kubernetes/clusters/homelab/uptime-monitor.yaml, +76/−3. One functional line: failed gains && (c.reason == 'SecretOwnedByOther' || c.reason == 'SecretImmutable'). current is byte-identical to master. Everything else is comment.

The addendum really is comment-only. git diff -U0 16c0c0a7 5f335847 filtered for non-# lines is empty, and the two expression lines hash the same at both commits (md5 37d2a5d6… on grep -E '^\s+(failed|current):'). The review done on 16c0c0a7 still stands at the head.

The path is live, checked from git, not from the description. kubernetes/apps/uptime-monitor/externalsecret.yaml is on master and listed in that directory's kustomization.yaml; this Kustomization has path: ./kubernetes/apps/uptime-monitor, wait: true, interval: 10m, timeout: 5m. kubernetes/infrastructure/flux-notifications/alerts.yaml has Alert flux-failures at eventSeverity: error matching Kustomization name: '*' → alertmanager. So today's broad failed does reach the owner on a single bad poll, with no for: window. The noise this fixes is real, not theoretical.

In-repo precedent for the shape. git grep over origin/master finds exactly one other c.reason health check — kubernetes/clusters/homelab/wordpress.yaml:28 — and exactly one external-secrets.io/v1 healthCheckExprs in the tree, this file. No divergent second copy left by #995.

Blast radius. Nothing dependsOn uptime-monitor. The Kustomization's only ExternalSecret is the one in its own inventory. The change can only make this leaf fail later (at timeout: 5m) or not at all for a blip that self-heals; it cannot make anything else fail and it touches no workload.

Revertibility — tested, and the description is wrong about it. I merged 5f335847 onto origin/master in a scratch branch and tried all three paths:

git revert 16c0c0a7                 -> CONFLICT (UU uptime-monitor.yaml)
git revert --no-edit 5f335847 16c0c0a7 -> CLEAN, file identical to master
git revert --no-edit -m 1 <merge>      -> CLEAN, file identical to master

git revert 16c0c0a7 alone — the path this PR body and the review card both name — does not apply, because 5f335847 rewrote the same comment block. Not blocking, because two paths do work and I have run both, but the documented one is unusable. The revert of record is git revert -m 1 <merge commit>.

CI. Combined status success on 5f335847: ci-manifests-test, ci-scripts-test, pr-review. The manifests job builds every kustomization, so the file parses and the overlay still renders.

On the offline verification

The ESO reason semantics are read from source, not observed live, and the PR says so. I merge anyway because the failure mode of being wrong is bounded in both directions:

• If SecretOwnedByOther / SecretImmutable do in fact retry, this fails fast on a retryable state — which is exactly what master does today for those reasons. No regression.
• If SecretSyncedError is terminal rather than transient, it is still reported, ~5 minutes later, via timeout: 5m.

Neither direction is worse than the expression it replaces. That is what makes an unapplied change acceptable here.

Residual risks, accepted and recorded

1. A SecretSyncedError that clears inside 5m now produces no signal at all. That is the intended dampening, but it means chronic flapping is invisible to this path; catching it needs the metric-based alert in REN-103.
2. c.reason on a condition with no reason key is a CEL "no such key" error, and ESO's condition struct has reason,omitempty. The && short-circuit keeps Ready=True conditions safe, so the only exposure is Ready=False with an empty reason — where master's expression already produces an error event too. Same alert volume either way, worse message. Pre-existing in wordpress.yaml as well.

Merging with head_commit_id pinned to 5f335847a36f43100ae93d816f399de264877923.

## APPROVED — commit `5f335847a36f43100ae93d816f399de264877923` Reviewed in a private worktree at that SHA (`git rev-parse HEAD` = `5f335847a3…`), diffed against `origin/master` = `f96f04cd`. ### What I checked **Scope.** `git diff origin/master...5f335847` touches one file, `kubernetes/clusters/homelab/uptime-monitor.yaml`, +76/−3. One functional line: `failed` gains `&& (c.reason == 'SecretOwnedByOther' || c.reason == 'SecretImmutable')`. `current` is byte-identical to `master`. Everything else is comment. **The addendum really is comment-only.** `git diff -U0 16c0c0a7 5f335847` filtered for non-`#` lines is empty, and the two expression lines hash the same at both commits (`md5 37d2a5d6…` on `grep -E '^\s+(failed|current):'`). The review done on `16c0c0a7` still stands at the head. **The path is live, checked from git, not from the description.** `kubernetes/apps/uptime-monitor/externalsecret.yaml` is on `master` and listed in that directory's `kustomization.yaml`; this Kustomization has `path: ./kubernetes/apps/uptime-monitor`, `wait: true`, `interval: 10m`, `timeout: 5m`. `kubernetes/infrastructure/flux-notifications/alerts.yaml` has Alert `flux-failures` at `eventSeverity: error` matching `Kustomization` `name: '*'` → `alertmanager`. So today's broad `failed` does reach the owner on a single bad poll, with no `for:` window. The noise this fixes is real, not theoretical. **In-repo precedent for the shape.** `git grep` over `origin/master` finds exactly one other `c.reason` health check — `kubernetes/clusters/homelab/wordpress.yaml:28` — and exactly one `external-secrets.io/v1` `healthCheckExprs` in the tree, this file. No divergent second copy left by #995. **Blast radius.** Nothing `dependsOn` `uptime-monitor`. The Kustomization's only ExternalSecret is the one in its own inventory. The change can only make this leaf fail *later* (at `timeout: 5m`) or not at all for a blip that self-heals; it cannot make anything else fail and it touches no workload. **Revertibility — tested, and the description is wrong about it.** I merged `5f335847` onto `origin/master` in a scratch branch and tried all three paths: ``` git revert 16c0c0a7 -> CONFLICT (UU uptime-monitor.yaml) git revert --no-edit 5f335847 16c0c0a7 -> CLEAN, file identical to master git revert --no-edit -m 1 <merge> -> CLEAN, file identical to master ``` `git revert 16c0c0a7` alone — the path this PR body and the review card both name — **does not apply**, because `5f335847` rewrote the same comment block. Not blocking, because two paths do work and I have run both, but the documented one is unusable. The revert of record is `git revert -m 1 <merge commit>`. **CI.** Combined status `success` on `5f335847`: `ci-manifests-test`, `ci-scripts-test`, `pr-review`. The manifests job builds every kustomization, so the file parses and the overlay still renders. ### On the offline verification The ESO reason semantics are read from source, not observed live, and the PR says so. I merge anyway because the failure mode of being wrong is bounded in both directions: - If `SecretOwnedByOther` / `SecretImmutable` do in fact retry, this fails fast on a retryable state — which is exactly what `master` does today for those reasons. No regression. - If `SecretSyncedError` is terminal rather than transient, it is still reported, ~5 minutes later, via `timeout: 5m`. Neither direction is worse than the expression it replaces. That is what makes an unapplied change acceptable here. ### Residual risks, accepted and recorded 1. A `SecretSyncedError` that clears inside 5m now produces **no** signal at all. That is the intended dampening, but it means chronic flapping is invisible to this path; catching it needs the metric-based alert in REN-103. 2. `c.reason` on a condition with no `reason` key is a CEL "no such key" error, and ESO's condition struct has `reason,omitempty`. The `&&` short-circuit keeps `Ready=True` conditions safe, so the only exposure is `Ready=False` with an empty reason — where `master`'s expression already produces an error event too. Same alert volume either way, worse message. Pre-existing in `wordpress.yaml` as well. Merging with `head_commit_id` pinned to `5f335847a36f43100ae93d816f399de264877923`.

Ghost merged commit ea0abda79c into master 2026-09-26 08:10:59 +00:00

Ghost referenced this issue from a commit 2026-09-26 08:10:59 +00:00

Merge pull request 'fix(uptime-monitor): fail only on the ESO reasons that never retry (REN-106)' (#999) from atlas/ren-106-narrow-eso-failed into master

Sign in to join this conversation.

Reviewers

No Reviewers

review-bot

Ghost

Labels

Clear labels

apollo

Hermes expert: Immich, Audiobookshelf, Libation, Navidrome, Music Assistant

atlas

Hermes expert: Talos, Flux, network, Authentik, Matrix, monitoring

clio

Hermes expert: Paperless, wiki, ical-proxy, TRMNL, websites

daedalus

Hermes expert: Gitea, Woodpecker, Renovate, review-bot, Hermes

hestia

Hermes expert: Home Assistant, IoT, MQTT, ESPHome, voice

No labels

Milestone

No items

No Milestone

Projects

Clear projects

No projects

Assignees

Clear assignees

alert-monitor-bot claude-bot (Claude Bot) flux hermes-bot issue-fix-bot k.renner renovate-bot review-bot (Review Bot) security-bot update-audit-bot

No Assignees

2 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: konairius/homelab#999