OCPCLOUD-3542: Add CRD Compatibility Checker OTE e2e tests#599
OCPCLOUD-3542: Add CRD Compatibility Checker OTE e2e tests#599stefanonardo wants to merge 1 commit into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughAdds a comprehensive Ginkgo/Gomega e2e test suite for the CRDCompatibilityRequirement controller that verifies deployment health, validates CRD-level schema compatibility enforcement (accepting compatible updates, rejecting incompatible ones), and tests CR-level object validation through dynamic webhooks. Introduces test helper functions for resource lifecycle management, registers OpenShift ChangesCRD Compatibility e2e Test Suite
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~28 minutes 🚥 Pre-merge checks | ✅ 12 | ❌ 3❌ Failed checks (3 warnings)
✅ 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: 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 |
stefanonardo
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@stefanonardo: This pull request references OCPCLOUD-3542 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
![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 (1)
e2e/crd_compatibility_test.go (1)
132-137: ⚡ Quick winAdd explicit assertion messages on setup assertions for faster triage.
These setup-time
Expect(...).To(Succeed())calls should include failure messages (resource name/context) to make e2e failures actionable.As per coding guidelines, “Assertion messages—assertions should include meaningful failure messages, flag missing messages.”
Example improvement
- Expect(cl.Create(ctx, crd)).To(Succeed()) + Expect(cl.Create(ctx, crd)).To(Succeed(), "failed to create test CRD %s", crd.Name) - Expect(cl.Create(ctx, requirement)).To(Succeed()) + Expect(cl.Create(ctx, requirement)).To(Succeed(), "failed to create CompatibilityRequirement %s", requirement.Name)🤖 Prompt for 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. In `@e2e/crd_compatibility_test.go` around lines 132 - 137, Add explicit assertion failure messages to the setup-time Expect().To(Succeed()) calls for the CRD and CompatibilityRequirement creation. Include meaningful context in each assertion message (such as the resource name and operation being performed) by passing a message string as an argument to the To(Succeed()) calls. This will provide actionable error information when these assertions fail during e2e test execution, making it easier to diagnose failures in the test setup phase.Source: Coding guidelines
🤖 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 `@e2e/crd_compatibility_test.go`:
- Around line 160-201: The test title in the It() block claims to verify
"webhook services" but the implementation only validates
ValidatingWebhookConfiguration resources (at lines 196-200), without checking
any Service objects. Either add explicit assertions to verify the webhook
Services exist using cl.Get() with the appropriate Service object keys in the
framework.CompatibilityRequirementsNamespace, or update the test description
string to remove the reference to "webhook services" so it accurately reflects
only the ValidatingWebhookConfiguration validation that is performed.
---
Nitpick comments:
In `@e2e/crd_compatibility_test.go`:
- Around line 132-137: Add explicit assertion failure messages to the setup-time
Expect().To(Succeed()) calls for the CRD and CompatibilityRequirement creation.
Include meaningful context in each assertion message (such as the resource name
and operation being performed) by passing a message string as an argument to the
To(Succeed()) calls. This will provide actionable error information when these
assertions fail during e2e test execution, making it easier to diagnose failures
in the test setup phase.
🪄 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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: d3f9cce6-3717-44fb-8852-69c2ef9de93d
📒 Files selected for processing (2)
e2e/crd_compatibility_test.goe2e/e2e_common.go
stefanonardo
force-pushed
the
OCPCLOUD-3542
branch
2 times, most recently
from
2c1b25e to
46e7394
Compare
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 `@e2e/crd_compatibility_test.go`:
- Around line 183-200: The pod status checks and ValidatingWebhookConfiguration
existence assertions in the test (pod listing loop checking Phase equals
corev1.PodRunning, and the two client.Get calls for staticVWCCRDValidation and
staticVWCCRValidation webhook configurations) are performing one-shot
synchronous reads that don't account for eventual consistency. Wrap these
assertions with Eventually blocks that include appropriate timeout and retry
intervals to wait for pods to reach Running state and for the webhook
configurations to become available, rather than expecting them to be immediately
ready after deployment.
🪄 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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 846ab123-182b-4c77-8297-3d24aed34cea
⛔ Files ignored due to path filters (2)
e2e/go.sumis excluded by!**/*.sumvendor/modules.txtis excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (3)
e2e/crd_compatibility_test.goe2e/e2e_common.goe2e/go.mod
🚧 Files skipped from review as they are similar to previous changes (1)
- e2e/e2e_common.go
mdbooth
left a comment
mdbooth
left a comment
There was a problem hiding this comment.
I've paused reviewing for the moment. Posted discussion point in https://redhat-internal.slack.com/archives/C05KZA3NVU6/p1781619084762889
| func skipUnlessCompatibilityRequirementCRD() { | ||
| GinkgoHelper() | ||
|
|
||
| By("Checking for the presence of the CompatibilityRequirement CRD") | ||
|
|
||
| crd := &apiextensionsv1.CustomResourceDefinition{} | ||
|
|
||
| Eventually(func() error { | ||
| err := cl.Get(ctx, client.ObjectKey{Name: compatibilityRequirementCRDName}, crd) | ||
| if apierrors.IsNotFound(err) { | ||
| Skip("CompatibilityRequirement CRD is not installed (feature gate CRDCompatibilityRequirementOperator not enabled). Skipping CRD compatibility tests.") | ||
| } | ||
|
|
||
| return err | ||
| }).Should(Succeed(), "failed to check for CompatibilityRequirement CRD") | ||
| } |
There was a problem hiding this comment.
I feel like this should gate on the feature gate rather than the CRD. Can anybody more familiar with such things confirm? Perhaps @simkam or @JoelSpeed ?
There was a problem hiding this comment.
It looks like our existing tests check the feature gate instead of CRD presence.
| func cleanupCompatibilityRequirement(requirement *apiextensionsv1alpha1.CompatibilityRequirement) { | ||
| GinkgoHelper() | ||
|
|
||
| key := client.ObjectKeyFromObject(requirement) | ||
|
|
||
| Eventually(func() error { | ||
| err := cl.Delete(ctx, requirement) | ||
| if apierrors.IsNotFound(err) { | ||
| return nil | ||
| } | ||
|
|
||
| return err | ||
| }).WithTimeout(framework.WaitShort).Should(Succeed(), "failed to delete CompatibilityRequirement") | ||
|
|
||
| Eventually(func() bool { | ||
| err := cl.Get(ctx, key, &apiextensionsv1alpha1.CompatibilityRequirement{}) | ||
| return apierrors.IsNotFound(err) | ||
| }).WithTimeout(framework.WaitMedium).WithPolling(framework.RetryMedium).Should(BeTrue(), "CompatibilityRequirement was not fully deleted") | ||
| } | ||
|
|
||
| func cleanupTestCRD(crd *apiextensionsv1.CustomResourceDefinition) { | ||
| GinkgoHelper() | ||
|
|
||
| key := client.ObjectKeyFromObject(crd) | ||
|
|
||
| Eventually(func() error { | ||
| err := cl.Delete(ctx, crd) | ||
| if apierrors.IsNotFound(err) { | ||
| return nil | ||
| } | ||
|
|
||
| return err | ||
| }).WithTimeout(framework.WaitShort).Should(Succeed(), "failed to delete test CRD") | ||
|
|
||
| Eventually(func() bool { | ||
| err := cl.Get(ctx, key, &apiextensionsv1.CustomResourceDefinition{}) | ||
| return apierrors.IsNotFound(err) | ||
| }).WithTimeout(framework.WaitMedium).WithPolling(framework.RetryMedium).Should(BeTrue(), "test CRD was not fully deleted") | ||
| } |
There was a problem hiding this comment.
I think we already have a generic cleanupResource helper (or similar) which would cover both of these cases? Or perhaps that's only in the integration tests...
Can you look harder for one, anyway? If it doesn't exist we should write it. It's identical to both of these except it takes a client.Object instead of a specific type. You can use an Unstructured for the get if you don't want to get fancy.
| test.HaveCondition(apiextensionsv1alpha1.CompatibilityRequirementProgressing). | ||
| WithStatus(metav1.ConditionFalse). | ||
| WithReason(apiextensionsv1alpha1.CompatibilityRequirementUpToDateReason), | ||
| )) |
There was a problem hiding this comment.
We also need to check that status.ObservedCRD has the expected UID and Generation to avoid races.
| cleanupCompatibilityRequirement(requirement) | ||
| cleanupTestCRD(crd) |
There was a problem hiding this comment.
Ideally you want to put the DeferCleanup immediately after the create (so 2 DeferCleanups here). Otherwise a failure of the second Create means the first object will not be cleaned up.
Although, is there a createTestResource helper which both creates the object and also schedules its cleanup?
| By("Checking the CRD validation ValidatingWebhookConfiguration exists") | ||
| Expect(cl.Get(ctx, client.ObjectKey{Name: staticVWCCRDValidation}, &admissionregistrationv1.ValidatingWebhookConfiguration{})).To(Succeed()) | ||
|
|
||
| By("Checking the CompatibilityRequirement validation ValidatingWebhookConfiguration exists") | ||
| Expect(cl.Get(ctx, client.ObjectKey{Name: staticVWCCRValidation}, &admissionregistrationv1.ValidatingWebhookConfiguration{})).To(Succeed()) |
There was a problem hiding this comment.
IIRC these are created dynamically by the controller itself, so we should put these in an Eventually for robustness.
Also, top Ginkgo tip: the logs are better structured if you pass the actions associated with By to the By call as a lambda, e.g.:
By("Checking the CompatibilityRequirement validation ValidatingWebhookConfiguration exists", func() {
Eventually(cl.Get(ctx, client.ObjectKey{Name: staticVWCCRValidation}, &admissionregistrationv1.ValidatingWebhookConfiguration{})).Should(Succeed())
})| By("Checking all pods are Running") | ||
|
|
||
| pods := &corev1.PodList{} | ||
| Expect(cl.List(ctx, pods, | ||
| client.InNamespace(framework.CompatibilityRequirementsNamespace), | ||
| client.MatchingLabels{"k8s-app": crdCompatibilityDeploymentName}, | ||
| )).To(Succeed()) | ||
| Expect(pods.Items).ToNot(BeEmpty(), "expected at least one pod") |
| Context("CRD schema validation with Deny action", func() { | ||
| It("should reject incompatible CRD updates", func() { | ||
| testCRD := test.GenerateTestCRD() | ||
| requirement := test.GenerateTestCompatibilityRequirement(testCRD) | ||
| createTestCRDAndRequirement(testCRD, requirement) | ||
| waitForRequirementReady(requirement) |
There was a problem hiding this comment.
There's a lot of duplicated preamble here. We'd typically try to structure this nested. Perhaps:
Context("When a CRD has a valid CompatibilityRequirement", func() {
var (
testCRD *CRD...
required *CompatReq...
)
BeforeEach(func() {
testCRD = test.GenerateTestCRD()
requirement = test.GenerateTestCompatibilityRequirement(testCRD)
createTestCRDAndRequirement(testCRD, requirement)
waitForRequirementReady(requirement)
})
It("should reject incompatible CRD updates", func() {
...
})
It("should reject CRD deletion while a CompatibilityRequirement references it", func() {
...
})
})| Eventually(func() error { | ||
| fresh := &apiextensionsv1.CustomResourceDefinition{} | ||
| if err := cl.Get(ctx, client.ObjectKeyFromObject(testCRD), fresh); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| delete(fresh.Spec.Versions[0].Schema.OpenAPIV3Schema.Properties, "status") | ||
| fresh.Spec.Versions[0].Subresources = nil | ||
|
|
||
| err := cl.Update(ctx, fresh) | ||
| if err == nil { | ||
| return StopTrying("CRD update succeeded but should have been rejected") | ||
| } | ||
|
|
||
| if apierrors.IsForbidden(err) { | ||
| return nil | ||
| } | ||
|
|
||
| return err | ||
| }).WithTimeout(framework.WaitShort).WithPolling(framework.RetryShort).Should(Succeed(), "incompatible CRD update should be rejected with Forbidden") |
There was a problem hiding this comment.
Here we should match the returned error. So keep the StopTrying return if the error is nil, but after that just return the error. Then instead of Succeed() as the matcher, it should be "MatchError(...some specific portion of the error message...)"
stefanonardo
force-pushed
the
OCPCLOUD-3542
branch
from
46e7394 to
b622e08
Compare
|
/hold depends on OCPCLOUD-3539 |
openshift-ci
Bot
added
the
do-not-merge/hold
stefanonardo
force-pushed
the
OCPCLOUD-3542
branch
from
b622e08 to
f754efd
Compare
| Expect(*deployment.Spec.Replicas).To(BeNumerically(">", int32(1)), | ||
| "HA cluster should have more than 1 replica") | ||
| default: | ||
| Fail(fmt.Sprintf("unknown control plane topology %q, update this test to handle it", infra.Status.ControlPlaneTopology)) |
There was a problem hiding this comment.
it might change after review but currently all topologies, except SingleReplica, have 2 replicas.
openshift-ci
Bot
added
the
needs-rebase
stefanonardo
force-pushed
the
OCPCLOUD-3542
branch
from
f754efd to
b151aec
Compare
openshift-ci
Bot
removed
the
needs-rebase
Add 10 e2e tests covering all three webhook paths: CRD schema validation, object validation, and object pruning. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
stefanonardo
force-pushed
the
OCPCLOUD-3542
branch
from
b151aec to
bdd8eaa
Compare
stefanonardo
changed the title
|
@stefanonardo: The following test failed, say
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. |
|
Just want to leave the note that the OTE framework is now fully integrated (origin#31307, cluster-capi-operator#597). Once your test is integrated with OTE (e.g. by removing the
These are the presubmits that use |
5 participants
Summary
Add 10 e2e tests covering all three webhook paths of the CRD Compatibility Checker: CRD schema validation, object validation, and object pruning
Summary by CodeRabbit