Skip to content

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

Open
AshrafAhmed9 wants to merge 1 commit into
documentdb:mainfrom
AshrafAhmed9:developer/e2e-otel-psa-assertion
Open

fix(e2e): stop the PSA sidecar guard passing when otel-collector is absent#452
AshrafAhmed9 wants to merge 1 commit into
documentdb:mainfrom
AshrafAhmed9:developer/e2e-otel-psa-assertion

Conversation

@AshrafAhmed9

@AshrafAhmed9 AshrafAhmed9 commented Aug 30, 2026

Copy link
Copy Markdown

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

  • requireSidecars ...string names sidecars that must be present on every instance pod. Variadic, so the existing monitoring-off callers are untouched.
  • Presence only counts pods labelled cnpg.io/podRole=instance. CNPG puts 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.
  • Asking for a sidecar when no instance pod matches is an error. Otherwise an upstream label change could quietly bring the vacuous pass back.
  • The monitoring-on spec in tests/resources now passes otel-collector. It already deploys the suite's only monitoring-on cluster, so CI doesn't gain one.
  • Unit tests for the helper. 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.

Dropping the required-sidecar check reproduces the vacuous pass. Dropping the instance-pod scope reproduces the Job-pod false positive:

--- FAIL: ...RequiredSidecars
    want missing-otel error, got <nil>

--- FAIL: ...IgnoresJobPods
    instance pod jobs-1-initdb is missing required injected sidecar "otel-collector"

Both pass with the change.

I haven't run the e2e suite itself, since I don't have a kind cluster with the operator on it. The spec change is one Eventually against an existing fixture and the helper is unit-tested, but the end-to-end path wants a CI run.

Out of scope

…bsent

documentdb-gateway is injected unconditionally, so the matched == 0 guard
in AssertInjectedSidecarsPSARestricted is always satisfied and a
monitoring-on cluster whose otel-collector failed to inject still reports
its pods as hardened.

Add an optional requireSidecars parameter naming sidecars that must be
present on every instance pod. Presence is scoped by cnpg.io/podRole=instance
so CNPG's bootstrap and join Job pods, which also carry cnpg.io/cluster, are
not mistaken for instances that lost a sidecar. Requiring a sidecar when no
instance pod matches is an error, so a change to CNPG's labels cannot
silently reinstate the vacuous pass.

Call it with otel-collector from the monitoring-on spec in tests/resources,
which already deploys the suite's only monitoring-on cluster in a
PSA-restricted namespace, so this adds no cluster deploy to CI.

Adds unit tests for the helper; it was the only assertion in the package
without any.

Refs documentdb#412, documentdb#387

Signed-off-by: Ashrafahmed9 <ashrafahmed1232@gmail.com>
@AshrafAhmed9
AshrafAhmed9 force-pushed the developer/e2e-otel-psa-assertion branch from ba19916 to 0e7e2c6 Compare August 30, 2026 17:53
@documentdb-triage-tool documentdb-triage-tool Bot added bug Something isn't working test labels Aug 30, 2026
@documentdb-triage-tool

Copy link
Copy Markdown

🤖 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working test

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant