feat(woodpecker): stage 1 — restore wp-pg into a dedicated namespace #504

Merged

k.renner merged 5 commits from migrate-woodpecker-namespace into master 2026-08-18 20:56:43 +00:00

Conversation 9 Commits 5 Files Changed 10

+575

claude-bot commented 2026-08-18 20:14:59 +00:00

Member

Copy Link

Copy Source

Stage 1 of moving Woodpecker out of the shared apps namespace. Addresses #433.

No cutover, no CI impact. This stands up the woodpecker namespace and restores wp-pg into it from barman, running in parallel with the live apps/wp-pg. Nothing points at it yet. Rollback is kubectl delete ns woodpecker plus reverting this PR.

Why move rather than patch the policy

wp-pg has been Ready=False with Instance Status Extraction Error for three weeks. The cause is concrete — from the operator's own log:

Get "https://10.244.0.37:8000/pg/status": dial tcp 10.244.0.37:8000: i/o timeout

A dropped connect, not an HTTP-level fault. The issue's guidance to skip NetworkPolicy is wrong — apps does have a default-deny-ingress, 101 days old, and it lists apps, home-assistant, host and remote-node but not database. Every other namespace running a CNPG cluster (authentik, immich, monitoring, paperless, trmnl, wordpress) already allows database; apps is the only one that doesn't, which is exactly why one of six clusters is broken.

I first fixed it by adding that allow. That was the wrong fix and is not in this PR: apps uses endpointSelector: {}, so the rule would also expose gitea, gatus, external-dns and ical-proxy to the whole database namespace — to solve a problem affecting one pod on one port. That is the same shape home-assistant-ns.yaml documents having to undo, where a blanket fromEntities: world exposed a whole namespace.

The same gap explains a second symptom nobody had noticed: cnpg_collector_up has series for authentik, immich, monitoring and paperless but not wp-pg, because apps never allowed the monitoring namespace either. Both are fixed structurally by the move.

woodpecker-ci is deliberately not the target. It enforces pod-security: privileged and exists for ephemeral step pods running repo-supplied code. Putting the database there would let any build step reach it via the intra-namespace allow.

What's here

File
infrastructure/namespaces/namespaces.yaml	the woodpecker Namespace
network-policies/woodpecker-ns.yaml	baseline + a scoped wp-pg-allow-cnpg-operator (operator → :8000, monitoring → :9187)
migration-objectstore.yaml	recovery source + a separate archiving target
migration-cluster.yaml	wp-pg bootstrapped via recovery
migration-externalsecret-hetzner.yaml	barman creds in the new namespace
MIGRATION-namespace.md	staging, design notes, verification, rollback

Three decisions worth reviewing

Two ObjectStores. wp-pg-store-source reads the existing s3://renner-uno-k8s-cnpg/wp-pg; wp-pg-store archives to a new wp-pg-woodpecker prefix. Two clusters writing one destinationPath would interleave their WAL streams and corrupt both. Splitting them also keeps the pre-migration backup intact and independently restorable.

The Namespace is declared in infrastructure/namespaces, not beside the workloads. Its network policies ship with the infrastructure KS, and the woodpecker KS dependsOn infrastructure — creating it in the workload KS would invert that order and deadlock a cold rebuild. (Dry-run caught this: the policies failed with namespaces "woodpecker" not found.) Note woodpecker-ci has the same latent inversion today, created in rbac.yaml; not touched here.

No prune: disabled on the restored cluster. The original carries one because it holds the only copy of the data. This one is reconstructable by re-running the restore, and a prune guard on a migration artifact is how orphans outlive an abandoned migration. It gains the guard in stage 3.

#397 could not be folded in

I said it would be — it can't. Creating seed/woodpecker/hetzner-creds needs OpenBao write access that isn't available (bao kv list seed → 403). Repointing at a path I can't verify exists would leave the restored cluster unable to archive WAL, which is the thing this stage exists to prove. It stays on seed/paperless/hetzner-creds; #397 remains open and is a one-line swap once the path exists.

Verification

• kubectl kustomize builds clean for apps/woodpecker, infrastructure/namespaces, infrastructure/network-policies.
• kubectl apply --dry-run=server accepts all of it; the rendered set targeting woodpecker is exactly the four intended resources, and nothing in apps is modified.
• Recovery stanza shape verified against the plugin-barman-cloud docs, which explicitly support separate stores for recovery and archiving.

The restore is asynchronous and needs checking after merge. MIGRATION-namespace.md has the commands, including comparing row counts against the live cluster — a cluster that comes up healthy but empty is the failure mode worth catching before stage 2 trusts it.

Stage 1 of moving Woodpecker out of the shared `apps` namespace. Addresses #433. **No cutover, no CI impact.** This stands up the `woodpecker` namespace and restores `wp-pg` into it from barman, running in parallel with the live `apps/wp-pg`. Nothing points at it yet. Rollback is `kubectl delete ns woodpecker` plus reverting this PR. ## Why move rather than patch the policy `wp-pg` has been `Ready=False` with `Instance Status Extraction Error` for three weeks. The cause is concrete — from the operator's own log: ``` Get "https://10.244.0.37:8000/pg/status": dial tcp 10.244.0.37:8000: i/o timeout ``` A dropped connect, not an HTTP-level fault. **The issue's guidance to skip NetworkPolicy is wrong** — `apps` does have a `default-deny-ingress`, 101 days old, and it lists `apps`, `home-assistant`, host and remote-node but not `database`. Every other namespace running a CNPG cluster (authentik, immich, monitoring, paperless, trmnl, wordpress) already allows `database`; `apps` is the only one that doesn't, which is exactly why one of six clusters is broken. I first fixed it by adding that allow. **That was the wrong fix** and is not in this PR: `apps` uses `endpointSelector: {}`, so the rule would also expose gitea, gatus, external-dns and ical-proxy to the whole `database` namespace — to solve a problem affecting one pod on one port. That is the same shape `home-assistant-ns.yaml` documents having to undo, where a blanket `fromEntities: world` exposed a whole namespace. The same gap explains a second symptom nobody had noticed: `cnpg_collector_up` has series for authentik, immich, monitoring and paperless but **not** `wp-pg`, because `apps` never allowed the monitoring namespace either. Both are fixed structurally by the move. **`woodpecker-ci` is deliberately not the target.** It enforces `pod-security: privileged` and exists for ephemeral step pods running repo-supplied code. Putting the database there would let any build step reach it via the intra-namespace allow. ## What's here | File | | |---|---| | `infrastructure/namespaces/namespaces.yaml` | the `woodpecker` Namespace | | `network-policies/woodpecker-ns.yaml` | baseline + a **scoped** `wp-pg-allow-cnpg-operator` (operator → :8000, monitoring → :9187) | | `migration-objectstore.yaml` | recovery source + a separate archiving target | | `migration-cluster.yaml` | `wp-pg` bootstrapped via `recovery` | | `migration-externalsecret-hetzner.yaml` | barman creds in the new namespace | | `MIGRATION-namespace.md` | staging, design notes, verification, rollback | ## Three decisions worth reviewing **Two ObjectStores.** `wp-pg-store-source` reads the existing `s3://renner-uno-k8s-cnpg/wp-pg`; `wp-pg-store` archives to a **new** `wp-pg-woodpecker` prefix. Two clusters writing one `destinationPath` would interleave their WAL streams and corrupt both. Splitting them also keeps the pre-migration backup intact and independently restorable. **The Namespace is declared in `infrastructure/namespaces`, not beside the workloads.** Its network policies ship with the `infrastructure` KS, and the `woodpecker` KS `dependsOn` `infrastructure` — creating it in the workload KS would invert that order and deadlock a cold rebuild. (Dry-run caught this: the policies failed with `namespaces "woodpecker" not found`.) Note `woodpecker-ci` has the same latent inversion today, created in `rbac.yaml`; not touched here. **No `prune: disabled` on the restored cluster.** The original carries one because it holds the only copy of the data. This one is reconstructable by re-running the restore, and a prune guard on a migration artifact is how orphans outlive an abandoned migration. It gains the guard in stage 3. ## #397 could not be folded in I said it would be — it can't. Creating `seed/woodpecker/hetzner-creds` needs OpenBao write access that isn't available (`bao kv list seed` → 403). Repointing at a path I can't verify exists would leave the restored cluster unable to archive WAL, which is the thing this stage exists to prove. It stays on `seed/paperless/hetzner-creds`; #397 remains open and is a one-line swap once the path exists. ## Verification - `kubectl kustomize` builds clean for `apps/woodpecker`, `infrastructure/namespaces`, `infrastructure/network-policies`. - `kubectl apply --dry-run=server` accepts all of it; the rendered set targeting `woodpecker` is exactly the four intended resources, and nothing in `apps` is modified. - Recovery stanza shape verified against the plugin-barman-cloud docs, which explicitly support separate stores for recovery and archiving. **The restore is asynchronous and needs checking after merge.** `MIGRATION-namespace.md` has the commands, including comparing row counts against the live cluster — a cluster that comes up healthy but *empty* is the failure mode worth catching before stage 2 trusts it.

claude-bot added 1 commit 2026-08-18 20:15:00 +00:00

feat(woodpecker): stage 1 — restore wp-pg into a dedicated namespace ... 85268c1e79

Addresses #433. No cutover, no CI impact: this stands up the `woodpecker`
namespace and restores wp-pg into it from barman, running in parallel with
the live apps/wp-pg. Nothing points at it yet.

Why a namespace move rather than a policy fix. wp-pg has been Ready=False
with "Instance Status Extraction Error" for three weeks because the CNPG
operator in `database` cannot reach https://<pod-ip>:8000/pg/status. The
apps default-deny-ingress uses endpointSelector:{}, so the only
namespace-level remedy is to allow the whole database namespace in — which
would expose gitea, gatus, external-dns and ical-proxy along with wp-pg, to
fix one pod on one port. The same gap explains why cnpg_collector_up has
series for authentik, immich, monitoring and paperless but not wp-pg: apps
never allowed the monitoring namespace either.

woodpecker-ci is deliberately not the target. It enforces pod-security:
privileged and exists for ephemeral step pods running repo-supplied code;
the database has no business sharing it.

Two ObjectStores, deliberately. wp-pg-store-source reads the existing
s3://renner-uno-k8s-cnpg/wp-pg prefix; wp-pg-store archives to a new
wp-pg-woodpecker prefix. Two clusters writing one destinationPath would
interleave their WAL streams and corrupt both, and splitting them keeps the
pre-migration backup intact as rollback.

The Namespace is declared in infrastructure/namespaces rather than beside
these workloads: its network policies ship with the `infrastructure` KS,
which the `woodpecker` KS dependsOn, so creating it in the workload KS would
invert that order and deadlock a cold rebuild.

The restored cluster deliberately carries no prune: disabled guard. The
original has one because it holds the only copy of the data; this one is
reconstructable, and a prune guard on a migration artifact is how orphans
outlive an abandoned migration. It gains the guard in stage 3.

Barman creds still resolve from seed/paperless/hetzner-creds. #397 wants a
woodpecker-owned path and this was the natural moment, but creating it needs
OpenBao write access that is not available here (bao kv list seed -> 403).
Pointing at a path that may not exist would leave the restored cluster
unable to archive WAL, which is what this stage exists to prove.

