OCPCLOUD-2566: tests with VAP msgs #386
Conversation
📝 WalkthroughWalkthroughAdds a new AWS-focused Ginkgo end-to-end test file that implements Verification and Access Policy (VAP) enforcement scenarios for Machine API Migration (MAPI), including MAPI- and CAPI-enforcement suites, protected-field update checks, AWS providerSpec helpers, and a MachineAPI-authority bypass path. (≤50 words) Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant T as Ginkgo Test
participant K as Kubernetes API
participant V as VAP Admission
rect rgb(245,250,255)
Note over T: Setup (AWS env, MAPI migration enabled)
T->>K: Create MAPI Machine (authoritativeAPI=CAPI)
K-->>T: Created
T->>K: Create corresponding CAPI Machine
K-->>T: Created
end
rect rgb(220,235,255)
Note right of V: Enforcement when authoritativeAPI=CAPI
T->>K: Patch protected fields (spec/labels/annotations/providerSpec)
K->>V: AdmissionReview
V-->>K: Deny with VAP error
K-->>T: Update rejected (assert)
end
rect rgb(235,255,235)
Note right of V: Bypass when authoritativeAPI=MachineAPI
T->>K: Patch machine.authoritativeAPI -> MachineAPI
K->>V: AdmissionReview
V-->>K: Allow
K-->>T: Update succeeds (assert)
end
Note over T: Cleanup and restore authoritativeAPI if changed
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
![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 (7)
e2e/machine_migration_vap_tests.go (7)
43-43: Avoidvar _ = Describeinside a Describe; just call Describe/ContextInside a Describe, prefer
Describe(...)orContext(...)(novar _ =). This is cleaner and avoids confusing local var decls.- var _ = Describe("VAP: machine-api-machine-vap enforcement", Ordered, func() { + Describe("VAP: machine-api-machine-vap enforcement", Ordered, func() {
53-54: Make CAPI mirror retrieval resilient
GetMachine(...)may race the mirror creation. Wrap in Eventually (or a helper that waits) to avoid flaky NotFound.Example:
- testCAPIMachine = capiframework.GetMachine(cl, testMachineName, capiframework.CAPINamespace) + Eventually(func() (*clusterv1.Machine, error) { + m := capiframework.GetMachine(cl, testMachineName, capiframework.CAPINamespace) + if m == nil { + return nil, fmt.Errorf("CAPI machine not found yet") + } + return m, nil + }, capiframework.WaitMedium, capiframework.RetryMedium).Should(Not(BeNil())) + testCAPIMachine = capiframework.GetMachine(cl, testMachineName, capiframework.CAPINamespace)Also confirm
capiframework.GetMachinealready waits; if so, this is unnecessary. Please verify.
55-63: Guard cleanup against nilsIf mirror creation fails,
testCAPIMachinecan be nil. EnsurecleanupMachineResourcestolerates nils or guard before passing.
239-256: Unused helpers; either exercise them or remove
verifyNonProtectedLabelUpdateAllowedandverifyNonProtectedAnnotationUpdateAllowedaren’t called. Keep tests lean: add small Its using them or drop to avoid dead code.Also applies to: 297-314
279-295: Deletion test must ensure the annotation exists firstIf the protected annotation isn’t present, deleting it is a no-op and may not trigger VAP. Seed it first, then attempt deletion in a separate update.
func verifyProtectedAnnotationDeletionPrevented(machine *mapiv1beta1.Machine, annotationKey, expectedError string) { By(fmt.Sprintf("Verifying that deleting protected annotation %s is prevented by VAP", annotationKey)) + // Ensure the annotation exists + Eventually(func() error { + if err := cl.Get(ctx, client.ObjectKeyFromObject(machine), machine); err != nil { + return err + } + if machine.Annotations == nil { + machine.Annotations = map[string]string{} + } + if _, ok := machine.Annotations[annotationKey]; !ok { + machine.Annotations[annotationKey] = "seed" + } + return cl.Update(ctx, machine) + }, capiframework.WaitMedium, capiframework.RetryMedium).Should(Succeed()) + Eventually(func() error { if err := cl.Get(ctx, client.ObjectKeyFromObject(machine), machine); err != nil { return err } @@ }, capiframework.WaitMedium, capiframework.RetryMedium).Should( MatchError(ContainSubstring(expectedError)), "Expected protected annotation deletion to be blocked by VAP") }
316-340: Add a test that VAP is not applied when authoritativeAPI=MachineAPIYou wrote
verifyVAPNotAppliedForMachineAPIAuthority()but never call it. Add an It under its own Context to prevent regressions.Example:
@@ - Context("spec field restrictions", func() { + Context("spec field restrictions", func() { @@ }) + Context("when authoritativeAPI is MachineAPI", func() { + It("does not apply VAP to spec updates", func() { + verifyVAPNotAppliedForMachineAPIAuthority() + }) + })Also applies to: 66-86
25-30: Be cautious with exact error text assertionsHard-coding full messages is brittle across policy/message tweaks. You already use ContainSubstring; consider matching a stable fragment (e.g., “modify spec.authoritativeAPI” / “authoritativeAPI is set to Cluster API”) or a regex to reduce churn.
📜 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(1 hunks)
🔇 Additional comments (2)
e2e/machine_migration_vap_tests.go (2)
32-41: AWS + feature-gate guards look goodPlatform guard and MachineAPIMigration gate are correct and reduce flakiness.
66-86: Spec-field restriction assertions are solidProviderID, taints, and spec.metadata checks are clear and use Eventually with substring match on VAP error. Nice.
Optional: After each denial, assert the field remains unchanged to prove no mutation leaked through.
Also applies to: 106-128, 130-171
|
Hi Milind, thanks for helping to check this. I found Theo has some comments in pr #355, can you help to take a look? especially use Komega instead of cl.Update, cl.Get. verifyAnnotationModificationPrevented becomes a more generic call to verifyUpdatePrevented |
|
Just to add , sometimes , machine status is going to migrating and then the changes like annotations are not blocked , but reconciled so functionally it is working , but we won't get test for it to pass, as it is not getting the error message we expect. |
There was a problem hiding this comment.
Actionable comments posted: 8
📜 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(1 hunks)
🔇 Additional comments (2)
e2e/machine_migration_vap_tests.go (2)
220-248: LGTM! Good example of Komega usage.This helper correctly uses
komega.Updateandkomega.Object, demonstrating the pattern that should be followed in other helper functions.
345-363: LGTM! Correct Komega usage.This helper correctly uses
komega.Updateand demonstrates proper cleanup withmapiframework.DeleteMachines.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
e2e/machine_migration_vap_tests.go (1)
169-200: Refactor helper to use Komega (per earlier feedback).Prior review already asked for Komega over raw
cl.Get/cl.Update. Please alignverifyUpdatePrevented/verifyUpdateAllowedwith the project pattern:-func verifyUpdatePrevented(machine *mapiv1beta1.Machine, updateFunc func(*mapiv1beta1.Machine), expectedError string) { +func verifyUpdatePrevented(machine *mapiv1beta1.Machine, updateFunc func(), expectedError string) { @@ - Eventually(func() error { - freshMachine := &mapiv1beta1.Machine{} - if err := cl.Get(ctx, client.ObjectKeyFromObject(machine), freshMachine); err != nil { - return err - } - updateFunc(freshMachine) - return cl.Update(ctx, freshMachine) - }, capiframework.WaitMedium, capiframework.RetryMedium).Should( + Eventually(komega.Update(machine, updateFunc), capiframework.WaitMedium, capiframework.RetryMedium).Should(Apply the same pattern to
verifyUpdateAllowedand update call sites to pass closures without the machine argument.
📜 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(1 hunks)
openshift-ci
bot
added
the
do-not-merge/work-in-progress
miyadav
force-pushed
the
2566vaptests_latest
branch
from
918f2a5 to
f57cd5c
Compare
|
/unhold |
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
e2e/machine_migration_vap_tests.go (3)
3-18: Stop using YAML for providerSpec; switch to encoding/json.YAML bytes in runtime.RawExtension cause JSON encoding to fail before VAP, so tests won’t observe the policy error. Replace the YAML import with encoding/json.
import ( + "encoding/json" "fmt" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" @@ "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/envtest/komega" - yaml "sigs.k8s.io/yaml" )
74-101: Missing coverage: spec.providerSpec update should be blockedAdd a test that mutates spec.providerSpec (e.g., instanceType) and asserts the VAP rejection.
Context("spec field restrictions", func() { It("should prevent updating spec.providerID", func() { @@ }) + It("should prevent updating spec.providerSpec", func() { + verifyAWSProviderSpecUpdatePrevented( + testMAPIMachine, + "instanceType", + testInstanceType, + vapSpecLockedMessage, + ) + }) })
183-255: providerSpec helper marshals with YAML; this breaks the update pathjson is required for RawExtension. Swap yaml.Unmarshal/Marshal to encoding/json to let the Update reach VAP.
- providerSpec := &mapiv1beta1.AWSMachineProviderConfig{} - if err := yaml.Unmarshal(freshMachine.Spec.ProviderSpec.Value.Raw, providerSpec); err != nil { + providerSpec := &mapiv1beta1.AWSMachineProviderConfig{} + if err := json.Unmarshal(freshMachine.Spec.ProviderSpec.Value.Raw, providerSpec); err != nil { return fmt.Errorf("failed to unmarshal providerSpec: %v", err) } @@ - // Marshal back to raw extension - modifiedRaw, err := yaml.Marshal(providerSpec) + // Marshal back to raw extension (JSON) + modifiedRaw, err := json.Marshal(providerSpec) if err != nil { return fmt.Errorf("failed to marshal modified providerSpec: %v", err) } freshMachine.Spec.ProviderSpec.Value = &runtime.RawExtension{Raw: modifiedRaw}
🧹 Nitpick comments (3)
e2e/machine_migration_vap_tests.go (3)
167-259: Prefer Komega here for consistency with other helpersUse komega.Update instead of manual cl.Get/cl.Update for consistency and less boilerplate. Keep JSON marshal/unmarshal as above.
-func verifyAWSProviderSpecUpdatePrevented(machine *mapiv1beta1.Machine, fieldName string, testValue interface{}, expectedError string) { - By(fmt.Sprintf("Verifying that updating AWS providerSpec.%s is prevented by VAP", fieldName)) - - Eventually(func() error { - // Get fresh copy to avoid conflicts - freshMachine := &mapiv1beta1.Machine{} - if err := cl.Get(ctx, client.ObjectKeyFromObject(machine), freshMachine); err != nil { - return err - } - // Parse the current providerSpec - if freshMachine.Spec.ProviderSpec.Value == nil { - return fmt.Errorf("providerSpec is nil") - } - providerSpec := &mapiv1beta1.AWSMachineProviderConfig{} - if err := json.Unmarshal(freshMachine.Spec.ProviderSpec.Value.Raw, providerSpec); err != nil { - return fmt.Errorf("failed to unmarshal providerSpec: %v", err) - } - // ... mutate switch ... - modifiedRaw, err := json.Marshal(providerSpec) - if err != nil { - return fmt.Errorf("failed to marshal modified providerSpec: %v", err) - } - freshMachine.Spec.ProviderSpec.Value = &runtime.RawExtension{Raw: modifiedRaw} - return cl.Update(ctx, freshMachine) - }, capiframework.WaitMedium, capiframework.RetryMedium).Should( - MatchError(ContainSubstring(expectedError)), - "Expected AWS providerSpec.%s update to be blocked by VAP", fieldName) -} +func verifyAWSProviderSpecUpdatePrevented(machine *mapiv1beta1.Machine, fieldName string, testValue interface{}, expectedError string) { + By(fmt.Sprintf("Verifying that updating AWS providerSpec.%s is prevented by VAP", fieldName)) + Eventually(komega.Update(machine, func() { + if machine.Spec.ProviderSpec.Value == nil || len(machine.Spec.ProviderSpec.Value.Raw) == 0 { + return + } + providerSpec := &mapiv1beta1.AWSMachineProviderConfig{} + if err := json.Unmarshal(machine.Spec.ProviderSpec.Value.Raw, providerSpec); err != nil { + return + } + switch fieldName { + case "instanceType": + providerSpec.InstanceType = testValue.(string) + case "amiID": + s := testValue.(string) + if providerSpec.AMI.ID == nil { + providerSpec.AMI.ID = &s + } else { + *providerSpec.AMI.ID = s + } + case "availabilityZone": + providerSpec.Placement.AvailabilityZone = testValue.(string) + case "subnetID": + s := testValue.(string) + providerSpec.Subnet = mapiv1beta1.AWSResourceReference{ID: &s} + case "securityGroups": + s := testValue.(string) + providerSpec.SecurityGroups = []mapiv1beta1.AWSResourceReference{{ID: &s}} + case "volumeSize": + if len(providerSpec.BlockDevices) > 0 && providerSpec.BlockDevices[0].EBS != nil { + v := testValue.(int64) + providerSpec.BlockDevices[0].EBS.VolumeSize = &v + } + case "volumeType": + if len(providerSpec.BlockDevices) > 0 && providerSpec.BlockDevices[0].EBS != nil { + s := testValue.(string) + providerSpec.BlockDevices[0].EBS.VolumeType = &s + } + case "encryption": + if len(providerSpec.BlockDevices) > 0 && providerSpec.BlockDevices[0].EBS != nil { + b := testValue.(bool) + providerSpec.BlockDevices[0].EBS.Encrypted = &b + } + case "tags": + tagMap := testValue.(map[string]string) + var tags []mapiv1beta1.TagSpecification + for k, v := range tagMap { + tags = append(tags, mapiv1beta1.TagSpecification{Name: k, Value: v}) + } + providerSpec.Tags = tags + default: + return + } + if raw, err := json.Marshal(providerSpec); err == nil { + machine.Spec.ProviderSpec.Value.Raw = raw + } + }), capiframework.WaitMedium, capiframework.RetryMedium).Should( + MatchError(ContainSubstring(expectedError)), + fmt.Sprintf("Expected AWS providerSpec.%s update to be blocked by VAP", fieldName), + ) +}
26-33: Unused providerSpec test constantsThese constants are currently unused. Either use them in providerSpec tests or remove to reduce noise.
102-120: Optional: also assert kubernetes.io/ label protection*You only test machine.openshift.io/; consider adding a kubernetes.io/ case for completeness.
It("should prevent modifying kubernetes.io/* labels", func() { verifyUpdatePrevented(testMAPIMachine, func() { if testMAPIMachine.Labels == nil { testMAPIMachine.Labels = make(map[string]string) } testMAPIMachine.Labels["kubernetes.io/test-label"] = testLabelValue }, vapProtectedLabelMessage) })
📜 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(1 hunks)
🔇 Additional comments (1)
e2e/machine_migration_vap_tests.go (1)
150-165: Good use of Komega wrappersverifyUpdatePrevented/Allowed follow the recommended Komega pattern.
|
@miyadav can we have a proper title for the PR? (including |
miyadav
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@miyadav: This pull request references OCPCLOUD-2566 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 either version "4.21." or "openshift-4.21.", but it targets "openshift-4.20" instead. 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. |
|
@miyadav: This pull request references OCPCLOUD-2566 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 either version "4.21." or "openshift-4.21.", but it targets "openshift-4.20" instead. 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
🧹 Nitpick comments (5)
e2e/machine_migration_vap_tests.go (5)
74-100: Add a providerSpec update test to fully exercise spec lock.spec.providerSpec changes are part of VAP enforcement but are not covered here. You already have constants and a helper for this. Add a focused test to prevent regressions.
Apply near this context:
Context("spec field restrictions", func() { It("should prevent updating spec.providerID", func() { verifyUpdatePrevented(testMAPIMachine, func() { providerIDValue := testProviderID testMAPIMachine.Spec.ProviderID = &providerIDValue }, vapSpecLockedMessage) }) + It("should prevent updating spec.providerSpec (e.g., instanceType)", func() { + // Reuses the existing helper and constants + verifyAWSProviderSpecUpdatePrevented(testMAPIMachine, "instanceType", testInstanceType, vapSpecLockedMessage) + }) })
56-72: Optional: wait for status.authoritativeAPI=ClusterAPI to reduce flakes.Enforcement depends on status.authoritativeAPI. A short readiness wait here can make CI steadier without changing behavior.
BeforeAll(func() { // Create a MAPI machine with ClusterAPI authority to trigger VAP enforcement testMAPIMachine = createMAPIMachineWithAuthority(ctx, cl, testMachineName, mapiv1beta1.MachineAuthorityClusterAPI) // The VAP requires a matching CAPI machine as parameter testCAPIMachine = capiframework.GetMachine(cl, testMachineName, capiframework.CAPINamespace) + // Harden: wait until enforcement condition is actually met + Eventually(func() (mapiv1beta1.MachineAuthority, error) { + fresh := &mapiv1beta1.Machine{} + err := cl.Get(ctx, client.ObjectKeyFromObject(testMAPIMachine), fresh) + return fresh.Status.AuthoritativeAPI, err + }, capiframework.WaitLong, capiframework.RetryMedium).Should( + Equal(mapiv1beta1.MachineAuthorityClusterAPI), + "expect status.authoritativeAPI=ClusterAPI before enforcement checks", + )
167-259: ProviderSpec helper: avoid silent no-ops and align with Komega.
- Today, several cases can end up not modifying anything (e.g., missing BlockDevices/EBS) yet proceed to Update, risking misleading outcomes if used later.
- Style: the rest of the file uses komega.Update; consider the same for consistency.
Minimal robustness tweak (keep cl.Get/Update) — ensure a real mutation or fail fast:
func verifyAWSProviderSpecUpdatePrevented(machine *mapiv1beta1.Machine, fieldName string, testValue interface{}, expectedError string) { By(fmt.Sprintf("Verifying that updating AWS providerSpec.%s is prevented by VAP", fieldName)) Eventually(func() error { @@ - // Modify the specified field + // Modify the specified field + modified := false switch fieldName { case "instanceType": providerSpec.InstanceType = testValue.(string) + modified = true case "amiID": testValueStr := testValue.(string) if providerSpec.AMI.ID == nil { providerSpec.AMI.ID = &testValueStr } else { *providerSpec.AMI.ID = testValueStr } + modified = true case "availabilityZone": providerSpec.Placement.AvailabilityZone = testValue.(string) + modified = true case "subnetID": testValueStr := testValue.(string) providerSpec.Subnet = mapiv1beta1.AWSResourceReference{ ID: &testValueStr, } + modified = true case "securityGroups": providerSpec.SecurityGroups = []mapiv1beta1.AWSResourceReference{{ ID: &[]string{testValue.(string)}[0], }} + modified = true case "volumeSize": if len(providerSpec.BlockDevices) > 0 && providerSpec.BlockDevices[0].EBS != nil { providerSpec.BlockDevices[0].EBS.VolumeSize = &[]int64{testValue.(int64)}[0] + modified = true } case "volumeType": if len(providerSpec.BlockDevices) > 0 && providerSpec.BlockDevices[0].EBS != nil { providerSpec.BlockDevices[0].EBS.VolumeType = &[]string{testValue.(string)}[0] + modified = true } case "encryption": if len(providerSpec.BlockDevices) > 0 && providerSpec.BlockDevices[0].EBS != nil { providerSpec.BlockDevices[0].EBS.Encrypted = &[]bool{testValue.(bool)}[0] + modified = true } case "tags": @@ providerSpec.Tags = tags + modified = true - case "capacityReservationId": - providerSpec.SpotMarketOptions = &mapiv1beta1.SpotMarketOptions{ - MaxPrice: &[]string{"0.10"}[0], - } - // Note: capacityReservationId might be in different location based on AWS API version default: return fmt.Errorf("unsupported field: %s", fieldName) } + + if !modified { + return fmt.Errorf("no-op mutation for field %q (missing nested fields?)", fieldName) + }If you prefer full consistency, I can also rewrite this helper to use komega.Update and return a komega closure while still surfacing parse/mutation errors. Want me to push that version?
20-38: Prune or use the extra providerSpec constants.testInstanceType, testAMIID, testAvailabilityZone, testSubnetID, testSecurityGroupID, testVolumeSize, and testCapacityReservationID are currently unused in tests. Either use them (e.g., in the providerSpec test) or remove to keep the file lean.
261-280: Optional: avoid name collisions for the MachineAPI-authority test.To be safer under parallel CI or repeated runs, add a unique suffix.
- testMachine := createMAPIMachineWithAuthority(ctx, cl, "vap-test-mapi-authority", mapiv1beta1.MachineAuthorityMachineAPI) + uniqueName := fmt.Sprintf("vap-test-mapi-authority-%d", GinkgoParallelProcess()) + testMachine := createMAPIMachineWithAuthority(ctx, cl, uniqueName, mapiv1beta1.MachineAuthorityMachineAPI)
📜 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(1 hunks)
🔇 Additional comments (3)
e2e/machine_migration_vap_tests.go (3)
102-120: Protected labels: good coverage.Blocking machine.openshift.io/* while allowing non-protected labels is clearly tested.
122-140: Protected annotations: good coverage.Denial for machine.openshift.io/* and allow-list for others looks correct.
150-165: Komega-based update helpers look solid.Nice switch to komega.Update with bounded waits/retries; assertions read well.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
e2e/machine_migration_vap_tests.go (2)
26-33: Build blocker: unused constants cause compile failureThese constants are never referenced, so
go testwill fail with “declared and not used”. Either use them in tests or remove them.Apply this minimal fix (keep only those actually used today and the one we’ll use in the providerSpec test below):
const ( // Test values for MAPI machine updates testProviderID = "aws:///us-west-2a/i-test123456" testTaintValue = "test-taint-value" testLabelValue = "test-label-value" testInstanceType = "m5.xlarge" - testAMIID = "ami-test123456" - testAvailabilityZone = "us-west-2b" - testSubnetID = "subnet-test123456" - testSecurityGroupID = "sg-test123456" - testVolumeSize = int64(120) - testCapacityReservationID = "cr-test123456"
74-100: Missing providerSpec coverage — add a focused testVAP should block providerSpec edits, but there’s no test exercising it. Use the existing helper.
Context("spec field restrictions", func() { @@ It("should prevent updating spec.metadata", func() { verifyUpdatePrevented(testMAPIMachine, func() { if testMAPIMachine.Spec.ObjectMeta.Labels == nil { testMAPIMachine.Spec.ObjectMeta.Labels = make(map[string]string) } testMAPIMachine.Spec.ObjectMeta.Labels["test-spec-label"] = testLabelValue }, vapSpecLockedMessage) }) + + It("should prevent updating spec.providerSpec (instanceType)", func() { + verifyAWSProviderSpecUpdatePrevented( + testMAPIMachine, + "instanceType", + testInstanceType, + vapSpecLockedMessage, + ) + }) })
🧹 Nitpick comments (5)
e2e/machine_migration_vap_tests.go (5)
171-176: Use Komega here for consistency with the rest of the suiteThis helper still uses raw client Get/Update. Prefer Komega like your other helpers.
func verifyAWSProviderSpecUpdatePrevented(machine *mapiv1beta1.Machine, fieldName string, testValue interface{}, expectedError string) { By(fmt.Sprintf("Verifying that updating AWS providerSpec.%s is prevented by VAP", fieldName)) - - Eventually(func() error { - // Get fresh copy to avoid conflicts - freshMachine := &mapiv1beta1.Machine{} - if err := cl.Get(ctx, client.ObjectKeyFromObject(machine), freshMachine); err != nil { - return err - } + Eventually(komega.Update(machine, func() { + freshMachine := machine @@ - freshMachine.Spec.ProviderSpec.Value = &runtime.RawExtension{Raw: modifiedRaw} - return cl.Update(ctx, freshMachine) - }, capiframework.WaitMedium, capiframework.RetryMedium).Should( + freshMachine.Spec.ProviderSpec.Value = &runtime.RawExtension{Raw: modifiedRaw} + }), capiframework.WaitMedium, capiframework.RetryMedium).Should( MatchError(ContainSubstring(expectedError)), "Expected AWS providerSpec.%s update to be blocked by VAP", fieldName) }Also remove the now-unused import "sigs.k8s.io/controller-runtime/pkg/client".
Also applies to: 251-253
211-227: Guard against no-op when BlockDevices/EBS is emptyIf BlockDevices is empty or EBS is nil, the mutation won’t change anything, risking false negatives.
- case "volumeSize": - if len(providerSpec.BlockDevices) > 0 { - if providerSpec.BlockDevices[0].EBS != nil { - providerSpec.BlockDevices[0].EBS.VolumeSize = &[]int64{testValue.(int64)}[0] - } - } + case "volumeSize": + if len(providerSpec.BlockDevices) == 0 { + providerSpec.BlockDevices = []mapiv1beta1.BlockDeviceMappingSpec{{EBS: &mapiv1beta1.EBSBlockDeviceSpec{}}} + } else if providerSpec.BlockDevices[0].EBS == nil { + providerSpec.BlockDevices[0].EBS = &mapiv1beta1.EBSBlockDeviceSpec{} + } + providerSpec.BlockDevices[0].EBS.VolumeSize = &[]int64{testValue.(int64)}[0] @@ - case "volumeType": - if len(providerSpec.BlockDevices) > 0 { - if providerSpec.BlockDevices[0].EBS != nil { - providerSpec.BlockDevices[0].EBS.VolumeType = &[]string{testValue.(string)}[0] - } - } + case "volumeType": + if len(providerSpec.BlockDevices) == 0 { + providerSpec.BlockDevices = []mapiv1beta1.BlockDeviceMappingSpec{{EBS: &mapiv1beta1.EBSBlockDeviceSpec{}}} + } else if providerSpec.BlockDevices[0].EBS == nil { + providerSpec.BlockDevices[0].EBS = &mapiv1beta1.EBSBlockDeviceSpec{} + } + providerSpec.BlockDevices[0].EBS.VolumeType = &[]string{testValue.(string)}[0] @@ - case "encryption": - if len(providerSpec.BlockDevices) > 0 { - if providerSpec.BlockDevices[0].EBS != nil { - providerSpec.BlockDevices[0].EBS.Encrypted = &[]bool{testValue.(bool)}[0] - } - } + case "encryption": + if len(providerSpec.BlockDevices) == 0 { + providerSpec.BlockDevices = []mapiv1beta1.BlockDeviceMappingSpec{{EBS: &mapiv1beta1.EBSBlockDeviceSpec{}}} + } else if providerSpec.BlockDevices[0].EBS == nil { + providerSpec.BlockDevices[0].EBS = &mapiv1beta1.EBSBlockDeviceSpec{} + } + providerSpec.BlockDevices[0].EBS.Encrypted = &[]bool{testValue.(bool)}[0]
102-120: Add coverage for kubernetes.io/ protected labels*Your message asserts both machine.openshift.io/* and kubernetes.io/* are protected; only the former is tested.
Context("protected label restrictions", func() { @@ It("should allow modifying non-protected labels", func() { verifyUpdateAllowed(testMAPIMachine, func() { if testMAPIMachine.Labels == nil { testMAPIMachine.Labels = make(map[string]string) } testMAPIMachine.Labels["test-label"] = "allowed-value" }) }) + + It("should prevent modifying kubernetes.io/* labels", func() { + verifyUpdatePrevented(testMAPIMachine, func() { + if testMAPIMachine.Labels == nil { + testMAPIMachine.Labels = make(map[string]string) + } + testMAPIMachine.Labels["kubernetes.io/test-label"] = testLabelValue + }, vapProtectedLabelMessage) + }) })
207-209: Minor: avoid ephemeral pointer trick for stringsPrefer a helper for clarity, e.g., k8s.io/utils/ptr.
- providerSpec.SecurityGroups = []mapiv1beta1.AWSResourceReference{{ - ID: &[]string{testValue.(string)}[0], - }} + sg := testValue.(string) + providerSpec.SecurityGroups = []mapiv1beta1.AWSResourceReference{{ID: &sg}}
56-72: Optional: wait for authoritativeAPI readiness to de-flakeIf you hit flakes, explicitly wait until status.authoritativeAPI == ClusterAPI before assertions.
BeforeAll(func() { @@ testCAPIMachine = capiframework.GetMachine(cl, testMachineName, capiframework.CAPINamespace) + + Eventually(func() (mapiv1beta1.MachineAuthority, error) { + fresh := &mapiv1beta1.Machine{} + err := cl.Get(ctx, client.ObjectKeyFromObject(testMAPIMachine), fresh) + return fresh.Status.AuthoritativeAPI, err + }, capiframework.WaitLong, capiframework.RetryMedium).Should( + Equal(mapiv1beta1.MachineAuthorityClusterAPI), + "VAP enforces only after status.authoritativeAPI=ClusterAPI", + )
📜 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(1 hunks)
🔇 Additional comments (1)
e2e/machine_migration_vap_tests.go (1)
239-241: capacityReservationId mutation looks correct nowThis fixes the earlier mismatch (was mutating Spot options). No further action.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
e2e/machine_migration_vap_tests.go (1)
238-241: Simplify the pointer creation for SecurityGroups ID.The pattern
&[]string{testValue.(string)}[0]creates a slice just to take a pointer to its first element, which is unnecessarily complex and harder to read.Apply this diff to improve clarity:
case "securityGroups": + id := testValue.(string) providerSpec.SecurityGroups = []mapiv1beta1.AWSResourceReference{{ - ID: &[]string{testValue.(string)}[0], + ID: &id, }}
📜 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(1 hunks)
|
/test okd-scos-e2e-aws-ovn |
1 similar comment
|
/test okd-scos-e2e-aws-ovn |
e2e/machine_migration_vap_tests.go
Outdated
| return fmt.Errorf("providerSpec is nil") | ||
| } | ||
|
|
||
| providerSpec := &mapiv1beta1.AWSMachineProviderConfig{} |
There was a problem hiding this comment.
do we have a helperfunc elsewhere that does something very similar to this? I think we added one in one of the other PRs?
(specifically one with an updateFunc() being passed in pattern)
There was a problem hiding this comment.
remember now did it in pr 355. Will update
miyadav
force-pushed
the
2566vaptests_latest
branch
from
3f188cb to
b01f005
Compare
|
@miyadav: This pull request references OCPCLOUD-2566 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 either version "4.21." or "openshift-4.21.", but it targets "openshift-4.20" instead. 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. |
openshift-ci
bot
added
the
do-not-merge/hold
|
/retest-required |
|
/unhold |
openshift-ci
bot
removed
the
do-not-merge/hold
|
@damdo @theobarberbany I will rebase this and add a small test scenario to it , seems like the unit test was updated in master , this branch(my PR) doesn't have it. Also some imports changed ( clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" to "clusterv1 "sigs.k8s.io/cluster-api/api/core/v1beta2") , hence it is failing. |
removing the part which is covered in other PRs co-author: cursor & coderabbitAI
reduce time,changes to follow consistent pattern
miyadav
force-pushed
the
2566vaptests_latest
branch
from
ebde40b to
8b854d4
Compare
openshift-ci-robot
removed
the
verified
|
@miyadav: This pull request references OCPCLOUD-2566 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 either version "4.22." or "openshift-4.22.", but it targets "openshift-4.20" instead. 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: 0
🧹 Nitpick comments (1)
e2e/machine_migration_vap_tests.go (1)
52-56: Consider removingvar _ =from nested Describe blocks.The
var _ = Describe(...)pattern is typically used at the top level to ensure test registration during init. Inside a parentDescribeblock, the nestedDescribeis just a function call, sovar _ =is unnecessary.♻️ Suggested simplification
- var _ = Describe("VAP: machine-api-machine-vap enforcement", Ordered, func() { + Describe("VAP: machine-api-machine-vap enforcement", Ordered, func() {Same applies to line 216.
📜 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 (4)
e2e/framework/util.go (1)
IsMachineAPIMigrationEnabled(41-76)pkg/conversion/mapi2capi/interface.go (1)
Machine(24-26)e2e/framework/machine.go (2)
GetMachine(75-86)DeleteMachines(105-135)e2e/framework/framework.go (3)
CAPINamespace(14-14)WaitMedium(24-24)RetryMedium(18-18)
🔇 Additional comments (11)
e2e/machine_migration_vap_tests.go (11)
3-39: LGTM - Imports and constants are well-organized.The use of
encoding/jsonfor providerSpec marshaling is correct (addresses previous YAML-related concerns). Constants are appropriately defined for VAP error messages and test values.
41-51: LGTM - Proper test gating.The platform and feature gate checks appropriately skip tests on non-AWS platforms and clusters without MachineAPIMigration enabled.
57-81: LGTM - Proper test setup with status synchronization.The wait for
status.authoritativeAPIbefore running VAP enforcement tests ensures the VAP match conditions are met, preventing flakiness. Cleanup is properly registered withDeferCleanup.
83-109: LGTM - Spec field restriction tests are comprehensive.Tests appropriately verify that
spec.providerID,spec.taints, andspec.metadataupdates are blocked by VAP when authoritativeAPI is ClusterAPI.
111-149: LGTM - Label and annotation restriction tests provide good coverage.Tests appropriately verify that
machine.openshift.io/*labels and annotations are blocked while non-protected labels and annotations are allowed.
151-207: LGTM - AWS providerSpec field restriction tests are thorough.Tests cover key AWS-specific fields including instanceType, AMI, availability zone, subnet, security groups, tags, and capacity reservation. The
capacityReservationIdtest now correctly modifiesproviderSpec.CapacityReservationID.
216-268: LGTM - CAPI Machine VAP enforcement tests are well-structured.Tests appropriately verify that forbidden CAPI fields (
spec.versionandspec.readinessGates) are blocked by the CAPI VAP. ThereadinessGatestest correctly handles both modification and addition scenarios.
271-295: LGTM - Helper functions use Komega correctly.The
verifyUpdatePrevented,verifyUpdateAllowed, andverifyCAPIUpdatePreventedhelpers appropriately usekomega.Updatewith the closure pattern, addressing previous review feedback.
297-321: LGTM - AWS providerSpec helpers use JSON correctly.The use of
encoding/jsonfor marshaling/unmarshaling the providerSpec addresses the previous YAML-related issue. ThegetAWSProviderSpecFromMachinehelper appropriately uses Gomega assertions for validation.
323-332: LGTM - AWS providerSpec update verification helper is well-designed.The nested
Expect(updateAWSMachineProviderSpec(...))inside thekomega.Updateclosure correctly propagates any marshaling failures to theEventuallyassertion.
334-361: LGTM - VAP bypass verification is correctly implemented.The function appropriately waits for
status.authoritativeAPI=MachineAPIbefore testing, addressing previous flakiness concerns. The use ofmapiframework.DeleteMachinesinstead ofcleanupMachineResourcesis correct here since no corresponding CAPI machine is created when authority is MachineAPI.
|
/retest |
|
/test ci/prow/okd-scos-images |
|
@damdo: The specified target(s) for The following commands are available to trigger optional jobs: Use 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 kubernetes-sigs/prow repository. |
|
/override ci/prow/okd-scos-images |
|
/verified by e2es in CI |
|
@damdo: Overrode contexts on behalf of damdo: ci/prow/okd-scos-images 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 kubernetes-sigs/prow repository. |
openshift-ci-robot
added
the
verified
|
@damdo: This PR has been marked as verified by 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. |
|
Scheduling tests matching the |
|
/retest |
|
@miyadav: The following tests 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. |
6 participants
@sunzhaohua2 @huali9 PTAL when time permits , I have to omit few cases as error messages were not there for them , they actually get reconciled to old values , example labels of machinetype even after changing.
Also , my apologies the PR - #355 has been opened so long , even I forgot changes so , I took the rebase of latest code and recreated this one.
I will close that one.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.