fix(e2e): stop the PSA sidecar guard passing when otel-collector is absent

[AshrafAhmed9]

Summary

AssertInjectedSidecarsPSARestricted can't fail when the otel-collector is missing. documentdb-gateway is injected unconditionally, so matched == 0 never trips, and a monitoring-on cluster whose collector never injected still comes back hardened.

That's the gap behind #412. While chasing it I think most of the monitoring-on coverage the issue asks for already landed in #409: tests/resources/sidecar_resources_test.go deploys a cluster with monitoring.enabled: true and waits for healthy, and fixtures.CreateLabeledNamespace applies the restricted PSA labels. The assertion is the part that's missing. All three callers of the helper deploy monitoring-off, so nothing ever checks the collector's securityContext.

Changes

Presence and hardening are separate checks, because they fail for different reasons: a missing collector is a plumbing problem, a bad securityContext is the #387 problem, and one error shouldn't have to be read to work out which happened.

• AssertInjectedSidecarsPSARestricted is unchanged from main, byte for byte.
• AssertSidecarsInjected(ctx, c, ns, cluster, names...) requires the named sidecars on every instance pod. Naming none is an error rather than a pass, so it can't be called into a no-op.
• Presence only counts pods labelled cnpg.io/podRole=instance. CNPG stamps cnpg.io/cluster on bootstrap and join Job pods too and the injector skips those, so without the scope a leftover initdb pod looks like an instance that lost its collector.
• The monitoring-on spec in tests/resources runs both. It already deploys the suite's only monitoring-on cluster, so CI doesn't gain one.
• Unit tests for both helpers. The PSA one had none, and it was the only assertion in the package without any.

On the observability area

#412 suggests a new area for this. I used the existing monitoring-on cluster instead, since a second area buys another cluster deploy for the same signal. The scrape check listed as a follow-on there would justify its own area, but that needs a real exporter round-trip and reads better as a separate PR. Happy to move this if you'd rather areas stay single-purpose.

Testing

go vet ./... and gofmt clean, go test ./pkg/... green, tests/{resources,lifecycle,backup} all compile. Full CI is green, including the E2E matrix on amd64 and arm64.

Dropping the instance-pod scope reproduces the Job-pod false positive:

--- FAIL: TestAssertSidecarsInjected
    otel present: instance pod full-1-initdb is missing injected sidecar "otel-collector"

The tests also pin the case that motivated the split: on a gateway-only cluster the PSA checker passes, as it should, while AssertSidecarsInjected with otel-collector fails.

Out of scope

• The scrape/export check, listed as an optional follow-on in test(e2e): add monitoring-on e2e coverage for the otel-collector sidecar #412.
• Enforcing presence structurally rather than by convention. A new spec can still forget to call AssertSidecarsInjected; the fix for that belongs in the shared cluster fixture, not in either assertion.
• Pre-existing gofmt drift under Go 1.26 in ~39 files in test/e2e that I didn't touch.

[documentdb-triage-tool]

🤖 Auto-triaged by documentdb-triage-tool.

Applied: test, bug
Project fields suggested: Component test · Priority P2 · Effort M · Status Needs Review
Confidence: 0.85 (mixed)

Reasoning

component from path globs (test); effort from diff stats (262+7 LOC, 4 files); LLM: Fixes a false-passing PSA sidecar assertion in e2e tests that masked missing otel-collector injection, with unit tests and scoped instance-pod filtering added.

If a label is wrong, remove it manually and ping @patty-chow so the rules can be tuned. The bot will not re-label items that already have component labels.

[WentingWu666666]

@AshrafAhmed9 Nice catch on the vacuous pass!

One idea: I think the presence check should be its own helper, not a variadic on AssertInjectedSidecarsPSARestricted . Right now the function checks two different things — "the right sidecars are there" and "the sidecars are PSA-hardened." These fail for different reasons: a missing collector is a plumbing problem, a bad securityContext is the #387 problem. If they are separate helpers, a failed test tells you right away which one broke.

Also, the variadic is optional. Two of the three callers pass nothing, so they skip the presence check without any warning. It is easy to forget on the next caller too.

So maybe: keep this function as it was on main (PSA only), add a new AssertSidecarsInjected(...)  for presence, and let the monitoring-on test check both.

[AshrafAhmed9]

You're right that the optional variadic was the weak part, and two callers were already demonstrating it. A check that silently does nothing when you forget an argument isn't much of a check. Done in 835a9f8.

AssertInjectedSidecarsPSARestricted is back to exactly what's on main, byte for byte. The new AssertSidecarsInjected(ctx, c, ns, cluster, names...) takes the sidecars a caller expects and requires them on every instance pod. Naming none of them is an error rather than a pass, so you can't call it into a no-op the way you could with the variadic.

The monitoring-on spec runs both, and the failure messages now say which thing broke:

instance pod carveout-mon-1 is missing injected sidecar "otel-collector"
pod carveout-mon-1 container "otel-collector": seccompProfile.type must be RuntimeDefault

The new helper only counts pods labelled cnpg.io/podRole=instance. CNPG stamps cnpg.io/cluster on its bootstrap and join Job pods as well, and the injector never touches those, so without that scope a leftover initdb pod reads as an instance that lost its collector. Dropping the scope turns the test red:

--- FAIL: TestAssertSidecarsInjected
    otel present: instance pod full-1-initdb is missing injected sidecar "otel-collector"

Both helpers now have unit tests; the PSA one had none before.

On it being easy to forget on the next caller: splitting doesn't fully solve that, since a new spec can just as easily not call AssertSidecarsInjected at all. What it does fix is the API no longer implying it checked something it didn't. If you want presence enforced structurally rather than by convention, that belongs in the shared cluster fixture rather than in either assertion, and I'd rather do it separately than grow this PR.