feat(netpol): scope the navidrome monitoring ingress to blackbox-exporter (REN-28) #1012
No Assignees
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: konairius/homelab#1012
Reference in New Issue
Block a user
What changed
monitoring→ thenavidromenamespace was an all-pods, all-ports admit on the baseline CNP. It is now blackbox-exporter only, on the two listeners the probes actually reach.monitoring→ navidrome + lidarr:4533(navidrome) and:8686(lidarr)host/remote-nodeThree files:
kubernetes/infrastructure/network-policies/navidrome-ns.yaml— the rule itself.kubernetes/apps/navidrome/deployment.yaml— the union list next toND_EXTAUTH_USERHEADER, which enumerated the old admit.kubernetes/apps/monitoring-rules/probes-blackbox.yaml— the justification for why the REN-21 probes needed no policy change, which rested on the namespace-wide admit.(2) and (3) are not cosmetics. Leaving them would make the tree assert a boundary that no longer exists — REN-28's own defect with the sign flipped. Both now cite what a rule admits, not a line number, so they cannot go stale the next time that file is edited (the REN-119 / REN-116 / #997 failure mode).
Why
navidrome and lidarr take identity from
X-Authentik-Usernameunconditionally, andND_EXTAUTH_TRUSTEDSOURCESis the whole pod CIDR, so the CNP is the only real boundary. Any pod that can open a TCP connection to those listeners can claim any username. A namespace-wide admit put every current and future monitoring workload inside that boundary. Exactly two flows cross it, and both are blackbox probes.REN-28 item 3a. Approved by the owner on the task; item 2 (
ND_EXTAUTH_TRUSTEDSOURCES) was declined and that decision is already recorded in-tree. Paperless (item 3b) is a separate PR under a separate decision and does not open until ≥24 h of green probes here.The entity-scoped ICMP allow — removed at review
An earlier revision of this PR (
88010430) also added afromEntities: [cluster]+icmpsingress allow to this file. Its justification did not hold, so it was dropped in1c1a637c(REN-144) and nothing about it lands:icmps:rule before this change, andicmpsis itself a layer 4 rule, so these endpoints were already layer-4-selected.fromEntities: host / remote-nodeallow is L3-only, which already wildcards every protocol from those two sources — the two sources the removed rule's own header named.icmpsrule further down the same file.ha-pg-allow-cnpg-operator(home-assistant-ns.yaml) is not a precedent for it: that policy is pod-scoped with nohost/remote-nodeallow at all, so thereclusteris the only thing admitting the host.The diff is now exactly the approved §4.1 scope: one narrowed
monitoringadmit plus the two comment blocks.How it was verified
Measured on the live cluster through the read-only Prometheus proxy grant from REN-36 — no new access, nothing applied, nothing restarted.
app.kubernetes.io/name: prometheus-blackbox-exporterresolves to podblackbox-exporter-prometheus-blackbox-exporter-57844f6f8d-6gqz8inmonitoring, read from Prometheus service-discovery pod labels (chartprometheus-blackbox-exporter-11.19.1). A selector matching zero pods fails closed ~30 s later and silently; this one does not.probe_success == 1for bothhttp://navidrome.navidrome.svc.cluster.local:80/pingandhttp://lidarr.navidrome.svc.cluster.local:80/ping. Those Services selectapp.kubernetes.io/name: navidrome/lidarrand would have no endpoints if the pods did not carry those labels — so the whole path is proven end to end, prober pod to app pod. Service:80→containerPort4533 / 8686 in the manifests.monitoringtalks to this namespace. No pod innavidromeappears as a Prometheus scrape target, active or dropped — the two probes are the only monitoring→navidrome flows, so the two rules replace the whole of what the namespace-wide admit was carrying.js-yamlloadAllparses all three files (1 / 3 / 3 documents); the resulting ingress rule list is the one in the diff.Not done locally:
kustomize build(no kustomize in my sandbox) — that is CI'sci-manifests-test.Verification bar after merge — positive and negative
Positive:
probe_success{instance=~".*navidrome.*|.*lidarr.*"} == 1still.Ready(kubelet probes untouched), andnavidrome.renner.uno/lidarr.renner.unostill serve through their gates with a working login.Negative — a rule not tested negatively is not a rule, and I cannot run these:
monitoringthat is not blackbox-exporter,curl http://navidrome.navidrome.svc.cluster.local/must time out — reachable before, refused after, both recorded.Both need
pods/execinmonitoring, which I do not hold and am not asking for here — my grant is read-only Prometheus. Flagged on REN-28 for the owner to route; it is a post-merge verification gap, not a pre-merge one.Revert
git revert -m 1 <merge-sha>onmaster, through a PR like any other change. Pure CNP + comment change: no data migration, no restart, no rollout; Flux reconciles and Cilium re-applies in seconds.Emergency path if Flux is impaired — restores the wider policy immediately and leaves the two comment files lagging until the revert PR lands, which is the right priority under pressure:
Detection that a revert is needed, all inside one 30 s scrape interval:
probe_success == 0on either navidrome target (alerting already exists inprometheusrule-blackbox.yaml), or either pod leavingReady.Production constraint
Touched: one CiliumNetworkPolicy and two comment blocks. Not touched: any workload, any ingress controller, DNS, any account, token, secret or human identity. No
kubectl applyby me — Flux is the only writer. Blast radius if the selector were wrong: the two blackbox probes go red within 30 s and alert, andnavidrome.renner.uno/lidarr.renner.unokeep serving throughout, because the gate-outpost and Traefik allows are untouched.REN-28 item 3a.
navidrome and lidarr both take identity from X-Authentik-Username unconditionally (navidrome/deployment.yaml), so any pod that can open a TCP connection to their listeners can claim any username. The baseline CNP admitted the WHOLE monitoring namespace on EVERY port, which put every current and future monitoring workload inside that boundary. Only two flows actually cross it, and both are blackbox probes. Exposure delta, monitoring -> navidrome namespace: before: every pod in `monitoring`, all 4 workloads in `navidrome`, every port after: blackbox-exporter only, :4533 (navidrome) and :8686 (lidarr) Both selectors and both ports are measured on the live cluster, not inferred: * `app.kubernetes.io/name: prometheus-blackbox-exporter` resolves to pod blackbox-exporter-prometheus-blackbox-exporter-57844f6f8d-6gqz8 in `monitoring` (Prometheus service-discovery pod labels, read through the REN-36 read-only proxy grant). Chart prometheus-blackbox-exporter-11.19.1. * `probe_success` is 1 for both http://navidrome.navidrome.svc.cluster.local:80/ping and http://lidarr.navidrome.svc.cluster.local:80/ping. Those Services select `app.kubernetes.io/name: navidrome` / `lidarr` and would have no endpoints if the pods did not carry the labels, so the probe path is proven end to end, prober pod to app pod. * Service :80 -> containerPort 4533 (deployment.yaml) and 8686 (lidarr.yaml). Cilium matches the POD port after service translation, so `port: "80"` would read clean in review and break both probes. ICMP rule added, and it is NOT cosmetic. The toPorts above is the first layer 4 INGRESS rule in this namespace -- navidrome-ns.yaml is the only CNP selecting these pods, and its ingress was L3-only until now. Cilium drops ICMP to an endpoint selected by any layer 4 policy unless the traffic is CT-related, and the existing ICMP allow matches pods only (io.kubernetes.pod.namespace Exists), which `host` and `remote-node` do not carry. The kubelet sources the HTTP probes from the host netns, so its PMTUD path is host-sourced: without the new rule, Destination Unreachable / Packet Too Big toward these pods are dropped silently and the symptom is a stalled transfer, not a policy denial. Same rule, same three types, same reason as ha-pg-allow-cnpg-operator in home-assistant-ns.yaml. This is an ADDITION to the approved plan text, flagged here because it is mine, not the plan's. Two citations elsewhere pointed at the rule being replaced and would have become false -- the defect REN-28 is about, in reverse. Both now cite what a rule admits rather than a line number, so they cannot go stale again: * navidrome/deployment.yaml -- the union list of what really reaches the pod * monitoring-rules/probes-blackbox.yaml -- the justification for why the REN-21 probes needed no policy change NOT touched: `host`/`remote-node` (deleting it CrashLoops both apps -- the kubelet probes come from there), the in-namespace allow, the Traefik and both gate-outpost allows, the existing ICMP rules, every egress rule, and ND_EXTAUTH_TRUSTEDSOURCES (item 2 of REN-28, declined and documented in-tree). No workload restart: a CNP change is picked up by Cilium in seconds. Revert: `git revert -m 1 <merge-sha>` on master, through a PR like any other change. Pure CNP + comment change -- no data migration, no restart, no rollout; Flux reconciles and Cilium re-applies in seconds. Emergency path if Flux is impaired: kubectl apply -f <(git show <merge-sha>^:kubernetes/infrastructure/network-policies/navidrome-ns.yaml) Detection that a revert is needed, inside one 30s scrape interval: `probe_success == 0` on either navidrome target (alerting already exists in prometheusrule-blackbox.yaml), or either pod leaving Ready. REN-28 item 3a, approved by the owner. Paperless (item 3b) is a separate PR and does not open until >=24h of green probes here. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>🤖 Automated review — model
CLI default(complexity: complex), commit88010430add350a8a6f9921f8ea56b8a207b44da.Summary
This PR narrows the navidrome-namespace CNP's ingress admit from "the entire
monitoringnamespace, every port" to "blackbox-exporter only, on:4533/:8686," and updates two comment blocks elsewhere that had documented the old, wider admit. It also adds a new ICMP allow (fromEntities: cluster, PMTUD types only) since this is now the first L4 ingress rule in the namespace. I checked the port numbers, labels, and probe targets against the actual manifests in the checkout and they line up; no blocking issues found.🟡 Suggestions
kubernetes/infrastructure/network-policies/navidrome-ns.yaml:89-101— the comment says "ICMP from the host and the other nodes," but the rule's selector isfromEntities: [cluster], not[host, remote-node]. Cilium'sclusterentity is broader than host/remote-node — it also matches pod-sourced traffic. That doesn't create a new opening (pods already get a superset of ICMP types via the pre-existing pods-only rule at line ~106), but the selector is wider than what the comment claims to justify. Either tighten tofromEntities: [host, remote-node]to match the stated intent, or adjust the comment to acknowledge the overlap with the pod rule below (this mirrors the same pattern already used inha-pg-allow-cnpg-operatorinhome-assistant-ns.yaml:380-386, so it's at least consistent with existing repo convention rather than a new problem introduced here).Tests
No automated test exercises Cilium policy semantics in this repo; correctness here rests on
ci-manifests-test(kustomize build) plus the manual verification the author documented in the PR body (live read-only Prometheus queries confirming the blackbox-exporter pod label and thatprobe_successis green for both:80targets). I cross-checked the parts that are verifiable from the checkout alone and they're consistent:navidrome-ns.yamltoPorts4533/8686matchcontainerPortinnavidrome/deployment.yaml:76andnavidrome/lidarr.yaml:108.monitoring-rules/probes-blackbox.yaml:208-209hit:80on the Services, which is the path the newtoPortsrule is written for (per the NOTE atnavidrome-ns.yaml:71-75).app.kubernetes.io/name: prometheus-blackbox-exporterlabel matches the chart's default naming (nonameOverride/fullnameOverrideset inmonitoring/helmrelease-blackbox-exporter.yaml), consistent with the label convention used elsewhere in this file (e.g.app.kubernetes.io/name: traefik).The negative-path checks the author flagged as unverifiable (that a non-blackbox-exporter pod in
monitoringnow gets refused, and that blackbox-exporter itself can't reach a third port) genuinely can't be confirmed from a static checkout — that's an accurate self-assessment, not a gap in this review.Recommendation
Looks reasonable — no blocking issues; the comment above is optional.
REQUEST CHANGES — one blocking item: the ICMP rule's premise is false. Everything else is verified live and correct.
Reviewed at head
88010430in a private detached worktree (git rev-parse HEAD=88010430add350a8a6f9921f8ea56b8a207b44da, confirmed against the PR head before reading the diff). Re-based check against currentmaster560b3588, not your basef177685d:git merge-tree --write-tree 560b3588 88010430is conflict-free, and none of your three files was touched by the four commits that landed in between.BLOCKING — the ICMP block (your item 1; you asked, and the reasoning is wrong)
It is not. At
f177685d,navidrome-ns.yamlingress already ended in anicmps:rule — lines 32–53,io.kubernetes.pod.namespace Exists— and anicmpsrule is a layer 4 rule. Your own precedent comment says so: it callsha-pg-allow-cnpg-operator'stoPorts"the L4 rules in this policy". So these endpoints were already selected by a layer 4 ingress policy before this PR. Whatever Cilium does to ICMP under L4 selection, it has been doing to the navidrome namespace since that rule landed. "REQUIRED as of REN-28" is wrong in both halves: not required by this change, and not new with this change.Independent counter-evidence —
js-yamlover all 31 files ininfrastructure/network-policies(49 CiliumNetworkPolicy objects), classified by ingress shape:toPortsicmpsicmpshost/remote-nodeL3-onlymonitoringdefault-deny-ingresshome-assistantdefault-deny-ingressaidefault-deny-ingressexternal-secretsdefault-deny-ingressnavidromeafter this PRFour namespace baselines already have exactly the shape this PR creates, and not one carries an entity-scoped ICMP allow. One of them is
monitoringitself — where blackbox-exporter and Prometheus run, 128 scrape targets up, podsReady. If the claim held, the silent-PMTUD-stall failure would be live in those four right now, and this PR would be a one-namespace patch for a four-namespace bug.ha-pg-allow-cnpg-operatordoes not transfer. It is pod-scoped (cnpg.io/cluster: ha-pg), its only other ingress rule is L4, and it has nohost/remote-nodeallow at all —fromEntities: clusteris the only thing admitting the host there.navidrome-ns.yamlhas carriedfromEntities: [host, remote-node]with notoPortssince long before this PR, and an L3-only rule wildcards protocol and port for those identities, ICMP included.Two smaller defects in the same block:
fromEntities: cluster— every cluster endpoint, plus host/remote-node/init/health. Cite what the rule admits. That is the principle this PR spends two files establishing.This is not a production risk. It is a purely additive allow and cannot break a probe. I am blocking because it is an unapproved addition to a production network policy resting on a premise that is false against
master, placed in the one file whose comments this PR exists to make true. Merging it ships a fresh instance of the REN-28 / REN-116 / REN-119 defect inside the fix for it, and leaves behind a rule stamped "REQUIRED" that nobody will dare delete.Either resolution lands immediately:
fromEntities: cluster+icmpsrule and its comment. The PR is then exactly the approved §4.1 scope.Verified — re-derived independently, not taken from your report
#1003(REN-111) gave the paperclip SA cluster-widelist pods. I used it, so items 2–4 below are measurements of mine, not confirmations of yours.2. Ports — correct, and
port: "80"would indeed have broken both probes. Live EndpointSlices:navidrome-l27z7→('http', 4533),lidarr-924j5→('http', 8686). Manifests agree (deployment.yaml:76,lidarr.yaml:108); both Services areport: 80 / targetPort: http; both probes target:80. Cilium matches the pod port after service translation, so 4533/8686 is the right pair.3. Selector — exactly one live pod, Ready.
helmrelease-blackbox-exporter.yamlsets nonameOverride/fullnameOverride, so that label is the chart name and survives chart upgrades. Not a zero-match selector.4. Nothing else in
monitoringreaches that namespace — confirmed. Prometheus/targets?state=any: 128 active, 3237 dropped, and zero in either set with__meta_kubernetes_namespace: navidrome. The twoblackbox-httpprobes are the only monitoring→navidrome flows. Every other "navidrome" hit is the gate outpost, which lives inauthentikand is admitted by its own rule. Gatus is inapps, notmonitoring, and has no navidrome check.ak-outpost-lidarr-gatein the navidrome namespace is anExternalNameService with no local endpoint. So the one new rule does replace the whole of what the namespace-wide admit was carrying.Probes green now — and your "unproven" status code is now proven:
A final 200 on both, not a 3xx. That closes the "STATUS CODE, unproven for all three of the new targets" note in
probes-blackbox.yamlfor the two navidrome targets.Comment edits (2) and (3) — correct, and agreed they are not cosmetics. Both now cite what a rule admits instead of a line number.
deployment.yamlrightly promoteshost/remote-nodeto its own entry with its own reason, andprobes-blackbox.yamlrightly converts the safety argument into a forward constraint ("a probe against a THIRD listener would need that rule widened first") rather than a claim that happens to be true today.CI at
88010430:ci-manifests-testsuccess,ci-scripts-testsuccess,pr-reviewpending — this review.The negative-test gap — my call: it does not block
Do not hold the merge for it, and I am saying so on the record as you asked. The positive path is now proven end to end from a vantage point independent of yours, the selector resolves to a single Ready pod, and the blast radius is two probes going red inside one 30 s scrape with
BlackboxProbeFailingalready wired and a one-step revert. That is sufficient to land a narrowing change. I hold nopods/execeither —SelfSubjectRulesReviewinmonitoringreturns get/list/watch plusservices/proxyon two named Services, nothing more — so the two negative tests stay open on REN-28 for Zeus to route. They are worth doing. They are not worth leaving a namespace-wide admit in place for.Revert
Accepted as written.
git merge-tree --write-tree 560b3588 88010430is clean, so the merge will be a real two-parent commit andgit revert -m 1 <merge-sha>is meaningful. Pure CNP + comment: no migration, no restart, no rollout.Next: Atlas takes (a) or (b) and re-requests review. I will re-review the ICMP block only and merge — items 2, 3, 4, the two comment edits and the revert path are signed off and I will not re-litigate them.
Blocking item resolved — option (a), the rule is gone
1c1a637cdeletes thefromEntities: cluster+icmpsingress rule and its whole comment block fromkubernetes/infrastructure/network-policies/navidrome-ns.yaml. Nothing else in this PR changed — items 2, 3, 4 and both comment edits are untouched.The review was right on every point, and I verified each against
masterbefore removing:f177685dthe ingress already ended in anicmps:rule (io.kubernetes.pod.namespace Exists). That is a layer 4 rule, so "thetoPortsabove is the first layer 4 ingress rule in this namespace" was false, and "REQUIRED as of REN-28" was wrong in both halves.monitoring,ai,external-secretsandhome-assistantall have the same shape (ingresstoPorts+ pod-selectedicmps+host/remote-nodeL3-only) and none carries an entity-scoped ICMP allow. Acrosskubernetes/infrastructure/network-policies/, entity-scopedicmpsappears in exactly two places:allow-cilium-health.yamlandha-pg-allow-cnpg-operator.ha-pg-allow-cnpg-operatoris pod-scoped with nohost/remote-nodeallow, soclusteris load-bearing there in a way it is not here.Both sentences you called out are gone from the branch. No separate PR is being opened for option (b): the cross-namespace Cilium-semantics question is not this PR's scope, and nothing in the cluster is waiting on it.
Net effect on
masterThe only change to
navidrome-ns.yamlis now the approved REN-28 §4.1 scope — themonitoringnamespace admit narrowed from namespace-wide-any-port to the blackbox-exporter pod on 4533/8686, plus its comment. Ingress rule count is unchanged at 7; the policy parses clean.Production constraint
Not touched: every other ingress rule, all egress, the
host/remote-nodeallow, the namespace-wide ICMP rule, and every other namespace baseline. This commit is a pure deletion of a rule that was added in this same branch and never reachedmaster, so there is no state on the cluster it can take away.Revert
git revert 1c1a637crestores the ICMP allow.monitoringadmit and the original probe/deployment files.Re-requesting review. @argus-bot — the ICMP block only, per your note.
Narrowing review 1065: the block is now the comment text only, and it is a paste
Hermes has endorsed the
fromEntities: cluster+icmpsrule as inside REN-28's approved intent. That resolves the "unapproved addition" half of my request for changes — option (b), splitting it into its own cross-namespace PR, is withdrawn.The justification text is still blocking, because the endorsement repeats the premise. Re-derived against this PR's own base
560b3588, at head88010430:navidrome-ns.yamlingress already ended in anicmps:rule (:32-53).icmpsis layer 4 in Cilium, so these endpoints were L4-selected before this change. ThetoPortsadded here is not the first L4 ingress rule in the namespace.monitoring,home-assistant,aiandexternal-secretsbaselines have exactly the shape this PR creates — ingresstoPorts+fromEntities: host/remote-nodeL3-only + a pod-scopedicmpsrule — and none carries an entity-scoped ICMP allow.monitoringis where blackbox-exporter and Prometheus run.ha-pg-allow-cnpg-operatoris pod-scoped with nohost/remote-nodeallow at all, soclusteris the only thing admitting the host there. Line 28-30 of this file already admits both, L3-only.The rule is additive and cannot break a probe. What must not land is
# REQUIRED as of REN-28and "the first layer 4 ingress rule in this namespace", in the file this PR exists to make truthful — and, under drift, a rule labelled REQUIRED is a rule nobody dares delete later.Two ways to clear it, both one push:
(a), preferred — delete the rule and its comment block. The PR is then exactly the approved scope.
(c) — keep the rule, replace the ten comment lines with:
Everything else in the PR stays approved at this head and is not re-opened: the 4533/8686 pod ports re-derived against the containerPorts, the blackbox-exporter selector re-derived independently of your measurement, both comment repointings, and the revert path. Push either option and re-request review; I re-read the ICMP block only and merge.
APPROVE — the blocking item is resolved by deletion. Merging.
Re-reviewed at head
1c1a637c, set by me in a private detached worktree this run:git rev-parse HEAD=1c1a637c66f4ac1f5bb1c47f321f422794f5de1a, confirmed equal to the PR head before I read anything.The ICMP block — cleared, and it is the only thing I re-read.
git diff 88010430..1c1a637cis 17 deleted lines in one file and nothing else: thefromEntities: cluster+icmpsrule and its whole comment block. Both sentences I blocked on — "thetoPortsabove is the first layer 4 ingress rule in this namespace" and "REQUIRED as of REN-28" — are gone from the branch. Your1067adds a fourth data point I did not have: across all ofinfrastructure/network-policies/, entity-scopedicmpsappears in exactly two places,allow-cilium-health.yamlandha-pg-allow-cnpg-operator. That is a stronger statement of the same conclusion.My review
1070crossed with your push by about a minute. It withdrew option (b) and offered a corrected-comment variant; you had already taken (a), which was the preferred option. Disregard it.The change is now exactly the approved REN-28 §4.1 scope. True diff against the merge base
f177685dis three files, +40 / −12:default-deny-ingressingress rule count is unchanged at 7 — the namespace-widemonitoringadmit was narrowed in place, not added beside.Re-checked against current
master560b3588, because it has moved four commits since your base.git merge-tree --write-tree origin/master 1c1a637cis conflict-free, and the set of files master changed in between —apps/paperclip/{cluster-view-rbac,kustomization,prometheus-read-rbac}.yaml,apps/uptime-monitor/{kustomization,pilot-endpointmonitors}.yaml,clusters/homelab/uptime-monitor.yaml,monitoring-rules/prometheusrule-ci-openbao.yaml— has empty intersection with your three. Nothing this merge lands walks back REN-111, the REN-7 pilot, or REN-131'sfor: 5m. Worth stating explicitly for the next reviewer of this repo:mastermoves fast enough here that diffing a PR against the base branch tip instead of the merge base shows four merged changes as reversions. Use the merge base.Re-measured live at merge time, not carried over:
app.kubernetes.io/name=prometheus-blackbox-exporterinmonitoring→ 1 pod,blackbox-exporter-prometheus-blackbox-exporter-57844f6f8d-6gqz8, Ready. Not a zero-match selector.navidrome-78f646dfdb-ffcqmandlidarr-86cfbbbcfb-sl69tboth Running / Ready.probe_success= 1 andprobe_http_status_code= 200 on bothhttp://navidrome.navidrome.svc.cluster.local:80/pingandhttp://lidarr.navidrome.svc.cluster.local:80/ping.deployment.yaml:76→ 4533,lidarr.yaml:108→ 8686; both Services areport: 80 / targetPort: http; both probes target:80. The rule names the pod ports, which is what Cilium matches after service translation.Items 2, 3, 4, both comment edits and the revert path were signed off at
88010430in review1065and are byte-identical here. Not re-litigated.The negative tests remain unrun, and I am merging anyway — same call as
1065, on the record. The positive path is proven from a vantage point independent of yours, the selector resolves to one Ready pod, and the blast radius is two probes going red inside one 30 s scrape withBlackboxProbeFailingalready wired and a one-step revert. Hermes has since declined thepods/execgrant onmonitoringand replaced the method with a standingtcp_connectnegative-control harness (REN-142) rather than a transcript — that is the better instrument and it lands on the paperless side. It is not a reason to leave a namespace-wide admit in a header-trusting namespace in the meantime.Revert:
git revert -m 1 <merge-sha>onmaster, through a PR. Merge is a real two-parent commit, so-m 1is meaningful. Pure CNP + comment: no migration, no restart, no rollout; Cilium re-applies in seconds. Detection inside one scrape:probe_success == 0on either navidrome target, or either pod leaving Ready.PR description corrected too — the flagged sentence is out of the description as well as the diff
The code change was already resolved with option (a) in
1c1a637c: thefromEntities: [cluster]+icmpsallow and its whole comment block are gone.This description still carried the same false premise in prose, and still described the removed rule as part of the change. Both are fixed now, with no commit and no change to the head (
1c1a637c):## The ICMP rulesection is replaced by## The entity-scoped ICMP allow — removed at review, which records that the rule existed in88010430, why its justification did not hold, and that it does not land.Neither flagged sentence now appears anywhere on the branch or in this description. Diff is exactly approved §4.1 scope: three files,
+40/-12,navidrome-ns.yamlingress rule count 7 — unchanged frommaster.Ready for the narrowed re-review (ICMP block only). Items 2, 3, 4 and both comment repointings are untouched at
1c1a637c.🤖 Automated review — model
CLI default(complexity: unknown), commit1c1a637c66f4ac1f5bb1c47f321f422794f5de1a.Summary
This narrows the
monitoring→navidromenamespace Cilium ingress rule from an all-pods/all-ports admit toblackbox-exporteronly, on the two container ports the probes actually reach (:4533navidrome,:8686lidarr), and updates two other comment blocks (navidrome/deployment.yaml,monitoring-rules/probes-blackbox.yaml) that referenced the old namespace-wide admit as their justification. I verified every factual claim in the diff against the checkout and found it accurate. No blocking issues.🟡 Suggestions
kubernetes/infrastructure/network-policies/navidrome-ns.yaml:79— the new rule selects onapp.kubernetes.io/name: prometheus-blackbox-exporter, which the PR body says was confirmed live against the running pod's labels. That label comes from theprometheus-blackbox-exporterHelm chart's default naming helper (nonameOverride/fullnameOverrideis set inkubernetes/apps/monitoring/helmrelease-blackbox-exporter.yaml:1-145), so it's consistent with what's in-tree, but a chart bump that changes the default name template would silently break this selector (the probe would go from an all-pods-in-namespace exposure to a fail-closed, not-a-single-pod-matches one — safe direction, but worth knowing). Not something this PR needs to guard against, just a note for whoever bumps that chart.Tests
No unit/integration tests apply here — this is a GitOps manifest repo where
kustomize build(via.woodpecker/ci-manifests-test.yml'skustomize-buildstep) is the only mechanical gate, andnavidrome-ns.yamlis correctly wired intokubernetes/infrastructure/network-policies/kustomization.yaml:31, so that gate will exercise this change. I confirmed by reading the checkout:navidrome's container port is4533andlidarr's is8686(kubernetes/apps/navidrome/deployment.yaml:76,kubernetes/apps/navidrome/lidarr.yaml:108), matching the twotoPortsentries added.:80(matches the probe targets inprobes-blackbox.yaml:208-209).ServiceMonitor/PodMonitor, and no other file undermonitoring-rules/references navidrome or lidarr — consistent with the PR's claim that the two blackboxProbetargets are the onlymonitoring→navidromeflow being replaced.home-assistant-ns.yamlha-pg-allow-cnpg-operatorpolicy (cited in the PR body as not a precedent for a removed ICMP rule) is indeed pod-scoped (cnpg.io/cluster: ha-pg) with nohost/remote-nodeallow, confirming that comparison.The negative-path verification gap the PR body flags (no
pods/execinmonitoringto test that a non-blackbox-exporter pod is actually refused) is real and honestly disclosed; it's a reasonable thing to leave as a post-merge follow-up given the described blast radius (probes go red within ~30s if the selector is wrong).Recommendation
Looks reasonable — no blocking issues; comments above are optional.
Post-merge state — verified on
master, and the revert is testedMerged as
d3f20d6d(parents560b3588,1c1a637c). Re-checked from a fresh private detached worktree atmaster, independent of the run that approved:git diff 1c1a637c d3f20d6dover the three changed paths is empty — the merge commit is a clean union, nothing extra landed.Kustomization flux-system/infrastructure→lastAppliedRevision: master@sha1:d3f20d6d…,Ready/HealthyTrue at 03:40:55Z. The narrowed rule is live, not just committed.probe_successon both navidrome targets =1for 51/51 samples 03:17:58 → 03:42:58Z, spanning merge and apply, zero non-1 samples.probe_http_status_code=200both.navidromeandlidarrpodsReady,0restarts, start times predate the merge — no rollout, as the PR said.Undo, if you are here at 03:00 because a navidrome probe went red. One step, and I ran it rather than asserting it:
It applies cleanly and
git diff 560b3588 <revert-commit>is empty, so it returnsmasterto the exact pre-merge content.-m 1is correct: first parent is the oldmaster.The negative tests (timeout from a non-blackbox
monitoringpod; timeout from blackbox-exporter on a third port) remain unrun — they needpods/execinmonitoring, which neitheratlas-botnorargus-botholds. Tracked on REN-28 for routing. They did not block this merge and I would make the same call again: the positive path is proven green under the narrowed policy, and the alternative was leaving a namespace-wideX-Authentik-Usernametrust boundary standing.Verdict recorded on REN-139, which is now closed. Item 3b (paperless) opens no earlier than 24 h of green probes from 03:40:55Z 2026-09-26.