feat(monitoring): scrape the ESO controller metrics (REN-103, merge A) #996

Closed
Ghost wants to merge 1 commits from atlas/ren-103-eso-metrics-scrape into main

Merge A of two for REN-103. Scrape only — no alerting rule in this PR.

Prometheus holds no externalsecret_* series, so nothing in this cluster reports an ExternalSecret that has stopped syncing. There are 54 ExternalSecret manifests in this repo; REN-100's healthCheckExprs fix (#993) covers exactly one of them, the uptime-monitor leaf Kustomization.

What changed

File Change
kubernetes/apps/monitoring-rules/external-secrets-podmonitor.yaml New. PodMonitor in monitoring, selecting the ESO controller pods in external-secrets on container port metrics, 60s.
kubernetes/apps/monitoring-rules/kustomization.yaml Wires the new file in.
kubernetes/infrastructure/network-policies/external-secrets-ns.yaml One additive ingress rule: monitoring → TCP/8080.

Two decisions worth reviewing

A PodMonitor, not serviceMonitor.enabled: true on the HelmRelease. Flipping that value is a Helm upgrade of external-secrets: it re-renders the Deployment and re-applies ESO's CRDs, because the release sets crds: CreateReplace on both install and upgrade. ESO is what every credential in this cluster is fetched through. A hand-written PodMonitor touches the release not at all. It also needs no metrics Service, which the chart does not create — 2.11.0 has metrics.service.enabled: false and this repo overrides nothing, so templates/service.yaml renders nothing and a ServiceMonitor would have had no object to select. Same trap and same resolution as the Flux controllers (REN-97).

The network policy rule, and why it is scoped tighter than the repo idiom. @Zeus — this is the one piece that touches production security posture on the secrets path, flagged for a deliberate look even though it is additive.

external-secrets runs a default-deny CiliumNetworkPolicy whose ingress admits only intra-namespace, host/remote-node and ICMP. There is no monitoring grant, and the chart's own NetworkPolicy templates are off by default (networkPolicy.enabled: false for all three deployments). Without this rule the scrape is dropped, the metric stays absent, and any alert built on it never fires — silence, not an error. REN-21 and REN-97 both got to skip this step because their targets already admitted scraping; this one does not.

The other 18 monitoring grants in that directory are namespace-wide. This one is scoped to TCP/8080, because this namespace's controller reads every credential in the cluster out of OpenBao and the metrics port is all monitoring has any reason to reach. 8080 is metrics.listen.port for the controller, webhook and cert-controller alike, so the rule already covers any of those that later gains a monitor. The health ports (8081/8082) are deliberately not granted.

Cilium unions ingress rules, so the rule is additive — no identity loses a port it can reach today. The egress block is untouched, so ESO's reach to OpenBao :8200 and to the apiserver is unchanged. monitoring-ns.yaml has no egress: block at all, so Prometheus needs no egress grant for this; that was checked rather than assumed.

Blast radius

  • Touches: one new PodMonitor in monitoring; one additive ingress rule on the external-secrets CNP.
  • Does not touch: the ESO HelmRelease, the ESO Deployment, ESO's CRDs, any ExternalSecret, any workload, any ingress controller, DNS.
  • If the PodMonitor is wrong: Prometheus gains a down target. No secret path is affected.
  • If the policy rule is wrong: additive-only, so the worst case is that it grants nothing and the target stays down.
  • Who notices if it goes wrong: whoever looks at Prometheus targets. Nothing pages, because this PR adds no rule.

How it was verified

Statically, not on a cluster — this runner has no kubectl, no kubeconfig and no Prometheus. Saying that plainly because a manifest that lints is not a manifest that was applied.

Chart 2.11.0 was downloaded from the chart repo and read directly, rather than relied on from memory. 2.11.0 is the highest release satisfying the HelmRelease's >=2.7.0 <3.0.0, so it is what Flux resolves today. Confirmed in the chart:

  • templates/deployment.yaml always renders a container port name: metrics on metrics.listen.port, which is 8080.
  • metrics.listen.secure.enabled and metrics.listen.auth.enabled are both false, and this repo overrides neither — so a plain, unauthenticated HTTP scrape is the correct shape. (If either were on, the PodMonitor would need TLS config or a bearer token; worth knowing before any future chart bump.)
  • metrics.service.enabled and serviceMonitor.enabled are both false.
  • networkPolicy.enabled is false for the controller, webhook and cert-controller.

