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

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>
2026-08-18 22:25:32 +02:00
parent 85268c1e79
commit b0ddbe0dfd
4 changed files with 66 additions and 35 deletions
@@ -56,9 +56,18 @@ and independently restorable.

 **The namespace is declared in `infrastructure/namespaces/namespaces.yaml`,**
 not alongside these workloads. Its CiliumNetworkPolicies ship with the
-`infrastructure` KS, and the `woodpecker` KS `dependsOn` `infrastructure` —
-creating the namespace in the workload KS would invert that order and deadlock
-a cold rebuild.
+`infrastructure-crds` KS (`infrastructure/post-crds` includes
+`../network-policies/`), which `dependsOn` `infrastructure` — so the namespace
+exists before any policy targets it. Creating it in the `woodpecker` workload
+KS would invert that order, since that KS runs after both.
+
+**Ingress from `woodpecker-ci` is deliberately absent in stage 1.** Step pods do
+call the server back on `:8000`, but the CNPG instance manager serves its status
+API on `:8000` too, and the baseline policy uses `endpointSelector: {}`. A
+`toPorts` block narrows the port, not the destination workload — so putting that
+rule on the baseline would hand arbitrary repo-supplied build steps a route to
+wp-pg's instance manager. It lands in stage 2 as its own policy selecting the
+server pod.

 **No `prune: disabled` guard on the restored cluster.** The original carries one
 because it holds the only copy of the data. This one is reconstructable by