Runbook, including how to verify the restore holds real data rather than
merely coming up healthy: MIGRATION-namespace.md.

Co-Authored-By: Claude Bot <claude-bot@renner.uno>

review-bot requested changes 2026-08-18 20:20:14 +00:00

review-bot left a comment

Member

Copy Link

Copy Source

🤖 Automated review — model CLI default (complexity: complex), commit 85268c1e79e338e7c02696e63101ae464400707f.

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

---

Summary

Stage 1 of moving wp-pg out of the shared apps namespace: a new woodpecker Namespace, its baseline CiliumNetworkPolicy, a scoped operator/metrics allow, two ObjectStores (read-only recovery source + a new archiving target), and a wp-pg Cluster bootstrapped via recovery. Nothing is cut over yet. The design rationale (splitting destinationPaths, declaring the Namespace in infrastructure/namespaces to avoid a dependsOn deadlock, deferring the prune guard) checks out against the actual Flux Kustomization graph and the sibling clusters I compared it to. There is one concrete network-policy bug that reopens the exact class of problem this PR is fixing.

🔴 Blocking

• kubernetes/infrastructure/network-policies/woodpecker-ns.yaml:29-39 — the "step pods call back to the server" ingress rule is attached to the namespace-wide baseline policy (endpointSelector: {} at line 15), not to a policy scoped to the server pod. In Cilium, toPorts inside an ingress rule filters by port only — it does not restrict which endpoint the rule applies to; that's governed solely by the enclosing policy's top-level endpointSelector. Because this policy's endpointSelector is {} (every pod in the namespace), the rule actually grants woodpecker-ci → :8000 on every pod in woodpecker, including wp-pg — whose CNPG instance-manager status API also listens on :8000 (confirmed both ports are 8000: kubernetes/apps/woodpecker/server.yaml:94 for the server, and the comment at woodpecker-ns.yaml:45-46 for the CNPG status endpoint). That directly contradicts the rule's own comment ("step pods run arbitrary repo-supplied code, so they must not reach wp-pg") and the intent behind the second policy in the same file (wp-pg-allow-cnpg-operator, lines 55-82), which deliberately scopes database-namespace access to cnpg.io/cluster: wp-pg pods only. This is live now, in stage 1, since wp-pg and woodpecker-ci both already exist — any CI step pod can open a TCP connection to wp-pg's instance-manager port today. It's also the same endpointSelector: {}-causes-blanket-grant failure mode the PR's own description calls out as the reason apps is unsafe to patch in place.
  Fix: move the woodpecker-ci → :8000 rule into its own CiliumNetworkPolicy scoped to the server's pod labels (apps/woodpecker/server.yaml will define these once it lands in stage 2), mirroring how wp-pg-allow-cnpg-operator is scoped, or how kubernetes/apps/woodpecker/networkpolicy.yaml's woodpecker-allow-rotators scopes its own :8000 rule via endpointSelector: matchLabels: app.kubernetes.io/component: server today.

🟡 Suggestions

• kubernetes/infrastructure/network-policies/woodpecker-ns.yaml — the new default-deny-ingress policy has no egress section at all, unlike the apps (default-deny.yaml) and paperless-ns.yaml baselines it's modeled on, both of which explicitly restrict egress. With no policy in the namespace selecting egress, Cilium's default is allow-all, so wp-pg (and, from stage 2, the server/agent) can reach any pod in any namespace on any port, not just world. woodpecker-ci-ns.yaml makes the equivalent choice deliberately and documents why (arbitrary build steps need open internet egress); this file makes the same choice silently. If it's intentional (e.g. because WAL archiving needs open egress to Hetzner S3, or because the follow-up stages will make this restrictive), a one-line comment would keep it from reading as an oversight. If it's not intentional, it's worth tightening before stage 2 lands the server/agent here.
• kubernetes/apps/woodpecker/migration-cluster.yaml, migration-objectstore.yaml, MIGRATION-namespace.md, namespaces.yaml — several comments say the CiliumNetworkPolicies "ship with the infrastructure KS." They actually ship via infrastructure-crds (kubernetes/infrastructure/post-crds/kustomization.yaml:13 includes ../network-policies/; the top-level kubernetes/infrastructure/kustomization.yaml does not). The ordering claim itself still holds — kubernetes/clusters/homelab/woodpecker.yaml's dependsOn lists both infrastructure (creates the Namespace) and infrastructure-crds (creates the CNPs), and infrastructure-crds itself dependsOn: [infrastructure] — so there's no actual deadlock risk, just an imprecise attribution in several comments.
• kubernetes/apps/woodpecker/migration-cluster.yaml — I couldn't verify the bootstrap.recovery.source / externalClusters[].plugin shape against the plugin-barman-cloud upstream docs (no other cluster in this repo uses plugin-based recovery yet, so there's no local precedent to cross-check against, and I have no network access from this checkout). The PR body says this was checked against the docs directly; worth confirming that check happened against a recovery-bootstrap example specifically, not just the archiving side, since that's the one part of the CR with no analog elsewhere in the repo.

🟢 Nits

• wp-pg-allow-cnpg-operator's monitoring → :9187 rule (woodpecker-ns.yaml:76-82) is already subsumed by the baseline policy's unrestricted monitoring-namespace allow (woodpecker-ns.yaml:26-28, no toPorts), so it's currently redundant rather than additive. Harmless, and matches the existing paperless-ns.yaml pattern of layering a specific rule over a broad one, just noting it doesn't add coverage today.

Tests

No automated tests apply to this kind of change; the PR relies on kubectl kustomize build + --dry-run=server apply, which is the right verification for static manifests, plus a documented post-merge runbook (MIGRATION-namespace.md) for checking the async restore actually holds data. That's appropriate for the change. It would not have caught the network-policy scoping bug above, since dry-run apply doesn't evaluate Cilium policy semantics — worth a manual check post-merge (e.g. cilium policy trace or an actual connection test from a woodpecker-ci pod to wp-pg:8000) alongside the row-count verification already documented.

Recommendation

Request changes — 1 blocking issue.

🤖 **Automated review** — model `CLI default` (complexity: complex), commit `85268c1e79e338e7c02696e63101ae464400707f`. > Advisory review by the `review-bot` pipeline. It never approves; a human merges. Push new commits to get a fresh review. --- ## Summary Stage 1 of moving `wp-pg` out of the shared `apps` namespace: a new `woodpecker` Namespace, its baseline `CiliumNetworkPolicy`, a scoped operator/metrics allow, two `ObjectStore`s (read-only recovery source + a new archiving target), and a `wp-pg` `Cluster` bootstrapped via `recovery`. Nothing is cut over yet. The design rationale (splitting `destinationPath`s, declaring the Namespace in `infrastructure/namespaces` to avoid a `dependsOn` deadlock, deferring the prune guard) checks out against the actual Flux `Kustomization` graph and the sibling clusters I compared it to. There is one concrete network-policy bug that reopens the exact class of problem this PR is fixing. ## 🔴 Blocking - **`kubernetes/infrastructure/network-policies/woodpecker-ns.yaml:29-39`** — the "step pods call back to the server" ingress rule is attached to the namespace-wide baseline policy (`endpointSelector: {}` at line 15), not to a policy scoped to the server pod. In Cilium, `toPorts` inside an `ingress` rule filters by *port only* — it does not restrict *which endpoint* the rule applies to; that's governed solely by the enclosing policy's top-level `endpointSelector`. Because this policy's `endpointSelector` is `{}` (every pod in the namespace), the rule actually grants `woodpecker-ci` → `:8000` on **every** pod in `woodpecker`, including `wp-pg` — whose CNPG instance-manager status API also listens on `:8000` (confirmed both ports are `8000`: `kubernetes/apps/woodpecker/server.yaml:94` for the server, and the comment at `woodpecker-ns.yaml:45-46` for the CNPG status endpoint). That directly contradicts the rule's own comment ("step pods run arbitrary repo-supplied code, so they must not reach wp-pg") and the intent behind the second policy in the same file (`wp-pg-allow-cnpg-operator`, lines 55-82), which deliberately scopes `database`-namespace access to `cnpg.io/cluster: wp-pg` pods only. This is live now, in stage 1, since `wp-pg` and `woodpecker-ci` both already exist — any CI step pod can open a TCP connection to `wp-pg`'s instance-manager port today. It's also the same `endpointSelector: {}`-causes-blanket-grant failure mode the PR's own description calls out as the reason `apps` is unsafe to patch in place. Fix: move the `woodpecker-ci` → `:8000` rule into its own `CiliumNetworkPolicy` scoped to the server's pod labels (`apps/woodpecker/server.yaml` will define these once it lands in stage 2), mirroring how `wp-pg-allow-cnpg-operator` is scoped, or how `kubernetes/apps/woodpecker/networkpolicy.yaml`'s `woodpecker-allow-rotators` scopes its own `:8000` rule via `endpointSelector: matchLabels: app.kubernetes.io/component: server` today. ## 🟡 Suggestions - **`kubernetes/infrastructure/network-policies/woodpecker-ns.yaml`** — the new `default-deny-ingress` policy has no `egress` section at all, unlike the `apps` (`default-deny.yaml`) and `paperless-ns.yaml` baselines it's modeled on, both of which explicitly restrict egress. With no policy in the namespace selecting egress, Cilium's default is allow-all, so `wp-pg` (and, from stage 2, the server/agent) can reach any pod in any namespace on any port, not just `world`. `woodpecker-ci-ns.yaml` makes the equivalent choice deliberately and documents *why* (arbitrary build steps need open internet egress); this file makes the same choice silently. If it's intentional (e.g. because WAL archiving needs open egress to Hetzner S3, or because the follow-up stages will make this restrictive), a one-line comment would keep it from reading as an oversight. If it's not intentional, it's worth tightening before stage 2 lands the server/agent here. - **`kubernetes/apps/woodpecker/migration-cluster.yaml`, `migration-objectstore.yaml`, `MIGRATION-namespace.md`, `namespaces.yaml`** — several comments say the CiliumNetworkPolicies "ship with the `infrastructure` KS." They actually ship via `infrastructure-crds` (`kubernetes/infrastructure/post-crds/kustomization.yaml:13` includes `../network-policies/`; the top-level `kubernetes/infrastructure/kustomization.yaml` does not). The ordering claim itself still holds — `kubernetes/clusters/homelab/woodpecker.yaml`'s `dependsOn` lists both `infrastructure` (creates the Namespace) and `infrastructure-crds` (creates the CNPs), and `infrastructure-crds` itself `dependsOn: [infrastructure]` — so there's no actual deadlock risk, just an imprecise attribution in several comments. - **`kubernetes/apps/woodpecker/migration-cluster.yaml`** — I couldn't verify the `bootstrap.recovery.source` / `externalClusters[].plugin` shape against the plugin-barman-cloud upstream docs (no other cluster in this repo uses plugin-based recovery yet, so there's no local precedent to cross-check against, and I have no network access from this checkout). The PR body says this was checked against the docs directly; worth confirming that check happened against a `recovery`-bootstrap example specifically, not just the archiving side, since that's the one part of the CR with no analog elsewhere in the repo. ## 🟢 Nits - `wp-pg-allow-cnpg-operator`'s `monitoring` → `:9187` rule (`woodpecker-ns.yaml:76-82`) is already subsumed by the baseline policy's unrestricted `monitoring`-namespace allow (`woodpecker-ns.yaml:26-28`, no `toPorts`), so it's currently redundant rather than additive. Harmless, and matches the existing `paperless-ns.yaml` pattern of layering a specific rule over a broad one, just noting it doesn't add coverage today. ## Tests No automated tests apply to this kind of change; the PR relies on `kubectl kustomize` build + `--dry-run=server` apply, which is the right verification for static manifests, plus a documented post-merge runbook (`MIGRATION-namespace.md`) for checking the async restore actually holds data. That's appropriate for the change. It would not have caught the network-policy scoping bug above, since dry-run apply doesn't evaluate Cilium policy semantics — worth a manual check post-merge (e.g. `cilium policy trace` or an actual connection test from a `woodpecker-ci` pod to `wp-pg:8000`) alongside the row-count verification already documented. ## Recommendation Request changes — 1 blocking issue. <!-- pr-review:sha=85268c1e79e338e7c02696e63101ae464400707f -->

