use providerID string as-is

api/v1beta1/index/machine.go

@@ -26,7 +26,6 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
-	"sigs.k8s.io/cluster-api/controllers/noderefutil"
 )
 
 const (
@@ -86,10 +85,5 @@ func machineByProviderID(o client.Object) []string {
 		return nil
 	}
 
-	providerID, err := noderefutil.NewProviderID(*machine.Spec.ProviderID)
-	if err != nil {
-		// Failed to create providerID, skipping.
-		return nil
-	}
-	return []string{providerID.IndexKey()}
+	return []string{*machine.Spec.ProviderID}
 }

api/v1beta1/index/machine_test.go

@@ -25,7 +25,6 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
-	"sigs.k8s.io/cluster-api/controllers/noderefutil"
 )
 
 func TestIndexMachineByNodeName(t *testing.T) {
@@ -62,9 +61,7 @@ func TestIndexMachineByNodeName(t *testing.T) {
 }
 
 func TestIndexMachineByProviderID(t *testing.T) {
-	validProviderID, err := noderefutil.NewProviderID("aws://region/zone/id")
-	g := NewWithT(t)
-	g.Expect(err).ToNot(HaveOccurred())
+	validProviderID := "aws://region/zone/id"
 
 	testCases := []struct {
 		name     string
@@ -80,7 +77,7 @@ func TestIndexMachineByProviderID(t *testing.T) {
 			name: "Machine has invalid providerID",
 			object: &clusterv1.Machine{
 				Spec: clusterv1.MachineSpec{
-					ProviderID: pointer.String("invalid"),
+					ProviderID: pointer.String(""),
 				},
 			},
 			expected: nil,
@@ -89,10 +86,10 @@ func TestIndexMachineByProviderID(t *testing.T) {
 			name: "Machine has valid providerID",
 			object: &clusterv1.Machine{
 				Spec: clusterv1.MachineSpec{
-					ProviderID: pointer.String(validProviderID.String()),
+					ProviderID: pointer.String(validProviderID),
 				},
 			},
-			expected: []string{validProviderID.IndexKey()},
+			expected: []string{validProviderID},
 		},
 	}
 

api/v1beta1/index/machinepool.go

@@ -24,7 +24,6 @@ import (
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
-	"sigs.k8s.io/cluster-api/controllers/noderefutil"
 	expv1 "sigs.k8s.io/cluster-api/exp/api/v1beta1"
 )
 
@@ -93,12 +92,11 @@ func machinePoolByProviderID(o client.Object) []string {
 
 	providerIDs := make([]string, 0, len(machinepool.Spec.ProviderIDList))
 	for _, id := range machinepool.Spec.ProviderIDList {
-		providerID, err := noderefutil.NewProviderID(id)
-		if err != nil {
-			// Failed to create providerID, skipping.
+		if id == "" {
+			// Valid providerID not found, skipping.
 			continue
 		}
-		providerIDs = append(providerIDs, providerID.IndexKey())
+		providerIDs = append(providerIDs, id)
 	}
 
 	return providerIDs

api/v1beta1/index/machinepool_test.go

@@ -23,7 +23,6 @@ import (
 	corev1 "k8s.io/api/core/v1"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
-	"sigs.k8s.io/cluster-api/controllers/noderefutil"
 	expv1 "sigs.k8s.io/cluster-api/exp/api/v1beta1"
 )
 
@@ -66,11 +65,8 @@ func TestIndexMachinePoolByNodeName(t *testing.T) {
 }
 
 func TestIndexMachinePoolByProviderID(t *testing.T) {
-	g := NewWithT(t)
-	validProviderID, err := noderefutil.NewProviderID("aws://region/zone/1")
-	g.Expect(err).ToNot(HaveOccurred())
-	otherValidProviderID, err := noderefutil.NewProviderID("aws://region/zone/2")
-	g.Expect(err).ToNot(HaveOccurred())
+	validProviderID := "aws://region/zone/1"
+	otherValidProviderID := "aws://region/zone/2"
 
 	testCases := []struct {
 		name     string
@@ -86,7 +82,7 @@ func TestIndexMachinePoolByProviderID(t *testing.T) {
 			name: "MachinePool has invalid providerID",
 			object: &expv1.MachinePool{
 				Spec: expv1.MachinePoolSpec{
-					ProviderIDList: []string{"invalid"},
+					ProviderIDList: []string{""},
 				},
 			},
 			expected: []string{},
@@ -95,10 +91,10 @@ func TestIndexMachinePoolByProviderID(t *testing.T) {
 			name: "MachinePool has valid providerIDs",
 			object: &expv1.MachinePool{
 				Spec: expv1.MachinePoolSpec{
-					ProviderIDList: []string{validProviderID.String(), otherValidProviderID.String()},
+					ProviderIDList: []string{validProviderID, otherValidProviderID},
 				},
 			},
-			expected: []string{validProviderID.IndexKey(), otherValidProviderID.IndexKey()},
+			expected: []string{validProviderID, otherValidProviderID},
 		},
 	}
 

api/v1beta1/index/node.go

@@ -21,8 +21,6 @@ import (
 
 	corev1 "k8s.io/api/core/v1"
 	"sigs.k8s.io/controller-runtime/pkg/client"
-
-	"sigs.k8s.io/cluster-api/controllers/noderefutil"
 )
 
 const (
@@ -42,10 +40,5 @@ func NodeByProviderID(o client.Object) []string {
 		return nil
 	}
 
-	providerID, err := noderefutil.NewProviderID(node.Spec.ProviderID)
-	if err != nil {
-		// Failed to create providerID, skipping.
-		return nil
-	}
-	return []string{providerID.IndexKey()}
+	return []string{node.Spec.ProviderID}
 }

api/v1beta1/index/node_test.go

@@ -22,14 +22,10 @@ import (
 	. "github.com/onsi/gomega"
 	corev1 "k8s.io/api/core/v1"
 	"sigs.k8s.io/controller-runtime/pkg/client"
-
-	"sigs.k8s.io/cluster-api/controllers/noderefutil"
 )
 
 func TestIndexNodeByProviderID(t *testing.T) {
-	validProviderID, err := noderefutil.NewProviderID("aws://region/zone/id")
-	g := NewWithT(t)
-	g.Expect(err).ToNot(HaveOccurred())
+	validProviderID := "aws://region/zone/id"
 
 	testCases := []struct {
 		name     string
@@ -45,7 +41,7 @@ func TestIndexNodeByProviderID(t *testing.T) {
 			name: "Node has invalid providerID",
 			object: &corev1.Node{
 				Spec: corev1.NodeSpec{
-					ProviderID: "invalid",
+					ProviderID: "",
 				},
 			},
 			expected: nil,
@@ -54,10 +50,10 @@ func TestIndexNodeByProviderID(t *testing.T) {
 			name: "Node has valid providerID",
 			object: &corev1.Node{
 				Spec: corev1.NodeSpec{
-					ProviderID: validProviderID.String(),
+					ProviderID: validProviderID,
 				},
 			},
-			expected: []string{validProviderID.IndexKey()},
+			expected: []string{validProviderID},
 		},
 	}
 

controllers/noderefutil/providerid.go

@@ -14,6 +14,8 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */
 
+// The ProviderID type is deprecated and unused by Cluster API internally.
+// It will be removed entirely in a future release.
 // Package noderefutil implements NodeRef utils.
 package noderefutil
 

controllers/noderefutil/providerid_test.go

@@ -14,6 +14,8 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */
 
+// The ProviderID type is deprecated and unused by Cluster API internally.
+// It will be removed entirely in a future release.
 package noderefutil
 
 import (

exp/internal/controllers/machinepool_controller.go

@@ -41,7 +41,6 @@ import (
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
 	"sigs.k8s.io/cluster-api/api/v1beta1/index"
 	"sigs.k8s.io/cluster-api/controllers/external"
-	"sigs.k8s.io/cluster-api/controllers/noderefutil"
 	"sigs.k8s.io/cluster-api/controllers/remote"
 	expv1 "sigs.k8s.io/cluster-api/exp/api/v1beta1"
 	"sigs.k8s.io/cluster-api/util"
@@ -352,15 +351,14 @@ func (r *MachinePoolReconciler) nodeToMachinePool(o client.Object) []reconcile.R
 
 	// Otherwise let's match by providerID. This is useful when e.g the NodeRef has not been set yet.
 	// Match by providerID
-	nodeProviderID, err := noderefutil.NewProviderID(node.Spec.ProviderID)
-	if err != nil {
+	if node.Spec.ProviderID == "" {
 		return nil
 	}
 	machinePoolList = &expv1.MachinePoolList{}
 	if err := r.Client.List(
 		context.TODO(),
 		machinePoolList,
-		append(filters, client.MatchingFields{index.MachinePoolProviderIDField: nodeProviderID.IndexKey()})...); err != nil {
+		append(filters, client.MatchingFields{index.MachinePoolProviderIDField: node.Spec.ProviderID})...); err != nil {
 		return nil
 	}
 

exp/internal/controllers/machinepool_controller_noderef.go

@@ -27,7 +27,6 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
-	"sigs.k8s.io/cluster-api/controllers/noderefutil"
 	"sigs.k8s.io/cluster-api/controllers/remote"
 	expv1 "sigs.k8s.io/cluster-api/exp/api/v1beta1"
 	"sigs.k8s.io/cluster-api/internal/util/taints"
@@ -132,21 +131,19 @@ func (r *MachinePoolReconciler) deleteRetiredNodes(ctx context.Context, c client
 			continue
 		}
 
-		nodeProviderID, err := noderefutil.NewProviderID(node.Spec.ProviderID)
-		if err != nil {
-			log.V(2).Info("Failed to parse ProviderID, skipping", "err", err, "providerID", node.Spec.ProviderID)
+		if node.Spec.ProviderID == "" {
+			log.V(2).Info("No ProviderID detected, skipping", "providerID", node.Spec.ProviderID)
 			continue
 		}
 
-		nodeRefsMap[nodeProviderID.String()] = node
+		nodeRefsMap[node.Spec.ProviderID] = node
 	}
 	for _, providerID := range providerIDList {
-		pid, err := noderefutil.NewProviderID(providerID)
-		if err != nil {
-			log.V(2).Info("Failed to parse ProviderID, skipping", "err", err, "providerID", providerID)
+		if providerID == "" {
+			log.V(2).Info("No ProviderID detected, skipping", "providerID", providerID)
 			continue
 		}
-		delete(nodeRefsMap, pid.String())
+		delete(nodeRefsMap, providerID)
 	}
 	for _, node := range nodeRefsMap {
 		if err := c.Delete(ctx, node); err != nil {
@@ -168,13 +165,12 @@ func (r *MachinePoolReconciler) getNodeReferences(ctx context.Context, c client.
 		}
 
 		for _, node := range nodeList.Items {
-			nodeProviderID, err := noderefutil.NewProviderID(node.Spec.ProviderID)
-			if err != nil {
-				log.V(2).Info("Failed to parse ProviderID, skipping", "err", err, "providerID", node.Spec.ProviderID)
+			if node.Spec.ProviderID == "" {
+				log.V(2).Info("No ProviderID detected, skipping", "providerID", node.Spec.ProviderID)
 				continue
 			}
 
-			nodeRefsMap[nodeProviderID.String()] = node
+			nodeRefsMap[node.Spec.ProviderID] = node
 		}
 
 		if nodeList.Continue == "" {
@@ -184,12 +180,11 @@ func (r *MachinePoolReconciler) getNodeReferences(ctx context.Context, c client.
 
 	var nodeRefs []corev1.ObjectReference
 	for _, providerID := range providerIDList {
-		pid, err := noderefutil.NewProviderID(providerID)
-		if err != nil {
-			log.V(2).Info("Failed to parse ProviderID, skipping", "err", err, "providerID", providerID)
+		if providerID == "" {
+			log.V(2).Info("No ProviderID detected, skipping", "providerID", providerID)
 			continue
 		}
-		if node, ok := nodeRefsMap[pid.String()]; ok {
+		if node, ok := nodeRefsMap[providerID]; ok {
 			available++
 			if nodeIsReady(&node) {
 				ready++

internal/controllers/machine/machine_controller.go

@@ -837,15 +837,11 @@ func (r *Reconciler) nodeToMachine(o client.Object) []reconcile.Request {
 
 	// Otherwise let's match by providerID. This is useful when e.g the NodeRef has not been set yet.
 	// Match by providerID
-	nodeProviderID, err := noderefutil.NewProviderID(node.Spec.ProviderID)
-	if err != nil {
-		return nil
-	}
 	machineList = &clusterv1.MachineList{}
 	if err := r.Client.List(
 		context.TODO(),
 		machineList,
-		append(filters, client.MatchingFields{index.MachineProviderIDField: nodeProviderID.IndexKey()})...); err != nil {
+		append(filters, client.MatchingFields{index.MachineProviderIDField: node.Spec.ProviderID})...); err != nil {
 		return nil
 	}
 

internal/controllers/machine/machine_controller_noderef.go

@@ -30,7 +30,6 @@ import (
 
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
 	"sigs.k8s.io/cluster-api/api/v1beta1/index"
-	"sigs.k8s.io/cluster-api/controllers/noderefutil"
 	"sigs.k8s.io/cluster-api/internal/util/taints"
 	"sigs.k8s.io/cluster-api/util"
 	"sigs.k8s.io/cluster-api/util/annotations"
@@ -57,18 +56,13 @@ func (r *Reconciler) reconcileNode(ctx context.Context, cluster *clusterv1.Clust
 		return ctrl.Result{}, nil
 	}
 
-	providerID, err := noderefutil.NewProviderID(*machine.Spec.ProviderID)
-	if err != nil {
-		return ctrl.Result{}, err
-	}
-
 	remoteClient, err := r.Tracker.GetClient(ctx, util.ObjectKey(cluster))
 	if err != nil {
 		return ctrl.Result{}, err
 	}
 
 	// Even if Status.NodeRef exists, continue to do the following checks to make sure Node is healthy
-	node, err := r.getNode(ctx, remoteClient, providerID)
+	node, err := r.getNode(ctx, remoteClient, *machine.Spec.ProviderID)
 	if err != nil {
 		if err == ErrNodeNotFound {
 			// While a NodeRef is set in the status, failing to get that node means the node is deleted.
@@ -94,7 +88,7 @@ func (r *Reconciler) reconcileNode(ctx context.Context, cluster *clusterv1.Clust
 			Name:       node.Name,
 			UID:        node.UID,
 		}
-		log.Info("Infrastructure provider reporting spec.providerID, Kubernetes node is now available", machine.Spec.InfrastructureRef.Kind, klog.KRef(machine.Spec.InfrastructureRef.Namespace, machine.Spec.InfrastructureRef.Name), "providerID", providerID, "node", klog.KRef("", machine.Status.NodeRef.Name))
+		log.Info("Infrastructure provider reporting spec.providerID, Kubernetes node is now available", machine.Spec.InfrastructureRef.Kind, klog.KRef(machine.Spec.InfrastructureRef.Namespace, machine.Spec.InfrastructureRef.Name), "node", klog.KRef("", machine.Status.NodeRef.Name))
 		r.recorder.Event(machine, corev1.EventTypeNormal, "SuccessfulSetNodeRef", machine.Status.NodeRef.Name)
 	}
 
@@ -199,10 +193,9 @@ func summarizeNodeConditions(node *corev1.Node) (corev1.ConditionStatus, string)
 	return corev1.ConditionUnknown, message
 }
 
-func (r *Reconciler) getNode(ctx context.Context, c client.Reader, providerID *noderefutil.ProviderID) (*corev1.Node, error) {
-	log := ctrl.LoggerFrom(ctx, "providerID", providerID)
+func (r *Reconciler) getNode(ctx context.Context, c client.Reader, providerID string) (*corev1.Node, error) {
 	nodeList := corev1.NodeList{}
-	if err := c.List(ctx, &nodeList, client.MatchingFields{index.NodeProviderIDField: providerID.IndexKey()}); err != nil {
+	if err := c.List(ctx, &nodeList, client.MatchingFields{index.NodeProviderIDField: providerID}); err != nil {
 		return nil, err
 	}
 	if len(nodeList.Items) == 0 {
@@ -213,14 +206,8 @@ func (r *Reconciler) getNode(ctx context.Context, c client.Reader, providerID *n
 				return nil, err
 			}
 
-			for key, node := range nl.Items {
-				nodeProviderID, err := noderefutil.NewProviderID(node.Spec.ProviderID)
-				if err != nil {
-					log.Error(err, "Failed to parse ProviderID", "Node", klog.KRef("", nl.Items[key].GetName()))
-					continue
-				}
-
-				if providerID.Equals(nodeProviderID) {
+			for _, node := range nl.Items {
+				if providerID == node.Spec.ProviderID {
 					return &node, nil
 				}
 			}
@@ -234,7 +221,7 @@ func (r *Reconciler) getNode(ctx context.Context, c client.Reader, providerID *n
 	}
 
 	if len(nodeList.Items) != 1 {
-		return nil, fmt.Errorf("unexpectedly found more than one Node matching the providerID %s", providerID.String())
+		return nil, fmt.Errorf("unexpectedly found more than one Node matching the providerID %s", providerID)
 	}
 
 	return &nodeList.Items[0], nil