diff --git a/api/v1beta1/index/machine.go b/api/v1beta1/index/machine.go index efffee9e27c3..9a3836ae71da 100644 --- 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 ( @@ -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} } diff --git a/api/v1beta1/index/machine_test.go b/api/v1beta1/index/machine_test.go index 33ed9b1dda77..e96519e537c2 100644 --- 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 index 612dafe65cfb..b877d5cbdaca 100644 --- 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 index eaaab680db31..09ab8769acc8 100644 --- 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 index 6eed2de6545f..64fa4af29065 100644 --- 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 index 8137c3f99026..a0e69d536bea 100644 --- 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 index 6c6dede690b8..7ca4570ba283 100644 --- a/controllers/noderefutil/providerid.go +++ b/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 diff --git a/controllers/noderefutil/providerid_test.go b/controllers/noderefutil/providerid_test.go index 77a77a831a4f..1572c165985c 100644 --- 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 index 97cd4946a982..555a7bd5b79d 100644 --- 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 index 6b8f953b1a17..14c40ce64add 100644 --- 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++ diff --git a/internal/controllers/machine/machine_controller.go b/internal/controllers/machine/machine_controller.go index 4335bac6c2d4..1d355bf4ceee 100644 --- a/internal/controllers/machine/machine_controller.go +++ b/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 } diff --git a/internal/controllers/machine/machine_controller_noderef.go b/internal/controllers/machine/machine_controller_noderef.go index 796b6c764791..2b9afc3f9d6f 100644 --- a/internal/controllers/machine/machine_controller_noderef.go +++ b/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 diff --git a/internal/controllers/machine/machine_controller_noderef_test.go b/internal/controllers/machine/machine_controller_noderef_test.go index 3c0a72f53169..d1e35459f266 100644 --- a/internal/controllers/machine/machine_controller_noderef_test.go +++ b/internal/controllers/machine/machine_controller_noderef_test.go @@ -31,7 +31,6 @@ import ( "sigs.k8s.io/controller-runtime/pkg/reconcile" clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" - "sigs.k8s.io/cluster-api/controllers/noderefutil" "sigs.k8s.io/cluster-api/controllers/remote" "sigs.k8s.io/cluster-api/util" "sigs.k8s.io/cluster-api/util/kubeconfig" @@ -154,10 +153,7 @@ func TestGetNode(t *testing.T) { remoteClient, err := r.Tracker.GetClient(ctx, util.ObjectKey(testCluster)) g.Expect(err).ToNot(HaveOccurred()) - providerID, err := noderefutil.NewProviderID(tc.providerIDInput) - g.Expect(err).ToNot(HaveOccurred()) - - node, err := r.getNode(ctx, remoteClient, providerID) + node, err := r.getNode(ctx, remoteClient, tc.providerIDInput) if tc.error != nil { g.Expect(err).To(Equal(tc.error)) return