OCPCLOUD-2714,OCPCLOUD-2807: e2e test for vap #360
Conversation
|
Pipeline controller notification |
openshift-ci-robot
added
the
jira/valid-reference
|
@miyadav: This pull request references OCPCLOUD-2714 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.21.0" version, but no target version was set. In 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. |
WalkthroughAdds a new e2e test group "VAP Validation Tests Core" that verifies VAP blocks CAPI Machine updates to fields unsupported by MAPI; introduces helper verifyCAPIMachineUpdateBlocked to mutate, assert update denial (checking policy name and keywords), restores original machine state, and adds a strings import. Tests placed before the deletion tests. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Tester as e2e Test
participant K8s as API Server
participant VAP as Validation Admission Policy
Tester->>K8s: GET CAPI Machine
note right of Tester: Mutate fields (e.g., spec.version, spec.readinessGates)
Tester->>K8s: UPDATE CAPI Machine
K8s->>VAP: AdmissionReview (update)
VAP-->>K8s: Deny (policy: openshift-cluster-api-prevent-setting-of-capi-fields-unsupported-by-mapi, contains "forbidden"/"denied request")
K8s-->>Tester: Update rejected (error)
rect rgb(245,250,245)
note over Tester: Deferred cleanup
Tester->>K8s: Restore original Machine state
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration 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)
🚧 Files skipped from review as they are similar to previous changes (1)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
|
|
[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.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@miyadav: This pull request references OCPCLOUD-2714 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.21.0" version, but no target version was set. In 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: 3
🧹 Nitpick comments (1)
e2e/machine_migration_test.go (1)
118-137: Optional: consolidate the two test cases into a table for less duplication.Use DescribeTable with entries for spec.version and spec.readinessGates to reduce boilerplate.
📜 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_test.go(3 hunks)
🔇 Additional comments (1)
e2e/machine_migration_test.go (1)
6-6: LGTM on import.strings is correctly added for message assertions.
| 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") | ||
| }) | ||
| }) | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Do not mutate an arbitrary, live CAPI Machine; create a dedicated test Machine.
Selecting the first machine from the cluster can alter production-like nodes if the VAP isn’t present yet (as noted, tests intentionally fail pre-merge). Make this block hermetic by creating its own CAPI Machine and passing it to the helper.
Apply:
var _ = Describe("VAP Validation Tests Core", Ordered, func() {
- Context("CAPI Machine field validation", func() {
+ var vapTarget *clusterv1.Machine
+ BeforeAll(func() {
+ name := fmt.Sprintf("vap-validate-%d", time.Now().UnixNano())
+ vapTarget = createCAPIMachine(ctx, cl, name)
+ DeferCleanup(func() {
+ cleanupMachineResources(ctx, cl, []*clusterv1.Machine{vapTarget}, []*machinev1beta1.Machine{})
+ })
+ })
+ 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) {
+ verifyCAPIMachineUpdateBlocked(ctx, cl, vapTarget.Name, 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) {
+ verifyCAPIMachineUpdateBlocked(ctx, cl, vapTarget.Name, func(capiMachine *clusterv1.Machine) {
capiMachine.Spec.ReadinessGates = []clusterv1.MachineReadinessGate{
{
ConditionType: "READY",
},
}
}, "spec.readinessGates")
})
})
})📝 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.
| 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("VAP Validation Tests Core", Ordered, func() { | |
| var vapTarget *clusterv1.Machine | |
| BeforeAll(func() { | |
| name := fmt.Sprintf("vap-validate-%d", time.Now().UnixNano()) | |
| vapTarget = createCAPIMachine(ctx, cl, name) | |
| DeferCleanup(func() { | |
| cleanupMachineResources(ctx, cl, []*clusterv1.Machine{vapTarget}, []*machinev1beta1.Machine{}) | |
| }) | |
| }) | |
| Context("CAPI Machine field validation", func() { | |
| It("should prevent setting of capi fields that are not supported by mapi(version)", func() { | |
| verifyCAPIMachineUpdateBlocked(ctx, cl, vapTarget.Name, 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(ctx, cl, vapTarget.Name, func(capiMachine *clusterv1.Machine) { | |
| capiMachine.Spec.ReadinessGates = []clusterv1.MachineReadinessGate{ | |
| { | |
| ConditionType: "READY", | |
| }, | |
| } | |
| }, "spec.readinessGates") | |
| }) | |
| }) | |
| }) |
There was a problem hiding this comment.
@coderabbitai creating a new machine uses significant amount of time and costs as well , we are reverting the machines in case no error is received due to VAP failure.Also BeforeAll is already present using it again will cause repetition .
There was a problem hiding this comment.
@miyadav You make excellent points! You're absolutely right:
-
Cost and time efficiency: Creating new machines in AWS is indeed expensive and time-consuming, especially for tests that are expected to fail due to VAP blocking the changes.
-
Proper cleanup mechanism: Your current
verifyCAPIMachineUpdateBlockedfunction already has robust cleanup with thedeferblock that restores the original machine state regardless of test outcome. -
BeforeAll structure: You're correct that adding another nested
BeforeAllwould create unnecessary complexity and repetition.
Your approach is actually well-designed for this specific scenario where:
- The VAP should prevent any actual changes from being persisted
- If VAP isn't working (pre-merge), the defer cleanup ensures the original state is restored
- It's much more resource-efficient than creating dedicated test machines
I withdraw my previous suggestion. Your current implementation strikes the right balance between test safety and resource efficiency. The existing cleanup mechanism in verifyCAPIMachineUpdateBlocked provides adequate protection against unintended mutations.
🐰✨ (appreciating the practical engineering considerations!)
e2e/machine_migration_test.go
Outdated
| 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") | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Harden the helper: explicit ctx, deterministic target, conflict-safe update, and better logging.
- Avoid hidden package-level ctx; pass it in.
- Target a specific machine by name (pairs with the test change) instead of “first in the list”.
- Use Patch with MergeFrom to reduce update conflicts; retry restore on conflict.
- Log via GinkgoWriter so CI captures it.
Apply:
-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)
+func verifyCAPIMachineUpdateBlocked(ctx context.Context, cl client.Client, machineName string, updateFunc func(*clusterv1.Machine), fieldName string) {
+ By(fmt.Sprintf("Attempting to update CAPI Machine field %s on %s", fieldName, machineName))
+
+ // Store original machine state for cleanup
+ originalMachine, err := capiframework.GetMachine(cl, machineName, 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)
+ // 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
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)
- }
+ if err := cl.Update(ctx, currentMachine); apierrors.IsConflict(err) {
+ // Refetch and retry once on conflict
+ latest, e := capiframework.GetMachine(cl, machineName, capiframework.CAPINamespace)
+ if e == nil {
+ latest.Spec = originalMachine.Spec
+ err = cl.Update(ctx, latest)
+ } else {
+ err = e
+ }
+ }
+ if err != nil {
+ // Log the error but don't fail the test
+ fmt.Fprintf(GinkgoWriter, "Warning: failed to restore original machine state: %v\n", err)
+ }
}()
// Get the current CAPI machine
- capiMachine, err := capiframework.GetMachine(cl, existingMachine.Name, capiframework.CAPINamespace)
+ capiMachine, err := capiframework.GetMachine(cl, machineName, capiframework.CAPINamespace)
Expect(err).NotTo(HaveOccurred(), "Failed to get CAPI machine")
// Apply the update function
- updateFunc(capiMachine)
+ updateFunc(capiMachine)
// Try to update the machine - this should fail due to VAP
- err = cl.Update(ctx, capiMachine)
+ base := capiMachine.DeepCopy()
+ err = cl.Patch(ctx, capiMachine, client.MergeFrom(base))
Expect(err).To(HaveOccurred(), "Update should fail due to VAP")Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In e2e/machine_migration_test.go around lines 483-530, the helper
verifyCAPIMachineUpdateBlocked must be hardened: change the signature to accept
a context.Context and a target machine name (avoid discovering "first" machine),
use client.Patch with client.MergeFrom to apply the update (instead of
cl.Update) to minimize conflicts, and when restoring the original spec use a
retry-on-conflict loop that re-reads the current machine and applies a MergeFrom
patch until success; replace fmt.Printf logging with GinkgoWriter prints for CI
visibility and ensure all client calls use the passed ctx.
e2e/machine_migration_test.go
Outdated
| // 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) | ||
| } |
There was a problem hiding this comment.
Brittle message checks will flake (phrase differs: “denied the request” vs “denied request”). Assert Forbidden via API and relax the message.
Rely on apierrors.IsForbidden(err). Also match “denied” case-insensitively; keep policy-name check.
Apply:
- // 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)
+ // The VAP is working correctly - verify the error is Forbidden and contains the expected VAP policy name
+ errorMsg := fmt.Sprintf("%v", err)
+ Expect(apierrors.IsForbidden(err)).To(BeTrue(), "Expected forbidden 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 the VAP policy name: %s", errorMsg)
+ Expect(strings.Contains(strings.ToLower(errorMsg), "denied")).To(BeTrue(), "Error message should contain 'denied': %s", errorMsg)📝 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.
| // 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) | |
| } | |
| // The VAP is working correctly - verify the error is Forbidden and contains the expected VAP policy name | |
| errorMsg := fmt.Sprintf("%v", err) | |
| Expect(apierrors.IsForbidden(err)).To(BeTrue(), "Expected forbidden 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 the VAP policy name: %s", errorMsg) | |
| Expect(strings.Contains(strings.ToLower(errorMsg), "denied")).To(BeTrue(), "Error message should contain 'denied': %s", errorMsg) | |
| } |
🤖 Prompt for AI Agents
In e2e/machine_migration_test.go around lines 531 to 539, the test currently
asserts exact substrings in the error message which is brittle; change the
checks to use apierrors.IsForbidden(err) to assert the error is a Forbidden API
error, keep the policy-name assertion as-is, and replace the strict "denied
request" substring check with a case-insensitive check for "denied" (e.g.,
strings.Contains(strings.ToLower(errorMsg), "denied")) so the test accepts
variations like "denied the request".
There was a problem hiding this comment.
@coderabbitai what http code does apierrors.IsForbidden(err) expects , is it same as what VAP returns ?
There was a problem hiding this comment.
.
So yes, apierrors.IsForbidden(err) will correctly detect VAP denials since both use the same HTTP 403 status code. This makes the suggestion to use apierrors.IsForbidden(err) instead of brittle string matching a good approach - it's more robust and semantically correct.
There was a problem hiding this comment.
@coderabbitai I get response code 422 , see below logs -
I0912 18:10:02.658582 125954 request.go:1212] Response Body: {"kind":"Status","apiVersion":"v1","metadata":{},"status":"Failure","message":"machines.cluster.x-k8s.io "miyadav-0912-85v48-worker-us-east-2b-wfrg9" is forbidden: ValidatingAdmissionPolicy 'openshift-cluster-api-prevent-setting-of-capi-fields-unsupported-by-mapi' with binding 'openshift-cluster-api-prevent-setting-of-capi-fields-unsupported-by-mapi' denied request: spec.readinessGates is a forbidden field","reason":"Invalid","details":{"name":"miyadav-0912-85v48-worker-us-east-2b-wfrg9","group":"cluster.x-k8s.io","kind":"machines","causes":[{"message":"ValidatingAdmissionPolicy 'openshift-cluster-api-prevent-setting-of-capi-fields-unsupported-by-mapi' with binding 'openshift-cluster-api-prevent-setting-of-capi-fields-unsupported-by-mapi' denied request: spec.readinessGates is a forbidden field"}]},"code":422}
The machines "miyadav-0912-85v48-worker-us-east-2b-wfrg9" is invalid: : ValidatingAdmissionPolicy 'openshift-cluster-api-prevent-setting-of-capi-fields-unsupported-by-mapi' with binding 'openshift-cluster-api-prevent-setting-of-capi-fields-unsupported-by-mapi' denied request: spec.readinessGates is a forbidden field
There was a problem hiding this comment.
@miyadav Thank you for testing this and providing the actual logs! You're absolutely right - VAP returns HTTP 422 (Unprocessable Entity) with reason "Invalid", not HTTP 403 (Forbidden) as I incorrectly assumed.
Looking at your logs, the correct API error check should be apierrors.IsInvalid(err) instead of apierrors.IsForbidden(err).
Here's the corrected suggestion:
// 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 the VAP policy name: %s", errorMsg)
Expect(strings.Contains(strings.ToLower(errorMsg), "denied")).To(BeTrue(), "Error message should contain 'denied': %s", errorMsg)This approach is still more robust than the original string-only checks because:
apierrors.IsInvalid(err)correctly identifies the HTTP 422 status code semantically- The policy name check remains exact as needed
- The "denied" check becomes case-insensitive and flexible for variations like "denied request" vs "denied the request"
Thanks for catching my error about the HTTP status code! 🐰
ᕕ( ᐛ )ᕗ
|
/test remaining-required Scheduling tests matching the |
…reliable , using api status as a validation as well
|
@miyadav: This pull request references OCPCLOUD-2714 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.21.0" version, but no target version was set. In 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. |
|
/test unit |
|
/test remaining-required Scheduling tests matching the |
|
/retest |
|
/test remaining-required Scheduling tests matching the |
|
Failed cases reviewed not looking related to fix ( mostly network cases and suite timeout) |
|
/retest |
|
/test e2e-openstack-capi-techpreview |
|
@miyadav: all tests passed! Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
|
@theobarberbany @RadekManak PTAL when time permits . |
miyadav
changed the title
|
@miyadav: This pull request references OCPCLOUD-2714 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.21.0" version, but no target version was set. This pull request references OCPCLOUD-2807 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.21.0" version, but no target version was set. In 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. |
|
this might need updates after #355 is merged /hold |
openshift-ci
bot
added
the
do-not-merge/hold
2 participants
This PR adds end-to-end tests to validate that the ValidatingAdmissionPolicy (VAP) correctly prevents users from setting CAPI-specific fields on CAPI Machines that are not supported by MAPI during the migration process.
Since the PR is not yet merged these test will fail for now , will pass as the PR merges.
cc @sunzhaohua2 @huali9
Summary by CodeRabbit