test(system): anchor index-addressed negative asserts to what they name - #3608

Actionable comments posted: 1

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@packages/system/ouroboros/tests/cozystack_overrides_test.yaml`:
- Around line 37-50: Add an assertion before the existing containers[0].args
check to verify that containers[0].name is proxy. Keep the current proxy
argument assertion unchanged and ensure the container identity is validated
first.

Overview

Reviewed statically against the actual chart templates and values.yaml defaults for all seven converted assertions, plus the helm-unittest contains/notContains validator source to confirm the any: true semantics the PR relies on.

Verdict: LGTM with non-blocking notes.

The core claim of this PR checks out: several of the "converted" assertions were genuinely dead before this change, not just theoretically weak.

• packages/system/monitoring/templates/grafana/grafana.yaml always renders GF_AUTH_GENERIC_OAUTH_CLIENT_SECRET and GF_PATHS_CUSTOM_INI with a valueFrom/value field beside name. The pre-PR bare-map notContains (no any: true) does an exact reflect.DeepEqual per the validator source, so it could never match those entries and was permanently green regardless of OIDC mode. The any: true addition (a genuine subset match, confirmed against contains_validator.go) is a real fix.
• The --pinned-tenants=, --target-host=, and --shard-cpu-limit= prefix-style notContains/removed checks were indeed dead: Helm renders each flag as one joined string (--flag=value), so a bare-prefix element can never equal a rendered element. The equal-on-full-arg-list replacements in flux-shard-operator/tests/workload_test.yaml and ouroboros/tests/cozystack_overrides_test.yaml were checked element-by-element against the rendered defaults (templates/workload.yaml + package values.yaml, proxy-deployment.yaml + package values.yaml including the cozy-ingress-nginx namespace override) and match exactly, in order.
• The four equal: containers[0].name anchors (proxy/kilo/grafana/controller) were checked against their respective single-container templates — all correct, and a reasonable defensive measure against a future sidecar shifting the index.
• The Cilium lengthEqual + selector + any: true conversion was checked against templates/networkpolicy.yaml: port 7473 is indeed absent today, and the neighbouring 7946 entry does carry protocol: ANY beside port, which is exactly the shape the PR says would slip past a plain notContains — confirming the stated regression scenario is realistic, not hypothetical.

Non-blocking notes

1. packages/system/ouroboros/tests/cozystack_overrides_test.yaml:37-40 — the "proxy dials the cozy-ingress-nginx Service..." test converts its dead notContains to a full-list equal on spec.template.spec.containers[0].args, but does not add a containers[0].name: proxy anchor first. Every other converted assertion in this PR gets that anchor — including the second test in this same file — and CodeRabbit flagged this exact gap with a concrete suggested diff that is still unaddressed at head. It isn't a silent-pass risk today (proxy-deployment.yaml renders exactly one container, and an equal on the full list fails loudly rather than silently if that ever changes), but it's inconsistent with the PR's own stated principle ("every index on a path gets anchored") and worth a one-line fix before merge for consistency and clearer failure messages down the line.
2. Nice self-imposed scope discipline: explicitly not sweeping the whole tree, and explicitly not switching to name-based JSONPath filters (which would trade "wrong element" for "no element" — the same failure family). Both calls are correct and the reasoning is sound.

Closing

Solid, well-evidenced test-hygiene fix; the "dead assertion" diagnoses all held up against the actual templates. Addressing note #1 (or explicitly deferring it with a reason) would make the PR fully consistent with its own methodology, but it's not a blocker.

Reproduced both dead asserts. Pre-PR forms in networkpolicy_test.yaml and oidc_test.yaml stay green while the template regresses, the any: true forms go red. Nine converted guards mutation tested, all nine catch it. Anchors are not vacuous either, dropping a container name: makes equal error on unknown path instead of passing.

All six packages have a Makefile test: target so the runner picks them all up, none skipped. Local helm-unittest is v1.0.3 and CI installs unpinned which is v1.1.2 now, contains_validator.go is identical between them so results transfer.

Every assertion this rewrites was checked in both directions, and in 11 of 13 violation scenarios the old form was green on the exact violation its own name forbids.

Two are worth naming because they are the sharpest. A test called no --compatibility arg in the default render passed at base while --compatibility=cilium, the literal string it forbade, was rendered on the real kilo container, because a sidecar occupied containers[0]. And drops the stale frr-mode metrics port passed with port 7473 back in the deny rule, because an unrelated deny inserted ahead shifted the real rule to index 1. Both go red on this PR.

The two exceptions are cases where the old form happened to catch the one literal spelling it named, and the new form catches those too, so nothing regressed.

I also checked the design rule rather than trusting the commit message. notMatchRegex over containers[0].args[*] where containers[0] is a sidecar with no args passes silently, so the name anchors are load-bearing. And the converse: the four notContains left unanchored in the same keycloak file and the two unanchored notMatchRegex in ouroboros are each held by a positive contains on the same path, and every one of them reddens under the corresponding sidecar-ahead mutation in both forms. The selectivity is principled.

All six touched packages define a test: target and pass the make -n test probe, so none is in the silently-skipped class, and all six are green before and after, so no new assertion is vacuous on the unmutated chart.

Three things worth a line each.

The cilium length anchor at networkpolicy_test.yaml:98 couples a port-named test to the rule count. Appending an unrelated cluster-scoped deny to templates/networkpolicy.yaml, with 7473 nowhere in sight, reddens exactly drops the stale frr-mode metrics port and nothing else. That is defensible on your own criterion, since the template is first-party and adding a deny rule there is a deliberate change asking to be read. What is missing is a sentence saying so, so the next person to add a deny rule reads why a test about 7473 turned red instead of concluding the anchor is wrong.

Six assertions now rest on notMatchRegex plus wildcard-path semantics that CI does not pin. The plugin is installed unversioned at .github/workflows/pull-requests.yaml:209-210. On 1.0.3 a wildcard path makes notMatchRegex mean no element may match, which is what all six depend on. If a release changed that to some element does not match, all six become vacuous in precisely the way this PR exists to prevent and nothing in the repo would notice. Not yours, but this PR increases the blast radius of an unpinned tool and pinning it is cheap.

The flux-shard whole-list equal correctly catches --shard-cpu-limit=500m, which the deleted notContains never could, but the failure now surfaces in a test called renders the default Deployment shape, whose name carries no signal that the absence of a cpu-limit flag is deliberate. The comment at :20-27 explains the instrument and not that particular absence.

The red E2E is the node-join flake, 43 of 45 suites passed and the diff is helm-unittest YAML that E2E never loads.