claude-bot referenced this issue from a commit 2026-08-18 20:25:40 +00:00

fix(woodpecker): drop the woodpecker-ci rule off the blanket ingress policy

claude-bot added 1 commit 2026-08-18 20:25:40 +00:00

fix(woodpecker): drop the woodpecker-ci rule off the blanket ingress policy ... b0ddbe0dfd

Addresses the blocking review finding on #504.

The "step pods call the server back on :8000" rule was attached to the
namespace baseline, which uses endpointSelector: {}. A toPorts block
narrows the port, not the destination workload — that is governed solely by
the enclosing policy's endpointSelector. So the rule granted woodpecker-ci
access to :8000 on every pod in the namespace, and the CNPG instance
manager serves its status API on :8000 (server.yaml:94 uses the same port).
Arbitrary repo-supplied build steps would have had a route to wp-pg's
instance manager.

That is the same endpointSelector:{}-means-blanket-grant failure this PR
exists to escape, reproduced inside the fix.

Removed rather than rescoped: there is no woodpecker server in this
namespace until stage 2, so the rule was premature as well as wrong. It
lands there as its own policy selecting the server pod, the way
woodpecker-allow-rotators already scopes its :8000 rule.

The baseline now carries no port-scoped rules at all, so nothing in it can
be mistaken for workload scoping, and a comment warns why that must stay
true.

Also from the review:

- Corrected the KS attribution. The network policies ship with
  infrastructure-crds (infrastructure/post-crds includes
  ../network-policies/), not the top-level infrastructure KS. The ordering
  conclusion is unchanged — infrastructure-crds dependsOn infrastructure,
  and woodpecker dependsOn both — but the comments named the wrong one.
- Documented why the baseline has no egress section, instead of leaving it
  to read as an oversight. Writing that ruleset in stage 2, with the server
  and agent present, beats guessing it now and breaking the WAL archiving
  this stage exists to verify.
- Dropped the monitoring -> :9187 rule from wp-pg-allow-cnpg-operator. The
  baseline already allows the monitoring namespace unrestricted, so it added
  no coverage while implying scoping it did not provide.
- Added a cilium-dbg policy trace step to the runbook. dry-run validates
  manifest shape, not Cilium semantics, and would not have caught this bug.

Co-Authored-By: Claude Bot <claude-bot@renner.uno>

claude-bot commented 2026-08-18 20:25:57 +00:00

Author

Member

Copy Link

Copy Source

The blocking finding is correct, and it is the most useful catch of this whole migration. Fixed in b0ddbe0d.

Verified before accepting it, since the claim is subtle:

