test(ci): assert a chart's egress ports against the POD ports of the chart it dials
Validate Helm Charts / Detect changed charts (pull_request) Successful in 3s
Validate Helm Charts / Resource quantities (pull_request) Successful in 10s
Validate Helm Charts / Selector match (pull_request) Successful in 9s
Validate Helm Charts / HTTPRoute match (pull_request) Successful in 9s
Validate Helm Charts / NetworkPolicy (pull_request) Successful in 12s
Validate Helm Charts / Bump level (pull_request) Successful in 4s
Validate Helm Charts / YAML scalars (pull_request) Successful in 10s
Validate Helm Charts / Build paperless-ngx (pull_request) Successful in 6s
Validate Helm Charts / Trivy config paperless-ngx (pull_request) Successful in 20s
Install-test / Install-test paperless-ngx (pull_request) Successful in 3m2s
Validate Helm Charts / Install-test (pull_request) Successful in 3m2s

The four assertions from Gitea #5 hold ports against the pod spec the policy
SELECTS. That is right for ingress and meaningless for egress: an egress port
belongs to the destination, and for a database, a cache or the web that
destination is in no render this repository has. So the mistake behind #136 -
an egress rule naming a ClusterIP Service's port, which kube-proxy has already
translated away by the time the policy is evaluated at the pod's interface - was
not catchable within one chart.

It is catchable across two, and paperless-ngx -> apache-tika, gotenberg is the
only place in the repository where a chart dials another chart's Service (the
three other policies naming port 80 aim at the internet, where there is no DNAT
and 80 is the real port). The pairs are an explicit table rather than a
discovery, because no render can know which Service another chart dials - the
address is a value or a 1Password field - and a row naming a chart that no
longer exists fails instead of being skipped.

Each row is checked on two renders of the consumer: with the feature gate ON the
egress ports must CONTAIN every pod port behind the peers' Services, resolved
Service port -> targetPort -> containerPort, which is exactly the translation the
policy is evaluated after; with the gate OFF the requirement must not apply and
those ports must be absent, so a gated rule cannot quietly become one that
permits what the release cannot use. The evidence line prints the chain
("port 80 -> targetPort 'tika-port' -> pod port 9998"), so a reader sees the
DNAT rather than two numbers.

Built into run-networkpolicy-test.sh rather than added as a new script with its
own job: that job already runs over every chart on every PR, Helm is already the
YAML parser through ci/yaml2json, and the verdict and summary plumbing is
shared. python3 standard library only, no yq, no pyyaml, no Docker.

Six fail paths executed, not assumed: a required port removed, a wrong number in
its place, the peer chart renamed away, the policy name no longer following the
<release>-<chart>-<kind> convention, the peers rendering no Service at all (an
empty requirement reads like a satisfied one, so it fails), and the rule made
unconditional, which is the fail path of the gate-off half.

Measured: 30 charts, 210 assertions held (208 before), 0 failed, ~15s, no
cluster. README's stated figure was 206 and is corrected here.

Part of #136
This commit is contained in:
jklein committed 2026-10-15 17:46:45 +00:00
1 parent 530227405c
commit 1894e7d465
3 files changed
+409 -4

No files matched your search

