OCPCLOUD-2992: added tests for OCP-86581 capi machine vaps #443
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@miyadav: This pull request references OCPCLOUD-2992 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 "4.22.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. |
📝 WalkthroughWalkthroughAdds MAPI-specific VAP error message constants, refactors a machine update verifier to accept generic machine types, and extends end-to-end tests to cover MachineAPI-authoritative VAP enforcement (spec, label, annotation) and cross-type update validations. Also adds a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
|
@miyadav: This pull request references OCPCLOUD-2992 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 "4.22.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: 0
🧹 Nitpick comments (1)
e2e/machine_migration_vap_tests.go (1)
408-420: Useclient.Objectparameter to eliminate unnecessary type-switching.
Both*mapiv1beta1.Machineand*clusterv1.Machineimplementclient.Object, andkomega.Update()accepts it directly—allowing you to drop the runtime type assertion.Proposed simplification
-func verifyUpdateAllowed(machine interface{}, updateFunc func()) { +func verifyUpdateAllowed(machine client.Object, updateFunc func()) { By("Verifying that machine update is allowed") - - switch m := machine.(type) { - case *mapiv1beta1.Machine: - Eventually(komega.Update(m, updateFunc), capiframework.WaitMedium, capiframework.RetryMedium).Should(Succeed(), - "Expected MAPI machine update to succeed") - case *clusterv1.Machine: - Eventually(komega.Update(m, updateFunc), capiframework.WaitMedium, capiframework.RetryMedium).Should(Succeed(), - "Expected CAPI machine update to succeed") - default: - Fail(fmt.Sprintf("unknown machine type: %T", machine)) - } + Eventually(komega.Update(machine, updateFunc), capiframework.WaitMedium, capiframework.RetryMedium).Should(Succeed(), + "Expected machine update to succeed") }You'll need to import
clientfrom controller-runtime:"sigs.k8s.io/controller-runtime/pkg/client".
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
e2e/machine_migration_vap_tests.go
🧰 Additional context used
🧬 Code graph analysis (1)
e2e/machine_migration_vap_tests.go (2)
pkg/conversion/mapi2capi/interface.go (1)
Machine(24-26)e2e/framework/framework.go (3)
CAPINamespace(14-14)WaitMedium(24-24)RetryMedium(18-18)
🔇 Additional comments (2)
e2e/machine_migration_vap_tests.go (2)
37-44: The error message constants are correctly sourced from the cluster-api-machine-vap ValidatingAdmissionPolicy defined inmanifests/0000_30_cluster-api_09_admission-policies.yaml. All three messages match the VAP policy exactly:
vapCAPISpecLockedMessage: matches the VAP spec validation messagevapCAPIProtectedLabelMessage: matches the VAP label guard messagevapCAPIProtectedAnnotationMessage: matches the VAP annotation guard messageSince these error messages are controlled by the VAP policy manifest in the same repository and the test assertions already use
ContainSubstring()(which allows for flexibility rather than exact matching), the brittleness risk is manageable. Any drift between the constants and the VAP policy will be caught by test failures, which is the intended behavior.
275-395: Nil-deref concerns in the MAPI-authoritative test suite are overstated.The code follows established patterns in the test suite:
testCAPIMachine lookup:
capiframework.GetMachine()usesEventually(komega.Get(...))with a 1-minute timeout, polling the API. It either succeeds (returning a valid object) or fails the test assertion. The testCAPIMachine pointer cannot be nil after the call succeeds.InfrastructureRef field access: The pattern
testCAPIMachine.Spec.InfrastructureRef.Name = ...is used consistently throughout the codebase without nil guards:
machine_migration_helpers.goline 158machineset_migration_helpers.goline 332- Other existing VAP test cases in the same file
This consistent usage suggests InfrastructureRef is a struct value, not a pointer. If it were a pointer and could be nil, the existing code would already fail.
No changes are needed. The test fixture setup is sound, and the code aligns with established patterns in the suite.
|
@miyadav: This pull request references OCPCLOUD-2992 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 "4.22.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. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @e2e/machine_migration_vap_tests.go:
- Around line 302-306: The test mutates
testCAPIMachine.Spec.InfrastructureRef.Name directly which can nil-pointer panic
if InfrastructureRef is nil; update the test before calling
verifyCAPIUpdatePrevented to either ensure Spec.InfrastructureRef is non-nil
(allocate & set a minimal corev1.ObjectReference) or guard the mutation with an
if testCAPIMachine.Spec.InfrastructureRef == nil {
testCAPIMachine.Spec.InfrastructureRef = &corev1.ObjectReference{ /* minimal
fields */ } } so that assigning to Spec.InfrastructureRef.Name is safe while
still invoking verifyCAPIUpdatePrevented with vapCAPISpecLockedMessage.
🧹 Nitpick comments (1)
e2e/machine_migration_vap_tests.go (1)
403-416: Consider using generics for type safety.Using
interface{}loses compile-time type checking. While the runtime type switch withFail()handles errors appropriately for tests, you could leverage Go generics for stronger compile-time guarantees.♻️ Optional: Type-safe alternative using generics
-func verifyUpdateAllowed(machine interface{}, updateFunc func()) { +func verifyUpdateAllowed[T *mapiv1beta1.Machine | *clusterv1.Machine](machine T, updateFunc func()) { By("Verifying that machine update is allowed") - - switch m := machine.(type) { - case *mapiv1beta1.Machine: - Eventually(komega.Update(m, updateFunc), capiframework.WaitMedium, capiframework.RetryMedium).Should(Succeed(), - "Expected MAPI machine update to succeed") - case *clusterv1.Machine: - Eventually(komega.Update(m, updateFunc), capiframework.WaitMedium, capiframework.RetryMedium).Should(Succeed(), - "Expected CAPI machine update to succeed") - default: - Fail(fmt.Sprintf("unknown machine type: %T", machine)) - } + Eventually(komega.Update(machine, updateFunc), capiframework.WaitMedium, capiframework.RetryMedium).Should(Succeed(), + fmt.Sprintf("Expected %T update to succeed", machine)) }Note: This requires
komega.Updateto accept both types. Ifkomega.Updatehas specific type constraints, keeping the type switch may be necessary.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (2)
.tool-versionse2e/machine_migration_vap_tests.go
✅ Files skipped from review due to trivial changes (1)
- .tool-versions
🧰 Additional context used
🧬 Code graph analysis (1)
e2e/machine_migration_vap_tests.go (3)
pkg/conversion/mapi2capi/interface.go (1)
Machine(24-26)e2e/framework/machine.go (1)
GetMachine(75-86)e2e/framework/framework.go (3)
CAPINamespace(14-14)WaitMedium(24-24)RetryMedium(18-18)
🔇 Additional comments (3)
e2e/machine_migration_vap_tests.go (3)
42-43: Potential typo in annotation error message constant.The label message on line 42 consistently uses the
/*wildcard pattern (cluster.x-k8s.io/*), but the annotation message on line 43 appears to be missing the wildcards:"cluster.x-k8s.io or clusters.x-k8s.io"instead of"cluster.x-k8s.io/* or clusters.x-k8s.io/*".If this matches the actual VAP policy message exactly, please disregard. Otherwise, consider aligning the pattern format.
275-390: LGTM!The new test context for MAPI-authoritative VAP enforcement is well-structured with proper
BeforeAllsetup,DeferCleanupteardown, and comprehensive coverage of spec, label, and annotation restrictions. The test cases appropriately verify both blocked and allowed updates.
288-288: VerifyverifyMachineAuthoritativehelper function exists.This line calls
verifyMachineAuthoritative, but the function is not visible in the provided code. The other test contexts (lines 70-75 and 234-239) use inlineEventuallywithWithTransformassertions instead.#!/bin/bash # Search for the verifyMachineAuthoritative function definition rg -n "func verifyMachineAuthoritative" --type go
| It("should prevent updating spec.infrastructureRef", func() { | ||
| verifyCAPIUpdatePrevented(testCAPIMachine, func() { | ||
| testCAPIMachine.Spec.InfrastructureRef.Name = "new-infra-ref" | ||
| }, vapCAPISpecLockedMessage) | ||
| }) |
There was a problem hiding this comment.
Potential nil pointer dereference on InfrastructureRef.
Accessing testCAPIMachine.Spec.InfrastructureRef.Name directly could cause a nil pointer panic if InfrastructureRef is nil. Consider adding a nil check or ensuring the machine always has this field populated.
🐛 Proposed fix with nil check
It("should prevent updating spec.infrastructureRef", func() {
verifyCAPIUpdatePrevented(testCAPIMachine, func() {
+ if testCAPIMachine.Spec.InfrastructureRef == nil {
+ testCAPIMachine.Spec.InfrastructureRef = &corev1.ObjectReference{}
+ }
testCAPIMachine.Spec.InfrastructureRef.Name = "new-infra-ref"
}, vapCAPISpecLockedMessage)
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| It("should prevent updating spec.infrastructureRef", func() { | |
| verifyCAPIUpdatePrevented(testCAPIMachine, func() { | |
| testCAPIMachine.Spec.InfrastructureRef.Name = "new-infra-ref" | |
| }, vapCAPISpecLockedMessage) | |
| }) | |
| It("should prevent updating spec.infrastructureRef", func() { | |
| verifyCAPIUpdatePrevented(testCAPIMachine, func() { | |
| if testCAPIMachine.Spec.InfrastructureRef == nil { | |
| testCAPIMachine.Spec.InfrastructureRef = &corev1.ObjectReference{} | |
| } | |
| testCAPIMachine.Spec.InfrastructureRef.Name = "new-infra-ref" | |
| }, vapCAPISpecLockedMessage) | |
| }) |
🤖 Prompt for AI Agents
In @e2e/machine_migration_vap_tests.go around lines 302 - 306, The test mutates
testCAPIMachine.Spec.InfrastructureRef.Name directly which can nil-pointer panic
if InfrastructureRef is nil; update the test before calling
verifyCAPIUpdatePrevented to either ensure Spec.InfrastructureRef is non-nil
(allocate & set a minimal corev1.ObjectReference) or guard the mutation with an
if testCAPIMachine.Spec.InfrastructureRef == nil {
testCAPIMachine.Spec.InfrastructureRef = &corev1.ObjectReference{ /* minimal
fields */ } } so that assigning to Spec.InfrastructureRef.Name is safe while
still invoking verifyCAPIUpdatePrevented with vapCAPISpecLockedMessage.
|
@miyadav: 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. |
3 participants
e2e tests for capi machine vap .
@sunzhaohua2 @huali9 PTAL when time permits.
Summary by CodeRabbit
Tests
Refactor
Chores
✏️ Tip: You can customize this high-level summary in your review settings.