From 0e7e2c6f98e1604344b6d880c871a75c08364ac3 Mon Sep 17 00:00:00 2001 From: Ashrafahmed9 Date: Sun, 30 Aug 2026 23:22:17 +0530 Subject: [PATCH 1/2] fix(e2e): stop the PSA sidecar guard passing when otel-collector is absent 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 #412, #387 Signed-off-by: Ashrafahmed9 --- .../e2e/pkg/e2eutils/assertions/assertions.go | 61 +++++- .../e2eutils/assertions/assertions_test.go | 189 ++++++++++++++++++ test/e2e/tests/lifecycle/deploy_test.go | 3 +- .../tests/resources/sidecar_resources_test.go | 16 ++ 4 files changed, 262 insertions(+), 7 deletions(-) diff --git a/test/e2e/pkg/e2eutils/assertions/assertions.go b/test/e2e/pkg/e2eutils/assertions/assertions.go index e56d39e2c..de2d6a2a7 100644 --- a/test/e2e/pkg/e2eutils/assertions/assertions.go +++ b/test/e2e/pkg/e2eutils/assertions/assertions.go @@ -204,6 +204,15 @@ var injectedSidecarNames = map[string]bool{ "otel-collector": true, } +// CNPG stamps cnpg.io/cluster on its bootstrap/join Job pods as well as on +// instance pods, but only instance pods get the injected sidecars. Presence +// checks are therefore scoped by cnpg.io/podRole=instance so a lingering +// bootstrap Job pod is not mistaken for an instance missing its sidecar. +const ( + cnpgPodRoleLabel = "cnpg.io/podRole" + cnpgPodRoleInstance = "instance" +) + // checkPSARestricted returns an error if ctr lacks any SecurityContext field // required by the Kubernetes Pod Security Admission "restricted" profile. func checkPSARestricted(podName string, ctr corev1.Container) error { @@ -242,10 +251,32 @@ func containsCapability(caps []corev1.Capability, want corev1.Capability) bool { // restricted-labeled namespace already implies the pods passed admission; the // explicit field checks turn an otherwise opaque CNPG pod-creation failure into // a precise message. Regression guard for #387 — works for both freshly -// deployed and restored (recovery) clusters. It errors if no injected sidecar -// is found so it cannot pass vacuously. -func AssertInjectedSidecarsPSARestricted(ctx context.Context, c client.Client, ns, clusterName string) func() error { +// deployed and restored (recovery) clusters. +// +// requireSidecars names injected sidecars that must be present on every +// instance pod. documentdb-gateway is injected unconditionally, so without +// this a monitoring-on cluster whose otel-collector failed to inject still +// passes: the gateway alone satisfies the "found at least one sidecar" guard. +// Callers that enable monitoring should pass "otel-collector" so a silently +// missing collector fails instead of being reported as hardened. Passing a +// name that is not an injected sidecar is a spec bug and fails immediately. +func AssertInjectedSidecarsPSARestricted( + ctx context.Context, + c client.Client, + ns, clusterName string, + requireSidecars ...string, +) func() error { + var requireErr error + for _, name := range requireSidecars { + if !injectedSidecarNames[name] { + requireErr = fmt.Errorf("required sidecar %q is not a CNPG-I-injected sidecar", name) + break + } + } return func() error { + if requireErr != nil { + return requireErr + } var pods corev1.PodList if err := c.List(ctx, &pods, client.InNamespace(ns), @@ -256,21 +287,39 @@ func AssertInjectedSidecarsPSARestricted(ctx context.Context, c client.Client, n return fmt.Errorf("no pods found for cluster %s/%s", ns, clusterName) } matched := 0 + instancePods := 0 for i := range pods.Items { - for j := range pods.Items[i].Spec.Containers { - ctr := pods.Items[i].Spec.Containers[j] + pod := &pods.Items[i] + present := make(map[string]bool, len(pod.Spec.Containers)) + for j := range pod.Spec.Containers { + ctr := pod.Spec.Containers[j] if !injectedSidecarNames[ctr.Name] { continue } + present[ctr.Name] = true matched++ - if err := checkPSARestricted(pods.Items[i].Name, ctr); err != nil { + if err := checkPSARestricted(pod.Name, ctr); err != nil { return err } } + if pod.Labels[cnpgPodRoleLabel] != cnpgPodRoleInstance { + continue + } + instancePods++ + for _, name := range requireSidecars { + if !present[name] { + return fmt.Errorf("instance pod %s is missing required injected sidecar %q", + pod.Name, name) + } + } } if matched == 0 { return fmt.Errorf("no injected sidecar containers found on pods for cluster %s/%s", ns, clusterName) } + if len(requireSidecars) > 0 && instancePods == 0 { + return fmt.Errorf("no instance pods (%s=%s) found for cluster %s/%s to check required sidecars %v", + cnpgPodRoleLabel, cnpgPodRoleInstance, ns, clusterName, requireSidecars) + } return nil } } diff --git a/test/e2e/pkg/e2eutils/assertions/assertions_test.go b/test/e2e/pkg/e2eutils/assertions/assertions_test.go index 19fd2a75a..fb4ff1bc8 100644 --- a/test/e2e/pkg/e2eutils/assertions/assertions_test.go +++ b/test/e2e/pkg/e2eutils/assertions/assertions_test.go @@ -159,3 +159,192 @@ func TestAssertConnectionStringMatches(t *testing.T) { t.Fatalf("want regex compile error") } } + +// psaRestrictedSC returns a SecurityContext satisfying every field +// checkPSARestricted requires. +func psaRestrictedSC() *corev1.SecurityContext { + yes, no := true, false + return &corev1.SecurityContext{ + RunAsNonRoot: &yes, + AllowPrivilegeEscalation: &no, + Capabilities: &corev1.Capabilities{Drop: []corev1.Capability{"ALL"}}, + SeccompProfile: &corev1.SeccompProfile{Type: corev1.SeccompProfileTypeRuntimeDefault}, + } +} + +// clusterPod builds a CNPG instance pod: the cluster label the assertion +// selects on, plus the podRole marking it an instance rather than a Job pod. +func clusterPod(name, cluster string, ctrs ...corev1.Container) *corev1.Pod { + p := jobPod(name, cluster, ctrs...) + p.Labels[cnpgPodRoleLabel] = cnpgPodRoleInstance + return p +} + +// jobPod builds a CNPG bootstrap/join Job pod. CNPG stamps cnpg.io/cluster on +// these too, but the sidecar injector does not touch them. +func jobPod(name, cluster string, ctrs ...corev1.Container) *corev1.Pod { + return &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: "ns", + Labels: map[string]string{"cnpg.io/cluster": cluster}, + }, + Spec: corev1.PodSpec{Containers: ctrs}, + } +} + +func TestAssertInjectedSidecarsPSARestricted(t *testing.T) { + t.Parallel() + s := newScheme(t) + + compliant := clusterPod("ok-0", "ok", + corev1.Container{Name: "postgres"}, + corev1.Container{Name: "documentdb-gateway", SecurityContext: psaRestrictedSC()}, + ) + // Gateway without a securityContext at all — the #387 regression. + bare := clusterPod("bare-0", "bare", + corev1.Container{Name: "documentdb-gateway"}, + ) + // Gateway that runs as root. + rootSC := psaRestrictedSC() + rootSC.RunAsNonRoot = nil + asRoot := clusterPod("root-0", "root", + corev1.Container{Name: "documentdb-gateway", SecurityContext: rootSC}, + ) + // No injected sidecar on the pod at all. + noSidecar := clusterPod("none-0", "none", + corev1.Container{Name: "postgres"}, + ) + + c := fake.NewClientBuilder().WithScheme(s). + WithObjects(compliant, bare, asRoot, noSidecar).Build() + ctx := context.Background() + + if err := AssertInjectedSidecarsPSARestricted(ctx, c, "ns", "ok")(); err != nil { + t.Fatalf("compliant: %v", err) + } + if err := AssertInjectedSidecarsPSARestricted(ctx, c, "ns", "bare")(); err == nil || + !strings.Contains(err.Error(), "no securityContext") { + t.Fatalf("want missing-securityContext error, got %v", err) + } + if err := AssertInjectedSidecarsPSARestricted(ctx, c, "ns", "root")(); err == nil || + !strings.Contains(err.Error(), "runAsNonRoot") { + t.Fatalf("want runAsNonRoot error, got %v", err) + } + if err := AssertInjectedSidecarsPSARestricted(ctx, c, "ns", "none")(); err == nil || + !strings.Contains(err.Error(), "no injected sidecar") { + t.Fatalf("want no-injected-sidecar error, got %v", err) + } + if err := AssertInjectedSidecarsPSARestricted(ctx, c, "ns", "absent")(); err == nil || + !strings.Contains(err.Error(), "no pods found") { + t.Fatalf("want no-pods error, got %v", err) + } +} + +func TestAssertInjectedSidecarsPSARestrictedRequiredSidecars(t *testing.T) { + t.Parallel() + s := newScheme(t) + + // A monitoring-on cluster whose otel-collector never got injected. The + // always-present gateway is compliant, so the pod looks healthy. + noOtel := clusterPod("mon-0", "mon", + corev1.Container{Name: "postgres"}, + corev1.Container{Name: "documentdb-gateway", SecurityContext: psaRestrictedSC()}, + ) + withOtel := clusterPod("full-0", "full", + corev1.Container{Name: "postgres"}, + corev1.Container{Name: "documentdb-gateway", SecurityContext: psaRestrictedSC()}, + corev1.Container{Name: "otel-collector", SecurityContext: psaRestrictedSC()}, + ) + otelRoot := psaRestrictedSC() + otelRoot.SeccompProfile = nil + badOtel := clusterPod("badotel-0", "badotel", + corev1.Container{Name: "documentdb-gateway", SecurityContext: psaRestrictedSC()}, + corev1.Container{Name: "otel-collector", SecurityContext: otelRoot}, + ) + + c := fake.NewClientBuilder().WithScheme(s). + WithObjects(noOtel, withOtel, badOtel).Build() + ctx := context.Background() + + // Without an explicit requirement the checker cannot tell "monitoring is + // off" from "monitoring is on but otel never got injected": the gateway + // satisfies it either way. That is correct for monitoring-off callers. + if err := AssertInjectedSidecarsPSARestricted(ctx, c, "ns", "mon")(); err != nil { + t.Fatalf("gateway-only cluster: %v", err) + } + // Naming otel-collector as required turns the missing sidecar into a failure. + if err := AssertInjectedSidecarsPSARestricted(ctx, c, "ns", "mon", "otel-collector")(); err == nil || + !strings.Contains(err.Error(), "otel-collector") { + t.Fatalf("want missing-otel error, got %v", err) + } + if err := AssertInjectedSidecarsPSARestricted(ctx, c, "ns", "full", "otel-collector")(); err != nil { + t.Fatalf("otel present and compliant: %v", err) + } + if err := AssertInjectedSidecarsPSARestricted(ctx, c, "ns", "badotel", "otel-collector")(); err == nil || + !strings.Contains(err.Error(), "seccompProfile") { + t.Fatalf("want otel seccomp error, got %v", err) + } + // A name that is not an injected sidecar is a spec bug; fail fast rather + // than spin in Eventually until the timeout. + if err := AssertInjectedSidecarsPSARestricted(ctx, c, "ns", "full", "postgres")(); err == nil || + !strings.Contains(err.Error(), "not a CNPG-I-injected sidecar") { + t.Fatalf("want unknown-sidecar error, got %v", err) + } +} + +func TestAssertInjectedSidecarsPSARestrictedIgnoresJobPods(t *testing.T) { + t.Parallel() + s := newScheme(t) + + // CNPG labels its bootstrap/join Job pods with cnpg.io/cluster, but the + // sidecar injector never adds containers to them. A lingering Job pod must + // not read as an instance that lost its collector. + instance := clusterPod("jobs-1", "jobs", + corev1.Container{Name: "documentdb-gateway", SecurityContext: psaRestrictedSC()}, + corev1.Container{Name: "otel-collector", SecurityContext: psaRestrictedSC()}, + ) + bootstrap := jobPod("jobs-1-initdb", "jobs", + corev1.Container{Name: "bootstrap-controller"}, + ) + // A cluster whose only pod is a bootstrap Job: nothing to check yet, and + // requiring a sidecar must not pass vacuously. + onlyJob := jobPod("early-1-initdb", "early", + corev1.Container{Name: "bootstrap-controller"}, + ) + + c := fake.NewClientBuilder().WithScheme(s). + WithObjects(instance, bootstrap, onlyJob).Build() + ctx := context.Background() + + if err := AssertInjectedSidecarsPSARestricted(ctx, c, "ns", "jobs", "otel-collector")(); err != nil { + t.Fatalf("job pod alongside a healthy instance: %v", err) + } + if err := AssertInjectedSidecarsPSARestricted(ctx, c, "ns", "early", "otel-collector")(); err == nil || + !strings.Contains(err.Error(), "no injected sidecar") { + t.Fatalf("want no-injected-sidecar error, got %v", err) + } +} + +func TestAssertInjectedSidecarsPSARestrictedNeedsAnInstancePod(t *testing.T) { + t.Parallel() + s := newScheme(t) + + // Sidecars present, but on a pod that is not labelled as an instance — + // the shape we would see if CNPG stopped stamping cnpg.io/podRole. The + // per-pod requirement would then match nothing, so without this guard + // requireSidecars would silently stop being enforced. + orphan := jobPod("orphan-0", "orphan", + corev1.Container{Name: "documentdb-gateway", SecurityContext: psaRestrictedSC()}, + ) + c := fake.NewClientBuilder().WithScheme(s).WithObjects(orphan).Build() + + err := AssertInjectedSidecarsPSARestricted(context.Background(), c, "ns", "orphan", "otel-collector")() + if err == nil || !strings.Contains(err.Error(), "no instance pods") { + t.Fatalf("want no-instance-pods error, got %v", err) + } + // With nothing required, the same cluster is still a normal pass. + if err := AssertInjectedSidecarsPSARestricted(context.Background(), c, "ns", "orphan")(); err != nil { + t.Fatalf("no requirement: %v", err) + } +} diff --git a/test/e2e/tests/lifecycle/deploy_test.go b/test/e2e/tests/lifecycle/deploy_test.go index 974b8653e..8538bc66f 100644 --- a/test/e2e/tests/lifecycle/deploy_test.go +++ b/test/e2e/tests/lifecycle/deploy_test.go @@ -105,7 +105,8 @@ var _ = Describe("DocumentDB lifecycle — deploy", // This spec deploys with monitoring off, so only the // always-injected documentdb-gateway sidecar is present; // the otel-collector sidecar (injected only when monitoring - // is enabled) is covered by the sidecar-injector unit test. + // is enabled) is covered end-to-end by the monitoring-on spec + // in tests/resources and by the sidecar-injector unit test. // The shared helper errors if no injected sidecar is found, // so this cannot pass vacuously. Eventually(assertions.AssertInjectedSidecarsPSARestricted(ctx, c, ns, name), diff --git a/test/e2e/tests/resources/sidecar_resources_test.go b/test/e2e/tests/resources/sidecar_resources_test.go index ec65e612f..d80881e6a 100644 --- a/test/e2e/tests/resources/sidecar_resources_test.go +++ b/test/e2e/tests/resources/sidecar_resources_test.go @@ -10,6 +10,8 @@ import ( corev1 "k8s.io/api/core/v1" "github.com/documentdb/documentdb-operator/test/e2e" + "github.com/documentdb/documentdb-operator/test/e2e/pkg/e2eutils/assertions" + "github.com/documentdb/documentdb-operator/test/e2e/pkg/e2eutils/timeouts" ) // These specs validate the pod memory carve-out (sidecar resource isolation). @@ -103,6 +105,20 @@ var _ = Describe("Sidecar memory carve-out", pg := containerByName(pod, postgresContainerName) Expect(pg).ToNot(BeNil(), "postgres container present") assertGuaranteedMemory(pg, wantPostgresWithMon) + + // This is the suite's only monitoring-on cluster, so it is the + // only place the collector's PSA hardening (#387) can be checked + // end-to-end. The fixture labels the namespace restricted, so + // reaching healthy already proves the sidecar passed admission; + // the explicit field checks name the offending field instead of + // leaving an opaque pod-creation failure. otel-collector is + // named as required so a collector that silently fails to inject + // is a failure rather than a vacuous pass on the gateway alone. + Eventually(assertions.AssertInjectedSidecarsPSARestricted( + ctx, c, cr.Namespace, cr.Name, otelContainerName), + timeouts.For(timeouts.DocumentDBReady), + timeouts.PollInterval(timeouts.DocumentDBReady), + ).Should(Succeed(), "monitoring-on cluster pods must carry PSA-restricted securityContext") }) It("derives the envelope from per-container memory when the envelope is omitted", From 835a9f8354e942e1a5dcd8a0efba5946b15f4776 Mon Sep 17 00:00:00 2001 From: Ashrafahmed9 Date: Sat, 5 Sep 2026 17:37:03 +0530 Subject: [PATCH 2/2] fix(e2e): assert the otel-collector is injected, not just hardened AssertInjectedSidecarsPSARestricted cannot fail when the otel-collector is missing. documentdb-gateway is injected unconditionally, so the matched == 0 guard is always satisfied and a monitoring-on cluster whose collector never injected still reports its pods as hardened. Split the two questions rather than widening that checker. Presence and hardening fail for different reasons: a missing collector is a plumbing problem, a bad securityContext is the #387 problem, and one error should not have to be read to work out which happened. AssertInjectedSidecarsPSARestricted is left exactly as it was on main. AssertSidecarsInjected takes the sidecars a caller expects and requires them on every instance pod. Presence is scoped by cnpg.io/podRole=instance, since CNPG stamps cnpg.io/cluster on its bootstrap and join Job pods too and the injector never touches those. Naming no sidecars, or naming something that is not an injected sidecar, is a spec bug and fails immediately instead of spinning until the Eventually timeout. The monitoring-on spec in tests/resources now runs both. It already deploys the suite's only monitoring-on cluster, so this adds no cluster deploy to CI. Adds unit tests for both helpers; the PSA one had none, and it was the only assertion in the package without any. --- .../e2e/pkg/e2eutils/assertions/assertions.go | 111 +++++++++------ .../e2eutils/assertions/assertions_test.go | 131 +++++++----------- .../tests/resources/sidecar_resources_test.go | 21 ++- 3 files changed, 128 insertions(+), 135 deletions(-) diff --git a/test/e2e/pkg/e2eutils/assertions/assertions.go b/test/e2e/pkg/e2eutils/assertions/assertions.go index de2d6a2a7..7df7e05dd 100644 --- a/test/e2e/pkg/e2eutils/assertions/assertions.go +++ b/test/e2e/pkg/e2eutils/assertions/assertions.go @@ -204,10 +204,10 @@ var injectedSidecarNames = map[string]bool{ "otel-collector": true, } -// CNPG stamps cnpg.io/cluster on its bootstrap/join Job pods as well as on -// instance pods, but only instance pods get the injected sidecars. Presence -// checks are therefore scoped by cnpg.io/podRole=instance so a lingering -// bootstrap Job pod is not mistaken for an instance missing its sidecar. +// CNPG stamps cnpg.io/cluster on its bootstrap and join Job pods as well as on +// instance pods, but only instance pods carry injected sidecars. Presence is +// therefore scoped by cnpg.io/podRole=instance so a lingering bootstrap Job pod +// is not mistaken for an instance that lost a sidecar. const ( cnpgPodRoleLabel = "cnpg.io/podRole" cnpgPodRoleInstance = "instance" @@ -251,32 +251,10 @@ func containsCapability(caps []corev1.Capability, want corev1.Capability) bool { // restricted-labeled namespace already implies the pods passed admission; the // explicit field checks turn an otherwise opaque CNPG pod-creation failure into // a precise message. Regression guard for #387 — works for both freshly -// deployed and restored (recovery) clusters. -// -// requireSidecars names injected sidecars that must be present on every -// instance pod. documentdb-gateway is injected unconditionally, so without -// this a monitoring-on cluster whose otel-collector failed to inject still -// passes: the gateway alone satisfies the "found at least one sidecar" guard. -// Callers that enable monitoring should pass "otel-collector" so a silently -// missing collector fails instead of being reported as hardened. Passing a -// name that is not an injected sidecar is a spec bug and fails immediately. -func AssertInjectedSidecarsPSARestricted( - ctx context.Context, - c client.Client, - ns, clusterName string, - requireSidecars ...string, -) func() error { - var requireErr error - for _, name := range requireSidecars { - if !injectedSidecarNames[name] { - requireErr = fmt.Errorf("required sidecar %q is not a CNPG-I-injected sidecar", name) - break - } - } +// deployed and restored (recovery) clusters. It errors if no injected sidecar +// is found so it cannot pass vacuously. +func AssertInjectedSidecarsPSARestricted(ctx context.Context, c client.Client, ns, clusterName string) func() error { return func() error { - if requireErr != nil { - return requireErr - } var pods corev1.PodList if err := c.List(ctx, &pods, client.InNamespace(ns), @@ -287,38 +265,83 @@ func AssertInjectedSidecarsPSARestricted( return fmt.Errorf("no pods found for cluster %s/%s", ns, clusterName) } matched := 0 - instancePods := 0 for i := range pods.Items { - pod := &pods.Items[i] - present := make(map[string]bool, len(pod.Spec.Containers)) - for j := range pod.Spec.Containers { - ctr := pod.Spec.Containers[j] + for j := range pods.Items[i].Spec.Containers { + ctr := pods.Items[i].Spec.Containers[j] if !injectedSidecarNames[ctr.Name] { continue } - present[ctr.Name] = true matched++ - if err := checkPSARestricted(pod.Name, ctr); err != nil { + if err := checkPSARestricted(pods.Items[i].Name, ctr); err != nil { return err } } + } + if matched == 0 { + return fmt.Errorf("no injected sidecar containers found on pods for cluster %s/%s", ns, clusterName) + } + return nil + } +} + +// AssertSidecarsInjected returns a checker that succeeds when every sidecar in +// names is present on every instance pod backing clusterName in ns. +// +// This is deliberately separate from AssertInjectedSidecarsPSARestricted. The +// two fail for different reasons: a missing collector is a plumbing problem, +// a bad SecurityContext is the #387 problem, and a caller that sees one error +// should not have to work out which it got. It also gives the monitoring-on +// path a check that cannot pass vacuously: documentdb-gateway is injected +// unconditionally, so a PSA check alone still succeeds on a cluster whose +// otel-collector never got injected at all. +// +// names must not be empty, and every name must be a CNPG-I-injected sidecar; +// either is a bug in the calling spec and fails immediately rather than +// spinning until the Eventually timeout. +func AssertSidecarsInjected(ctx context.Context, c client.Client, ns, clusterName string, names ...string) func() error { + var argErr error + switch { + case len(names) == 0: + argErr = fmt.Errorf("AssertSidecarsInjected requires at least one sidecar name") + default: + for _, name := range names { + if !injectedSidecarNames[name] { + argErr = fmt.Errorf("sidecar %q is not a CNPG-I-injected sidecar", name) + break + } + } + } + return func() error { + if argErr != nil { + return argErr + } + var pods corev1.PodList + if err := c.List(ctx, &pods, + client.InNamespace(ns), + client.MatchingLabels{"cnpg.io/cluster": clusterName}); err != nil { + return fmt.Errorf("list pods for cluster %s/%s: %w", ns, clusterName, err) + } + instancePods := 0 + for i := range pods.Items { + pod := &pods.Items[i] if pod.Labels[cnpgPodRoleLabel] != cnpgPodRoleInstance { continue } instancePods++ - for _, name := range requireSidecars { + present := make(map[string]bool, len(pod.Spec.Containers)) + for j := range pod.Spec.Containers { + present[pod.Spec.Containers[j].Name] = true + } + for _, name := range names { if !present[name] { - return fmt.Errorf("instance pod %s is missing required injected sidecar %q", + return fmt.Errorf("instance pod %s is missing injected sidecar %q", pod.Name, name) } } } - if matched == 0 { - return fmt.Errorf("no injected sidecar containers found on pods for cluster %s/%s", ns, clusterName) - } - if len(requireSidecars) > 0 && instancePods == 0 { - return fmt.Errorf("no instance pods (%s=%s) found for cluster %s/%s to check required sidecars %v", - cnpgPodRoleLabel, cnpgPodRoleInstance, ns, clusterName, requireSidecars) + if instancePods == 0 { + return fmt.Errorf("no instance pods (%s=%s) found for cluster %s/%s", + cnpgPodRoleLabel, cnpgPodRoleInstance, ns, clusterName) } return nil } diff --git a/test/e2e/pkg/e2eutils/assertions/assertions_test.go b/test/e2e/pkg/e2eutils/assertions/assertions_test.go index fb4ff1bc8..bff40f2ef 100644 --- a/test/e2e/pkg/e2eutils/assertions/assertions_test.go +++ b/test/e2e/pkg/e2eutils/assertions/assertions_test.go @@ -172,16 +172,16 @@ func psaRestrictedSC() *corev1.SecurityContext { } } -// clusterPod builds a CNPG instance pod: the cluster label the assertion -// selects on, plus the podRole marking it an instance rather than a Job pod. +// clusterPod builds a CNPG instance pod: the cluster label the assertions +// select on, plus the podRole marking it an instance rather than a Job pod. func clusterPod(name, cluster string, ctrs ...corev1.Container) *corev1.Pod { p := jobPod(name, cluster, ctrs...) p.Labels[cnpgPodRoleLabel] = cnpgPodRoleInstance return p } -// jobPod builds a CNPG bootstrap/join Job pod. CNPG stamps cnpg.io/cluster on -// these too, but the sidecar injector does not touch them. +// jobPod builds a CNPG bootstrap or join Job pod. CNPG stamps cnpg.io/cluster +// on these too, but the sidecar injector does not touch them. func jobPod(name, cluster string, ctrs ...corev1.Container) *corev1.Pod { return &corev1.Pod{ ObjectMeta: metav1.ObjectMeta{ @@ -200,24 +200,29 @@ func TestAssertInjectedSidecarsPSARestricted(t *testing.T) { compliant := clusterPod("ok-0", "ok", corev1.Container{Name: "postgres"}, corev1.Container{Name: "documentdb-gateway", SecurityContext: psaRestrictedSC()}, + corev1.Container{Name: "otel-collector", SecurityContext: psaRestrictedSC()}, ) // Gateway without a securityContext at all — the #387 regression. bare := clusterPod("bare-0", "bare", corev1.Container{Name: "documentdb-gateway"}, ) - // Gateway that runs as root. rootSC := psaRestrictedSC() rootSC.RunAsNonRoot = nil asRoot := clusterPod("root-0", "root", corev1.Container{Name: "documentdb-gateway", SecurityContext: rootSC}, ) - // No injected sidecar on the pod at all. + noSeccomp := psaRestrictedSC() + noSeccomp.SeccompProfile = nil + badOtel := clusterPod("badotel-0", "badotel", + corev1.Container{Name: "documentdb-gateway", SecurityContext: psaRestrictedSC()}, + corev1.Container{Name: "otel-collector", SecurityContext: noSeccomp}, + ) noSidecar := clusterPod("none-0", "none", corev1.Container{Name: "postgres"}, ) c := fake.NewClientBuilder().WithScheme(s). - WithObjects(compliant, bare, asRoot, noSidecar).Build() + WithObjects(compliant, bare, asRoot, badOtel, noSidecar).Build() ctx := context.Background() if err := AssertInjectedSidecarsPSARestricted(ctx, c, "ns", "ok")(); err != nil { @@ -231,6 +236,10 @@ func TestAssertInjectedSidecarsPSARestricted(t *testing.T) { !strings.Contains(err.Error(), "runAsNonRoot") { t.Fatalf("want runAsNonRoot error, got %v", err) } + if err := AssertInjectedSidecarsPSARestricted(ctx, c, "ns", "badotel")(); err == nil || + !strings.Contains(err.Error(), "seccompProfile") { + t.Fatalf("want otel seccomp error, got %v", err) + } if err := AssertInjectedSidecarsPSARestricted(ctx, c, "ns", "none")(); err == nil || !strings.Contains(err.Error(), "no injected sidecar") { t.Fatalf("want no-injected-sidecar error, got %v", err) @@ -241,12 +250,12 @@ func TestAssertInjectedSidecarsPSARestricted(t *testing.T) { } } -func TestAssertInjectedSidecarsPSARestrictedRequiredSidecars(t *testing.T) { +func TestAssertSidecarsInjected(t *testing.T) { t.Parallel() s := newScheme(t) // A monitoring-on cluster whose otel-collector never got injected. The - // always-present gateway is compliant, so the pod looks healthy. + // always-present gateway is compliant, so a PSA check alone still passes. noOtel := clusterPod("mon-0", "mon", corev1.Container{Name: "postgres"}, corev1.Container{Name: "documentdb-gateway", SecurityContext: psaRestrictedSC()}, @@ -256,95 +265,49 @@ func TestAssertInjectedSidecarsPSARestrictedRequiredSidecars(t *testing.T) { corev1.Container{Name: "documentdb-gateway", SecurityContext: psaRestrictedSC()}, corev1.Container{Name: "otel-collector", SecurityContext: psaRestrictedSC()}, ) - otelRoot := psaRestrictedSC() - otelRoot.SeccompProfile = nil - badOtel := clusterPod("badotel-0", "badotel", - corev1.Container{Name: "documentdb-gateway", SecurityContext: psaRestrictedSC()}, - corev1.Container{Name: "otel-collector", SecurityContext: otelRoot}, + // A bootstrap Job pod carries cnpg.io/cluster but no injected sidecars. + bootstrap := jobPod("full-1-initdb", "full", + corev1.Container{Name: "bootstrap-controller"}, + ) + // A cluster whose only pod is a bootstrap Job: nothing to check yet. + onlyJob := jobPod("early-1-initdb", "early", + corev1.Container{Name: "bootstrap-controller"}, ) c := fake.NewClientBuilder().WithScheme(s). - WithObjects(noOtel, withOtel, badOtel).Build() + WithObjects(noOtel, withOtel, bootstrap, onlyJob).Build() ctx := context.Background() - // Without an explicit requirement the checker cannot tell "monitoring is - // off" from "monitoring is on but otel never got injected": the gateway - // satisfies it either way. That is correct for monitoring-off callers. + // The case the PSA checker cannot catch on its own. if err := AssertInjectedSidecarsPSARestricted(ctx, c, "ns", "mon")(); err != nil { - t.Fatalf("gateway-only cluster: %v", err) + t.Fatalf("PSA check passes on a gateway-only cluster, as designed: %v", err) } - // Naming otel-collector as required turns the missing sidecar into a failure. - if err := AssertInjectedSidecarsPSARestricted(ctx, c, "ns", "mon", "otel-collector")(); err == nil || + if err := AssertSidecarsInjected(ctx, c, "ns", "mon", "otel-collector")(); err == nil || !strings.Contains(err.Error(), "otel-collector") { t.Fatalf("want missing-otel error, got %v", err) } - if err := AssertInjectedSidecarsPSARestricted(ctx, c, "ns", "full", "otel-collector")(); err != nil { - t.Fatalf("otel present and compliant: %v", err) - } - if err := AssertInjectedSidecarsPSARestricted(ctx, c, "ns", "badotel", "otel-collector")(); err == nil || - !strings.Contains(err.Error(), "seccompProfile") { - t.Fatalf("want otel seccomp error, got %v", err) - } - // A name that is not an injected sidecar is a spec bug; fail fast rather - // than spin in Eventually until the timeout. - if err := AssertInjectedSidecarsPSARestricted(ctx, c, "ns", "full", "postgres")(); err == nil || - !strings.Contains(err.Error(), "not a CNPG-I-injected sidecar") { - t.Fatalf("want unknown-sidecar error, got %v", err) - } -} - -func TestAssertInjectedSidecarsPSARestrictedIgnoresJobPods(t *testing.T) { - t.Parallel() - s := newScheme(t) - - // CNPG labels its bootstrap/join Job pods with cnpg.io/cluster, but the - // sidecar injector never adds containers to them. A lingering Job pod must - // not read as an instance that lost its collector. - instance := clusterPod("jobs-1", "jobs", - corev1.Container{Name: "documentdb-gateway", SecurityContext: psaRestrictedSC()}, - corev1.Container{Name: "otel-collector", SecurityContext: psaRestrictedSC()}, - ) - bootstrap := jobPod("jobs-1-initdb", "jobs", - corev1.Container{Name: "bootstrap-controller"}, - ) - // A cluster whose only pod is a bootstrap Job: nothing to check yet, and - // requiring a sidecar must not pass vacuously. - onlyJob := jobPod("early-1-initdb", "early", - corev1.Container{Name: "bootstrap-controller"}, - ) - - c := fake.NewClientBuilder().WithScheme(s). - WithObjects(instance, bootstrap, onlyJob).Build() - ctx := context.Background() - if err := AssertInjectedSidecarsPSARestricted(ctx, c, "ns", "jobs", "otel-collector")(); err != nil { - t.Fatalf("job pod alongside a healthy instance: %v", err) + if err := AssertSidecarsInjected(ctx, c, "ns", "full", "otel-collector")(); err != nil { + t.Fatalf("otel present: %v", err) } - if err := AssertInjectedSidecarsPSARestricted(ctx, c, "ns", "early", "otel-collector")(); err == nil || - !strings.Contains(err.Error(), "no injected sidecar") { - t.Fatalf("want no-injected-sidecar error, got %v", err) + if err := AssertSidecarsInjected(ctx, c, "ns", "full", + "documentdb-gateway", "otel-collector")(); err != nil { + t.Fatalf("both sidecars present: %v", err) } -} - -func TestAssertInjectedSidecarsPSARestrictedNeedsAnInstancePod(t *testing.T) { - t.Parallel() - s := newScheme(t) - - // Sidecars present, but on a pod that is not labelled as an instance — - // the shape we would see if CNPG stopped stamping cnpg.io/podRole. The - // per-pod requirement would then match nothing, so without this guard - // requireSidecars would silently stop being enforced. - orphan := jobPod("orphan-0", "orphan", - corev1.Container{Name: "documentdb-gateway", SecurityContext: psaRestrictedSC()}, - ) - c := fake.NewClientBuilder().WithScheme(s).WithObjects(orphan).Build() - - err := AssertInjectedSidecarsPSARestricted(context.Background(), c, "ns", "orphan", "otel-collector")() - if err == nil || !strings.Contains(err.Error(), "no instance pods") { + // The Job pod above shares the cluster label and has no sidecars; it must + // not be read as an instance that lost its collector. + if err := AssertSidecarsInjected(ctx, c, "ns", "early", "otel-collector")(); err == nil || + !strings.Contains(err.Error(), "no instance pods") { t.Fatalf("want no-instance-pods error, got %v", err) } - // With nothing required, the same cluster is still a normal pass. - if err := AssertInjectedSidecarsPSARestricted(context.Background(), c, "ns", "orphan")(); err != nil { - t.Fatalf("no requirement: %v", err) + + // Argument bugs fail immediately rather than spinning in Eventually. + if err := AssertSidecarsInjected(ctx, c, "ns", "full")(); err == nil || + !strings.Contains(err.Error(), "at least one sidecar name") { + t.Fatalf("want empty-names error, got %v", err) + } + if err := AssertSidecarsInjected(ctx, c, "ns", "full", "postgres")(); err == nil || + !strings.Contains(err.Error(), "not a CNPG-I-injected sidecar") { + t.Fatalf("want unknown-sidecar error, got %v", err) } } diff --git a/test/e2e/tests/resources/sidecar_resources_test.go b/test/e2e/tests/resources/sidecar_resources_test.go index d80881e6a..18aae42b0 100644 --- a/test/e2e/tests/resources/sidecar_resources_test.go +++ b/test/e2e/tests/resources/sidecar_resources_test.go @@ -108,16 +108,23 @@ var _ = Describe("Sidecar memory carve-out", // This is the suite's only monitoring-on cluster, so it is the // only place the collector's PSA hardening (#387) can be checked - // end-to-end. The fixture labels the namespace restricted, so - // reaching healthy already proves the sidecar passed admission; - // the explicit field checks name the offending field instead of - // leaving an opaque pod-creation failure. otel-collector is - // named as required so a collector that silently fails to inject - // is a failure rather than a vacuous pass on the gateway alone. - Eventually(assertions.AssertInjectedSidecarsPSARestricted( + // end-to-end. Two separate assertions because they fail for + // different reasons: a missing collector is a plumbing problem, + // a bad securityContext is the #387 problem. + Eventually(assertions.AssertSidecarsInjected( ctx, c, cr.Namespace, cr.Name, otelContainerName), timeouts.For(timeouts.DocumentDBReady), timeouts.PollInterval(timeouts.DocumentDBReady), + ).Should(Succeed(), "monitoring-on cluster must have the otel-collector injected") + + // The fixture labels the namespace restricted, so reaching + // healthy already proves the sidecars passed admission; the + // explicit field checks name the offending field instead of + // leaving an opaque pod-creation failure. + Eventually(assertions.AssertInjectedSidecarsPSARestricted( + ctx, c, cr.Namespace, cr.Name), + timeouts.For(timeouts.DocumentDBReady), + timeouts.PollInterval(timeouts.DocumentDBReady), ).Should(Succeed(), "monitoring-on cluster pods must carry PSA-restricted securityContext") })