Conversation
Reviewer's GuideThis PR hardens Kubernetes e2e tests for the policy-controller-operator by adding robust cleanup of controller resources, making the installation suites serial, retrying pod admission, and tightening image policy configuration using a computed image repository prefix while also updating the helm chart webhook image digest. Sequence diagram for hardened e2e installation and cleanup flowsequenceDiagram
participant E2E as E2ETestSuite
participant K8s as KubernetesCluster
participant Op as PolicyControllerOperator
participant WH as PolicyControllerWebhook
E2E->>K8s: Apply operator Helm chart
K8s-->>Op: Create operator deployment
Op-->>K8s: Create webhook deployment
K8s-->>WH: Start webhook pods
E2E->>K8s: Create test namespace and resources
E2E->>K8s: Create test pod
loop PodAdmissionRetry
K8s->>WH: AdmissionReview for test pod
WH-->>K8s: Validate pod image against ClusterImagePolicy
alt AdmissionDeniedOrWebhookNotReady
E2E->>K8s: Wait and retry getting pod status
else AdmissionAllowed
K8s-->>E2E: Pod becomes Running (loop ends)
end
end
E2E->>K8s: Run verification assertions
par RobustCleanup
E2E->>K8s: Delete test pod and namespace
E2E->>K8s: Delete test ClusterImagePolicy and other CRs
E2E->>K8s: Delete operator resources
and
K8s-->>WH: Terminate webhook pods
K8s-->>Op: Terminate operator deployment
end
K8s-->>E2E: All resources removed
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
PR Compliance Guide 🔍Below is a summary of compliance checks for this PR:
Compliance status legend🟢 - Fully Compliant🟡 - Partial Compliant 🔴 - Not Compliant ⚪ - Requires Further Human Verification 🏷️ - Compliance label |
||||||||||||||||||||||||
SequeI
approved these changes
Dec 4, 2025
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- The
Eventuallyblocks that wrapCreateTestPodin the three e2e suites are identical aside from namespace/image; consider extracting this into a shared helper to avoid repetition and keep the behavior consistent if the polling logic needs to change again. - In
WaitForPolicyControllerResourcesDeleted, the hard-coded list of policy-controller resources may drift from what the operator actually creates; if possible, consider deriving this list from labels or ownership (e.g., listing by label selector) to make the cleanup more resilient to future changes. - For the new
Eventuallyusages, you configure polling but rely on the default timeout; consider setting an explicit.WithTimeout(...)to make the expected maximum wait time clear and avoid excessively long hangs if admission never becomes allowed.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `Eventually` blocks that wrap `CreateTestPod` in the three e2e suites are identical aside from namespace/image; consider extracting this into a shared helper to avoid repetition and keep the behavior consistent if the polling logic needs to change again.
- In `WaitForPolicyControllerResourcesDeleted`, the hard-coded list of policy-controller resources may drift from what the operator actually creates; if possible, consider deriving this list from labels or ownership (e.g., listing by label selector) to make the cleanup more resilient to future changes.
- For the new `Eventually` usages, you configure polling but rely on the default timeout; consider setting an explicit `.WithTimeout(...)` to make the expected maximum wait time clear and avoid excessively long hangs if admission never becomes allowed.
## Individual Comments
### Comment 1
<location> `test/e2e/byok_install_test.go:249-256` </location>
<code_context>
})
It("should accept the pod", func(ctx SpecContext) {
- Expect(e2e_utils.CreateTestPod(ctx, k8sClient, byokTestNS, byokImage)).NotTo(HaveOccurred())
+ Eventually(func(ctx SpecContext) error {
+ err := e2e_utils.CreateTestPod(ctx, k8sClient, byokTestNS, byokImage)
+ if apierrors.IsAlreadyExists(err) {
+ return nil
+ }
+ return err
+ }).WithContext(ctx).WithPolling(5*time.Second).Should(Succeed(), "pod admission never became allowed")
})
})
</code_context>
<issue_to_address>
**suggestion (testing):** Deduplicate the new Eventually pod-creation pattern across e2e suites
This Eventually-based pod admission check (including the AlreadyExists handling) is now duplicated across BYOK, common installation, and serialized TUF root suites. To simplify future changes to polling or error handling, extract it into a shared helper in `test/e2e/utils` (e.g., `EventuallyCreateTestPod(...)`) and reuse it in all three tests.
Suggested implementation:
```golang
It("should accept the pod", func(ctx SpecContext) {
e2e_utils.EventuallyCreateTestPod(ctx, k8sClient, byokTestNS, byokImage)
})
})
```
To fully implement the deduplication you described, you will also need to:
1. Add a helper to `test/e2e/utils` (e.g., in `test/e2e/utils/pod.go` or similar):
- `func EventuallyCreateTestPod(ctx SpecContext, k8sClient client.Client, ns, image string)` that:
- Wraps `Eventually(func(ctx SpecContext) error { ... }).WithContext(ctx).WithPolling(5*time.Second).Should(Succeed(), "pod admission never became allowed")`
- Calls `CreateTestPod(...)` and treats `apierrors.IsAlreadyExists(err)` as success.
2. Update the other suites that currently duplicate this pattern (common installation and serialized TUF root tests) to call `e2e_utils.EventuallyCreateTestPod(...)` instead of inlining the `Eventually` block.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
PR Code Suggestions ✨Explore these optional code suggestions:
|
||||||||||||
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.
PR Type
Tests, Enhancement
Description
Add
Serialflag to e2e test suites for sequential executionImplement proper wait function for policy controller resource cleanup
Add retry logic with polling for pod admission test cases
Extract image repository prefix for cluster image policy templates
Update policy controller image version in helm values
Diagram Walkthrough
File Walkthrough
byok_install_test.go
Add serial execution and retry logic to BYOK teststest/e2e/byok_install_test.go
Serialflag to test suite for sequential executionWaitForPolicyControllerResourcesDeletedcall in cleanupTEST_IMAGE_PREFIXparameter to template renderingEventuallywith retry logic and pollingtimeandapierrorspackagescommon_install_test.go
Add serial execution and retry logic to common teststest/e2e/common_install_test.go
Serialflag to test suite for sequential executionWaitForPolicyControllerResourcesDeletedcall in cleanupTEST_IMAGE_PREFIXparameter to template renderingEventuallywith retry logic and pollingtimeandapierrorspackagesserialized_tuf_root_install_test.go
Add serial execution and retry logic to TUF teststest/e2e/serialized_tuf_root_install_test.go
Serialflag to test suite for sequential executionWaitForPolicyControllerResourcesDeletedcall in cleanupTEST_IMAGE_PREFIXparameter to template renderingEventuallywith retry logic and pollingtimeandapierrorspackagesimage.go
Add image repository prefix extraction utilitytest/e2e/utils/image.go
ImageRepoPrefixfunction to extract image repository prefix@or
:characterstringspackagekubernetes.go
Add comprehensive resource cleanup wait functiontest/e2e/utils/kubernetes.go
WaitForPolicyControllerResourcesDeletedfunction for comprehensiveresource cleanup
exponential backoff
admissionregistrationv1importcommon_cluster_image_policy.yaml.tpl
Update cluster image policy glob pattern templatetest/e2e/custom_resources/cluster_image_policies/common_cluster_image_policy.yaml.tpl
TEST_IMAGE_PREFIXtemplate variable**to{{ .TEST_IMAGE_PREFIX }}**for morespecific image matching
values.yaml
Update policy controller image versionhelm-charts/policy-controller-operator/values.yaml
sha256:ea4ac2b005571bc28270f8420dff30a0f02381bcd78153e1014f9006d1a17609to
sha256:8b9bd18603f4cdba8b1243c3285e716b31e00d238e0566ba101ffe5f5678739a