From f7057ff7a7c4c7774fb1f9f835cd80a0563c4613 Mon Sep 17 00:00:00 2001
From: Ashley Dumaine <ashley.dumaine@gmail.com>
Date: Thu, 1 Feb 2024 10:06:19 -0500
Subject: [PATCH] address review comments

---
 cloud/scope/machine.go                 | 43 ++++++++++++++++++--------
 controller/linodemachine_controller.go | 10 ++++--
 2 files changed, 37 insertions(+), 16 deletions(-)

diff --git a/cloud/scope/machine.go b/cloud/scope/machine.go
index 81cb31239..1857400d0 100644
--- a/cloud/scope/machine.go
+++ b/cloud/scope/machine.go
@@ -2,26 +2,26 @@ package scope
 
 import (
 	"context"
-	b64 "encoding/base64"
 	"errors"
 	"fmt"
 
+	"github.com/linode/linodego"
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/types"
-
-	infrav1 "github.com/linode/cluster-api-provider-linode/api/v1alpha1"
-	"github.com/linode/linodego"
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
 	"sigs.k8s.io/cluster-api/util/patch"
 	"sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
+
+	infrav1alpha1 "github.com/linode/cluster-api-provider-linode/api/v1alpha1"
 )
 
 type MachineScopeParams struct {
 	Client        client.Client
 	Cluster       *clusterv1.Cluster
 	Machine       *clusterv1.Machine
-	LinodeCluster *infrav1.LinodeCluster
-	LinodeMachine *infrav1.LinodeMachine
+	LinodeCluster *infrav1alpha1.LinodeCluster
+	LinodeMachine *infrav1alpha1.LinodeMachine
 }
 
 type MachineScope struct {
@@ -31,8 +31,8 @@ type MachineScope struct {
 	Cluster       *clusterv1.Cluster
 	Machine       *clusterv1.Machine
 	LinodeClient  *linodego.Client
-	LinodeCluster *infrav1.LinodeCluster
-	LinodeMachine *infrav1.LinodeMachine
+	LinodeCluster *infrav1alpha1.LinodeCluster
+	LinodeMachine *infrav1alpha1.LinodeMachine
 }
 
 func validateMachineScopeParams(params MachineScopeParams) error {
@@ -75,10 +75,27 @@ func NewMachineScope(apiKey string, params MachineScopeParams) (*MachineScope, e
 	}, nil
 }
 
+// PatchObject persists the machine configuration and status.
+func (s *MachineScope) PatchObject(ctx context.Context) error {
+	return s.PatchHelper.Patch(ctx, s.LinodeMachine)
+}
+
+// Close closes the current scope persisting the machine configuration and status.
+func (s *MachineScope) Close(ctx context.Context) error {
+	return s.PatchObject(ctx)
+}
+
+// AddFinalizer adds a finalizer and immediately patches the object to avoid any race conditions
+func (s *MachineScope) AddFinalizer(ctx context.Context) error {
+	controllerutil.AddFinalizer(s.LinodeMachine, infrav1alpha1.GroupVersion.String())
+
+	return s.Close(ctx)
+}
+
 // GetBootstrapData returns the bootstrap data from the secret in the Machine's bootstrap.dataSecretName.
-func (m *MachineScope) GetBootstrapData(ctx context.Context) (string, error) {
+func (m *MachineScope) GetBootstrapData(ctx context.Context) ([]byte, error) {
 	if m.Machine.Spec.Bootstrap.DataSecretName == nil {
-		return "", fmt.Errorf(
+		return []byte{}, fmt.Errorf(
 			"bootstrap data secret is nil for LinodeMachine %s/%s",
 			m.LinodeMachine.Namespace,
 			m.LinodeMachine.Name,
@@ -88,7 +105,7 @@ func (m *MachineScope) GetBootstrapData(ctx context.Context) (string, error) {
 	secret := &corev1.Secret{}
 	key := types.NamespacedName{Namespace: m.LinodeMachine.Namespace, Name: *m.Machine.Spec.Bootstrap.DataSecretName}
 	if err := m.client.Get(ctx, key, secret); err != nil {
-		return "", fmt.Errorf(
+		return []byte{}, fmt.Errorf(
 			"failed to retrieve bootstrap data secret for LinodeMachine %s/%s",
 			m.LinodeMachine.Namespace,
 			m.LinodeMachine.Name,
@@ -97,12 +114,12 @@ func (m *MachineScope) GetBootstrapData(ctx context.Context) (string, error) {
 
 	value, ok := secret.Data["value"]
 	if !ok {
-		return "", fmt.Errorf(
+		return []byte{}, fmt.Errorf(
 			"bootstrap data secret value key is missing for LinodeMachine %s/%s",
 			m.LinodeMachine.Namespace,
 			m.LinodeMachine.Name,
 		)
 	}
 
-	return b64.StdEncoding.EncodeToString(value), nil
+	return value, nil
 }
diff --git a/controller/linodemachine_controller.go b/controller/linodemachine_controller.go
index 9e75f37ba..7e6fff34e 100644
--- a/controller/linodemachine_controller.go
+++ b/controller/linodemachine_controller.go
@@ -18,6 +18,7 @@ package controller
 
 import (
 	"context"
+	b64 "encoding/base64"
 	"encoding/json"
 	"errors"
 	"fmt"
@@ -202,7 +203,8 @@ func (r *LinodeMachineReconciler) reconcile(
 			r.Recorder.Event(machineScope.LinodeMachine, corev1.EventTypeWarning, string(failureReason), err.Error())
 		}
 
-		if patchErr := machineScope.PatchHelper.Patch(ctx, machineScope.LinodeMachine); patchErr != nil && client.IgnoreNotFound(patchErr) != nil {
+		// Always close the scope when exiting this function so we can persist any LinodeMachine changes.
+		if patchErr := machineScope.Close(ctx); patchErr != nil && client.IgnoreNotFound(patchErr) != nil {
 			logger.Error(patchErr, "failed to patch LinodeMachine")
 
 			err = errors.Join(err, patchErr)
@@ -210,7 +212,9 @@ func (r *LinodeMachineReconciler) reconcile(
 	}()
 
 	// Add the finalizer if not already there
-	controllerutil.AddFinalizer(machineScope.LinodeMachine, infrav1.GroupVersion.String())
+	if err := machineScope.AddFinalizer(ctx); err != nil {
+		return res, err
+	}
 
 	// Delete
 	if !machineScope.LinodeMachine.ObjectMeta.DeletionTimestamp.IsZero() {
@@ -295,7 +299,7 @@ func (*LinodeMachineReconciler) reconcileCreate(ctx context.Context, machineScop
 			return nil, err
 		}
 		createConfig.Metadata = &linodego.InstanceMetadataOptions{
-			UserData: bootstrapData,
+			UserData: b64.StdEncoding.EncodeToString(bootstrapData),
 		}
 
 		if linodeInstance, err = machineScope.LinodeClient.CreateInstance(ctx, *createConfig); err != nil {