diff --git a/pkg/controllers/machinesync/machine_sync_controller.go b/pkg/controllers/machinesync/machine_sync_controller.go index 36e280e1e..df761dadf 100644 --- a/pkg/controllers/machinesync/machine_sync_controller.go +++ b/pkg/controllers/machinesync/machine_sync_controller.go @@ -191,14 +191,14 @@ func (r *MachineSyncReconciler) SetupWithManager(mgr ctrl.Manager) error { // Reconcile reconciles CAPI and MAPI machines for their respective namespaces. // -//nolint:funlen +//nolint:funlen,cyclop func (r *MachineSyncReconciler) Reconcile(ctx context.Context, req reconcile.Request) (ctrl.Result, error) { logger := logf.FromContext(ctx, "namespace", req.Namespace, "name", req.Name) logger.V(1).Info("Reconciling machine") defer logger.V(1).Info("Finished reconciling machine") - var mapiMachineNotFound, capiMachineNotFound bool + var mapiMachineNotFound, capiMachineNotFound, capiInfraMachineExists bool // Get the MAPI Machine. mapiMachine := &mapiv1beta1.Machine{} @@ -254,12 +254,25 @@ func (r *MachineSyncReconciler) Reconcile(ctx context.Context, req reconcile.Req return ctrl.Result{}, nil } + // Check for existence of the Cluster API Infrastructure Machine or if it needs to get created from MAPI first. + capiInfraMachineExists, err := r.doesCAPIInfraMachineExist(ctx, capiMachine, mapiMachine) + if err != nil { + return ctrl.Result{}, fmt.Errorf("failed to check for existence of Cluster API infrastructure machine: %w", err) + } + authoritativeAPI := mapiMachine.Status.AuthoritativeAPI switch { case authoritativeAPI == mapiv1beta1.MachineAuthorityMachineAPI: return r.reconcileMAPIMachinetoCAPIMachine(ctx, mapiMachine, capiMachine) case authoritativeAPI == mapiv1beta1.MachineAuthorityClusterAPI && !capiMachineNotFound: + // Create Cluster API Infrastructure Machine from MAPI if it doesn't exist and no deletion is in progress. + // the CAPI machine is not marked for deletion and the MAPI machine was created before the Cluster API machine. + if capiMachine.DeletionTimestamp.IsZero() && mapiMachine.DeletionTimestamp.IsZero() && + !capiInfraMachineExists && !mapiMachine.CreationTimestamp.After(capiMachine.CreationTimestamp.Time) { + return r.reconcileMAPIMachinetoCAPIMachine(ctx, mapiMachine, capiMachine) + } + return r.reconcileCAPIMachinetoMAPIMachine(ctx, capiMachine, mapiMachine) case authoritativeAPI == mapiv1beta1.MachineAuthorityClusterAPI && capiMachineNotFound: return r.reconcileMAPIMachinetoCAPIMachine(ctx, mapiMachine, capiMachine) @@ -987,7 +1000,7 @@ func (r *MachineSyncReconciler) fetchCAPIInfraResources(ctx context.Context, cap return nil, nil, nil } - var infraCluster, infraMachine client.Object + var infraCluster client.Object infraClusterKey := client.ObjectKey{ Namespace: capiMachine.Namespace, @@ -995,10 +1008,6 @@ func (r *MachineSyncReconciler) fetchCAPIInfraResources(ctx context.Context, cap } infraMachineRef := capiMachine.Spec.InfrastructureRef - infraMachineKey := client.ObjectKey{ - Namespace: infraMachineRef.Namespace, - Name: infraMachineRef.Name, - } // Validate that required references are not empty to avoid nil pointer issues later. // These are terminal configuration errors that require user intervention. @@ -1012,7 +1021,7 @@ func (r *MachineSyncReconciler) fetchCAPIInfraResources(ctx context.Context, cap capiMachine.Namespace, capiMachine.Name, errInvalidInfraMachineReference) } - infraMachine, infraCluster, err := controllers.InitInfraMachineAndInfraClusterFromProvider(r.Platform) + _, infraCluster, err := controllers.InitInfraMachineAndInfraClusterFromProvider(r.Platform) if err != nil { return nil, nil, fmt.Errorf("unable to devise Cluster API infra resources: %w", err) } @@ -1021,13 +1030,50 @@ func (r *MachineSyncReconciler) fetchCAPIInfraResources(ctx context.Context, cap return nil, nil, fmt.Errorf("failed to get Cluster API infrastructure cluster: %w", err) } + infraMachine, err := r.fetchCAPIInfraMachine(ctx, infraMachineRef.Name, infraMachineRef.Namespace) + if err != nil { + return nil, nil, fmt.Errorf("failed to fetch Cluster API infrastructure machine: %w", err) + } + + return infraCluster, infraMachine, nil +} + +func (r *MachineSyncReconciler) fetchCAPIInfraMachine(ctx context.Context, name, namespace string) (client.Object, error) { + infraMachine, _, err := controllers.InitInfraMachineAndInfraClusterFromProvider(r.Platform) + if err != nil { + return nil, fmt.Errorf("unable to devise Cluster API infra resources: %w", err) + } + + infraMachineKey := client.ObjectKey{ + Namespace: namespace, + Name: name, + } + if err := r.Get(ctx, infraMachineKey, infraMachine); err != nil && !apierrors.IsNotFound(err) { - return nil, nil, fmt.Errorf("failed to get Cluster API infrastructure machine: %w", err) + return nil, fmt.Errorf("failed to get: %w", err) } else if apierrors.IsNotFound(err) { infraMachine = nil } - return infraCluster, infraMachine, nil + return infraMachine, nil +} + +// doesCAPIInfraMachineExist checks if the Cluster API Infrastructure Machine exists. It uses the infrastructureRef of the Cluster API Machine with fallback to the name of the MAPI Machine. +func (r *MachineSyncReconciler) doesCAPIInfraMachineExist(ctx context.Context, capiMachine *clusterv1.Machine, mapiMachine *mapiv1beta1.Machine) (bool, error) { + namespace := r.CAPINamespace + name := mapiMachine.Name + + if capiMachine != nil { + name = capiMachine.Spec.InfrastructureRef.Name + namespace = capiMachine.Spec.InfrastructureRef.Namespace + } + + infraMachine, err := r.fetchCAPIInfraMachine(ctx, name, namespace) + if err != nil { + return false, fmt.Errorf("checking existence: %w", err) + } + + return infraMachine != nil, nil } //nolint:funlen,gocognit,cyclop diff --git a/pkg/controllers/machinesync/machine_sync_controller_test.go b/pkg/controllers/machinesync/machine_sync_controller_test.go index 4affc27a8..82f71e0c7 100644 --- a/pkg/controllers/machinesync/machine_sync_controller_test.go +++ b/pkg/controllers/machinesync/machine_sync_controller_test.go @@ -949,20 +949,19 @@ var _ = Describe("With a running MachineSync Reconciler", func() { }) Context("and the InfraMachine does not exist", func() { - It("should update the synchronized condition on the MAPI machine to False", func() { - // TODO: Revert this to a useful error message. - // We changed how fetchCAPIInfraResources behaves, so now we fail at a later point in the code. - // This still fails, the behaviour is the same - it's just a less useful error for an end user. - // We need to revisit fetchCAPIInfraResources. + It("should create the Cluster API InfraMachine", func() { Eventually(k.Object(mapiMachine), timeout).Should( HaveField("Status.Conditions", ContainElement( SatisfyAll( HaveField("Type", Equal(consts.SynchronizedCondition)), - HaveField("Status", Equal(corev1.ConditionFalse)), - HaveField("Reason", Equal("FailedToConvertCAPIMachineToMAPI")), - HaveField("Message", ContainSubstring("unexpected InfraMachine type, expected AWSMachine, got ")), + HaveField("Status", Equal(corev1.ConditionTrue)), + HaveField("Reason", Equal("ResourceSynchronized")), + HaveField("Message", ContainSubstring("Successfully synchronized CAPI Machine to MAPI")), ))), ) + + capiInfraMachine := awsv1resourcebuilder.AWSMachine().WithName(mapiMachine.Name).WithNamespace(capiNamespace.Name).Build() + Eventually(k.Get(capiInfraMachine), timeout).Should(Succeed()) }) }) }) diff --git a/pkg/controllers/machinesync/machine_sync_controller_unit_test.go b/pkg/controllers/machinesync/machine_sync_controller_unit_test.go new file mode 100644 index 000000000..14f30c7f5 --- /dev/null +++ b/pkg/controllers/machinesync/machine_sync_controller_unit_test.go @@ -0,0 +1,119 @@ +/* +Copyright 2024 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/ginkgo/v2" + . "github.com/onsi/gomega" + configv1 "github.com/openshift/api/config/v1" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + awsv1 "sigs.k8s.io/cluster-api-provider-aws/v2/api/v1beta2" + clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" +) + +// Unit tests for fetchCAPIInfraResources because reconciling deletion depends on it. +var _ = Describe("Unit tests for fetchCAPIInfraResources", func() { + var reconciler *MachineSyncReconciler + var capiMachine *clusterv1.Machine + + BeforeEach(func() { + capiMachine = &clusterv1.Machine{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-machine", + Namespace: "test-namespace", + }, + Spec: clusterv1.MachineSpec{ + ClusterName: "test-cluster", + InfrastructureRef: corev1.ObjectReference{ + Kind: "AWSMachine", + Name: "test-machine", + Namespace: "test-namespace", + }, + }, + } + + reconciler = &MachineSyncReconciler{ + Scheme: testEnv.Scheme, + Platform: configv1.AWSPlatformType, + } + }) + + Describe("when fetching Cluster API Infrastructure Resources", func() { + Context("when capiMachine is nil", func() { + It("should return nil and no error", func() { + reconciler.Client = fake.NewClientBuilder().WithScheme(testEnv.Scheme).Build() + infraCluster, infraMachine, err := reconciler.fetchCAPIInfraResources(ctx, nil) + Expect(err).ToNot(HaveOccurred()) + Expect(infraCluster).To(BeNil()) + Expect(infraMachine).To(BeNil()) + }) + }) + + Context("and Infrastructure Cluster is not present", func() { + It("should return nil for both infra cluster and infra machine", func() { + reconciler.Client = fake.NewClientBuilder().WithScheme(testEnv.Scheme).Build() + + infraCluster, infraMachine, err := reconciler.fetchCAPIInfraResources(ctx, capiMachine) + Expect(err).To(HaveOccurred()) + Expect(err).To(MatchError(ContainSubstring("failed to get Cluster API infrastructure cluster"))) + Expect(infraCluster).To(BeNil()) + Expect(infraMachine).To(BeNil()) + }) + }) + + Context("and Infrastructure is present", func() { + var objs []client.Object + BeforeEach(func() { + objs = []client.Object{ + &awsv1.AWSCluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: capiMachine.Spec.ClusterName, + Namespace: capiMachine.Namespace, + }, + }, + } + }) + + It("should return nil if infrastructure machine is not present", func() { + reconciler.Client = fake.NewClientBuilder().WithScheme(testEnv.Scheme).WithObjects(objs...).Build() + infraCluster, infraMachine, err := reconciler.fetchCAPIInfraResources(ctx, capiMachine) + + Expect(err).ToNot(HaveOccurred()) + Expect(infraCluster).ToNot(BeNil()) + Expect(infraMachine).To(BeNil()) + }) + + It("should return infrastructure machine if it is present", func() { + objs = append(objs, &awsv1.AWSMachine{ + ObjectMeta: metav1.ObjectMeta{ + Name: capiMachine.Name, + Namespace: capiMachine.Namespace, + }, + }) + reconciler.Client = fake.NewClientBuilder().WithScheme(testEnv.Scheme).WithObjects(objs...).Build() + infraCluster, infraMachine, err := reconciler.fetchCAPIInfraResources(ctx, capiMachine) + + Expect(err).ToNot(HaveOccurred()) + Expect(infraCluster).ToNot(BeNil()) + Expect(infraMachine).ToNot(BeNil()) + }) + }) + }) +})