OCPBUGS-63320: Fix MAPI->CAPI conversion not adding ControlPlane label

pkg/controllers/machinesync/aws_test.go

@@ -18,6 +18,7 @@ package machinesync
 
 import (
 	"context"
+	"encoding/json"
 
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
@@ -93,7 +94,7 @@ var _ = Describe("AWS load balancer validation during MAPI->CAPI conversion", fu
 		Expect(k8sClient.Create(ctx, capiNamespace)).To(Succeed())
 
 		infrastructureName = "cluster-aws-lb"
-		awsClusterBuilder = awsv1resourcebuilder.AWSCluster().WithNamespace(capiNamespace.GetName()).WithName(infrastructureName)
+		awsClusterBuilder = awsv1resourcebuilder.AWSCluster().WithNamespace(capiNamespace.GetName()).WithName(infrastructureName).WithRegion("us-east-1")
 
 		// Create CAPI Cluster that all tests will use
 		capiCluster := clusterv1resourcebuilder.Cluster().WithNamespace(capiNamespace.GetName()).WithName(infrastructureName).WithInfrastructureRef(clusterv1.ContractVersionedObjectReference{
@@ -184,7 +185,8 @@ var _ = Describe("AWS load balancer validation during MAPI->CAPI conversion", fu
 		)
 	})
 
-	It("rejects control-plane machines missing required control-plane LB", func() {
+	// TODO(OCPCLOUD-2115): Re-enable once control plane machine conversion is enabled.
+	XIt("rejects control-plane machines missing required control-plane LB", func() {
 		loadBalancerSpec := &awsv1.AWSLoadBalancerSpec{
 			Name:             ptr.To("cluster-int"),
 			LoadBalancerType: awsv1.LoadBalancerTypeNLB,
@@ -213,7 +215,8 @@ var _ = Describe("AWS load balancer validation during MAPI->CAPI conversion", fu
 		)
 	})
 
-	It("rejects control-plane machines with wrong LB type", func() {
+	// TODO(OCPCLOUD-2115): Re-enable once control plane machine conversion is enabled.
+	XIt("rejects control-plane machines with wrong LB type", func() {
 		loadBalancerSpec := &awsv1.AWSLoadBalancerSpec{
 			Name:             ptr.To("cluster-int"),
 			LoadBalancerType: awsv1.LoadBalancerTypeNLB,
@@ -256,7 +259,8 @@ var _ = Describe("AWS load balancer validation during MAPI->CAPI conversion", fu
 		)
 	})
 
-	It("accepts control-plane machines with matching LB names and types", func() {
+	// TODO(OCPCLOUD-2115): Re-enable once control plane machine conversion is enabled.
+	XIt("accepts control-plane machines with matching LB names and types", func() {
 		loadBalancerSpec := &awsv1.AWSLoadBalancerSpec{
 			Name:             ptr.To("cluster-int"),
 			LoadBalancerType: awsv1.LoadBalancerTypeNLB,
@@ -298,6 +302,87 @@ var _ = Describe("AWS load balancer validation during MAPI->CAPI conversion", fu
 		capiMachine := clusterv1resourcebuilder.Machine().WithNamespace(capiNamespace.GetName()).WithName(mapiMachine.GetName()).Build()
 		Eventually(k8sClient.Get(ctx, client.ObjectKeyFromObject(capiMachine), capiMachine), timeout).Should(Succeed())
 	})
+
+	// TODO(OCPCLOUD-2115): Re-enable once control plane machine conversion is enabled.
+	XIt("should preserve load balancers when toggling authoritativeAPI MAPI -> CAPI -> MAPI", func() {
+		By("Creating AWSCluster with load balancers")
+		loadBalancerSpec := &awsv1.AWSLoadBalancerSpec{
+			Name:             ptr.To("cluster-int"),
+			LoadBalancerType: awsv1.LoadBalancerTypeNLB,
+		}
+		secondaryLoadBalancerSpec := &awsv1.AWSLoadBalancerSpec{
+			Name:             ptr.To("cluster-ext"),
+			LoadBalancerType: awsv1.LoadBalancerTypeNLB,
+		}
+		awsCluster := awsClusterBuilder.WithControlPlaneLoadBalancer(loadBalancerSpec).WithSecondaryControlPlaneLoadBalancer(secondaryLoadBalancerSpec).Build()
+		Expect(k8sClient.Create(ctx, awsCluster)).To(Succeed())
+
+		lbRefs := []mapiv1beta1.LoadBalancerReference{
+			{Name: "cluster-int", Type: mapiv1beta1.NetworkLoadBalancerType},
+			{Name: "cluster-ext", Type: mapiv1beta1.NetworkLoadBalancerType},
+		}
+
+		By("Creating MAPI master machine with load balancers and authoritativeAPI: MachineAPI")
+		mapiMachine := machinev1resourcebuilder.Machine().
+			WithNamespace(mapiNamespace.GetName()).
+			WithGenerateName("master-").
+			AsMaster().
+			WithProviderSpecBuilder(machinev1resourcebuilder.AWSProviderSpec().WithLoadBalancers(lbRefs)).
+			Build()
+		Expect(k8sClient.Create(ctx, mapiMachine)).To(Succeed())
+		Eventually(k.UpdateStatus(mapiMachine, func() { mapiMachine.Status.AuthoritativeAPI = mapiv1beta1.MachineAuthorityMachineAPI })).Should(Succeed())
+
+		By("Verifying MAPI -> CAPI sync is successful")
+		Eventually(k.Object(mapiMachine), timeout).Should(
+			HaveField("Status.Conditions", ContainElement(
+				SatisfyAll(
+					HaveField("Type", Equal(consts.SynchronizedCondition)),
+					HaveField("Status", Equal(corev1.ConditionTrue)),
+					HaveField("Reason", Equal("ResourceSynchronized")),
+					HaveField("Message", Equal("Successfully synchronized MAPI Machine to CAPI")),
+				))),
+		)
+
+		By("Capturing original providerSpec")
+		Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(mapiMachine), mapiMachine)).To(Succeed())
+		originalProviderSpec := mapiMachine.Spec.ProviderSpec.Value.DeepCopy()
+
+		By("Switching authoritativeAPI to ClusterAPI")
+		transitionToAuthoritativeAPI(k, mapiMachine, mapiv1beta1.MachineAuthorityClusterAPI, timeout)
+
+		By("Verifying CAPI -> MAPI sync is successful")
+		Eventually(k.Object(mapiMachine), timeout).Should(
+			HaveField("Status.Conditions", ContainElement(
+				SatisfyAll(
+					HaveField("Type", Equal(consts.SynchronizedCondition)),
+					HaveField("Status", Equal(corev1.ConditionTrue)),
+					HaveField("Reason", Equal("ResourceSynchronized")),
+					HaveField("Message", Equal("Successfully synchronized CAPI Machine to MAPI")),
+				))),
+		)
+
+		By("Switching authoritativeAPI back to MachineAPI")
+		transitionToAuthoritativeAPI(k, mapiMachine, mapiv1beta1.MachineAuthorityMachineAPI, timeout)
+
+		By("Verifying MAPI -> CAPI sync is successful")
+		Eventually(k.Object(mapiMachine), timeout).Should(
+			HaveField("Status.Conditions", ContainElement(
+				SatisfyAll(
+					HaveField("Type", Equal(consts.SynchronizedCondition)),
+					HaveField("Status", Equal(corev1.ConditionTrue)),
+					HaveField("Reason", Equal("ResourceSynchronized")),
+					HaveField("Message", Equal("Successfully synchronized MAPI Machine to CAPI")),
+				))),
+		)
+
+		By("Verifying load balancers are unchanged after round trip")
+		Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(mapiMachine), mapiMachine)).To(Succeed())
+
+		var originalSpec, finalSpec mapiv1beta1.AWSMachineProviderConfig
+		Expect(json.Unmarshal(originalProviderSpec.Raw, &originalSpec)).To(Succeed())
+		Expect(json.Unmarshal(mapiMachine.Spec.ProviderSpec.Value.Raw, &finalSpec)).To(Succeed())
+		Expect(finalSpec.LoadBalancers).To(Equal(originalSpec.LoadBalancers), "load balancers should not change after round trip")
+	})
 })
 
 var _ = Describe("validateLoadBalancerReferencesAgainstExpected", func() {

pkg/controllers/machinesync/helpers_test.go

@@ -0,0 +1,49 @@
+/*
+Copyright 2025 Red Hat, Inc.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+	http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package machinesync
+
+import (
+	. "github.com/onsi/gomega"
+	mapiv1beta1 "github.com/openshift/api/machine/v1beta1"
+	"sigs.k8s.io/controller-runtime/pkg/envtest/komega"
+)
+
+// transitionToAuthoritativeAPI transitions the AuthoritativeAPI of a machine to the target API.
+// It sets spec.AuthoritativeAPI to the target API and then transitions status.AuthoritativeAPI through the Migrating state to the target API.
+// This simulates the migration controller behavior for unit tests.
+func transitionToAuthoritativeAPI(k komega.Komega, machine *mapiv1beta1.Machine, targetAPI mapiv1beta1.MachineAuthority, timeout interface{}) {
+	if machine.Spec.AuthoritativeAPI == targetAPI && machine.Status.AuthoritativeAPI == targetAPI {
+		return
+	}
+
+	if machine.Spec.AuthoritativeAPI != targetAPI {
+		Eventually(k.Update(machine, func() {
+			machine.Spec.AuthoritativeAPI = targetAPI
+		}), timeout).Should(Succeed(), "Failed to update Machine spec.AuthoritativeAPI to %s", targetAPI)
+	}
+
+	// AuthoritativeAPI must transition through Migrating state
+	if machine.Status.AuthoritativeAPI != mapiv1beta1.MachineAuthorityMigrating {
+		Eventually(k.UpdateStatus(machine, func() {
+			machine.Status.AuthoritativeAPI = mapiv1beta1.MachineAuthorityMigrating
+		}), timeout).Should(Succeed(), "Failed to transition Machine status.AuthoritativeAPI to Migrating")
+	}
+
+	Eventually(k.UpdateStatus(machine, func() {
+		machine.Status.AuthoritativeAPI = targetAPI
+	}), timeout).Should(Succeed(), "Failed to transition Machine status.AuthoritativeAPI to %s", targetAPI)
+}

pkg/controllers/machinesync/machine_sync_controller.go

@@ -58,22 +58,22 @@ import (
 )
 
 const (
-	reasonCAPIMachineNotFound                 = "CAPIMachineNotFound"
-	reasonFailedToConvertCAPIMachineToMAPI    = "FailedToConvertCAPIMachineToMAPI"
-	reasonFailedToConvertMAPIMachineToCAPI    = "FailedToConvertMAPIMachineToCAPI"
-	reasonFailedToCreateCAPIInfraMachine      = "FailedToCreateCAPIInfraMachine"
-	reasonFailedToCreateCAPIMachine           = "FailedToCreateCAPIMachine"
-	reasonFailedToCreateMAPIMachine           = "FailedToCreateMAPIMachine"
-	reasonFailedToGetCAPIInfraResources       = "FailedToGetCAPIInfraResources"
-	reasonFailedToUpdateCAPIInfraMachine      = "FailedToUpdateCAPIInfraMachine"
-	reasonFailedToUpdateCAPIMachine           = "FailedToUpdateCAPIMachine"
-	reasonFailedToUpdateMAPIMachine           = "FailedToUpdateMAPIMachine"
-	reasonProgressingToCreateCAPIInfraMachine = "ProgressingToCreateCAPIInfraMachine"
+	reasonCAPIMachineNotFound                      = "CAPIMachineNotFound"
+	reasonFailedToConvertCAPIMachineToMAPI         = "FailedToConvertCAPIMachineToMAPI"
+	reasonFailedToConvertMAPIMachineToCAPI         = "FailedToConvertMAPIMachineToCAPI"
+	reasonUnsupportedControlPlaneMachineConversion = "UnsupportedControlPlaneMachineConversion"
+	reasonFailedToCreateCAPIInfraMachine           = "FailedToCreateCAPIInfraMachine"
+	reasonFailedToCreateCAPIMachine                = "FailedToCreateCAPIMachine"
+	reasonFailedToCreateMAPIMachine                = "FailedToCreateMAPIMachine"
+	reasonFailedToGetCAPIInfraResources            = "FailedToGetCAPIInfraResources"
+	reasonFailedToUpdateCAPIInfraMachine           = "FailedToUpdateCAPIInfraMachine"
+	reasonFailedToUpdateCAPIMachine                = "FailedToUpdateCAPIMachine"
+	reasonFailedToUpdateMAPIMachine                = "FailedToUpdateMAPIMachine"
+	reasonProgressingToCreateCAPIInfraMachine      = "ProgressingToCreateCAPIInfraMachine"
 
 	capiNamespace                  string = "openshift-cluster-api"
 	machineKind                    string = "Machine"
 	machineSetKind                 string = "MachineSet"
-	cpmsKind                       string = "ControlPlaneMachineSet"
 	controllerName                 string = "MachineSyncController"
 	mapiNamespace                  string = "openshift-machine-api"
 	capiInfraCommonFinalizerSuffix string = ".cluster.x-k8s.io"
@@ -118,8 +118,8 @@ var (
 	// errUnsuportedOwnerKindForConversion is returned when attempting to convert unsupported ownerReference.
 	errUnsuportedOwnerKindForConversion = errors.New("unsupported owner kind for owner reference conversion")
 
-	// errUnsupportedCPMSOwnedMachineConversion is returned when attempting to convert ControlPlaneMachineSet owned machines.
-	errUnsupportedCPMSOwnedMachineConversion = errors.New("conversion of control plane machines owned by control plane machine set is currently not supported")
+	// errUnsupportedControlPlaneMachineConversion is returned when attempting to convert any control plane machine.
+	errUnsupportedControlPlaneMachineConversion = errors.New("conversion of control plane machines is currently not supported")
 
 	// errInvalidInfraClusterReference is returned when the cluster name is empty in CAPI machine spec.
 	errInvalidInfraClusterReference = errors.New("cluster name is empty in Cluster API machine spec")
@@ -285,6 +285,19 @@ func (r *MachineSyncReconciler) reconcileCAPIMachinetoMAPIMachine(ctx context.Co
 		return ctrl.Result{}, errCAPIMachineNotFound
 	}
 
+	// TODO(OCPCLOUD-2115): Remove this guard once CPMS has a CAPI provider implementation and control plane machine conversion is enabled.
+	if _, ok := sourceCAPIMachine.Labels[clusterv1.MachineControlPlaneLabel]; ok {
+		logger.Info("Not converting control plane Machine. Conversion of control plane machines is currently not supported")
+
+		if existingMAPIMachine != nil {
+			if condErr := r.applySynchronizedConditionWithPatch(ctx, existingMAPIMachine, corev1.ConditionFalse, reasonUnsupportedControlPlaneMachineConversion, errUnsupportedControlPlaneMachineConversion.Error(), nil); condErr != nil {
+				return ctrl.Result{}, condErr
+			}
+		}
+
+		return ctrl.Result{}, nil
+	}
+
 	infraCluster, infraMachine, err := r.fetchCAPIInfraResources(ctx, sourceCAPIMachine)
 	if err != nil { //nolint:nestif
 		fetchErr := fmt.Errorf("failed to fetch Cluster API infra resources: %w", err)
@@ -414,6 +427,17 @@ func (r *MachineSyncReconciler) reconcileCAPIMachinetoMAPIMachine(ctx context.Co
 func (r *MachineSyncReconciler) reconcileMAPIMachinetoCAPIMachine(ctx context.Context, sourceMAPIMachine *mapiv1beta1.Machine, existingCAPIMachine *clusterv1.Machine) (ctrl.Result, error) {
 	logger := logf.FromContext(ctx)
 
+	// TODO(OCPCLOUD-2115): Remove this guard once CPMS has a CAPI provider implementation and control plane machine conversion is enabled.
+	if util.IsControlPlaneMAPIMachine(sourceMAPIMachine) {
+		logger.Info("Not converting control plane Machine. Conversion of control plane machines is currently not supported")
+
+		if condErr := r.applySynchronizedConditionWithPatch(ctx, sourceMAPIMachine, corev1.ConditionFalse, reasonUnsupportedControlPlaneMachineConversion, errUnsupportedControlPlaneMachineConversion.Error(), nil); condErr != nil {
+			return ctrl.Result{}, condErr
+		}
+
+		return ctrl.Result{}, nil
+	}
+
 	authoritativeAPI := sourceMAPIMachine.Status.AuthoritativeAPI
 
 	if authoritativeAPI == mapiv1beta1.MachineAuthorityClusterAPI {
@@ -451,19 +475,13 @@ func (r *MachineSyncReconciler) reconcileMAPIMachinetoCAPIMachine(ctx context.Co
 	}
 
 	convertedCAPIOwnerReferences, err := r.convertMAPIMachineOwnerReferencesToCAPI(ctx, sourceMAPIMachine)
-	//nolint:nestif
 	if err != nil {
 		var fe *field.Error
 		if errors.As(err, &fe) {
 			if condErr := r.applySynchronizedConditionWithPatch(ctx, sourceMAPIMachine, corev1.ConditionFalse, reasonFailedToConvertMAPIMachineToCAPI, fe.Detail, nil); condErr != nil {
 				return ctrl.Result{}, utilerrors.NewAggregate([]error{err, condErr})
 			}
 
-			if fe.Detail == errUnsupportedCPMSOwnedMachineConversion.Error() {
-				logger.Info("Not converting control plane Machine. Conversion of Machine API machines owned by control plane machine set is currently not supported")
-				return ctrl.Result{}, nil
-			}
-
 			logger.Error(err, "unable to convert Machine API machine to Cluster API, unsupported owner reference in conversion")
 
 			return ctrl.Result{}, nil
@@ -875,10 +893,6 @@ func (r *MachineSyncReconciler) convertMAPIMachineOwnerReferencesToCAPI(ctx cont
 	}
 
 	mapiOwnerReference := mapiMachine.OwnerReferences[0]
-	if mapiOwnerReference.Kind == cpmsKind {
-		return nil, field.Invalid(field.NewPath("metadata", "ownerReferences"), mapiMachine.OwnerReferences, errUnsupportedCPMSOwnedMachineConversion.Error())
-	}
-
 	if mapiOwnerReference.Kind != machineSetKind || mapiOwnerReference.APIVersion != mapiv1beta1.GroupVersion.String() {
 		return nil, field.Invalid(field.NewPath("metadata", "ownerReferences"), mapiMachine.OwnerReferences, errUnsuportedOwnerKindForConversion.Error())
 	}

pkg/controllers/machinesync/machine_sync_controller_test.go

@@ -608,8 +608,8 @@ var _ = Describe("With a running MachineSync Reconciler", func() {
 					SatisfyAll(
 						HaveField("Type", Equal(consts.SynchronizedCondition)),
 						HaveField("Status", Equal(corev1.ConditionFalse)),
-						HaveField("Reason", Equal("FailedToConvertMAPIMachineToCAPI")),
-						HaveField("Message", Equal("conversion of control plane machines owned by control plane machine set is currently not supported")),
+						HaveField("Reason", Equal("UnsupportedControlPlaneMachineConversion")),
+						HaveField("Message", Equal("conversion of control plane machines is currently not supported")),
 					))),
 			)
 		})

pkg/conversion/mapi2capi/util.go

@@ -75,6 +75,11 @@ func convertMAPILabelsToCAPI(mapiLabels map[string]string) map[string]string {
 			capiLabelKey, capiLabelVal := transformFunc(mapiLabelVal)
 			capiLabels[capiLabelKey] = capiLabelVal
 
+			// If the machine role is "master", also add the control-plane label.
+			if mapiLabelKey == "machine.openshift.io/cluster-api-machine-role" && mapiLabelVal == "master" {
+				capiLabels[clusterv1.MachineControlPlaneLabel] = ""
+			}
+
 			continue
 		}