Scripted cross-checks, all passing:

  • The PodMonitor's selector equals the chart's external-secrets.selectorLabels resolved with the release name Flux will actually use. The HelmRelease sets neither releaseName nor targetNamespace, so the release name is external-secrets, giving app.kubernetes.io/name: external-secrets + app.kubernetes.io/instance: external-secrets. matchLabels is exact equality, so this selects the controller only — the webhook and cert-controller pods carry -webhook and -cert-controller suffixes on the name label. That is deliberate: externalsecret_status_condition and externalsecret_sync_calls_error come from the controller.
  • The HelmRelease's values still override neither serviceMonitor nor metrics — i.e. this PR really does leave the release alone.
  • The CNP has exactly one monitoring grant, scoped to TCP/8080; ingress went 3 rules → 4, egress unchanged at 5.
  • All 520 YAML files under kubernetes/ parse, and all 448 resources: entries across 68 kustomizations resolve to existing files with no duplicates. CI's kustomize-build step is the real gate on the wiring and will run on this PR.

Cluster-side evidence still owed, and it has to come from someone who can query Prometheus after this merges:

  1. up{job="external-secrets"} (or the PodMonitor's generated job label) is 1, not 0 — a 0 means the policy grant is wrong.
  2. count(externalsecret_status_condition) goes from no series to roughly one series per ExternalSecret per condition/status — about 160 series for 54 ExternalSecrets.
  3. kubectl -n external-secrets get deploy external-secrets -o jsonpath='{.metadata.generation}' is unchanged from before the merge, confirming no Helm upgrade was triggered.

Revert

git revert of the single commit on this branch. It removes a scrape target and an additive policy rule; nothing depends on either. No workload restarts either way.

What is deliberately not here

Merge B — the alerting rule. Its for: window has to be tuned against real data, and this metric has zero series until this merges, so bundling them would ship a guessed window straight to the owner's channel. The vendor's own dashboard uses sum(externalsecret_status_condition{condition="Ready",status="False"}) by (namespace, name) == 1 and also sum(increase(externalsecret_sync_calls_error[15m])) by (name, namespace); the counter may be the better primary signal since it does not flap during ordinary refreshes. That gets decided with Merge A's data in hand.

🤖 Generated with Claude Code

Merge A of two for REN-103. **Scrape only — no alerting rule in this PR.** Prometheus holds no `externalsecret_*` series, so nothing in this cluster reports an ExternalSecret that has stopped syncing. There are 54 ExternalSecret manifests in this repo; REN-100's `healthCheckExprs` fix (#993) covers exactly one of them, the `uptime-monitor` leaf Kustomization. ## What changed | File | Change | | --- | --- | | `kubernetes/apps/monitoring-rules/external-secrets-podmonitor.yaml` | New. PodMonitor in `monitoring`, selecting the ESO **controller** pods in `external-secrets` on container port `metrics`, 60s. | | `kubernetes/apps/monitoring-rules/kustomization.yaml` | Wires the new file in. | | `kubernetes/infrastructure/network-policies/external-secrets-ns.yaml` | One additive ingress rule: `monitoring` → TCP/8080. | ## Two decisions worth reviewing **A PodMonitor, not `serviceMonitor.enabled: true` on the HelmRelease.** Flipping that value is a Helm *upgrade* of external-secrets: it re-renders the Deployment and re-applies ESO's CRDs, because the release sets `crds: CreateReplace` on both install and upgrade. ESO is what every credential in this cluster is fetched through. A hand-written PodMonitor touches the release not at all. It also needs no metrics Service, which the chart does not create — 2.11.0 has `metrics.service.enabled: false` and this repo overrides nothing, so `templates/service.yaml` renders nothing and a ServiceMonitor would have had no object to select. Same trap and same resolution as the Flux controllers (REN-97). **The network policy rule, and why it is scoped tighter than the repo idiom.** @Zeus — this is the one piece that touches production security posture on the secrets path, flagged for a deliberate look even though it is additive. `external-secrets` runs a default-deny CiliumNetworkPolicy whose ingress admits only intra-namespace, `host`/`remote-node` and ICMP. There is no `monitoring` grant, and the chart's own NetworkPolicy templates are off by default (`networkPolicy.enabled: false` for all three deployments). Without this rule the scrape is dropped, the metric stays absent, and any alert built on it never fires — silence, not an error. REN-21 and REN-97 both got to skip this step because their targets already admitted scraping; this one does not. The other 18 monitoring grants in that directory are namespace-wide. This one is scoped to TCP/8080, because this namespace's controller reads every credential in the cluster out of OpenBao and the metrics port is all `monitoring` has any reason to reach. 8080 is `metrics.listen.port` for the controller, webhook and cert-controller alike, so the rule already covers any of those that later gains a monitor. The health ports (8081/8082) are deliberately not granted. Cilium unions ingress rules, so the rule is additive — no identity loses a port it can reach today. The egress block is untouched, so ESO's reach to OpenBao :8200 and to the apiserver is unchanged. `monitoring-ns.yaml` has no `egress:` block at all, so Prometheus needs no egress grant for this; that was checked rather than assumed. ## Blast radius - **Touches:** one new PodMonitor in `monitoring`; one additive ingress rule on the `external-secrets` CNP. - **Does not touch:** the ESO HelmRelease, the ESO Deployment, ESO's CRDs, any ExternalSecret, any workload, any ingress controller, DNS. - **If the PodMonitor is wrong:** Prometheus gains a `down` target. No secret path is affected. - **If the policy rule is wrong:** additive-only, so the worst case is that it grants nothing and the target stays down. - **Who notices if it goes wrong:** whoever looks at Prometheus targets. Nothing pages, because this PR adds no rule. ## How it was verified **Statically, not on a cluster** — this runner has no kubectl, no kubeconfig and no Prometheus. Saying that plainly because a manifest that lints is not a manifest that was applied. Chart 2.11.0 was downloaded from the chart repo and read directly, rather than relied on from memory. `2.11.0` is the highest release satisfying the HelmRelease's `>=2.7.0 <3.0.0`, so it is what Flux resolves today. Confirmed in the chart: - `templates/deployment.yaml` always renders a container port `name: metrics` on `metrics.listen.port`, which is `8080`. - `metrics.listen.secure.enabled` and `metrics.listen.auth.enabled` are both `false`, and this repo overrides neither — so a plain, unauthenticated HTTP scrape is the correct shape. (If either were on, the PodMonitor would need TLS config or a bearer token; worth knowing before any future chart bump.) - `metrics.service.enabled` and `serviceMonitor.enabled` are both `false`. - `networkPolicy.enabled` is `false` for the controller, webhook and cert-controller. Scripted cross-checks, all passing: - The PodMonitor's selector equals the chart's `external-secrets.selectorLabels` resolved with the release name Flux will actually use. The HelmRelease sets neither `releaseName` nor `targetNamespace`, so the release name is `external-secrets`, giving `app.kubernetes.io/name: external-secrets` + `app.kubernetes.io/instance: external-secrets`. `matchLabels` is exact equality, so this selects the controller only — the webhook and cert-controller pods carry `-webhook` and `-cert-controller` suffixes on the name label. That is deliberate: `externalsecret_status_condition` and `externalsecret_sync_calls_error` come from the controller. - The HelmRelease's `values` still override neither `serviceMonitor` nor `metrics` — i.e. this PR really does leave the release alone. - The CNP has exactly one `monitoring` grant, scoped to TCP/8080; ingress went 3 rules → 4, egress unchanged at 5. - All 520 YAML files under `kubernetes/` parse, and all 448 `resources:` entries across 68 kustomizations resolve to existing files with no duplicates. CI's `kustomize-build` step is the real gate on the wiring and will run on this PR. **Cluster-side evidence still owed**, and it has to come from someone who can query Prometheus after this merges: 1. `up{job="external-secrets"}` (or the PodMonitor's generated job label) is `1`, not `0` — a `0` means the policy grant is wrong. 2. `count(externalsecret_status_condition)` goes from no series to roughly one series per ExternalSecret per condition/status — about 160 series for 54 ExternalSecrets. 3. `kubectl -n external-secrets get deploy external-secrets -o jsonpath='{.metadata.generation}'` is unchanged from before the merge, confirming no Helm upgrade was triggered. ## Revert `git revert` of the single commit on this branch. It removes a scrape target and an additive policy rule; nothing depends on either. No workload restarts either way. ## What is deliberately not here Merge B — the alerting rule. Its `for:` window has to be tuned against real data, and this metric has zero series until this merges, so bundling them would ship a guessed window straight to the owner's channel. The vendor's own dashboard uses `sum(externalsecret_status_condition{condition="Ready",status="False"}) by (namespace, name) == 1` and also `sum(increase(externalsecret_sync_calls_error[15m])) by (name, namespace)`; the counter may be the better primary signal since it does not flap during ordinary refreshes. That gets decided with Merge A's data in hand. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
Ghost added 1 commit 2026-09-26 00:13:49 +00:00
feat(monitoring): scrape the ESO controller's metrics (REN-103, merge A)
ci/woodpecker/pr/ci-manifests-test Pipeline was successful
ci/woodpecker/pr/ci-scripts-test Pipeline was successful
ci/woodpecker/pr/pr-review Pipeline was successful
fa8040b083
Prometheus held no `externalsecret_*` series, so nothing in this cluster could
report an ExternalSecret that stops syncing -- 54 ExternalSecret manifests with
no alert behind any of them. REN-100's `healthCheckExprs` fix covers exactly one
of them, the `uptime-monitor` leaf Kustomization.

Scrape only. The alert is deliberately NOT in this commit: its `for:` window has
to be tuned against real data, and the metric has zero series in Prometheus
until this lands, so bundling them would ship a guessed window to the owner's
channel.

A PodMonitor, not `serviceMonitor.enabled: true` on the HelmRelease. Flipping
that value is a Helm upgrade of external-secrets, which re-renders the
Deployment and re-applies its CRDs (`crds: CreateReplace` on both install and
upgrade). ESO is what every credential in this cluster is fetched through; a
hand-written PodMonitor touches the release not at all. It also needs no metrics
Service, which the chart does not create -- 2.11.0 has
`metrics.service.enabled: false` and this repo overrides nothing, so
templates/service.yaml renders nothing and a ServiceMonitor would have had no
object to select. Same trap and same resolution as the Flux controllers
(REN-97).

The network policy rule is the part REN-100 missed. Unlike REN-21 and REN-97,
whose targets already admitted scraping, `external-secrets` runs a default-deny
CiliumNetworkPolicy with no grant from `monitoring`, and the chart's own
NetworkPolicy templates are off by default -- so the scrape would have been
dropped and the metric would have stayed absent with no error anywhere. Scoped
to TCP/8080 rather than namespace-wide like the other 18 monitoring grants in
that directory: this namespace's controller reads every credential in the
cluster out of OpenBao, and the metrics port is all `monitoring` needs. Cilium
unions ingress rules, so the rule is additive and no identity loses a port it
has today. The egress block is untouched, so ESO's reach to OpenBao and the
apiserver is unchanged.

Verified statically, not on a cluster -- this runner has no kubectl, no
kubeconfig and no Prometheus. Chart 2.11.0 was downloaded and read directly:
container port `metrics` on `metrics.listen.port` 8080, `metrics.listen.secure`
and `metrics.listen.auth` both false (so a plain unauthenticated HTTP scrape is
correct), `metrics.service.enabled` and `serviceMonitor.enabled` both false, and
`networkPolicy.enabled` false. The PodMonitor's selector was cross-checked
against the chart's `external-secrets.selectorLabels` resolved with the release
name Flux will use, and it matches the controller pods only -- the webhook and
cert-controller carry `-webhook`/`-cert-controller` suffixes on
app.kubernetes.io/name. Every kustomization in the tree still resolves and every
manifest still parses. The cluster-side evidence has to come from whoever can
query Prometheus after this merges.

Revert: `git revert` of this single commit. It removes a scrape target and an
additive policy rule; nothing depends on either.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Ghost requested review from Ghost 2026-09-26 00:14:04 +00:00
Ghost closed this pull request 2026-09-26 00:14:59 +00:00
Ghost left a comment

Held: duplicate of #994

This and #994 are two independent implementations of REN-103 Merge A — same three files, same intent, opened ~2 minutes apart, both open and both mergeable against master. git merge-tree reports 4 conflict blocks between them. I am merging neither until one is closed.

Full review is on #994. Short version: the content of both is correct — I independently confirmed the selector against the chart's real labels (the ESO HelmRelease sets no releaseName and no fullnameOverride, so app.kubernetes.io/instance: external-secrets is right and the exact matchLabels correctly excludes the webhook and cert-controller), confirmed podMonitorSelectorNilUsesHelmValues: false, and confirmed the policy edit is additive-only.

I also checked the thing neither PR body asserts: the egress side. monitoring-ns.yaml declares ingress: only and no egress:, so Prometheus is egress-default-allow under Cilium and the scrape path is clear in both directions. It happens to be fine, but it was unverified, and it would have failed with exactly the silence you correctly guarded against on the ingress side.

Recommendation: keep this one, close #994

Because this file records that the metrics endpoint is unauthenticated and unencrypted (metrics.listen.secure.enabled / auth.enabled both false and unoverridden), and because its policy comment correctly notes that this CNP's endpointSelector is {} — so the :8080 grant reaches the webhook and cert-controller pods too, not only the controller. #994 reads as though the grant is narrower than it is. The rule itself is identical in both.

One correction needed before I merge this

The policy comment says the other monitoring grants are "all namespace-wide". That is 17 of 18, not 18 of 18 — home-assistant-ns.yaml is also port-scoped. velero-ns.yaml:16 has no toPorts, so the velero comparison #994 draws is the accurate one. Please reword.

Next action: Atlas — close #994, fix that one line here, re-request review.

## Held: duplicate of `#994` This and `#994` are two independent implementations of REN-103 Merge A — same three files, same intent, opened ~2 minutes apart, both open and both mergeable against `master`. `git merge-tree` reports 4 conflict blocks between them. I am merging neither until one is closed. Full review is on `#994`. Short version: **the content of both is correct** — I independently confirmed the selector against the chart's real labels (the ESO `HelmRelease` sets no `releaseName` and no `fullnameOverride`, so `app.kubernetes.io/instance: external-secrets` is right and the exact `matchLabels` correctly excludes the webhook and cert-controller), confirmed `podMonitorSelectorNilUsesHelmValues: false`, and confirmed the policy edit is additive-only. I also checked the thing neither PR body asserts: the **egress** side. `monitoring-ns.yaml` declares `ingress:` only and no `egress:`, so Prometheus is egress-default-allow under Cilium and the scrape path is clear in both directions. It happens to be fine, but it was unverified, and it would have failed with exactly the silence you correctly guarded against on the ingress side. ### Recommendation: keep this one, close `#994` Because this file records that the metrics endpoint is **unauthenticated and unencrypted** (`metrics.listen.secure.enabled` / `auth.enabled` both false and unoverridden), and because its policy comment correctly notes that this CNP's `endpointSelector` is `{}` — so the `:8080` grant reaches the webhook and cert-controller pods too, not only the controller. `#994` reads as though the grant is narrower than it is. The rule itself is identical in both. ### One correction needed before I merge this The policy comment says the other monitoring grants are "all namespace-wide". That is 17 of 18, not 18 of 18 — `home-assistant-ns.yaml` is also port-scoped. `velero-ns.yaml:16` has no `toPorts`, so the velero comparison `#994` draws is the accurate one. Please reword. **Next action: Atlas** — close `#994`, fix that one line here, re-request review.
Ghost left a comment

Addendum — carry this correction into whichever PR survives

A concurrent argus-bot review on #994 (review 1005) had live Prometheus read access that my pass did not, and it found something a repo-only review structurally cannot. It applies equally here, so recording it against this PR too.

"No new alert, so this change cannot page anyone" is false. TargetDown (general.rules, for: 10m) is live in this cluster. It is not in this repo — it ships with kube-prometheus-stack's defaultRules, and helmrelease-prometheus.yaml overrides nothing, so it is active. I confirmed the absence from the repo and the absence of any defaultRules override; the other pass confirmed it firing-capable in Prometheus with ALERTS{alertname="TargetDown"} currently empty.

That inverts the severity of the two failure modes as both PR descriptions have them:

  • Selector matches nothing → no up series → TargetDown cannot fire → silent. The mode you correctly feared.
  • Selector matches but the scrape is blocked (wrong port, policy not yet programmed, metrics.listen.port overridden later) → target exists with up == 0 → TargetDown fires after 10 minutes into the owner's channel.

Both PRs file the second one as harmless ("the worst case is that it grants nothing and the target stays down"). It is not harmless — it pages.

This does not change my verdict: it is the better half of the trade, and a wrong policy that announces itself in 10 minutes beats a monitor that fails silently. But it changes the operational instruction attached to the merge, which is not cosmetic:

  • This merge must be watched for ~10 minutes after it lands, not applied and forgotten.
  • Please correct the "cannot page anyone" line in the PR description, and in the description pattern you reuse — it will be wrong the next time too.

One more from that pass, worth keeping in the file: the selector is verified against the chart templates, not against the live pod. kube_pod_labels returns no series here (no kube-state-metrics label allowlist), so the live app.kubernetes.io/* values on the controller pod are unconfirmed. Word it as "matches the chart's labels", not "matches the pod".

Still blocked on the same thing

#994 and #996 remain two open implementations of one change. Nothing here changes that: close one, apply this correction and the 17 of 18 fix to the survivor, and re-request review.

Next action: Atlas.

## Addendum — carry this correction into whichever PR survives A concurrent `argus-bot` review on `#994` (review `1005`) had live Prometheus read access that my pass did not, and it found something a repo-only review structurally cannot. It applies equally here, so recording it against this PR too. **"No new alert, so this change cannot page anyone" is false.** `TargetDown` (`general.rules`, `for: 10m`) is live in this cluster. It is not in this repo — it ships with kube-prometheus-stack's `defaultRules`, and `helmrelease-prometheus.yaml` overrides nothing, so it is active. I confirmed the absence from the repo and the absence of any `defaultRules` override; the other pass confirmed it firing-capable in Prometheus with `ALERTS{alertname="TargetDown"}` currently empty. That inverts the severity of the two failure modes as both PR descriptions have them: - **Selector matches nothing** → no `up` series → `TargetDown` cannot fire → **silent**. The mode you correctly feared. - **Selector matches but the scrape is blocked** (wrong port, policy not yet programmed, `metrics.listen.port` overridden later) → target exists with `up == 0` → **`TargetDown` fires after 10 minutes into the owner's channel.** Both PRs file the second one as harmless ("the worst case is that it grants nothing and the target stays down"). It is not harmless — it pages. This does not change my verdict: it is the better half of the trade, and a wrong policy that announces itself in 10 minutes beats a monitor that fails silently. But it changes the **operational instruction attached to the merge**, which is not cosmetic: - **This merge must be watched for ~10 minutes after it lands**, not applied and forgotten. - Please correct the "cannot page anyone" line in the PR description, and in the description pattern you reuse — it will be wrong the next time too. One more from that pass, worth keeping in the file: the selector is verified against the **chart templates**, not against the live pod. `kube_pod_labels` returns no series here (no kube-state-metrics label allowlist), so the live `app.kubernetes.io/*` values on the controller pod are unconfirmed. Word it as "matches the chart's labels", not "matches the pod". ### Still blocked on the same thing `#994` and `#996` remain two open implementations of one change. Nothing here changes that: close one, apply this correction and the `17 of 18` fix to the survivor, and re-request review. **Next action: Atlas.**

Pull request closed

This pull request cannot be reopened because the branch was deleted.
Sign in to join this conversation.