QA: E2E tests for SPIRE-439 SCC Hardening for SPIRE Agent (PR #105)#111
QA: E2E tests for SPIRE-439 SCC Hardening for SPIRE Agent (PR #105)#111sujkini wants to merge 1 commit into
Conversation
…enshift#105) Add E2E coverage for the security hardening introduced by PR openshift#105: - Assert HostIPC=false and HostPort=0 on SPIRE Agent DaemonSet pod spec - Assert AllowHostIPC=false on the spire-agent SCC - Verify replacement pods after deletion are still admitted under the hardened spire-agent SCC (not a privileged fallback) - Verify the operator reconciles SCC drift back to AllowHostNetwork=false - Register securityv1 scheme in BeforeSuite for typed SCC client access Overall coverage score for PR-relevant journeys: 87/100 (>= 85 threshold). Deferred: runAsNonRoot and seccompProfile assertions pending upstream container SecurityContext additions. Made-with: Cursor
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
WalkthroughThese changes add OpenShift security hardening validation to the end-to-end test suite by registering the Security API with the runtime scheme and implementing comprehensive tests that verify SPIRE Agent DaemonSet configuration, Security Context Constraints (SCC) policies, container security contexts, pod admission, and SCC drift correction behavior. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 9 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (9 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.11.4)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: sujkini The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
test/e2e/e2e_test.go (3)
1807-1811: RedundantBeforeEachblocks — outerBeforeEachalready handles this.The outer
BeforeEachat lines 76‑80 already initializestestCtxwithutils.TestContextTimeoutand registers the cancel asDeferCleanupfor everyItin the suite. Re-doing it inside these Contexts shadows the outer setup with no behavioral difference and is dead code. Other Contexts in this file (e.g., "SpireAgent attestation", "Common configurations") deliberately rely on the outerBeforeEachand don't redeclare it.♻️ Suggested cleanup
Context("SpireAgent DaemonSet security hardening", func() { - BeforeEach(func() { - var cancel context.CancelFunc - testCtx, cancel = context.WithTimeout(context.Background(), utils.TestContextTimeout) - DeferCleanup(cancel) - }) - It("SPIRE Agent DaemonSet should have host networking disabled", ...Apply the same removal in the "SpireAgent SCC hardening" Context.
Also applies to: 1879-1883
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/e2e_test.go` around lines 1807 - 1811, Remove the redundant BeforeEach blocks that reinitialize testCtx and register the cancel via DeferCleanup inside the "SpireAgent SCC hardening" Context (and the other Context at the referenced location); these duplicate the outer BeforeEach already setting testCtx with utils.TestContextTimeout. Locate the inner BeforeEach functions that create a local cancel via context.WithTimeout(context.Background(), utils.TestContextTimeout) and call DeferCleanup(cancel) and delete those inner blocks so the suite relies on the outer BeforeEach's testCtx initialization.
1929-1942: Pod list may include terminating/pending pods — consider filtering to active pods.
pods.Itemsreturned fromListincludes pods in any phase (Pending,Succeeded,Failed) and pods with aDeletionTimestampstill in the API server. Earlier tests in this file (e.g., the OIDC resource test at line 1434) defensively useutils.FilterActivePods(...). If a previous test leaves a terminating agent pod transiently visible, this assertion can flake on the lingering pod's annotation. Suggest filtering to running, non-terminating pods before iterating:♻️ Suggested filter
Expect(err).NotTo(HaveOccurred(), "failed to list SPIRE Agent pods") Expect(pods.Items).NotTo(BeEmpty(), "expected at least one running SPIRE Agent pod") + activePods := utils.FilterActivePods(pods.Items) + Expect(activePods).NotTo(BeEmpty(), "expected at least one Running SPIRE Agent pod") By("Verifying each pod is admitted under the spire-agent SCC") - for _, pod := range pods.Items { + for _, pod := range activePods {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/e2e_test.go` around lines 1929 - 1942, The loop that checks pods.Items for the openshift.io/scc annotation can include terminating or non-running pods and flake; before iterating use the existing utility to restrict to active, non-terminating pods (e.g., call utils.FilterActivePods(pods.Items) or an equivalent filter that excludes pods with non-Running phase or a non-nil DeletionTimestamp) and then iterate over the filtered slice when checking each pod.Annotate references: use pods.Items as input, FilterActivePods helper, and still assert the "openshift.io/scc" annotation and value "spire-agent" on each resulting pod.
1854-1863: Use value-aware matchers for*boolfields to avoid coupling to "explicitly set" semantics and surface clearer failures when nil.
Equal(ptr.To(false))usesreflect.DeepEqual, which conflates two states:
- the pointer is
nil(field unset; default behavior applies), and- the pointer is set to
false.If code removes the explicit
ptr.To(true)rather than replacing it withptr.To(false), the assertion will fail with an opaque diff (<nil> != *false) even though runtime behavior is correct. UseHaveValue(BeFalse())(orHaveValue(BeTrue())) instead, which explicitly asserts the pointer is non-nil and holds the expected value, with clearer failure messages.Suggested changes
-Expect(sc.Privileged).To(Equal(ptr.To(false)), +Expect(sc.Privileged).To(HaveValue(BeFalse()), "spire-agent container must not run as privileged after SCC hardening") -Expect(sc.AllowPrivilegeEscalation).To(Equal(ptr.To(false)), +Expect(sc.AllowPrivilegeEscalation).To(HaveValue(BeFalse()), "spire-agent container must not allow privilege escalation") -Expect(sc.ReadOnlyRootFilesystem).To(Equal(ptr.To(true)), +Expect(sc.ReadOnlyRootFilesystem).To(HaveValue(BeTrue()), "spire-agent container must use a read-only root filesystem")Also applies to lines 1908‑1909 (
scc.AllowPrivilegeEscalation).If the intent is specifically to require the field be explicitly set (i.e., reject
nil), keepEqualbut add a precedingExpect(sc.Privileged).NotTo(BeNil())for clarity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/e2e_test.go` around lines 1854 - 1863, Replace pointer equality checks that use Equal(ptr.To(false/true)) on the securityContext fields with value-aware matchers so nil vs set is clear: for sc.Privileged and sc.AllowPrivilegeEscalation use Expect(sc.Privileged).To(HaveValue(BeFalse())) and Expect(sc.AllowPrivilegeEscalation).To(HaveValue(BeFalse())); for sc.ReadOnlyRootFilesystem use Expect(sc.ReadOnlyRootFilesystem).To(HaveValue(BeTrue())). If you intentionally require non-nil semantics instead, add explicit NotTo(BeNil()) checks (e.g., Expect(sc.Privileged).NotTo(BeNil())) before using Equal.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/e2e/e2e_test.go`:
- Around line 2002-2024: Capture the SCC's original AllowHostNetwork value
before calling UpdateCRWithRetry and register a Ginkgo DeferCleanup that
restores that original state (e.g., using the same utils.UpdateCRWithRetry or
k8sClient operations) so the scc named "spire-agent" is reset even if the
operator never reconciles; reference the existing variables/test helpers (scc,
UpdateCRWithRetry, testCtx, k8sClient) to implement the cleanup and ensure it
runs immediately after reading the original value and before mutating the SCC.
---
Nitpick comments:
In `@test/e2e/e2e_test.go`:
- Around line 1807-1811: Remove the redundant BeforeEach blocks that
reinitialize testCtx and register the cancel via DeferCleanup inside the
"SpireAgent SCC hardening" Context (and the other Context at the referenced
location); these duplicate the outer BeforeEach already setting testCtx with
utils.TestContextTimeout. Locate the inner BeforeEach functions that create a
local cancel via context.WithTimeout(context.Background(),
utils.TestContextTimeout) and call DeferCleanup(cancel) and delete those inner
blocks so the suite relies on the outer BeforeEach's testCtx initialization.
- Around line 1929-1942: The loop that checks pods.Items for the
openshift.io/scc annotation can include terminating or non-running pods and
flake; before iterating use the existing utility to restrict to active,
non-terminating pods (e.g., call utils.FilterActivePods(pods.Items) or an
equivalent filter that excludes pods with non-Running phase or a non-nil
DeletionTimestamp) and then iterate over the filtered slice when checking each
pod.Annotate references: use pods.Items as input, FilterActivePods helper, and
still assert the "openshift.io/scc" annotation and value "spire-agent" on each
resulting pod.
- Around line 1854-1863: Replace pointer equality checks that use
Equal(ptr.To(false/true)) on the securityContext fields with value-aware
matchers so nil vs set is clear: for sc.Privileged and
sc.AllowPrivilegeEscalation use Expect(sc.Privileged).To(HaveValue(BeFalse()))
and Expect(sc.AllowPrivilegeEscalation).To(HaveValue(BeFalse())); for
sc.ReadOnlyRootFilesystem use
Expect(sc.ReadOnlyRootFilesystem).To(HaveValue(BeTrue())). If you intentionally
require non-nil semantics instead, add explicit NotTo(BeNil()) checks (e.g.,
Expect(sc.Privileged).NotTo(BeNil())) before using Equal.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: cfdbabb6-f725-477a-8be2-3413b9457442
📒 Files selected for processing (2)
test/e2e/e2e_suite_test.gotest/e2e/e2e_test.go
| By("Patching the SCC to reintroduce host network access (simulating manual drift)") | ||
| err = utils.UpdateCRWithRetry(testCtx, k8sClient, scc, func() { | ||
| scc.AllowHostNetwork = true | ||
| }) | ||
| Expect(err).NotTo(HaveOccurred(), "failed to patch SCC to introduce drift") | ||
| fmt.Fprintf(GinkgoWriter, "introduced drift: SCC AllowHostNetwork set to true\n") | ||
|
|
||
| By("Verifying the operator reconciles the SCC back to AllowHostNetwork: false") | ||
| Eventually(func() bool { | ||
| current := &securityv1.SecurityContextConstraints{} | ||
| if err := k8sClient.Get(testCtx, types.NamespacedName{Name: "spire-agent"}, current); err != nil { | ||
| fmt.Fprintf(GinkgoWriter, "failed to get SCC: %v\n", err) | ||
| return false | ||
| } | ||
| if current.AllowHostNetwork { | ||
| fmt.Fprintf(GinkgoWriter, "SCC AllowHostNetwork is still true — drift not yet corrected\n") | ||
| return false | ||
| } | ||
| fmt.Fprintf(GinkgoWriter, "SCC drift corrected: AllowHostNetwork restored to false\n") | ||
| return true | ||
| }).WithTimeout(utils.DefaultTimeout).WithPolling(utils.ShortInterval).Should(BeTrue(), | ||
| "operator should reconcile SCC AllowHostNetwork back to false within %v", utils.DefaultTimeout) | ||
| }) |
There was a problem hiding this comment.
Add a defensive DeferCleanup to restore the SCC if the reconciler fails to converge.
If the operator does not reconcile within DefaultTimeout (or the test fails for any other reason after introducing drift), the cluster is left with spire-agent SCC AllowHostNetwork=true. That weakens the security posture for the rest of the suite (and any subsequent re-run that polls "must already be in hardened state before drift test" at line 2000 will pass — masking the original failure). A best-effort cleanup keeps the suite hermetic regardless of reconciler behavior:
🛡️ Suggested defensive cleanup
By("Patching the SCC to reintroduce host network access (simulating manual drift)")
err = utils.UpdateCRWithRetry(testCtx, k8sClient, scc, func() {
scc.AllowHostNetwork = true
})
Expect(err).NotTo(HaveOccurred(), "failed to patch SCC to introduce drift")
+ DeferCleanup(func(ctx context.Context) {
+ current := &securityv1.SecurityContextConstraints{}
+ if err := k8sClient.Get(ctx, types.NamespacedName{Name: "spire-agent"}, current); err == nil && current.AllowHostNetwork {
+ _ = utils.UpdateCRWithRetry(ctx, k8sClient, current, func() {
+ current.AllowHostNetwork = false
+ })
+ }
+ })
fmt.Fprintf(GinkgoWriter, "introduced drift: SCC AllowHostNetwork set to true\n")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/e2e/e2e_test.go` around lines 2002 - 2024, Capture the SCC's original
AllowHostNetwork value before calling UpdateCRWithRetry and register a Ginkgo
DeferCleanup that restores that original state (e.g., using the same
utils.UpdateCRWithRetry or k8sClient operations) so the scc named "spire-agent"
is reset even if the operator never reconciles; reference the existing
variables/test helpers (scc, UpdateCRWithRetry, testCtx, k8sClient) to implement
the cleanup and ensure it runs immediately after reading the original value and
before mutating the SCC.
1 participant
E2E Test Generation
Source PR: #105
Jira: SPIRE-439
Generated by: Iterative Analysis Rule (automated QA E2E tooling)
What this PR adds
E2E test coverage for the security hardening introduced by PR #105
(
pkg/controller/spire-agent/daemonset.goandpkg/controller/spire-agent/scc.go).New
Contextblocks intest/e2e/e2e_test.goSpireAgent DaemonSet security hardeningSPIRE Agent DaemonSet should have host networking disabledAsserts
HostNetwork=false,DNSPolicy=ClusterFirst,HostPID=true(retained), andHostIPC=false(mirrors SCCAllowHostIPC: false).SPIRE Agent DaemonSet container should have a hardened security contextAsserts
Privileged=false,AllowPrivilegeEscalation=false,ReadOnlyRootFilesystem=true,Capabilities.Drop=[ALL], and no container HostPorts (mirrors SCCAllowHostPorts: false).SpireAgent SCC hardeningSPIRE Agent SCC should restrict host access and privileged executionAsserts all tightened SCC fields including
AllowHostIPC=false(added),AllowHostNetwork=false,AllowHostPorts=false,AllowPrivilegedContainer=false,AllowPrivilegeEscalation=false,RunAsUser=RunAsAny,ReadOnlyRootFilesystem=true,AllowHostPID=true,RequiredDropCapabilities=[ALL].SPIRE Agent pods should be bound to the hardened spire-agent SCCVerifies
openshift.io/scc: spire-agentannotation on running pods.SPIRE Agent replacement pod after deletion should be admitted under the hardened spire-agent SCC(new)Deletes one SPIRE Agent pod, waits for the DaemonSet replacement, then verifies the replacement pod is also admitted under
spire-agent(not aprivilegedfallback).spire-agent SCC should be reconciled back to hardened state if manually modified(new)Patches
AllowHostNetwork=trueon the SCC, then verifies the operator reconciles it back tofalsewithin the standard timeout — proving the SCC is under active reconciliation.Suite file update (
test/e2e/e2e_suite_test.go)Registers
securityv1scheme inBeforeSuiteso the typedk8sClient.GetforSecurityContextConstraintsworks correctly.Test Coverage
Red Hat Certification Checklist
InstallationcontextSpireAgent SCC hardeningcontextrunAsNonRootdeferred (not set by PR SPIRE-439: SCC Hardening for Spire Agent #105)SpireAgent attestationcontextDeferred (not actionable against PR #105 implementation)
runAsNonRoot: trueassertion — PR SPIRE-439: SCC Hardening for Spire Agent #105 does not setrunAsNonRooton the container; a follow-up should addrunAsUser: 65532 / runAsNonRoot: trueto close the CodeRabbit-flagged risk.seccompProfile.type: RuntimeDefault— not set by PR SPIRE-439: SCC Hardening for Spire Agent #105; future improvement.Files Modified
test/e2e/e2e_test.go— newContextblocks and extended assertionstest/e2e/e2e_suite_test.go—securityv1scheme registrationHow to Run
Made with Cursor
Summary by CodeRabbit