SPLAT-2826: Compare against oldObject in vSphere failure-domain VAPs

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),
 				},

pkg/webhooks/vap_test.go

@@ -36,19 +36,29 @@ 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))
 	validation := spec.Validations[0]
 	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))

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)")
+		})
 	},
 )