From 0879a538bd14f3af4310154aacc1cd9c9946c626 Mon Sep 17 00:00:00 2001 From: Theo Barber-Bany Date: Fri, 14 Nov 2025 17:43:16 +0000 Subject: [PATCH] Disallow authoritativeAPI swap when CAPI Machine is deleting or its Infra is not ready --- ..._30_cluster-api_09_admission-policies.yaml | 75 +++- .../machine_sync_controller_test.go | 330 ++++++++++++++++++ 2 files changed, 404 insertions(+), 1 deletion(-) diff --git a/manifests/0000_30_cluster-api_09_admission-policies.yaml b/manifests/0000_30_cluster-api_09_admission-policies.yaml index fd0f2041f..38aec9600 100644 --- a/manifests/0000_30_cluster-api_09_admission-policies.yaml +++ b/manifests/0000_30_cluster-api_09_admission-policies.yaml @@ -108,7 +108,7 @@ data: ? p[1].hasValue() && p[0].value() == p[1].value() : !p[1].hasValue() ) - + # All validations must evaluate to TRUE validations: # Only spec.authoritativeAPI may change @@ -479,6 +479,79 @@ data: policyName: openshift-provide-warning-when-not-synchronized validationActions: - Warn + --- + apiVersion: admissionregistration.k8s.io/v1 + kind: ValidatingAdmissionPolicyBinding + metadata: + name: openshift-mapi-authoritative-api-transition-requires-capi-infrastructure-ready-and-not-deleting + spec: + matchResources: + namespaceSelector: + matchLabels: + kubernetes.io/metadata.name: openshift-machine-api + paramRef: + namespace: openshift-cluster-api + # We 'Allow' here as we don't want to block MAPI Machine + # functionality when no CAPI machine (param) exists. + # This might happen when the synchronisation controller first + # is installed or when an unmigrateable machine is encountered. + parameterNotFoundAction: Allow + selector: {} + policyName: openshift-mapi-authoritative-api-transition-requires-capi-infrastructure-ready-and-not-deleting + validationActions: [Deny] + --- + apiVersion: admissionregistration.k8s.io/v1 + kind: ValidatingAdmissionPolicy + metadata: + name: openshift-mapi-authoritative-api-transition-requires-capi-infrastructure-ready-and-not-deleting + spec: + failurePolicy: Fail + + paramKind: + apiVersion: cluster.x-k8s.io/v1beta1 + kind: Machine + + matchConstraints: + resourceRules: + - apiGroups: ["machine.openshift.io"] + apiVersions: ["v1beta1"] + operations: ["UPDATE"] + resources: ["machines"] + + # Requests must satisfy every matchCondition to reach the validations + matchConditions: + - name: check-only-non-service-account-requests + expression: >- + !(request.userInfo.username in [ + "system:serviceaccount:openshift-machine-api:machine-api-controllers", + "system:serviceaccount:openshift-cluster-api:cluster-capi-operator" + ]) + - name: check-param-match + expression: 'object.metadata.name == params.metadata.name' + + variables: + - name: authAPIChanged + expression: oldObject.spec.authoritativeAPI != object.spec.authoritativeAPI + + - name: capiInfraReady + expression: params.?status.?infrastructureReady.orValue(false) + + - name: capiDeleting + expression: has(params.metadata.deletionTimestamp) + + # All validations must evaluate to TRUE + validations: + # Don't allow changing the AuthoritativeAPI if the CAPI Machine mirror infrastructureReady is not true + - name: block-when-not-infraReady + expression: > + !(variables.authAPIChanged && !variables.capiInfraReady) + message: "spec.authoritativeAPI may only be updated when the Cluster API Machine's status.infrastructureReady is true." + + # Don't allow changing the AuthoritativeAPI if the CAPI Machine mirror is deleting + - name: block-when-capi-deleting + expression: > + !(variables.authAPIChanged && variables.capiDeleting) + message: "spec.authoritativeAPI cannot be modified while the corresponding CAPI Machine is being deleted (metadata.deletionTimestamp is set)." --- apiVersion: v1 kind: ConfigMap diff --git a/pkg/controllers/machinesync/machine_sync_controller_test.go b/pkg/controllers/machinesync/machine_sync_controller_test.go index 74e3bc764..314f35956 100644 --- a/pkg/controllers/machinesync/machine_sync_controller_test.go +++ b/pkg/controllers/machinesync/machine_sync_controller_test.go @@ -1876,6 +1876,336 @@ var _ = Describe("With a running MachineSync Reconciler", func() { }) }) + Context("when validating MAPI authoritativeAPI transitions", func() { + const vapName = "openshift-mapi-authoritative-api-transition-requires-capi-infrastructure-ready-and-not-deleting" + + BeforeEach(func() { + By("Waiting for VAP to be ready") + machineVap = &admissionregistrationv1.ValidatingAdmissionPolicy{} + Eventually(k8sClient.Get(ctx, client.ObjectKey{Name: vapName}, machineVap), timeout).Should(Succeed()) + Eventually(k.Update(machineVap, func() { + admissiontestutils.AddSentinelValidation(machineVap) + })).Should(Succeed()) + + Eventually(k.Object(machineVap), timeout).Should( + HaveField("Status.ObservedGeneration", BeNumerically(">=", 2)), + ) + + By("Updating the VAP binding") + policyBinding = &admissionregistrationv1.ValidatingAdmissionPolicyBinding{} + Eventually(k8sClient.Get(ctx, client.ObjectKey{ + Name: vapName}, policyBinding), timeout).Should(Succeed()) + + Eventually(k.Update(policyBinding, func() { + // paramNamespace=capiNamespace (CAPI resources are params) + // targetNamespace=mapiNamespace (MAPI resources are validated) + admissiontestutils.UpdateVAPBindingNamespaces(policyBinding, capiNamespace.GetName(), mapiNamespace.GetName()) + }), timeout).Should(Succeed()) + + Eventually(k.Object(policyBinding), timeout).Should( + SatisfyAll( + HaveField("Spec.MatchResources.NamespaceSelector.MatchLabels", + HaveKeyWithValue("kubernetes.io/metadata.name", + mapiNamespace.GetName())), + ), + ) + + By("Creating sentinel Machine pair for VAP verification") + sentinelCapiMachine := clusterv1resourcebuilder.Machine(). + WithNamespace(capiNamespace.Name). + WithName("sentinel-machine"). + Build() + Eventually(k8sClient.Create(ctx, sentinelCapiMachine)).Should(Succeed()) + + sentinelMapiMachine := machinev1resourcebuilder.Machine(). + WithNamespace(mapiNamespace.Name). + WithName("sentinel-machine"). + WithAuthoritativeAPI(mapiv1beta1.MachineAuthorityMachineAPI). + Build() + Eventually(k8sClient.Create(ctx, sentinelMapiMachine), timeout).Should(Succeed()) + + admissiontestutils.VerifySentinelValidation(k, sentinelMapiMachine, timeout) + }) + + Context("when validating infrastructure readiness", func() { + Context("when infrastructure is not ready", func() { + It("should deny authoritativeAPI change when infrastructureReady is false", func() { + By("Creating CAPI Machine with infrastructureReady=false") + Eventually(k.UpdateStatus(capiMachine, func() { + capiMachine.Status.InfrastructureReady = false + })).Should(Succeed()) + + By("Setting MAPI machine AuthoritativeAPI to MachineAPI") + Eventually(k.UpdateStatus(mapiMachine, func() { + mapiMachine.Status.AuthoritativeAPI = mapiv1beta1.MachineAuthorityMachineAPI + })).Should(Succeed()) + + By("Attempting to change authoritativeAPI to ClusterAPI") + Eventually(k.Update(mapiMachine, func() { + mapiMachine.Spec.AuthoritativeAPI = mapiv1beta1.MachineAuthorityClusterAPI + }), timeout).Should(MatchError(ContainSubstring("status.infrastructureReady is true"))) + }) + + It("should deny authoritativeAPI change when infrastructureReady is missing", func() { + By("CAPI Machine was created in BeforeEach without setting infrastructureReady") + + By("Setting MAPI machine AuthoritativeAPI to MachineAPI") + Eventually(k.UpdateStatus(mapiMachine, func() { + mapiMachine.Status.AuthoritativeAPI = mapiv1beta1.MachineAuthorityMachineAPI + })).Should(Succeed()) + + By("Attempting to change authoritativeAPI to ClusterAPI") + Eventually(k.Update(mapiMachine, func() { + mapiMachine.Spec.AuthoritativeAPI = mapiv1beta1.MachineAuthorityClusterAPI + }), timeout).Should(MatchError(ContainSubstring("status.infrastructureReady is true"))) + }) + + It("should deny authoritativeAPI change when status field doesn't exist", func() { + By("CAPI Machine was created in BeforeEach without status") + + By("Setting MAPI machine AuthoritativeAPI to MachineAPI") + Eventually(k.UpdateStatus(mapiMachine, func() { + mapiMachine.Status.AuthoritativeAPI = mapiv1beta1.MachineAuthorityMachineAPI + })).Should(Succeed()) + + By("Attempting to change authoritativeAPI to ClusterAPI") + Eventually(k.Update(mapiMachine, func() { + mapiMachine.Spec.AuthoritativeAPI = mapiv1beta1.MachineAuthorityClusterAPI + }), timeout).Should(MatchError(ContainSubstring("status.infrastructureReady is true"))) + }) + }) + + Context("when infrastructure is ready", func() { + It("should allow authoritativeAPI change when infrastructureReady is true", func() { + By("Creating CAPI Machine with infrastructureReady=true") + Eventually(k.UpdateStatus(capiMachine, func() { + capiMachine.Status.InfrastructureReady = true + })).Should(Succeed()) + + By("Setting MAPI machine AuthoritativeAPI to MachineAPI") + Eventually(k.UpdateStatus(mapiMachine, func() { + mapiMachine.Status.AuthoritativeAPI = mapiv1beta1.MachineAuthorityMachineAPI + })).Should(Succeed()) + + By("Changing authoritativeAPI to ClusterAPI") + Eventually(k.Update(mapiMachine, func() { + mapiMachine.Spec.AuthoritativeAPI = mapiv1beta1.MachineAuthorityClusterAPI + }), timeout).Should(Succeed()) + }) + }) + }) + + Context("when validating deletion state", func() { + Context("when CAPI Machine is deleting", func() { + It("should deny authoritativeAPI change when deletionTimestamp is set", func() { + By("Creating CAPI Machine with infrastructureReady=true") + Eventually(k.UpdateStatus(capiMachine, func() { + capiMachine.Status.InfrastructureReady = true + })).Should(Succeed()) + + By("Adding finalizer and deleting CAPI Machine to set deletionTimestamp") + Eventually(k.Update(capiMachine, func() { + capiMachine.Finalizers = append(capiMachine.Finalizers, "test-finalizer") + })).Should(Succeed()) + + Eventually(k8sClient.Delete(ctx, capiMachine)).Should(Succeed()) + + By("Waiting for deletion timestamp to be set") + Eventually(k.Object(capiMachine)).Should(SatisfyAll( + HaveField("DeletionTimestamp", Not(BeNil())), + )) + + By("Setting MAPI machine AuthoritativeAPI to MachineAPI") + Eventually(k.UpdateStatus(mapiMachine, func() { + mapiMachine.Status.AuthoritativeAPI = mapiv1beta1.MachineAuthorityMachineAPI + })).Should(Succeed()) + + By("Attempting to change authoritativeAPI to ClusterAPI") + Eventually(k.Update(mapiMachine, func() { + mapiMachine.Spec.AuthoritativeAPI = mapiv1beta1.MachineAuthorityClusterAPI + }), timeout).Should(MatchError(ContainSubstring("CAPI Machine is being deleted"))) + }) + }) + + Context("when CAPI Machine is not deleting", func() { + It("should allow authoritativeAPI change when deletionTimestamp is not set", func() { + By("Creating CAPI Machine with infrastructureReady=true and no deletionTimestamp") + Eventually(k.UpdateStatus(capiMachine, func() { + capiMachine.Status.InfrastructureReady = true + })).Should(Succeed()) + + By("Setting MAPI machine AuthoritativeAPI to MachineAPI") + Eventually(k.UpdateStatus(mapiMachine, func() { + mapiMachine.Status.AuthoritativeAPI = mapiv1beta1.MachineAuthorityMachineAPI + })).Should(Succeed()) + + By("Changing authoritativeAPI to ClusterAPI") + Eventually(k.Update(mapiMachine, func() { + mapiMachine.Spec.AuthoritativeAPI = mapiv1beta1.MachineAuthorityClusterAPI + }), timeout).Should(Succeed()) + }) + }) + }) + + Context("when validating mixed states", func() { + It("should deny authoritativeAPI change when infrastructureReady is true but deleting", func() { + By("Creating CAPI Machine with infrastructureReady=true") + Eventually(k.UpdateStatus(capiMachine, func() { + capiMachine.Status.InfrastructureReady = true + })).Should(Succeed()) + + By("Adding finalizer and deleting CAPI Machine to set deletionTimestamp") + Eventually(k.Update(capiMachine, func() { + capiMachine.Finalizers = append(capiMachine.Finalizers, "test-finalizer") + })).Should(Succeed()) + + Eventually(k8sClient.Delete(ctx, capiMachine)).Should(Succeed()) + + By("Waiting for deletion timestamp to be set") + Eventually(k.Object(capiMachine)).Should(SatisfyAll( + HaveField("DeletionTimestamp", Not(BeNil())), + )) + + By("Setting MAPI machine AuthoritativeAPI to MachineAPI") + Eventually(k.UpdateStatus(mapiMachine, func() { + mapiMachine.Status.AuthoritativeAPI = mapiv1beta1.MachineAuthorityMachineAPI + })).Should(Succeed()) + + By("Attempting to change authoritativeAPI to ClusterAPI") + Eventually(k.Update(mapiMachine, func() { + mapiMachine.Spec.AuthoritativeAPI = mapiv1beta1.MachineAuthorityClusterAPI + }), timeout).Should(MatchError(ContainSubstring("CAPI Machine is being deleted"))) + }) + + It("should deny authoritativeAPI change when infrastructureReady is false but not deleting", func() { + By("Creating CAPI Machine with infrastructureReady=false") + Eventually(k.UpdateStatus(capiMachine, func() { + capiMachine.Status.InfrastructureReady = false + })).Should(Succeed()) + + By("Setting MAPI machine AuthoritativeAPI to MachineAPI") + Eventually(k.UpdateStatus(mapiMachine, func() { + mapiMachine.Status.AuthoritativeAPI = mapiv1beta1.MachineAuthorityMachineAPI + })).Should(Succeed()) + + By("Attempting to change authoritativeAPI to ClusterAPI") + Eventually(k.Update(mapiMachine, func() { + mapiMachine.Spec.AuthoritativeAPI = mapiv1beta1.MachineAuthorityClusterAPI + }), timeout).Should(MatchError(ContainSubstring("status.infrastructureReady is true"))) + }) + }) + + Context("when testing VAP trigger conditions", func() { + Context("when changing non-authoritativeAPI fields", func() { + It("should allow updating labels without changing authoritativeAPI", func() { + By("Creating CAPI Machine with infrastructureReady=false") + Eventually(k.UpdateStatus(capiMachine, func() { + capiMachine.Status.InfrastructureReady = false + })).Should(Succeed()) + + By("Setting MAPI machine AuthoritativeAPI to MachineAPI") + Eventually(k.UpdateStatus(mapiMachine, func() { + mapiMachine.Status.AuthoritativeAPI = mapiv1beta1.MachineAuthorityMachineAPI + })).Should(Succeed()) + + By("Updating labels without changing authoritativeAPI") + Eventually(k.Update(mapiMachine, func() { + if mapiMachine.Labels == nil { + mapiMachine.Labels = make(map[string]string) + } + mapiMachine.Labels["test-label"] = "test-value" + }), timeout).Should(Succeed()) + }) + + It("should allow updating annotations without changing authoritativeAPI", func() { + By("Adding finalizer and deleting CAPI Machine to set deletionTimestamp") + Eventually(k.Update(capiMachine, func() { + capiMachine.Finalizers = append(capiMachine.Finalizers, "test-finalizer") + })).Should(Succeed()) + + Eventually(k8sClient.Delete(ctx, capiMachine)).Should(Succeed()) + + By("Waiting for deletion timestamp to be set") + Eventually(k.Object(capiMachine)).Should(SatisfyAll( + HaveField("DeletionTimestamp", Not(BeNil())), + )) + + By("Setting MAPI machine AuthoritativeAPI to MachineAPI") + Eventually(k.UpdateStatus(mapiMachine, func() { + mapiMachine.Status.AuthoritativeAPI = mapiv1beta1.MachineAuthorityMachineAPI + })).Should(Succeed()) + + By("Updating annotations without changing authoritativeAPI") + Eventually(k.Update(mapiMachine, func() { + if mapiMachine.Annotations == nil { + mapiMachine.Annotations = make(map[string]string) + } + mapiMachine.Annotations["test-annotation"] = "test-value" + }), timeout).Should(Succeed()) + }) + + It("should allow updating spec fields without changing authoritativeAPI", func() { + By("CAPI Machine was created without infrastructureReady") + + By("Setting MAPI machine AuthoritativeAPI to MachineAPI") + Eventually(k.UpdateStatus(mapiMachine, func() { + mapiMachine.Status.AuthoritativeAPI = mapiv1beta1.MachineAuthorityMachineAPI + })).Should(Succeed()) + + By("Updating spec.objectMeta.labels without changing authoritativeAPI") + Eventually(k.Update(mapiMachine, func() { + mapiMachine.Spec.ObjectMeta.Labels = map[string]string{"new-label": "new-value"} + }), timeout).Should(Succeed()) + }) + }) + + Context("when CAPI Machine parameter is not found", func() { + It("should allow authoritativeAPI change when CAPI Machine does not exist", func() { + By("Creating a new MAPI Machine without CAPI counterpart") + newMapiMachine := machinev1resourcebuilder.Machine(). + WithNamespace(mapiNamespace.Name). + WithName("no-capi-equivalent"). + WithAuthoritativeAPI(mapiv1beta1.MachineAuthorityMachineAPI). + Build() + Eventually(k8sClient.Create(ctx, newMapiMachine)).Should(Succeed()) + + By("Setting AuthoritativeAPI to MachineAPI") + Eventually(k.UpdateStatus(newMapiMachine, func() { + newMapiMachine.Status.AuthoritativeAPI = mapiv1beta1.MachineAuthorityMachineAPI + })).Should(Succeed()) + + By("Changing authoritativeAPI to ClusterAPI") + Eventually(k.Update(newMapiMachine, func() { + newMapiMachine.Spec.AuthoritativeAPI = mapiv1beta1.MachineAuthorityClusterAPI + }), timeout).Should(Succeed()) + }) + + }) + }) + + Context("when testing edge cases", func() { + Context("when CAPI Machine is in various states", func() { + + It("should deny authoritativeAPI change when CAPI Machine is provisioning", func() { + By("Creating CAPI Machine with infrastructureReady=false and no deletionTimestamp") + Eventually(k.UpdateStatus(capiMachine, func() { + capiMachine.Status.InfrastructureReady = false + })).Should(Succeed()) + + By("Setting MAPI machine AuthoritativeAPI to MachineAPI") + Eventually(k.UpdateStatus(mapiMachine, func() { + mapiMachine.Status.AuthoritativeAPI = mapiv1beta1.MachineAuthorityMachineAPI + })).Should(Succeed()) + + By("Attempting to change authoritativeAPI to ClusterAPI") + Eventually(k.Update(mapiMachine, func() { + mapiMachine.Spec.AuthoritativeAPI = mapiv1beta1.MachineAuthorityClusterAPI + }), timeout).Should(MatchError(ContainSubstring("status.infrastructureReady is true"))) + }) + }) + }) + }) + }) })