From 4ef0ad13ef74778f478e74733af3f13d6e7c5a59 Mon Sep 17 00:00:00 2001 From: Neil Girard Date: Mon, 24 Aug 2026 14:09:03 -0400 Subject: [PATCH] SPLAT-2826: Compare against oldObject in vSphere failure-domain VAPs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Machine and MachineSet VAPs only checked whether a region/zone label pair existed in the incoming Infrastructure spec, so a Machine or MachineSet whose labels never matched any failure domain — old or new — caused every Infrastructure update to be denied. Add an oldFds variable sourced from oldObject and only deny when a failure domain existed in the old spec and was removed from the new one. --- pkg/webhooks/vap.go | 37 ++++++++++-- pkg/webhooks/vap_test.go | 25 ++++++-- test/e2e/vsphere/failure_domain_vap.go | 79 ++++++++++++++++++++++++++ 3 files changed, 132 insertions(+), 9 deletions(-) diff --git a/pkg/webhooks/vap.go b/pkg/webhooks/vap.go index e02d5deef..e02592868 100644 --- a/pkg/webhooks/vap.go +++ b/pkg/webhooks/vap.go @@ -101,6 +101,14 @@ func NewVSphereFailureDomainMachineVAP() *admissionregistrationv1.ValidatingAdmi Name: "fds", Expression: `object.?spec.platformSpec.vsphere.failureDomains.orValue([])`, }, + { + // oldFds: the failure domains list from the existing (pre-update) Infrastructure + // spec. Used to distinguish "this Machine's failure domain was removed" from + // "this Machine's failure domain never existed in the spec to begin with" + // (SPLAT-2826). + Name: "oldFds", + Expression: `oldObject.?spec.platformSpec.vsphere.failureDomains.orValue([])`, + }, { // machineRegion: the region label of the Machine param (empty string if absent). Name: "machineRegion", @@ -112,13 +120,19 @@ func NewVSphereFailureDomainMachineVAP() *admissionregistrationv1.ValidatingAdmi Expression: `params.?metadata.labels["` + machineZoneLabel + `"].orValue("")`, }, }, - // Core validation: the Machine's region+zone must still exist in the updated infra spec. + // Core validation: only deny when the Machine's failure domain existed in the OLD infra + // spec (something to protect) and is missing from the NEW spec (it was removed). Validations: []admissionregistrationv1.Validation{ { // Pass when: // - Machine has no region/zone label (not a failure-domain-managed Machine), OR + // - The Machine's region/zone never matched any failure domain in the OLD spec + // (nothing to protect — SPLAT-2826), OR // - The failure domain is still present in the incoming spec. Expression: `variables.machineRegion == "" || variables.machineZone == "" || +!variables.oldFds.exists(fd, + fd.region == variables.machineRegion && fd.zone == variables.machineZone +) || variables.fds.exists(fd, fd.region == variables.machineRegion && fd.zone == variables.machineZone )`, @@ -165,6 +179,11 @@ func NewVSphereFailureDomainMachineVAPBinding() *admissionregistrationv1.Validat // The CPMS references failure domains by the Name field of VSpherePlatformFailureDomainSpec. // The policy fires on every UPDATE of infrastructures.config.openshift.io and is evaluated // once per ControlPlaneMachineSet in the openshift-machine-api namespace. +// +// Unlike the Machine and MachineSet VAPs, this VAP intentionally does not compare against +// oldObject (see SPLAT-2826). CPMS matching is by failure domain Name, an unambiguous identity +// key, rather than by region/zone labels sourced from vCenter tags — so "is this name present +// in the current spec" is a correct check whether evaluated against the old or the new spec. func NewVSphereFailureDomainCPMSVAP() *admissionregistrationv1.ValidatingAdmissionPolicy { failurePolicy := admissionregistrationv1.Fail @@ -319,6 +338,12 @@ func NewVSphereFailureDomainMachineSetVAP() *admissionregistrationv1.ValidatingA Name: "fds", Expression: `object.?spec.platformSpec.vsphere.failureDomains.orValue([])`, }, + { + // oldFds: the failure domains list from the existing (pre-update) Infrastructure + // spec. See SPLAT-2826. + Name: "oldFds", + Expression: `oldObject.?spec.platformSpec.vsphere.failureDomains.orValue([])`, + }, { // msRegion: the region label of the MachineSet template (empty string if absent). Name: "msRegion", @@ -330,12 +355,16 @@ func NewVSphereFailureDomainMachineSetVAP() *admissionregistrationv1.ValidatingA Expression: `params.?spec.template.metadata.labels["` + machineZoneLabel + `"].orValue("")`, }, }, - // Core validation: the MachineSet template's region+zone must still exist in the updated infra spec. + // Core validation: only deny when the MachineSet's failure domain existed in the OLD + // infra spec and is missing from the NEW spec (see SPLAT-2826). Validations: []admissionregistrationv1.Validation{ { // MachineSet has no region/zone label in its template (not a failure-domain-managed - // MachineSet), OR the failure domain is still present in the incoming spec. - Expression: `variables.msRegion == "" || variables.msZone == "" || variables.fds.exists(fd, fd.region == variables.msRegion && fd.zone == variables.msZone)`, + // MachineSet), OR its region/zone never matched any FD in the OLD spec (nothing to + // protect), OR the failure domain is still present in the incoming spec. + Expression: `variables.msRegion == "" || variables.msZone == "" || +!variables.oldFds.exists(fd, fd.region == variables.msRegion && fd.zone == variables.msZone) || +variables.fds.exists(fd, fd.region == variables.msRegion && fd.zone == variables.msZone)`, MessageExpression: `"Infrastructure update would remove vSphere failure domain (region=" + variables.msRegion + ", zone=" + variables.msZone + ") that is still in use by MachineSet '" + params.metadata.name + "'"`, Reason: ptr.To(metav1.StatusReasonInvalid), }, diff --git a/pkg/webhooks/vap_test.go b/pkg/webhooks/vap_test.go index 5a766e829..bc8152dad 100644 --- a/pkg/webhooks/vap_test.go +++ b/pkg/webhooks/vap_test.go @@ -36,12 +36,20 @@ func TestNewVSphereFailureDomainMachineVAP(t *testing.T) { g.Expect(spec.MatchConditions[0].Name).To(Equal("is-vsphere-platform")) g.Expect(spec.MatchConditions[0].Expression).To(ContainSubstring(`"VSphere"`)) - // Must define the three CEL variables. + // Must define the four CEL variables. varNames := make([]string, 0, len(spec.Variables)) for _, v := range spec.Variables { varNames = append(varNames, v.Name) } - g.Expect(varNames).To(ConsistOf("fds", "machineRegion", "machineZone")) + g.Expect(varNames).To(ConsistOf("fds", "oldFds", "machineRegion", "machineZone")) + + // The oldFds variable must read from oldObject so removal (not mere absence) is what's checked. + for _, v := range spec.Variables { + if v.Name == "oldFds" { + g.Expect(v.Expression).To(ContainSubstring("oldObject")) + g.Expect(v.Expression).To(ContainSubstring("failureDomains")) + } + } // Must have exactly one validation rule. g.Expect(spec.Validations).To(HaveLen(1)) @@ -49,6 +57,8 @@ func TestNewVSphereFailureDomainMachineVAP(t *testing.T) { g.Expect(validation.Expression).To(ContainSubstring("variables.machineRegion")) g.Expect(validation.Expression).To(ContainSubstring("variables.machineZone")) g.Expect(validation.Expression).To(ContainSubstring("variables.fds.exists")) + g.Expect(validation.Expression).To(ContainSubstring("variables.oldFds.exists")) + g.Expect(validation.Expression).To(ContainSubstring("!variables.oldFds.exists")) g.Expect(validation.MessageExpression).To(ContainSubstring("params.metadata.name")) g.Expect(validation.Reason).NotTo(BeNil()) g.Expect(*validation.Reason).To(Equal(metav1.StatusReasonInvalid)) @@ -184,16 +194,19 @@ func TestNewVSphereFailureDomainMachineSetVAP(t *testing.T) { g.Expect(spec.MatchConditions[0].Name).To(Equal("is-vsphere-platform")) g.Expect(spec.MatchConditions[0].Expression).To(ContainSubstring(`"VSphere"`)) - // Must define the three CEL variables. + // Must define the four CEL variables. varNames := make([]string, 0, len(spec.Variables)) for _, v := range spec.Variables { varNames = append(varNames, v.Name) } - g.Expect(varNames).To(ConsistOf("fds", "msRegion", "msZone")) + g.Expect(varNames).To(ConsistOf("fds", "oldFds", "msRegion", "msZone")) - // The msRegion and msZone variables must read from the template labels path using optional chaining. + // The oldFds, msRegion, and msZone variables must read from the expected paths. for _, v := range spec.Variables { switch v.Name { + case "oldFds": + g.Expect(v.Expression).To(ContainSubstring("oldObject")) + g.Expect(v.Expression).To(ContainSubstring("failureDomains")) case "msRegion": g.Expect(v.Expression).To(ContainSubstring("params.?spec.template.metadata.labels")) g.Expect(v.Expression).To(ContainSubstring(machineRegionLabel)) @@ -209,6 +222,8 @@ func TestNewVSphereFailureDomainMachineSetVAP(t *testing.T) { g.Expect(validation.Expression).To(ContainSubstring("variables.msRegion")) g.Expect(validation.Expression).To(ContainSubstring("variables.msZone")) g.Expect(validation.Expression).To(ContainSubstring("variables.fds.exists")) + g.Expect(validation.Expression).To(ContainSubstring("variables.oldFds.exists")) + g.Expect(validation.Expression).To(ContainSubstring("!variables.oldFds.exists")) g.Expect(validation.MessageExpression).To(ContainSubstring("params.metadata.name")) g.Expect(validation.Reason).NotTo(BeNil()) g.Expect(*validation.Reason).To(Equal(metav1.StatusReasonInvalid)) diff --git a/test/e2e/vsphere/failure_domain_vap.go b/test/e2e/vsphere/failure_domain_vap.go index 5225bf5c4..ada21abbc 100644 --- a/test/e2e/vsphere/failure_domain_vap.go +++ b/test/e2e/vsphere/failure_domain_vap.go @@ -34,6 +34,12 @@ const ( // the VAP to catch up, so a single stuck attempt can't block Eventually past the point // where it should give up and report a failure. vapPollAttemptTimeout = 10 * time.Second + + // vapCacheSyncWindow bounds how long to keep re-checking a dry-run update after creating a + // param object (e.g. a MachineSet), so the assertion is exercised both before and after the + // VAP's informer-backed cache has observed the new object, rather than only immediately + // after creation while the cache may still be stale. + vapCacheSyncWindow = 30 * time.Second ) // infraWithFDRemoved returns a deep copy of the given Infrastructure with the named failure domain removed @@ -616,5 +622,78 @@ var _ = Describe( _, err = cc.Infrastructures().Update(ctx, freshWithFDRemoved, metav1.UpdateOptions{}) Expect(err).NotTo(HaveOccurred(), "expected infra update to succeed after MachineSet referencing FD %q was deleted", fd.Name) }) + + It("should allow an unrelated Infrastructure update when a MachineSet's region/zone labels match no failure domain [apigroup:machine.openshift.io][Suite:openshift/conformance/serial]", func() { + // Regression test for SPLAT-2826: a MachineSet whose region/zone labels never matched + // any failure domain — in the OLD spec or the NEW spec — must not block ANY + // Infrastructure update, including a no-op re-apply of the unchanged spec. + bogusFD := configv1.VSpherePlatformFailureDomainSpec{ + Region: "splat-2826-unmatched-region", + Zone: "splat-2826-unmatched-zone", + } + for _, fd := range infra.Spec.PlatformSpec.VSphere.FailureDomains { + Expect(fd.Region == bogusFD.Region && fd.Zone == bogusFD.Zone).To(BeFalse(), + "test precondition: bogus region+zone pair must not collide with a real failure domain") + } + + By("creating a zero-replica MachineSet whose region/zone labels match no known failure domain") + testMS, err := createVAPTestMachineSet(ctx, cfg, mc, infra, bogusFD) + Expect(err).NotTo(HaveOccurred(), "expected test MachineSet creation to succeed") + + DeferCleanup(func() { + By("cleaning up test MachineSet") + delErr := mc.MachineSets(e2eutil.MachineAPINamespace).Delete(ctx, testMS.Name, metav1.DeleteOptions{}) + if delErr != nil && !apierrors.IsNotFound(delErr) { + e2e.Logf("warning: could not delete test MachineSet %q: %v", testMS.Name, delErr) + return + } + // createVAPTestMachineSet always uses the same deterministic name, so a subsequent + // test's Create can collide if this one is still terminating — wait for confirmed + // absence before cleanup completes. + Eventually(func() bool { + _, getErr := mc.MachineSets(e2eutil.MachineAPINamespace).Get(ctx, testMS.Name, metav1.GetOptions{}) + return apierrors.IsNotFound(getErr) + }, vapTestWaitTimeout, 5*time.Second).Should(BeTrue(), "MachineSet %q should be deleted within %s", testMS.Name, vapTestWaitTimeout) + }) + + // The VAP's ParamRef resolves MachineSets via an informer-backed cache, so immediately + // after creating testMS the VAP may not yet be evaluating it — a single Update run right + // away could pass "by accident" before the fixed oldFds logic is ever exercised against + // testMS's mismatched labels. Poll dry-run updates with Consistently across a bounded + // window that comfortably covers cache propagation, so the assertion is proven both + // before and after testMS is actually observed by the VAP. + By("verifying dry-run no-op updates consistently succeed while the MachineSet is observed") + Consistently(func() error { + attemptCtx, cancel := context.WithTimeout(ctx, vapPollAttemptTimeout) + defer cancel() + for { + latest, getErr := cc.Infrastructures().Get(attemptCtx, "cluster", metav1.GetOptions{}) + if getErr != nil { + return getErr + } + _, updErr := cc.Infrastructures().Update(attemptCtx, latest, metav1.UpdateOptions{DryRun: []string{metav1.DryRunAll}}) + if updErr != nil && apierrors.IsConflict(updErr) { + // Someone else updated the Infrastructure object between our Get and dry-run + // Update (e.g. a status refresh bumping resourceVersion) — retry the whole + // sequence with a fresh object rather than treating this as a VAP denial. + select { + case <-attemptCtx.Done(): + return updErr + default: + continue + } + } + return updErr + } + }, vapCacheSyncWindow, time.Second).Should(Succeed(), + "expected the no-op Infrastructure update to remain allowed throughout the VAP cache-sync window, even once the MachineSet with mismatched region/zone labels is observed (SPLAT-2826 regression)") + + By("re-applying the unchanged Infrastructure spec (no-op update)") + current, err := cc.Infrastructures().Get(ctx, "cluster", metav1.GetOptions{}) + Expect(err).NotTo(HaveOccurred()) + _, err = cc.Infrastructures().Update(ctx, current, metav1.UpdateOptions{}) + Expect(err).NotTo(HaveOccurred(), + "expected a no-op Infrastructure update to succeed even though a MachineSet exists whose region/zone labels match no failure domain (SPLAT-2826 regression)") + }) }, )