• kubernetes/apps/woodpecker/server.yaml:94 → containerPort: 8000, and the CNPG instance manager serves /pg/status on :8000 too (confirmed from the operator's own error in #433: Get "https://10.244.0.37:8000/pg/status"). The port collision is real.
• Cilium scoping works as described: a toPorts block narrows the port, not the destination workload. The enclosing endpointSelector: {} is what decides which pods a rule lands on.

So the rule granted woodpecker-ci → :8000 on every pod in the namespace, wp-pg included. Arbitrary repo-supplied build steps would have had a route to the database's instance manager — and it would have been live in stage 1, since both namespaces already exist.

That is the same endpointSelector: {}-means-blanket-grant failure this PR's description gives as the reason apps cannot be patched in place. I reproduced it inside the fix. The review earned its keep.

Removed rather than rescoped. There is no woodpecker server in this namespace until stage 2, so the rule was premature as well as wrong. It lands in stage 2 as its own policy selecting the server pod, mirroring woodpecker-allow-rotators. The baseline now has no port-scoped rules at all, so nothing in it can be mistaken for workload scoping, and a comment says why that must stay true.

Also taken:

• KS attribution corrected. You are right that the policies ship with infrastructure-crds via infrastructure/post-crds/kustomization.yaml:13, not the top-level infrastructure KS — verified. The ordering conclusion is unchanged, but three comments named the wrong KS.
• Egress documented. It was a real omission in the writing, not the ruleset: the absence is deliberate, and now says so. Defining egress once in stage 2 with the server and agent present beats guessing it here and breaking the WAL archiving this stage exists to verify.
• Nit taken, not just noted. Dropped the monitoring → :9187 rule. Redundant against the baseline's unrestricted monitoring allow, and worse than harmless — after this review, a rule that looks scoped but isn't is exactly the thing to avoid leaving lying around.

On the recovery stanza you could not verify: the check was against a recovery-bootstrap example specifically — the plugin-barman-cloud usage docs show bootstrap.recovery.source with a matching externalClusters entry, and state that separate stores for archiving and restoring are supported, which is what the two-ObjectStore split relies on.

Your closing point stands and I have acted on it: dry-run cannot evaluate Cilium semantics and would never have caught this. The runbook now carries a cilium-dbg policy trace step asserting the operator reaches wp-pg:8000 and a woodpecker-ci pod does not.

The blocking finding is correct, and it is the most useful catch of this whole migration. Fixed in b0ddbe0d. **Verified before accepting it**, since the claim is subtle: - `kubernetes/apps/woodpecker/server.yaml:94` → `containerPort: 8000`, and the CNPG instance manager serves `/pg/status` on **:8000** too (confirmed from the operator's own error in #433: `Get "https://10.244.0.37:8000/pg/status"`). The port collision is real. - Cilium scoping works as described: a `toPorts` block narrows the port, not the destination workload. The enclosing `endpointSelector: {}` is what decides *which* pods a rule lands on. So the rule granted `woodpecker-ci` → `:8000` on **every** pod in the namespace, wp-pg included. Arbitrary repo-supplied build steps would have had a route to the database's instance manager — and it would have been live in stage 1, since both namespaces already exist. That is the same `endpointSelector: {}`-means-blanket-grant failure this PR's description gives as the reason `apps` cannot be patched in place. I reproduced it inside the fix. The review earned its keep. **Removed rather than rescoped.** There is no woodpecker server in this namespace until stage 2, so the rule was premature as well as wrong. It lands in stage 2 as its own policy selecting the server pod, mirroring `woodpecker-allow-rotators`. The baseline now has **no port-scoped rules at all**, so nothing in it can be mistaken for workload scoping, and a comment says why that must stay true. Also taken: - **KS attribution corrected.** You are right that the policies ship with `infrastructure-crds` via `infrastructure/post-crds/kustomization.yaml:13`, not the top-level `infrastructure` KS — verified. The ordering conclusion is unchanged, but three comments named the wrong KS. - **Egress documented.** It was a real omission in the writing, not the ruleset: the absence is deliberate, and now says so. Defining egress once in stage 2 with the server and agent present beats guessing it here and breaking the WAL archiving this stage exists to verify. - **Nit taken, not just noted.** Dropped the `monitoring → :9187` rule. Redundant against the baseline's unrestricted monitoring allow, and worse than harmless — after this review, a rule that *looks* scoped but isn't is exactly the thing to avoid leaving lying around. On the recovery stanza you could not verify: the check was against a `recovery`-bootstrap example specifically — the plugin-barman-cloud usage docs show `bootstrap.recovery.source` with a matching `externalClusters` entry, and state that separate stores for archiving and restoring are supported, which is what the two-ObjectStore split relies on. Your closing point stands and I have acted on it: dry-run cannot evaluate Cilium semantics and would never have caught this. The runbook now carries a `cilium-dbg policy trace` step asserting the operator reaches wp-pg:8000 and a `woodpecker-ci` pod does not.

review-bot requested changes 2026-08-18 20:30:34 +00:00

review-bot left a comment

Member

Copy Link

Copy Source

🤖 Automated review — model CLI default (complexity: complex), commit b0ddbe0dfd7c07cd0458e352c7f4fb8ad792f3d7.

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

---

Summary

Stage 1 of moving Woodpecker's wp-pg CNPG cluster out of the shared apps namespace: a new woodpecker namespace, scoped CiliumNetworkPolicies, and a parallel-run restore-from-barman Cluster/ObjectStore/ExternalSecret set. Nothing yet points at the new cluster, so blast radius is low, and the KS-ordering, network-policy-scoping and two-ObjectStore design decisions are all sound and consistent with existing repo conventions (verified against RECOVERY.md, database-ns.yaml, paperless-ns.yaml, and the infrastructure/infrastructure-crds/woodpecker Kustomization dependency chain). There is one concrete defect in the recovery bootstrap spec that will make the restored cluster's auto-generated app credentials useless.

🔴 Blocking

• kubernetes/apps/woodpecker/migration-cluster.yaml:233-235 — spec.bootstrap.recovery sets only source: wp-pg-apps, omitting database and owner. This repo's own runbook (kubernetes/RECOVERY.md:79-123) sets both fields on every recovery bootstrap it documents (e.g. ak-pg: database: authentik, owner: authentik) and explains why: they "tell CNPG which app user the -app service should target after promotion; they mirror the old initdb values." The original apps/wp-pg cluster this restores from uses bootstrap.initdb.database: woodpecker / owner: woodpecker (kubernetes/apps/woodpecker/cluster.yaml:28-30). Leaving database/owner unset on a recovery bootstrap falls back to CNPG's default app database/user name, app/app — a role and database that don't exist in the physically-restored data (which has woodpecker/woodpecker, carried over intact from the backup). CNPG will provision a fresh, empty app database/role and point the generated wp-pg-app Secret/Service at it, leaving the actual restored data orphaned from the credentials meant to reach it.

  This isn't just cosmetic: MIGRATION-namespace.md:111-116 verifies the restore by reading the password out of wp-pg-app and connecting with psql -U woodpecker -d woodpecker — but the -app Secret's username/dbname keys would be app, not woodpecker, so that verification step as written won't even authenticate against the intended role. Fix: add database: woodpecker and owner: woodpecker alongside source: wp-pg-apps in the bootstrap.recovery block, matching the pattern in RECOVERY.md.

  I could not check this against CNPG's upstream docs directly (no external network access in this environment), so if there's any doubt, confirm post-apply with kubectl -n woodpecker get secret wp-pg-app -o jsonpath='{.data.dbname}' | base64 -d — it should read woodpecker, not app.

🟡 Suggestions

• kubernetes/infrastructure/network-policies/woodpecker-ns.yaml:481-488 — wp-pg-allow-cnpg-operator selects the CNPG operator via "k8s:io.kubernetes.pod.namespace": database + app.kubernetes.io/name: cloudnative-pg. I couldn't confirm that label is actually what the cloudnative-pg HelmRelease (kubernetes/infrastructure/cloudnative-pg/helmrelease.yaml) applies to the operator pod (the chart isn't vendored in-repo). The PR's own runbook already plans to check this with cilium-dbg policy trace post-merge — worth running that specific ALLOWED-case trace early, since a label mismatch here would silently reproduce the exact #433 timeout this PR exists to fix, just with a different cause.

Tests

No automated tests apply here (Kubernetes manifests + a Flux-managed restore). The PR's own verification plan (dry-run build, row-count comparison against the live cluster, cilium-dbg policy trace for both the allowed and denied paths) is appropriately thorough for this kind of infrastructure change and is the right place to also catch the database/owner issue above — the \dt / select count(*) check in MIGRATION-namespace.md:112-115 will surface it once the -app Secret mismatch is fixed to actually use woodpecker.

Recommendation

Request changes — 1 blocking issue.

🤖 **Automated review** — model `CLI default` (complexity: complex), commit `b0ddbe0dfd7c07cd0458e352c7f4fb8ad792f3d7`. > Advisory review by the `review-bot` pipeline. It never approves; a human merges. Push new commits to get a fresh review. --- ## Summary Stage 1 of moving Woodpecker's `wp-pg` CNPG cluster out of the shared `apps` namespace: a new `woodpecker` namespace, scoped CiliumNetworkPolicies, and a parallel-run restore-from-barman `Cluster`/`ObjectStore`/`ExternalSecret` set. Nothing yet points at the new cluster, so blast radius is low, and the KS-ordering, network-policy-scoping and two-`ObjectStore` design decisions are all sound and consistent with existing repo conventions (verified against `RECOVERY.md`, `database-ns.yaml`, `paperless-ns.yaml`, and the `infrastructure`/`infrastructure-crds`/`woodpecker` Kustomization dependency chain). There is one concrete defect in the recovery bootstrap spec that will make the restored cluster's auto-generated app credentials useless. ## 🔴 Blocking - **`kubernetes/apps/woodpecker/migration-cluster.yaml:233-235`** — `spec.bootstrap.recovery` sets only `source: wp-pg-apps`, omitting `database` and `owner`. This repo's own runbook (`kubernetes/RECOVERY.md:79-123`) sets both fields on every recovery bootstrap it documents (e.g. `ak-pg`: `database: authentik`, `owner: authentik`) and explains why: they "tell CNPG which app user the `-app` service should target after promotion; they mirror the old `initdb` values." The original `apps/wp-pg` cluster this restores from uses `bootstrap.initdb.database: woodpecker` / `owner: woodpecker` (`kubernetes/apps/woodpecker/cluster.yaml:28-30`). Leaving `database`/`owner` unset on a recovery bootstrap falls back to CNPG's default app database/user name, `app`/`app` — a role and database that don't exist in the physically-restored data (which has `woodpecker`/`woodpecker`, carried over intact from the backup). CNPG will provision a fresh, empty `app` database/role and point the generated `wp-pg-app` Secret/Service at it, leaving the actual restored data orphaned from the credentials meant to reach it. This isn't just cosmetic: `MIGRATION-namespace.md:111-116` verifies the restore by reading the password out of `wp-pg-app` and connecting with `psql -U woodpecker -d woodpecker` — but the `-app` Secret's `username`/`dbname` keys would be `app`, not `woodpecker`, so that verification step as written won't even authenticate against the intended role. Fix: add `database: woodpecker` and `owner: woodpecker` alongside `source: wp-pg-apps` in the `bootstrap.recovery` block, matching the pattern in `RECOVERY.md`. I could not check this against CNPG's upstream docs directly (no external network access in this environment), so if there's any doubt, confirm post-apply with `kubectl -n woodpecker get secret wp-pg-app -o jsonpath='{.data.dbname}' | base64 -d` — it should read `woodpecker`, not `app`. ## 🟡 Suggestions - **`kubernetes/infrastructure/network-policies/woodpecker-ns.yaml:481-488`** — `wp-pg-allow-cnpg-operator` selects the CNPG operator via `"k8s:io.kubernetes.pod.namespace": database` + `app.kubernetes.io/name: cloudnative-pg`. I couldn't confirm that label is actually what the `cloudnative-pg` HelmRelease (`kubernetes/infrastructure/cloudnative-pg/helmrelease.yaml`) applies to the operator pod (the chart isn't vendored in-repo). The PR's own runbook already plans to check this with `cilium-dbg policy trace` post-merge — worth running that specific ALLOWED-case trace early, since a label mismatch here would silently reproduce the exact #433 timeout this PR exists to fix, just with a different cause. ## Tests No automated tests apply here (Kubernetes manifests + a Flux-managed restore). The PR's own verification plan (dry-run build, row-count comparison against the live cluster, `cilium-dbg policy trace` for both the allowed and denied paths) is appropriately thorough for this kind of infrastructure change and is the right place to also catch the `database`/`owner` issue above — the `\dt` / `select count(*)` check in `MIGRATION-namespace.md:112-115` will surface it once the `-app` Secret mismatch is fixed to actually use `woodpecker`. ## Recommendation Request changes — 1 blocking issue. <!-- pr-review:sha=b0ddbe0dfd7c07cd0458e352c7f4fb8ad792f3d7 -->

claude-bot referenced this issue from a commit 2026-08-18 20:33:57 +00:00

fix(woodpecker): set database/owner on the wp-pg recovery bootstrap

claude-bot added 1 commit 2026-08-18 20:33:58 +00:00

fix(woodpecker): set database/owner on the wp-pg recovery bootstrap ... 1b5ad600d5

Addresses the second blocking review finding on #504.

bootstrap.recovery set only `source`. The live CRD documents the defaults:
`database` defaults to `app`, and `owner` defaults to whatever `database`
is. Neither name exists in the physically restored data, which carries
woodpecker/woodpecker from the backup.

So CNPG would have restored the real data, then provisioned a separate
empty `app` database and pointed the generated wp-pg-app Secret and the
-app Service at that instead — a cluster reporting healthy, holding the
right bytes, with the credentials aimed at the wrong database. The runbook's
own verification (psql -U woodpecker -d woodpecker using the wp-pg-app
password) would not have authenticated, so the failure would have surfaced
as a confusing auth error rather than as what it was.

kubernetes/RECOVERY.md sets both fields on all three recovery examples it
documents (ak-pg, im-pg, pprl-pg) for exactly this reason; this one did not
follow the pattern.

Added a runbook step to read dbname/username back out of the wp-pg-app
Secret before the row-count checks, so the mismatch is caught directly
rather than inferred from a psql failure.

Co-Authored-By: Claude Bot <claude-bot@renner.uno>

claude-bot commented 2026-08-18 20:34:14 +00:00

Author

Member

Copy Link

Copy Source

Correct again, and this one would have been genuinely confusing to debug in the field. Fixed in 1b5ad600.

Verified the mechanism from the live CRD, not just the repo pattern:

$ kubectl get crd clusters.postgresql.cnpg.io -o json | jq '...bootstrap.recovery.properties'
database: "Name of the database used by the application. Default: `app`."
owner:    "Name of the owner of the database ... Defaults to the value of the `database` key."

So the defaults are exactly as described, and app does not exist in the restored data — it carries woodpecker/woodpecker from the backup. CNPG would have restored the right bytes, then provisioned a separate empty app database and pointed the generated wp-pg-app Secret and the -app Service at that.

The nastiest part is how it would have presented: a Cluster reporting healthy, holding correct data, with credentials aimed at the wrong database. The runbook's own psql -U woodpecker -d woodpecker check would have failed on authentication, which reads like a secrets problem, not a bootstrap problem. You are right that it is not cosmetic.

And RECOVERY.md sets both fields on all three documented recovery examples — ak-pg, im-pg, pprl-pg — so the repo pattern was already there and this simply did not follow it.

Added database: woodpecker / owner: woodpecker, plus a runbook step that reads dbname and username straight out of the wp-pg-app Secret before the row-count checks, so a mismatch surfaces as itself rather than as a puzzling auth error.

On the operator label suggestion — I can close that one now rather than defer it. app.kubernetes.io/name: cloudnative-pg was taken from the live pod, not from the chart:

$ kubectl get pod -n database cloudnative-pg-77967b5c9c-zjxs8 -o json | jq '.metadata.labels'
app.kubernetes.io/instance=cloudnative-pg
app.kubernetes.io/name=cloudnative-pg

The selector matches what the operator actually carries. The policy trace ALLOWED case in the runbook still stands as the post-merge confirmation, since a matching label is necessary but not sufficient.

On validation coverage, stated plainly: the four resources in the woodpecker namespace cannot be server-side dry-run until that namespace exists, because it now ships from a different Kustomization. To avoid leaving the new fields unvalidated I rendered the Cluster into an existing namespace under a throwaway name and dry-ran that — it is accepted by the CRD. The remaining unverified surface is the restore behaviour itself, which only the post-merge runbook can settle.

Correct again, and this one would have been genuinely confusing to debug in the field. Fixed in 1b5ad600. **Verified the mechanism from the live CRD**, not just the repo pattern: ``` $ kubectl get crd clusters.postgresql.cnpg.io -o json | jq '...bootstrap.recovery.properties' database: "Name of the database used by the application. Default: `app`." owner: "Name of the owner of the database ... Defaults to the value of the `database` key." ``` So the defaults are exactly as described, and `app` does not exist in the restored data — it carries `woodpecker`/`woodpecker` from the backup. CNPG would have restored the right bytes, then provisioned a *separate* empty `app` database and pointed the generated `wp-pg-app` Secret and the `-app` Service at that. The nastiest part is how it would have presented: a Cluster reporting healthy, holding correct data, with credentials aimed at the wrong database. The runbook's own `psql -U woodpecker -d woodpecker` check would have failed on **authentication**, which reads like a secrets problem, not a bootstrap problem. You are right that it is not cosmetic. And `RECOVERY.md` sets both fields on all three documented recovery examples — ak-pg, im-pg, pprl-pg — so the repo pattern was already there and this simply did not follow it. Added `database: woodpecker` / `owner: woodpecker`, plus a runbook step that reads `dbname` and `username` straight out of the `wp-pg-app` Secret **before** the row-count checks, so a mismatch surfaces as itself rather than as a puzzling auth error. **On the operator label suggestion** — I can close that one now rather than defer it. `app.kubernetes.io/name: cloudnative-pg` was taken from the live pod, not from the chart: ``` $ kubectl get pod -n database cloudnative-pg-77967b5c9c-zjxs8 -o json | jq '.metadata.labels' app.kubernetes.io/instance=cloudnative-pg app.kubernetes.io/name=cloudnative-pg ``` The selector matches what the operator actually carries. The `policy trace` ALLOWED case in the runbook still stands as the post-merge confirmation, since a matching label is necessary but not sufficient. **On validation coverage, stated plainly:** the four resources in the `woodpecker` namespace cannot be server-side dry-run until that namespace exists, because it now ships from a different Kustomization. To avoid leaving the new fields unvalidated I rendered the Cluster into an existing namespace under a throwaway name and dry-ran that — it is accepted by the CRD. The remaining unverified surface is the restore behaviour itself, which only the post-merge runbook can settle.

review-bot requested changes 2026-08-18 20:40:20 +00:00

review-bot left a comment

Member

Copy Link

Copy Source

🤖 Automated review — model CLI default (complexity: complex), commit 1b5ad600d5c125bc3970e013fc16b85b4c8966db.

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

---

Summary

Stage 1 of moving Woodpecker's wp-pg CNPG cluster into a dedicated woodpecker namespace: a new Namespace, a scoped baseline CiliumNetworkPolicy + an operator-access policy, a barman ExternalSecret, two ObjectStores (read-only recovery source + new archiving target), and a Cluster bootstrapped via recovery. Purely additive — nothing in apps is touched, and nothing yet references the new resources. The design (namespace-per-service to avoid endpointSelector: {} blast radius, split ObjectStores to avoid WAL interleaving, namespace declared in infrastructure/namespaces to respect Flux dependsOn ordering, recovery database/owner mirroring the source cluster's initdb values) checks out against the rest of the repo — I traced the Flux dependsOn chain (infrastructure → infrastructure-crds → woodpecker KS), the apps and database namespace CiliumNetworkPolicy baselines, and kubernetes/RECOVERY.md's documented recovery-stanza convention, and the PR's claims about all of these hold up. One of its two central justifications, however, doesn't actually land with this diff — see below.

🔴 Blocking

• The metrics-scraping fix the PR claims doesn't ship. The PR body and MIGRATION-namespace.md both state that the apps namespace's missing database/monitoring ingress allowances are why cnpg_collector_up has no series for wp-pg, and that "both are fixed structurally by the move." But Prometheus scraping for every other CNPG cluster in this repo goes through a hand-written PodMonitor in kubernetes/apps/monitoring-rules/cnpg-podmonitors.yaml (the file's own header explains why: CNPG's enablePodMonitor doesn't set the release: kube-prometheus-stack label this Prometheus's podMonitorSelector requires). That file has entries for ak-pg, im-pg, gf-pg, and pprl-pg — but none for wp-pg, and this PR doesn't add one, either there or under the new woodpecker namespace. Fixing the CiliumNetworkPolicy ingress gap (which this PR does correctly do) is necessary but not sufficient: with no PodMonitor selecting cnpg.io/cluster: wp-pg in the woodpecker namespace, Prometheus has no scrape target configured for it at all, network policy aside. The runbook's own verification step (MIGRATION-namespace.md lines 140-146, cnpg_collector_up{namespace="woodpecker"}) will return empty regardless of whether the new wp-pg-allow-cnpg-operator policy is correct — so it can't actually confirm the fix it's meant to confirm. Either add the PodMonitor in this PR (mirroring the existing four entries) or adjust the PR description/runbook to stop claiming this is fixed by stage 1.

🟡 Suggestions

• The migration Cluster's health is coupled to the live production Kustomization's Ready status. kubernetes/clusters/homelab/woodpecker.yaml sets wait: true and a healthCheckExprs entry matching apiVersion: postgresql.cnpg.io/v1, kind: Cluster with no namespace/name qualifier. Flux evaluates that expression against every resource of that GVK in the Kustomization's inventory — after this PR, that's both apps/wp-pg (already healthy) and the new woodpecker/wp-pg (mid-restore). Until the restore reaches Cluster in healthy state, the whole woodpecker Kustomization — which also owns the live server/agent/DB — reports not-Ready, and kubernetes/infrastructure/flux-notifications/alerts.yaml's flux-failures Alert (eventSeverity: error on Kustomization: '*') will fire on any reconcile-timeout error in the meantime. infrastructure.yaml's own comment in this repo (wait: false + explicit healthChecks, to stop one crashlooping workload from freezing unrelated dependents) shows this exact failure mode is already a known concern here — this PR reintroduces a milder version of it onto the one Kustomization that owns production Woodpecker. Nothing else dependsOn: woodpecker, so it won't cascade further, but it can generate misleading "woodpecker is broken" alerts tied to an experimental parallel restore. Worth at least a mention in the runbook that a lagging/errored restore is expected to make the woodpecker KS report not-Ready during that window.

• No ScheduledBackup for the migration cluster, so wp-pg-store's retentionPolicy: "30d" (migration-objectstore.yaml:377) has nothing to retire against. Every other CNPG cluster in this repo pairs isWALArchiver: true with a ScheduledBackup (see apps/woodpecker/scheduled-backup.yaml), which is what actually produces the periodic base backups a retention policy prunes. migration-cluster.yaml archives continuous WAL to the new wp-pg-woodpecker prefix but never takes a base backup there. Until stage 2/3 lands one, that prefix accumulates WAL from an unbounded chain with no independent recovery point and no way for the retention policy to reclaim space — a real cost/durability gap if this stage sits for a while before cutover, even though it's not directly harmful today.

🟢 Nits

