test/e2e: add focused SVID attestation coverage with shared helpers#115
Open
sayak-redhat wants to merge 6 commits into
Open
test/e2e: add focused SVID attestation coverage with shared helpers#115sayak-redhat wants to merge 6 commits into
sayak-redhat wants to merge 6 commits into
Conversation
Refactor attestation E2E setup into reusable utils and add targeted checks for SVID certificate validity, SPIFFE ID correctness, expiry bounds, and rotation using a short TTL. This removes duplicated setup/parsing logic and keeps attestation specs easier to read and maintain.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds PEM/X.509 utilities and a test attestation fixture; updates SpireServer TTL sourcing; replaces manual workload attestation with fixture-driven SVID validation (PEM parsing, SPIFFE ID, TTL bounds, bundle chain) and adds a rotation-before-expiry test. ChangesSVID Attestation Testing Infrastructure
Sequence DiagramsequenceDiagram
participant Tester as Test Runner
participant K8s as Kubernetes API
participant Pod as Attestation Pod
participant CSI as SPIFFE CSI Driver
participant Spire as SPIRE Agent/Server
participant Parser as Test Utilities (x509)
Tester->>K8s: Create Namespace, ClusterSPIFFEID, ServiceAccount, ConfigMap
Tester->>K8s: Create Pod spec (mount CSI, emptyDir /certs)
K8s->>CSI: CSI volume mount request for Pod
CSI->>Spire: Request SVID for Pod/SA
Spire->>CSI: Provide SVID files (svid.pem, svid_key.pem, bundle.pem)
CSI->>Pod: Populate /certs with SVID files
K8s->>Tester: Pod becomes Ready
Tester->>Pod: Exec "cat /certs/svid.pem"
Pod-->>Tester: stdout (PEM)
Tester->>Parser: Parse PEM -> x509.Certificate, inspect SPIFFE ID, expiry
Parser-->>Tester: Validation results (valid cert, SPIFFE ID, TTL bounds, rotation checks)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes 🚥 Pre-merge checks | ✅ 12✅ Passed checks (12 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: sayak-redhat 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
🤖 Prompt for all review comments with AI agents
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 `@test/e2e/utils/utils.go`:
- Around line 927-953: The test uses deterministic names (ns, podName, saName
and ClusterSPIFFEID ObjectMeta.Name -> cspiffeID) computed from prefix which
causes AlreadyExists flakes; update SetupAttestationTest to append a unique
suffix (e.g., short random/UUID or timestamp) to ns, podName, saName and the
ClusterSPIFFEID name (and update dependent selectors/labels like PodSelector
MatchLabels and NamespaceSelector MatchLabels if they reference the base name)
so every test run/parallel spec creates distinct Namespace, Pod, ServiceAccount
and ClusterSPIFFEID objects (ensure the code paths that reference attestationNS,
podName, saName, and cspiffeID use the new suffixed values).
🪄 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: 6a9aafc6-4d3c-4322-a75c-e9b03d69a865
📒 Files selected for processing (1)
test/e2e/utils/utils.go
rausingh-rh
reviewed
May 20, 2026
rausingh-rh
reviewed
May 20, 2026
rausingh-rh
reviewed
May 20, 2026
rausingh-rh
reviewed
May 20, 2026
rausingh-rh
reviewed
May 20, 2026
rausingh-rh
reviewed
May 20, 2026
| By("Deleting attestation test namespace") | ||
| _ = k8sClient.Delete(ctx, attestationNS) | ||
| }) | ||
| It("SVID expiry time should be within valid TTL bounds", Label("reconciliation"), func() { |
Contributor
There was a problem hiding this comment.
can be use Label("attestation") instead in all the newly added 4 test cases?
Contributor
|
suggestion: Consider adding a test (in this PR or a follow-up) that verifies:
|
- Share a single attestation fixture across X.509 validity, SPIFFE ID
match, and TTL bounds tests using a nested Context with BeforeAll,
reducing setup from 4 attestation cycles to 2.
- Reuse rotated certificate captured inside the Eventually block instead
of reading it again, and add assertion that rotated cert is genuinely
newer than the initial one.
- Replace hardcoded TTL bound with DefaultX509SVIDTTL constant shared
between SpireServer CR creation and the TTL assertion.
- Log cleanup errors to GinkgoWriter instead of silently discarding them.
- Use Label("attestation") on all SVID test cases.
- Add bundle.pem CA validation and SVID chain-of-trust verification test.
Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
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 `@test/e2e/e2e_test.go`:
- Around line 404-406: BeforeAll is calling SetupAttestationTest with testCtx
which is only initialized in BeforeEach, so create a dedicated context inside
BeforeAll (e.g., allCtx or beforeAllCtx) and pass that to SetupAttestationTest
instead of testCtx; also ensure you cancel/cleanup that context in AfterAll to
avoid leaks. Update references in the test to use the new before-all context for
the SetupAttestationTest call and add a matching cancellation in AfterAll.
🪄 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: 38558a73-0608-41d4-b9a8-7f1a2e8a9942
📒 Files selected for processing (3)
test/e2e/e2e_test.gotest/e2e/utils/constants.gotest/e2e/utils/utils.go
✅ Files skipped from review due to trivial changes (1)
- test/e2e/utils/constants.go
SPIRE adds a small clock skew buffer (~10s) to certificate validity, so the actual TTL can slightly exceed the configured DefaultX509Validity. Add a ClockSkewTolerance constant (1m) to the upper bound check to prevent flaky CI failures. Co-authored-by: Cursor <cursoragent@cursor.com>
BeforeAll runs before the first BeforeEach, so testCtx (initialized in BeforeEach) is uninitialized at that point. Create a dedicated context with TestContextTimeout for the setup call to avoid using a zero-value context. Co-authored-by: Cursor <cursoragent@cursor.com>
Contributor
Author
|
/retest |
1 similar comment
Contributor
Author
|
/retest |
Contributor
Author
|
/test |
Contributor
Author
|
/retest |
…ky AlreadyExists failures Generate a random 6-char hex suffix for namespace, pod, service account, and ClusterSPIFFEID names so that re-runs or parallel specs never collide. Co-authored-by: Cursor <cursoragent@cursor.com>
|
@sayak-redhat: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit