Add tests for issue-290

controllers/infrastructure/byomachine_controller.go

@@ -42,7 +42,7 @@ import (
 
 const (
 	ProviderIDPrefix       = "byoh://"
-	providerIDSuffixLength = 6
+	ProviderIDSuffixLength = 6
 	RequeueForbyohost      = 10 * time.Second
 )
 
@@ -259,27 +259,25 @@ func (r *ByoMachineReconciler) reconcileNormal(ctx context.Context, machineScope
 		r.Recorder.Eventf(machineScope.ByoMachine, corev1.EventTypeNormal, "ByoHostAttachSucceeded", "Attached ByoHost %s", machineScope.ByoHost.Name)
 	}
 
-	if machineScope.ByoMachine.Spec.ProviderID == "" {
-		logger.Info("Updating Node with ProviderID")
-		remoteClient, err := r.getRemoteClient(ctx, machineScope.ByoMachine)
-		if err != nil {
-			logger.Error(err, "failed to get remote client")
-			return ctrl.Result{}, err
-		}
-
-		providerID, err := r.setNodeProviderID(ctx, remoteClient, machineScope.ByoHost)
-		if err != nil {
-			logger.Error(err, "failed to set node providerID")
-			r.Recorder.Eventf(machineScope.ByoMachine, corev1.EventTypeWarning, "SetNodeProviderFailed", "Node %s does not exist", machineScope.ByoHost.Name)
-			return ctrl.Result{}, err
-		}
+	logger.Info("Updating Node with ProviderID")
+	remoteClient, err := r.getRemoteClient(ctx, machineScope.ByoMachine)
+	if err != nil {
+		logger.Error(err, "failed to get remote client")
+		return ctrl.Result{}, err
+	}
 
-		machineScope.ByoMachine.Spec.ProviderID = providerID
-		machineScope.ByoMachine.Status.Ready = true
-		conditions.MarkTrue(machineScope.ByoMachine, infrav1.BYOHostReady)
-		r.Recorder.Eventf(machineScope.ByoMachine, corev1.EventTypeNormal, "NodeProvisionedSucceeded", "Provisioned Node %s", machineScope.ByoHost.Name)
+	providerID, err := r.setNodeProviderID(ctx, remoteClient, machineScope.ByoHost)
+	if err != nil {
+		logger.Error(err, "failed to set node providerID")
+		r.Recorder.Eventf(machineScope.ByoMachine, corev1.EventTypeWarning, "SetNodeProviderFailed", "Node %s does not exist", machineScope.ByoHost.Name)
+		return ctrl.Result{}, err
 	}
 
+	machineScope.ByoMachine.Spec.ProviderID = providerID
+	machineScope.ByoMachine.Status.Ready = true
+	conditions.MarkTrue(machineScope.ByoMachine, infrav1.BYOHostReady)
+	r.Recorder.Eventf(machineScope.ByoMachine, corev1.EventTypeNormal, "NodeProvisionedSucceeded", "Provisioned Node %s", machineScope.ByoHost.Name)
+
 	return ctrl.Result{}, nil
 }
 
@@ -375,7 +373,7 @@ func (r *ByoMachineReconciler) setNodeProviderID(ctx context.Context, remoteClie
 		return "", err
 	}
 
-	node.Spec.ProviderID = fmt.Sprintf("%s%s/%s", ProviderIDPrefix, host.Name, util.RandomString(providerIDSuffixLength))
+	node.Spec.ProviderID = fmt.Sprintf("%s%s/%s", ProviderIDPrefix, host.Name, util.RandomString(ProviderIDSuffixLength))
 
 	return node.Spec.ProviderID, helper.Patch(ctx, node)
 }

controllers/infrastructure/byomachine_controller_test.go

@@ -18,6 +18,7 @@ import (
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/client-go/kubernetes/scheme"
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
+	"sigs.k8s.io/cluster-api/util"
 	"sigs.k8s.io/cluster-api/util/annotations"
 	"sigs.k8s.io/cluster-api/util/conditions"
 	"sigs.k8s.io/cluster-api/util/patch"
@@ -117,6 +118,37 @@ var _ = Describe("Controllers/ByomachineController", func() {
 			Expect(err).To(MatchError("nodes \"" + byoHost.Name + "\" not found"))
 		})
 
+		Context("When node.Spec.ProviderID is already set", func() {
+
+			BeforeEach(func() {
+				byoHost = builder.ByoHost(defaultNamespace, "test-node-providerid-host").Build()
+				Expect(k8sClientUncached.Create(ctx, byoHost)).Should(Succeed())
+				WaitForObjectsToBePopulatedInCache(byoHost)
+			})
+
+			AfterEach(func() {
+				Expect(k8sClientUncached.Delete(ctx, byoHost)).ToNot(HaveOccurred())
+			})
+
+			It("should not return error when node.Spec.ProviderID is with correct value", func() {
+				node := builder.Node(defaultNamespace, byoHost.Name).
+					WithProviderID(fmt.Sprintf("%s%s/%s", controllers.ProviderIDPrefix, byoHost.Name, util.RandomString(controllers.ProviderIDSuffixLength))).
+					Build()
+				Expect(clientFake.Create(ctx, node)).Should(Succeed())
+				_, err := reconciler.Reconcile(ctx, reconcile.Request{NamespacedName: byoMachineLookupKey})
+				Expect(err).ToNot(HaveOccurred())
+			})
+
+			It("should return error when node.Spec.ProviderID has stale value", func() {
+				node := builder.Node(defaultNamespace, byoHost.Name).
+					WithProviderID(fmt.Sprintf("%sanother-host/%s", controllers.ProviderIDPrefix, util.RandomString(controllers.ProviderIDSuffixLength))).
+					Build()
+				Expect(clientFake.Create(ctx, node)).Should(Succeed())
+				_, err := reconciler.Reconcile(ctx, reconcile.Request{NamespacedName: byoMachineLookupKey})
+				Expect(err).To(MatchError("invalid format for node.Spec.ProviderID"))
+			})
+		})
+
 		Context("When BYO Hosts are not available", func() {
 			It("should mark BYOHostReady as False", func() {
 				_, err := reconciler.Reconcile(ctx, reconcile.Request{NamespacedName: byoMachineLookupKey})

test/builder/builders.go

@@ -332,8 +332,9 @@ func (s *SecretBuilder) Build() *corev1.Secret {
 
 // NodeBuilder holds the variables and objects required to build a corev1.Node
 type NodeBuilder struct {
-	namespace string
-	name      string
+	namespace  string
+	name       string
+	providerID string
 }
 
 // Node returns a NodeBuilder with the given name and namespace
@@ -344,6 +345,12 @@ func Node(namespace, name string) *NodeBuilder {
 	}
 }
 
+// WithProviderID adds the passed providerID to the ByoHostBuilder
+func (n *NodeBuilder) WithProviderID(providerID string) *NodeBuilder {
+	n.providerID = providerID
+	return n
+}
+
 // Build returns a Node with the attributes added to the NodeBuilder
 func (n *NodeBuilder) Build() *corev1.Node {
 	node := &corev1.Node{
@@ -355,7 +362,9 @@ func (n *NodeBuilder) Build() *corev1.Node {
 			Name:      n.name,
 			Namespace: n.namespace,
 		},
-		Spec:   corev1.NodeSpec{},
+		Spec: corev1.NodeSpec{
+			ProviderID: n.providerID,
+		},
 		Status: corev1.NodeStatus{},
 	}