From 594ac9a0c900d36d372fa25460d55ef11069beeb Mon Sep 17 00:00:00 2001 From: Milind Yadav Date: Fri, 12 Sep 2025 17:10:48 +0530 Subject: [PATCH 1/2] OCPCLOUD-2714: e2e test for vap --- e2e/machine_migration_test.go | 80 +++++++++++++++++++++++++++++++++++ 1 file changed, 80 insertions(+) diff --git a/e2e/machine_migration_test.go b/e2e/machine_migration_test.go index 5ac807f45..77bd10f4d 100644 --- a/e2e/machine_migration_test.go +++ b/e2e/machine_migration_test.go @@ -3,6 +3,7 @@ package e2e import ( "context" "fmt" + "strings" "time" . "github.com/onsi/ginkgo/v2" @@ -114,6 +115,27 @@ var _ = Describe("[sig-cluster-lifecycle][OCPFeatureGate:MachineAPIMigration] Ma }) }) + var _ = Describe("VAP Validation Tests Core", Ordered, func() { + Context("CAPI Machine field validation", func() { + It("should prevent setting of capi fields that are not supported by mapi(version)", func() { + verifyCAPIMachineUpdateBlocked(cl, func(capiMachine *clusterv1.Machine) { + version := "v1" + capiMachine.Spec.Version = &version + }, "spec.version") + }) + + It("should prevent setting of capi fields that are not supported by mapi(readinessGates)", func() { + verifyCAPIMachineUpdateBlocked(cl, func(capiMachine *clusterv1.Machine) { + capiMachine.Spec.ReadinessGates = []clusterv1.MachineReadinessGate{ + { + ConditionType: "READY", + }, + } + }, "spec.readinessGates") + }) + }) + }) + var _ = Describe("Deleting MAPI/CAPI Machines", Ordered, func() { var mapiMachineAuthCAPINameDeletion = "machine-authoritativeapi-capi-deletion" var mapiMachineAuthMAPINameDeleteMAPIMachine = "machine-authoritativeapi-mapi-delete-mapi" @@ -458,6 +480,64 @@ func verifyCAPIMachinePausedCondition(capiMachine *clusterv1.Machine, authority ) } +func verifyCAPIMachineUpdateBlocked(cl client.Client, updateFunc func(*clusterv1.Machine), fieldName string) { + By(fmt.Sprintf("Attempting to update CAPI Machine %s field", fieldName)) + + // Get any existing CAPI machine from the namespace + capiMachineList, err := capiframework.GetMachines(cl) + Expect(err).NotTo(HaveOccurred(), "Failed to list CAPI machines") + Expect(capiMachineList).NotTo(BeEmpty(), "No CAPI machines found in the openshift-cluster-api namespace") + + // Use the first machine from the list + existingMachine := capiMachineList[0] + By(fmt.Sprintf("Using existing CAPI machine %s for VAP testing", existingMachine.Name)) + + // Store original machine state for cleanup + originalMachine, err := capiframework.GetMachine(cl, existingMachine.Name, capiframework.CAPINamespace) + Expect(err).NotTo(HaveOccurred(), "Failed to get original CAPI machine") + + // Ensure cleanup happens even if test fails + defer func() { + By("Restoring original machine state") + // Get current machine state + currentMachine, err := capiframework.GetMachine(cl, existingMachine.Name, capiframework.CAPINamespace) + if err != nil { + // If machine doesn't exist, that's fine - no cleanup needed + return + } + + // Restore original spec + currentMachine.Spec = originalMachine.Spec + + // Update back to original state + err = cl.Update(ctx, currentMachine) + if err != nil { + // Log the error but don't fail the test + fmt.Printf("Warning: Failed to restore original machine state: %v\n", err) + } + }() + + // Get the current CAPI machine + capiMachine, err := capiframework.GetMachine(cl, existingMachine.Name, capiframework.CAPINamespace) + Expect(err).NotTo(HaveOccurred(), "Failed to get CAPI machine") + + // Apply the update function + updateFunc(capiMachine) + + // Try to update the machine - this should fail due to VAP + err = cl.Update(ctx, capiMachine) + Expect(err).To(HaveOccurred(), "Update should fail due to VAP") + + // The VAP is working correctly - verify the error message contains the expected VAP policy name + errorMsg := fmt.Sprintf("%v", err) + Expect(strings.Contains(errorMsg, "openshift-cluster-api-prevent-setting-of-capi-fields-unsupported-by-mapi")).To(BeTrue(), + "Error message should contain VAP policy name: %s", errorMsg) + + // Also check that the error message contains "forbidden" and "denied request" + Expect(strings.Contains(errorMsg, "forbidden")).To(BeTrue(), "Error message should contain 'forbidden': %s", errorMsg) + Expect(strings.Contains(errorMsg, "denied request")).To(BeTrue(), "Error message should contain 'denied request': %s", errorMsg) +} + func verifyCAPIMachineRemoved(cl client.Client, machineName string) { By(fmt.Sprintf("Verifying the CAPI Machine %s is removed", machineName)) Eventually(komega.Get(&clusterv1.Machine{ From 8e3a1e00f497b2ab9238595e6b3c70fedfd1e3e8 Mon Sep 17 00:00:00 2001 From: Milind Yadav Date: Mon, 15 Sep 2025 09:58:52 +0530 Subject: [PATCH 2/2] Committed coderabbit suggestions on making blockupdate function more reliable , using api status as a validation as well --- e2e/machine_migration_test.go | 108 ++++++++++++++++++++-------------- 1 file changed, 65 insertions(+), 43 deletions(-) diff --git a/e2e/machine_migration_test.go b/e2e/machine_migration_test.go index 77bd10f4d..8fbe2af7d 100644 --- a/e2e/machine_migration_test.go +++ b/e2e/machine_migration_test.go @@ -118,14 +118,26 @@ var _ = Describe("[sig-cluster-lifecycle][OCPFeatureGate:MachineAPIMigration] Ma var _ = Describe("VAP Validation Tests Core", Ordered, func() { Context("CAPI Machine field validation", func() { It("should prevent setting of capi fields that are not supported by mapi(version)", func() { - verifyCAPIMachineUpdateBlocked(cl, func(capiMachine *clusterv1.Machine) { + // Get any existing CAPI machine for VAP testing + capiMachineList, err := capiframework.GetMachines(cl) + Expect(err).NotTo(HaveOccurred(), "Failed to list CAPI machines") + Expect(capiMachineList).NotTo(BeEmpty(), "No CAPI machines found in the openshift-cluster-api namespace") + + targetMachineName := capiMachineList[0].Name + verifyCAPIMachineUpdateBlocked(ctx, cl, targetMachineName, func(capiMachine *clusterv1.Machine) { version := "v1" capiMachine.Spec.Version = &version }, "spec.version") }) It("should prevent setting of capi fields that are not supported by mapi(readinessGates)", func() { - verifyCAPIMachineUpdateBlocked(cl, func(capiMachine *clusterv1.Machine) { + // Get any existing CAPI machine for VAP testing + capiMachineList, err := capiframework.GetMachines(cl) + Expect(err).NotTo(HaveOccurred(), "Failed to list CAPI machines") + Expect(capiMachineList).NotTo(BeEmpty(), "No CAPI machines found in the openshift-cluster-api namespace") + + targetMachineName := capiMachineList[0].Name + verifyCAPIMachineUpdateBlocked(ctx, cl, targetMachineName, func(capiMachine *clusterv1.Machine) { capiMachine.Spec.ReadinessGates = []clusterv1.MachineReadinessGate{ { ConditionType: "READY", @@ -480,64 +492,74 @@ func verifyCAPIMachinePausedCondition(capiMachine *clusterv1.Machine, authority ) } -func verifyCAPIMachineUpdateBlocked(cl client.Client, updateFunc func(*clusterv1.Machine), fieldName string) { - By(fmt.Sprintf("Attempting to update CAPI Machine %s field", fieldName)) - - // Get any existing CAPI machine from the namespace - capiMachineList, err := capiframework.GetMachines(cl) - Expect(err).NotTo(HaveOccurred(), "Failed to list CAPI machines") - Expect(capiMachineList).NotTo(BeEmpty(), "No CAPI machines found in the openshift-cluster-api namespace") +func verifyCAPIMachineUpdateBlocked(ctx context.Context, cl client.Client, machineName string, updateFunc func(*clusterv1.Machine), fieldName string) { + By(fmt.Sprintf("Attempting to update CAPI Machine %s field on machine %s", fieldName, machineName)) - // Use the first machine from the list - existingMachine := capiMachineList[0] - By(fmt.Sprintf("Using existing CAPI machine %s for VAP testing", existingMachine.Name)) - - // Store original machine state for cleanup - originalMachine, err := capiframework.GetMachine(cl, existingMachine.Name, capiframework.CAPINamespace) - Expect(err).NotTo(HaveOccurred(), "Failed to get original CAPI machine") + // Get the target machine + originalMachine, err := capiframework.GetMachine(cl, machineName, capiframework.CAPINamespace) + Expect(err).NotTo(HaveOccurred(), "Failed to get original CAPI machine %s", machineName) // Ensure cleanup happens even if test fails defer func() { By("Restoring original machine state") - // Get current machine state - currentMachine, err := capiframework.GetMachine(cl, existingMachine.Name, capiframework.CAPINamespace) - if err != nil { - // If machine doesn't exist, that's fine - no cleanup needed - return - } + // Retry-on-conflict loop for restoring original spec + for { + // Get current machine state + currentMachine, err := capiframework.GetMachine(cl, machineName, capiframework.CAPINamespace) + if err != nil { + // If machine doesn't exist, that's fine - no cleanup needed + GinkgoWriter.Printf("Warning: Machine %s no longer exists, skipping cleanup\n", machineName) + return + } - // Restore original spec - currentMachine.Spec = originalMachine.Spec + // Create a patch to restore original spec + patch := client.MergeFrom(currentMachine.DeepCopy()) + currentMachine.Spec = originalMachine.Spec - // Update back to original state - err = cl.Update(ctx, currentMachine) - if err != nil { - // Log the error but don't fail the test - fmt.Printf("Warning: Failed to restore original machine state: %v\n", err) + // Apply the patch + err = cl.Patch(ctx, currentMachine, patch) + if err == nil { + // Success - cleanup complete + break + } + + // Check if it's a conflict error + if apierrors.IsConflict(err) { + GinkgoWriter.Printf("Warning: Conflict restoring machine %s, retrying: %v\n", machineName, err) + // Continue the loop to retry + continue + } + + // For other errors, log and give up + GinkgoWriter.Printf("Warning: Failed to restore original machine state for %s: %v\n", machineName, err) + break } }() // Get the current CAPI machine - capiMachine, err := capiframework.GetMachine(cl, existingMachine.Name, capiframework.CAPINamespace) - Expect(err).NotTo(HaveOccurred(), "Failed to get CAPI machine") + capiMachine, err := capiframework.GetMachine(cl, machineName, capiframework.CAPINamespace) + Expect(err).NotTo(HaveOccurred(), "Failed to get CAPI machine %s", machineName) - // Apply the update function - updateFunc(capiMachine) + // Create a copy for modification + modifiedMachine := capiMachine.DeepCopy() - // Try to update the machine - this should fail due to VAP - err = cl.Update(ctx, capiMachine) - Expect(err).To(HaveOccurred(), "Update should fail due to VAP") + // Apply the update function to the copy + updateFunc(modifiedMachine) - // The VAP is working correctly - verify the error message contains the expected VAP policy name + // Create a patch from the original to the modified version + patch := client.MergeFrom(capiMachine) + + // Try to patch the machine with the modified version - this should fail due to VAP + err = cl.Patch(ctx, modifiedMachine, patch) + Expect(err).To(HaveOccurred(), "Patch should fail due to VAP") + + // The VAP is working correctly - verify the error is Invalid and contains the expected VAP policy name errorMsg := fmt.Sprintf("%v", err) + Expect(apierrors.IsInvalid(err)).To(BeTrue(), "Expected invalid error from VAP, got: %s", errorMsg) Expect(strings.Contains(errorMsg, "openshift-cluster-api-prevent-setting-of-capi-fields-unsupported-by-mapi")).To(BeTrue(), - "Error message should contain VAP policy name: %s", errorMsg) - - // Also check that the error message contains "forbidden" and "denied request" - Expect(strings.Contains(errorMsg, "forbidden")).To(BeTrue(), "Error message should contain 'forbidden': %s", errorMsg) - Expect(strings.Contains(errorMsg, "denied request")).To(BeTrue(), "Error message should contain 'denied request': %s", errorMsg) + "Error message should contain the VAP policy name: %s", errorMsg) + Expect(strings.Contains(strings.ToLower(errorMsg), "denied")).To(BeTrue(), "Error message should contain 'denied': %s", errorMsg) } - func verifyCAPIMachineRemoved(cl client.Client, machineName string) { By(fmt.Sprintf("Verifying the CAPI Machine %s is removed", machineName)) Eventually(komega.Get(&clusterv1.Machine{