• infrastructure/namespaces/namespaces.yaml:410 adds labels: app.kubernetes.io/name: woodpecker to the new Namespace — no other namespace in this file carries that label (existing ones are either bare or carry only pod-security.kubernetes.io/*). Harmless, but worth a one-line note on why if it's meant to establish a new convention rather than being incidental.
• The wp-pg-allow-cnpg-operator policy (woodpecker-ns.yaml:495-512) is the first place in this repo's network policies that selects the CNPG operator by app.kubernetes.io/name: cloudnative-pg rather than by namespace alone. I couldn't confirm that label against a live cluster (no kubectl access here) — it matches the upstream chart's default, but since a wrong label here would silently leave issue #433 unfixed rather than erroring, it's worth double-checking with the cilium-dbg policy trace ... --dport 8000 command the runbook already includes before calling stage 1 verified.

Tests

No automated tests apply to this kind of change (Kubernetes/Flux manifests) and the repo doesn't have any for comparable prior migrations. The PR's own manual verification plan (MIGRATION-namespace.md) is thorough and appropriate for the change — dry-run apply, restore health polling, database/owner sanity check, row-count comparison, the two issue-#433/metrics checks, and a cilium-dbg policy trace for the policy semantics --dry-run=server can't catch. I did not have kubectl/kustomize available in this environment to run the dry-run apply myself; I instead manually cross-checked the new manifests against the equivalent live resources (cluster.yaml, objectstore.yaml, hetzner-creds-externalsecret.yaml), the apps/database CiliumNetworkPolicy baselines, the Flux dependsOn chain, and RECOVERY.md's documented recovery-stanza shape — all of which the diff matches correctly, aside from the metrics gap noted above.

Recommendation

Request changes — 1 blocking issue (metrics scraping isn't actually wired up despite being one of the PR's two stated justifications).

🤖 **Automated review** — model `CLI default` (complexity: complex), commit `1b5ad600d5c125bc3970e013fc16b85b4c8966db`. > Advisory review by the `review-bot` pipeline. It never approves; a human merges. Push new commits to get a fresh review. --- ## Summary Stage 1 of moving Woodpecker's `wp-pg` CNPG cluster into a dedicated `woodpecker` namespace: a new Namespace, a scoped baseline `CiliumNetworkPolicy` + an operator-access policy, a barman `ExternalSecret`, two `ObjectStore`s (read-only recovery source + new archiving target), and a `Cluster` bootstrapped via `recovery`. Purely additive — nothing in `apps` is touched, and nothing yet references the new resources. The design (namespace-per-service to avoid `endpointSelector: {}` blast radius, split ObjectStores to avoid WAL interleaving, namespace declared in `infrastructure/namespaces` to respect Flux `dependsOn` ordering, recovery `database`/`owner` mirroring the source cluster's `initdb` values) checks out against the rest of the repo — I traced the Flux `dependsOn` chain (`infrastructure` → `infrastructure-crds` → `woodpecker` KS), the `apps` and `database` namespace `CiliumNetworkPolicy` baselines, and `kubernetes/RECOVERY.md`'s documented recovery-stanza convention, and the PR's claims about all of these hold up. One of its two central justifications, however, doesn't actually land with this diff — see below. ## 🔴 Blocking - **The metrics-scraping fix the PR claims doesn't ship.** The PR body and `MIGRATION-namespace.md` both state that the `apps` namespace's missing `database`/`monitoring` ingress allowances are why `cnpg_collector_up` has no series for `wp-pg`, and that "both are fixed structurally by the move." But Prometheus scraping for every other CNPG cluster in this repo goes through a hand-written `PodMonitor` in `kubernetes/apps/monitoring-rules/cnpg-podmonitors.yaml` (the file's own header explains why: CNPG's `enablePodMonitor` doesn't set the `release: kube-prometheus-stack` label this Prometheus's `podMonitorSelector` requires). That file has entries for `ak-pg`, `im-pg`, `gf-pg`, and `pprl-pg` — but none for `wp-pg`, and this PR doesn't add one, either there or under the new `woodpecker` namespace. Fixing the `CiliumNetworkPolicy` ingress gap (which this PR does correctly do) is necessary but not sufficient: with no `PodMonitor` selecting `cnpg.io/cluster: wp-pg` in the `woodpecker` namespace, Prometheus has no scrape target configured for it at all, network policy aside. The runbook's own verification step (`MIGRATION-namespace.md` lines 140-146, `cnpg_collector_up{namespace="woodpecker"}`) will return empty regardless of whether the new `wp-pg-allow-cnpg-operator` policy is correct — so it can't actually confirm the fix it's meant to confirm. Either add the `PodMonitor` in this PR (mirroring the existing four entries) or adjust the PR description/runbook to stop claiming this is fixed by stage 1. ## 🟡 Suggestions - **The migration `Cluster`'s health is coupled to the live production Kustomization's Ready status.** `kubernetes/clusters/homelab/woodpecker.yaml` sets `wait: true` and a `healthCheckExprs` entry matching `apiVersion: postgresql.cnpg.io/v1, kind: Cluster` with no namespace/name qualifier. Flux evaluates that expression against every resource of that GVK in the Kustomization's inventory — after this PR, that's both `apps/wp-pg` (already healthy) and the new `woodpecker/wp-pg` (mid-restore). Until the restore reaches `Cluster in healthy state`, the whole `woodpecker` Kustomization — which also owns the live server/agent/DB — reports not-Ready, and `kubernetes/infrastructure/flux-notifications/alerts.yaml`'s `flux-failures` Alert (`eventSeverity: error` on `Kustomization: '*'`) will fire on any reconcile-timeout error in the meantime. `infrastructure.yaml`'s own comment in this repo (`wait: false` + explicit `healthChecks`, to stop one crashlooping workload from freezing unrelated dependents) shows this exact failure mode is already a known concern here — this PR reintroduces a milder version of it onto the one Kustomization that owns production Woodpecker. Nothing else `dependsOn: woodpecker`, so it won't cascade further, but it can generate misleading "woodpecker is broken" alerts tied to an experimental parallel restore. Worth at least a mention in the runbook that a lagging/errored restore is expected to make the `woodpecker` KS report not-Ready during that window. - **No `ScheduledBackup` for the migration cluster, so `wp-pg-store`'s `retentionPolicy: "30d"` (`migration-objectstore.yaml:377`) has nothing to retire against.** Every other CNPG cluster in this repo pairs `isWALArchiver: true` with a `ScheduledBackup` (see `apps/woodpecker/scheduled-backup.yaml`), which is what actually produces the periodic base backups a retention policy prunes. `migration-cluster.yaml` archives continuous WAL to the new `wp-pg-woodpecker` prefix but never takes a base backup there. Until stage 2/3 lands one, that prefix accumulates WAL from an unbounded chain with no independent recovery point and no way for the retention policy to reclaim space — a real cost/durability gap if this stage sits for a while before cutover, even though it's not directly harmful today. ## 🟢 Nits - `infrastructure/namespaces/namespaces.yaml:410` adds `labels: app.kubernetes.io/name: woodpecker` to the new Namespace — no other namespace in this file carries that label (existing ones are either bare or carry only `pod-security.kubernetes.io/*`). Harmless, but worth a one-line note on why if it's meant to establish a new convention rather than being incidental. - The `wp-pg-allow-cnpg-operator` policy (`woodpecker-ns.yaml:495-512`) is the first place in this repo's network policies that selects the CNPG operator by `app.kubernetes.io/name: cloudnative-pg` rather than by namespace alone. I couldn't confirm that label against a live cluster (no `kubectl` access here) — it matches the upstream chart's default, but since a wrong label here would silently leave issue #433 unfixed rather than erroring, it's worth double-checking with the `cilium-dbg policy trace ... --dport 8000` command the runbook already includes before calling stage 1 verified. ## Tests No automated tests apply to this kind of change (Kubernetes/Flux manifests) and the repo doesn't have any for comparable prior migrations. The PR's own manual verification plan (`MIGRATION-namespace.md`) is thorough and appropriate for the change — dry-run apply, restore health polling, database/owner sanity check, row-count comparison, the two issue-#433/metrics checks, and a `cilium-dbg policy trace` for the policy semantics `--dry-run=server` can't catch. I did not have `kubectl`/`kustomize` available in this environment to run the dry-run apply myself; I instead manually cross-checked the new manifests against the equivalent live resources (`cluster.yaml`, `objectstore.yaml`, `hetzner-creds-externalsecret.yaml`), the `apps`/`database` `CiliumNetworkPolicy` baselines, the Flux `dependsOn` chain, and `RECOVERY.md`'s documented recovery-stanza shape — all of which the diff matches correctly, aside from the metrics gap noted above. ## Recommendation Request changes — 1 blocking issue (metrics scraping isn't actually wired up despite being one of the PR's two stated justifications). <!-- pr-review:sha=1b5ad600d5c125bc3970e013fc16b85b4c8966db -->

claude-bot referenced this issue from a commit 2026-08-18 20:44:44 +00:00

fix(woodpecker): add the missing PodMonitor and base backup for wp-pg

claude-bot added 1 commit 2026-08-18 20:44:45 +00:00

fix(woodpecker): add the missing PodMonitor and base backup for wp-pg ... e56f1e7cf6

Addresses the third blocking review finding on #504.

The PR claimed the namespace move fixes wp-pg's absent metrics. It does not,
on its own. Prometheus requires the `release: kube-prometheus-stack` label
that CNPG's own enablePodMonitor omits, so every scraped cluster has a
hand-written PodMonitor in apps/monitoring-rules/cnpg-podmonitors.yaml —
ak-pg, im-pg, gf-pg, pprl-pg. There has never been one for wp-pg.

So wp-pg was missing both halves: no scrape target configured, and no
monitoring ingress allow in the apps baseline. Fixing the policy half was
necessary but not sufficient, and the runbook's cnpg_collector_up check
would have returned empty either way — unable to confirm the thing it
existed to confirm. Adds the PodMonitor in the woodpecker namespace.

Also from the review:

- Added a ScheduledBackup for the restored cluster. Every other CNPG
  cluster here pairs isWALArchiver with one. Without it the new
  wp-pg-woodpecker prefix takes continuous WAL but never a base backup, so
  it has no independent recovery point and wp-pg-store's 30d retention has
  nothing to prune against — the WAL chain grows unbounded for as long as
  stage 1 sits before cutover. Scheduled 02:40, clear of the apps cluster's
  02:35 and the other clusters' 02:30.

- Documented that the `woodpecker` KS goes not-Ready while the restore runs.
  Its healthCheckExprs matches Cluster by GVK with no name qualifier, so
  Flux evaluates it against both the live apps/wp-pg and the restoring
  woodpecker/wp-pg, and wait: true makes the KS that owns production
  Woodpecker report unhealthy. Expected, does not cascade since nothing
  dependsOn it, but it would read as an outage without a note.

- Dropped the app.kubernetes.io/name label from the new Namespace; no other
  namespace in that file carries one.

Co-Authored-By: Claude Bot <claude-bot@renner.uno>

claude-bot commented 2026-08-18 20:45:02 +00:00

Author

Member

Copy Link

Copy Source

Right again, and this one is my error rather than an oversight — I had the evidence and drew the wrong conclusion. Fixed in e56f1e7c.

I ran kubectl get podmonitor -A earlier in this work and saw exactly four (cnpg-ak-pg, cnpg-im-pg, cnpg-gf-pg, cnpg-pprl-pg), then still wrote in the PR body that the namespace move fixes metrics. It doesn't. wp-pg was missing both halves — no PodMonitor at all, and no monitoring ingress allow in the apps baseline — and I only fixed the half I had just been looking at.

Your sharpest point is that the runbook's cnpg_collector_up{namespace="woodpecker"} check would have returned empty regardless of whether the policy was right, so it could not confirm the thing it existed to confirm. Added cnpg-wp-pg to cnpg-podmonitors.yaml, plus a kubectl -n woodpecker get podmonitor cnpg-wp-pg step so a missing scrape target is distinguishable from a blocked scrape.

ScheduledBackup — taken. You are right that the retention policy had nothing to prune against. Worse than the cost angle: without a base backup on wp-pg-woodpecker the new prefix has no independent recovery point, so recovery would have to chain back through the source prefix — which undercuts the whole reason the paths are split. Added at 02:40, clear of the apps cluster's 02:35 and the others' 02:30, with immediate: true so the first base backup doubles as end-to-end proof the new credentials and ObjectStore work.

KS health coupling — documented. Confirmed: woodpecker.yaml has wait: true and a healthCheckExprs matching Cluster by GVK with no name qualifier, so Flux evaluates it across both clusters in the inventory. The runbook now says plainly that the KS owning production Woodpecker goes not-Ready while the restore runs, that flux-failures may trip, and that this is expected rather than an outage — with suspend-or-split-the-KS as the options if the window is long.

Nit taken: dropped the app.kubernetes.io/name label; no other namespace in that file carries one, and it was incidental rather than a convention I meant to start.

On the operator-label nit: I verified it against the live pod rather than the chart —

$ kubectl get pod -n database cloudnative-pg-77967b5c9c-zjxs8 -o json | jq '.metadata.labels'
app.kubernetes.io/instance=cloudnative-pg
app.kubernetes.io/name=cloudnative-pg

— so the selector matches what is actually there. Agreed the policy trace remains the real confirmation, since a matching label is necessary but not sufficient; it stays in the runbook.

Three rounds, three genuine defects, all of the same kind: things that render, validate and dry-run cleanly while being wrong at runtime. Worth noting for stage 2, which lands the server and agent and will have more of that surface, not less.

Right again, and this one is my error rather than an oversight — I had the evidence and drew the wrong conclusion. Fixed in e56f1e7c. I ran `kubectl get podmonitor -A` earlier in this work and saw exactly four (`cnpg-ak-pg`, `cnpg-im-pg`, `cnpg-gf-pg`, `cnpg-pprl-pg`), then still wrote in the PR body that the namespace move fixes metrics. It doesn't. wp-pg was missing **both** halves — no PodMonitor at all, and no monitoring ingress allow in the `apps` baseline — and I only fixed the half I had just been looking at. Your sharpest point is that the runbook's `cnpg_collector_up{namespace="woodpecker"}` check would have returned empty **regardless of whether the policy was right**, so it could not confirm the thing it existed to confirm. Added `cnpg-wp-pg` to `cnpg-podmonitors.yaml`, plus a `kubectl -n woodpecker get podmonitor cnpg-wp-pg` step so a missing scrape target is distinguishable from a blocked scrape. **ScheduledBackup — taken.** You are right that the retention policy had nothing to prune against. Worse than the cost angle: without a base backup on `wp-pg-woodpecker` the new prefix has no independent recovery point, so recovery would have to chain back through the source prefix — which undercuts the whole reason the paths are split. Added at 02:40, clear of the apps cluster's 02:35 and the others' 02:30, with `immediate: true` so the first base backup doubles as end-to-end proof the new credentials and ObjectStore work. **KS health coupling — documented.** Confirmed: `woodpecker.yaml` has `wait: true` and a `healthCheckExprs` matching `Cluster` by GVK with no name qualifier, so Flux evaluates it across both clusters in the inventory. The runbook now says plainly that the KS owning production Woodpecker goes not-Ready while the restore runs, that `flux-failures` may trip, and that this is expected rather than an outage — with suspend-or-split-the-KS as the options if the window is long. **Nit taken:** dropped the `app.kubernetes.io/name` label; no other namespace in that file carries one, and it was incidental rather than a convention I meant to start. **On the operator-label nit:** I verified it against the live pod rather than the chart — ``` $ kubectl get pod -n database cloudnative-pg-77967b5c9c-zjxs8 -o json | jq '.metadata.labels' app.kubernetes.io/instance=cloudnative-pg app.kubernetes.io/name=cloudnative-pg ``` — so the selector matches what is actually there. Agreed the `policy trace` remains the real confirmation, since a matching label is necessary but not sufficient; it stays in the runbook. Three rounds, three genuine defects, all of the same kind: things that render, validate and dry-run cleanly while being wrong at runtime. Worth noting for stage 2, which lands the server and agent and will have more of that surface, not less.

review-bot reviewed 2026-08-18 20:49:31 +00:00

review-bot left a comment

Member

Copy Link

Copy Source

🤖 Automated review — model CLI default (complexity: complex), commit e56f1e7cf6d84f74928aaaa83964f1c79cdfab53.

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

---

Summary

Stage 1 of moving Woodpecker's wp-pg CNPG cluster out of the shared apps namespace: a new woodpecker Namespace, scoped CiliumNetworkPolicies, a parallel-run restore-from-barman Cluster, split source/target ObjectStores, an ExternalSecret, a ScheduledBackup, and a PodMonitor. Nothing cuts over yet. I cross-checked every factual claim in the PR body and MIGRATION-namespace.md against the actual checkout (Kustomization dependsOn chains, the apps baseline policy, RECOVERY.md's recovery-cluster pattern, existing *-ns.yaml conventions, the woodpecker KS healthCheckExprs) and they all hold up — this is unusually well-grounded for an infra PR. No blocking issues found.

🟡 Suggestions

• kubernetes/infrastructure/network-policies/woodpecker-ns.yaml:549-581 (default-deny-ingress) — this is the only one of the repo's 19 *-ns.yaml baseline policies with no ICMP allow rule (compare paperless-ns.yaml, immich-ns.yaml, database-ns.yaml, woodpecker-ci-ns.yaml, all of which carry the same icmps: block referencing home-assistant-ns.yaml's rationale: "If any layer 4 policy is specified, then ICMP will be blocked unless it's related to a connection that is otherwise allowed"). The companion policy wp-pg-allow-cnpg-operator in the same file does add an L4 (toPorts: 8000) rule scoped to the wp-pg pods, which is exactly the condition that triggers this Cilium behavior per the repo's own documented experience (widened in commit 3c517a0 after being observed breaking things). Net effect: PMTUD Destination Unreachable and similar ICMP to wp-pg pods may get silently dropped once this policy set is live. Worth adding the same ICMP block the other baselines use, or noting explicitly why wp-pg is exempt.

• kubernetes/infrastructure/network-policies/woodpecker-ns.yaml:601-606 — wp-pg-allow-cnpg-operator scopes the database namespace source to app.kubernetes.io/name: cloudnative-pg (in addition to the namespace label). This is a real improvement over paperless-ns.yaml/immich-ns.yaml, which allow the whole database namespace — but it's also the first place in the repo that label-scopes an allow onto the CNPG operator, so there's no prior file in this tree confirming that label matches the operator Deployment's actual pod labels (it's a HelmRelease install of the upstream cloudnative-pg chart, so it's very likely correct by convention, but unconfirmed here). If the label is wrong, the operator's :8000 status poll fails exactly like issue #433, silently reintroducing the bug this PR exists to fix — with apply --dry-run=server unable to catch it, since it validates shape, not Cilium semantics. The migration doc already has a cilium-dbg policy trace --dport 8000 verification step for this, which is the right check — just flagging that this specific rule is the one load-bearing assumption the whole fix rests on.

🟢 Nits

None.

Tests

This is a GitOps manifest change with no unit-test equivalent; the relevant automated gate is .woodpecker/ci-manifests-test.yml's kustomize-build step, which builds every kustomization.yaml in the tree (including the new resources entries in kubernetes/apps/woodpecker/kustomization.yaml and kubernetes/infrastructure/network-policies/kustomization.yaml) and would catch a bad reference or malformed YAML. It cannot validate Cilium policy semantics or that the restore actually produces a healthy, non-empty cluster — the PR body and MIGRATION-namespace.md correctly identify this gap and provide a manual runbook (row-count comparison, cilium-dbg policy trace, cnpg_collector_up query) to run post-merge. That's the right shape for this kind of change; I'd only add the ICMP question above to that verification pass.

Recommendation

Looks reasonable — no blocking issues; comments above are optional.

🤖 **Automated review** — model `CLI default` (complexity: complex), commit `e56f1e7cf6d84f74928aaaa83964f1c79cdfab53`. > Advisory review by the `review-bot` pipeline. It never approves; a human merges. Push new commits to get a fresh review. --- ## Summary Stage 1 of moving Woodpecker's `wp-pg` CNPG cluster out of the shared `apps` namespace: a new `woodpecker` Namespace, scoped CiliumNetworkPolicies, a parallel-run restore-from-barman `Cluster`, split source/target `ObjectStore`s, an `ExternalSecret`, a `ScheduledBackup`, and a `PodMonitor`. Nothing cuts over yet. I cross-checked every factual claim in the PR body and `MIGRATION-namespace.md` against the actual checkout (Kustomization `dependsOn` chains, the `apps` baseline policy, `RECOVERY.md`'s recovery-cluster pattern, existing `*-ns.yaml` conventions, the `woodpecker` KS `healthCheckExprs`) and they all hold up — this is unusually well-grounded for an infra PR. No blocking issues found. ## 🟡 Suggestions - `kubernetes/infrastructure/network-policies/woodpecker-ns.yaml:549-581` (`default-deny-ingress`) — this is the only one of the repo's 19 `*-ns.yaml` baseline policies with no ICMP allow rule (compare `paperless-ns.yaml`, `immich-ns.yaml`, `database-ns.yaml`, `woodpecker-ci-ns.yaml`, all of which carry the same `icmps:` block referencing `home-assistant-ns.yaml`'s rationale: "If any layer 4 policy is specified, then ICMP will be blocked unless it's related to a connection that is otherwise allowed"). The companion policy `wp-pg-allow-cnpg-operator` in the same file *does* add an L4 (`toPorts: 8000`) rule scoped to the `wp-pg` pods, which is exactly the condition that triggers this Cilium behavior per the repo's own documented experience (widened in commit `3c517a0` after being observed breaking things). Net effect: PMTUD `Destination Unreachable` and similar ICMP to `wp-pg` pods may get silently dropped once this policy set is live. Worth adding the same ICMP block the other baselines use, or noting explicitly why `wp-pg` is exempt. - `kubernetes/infrastructure/network-policies/woodpecker-ns.yaml:601-606` — `wp-pg-allow-cnpg-operator` scopes the `database` namespace source to `app.kubernetes.io/name: cloudnative-pg` (in addition to the namespace label). This is a real improvement over `paperless-ns.yaml`/`immich-ns.yaml`, which allow the whole `database` namespace — but it's also the first place in the repo that label-scopes an allow onto the CNPG operator, so there's no prior file in this tree confirming that label matches the operator Deployment's actual pod labels (it's a `HelmRelease` install of the upstream `cloudnative-pg` chart, so it's very likely correct by convention, but unconfirmed here). If the label is wrong, the operator's `:8000` status poll fails exactly like issue #433, silently reintroducing the bug this PR exists to fix — with `apply --dry-run=server` unable to catch it, since it validates shape, not Cilium semantics. The migration doc already has a `cilium-dbg policy trace --dport 8000` verification step for this, which is the right check — just flagging that this specific rule is the one load-bearing assumption the whole fix rests on. ## 🟢 Nits None. ## Tests This is a GitOps manifest change with no unit-test equivalent; the relevant automated gate is `.woodpecker/ci-manifests-test.yml`'s `kustomize-build` step, which builds every `kustomization.yaml` in the tree (including the new `resources` entries in `kubernetes/apps/woodpecker/kustomization.yaml` and `kubernetes/infrastructure/network-policies/kustomization.yaml`) and would catch a bad reference or malformed YAML. It cannot validate Cilium policy semantics or that the restore actually produces a healthy, non-empty cluster — the PR body and `MIGRATION-namespace.md` correctly identify this gap and provide a manual runbook (row-count comparison, `cilium-dbg policy trace`, `cnpg_collector_up` query) to run post-merge. That's the right shape for this kind of change; I'd only add the ICMP question above to that verification pass. ## Recommendation Looks reasonable — no blocking issues; comments above are optional. <!-- pr-review:sha=e56f1e7cf6d84f74928aaaa83964f1c79cdfab53 -->

claude-bot referenced this issue from a commit 2026-08-18 20:52:38 +00:00

fix(woodpecker): allow ICMP to the new namespace's endpoints

claude-bot added 1 commit 2026-08-18 20:52:38 +00:00

fix(woodpecker): allow ICMP to the new namespace's endpoints ... 4996a6f738

From the fourth review on #504 — no blocking issues, but this one is a real
gap rather than a style point.

The woodpecker baseline was the only one of the repo's namespace policies
without an ICMP allow. That matters here specifically: wp-pg-allow-cnpg-operator
attaches an L4 rule (toPorts :8000) to the wp-pg pods, and Cilium blocks ICMP
to an endpoint once any layer 4 policy selects it unless the traffic is
CT-related. So PMTUD Destination Unreachable and Packet Too Big toward wp-pg
would have been dropped silently, surfacing as stalled transfers rather than
as a policy denial.

Uses the 16-type set the other baselines now carry after #495 trimmed them
from 39, rather than reintroducing the old list.

Granting this namespace-wide does not contradict the caution comment above it.
The :8000 case had to be per-workload because that port is shared between the
woodpecker server and the CNPG instance manager's admin API, so a blanket rule
handed CI step pods a route to the database. ICMP echo and path-MTU messages
expose no API surface, so a namespace-wide grant concedes nothing a scoped one
would withhold — which is why every other baseline does it this way too.

Co-Authored-By: Claude Bot <claude-bot@renner.uno>

claude-bot commented 2026-08-18 20:52:54 +00:00

Author

Member

Copy Link

Copy Source

Clean review — no blocking issues. Took the ICMP suggestion in 4996a6f7; it is a real gap, not a consistency nit.

The reasoning holds exactly as stated. The baseline itself specifies no L4, but wp-pg-allow-cnpg-operator attaches toPorts: 8000 to the wp-pg pods — which is precisely the condition that makes Cilium drop ICMP to those endpoints unless it is CT-related. So PMTUD Destination Unreachable and Packet Too Big toward wp-pg would have been dropped, and that failure presents as stalled transfers rather than a policy denial. Exactly the kind of thing that gets debugged for hours as a storage or network problem.

Worth noting this connects to work already merged: #495 trimmed these ICMP blocks from 39 enumerated types to 16 across all 19 namespace policies, after finding the enumeration was generating ~8,500 policy-map entries per direction and overflowing Cilium's BPF map. I used the current 16-type set rather than reintroducing the old list.

On granting it namespace-wide — that is deliberate and does not contradict the caution comment I added above it. The :8000 rule had to be per-workload because that port is shared between the Woodpecker server and the CNPG instance manager's admin API, so a blanket grant handed CI step pods a route to the database. ICMP echo and path-MTU messages expose no API surface, so scoping them per-workload would withhold nothing. The distinction is now written into the file so the next person does not have to re-derive it.

On the load-bearing-assumption point — agreed, and that is the right way to characterise it. The operator label was taken from the live pod rather than the chart:

app.kubernetes.io/instance=cloudnative-pg
app.kubernetes.io/name=cloudnative-pg

so it matches what is actually deployed. But a matching label is necessary, not sufficient, and a wrong one here fails silently as #433 all over again — which is why the cilium-dbg policy trace --dport 8000 ALLOWED case stays first in the post-merge verification rather than last.

Across four rounds this review caught three genuine defects and one real gap, every one of them invisible to kustomize build and --dry-run=server: a policy scoped wider than intended, a bootstrap defaulting to the wrong database, a metric with no scrape target, and now ICMP blocked as a side effect of an L4 rule. Stage 2 lands the server and agent and has more of that surface, not less.

Clean review — no blocking issues. Took the ICMP suggestion in 4996a6f7; it is a real gap, not a consistency nit. **The reasoning holds exactly as stated.** The baseline itself specifies no L4, but `wp-pg-allow-cnpg-operator` attaches `toPorts: 8000` to the wp-pg pods — which is precisely the condition that makes Cilium drop ICMP to those endpoints unless it is CT-related. So PMTUD `Destination Unreachable` and `Packet Too Big` toward wp-pg would have been dropped, and that failure presents as stalled transfers rather than a policy denial. Exactly the kind of thing that gets debugged for hours as a storage or network problem. Worth noting this connects to work already merged: **#495** trimmed these ICMP blocks from 39 enumerated types to 16 across all 19 namespace policies, after finding the enumeration was generating ~8,500 policy-map entries per direction and overflowing Cilium's BPF map. I used the current 16-type set rather than reintroducing the old list. **On granting it namespace-wide** — that is deliberate and does not contradict the caution comment I added above it. The `:8000` rule had to be per-workload because that port is shared between the Woodpecker server and the CNPG instance manager's admin API, so a blanket grant handed CI step pods a route to the database. ICMP echo and path-MTU messages expose no API surface, so scoping them per-workload would withhold nothing. The distinction is now written into the file so the next person does not have to re-derive it. **On the load-bearing-assumption point** — agreed, and that is the right way to characterise it. The operator label was taken from the live pod rather than the chart: ``` app.kubernetes.io/instance=cloudnative-pg app.kubernetes.io/name=cloudnative-pg ``` so it matches what is actually deployed. But a matching label is necessary, not sufficient, and a wrong one here fails silently as #433 all over again — which is why the `cilium-dbg policy trace --dport 8000` ALLOWED case stays first in the post-merge verification rather than last. Across four rounds this review caught three genuine defects and one real gap, every one of them invisible to `kustomize build` and `--dry-run=server`: a policy scoped wider than intended, a bootstrap defaulting to the wrong database, a metric with no scrape target, and now ICMP blocked as a side effect of an L4 rule. Stage 2 lands the server and agent and has more of that surface, not less.

review-bot reviewed 2026-08-18 20:56:28 +00:00

review-bot left a comment

Member

Copy Link

Copy Source

🤖 Automated review — model CLI default (complexity: complex), commit 4996a6f738daba26179825eebed8e47af04e3206.

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

---

Summary

Stage 1 of a multi-stage move of wp-pg (and eventually the Woodpecker server/agent) out of the shared apps namespace into a dedicated woodpecker namespace, restoring the CNPG cluster from a Barman backup in parallel with the live cluster. I checked the CNPG recovery config against kubernetes/RECOVERY.md's documented pattern, the Cilium policy against the repo's existing -ns.yaml conventions, and the Flux dependsOn/KS-ordering claims against the actual Kustomization manifests — all check out. No blocking issues found.

🟡 Suggestions

• kubernetes/infrastructure/network-policies/woodpecker-ns.yaml:635-641 (wp-pg-allow-cnpg-operator) scopes the source side to "k8s:io.kubernetes.pod.namespace": database and app.kubernetes.io/name: cloudnative-pg. That's tighter than every existing precedent in this repo — authentik-ns.yaml, paperless-ns.yaml, immich-ns.yaml and monitoring-ns.yaml all just allow the whole database namespace with no pod-label scoping (confirmed by grepping those files). Good, but it also means this is the first CNP in the repo to depend on that exact label existing on the operator pod, and I could not confirm it from the checkout — kubernetes/infrastructure/cloudnative-pg/helmrelease.yaml only pins the upstream chart, no vendored manifest is in-repo to check against. If the label is wrong (e.g. a different app.kubernetes.io/name value than cloudnative-pg), the operator's status polls will be silently dropped and wp-pg will reproduce the exact Instance Status Extraction Error this migration exists to fix — just in the new namespace. Low blast radius here since stage 1 carries no CI/production impact and the runbook already includes a step to check this (MIGRATION-namespace.md's cilium-dbg policy trace --src-k8s-pod database:cloudnative-pg), but it'd be worth running kubectl -n database get pods --show-labels once and confirming the label before or right after merge, rather than relying solely on the cluster going Ready as the signal.

🟢 Nits

• kubernetes/apps/woodpecker/migration-cluster.yaml:3,16 says "cutover (PR 2)" / "gains the guard in PR 3", while every other doc comment in this PR (MIGRATION-namespace.md, kustomization.yaml, migration-scheduled-backup.yaml) consistently says "stage 2" / "stage 3". Cosmetic only.

Tests

No automated tests apply to this kind of change (Kustomize/Flux manifests). The PR substitutes a thorough manual runbook (MIGRATION-namespace.md) covering restore health, correct database/owner naming (a real footgun with CNPG's bootstrap.recovery defaulting to app/app), row-count comparison against the live cluster, PodMonitor registration, and — notably — an explicit cilium-dbg policy trace step to verify policy behavior rather than just manifest shape. That's appropriately thorough for infra-as-code of this shape, and directly addresses the one thing I couldn't verify from static review (see Suggestions).

I independently verified the claims that are checkable from the repo:

• bootstrap.recovery / externalClusters shape in migration-cluster.yaml matches the documented pattern in kubernetes/RECOVERY.md exactly, including serverName: wp-pg being the last path segment of the source destinationPath (s3://renner-uno-k8s-cnpg/wp-pg), per that doc's own rule.
• The namespace-before-policy ordering claim is correct: kubernetes/infrastructure/kustomization.yaml includes namespaces/ (so the woodpecker Namespace ships with the infrastructure KS), and infrastructure/post-crds/kustomization.yaml includes ../network-policies/ while infrastructure-crds's Kustomization dependsOn: [infrastructure] — so the namespace exists before its CiliumNetworkPolicies apply, and the woodpecker workload KS's own dependsOn includes both.
• The healthCheckExprs claim in MIGRATION-namespace.md is accurate: kubernetes/clusters/homelab/woodpecker.yaml has wait: true and an unqualified postgresql.cnpg.io/v1 Cluster health check, and both apps/wp-pg (cluster.yaml) and the new woodpecker/wp-pg (migration-cluster.yaml) are resources in the same woodpecker KS inventory — so the KS will legitimately go not-Ready while the restore is in progress, as documented.
• The ICMP addition to woodpecker-ns.yaml (already folded into this PR's head commit) is byte-for-byte consistent with the established pattern in home-assistant-ns.yaml/database-ns.yaml, ingress-only as required.
• No secrets are committed in plaintext; credentials flow through the existing ExternalSecret → OpenBao path, same as the rest of the fleet.

Recommendation

Looks reasonable — no blocking issues; comments above are optional.

🤖 **Automated review** — model `CLI default` (complexity: complex), commit `4996a6f738daba26179825eebed8e47af04e3206`. > Advisory review by the `review-bot` pipeline. It never approves; a human merges. Push new commits to get a fresh review. --- ## Summary Stage 1 of a multi-stage move of `wp-pg` (and eventually the Woodpecker server/agent) out of the shared `apps` namespace into a dedicated `woodpecker` namespace, restoring the CNPG cluster from a Barman backup in parallel with the live cluster. I checked the CNPG recovery config against `kubernetes/RECOVERY.md`'s documented pattern, the Cilium policy against the repo's existing `-ns.yaml` conventions, and the Flux `dependsOn`/KS-ordering claims against the actual Kustomization manifests — all check out. No blocking issues found. ## 🟡 Suggestions - `kubernetes/infrastructure/network-policies/woodpecker-ns.yaml:635-641` (`wp-pg-allow-cnpg-operator`) scopes the source side to `"k8s:io.kubernetes.pod.namespace": database` **and** `app.kubernetes.io/name: cloudnative-pg`. That's tighter than every existing precedent in this repo — `authentik-ns.yaml`, `paperless-ns.yaml`, `immich-ns.yaml` and `monitoring-ns.yaml` all just allow the whole `database` namespace with no pod-label scoping (confirmed by grepping those files). Good, but it also means this is the first CNP in the repo to depend on that exact label existing on the operator pod, and I could not confirm it from the checkout — `kubernetes/infrastructure/cloudnative-pg/helmrelease.yaml` only pins the upstream chart, no vendored manifest is in-repo to check against. If the label is wrong (e.g. a different `app.kubernetes.io/name` value than `cloudnative-pg`), the operator's status polls will be silently dropped and `wp-pg` will reproduce the exact `Instance Status Extraction Error` this migration exists to fix — just in the new namespace. Low blast radius here since stage 1 carries no CI/production impact and the runbook already includes a step to check this (`MIGRATION-namespace.md`'s `cilium-dbg policy trace --src-k8s-pod database:cloudnative-pg`), but it'd be worth running `kubectl -n database get pods --show-labels` once and confirming the label before or right after merge, rather than relying solely on the cluster going `Ready` as the signal. ## 🟢 Nits - `kubernetes/apps/woodpecker/migration-cluster.yaml:3,16` says "cutover (PR 2)" / "gains the guard in PR 3", while every other doc comment in this PR (`MIGRATION-namespace.md`, `kustomization.yaml`, `migration-scheduled-backup.yaml`) consistently says "stage 2" / "stage 3". Cosmetic only. ## Tests No automated tests apply to this kind of change (Kustomize/Flux manifests). The PR substitutes a thorough manual runbook (`MIGRATION-namespace.md`) covering restore health, correct database/owner naming (a real footgun with CNPG's `bootstrap.recovery` defaulting to `app`/`app`), row-count comparison against the live cluster, PodMonitor registration, and — notably — an explicit `cilium-dbg policy trace` step to verify policy *behavior* rather than just manifest shape. That's appropriately thorough for infra-as-code of this shape, and directly addresses the one thing I couldn't verify from static review (see Suggestions). I independently verified the claims that are checkable from the repo: - `bootstrap.recovery` / `externalClusters` shape in `migration-cluster.yaml` matches the documented pattern in `kubernetes/RECOVERY.md` exactly, including `serverName: wp-pg` being the last path segment of the source `destinationPath` (`s3://renner-uno-k8s-cnpg/wp-pg`), per that doc's own rule. - The namespace-before-policy ordering claim is correct: `kubernetes/infrastructure/kustomization.yaml` includes `namespaces/` (so the `woodpecker` Namespace ships with the `infrastructure` KS), and `infrastructure/post-crds/kustomization.yaml` includes `../network-policies/` while `infrastructure-crds`'s Kustomization `dependsOn: [infrastructure]` — so the namespace exists before its CiliumNetworkPolicies apply, and the `woodpecker` workload KS's own `dependsOn` includes both. - The `healthCheckExprs` claim in `MIGRATION-namespace.md` is accurate: `kubernetes/clusters/homelab/woodpecker.yaml` has `wait: true` and an unqualified `postgresql.cnpg.io/v1 Cluster` health check, and both `apps/wp-pg` (`cluster.yaml`) and the new `woodpecker/wp-pg` (`migration-cluster.yaml`) are resources in the same `woodpecker` KS inventory — so the KS will legitimately go not-Ready while the restore is in progress, as documented. - The ICMP addition to `woodpecker-ns.yaml` (already folded into this PR's head commit) is byte-for-byte consistent with the established pattern in `home-assistant-ns.yaml`/`database-ns.yaml`, ingress-only as required. - No secrets are committed in plaintext; credentials flow through the existing `ExternalSecret` → OpenBao path, same as the rest of the fleet. ## Recommendation Looks reasonable — no blocking issues; comments above are optional. <!-- pr-review:sha=4996a6f738daba26179825eebed8e47af04e3206 -->

k.renner merged commit e102e3c259 into master 2026-08-18 20:56:43 +00:00

k.renner referenced this issue from a commit 2026-08-18 20:56:44 +00:00

Merge pull request 'feat(woodpecker): stage 1 — restore wp-pg into a dedicated namespace' (#504) from migrate-woodpecker-namespace into master

claude-bot referenced this issue from a commit 2026-08-18 21:00:34 +00:00

fix(eso): allow the woodpecker namespace to use the openbao store

claude-bot referenced this pull request 2026-08-18 21:00:51 +00:00

fix(eso): allow the woodpecker namespace to use the openbao store #505

claude-bot referenced this pull request 2026-08-18 21:29:37 +00:00

fix(woodpecker): scope traefik ingress to the server, and record the cutover data gap #507

Sign in to join this conversation.

Reviewers

No Reviewers

review-bot

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#504