kubernetes-sigs · k8s-ci-robot · May 2, 2023 · Apr 27, 2023
diff --git a/api/v1beta1/index/machine.go b/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 (
@@ -82,14 +81,11 @@ func machineByProviderID(o client.Object) []string {
 		panic(fmt.Sprintf("Expected a Machine but got a %T", o))
 	}
 
-	if pointer.StringDeref(machine.Spec.ProviderID, "") == "" {
-		return nil
-	}
+	providerID := pointer.StringDeref(machine.Spec.ProviderID, "")
 
-	providerID, err := noderefutil.NewProviderID(*machine.Spec.ProviderID)
-	if err != nil {
-		// Failed to create providerID, skipping.
+	if providerID == "" {
 		return nil
 	}
-	return []string{providerID.IndexKey()}
+
+	return []string{providerID}
 }
diff --git a/api/v1beta1/index/machine_test.go b/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},
 		},
 	}
 

diff --git a/api/v1beta1/index/machinepool.go b/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

diff --git a/api/v1beta1/index/machinepool_test.go b/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},
 		},
 	}
 

diff --git a/api/v1beta1/index/node.go b/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}
 }
diff --git a/api/v1beta1/index/node_test.go b/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},
 		},
 	}
 

diff --git a/controllers/noderefutil/providerid.go b/controllers/noderefutil/providerid.go
@@ -15,6 +15,8 @@ limitations under the License.
 */
 
 // Package noderefutil implements NodeRef utils.
+// The ProviderID type is deprecated and unused by Cluster API internally.
+// It will be removed entirely in a future release.
 package noderefutil
 
 import (
@@ -25,14 +27,20 @@ import (
 
 var (
 	// ErrEmptyProviderID means that the provider id is empty.
+	//
+	// Deprecated: This var is going to be removed in a future release.
 	ErrEmptyProviderID = errors.New("providerID is empty")
 
 	// ErrInvalidProviderID means that the provider id has an invalid form.
+	//
+	// Deprecated: This var is going to be removed in a future release.
 	ErrInvalidProviderID = errors.New("providerID must be of the form <cloudProvider>://<optional>/<segments>/<provider id>")
 )
 
 // ProviderID is a struct representation of a Kubernetes ProviderID.
 // Format: cloudProvider://optional/segments/etc/id
+//
+// Deprecated: This struct is going to be removed in a future release.
 type ProviderID struct {
 	original      string
 	cloudProvider string
@@ -48,6 +56,8 @@ type ProviderID struct {
 var providerIDRegex = regexp.MustCompile("^[^:]+://.*[^/]$")
 
 // NewProviderID parses the input string and returns a new ProviderID.
+//
+// Deprecated: This constructor is going to be removed in a future release.
 func NewProviderID(id string) (*ProviderID, error) {
 	if id == "" {
 		return nil, ErrEmptyProviderID
@@ -77,32 +87,44 @@ func NewProviderID(id string) (*ProviderID, error) {
 }
 
 // CloudProvider returns the cloud provider portion of the ProviderID.
+//
+// Deprecated: This method is going to be removed in a future release.
 func (p *ProviderID) CloudProvider() string {
 	return p.cloudProvider
 }
 
 // ID returns the identifier portion of the ProviderID.
+//
+// Deprecated: This method is going to be removed in a future release.
 func (p *ProviderID) ID() string {
 	return p.id
 }
 
 // Equals returns true if this ProviderID string matches another ProviderID string.
+//
+// Deprecated: This method is going to be removed in a future release.
 func (p *ProviderID) Equals(o *ProviderID) bool {
 	return p.String() == o.String()
 }
 
 // String returns the string representation of this object.
+//
+// Deprecated: This method is going to be removed in a future release.
 func (p ProviderID) String() string {
 	return p.original
 }
 
 // Validate returns true if the provider id is valid.
+//
+// Deprecated: This method is going to be removed in a future release.
 func (p *ProviderID) Validate() bool {
 	return p.CloudProvider() != "" && p.ID() != ""
 }
 
 // IndexKey returns the required level of uniqueness
 // to represent and index machines uniquely from their node providerID.
+//
+// Deprecated: This method is going to be removed in a future release.
 func (p *ProviderID) IndexKey() string {
 	return p.String()
 }
diff --git a/controllers/noderefutil/providerid_test.go b/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 (

diff --git a/exp/internal/controllers/machinepool_controller.go b/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
 	}
 

diff --git a/exp/internal/controllers/machinepool_controller_noderef.go b/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++