@@ -120,6 +129,25 @@ kubectl -n woodpecker get cluster wp-pg -o jsonpath='{.status.conditions}'
 kubectl get --raw '/api/v1/namespaces/monitoring/services/kube-prometheus-stack-prometheus:9090/proxy/api/v1/query?query=cnpg_collector_up{namespace="woodpecker"}'
 ```

+**Check the policy semantics by hand.** `kubectl --dry-run=server` validates
+manifest shape, not Cilium behaviour, so it cannot catch a rule that is
+syntactically fine but scoped wider than intended. Confirm what actually reaches
+wp-pg's instance-manager port — the operator should, a CI step pod should not:
+
+```sh
+CILIUM=$(kubectl -n kube-system get pod -l k8s-app=cilium \
+  --field-selector spec.nodeName=$(kubectl -n woodpecker get pod wp-pg-1 -o jsonpath='{.spec.nodeName}') \
+  -o name | head -1)
+
+# Expect ALLOWED
+kubectl -n kube-system exec ${CILIUM#pod/} -c cilium-agent -- cilium-dbg policy trace \
+  --src-k8s-pod database:cloudnative-pg --dst-k8s-pod woodpecker:wp-pg-1 --dport 8000
+
+# Expect DENIED — arbitrary build steps must not reach the database
+kubectl -n kube-system exec ${CILIUM#pod/} -c cilium-agent -- cilium-dbg policy trace \
+  --src-k8s-pod woodpecker-ci:any --dst-k8s-pod woodpecker:wp-pg-1 --dport 8000
+```
+
 And that archiving goes to the new prefix, leaving the old one alone:

 ```sh
@@ -38,9 +38,8 @@ resources:
  # is stage 3. Runbook: MIGRATION-namespace.md.
  #
  # The `woodpecker` Namespace itself is declared in infrastructure/namespaces,
-  # not here: its CiliumNetworkPolicies ship with the `infrastructure` KS, which
-  # this KS dependsOn, so creating the namespace here would invert that order
-  # and deadlock a cold rebuild.
+  # not here: it must exist before the `infrastructure-crds` KS applies the
+  # namespace's CiliumNetworkPolicies, and this KS runs after both of those.
  - migration-externalsecret-hetzner.yaml
  - migration-objectstore.yaml
  - migration-cluster.yaml
@@ -172,9 +172,11 @@ metadata:
 # `database`. A namespace per service keeps that grant on the one workload.
 #
 # Declared here rather than alongside the workloads so it is created by the
-# `infrastructure` KS: the namespace's CiliumNetworkPolicies live in
-# infrastructure/network-policies, and the `woodpecker` KS dependsOn
-# `infrastructure`. Creating it in the workload KS would invert that order.
+# `infrastructure` KS. The namespace's CiliumNetworkPolicies ship with the
+# `infrastructure-crds` KS (infrastructure/post-crds includes
+# ../network-policies/), which itself dependsOn `infrastructure` — so the
+# namespace exists before any policy targets it. Creating it in the `woodpecker`
+# workload KS would invert that order, since that KS runs after both.
 #
 # No pod-security labels: the cluster default applies.
 apiVersion: v1
@@ -1,11 +1,25 @@
 ---
-# Baseline ingress policy for the `woodpecker` namespace (server, agent, wp-pg).
+# Baseline ingress policy for the `woodpecker` namespace (wp-pg now; server and
+# agent arrive in stage 2).
 #
 # Unlike the `apps` baseline this replaces, every allow here lands on a
 # namespace holding one service, so a namespace-scoped grant is not a blanket
 # grant across unrelated workloads. That is the point of the move: in `apps`,
 # letting the CNPG operator reach wp-pg would also have opened gitea, gatus,
 # external-dns and ical-proxy to the whole database namespace (issue #433).
+#
+# CAUTION when adding rules here: this policy has `endpointSelector: {}`, so
+# every rule below applies to EVERY pod in the namespace. A `toPorts` block
+# narrows the port, NOT the destination workload — that is governed solely by
+# the enclosing policy's endpointSelector. Anything meant for one workload
+# belongs in its own policy with its own selector, as
+# `wp-pg-allow-cnpg-operator` below does.
+#
+# No egress section, so egress is unrestricted. Deliberate for now and revisited
+# in stage 2: wp-pg needs DNS plus `world` for WAL archiving to Hetzner S3, and
+# the server/agent add gitea, the Kubernetes API and outbound plugin pulls.
+# Writing that ruleset once, with the full workload set present, beats guessing
+# it here and silently breaking the WAL archiving this stage exists to verify.
 apiVersion: cilium.io/v2
 kind: CiliumNetworkPolicy
 metadata:
@@ -18,29 +32,27 @@ spec:
    - fromEndpoints:
        - matchLabels:
            "k8s:io.kubernetes.pod.namespace": woodpecker
-    # Traefik terminates ci.renner.uno and forwards to the server.
+    # Traefik terminates ci.renner.uno and forwards to the server (stage 2).
    - fromEndpoints:
        - matchLabels:
            "k8s:io.kubernetes.pod.namespace": ingress
-    # Prometheus scrape.
+    # Prometheus scrape. Namespace-wide and port-unrestricted, matching the
+    # other *-ns baselines; this is what covers wp-pg's :9187 metrics.
    - fromEndpoints:
        - matchLabels:
            "k8s:io.kubernetes.pod.namespace": monitoring
-    # Pipeline step pods in the privileged CI namespace call back to the server
-    # to stream logs and report status. Scoped to the server's :8000 rather than
-    # granted namespace-wide — step pods run arbitrary repo-supplied code, so
-    # they must not reach wp-pg.
-    - fromEndpoints:
-        - matchLabels:
-            "k8s:io.kubernetes.pod.namespace": woodpecker-ci
-      toPorts:
-        - ports:
-            - port: "8000"
-              protocol: TCP
    # Kubelet probes and hostNetwork siblings.
    - fromEntities:
        - host
        - remote-node
+    #
+    # NOT here: ingress from `woodpecker-ci`. Pipeline step pods do call the
+    # server back on :8000 to stream logs, but the CNPG instance manager serves
+    # its status API on :8000 too — so granting that on this endpointSelector:{}
+    # policy would hand arbitrary repo-supplied build steps a route to wp-pg's
+    # instance manager. It lands in stage 2 as its own policy selecting the
+    # server pod, the way woodpecker-allow-rotators already scopes its :8000
+    # rule in apps/woodpecker/networkpolicy.yaml.
 ---
 # The CNPG operator polls each instance's status endpoint over HTTPS
 # (https://<pod-ip>:8000/pg/status) to populate Cluster status. Without it the
@@ -49,8 +61,8 @@ spec:
 # switchover decisions — issue #433, where the operator's dial timed out
 # because the apps baseline never allowed the database namespace.
 #
-# Scoped to the wp-pg endpoints and that one port. The operator has no reason
-# to reach the woodpecker server or agent, and the barman/mariadb operators
+# Scoped to the wp-pg endpoints and that one port: the operator has no reason to
+# reach the woodpecker server or agent, and the barman and mariadb operators
 # sharing its namespace have no reason to reach anything here at all.
 apiVersion: cilium.io/v2
 kind: CiliumNetworkPolicy
@@ -70,13 +82,3 @@ spec:
        - ports:
            - port: "8000"
              protocol: TCP
-    # CNPG instances expose Postgres metrics on :9187. The apps namespace never
-    # allowed this either, which is why cnpg_collector_up has series for
-    # authentik, immich, monitoring and paperless but not for wp-pg.
-    - fromEndpoints:
-        - matchLabels:
-            "k8s:io.kubernetes.pod.namespace": monitoring
-      toPorts:
-        - ports:
-            - port: "9187"
-              protocol: TCP