+8 -4
View File
@@ -230,8 +230,11 @@ Measured locally: **30 charts, 408 scalars verified, 0 failed**, no cluster. The
| every **ingress** port is a `containerPort` of the selected pods | a NodePort number where a pod port belongs. The policy is evaluated at the pod's interface, *after* kube-proxy translated the port, so `30025` matches nothing while looking strict — and being the only rule for that port, it blocks every inbound connection instead | nobody in CI (see below), and in production as a service that went mute |
| no ingress rule allows every source by accident | `from: []` allows everything, identically to having no `from` at all, while reading in a diff and in `kubectl` output as a restriction — the one mistake in this class that makes a policy *weaker* while looking *stricter* | nobody |
| every port a **network** probe addresses is either unrestricted, or the chart offers `networkPolicy.probeSources` **and** says in `NOTES.txt` what a wrong CIDR costs | the kubelet probes from the IP of the worker node running the pod, which no chart can know. A restricted probe port without that address drops every probe: the pod never becomes `Ready`, the rollout times out, and the symptom reads as a broken application | the install-test, in production's CNI — **but not in CI**, see below |
| **cross-chart egress**: where a chart dials another chart *of this repository*, its egress rule must contain the **pod** ports behind that chart's Services (#136) | an egress rule naming the **Service** port. kube-proxy does the ClusterIP's destination NAT *before* the policy is evaluated at the pod's interface, so the policy sees `podIP:targetPort` and the rule matches nothing wherever `port` and `targetPort` differ | nobody: the symptom is a **timeout**, not `connection refused`, and no error mentions a NetworkPolicy — `paperless-ngx` 14.0.0 shipped it and cost an outage on 2026-10-15 |
Egress ports are **deliberately not** held against the pod's `containerPort`s: an egress port is a port on the *destination*, and that comparison would fail every chart that reaches a database.
Egress ports are otherwise **deliberately not** held against the pod's `containerPort`s: an egress port is a port on the *destination*, and that comparison would fail every chart that reaches a database.
**The one exception is the fifth row, and it is the only assertion here that needs two charts.** For a database, a cache or the web the destination is in no render this repository has — but it *is* in a render when one chart dials the Service of another, and `paperless-ngx` → `apache-tika`, `gotenberg` is the single place in the repository where that happens (checked for #136: the three other charts naming `port: 80` in a policy are aiming at the **internet**, where there is no DNAT and 80 is the real port). So the pairs live in an explicit table (`EGRESS_PEER_ROWS`) rather than being discovered — a chart cannot know which Service another chart dials, because the address is a value or a 1Password field — and a row naming a chart that no longer exists **fails** instead of being skipped. Each row is checked on **two** renders: with the feature gate **on** the containment must hold, and with it **off** the requirement must not apply *and* those ports must be gone, so a gated rule cannot quietly become one that permits what the release cannot use.
Two guards beyond the four, both earned rather than designed. **A chart that renders no policy at all fails** — that is how this issue decayed in the first place: `garage` (#81) and `keycloak` (#83) were both created *after* #5 was filed and both arrived without a policy, `keycloak` with its unauthenticated management surface on 9000. And **a `#` comment inside a rendered policy fails**, because a `#` comment is part of the *object*: it travels into the cluster, shows up in `helm get manifest` and `kubectl get -o yaml`, and lands in every release diff. Reasoning belongs in Go template comments, which render nothing. Both precedent charts failed that check when it was first run, which is what it is for.
@@ -242,7 +245,7 @@ Four things to know before touching it:
- **Subchart documents are skipped by the comment assertion**, for a structural reason and not a lenient one: a dependency is fetched from the Helm registry rather than from the copy in this repo, so the policy `paperless-ngx` and `stalwart` bundle is whatever `valkey` version is *published*, and nothing in the parent chart can change its rendered comments. The subchart's own PR is where it is caught, because this test runs over `valkey` too.
- **Anti-vacuum guard**, as in the tests above: `ci/networkpolicy/<chart>.yaml` is a render-only overlay applied **last**, whose job is to render the **source-restricted probe rule** — a branch no chart's own defaults ever reach, and the one containing the expensive mistake (a `namespaceSelector` and a `podSelector` written as two list entries instead of one would be an OR, opening the port to every pod in `kube-system`). A chart that *has* an overlay and still renders no restricted **probe** rule fails. That counter is specifically the probe rule and not "any restricted rule", because `keycloak` restricts two rules in its own defaults and would have satisfied a looser guard while the branch went unrendered.
Measured locally: **30 charts, 206 assertions held, 0 failed**, no cluster, a few seconds. All twelve fail paths were exercised rather than assumed — and one earned its keep immediately: the `probeSources` check was a substring test, so a chart whose key had been **deleted** while the long comment above it still discussed the value passed it. It now looks for the key.
Measured locally: **30 charts, 210 assertions held, 0 failed**, no cluster, ~15 seconds. All twelve fail paths were exercised rather than assumed — and one earned its keep immediately: the `probeSources` check was a substring test, so a chart whose key had been **deleted** while the long comment above it still discussed the value passed it. It now looks for the key. The cross-chart assertion added two of those 210, and its own six fail paths were executed the same way: a required port removed from the rule, a wrong number in its place, a peer chart renamed away, the policy's name no longer following the `<release>-<chart>-<kind>` convention, the peers rendering no Service at all (which would make the requirement empty, and is treated as a failure because an empty requirement reads exactly like a satisfied one), and the gated rule made unconditional — which is the failure path of the *gate-off* half, since with the gate off and the rule properly gated the assertion must stay silent.
**What this test does not and cannot cover, named so the gap is not mistaken for coverage:** that traffic still flows. No ingress controller runs in the install-test cluster, so the path from Traefik to a pod is proven nowhere in this pipeline — and, measured on the PR for #5, **k3s exempts the kubelet's probe traffic from policy**, so a blocked probe path does *not* turn the install-test red either (`cyberchef` became `Ready` in 69s with its probe port restricted to an address no probe can come from). What the install-test *does* prove is enforcement of everything the application itself needs: with `outline`'s DNS egress rule removed, its pod never became `Ready` and the job failed after 5m59s. So the net under this convention is real, but it is an **egress** net — which raises the stakes for assertion 2, since a wrong ingress port is caught mechanically here or not at all.
@@ -690,7 +693,7 @@ Every port of a workload falls into one of four, and this is the part to get rig
#### Values
`networkPolicy.enabled`, `.allowedNamespaces`, `.probeSources`, `.ingressController`, `.egress.extraPorts` — and a chart ships **only the ones that reach a rule**. A value that silently does nothing is worse than one that is absent, because it reads as a control: `samba` and `satisfactory` carry `enabled` alone (every inbound rule is open to any source), and `teamspeak-server` has `allowedNamespaces` but no `probeSources`.
`networkPolicy.enabled`, `.allowedNamespaces`, `.probeSources`, `.ingressController`, `.egress.extraPorts` — and a chart ships **only the ones that reach a rule**. A chart may add a **named** egress switch where a port depends on something the values cannot see: `paperless-ngx`' `.egress.mailImport` opens IMAPS `993`, off by default, because its mail accounts are rows in its own database and no value says whether a release fetches mail. The bar for one of those: it names what it opens, and it is **not** a second switch for a port some *other* value already decides — that is why the Tika/Gotenberg rule stayed gated on `paperless.files.tikaGotenberg.enabled` and got no policy switch of its own. Two values that can drift apart re-arm the outage the rule was fixed for. A value that silently does nothing is worse than one that is absent, because it reads as a control: `samba` and `satisfactory` carry `enabled` alone (every inbound rule is open to any source), and `teamspeak-server` has `allowedNamespaces` but no `probeSources`.
**No chart ships `allowedNamespaces` pre-filled, ever.** A chart that named `mc-router`, or `paperless-jk`, or `stalwart` in its defaults would be baking in an assumption about *this* cluster's namespace layout — which is exactly the kind of claim a chart cannot keep true. So the default is the empty list and the chart is **restrictive**; filling it is the **deployment adaptation**, made where the namespace layout is actually visible. That is why a sweep like this produces a list of release-side one-liners: those are the expected procedure, not a defect list, and the breaking-change major on each chart is what announces them.
@@ -717,12 +720,13 @@ Every port of a workload falls into one of four, and this is the part to get rig
#### Mechanics
- **Pod ports, never NodePorts.** A `NetworkPolicy` is evaluated at the pod's network interface, *after* kube-proxy has translated a NodePort to the `targetPort`. `30025` where the pod listens on `25` matches nothing: accepted, strict-looking, and blocking every inbound connection instead. The render test asserts this mechanically against the selected pod spec's `containerPort`s.
- **And pod ports, never Service ports — in `egress` too (#136).** The same destination NAT runs on the way *out*: kube-proxy rewrites a ClusterIP's address and port **before** the egress policy is evaluated, so the policy sees `podIP:targetPort` and never `clusterIP:port`. An egress rule therefore names the **pod** port of its destination, and a Service port only ever matches by coincidence — wherever `port` and `targetPort` are the same number, which is the case for `5432`, `6379`, `993` and `443` and is why this mistake stayed invisible until a chart dialled a Service that forwards `80` to `9998`. It was argued the other way in `paperless-ngx` (*"an egress rule names the port on the destination as the client dials it, and the client dials the Service"*), shipped, and cost an outage. **The tell is the symptom:** a dropped packet runs into a **timeout**, a missing listener answers `connection refused` — so a timeout where the service demonstrably exists is a policy question, and no Kubernetes error will say so. For a destination *outside* the cluster there is no NAT in the path and the port dialled is the port to name; the deciding question is the DNAT, not the direction.
- **Protocol as well as number.** `teamspeak-server`'s voice port is UDP and `satisfactory`'s game port is both — a TCP-only rule on either is accepted, looks complete, and drops the traffic that actually arrives.
- **`kubernetes.io/metadata.name`** is set by Kubernetes itself on every namespace (≥ 1.21), so namespace selectors need no labelling work on namespaces the chart does not own — and get no typo protection either.
- **Never `from: []`.** It allows everything, identically to having no key, while reading as a restriction. Write no `from` where open is meant and say so in a template comment.
- **Reasoning goes in Go template comments, not `#` comments.** A `#` comment is part of the rendered object: it travels into the cluster, appears in `helm get manifest` and `kubectl get -o yaml`, and lands in every release diff. The render test fails on a rendered `#`. One second-order trap, found while writing the reference implementation: prose inside a template comment must not spell out the comment's own closing delimiter, or the parser ends the comment early and the rest is emitted as manifest content — the render then fails with `did not find expected node content`, pointing at the template and not at the sentence.
- **A conditional `containerPort` needs a conditional rule.** `minecraft-server`'s RCON port is rendered only when `server.rcon.enabled` is true; a rule without the same gate names a port the pod does not declare, and the render test rejects it.
- **A policy applies to traffic that arrived through a Service, and that is why the `ingressController` rule works at all.** A `NetworkPolicy` is evaluated at the pod's network interface and a Service is only destination NAT, so going through a ClusterIP does not exempt anything. What the Service *does* change is the **source address**: pod → ClusterIP → pod **preserves** it, which is what lets a `podSelector` match the Traefik pod or a `namespaceSelector` match a consumer's namespace. Traffic from outside through a `NodePort` with the default `externalTrafficPolicy: Cluster` is **masqueraded to a node**, which is why those rules cannot be restricted by source at all and are left open instead (`stalwart`'s template carries that second half; the first half is the load-bearing one).
- **A policy applies to traffic that arrived through a Service, and that is why the `ingressController` rule works at all.** A `NetworkPolicy` is evaluated at the pod's network interface and a Service is only destination NAT, so going through a ClusterIP does not exempt anything. What the Service *does* change is the **source address**: pod → ClusterIP → pod **preserves** it, which is what lets a `podSelector` match the Traefik pod or a `namespaceSelector` match a consumer's namespace. **And what it also changes is the destination port, which is the half #136 was paid for:** "a Service is only destination NAT" is exactly why an *egress* rule has to name the pod port — the same sentence, finished in the other direction. It was written here before the wrong rule was written there; the information was present and the conclusion was missing, which is the sort of gap no check catches. Traffic from outside through a `NodePort` with the default `externalTrafficPolicy: Cluster` is **masqueraded to a node**, which is why those rules cannot be restricted by source at all and are left open instead (`stalwart`'s template carries that second half; the first half is the load-bearing one).
#### The one cluster setting this convention depends on and does not control