SPLAT-2813: Create VAP E2Es#1517
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@vr4manta: This pull request references SPLAT-2813 which is a valid jira issue. 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/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds a vSphere end-to-end test suite for failure-domain ValidatingAdmissionPolicies. The tests verify policy deployment and Infrastructure failure-domain removal behavior when domains are referenced by Machines, MachineSets, or ControlPlaneMachineSets. ChangesvSphere failure-domain VAP tests
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Ginkgo
participant Infrastructure
participant AdmissionPolicy
participant MachineSet
participant ControlPlaneMachineSet
Ginkgo->>Infrastructure: Request failure-domain removal
Infrastructure->>AdmissionPolicy: Validate update
AdmissionPolicy->>MachineSet: Check failure-domain references
AdmissionPolicy->>ControlPlaneMachineSet: Check failure-domain references
AdmissionPolicy-->>Infrastructure: Allow or deny update
Infrastructure-->>Ginkgo: Return update result
🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Comment |
|
/payload-job periodic-ci-openshift-release-main-nightly-5.0-e2e-vsphere-ovn-zones |
|
@vr4manta: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/81721120-7f9f-11f1-941e-018542fca9e5-0 |
|
[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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
test/e2e/vsphere/failure_domain_vap.go (3)
34-48: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueLocal variable shadows builtin
copy.Naming the
DeepCopy()resultcopyshadows the built-incopy()function within this scope. Harmless here since the builtin isn't otherwise needed, but worth avoiding for clarity.♻️ Rename to avoid shadowing
- copy := infra.DeepCopy() - fds := copy.Spec.PlatformSpec.VSphere.FailureDomains + infraCopy := infra.DeepCopy() + fds := infraCopy.Spec.PlatformSpec.VSphere.FailureDomains filtered := fds[:0] for _, fd := range fds { if fd.Name != fdName { filtered = append(filtered, fd) } } - copy.Spec.PlatformSpec.VSphere.FailureDomains = filtered - return copy + infraCopy.Spec.PlatformSpec.VSphere.FailureDomains = filtered + return infraCopy🤖 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 `@test/e2e/vsphere/failure_domain_vap.go` around lines 34 - 48, Rename the DeepCopy() result local variable in infraWithFDRemoved from copy to a non-shadowing name, and update its subsequent references when accessing and assigning Spec.PlatformSpec.VSphere.FailureDomains. Preserve the existing filtering behavior.
234-234: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
defer GinkgoRecover()at the top of theDescribeclosure has no effect.This closure runs once during spec-tree construction, not during spec execution, so the deferred recover doesn't protect the
It/BeforeEachbodies where assertions actually run.GinkgoRecover()is only needed inside goroutines spawned from spec code; none are spawned in this file, so this line is effectively dead code.♻️ Remove the ineffective defer
func() { - defer GinkgoRecover() - ctx := context.Background()🤖 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 `@test/e2e/vsphere/failure_domain_vap.go` at line 234, Remove the ineffective defer GinkgoRecover() from the Describe closure in failure_domain_vap.go; no replacement is needed because this file does not spawn goroutines requiring recovery.
77-78: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRegion/zone label keys hardcoded and duplicated across the file.
The literal strings
"machine.openshift.io/region"and"machine.openshift.io/zone"are repeated at least seven times (lines 77-78, 120-121, 127-128, 214-215, 323-324, 334-335, 462-463). These same labels are read by the CEL-based VAPs inpkg/webhooks/vap.goviamachineRegionLabel/machineZoneLabelconstants. Duplicating the literals here risks silent drift (a typo just causes a Skip or a false pass instead of a compile error), and any future rename in the webhooks package would require finding every copy in this test file.Extract local package-level constants for these two labels and reference them everywhere in this file, mirroring the naming used in
pkg/webhooks/vap.goto make the coupling explicit.const ( machineRegionLabel = "machine.openshift.io/region" machineZoneLabel = "machine.openshift.io/zone" )Also applies to: 120-121, 127-128, 214-215, 323-324, 334-335, 462-463
🤖 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 `@test/e2e/vsphere/failure_domain_vap.go` around lines 77 - 78, Define package-level machineRegionLabel and machineZoneLabel constants in test/e2e/vsphere/failure_domain_vap.go, matching the values and names used by the VAP implementation, then replace every repeated region and zone label literal throughout the file with these constants.
🤖 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 `@test/e2e/vsphere/failure_domain_vap.go`:
- Around line 379-380: Update the error assertions in the Machine and MachineSet
VAP tests to use their exact MessageExpression phrasing, such as “in use by
Machine '” and “in use by MachineSet '”, instead of the ambiguous “Machine” and
“MachineSet” substrings; leave the ControlPlaneMachineSet assertion unchanged.
- Around line 91-160: Update findFDUsedByCPMS to identify when its returned
failure domain comes from the worker-shared fallback rather than being
CPMS-only, and propagate that ambiguity to the callers around the CPMS admission
assertion. When no CPMS-only FD exists, skip or relax the expected
“ControlPlaneMachineSet” substring check while preserving the strict assertion
for an exclusive FD.
---
Nitpick comments:
In `@test/e2e/vsphere/failure_domain_vap.go`:
- Around line 34-48: Rename the DeepCopy() result local variable in
infraWithFDRemoved from copy to a non-shadowing name, and update its subsequent
references when accessing and assigning
Spec.PlatformSpec.VSphere.FailureDomains. Preserve the existing filtering
behavior.
- Line 234: Remove the ineffective defer GinkgoRecover() from the Describe
closure in failure_domain_vap.go; no replacement is needed because this file
does not spawn goroutines requiring recovery.
- Around line 77-78: Define package-level machineRegionLabel and
machineZoneLabel constants in test/e2e/vsphere/failure_domain_vap.go, matching
the values and names used by the VAP implementation, then replace every repeated
region and zone label literal throughout the file with these constants.
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 692848d5-6190-4e89-856f-4cf264817a6a
📒 Files selected for processing (1)
test/e2e/vsphere/failure_domain_vap.go
| // findFDUsedByCPMS returns the name of a vSphere failure domain that is referenced by the | ||
| // ControlPlaneMachineSet but is NOT referenced by any worker Machine or MachineSet. Picking | ||
| // such an FD ensures the CPMS VAP fires first when we attempt to remove it, producing the | ||
| // "ControlPlaneMachineSet" error message we assert on. If every CPMS FD is also used by a | ||
| // worker resource, the function falls back to returning the first CPMS FD regardless — in | ||
| // that case the test caller should still pass since one of the VAPs will block the removal, | ||
| // though the error may mention a different resource type. | ||
| // | ||
| // Returns ("", false) when the CPMS has no vsphere failure domain entries. | ||
| func findFDUsedByCPMS( | ||
| cpms *machinev1.ControlPlaneMachineSet, | ||
| infra *configv1.Infrastructure, | ||
| machines *machinev1beta1.MachineList, | ||
| machineSets *machinev1beta1.MachineSetList, | ||
| ) (string, bool) { | ||
| template := cpms.Spec.Template.OpenShiftMachineV1Beta1Machine | ||
| if template == nil { | ||
| return "", false | ||
| } | ||
| cpmsFDs := template.FailureDomains | ||
| if cpmsFDs == nil || len(cpmsFDs.VSphere) == 0 { | ||
| return "", false | ||
| } | ||
|
|
||
| // Build a set of (region, zone) pairs used by worker Machines and MachineSets so we can | ||
| // avoid picking an FD that would also trigger the Machine or MachineSet VAP. | ||
| type regionZone struct{ region, zone string } | ||
| workerFDs := make(map[regionZone]bool) | ||
| for _, m := range machines.Items { | ||
| r := m.Labels["machine.openshift.io/region"] | ||
| z := m.Labels["machine.openshift.io/zone"] | ||
| if r != "" && z != "" { | ||
| workerFDs[regionZone{r, z}] = true | ||
| } | ||
| } | ||
| for _, ms := range machineSets.Items { | ||
| r := ms.Spec.Template.ObjectMeta.Labels["machine.openshift.io/region"] | ||
| z := ms.Spec.Template.ObjectMeta.Labels["machine.openshift.io/zone"] | ||
| if r != "" && z != "" { | ||
| workerFDs[regionZone{r, z}] = true | ||
| } | ||
| } | ||
|
|
||
| // Build a lookup of infra FD name → spec for quick resolution. | ||
| infraFDByName := make(map[string]configv1.VSpherePlatformFailureDomainSpec) | ||
| for _, fd := range infra.Spec.PlatformSpec.VSphere.FailureDomains { | ||
| infraFDByName[fd.Name] = fd | ||
| } | ||
|
|
||
| var firstMatch string | ||
| for _, cpmsFD := range cpmsFDs.VSphere { | ||
| infraFD, ok := infraFDByName[cpmsFD.Name] | ||
| if !ok { | ||
| continue // CPMS references an FD not present in infra — skip. | ||
| } | ||
| if firstMatch == "" { | ||
| firstMatch = infraFD.Name // record the first valid match as fallback | ||
| } | ||
| // Prefer an FD not shared with any worker Machine or MachineSet. | ||
| if !workerFDs[regionZone{infraFD.Region, infraFD.Zone}] { | ||
| return infraFD.Name, true | ||
| } | ||
| } | ||
|
|
||
| // Fall back to the first CPMS FD that exists in infra, even if workers also use it. | ||
| if firstMatch != "" { | ||
| return firstMatch, true | ||
| } | ||
| return "", false | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file map =="
git ls-files 'test/e2e/vsphere/failure_domain_vap.go' 'test/e2e/vsphere/*' | sed -n '1,120p'
echo
echo "== outline for target file =="
ast-grep outline test/e2e/vsphere/failure_domain_vap.go --view expanded || true
echo
echo "== relevant line ranges =="
sed -n '1,260p' test/e2e/vsphere/failure_domain_vap.go | cat -n | sed -n '1,260p'
echo
echo "== search for helper usage and substring assertion =="
rg -n "findFDUsedByCPMS|ControlPlaneMachineSet|ContainSubstring" test/e2e/vsphere/failure_domain_vap.go test/e2e/vsphere -SRepository: openshift/machine-api-operator
Length of output: 16253
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Searching target file and assertion context...\n'
rg -n -C 3 'findFDUsedByCPMS|ControlPlaneMachineSet|ContainSubstring' test/e2e/vsphere/failure_domain_vap.goRepository: openshift/machine-api-operator
Length of output: 4071
🌐 Web query:
Kubernetes ValidatingAdmissionPolicy multiple matching policies denial error message aggregated policy names
💡 Result:
In Kubernetes, when multiple ValidatingAdmissionPolicy bindings apply to a single request, the API server evaluates each binding. By design, the admission control process for ValidatingAdmissionPolicy typically stops evaluation after the first encountered validation failure that results in a denial (Deny action) [1][2]. Consequently, error messages returned to the client generally reflect only the first failed validation from the first policy that triggered a denial [2][3][4]. Key points regarding the behavior and aggregation of these errors include: 1. Stopping at the First Failure: The evaluation engine is optimized to stop at the first denial to prevent unnecessary processing and to limit information exposure, which is particularly relevant for security constraints [2]. 2. Error Message Content: The standard error response provided to the user includes the name of the specific ValidatingAdmissionPolicy and the Binding that caused the denial, along with the associated failure message [1][2][3]. 3. Audit vs. API Response: While API response errors remain restricted to the first failure, improvements have been made to audit logging. Recent updates to the Kubernetes API server have addressed cases where multiple audit-action failures were previously dropped; for instance, failures are now buffered and aggregated into a single annotation in audit logs to ensure all audit-related policy failures are captured [5]. 4. Developer Context: There has been active community discussion regarding surfacing all validation errors to the client rather than just the first one [2][6]. While this is a recognized desire—particularly for improving the user experience and reducing the trial-and-error cycle—current stable behavior remains focused on the first denial [2][6]. In summary, you cannot currently expect an aggregated list of multiple policy denial messages in the direct API error response to your client; the response will be dominated by the first failure that triggered the denial [1][2][4].
Citations:
- 1: https://kubernetes.io/blog/2024/04/24/validating-admission-policy-ga/
- 2: CEL ValidatingAdmissionPolicy only one validation result is displayed kubernetes/kubernetes#115920
- 3: https://kubernetes.ltd/docs/reference/access-authn-authz/validating-admission-policy/
- 4: https://github.com/kubernetes/website/blob/9a8c421c1e18dd9485788f1ffc23944c41e91483/content/en/docs/reference/access-authn-authz/validating-admission-policy.md
- 5: apiserver/admission: publish all VAP audit failures in a single annot… kubernetes/kubernetes#140001
- 6: When the API server calls multiple validating webhooks, it should surface all errors to the client, not just the first error returned by a webhook kubernetes/kubernetes#132637
Guard the CPMS check on the shared-FD fallback path. In test/e2e/vsphere/failure_domain_vap.go:100-157, 429-441, findFDUsedByCPMS can fall back to a failure domain that is also used by a worker Machine/MachineSet, so the admission error may mention Machine or MachineSet instead of ControlPlaneMachineSet. Return a flag for that ambiguous fallback and skip or relax the substring assertion when no CPMS-only FD exists.
🤖 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 `@test/e2e/vsphere/failure_domain_vap.go` around lines 91 - 160, Update
findFDUsedByCPMS to identify when its returned failure domain comes from the
worker-shared fallback rather than being CPMS-only, and propagate that ambiguity
to the callers around the CPMS admission assertion. When no CPMS-only FD exists,
skip or relax the expected “ControlPlaneMachineSet” substring check while
preserving the strict assertion for an exclusive FD.
|
/payload-job periodic-ci-openshift-release-main-nightly-5.0-e2e-vsphere-ovn-techpreview |
|
@vr4manta: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/452dd3d0-7fb7-11f1-8e5e-bc54849f548e-0 |
2 participants
SPLAT-2813
Changes
Summary by CodeRabbit