Change remote.NewClusterClient to return a controller-runtime client.Client

Author: noamran

bootstrap/kubeadm/controllers/kubeadmconfig_controller.go

@@ -26,16 +26,16 @@ import (
 	"github.com/pkg/errors"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/client-go/tools/clientcmd"
+	"k8s.io/apimachinery/pkg/runtime"
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1alpha3"
 	bootstrapv1 "sigs.k8s.io/cluster-api/bootstrap/kubeadm/api/v1alpha2"
 	"sigs.k8s.io/cluster-api/bootstrap/kubeadm/cloudinit"
 	internalcluster "sigs.k8s.io/cluster-api/bootstrap/kubeadm/internal/cluster"
 	"sigs.k8s.io/cluster-api/bootstrap/kubeadm/internal/locking"
 	kubeadmv1beta1 "sigs.k8s.io/cluster-api/bootstrap/kubeadm/types/v1beta1"
+	"sigs.k8s.io/cluster-api/controllers/remote"
 	capierrors "sigs.k8s.io/cluster-api/errors"
 	"sigs.k8s.io/cluster-api/util"
-	"sigs.k8s.io/cluster-api/util/kubeconfig"
 	"sigs.k8s.io/cluster-api/util/patch"
 	"sigs.k8s.io/cluster-api/util/secret"
 	ctrl "sigs.k8s.io/controller-runtime"
@@ -59,9 +59,10 @@ type KubeadmConfigReconciler struct {
 	Client          client.Client
 	KubeadmInitLock InitLocker
 	Log             logr.Logger
+	scheme          *runtime.Scheme
 
 	// for testing
-	remoteClient func(client.Client, *clusterv1.Cluster) (client.Client, error)
+	remoteClient func(client.Client, *clusterv1.Cluster, *runtime.Scheme) (client.Client, error)
 }
 
 // SetupWithManager sets up the reconciler with the Manager.
@@ -70,10 +71,12 @@ func (r *KubeadmConfigReconciler) SetupWithManager(mgr ctrl.Manager) error {
 		r.KubeadmInitLock = locking.NewControlPlaneInitMutex(ctrl.Log.WithName("init-locker"), mgr.GetClient())
 	}
 	if r.remoteClient == nil {
-		r.remoteClient = defaultRemoteClient
+		r.remoteClient = remote.NewClusterClient
 	}
 
-	return ctrl.NewControllerManagedBy(mgr).
+	r.scheme = mgr.GetScheme()
+
+	err := ctrl.NewControllerManagedBy(mgr).
 		For(&bootstrapv1.KubeadmConfig{}).
 		Watches(
 			&source.Kind{Type: &clusterv1.Machine{}},
@@ -88,6 +91,12 @@ func (r *KubeadmConfigReconciler) SetupWithManager(mgr ctrl.Manager) error {
 			},
 		).
 		Complete(r)
+
+	if err != nil {
+		return errors.Wrap(err, "failed setting up with a controller manager")
+	}
+
+	return nil
 }
 
 // Reconcile handles KubeadmConfig events.
@@ -156,8 +165,7 @@ func (r *KubeadmConfigReconciler) Reconcile(req ctrl.Request) (_ ctrl.Result, re
 	case config.Status.Ready && (config.Spec.JoinConfiguration != nil && config.Spec.JoinConfiguration.Discovery.BootstrapToken != nil):
 		token := config.Spec.JoinConfiguration.Discovery.BootstrapToken.Token
 
-		// TODO replace with remote.NewClusterClient once it returns a client.Client
-		remoteClient, err := r.remoteClient(r.Client, cluster)
+		remoteClient, err := r.remoteClient(r.Client, cluster, r.scheme)
 		if err != nil {
 			log.Error(err, "error creating remote cluster client")
 			return ctrl.Result{}, err
@@ -493,8 +501,7 @@ func (r *KubeadmConfigReconciler) reconcileDiscovery(cluster *clusterv1.Cluster,
 
 	// if BootstrapToken already contains a token, respect it; otherwise create a new bootstrap token for the node to join
 	if config.Spec.JoinConfiguration.Discovery.BootstrapToken.Token == "" {
-		// TODO replace with remote.NewClusterClient once it returns a client.Client
-		remoteClient, err := r.remoteClient(r.Client, cluster)
+		remoteClient, err := r.remoteClient(r.Client, cluster, r.scheme)
 		if err != nil {
 			return err
 		}
@@ -562,23 +569,3 @@ func (r *KubeadmConfigReconciler) reconcileTopLevelObjectSettings(cluster *clust
 		log.Info("Altering ClusterConfiguration", "KubernetesVersion", config.Spec.ClusterConfiguration.KubernetesVersion)
 	}
 }
-
-// TODO remove this when remote.NewClusterClient returns a client.Client
-func defaultRemoteClient(c client.Client, cluster *clusterv1.Cluster) (client.Client, error) {
-	kubeConfig, err := kubeconfig.FromSecret(c, cluster)
-	if err != nil {
-		return nil, errors.Wrapf(err, "failed to retrieve kubeconfig secret for Cluster %s/%s", cluster.Namespace, cluster.Name)
-	}
-
-	restConfig, err := clientcmd.RESTConfigFromKubeConfig(kubeConfig)
-	if err != nil {
-		return nil, errors.Wrapf(err, "failed to create client configuration for Cluster %s/%s", cluster.Namespace, cluster.Name)
-	}
-
-	ret, err := client.New(restConfig, client.Options{})
-	if err != nil {
-		return nil, errors.Wrapf(err, "failed to create client for Cluster %s/%s", cluster.Namespace, cluster.Name)
-	}
-
-	return ret, nil
-}

bootstrap/kubeadm/controllers/kubeadmconfig_controller_test.go

@@ -533,8 +533,8 @@ func TestKubeadmConfigReconciler_Reconcile_RequeueIfControlPlaneIsMissingAPIEndp
 	}
 }
 
-func testRemoteClient(c client.Client) func(client.Client, *clusterv1.Cluster) (client.Client, error) {
-	return func(client.Client, *clusterv1.Cluster) (client.Client, error) {
+func testRemoteClient(c client.Client) func(client.Client, *clusterv1.Cluster, *runtime.Scheme) (client.Client, error) {
+	return func(client.Client, *clusterv1.Cluster, *runtime.Scheme) (client.Client, error) {
 		return c, nil
 	}
 }

controllers/cluster_controller.go

@@ -76,9 +76,14 @@ func (r *ClusterReconciler) SetupWithManager(mgr ctrl.Manager, options controlle
 		WithOptions(options).
 		Build(r)
 
+	if err != nil {
+		return errors.Wrap(err, "failed setting up with a controller manager")
+	}
+
 	r.controller = c
 	r.recorder = mgr.GetEventRecorderFor("cluster-controller")
-	return err
+
+	return nil
 }
 
 func (r *ClusterReconciler) Reconcile(req ctrl.Request) (_ ctrl.Result, reterr error) {

controllers/machine_controller.go

@@ -28,6 +28,7 @@ import (
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
+	"k8s.io/apimachinery/pkg/runtime"
 	kerrors "k8s.io/apimachinery/pkg/util/errors"
 	"k8s.io/apimachinery/pkg/util/wait"
 	"k8s.io/client-go/kubernetes"
@@ -67,6 +68,7 @@ type MachineReconciler struct {
 	controller       controller.Controller
 	recorder         record.EventRecorder
 	externalWatchers sync.Map
+	scheme           *runtime.Scheme
 }
 
 func (r *MachineReconciler) SetupWithManager(mgr ctrl.Manager, options controller.Options) error {
@@ -75,10 +77,16 @@ func (r *MachineReconciler) SetupWithManager(mgr ctrl.Manager, options controlle
 		WithOptions(options).
 		Build(r)
 
+	if err != nil {
+		return errors.Wrap(err, "failed setting up with a controller manager")
+	}
+
 	r.controller = c
 	r.recorder = mgr.GetEventRecorderFor("machine-controller")
 	r.config = mgr.GetConfig()
-	return err
+
+	r.scheme = mgr.GetScheme()
+	return nil
 }
 
 func (r *MachineReconciler) Reconcile(req ctrl.Request) (_ ctrl.Result, reterr error) {
@@ -273,12 +281,12 @@ func (r *MachineReconciler) drainNode(cluster *clusterv1.Cluster, nodeName strin
 		}
 	} else {
 		// Otherwise, proceed to get the remote cluster client and get the Node.
-		remoteClient, err := remote.NewClusterClient(r.Client, cluster)
+		restConfig, err := remote.RESTConfig(r.Client, cluster)
 		if err != nil {
 			logger.Error(err, "Error creating a remote client while deleting Machine, won't retry")
 			return nil
 		}
-		kubeClient, err = kubernetes.NewForConfig(remoteClient.RESTConfig())
+		kubeClient, err = kubernetes.NewForConfig(restConfig)
 		if err != nil {
 			logger.Error(err, "Error creating a remote client while deleting Machine, won't retry")
 			return nil
@@ -335,32 +343,25 @@ func (r *MachineReconciler) drainNode(cluster *clusterv1.Cluster, nodeName strin
 
 func (r *MachineReconciler) deleteNode(ctx context.Context, cluster *clusterv1.Cluster, name string) error {
 	logger := r.Log.WithValues("machine", name)
+	logger = logger.WithValues("machine", name, "cluster", cluster.Name, "namespace", cluster.Namespace)
 
-	if cluster == nil {
-		// Try to retrieve the Node from the local cluster, if no Cluster reference is found.
-		var node corev1.Node
-		if err := r.Client.Get(ctx, client.ObjectKey{Name: name}, &node); err != nil {
-			return err
-		}
-		return r.Client.Delete(ctx, &node)
-	}
-
-	logger = logger.WithValues("cluster", cluster.Name, "namespace", cluster.Namespace)
-
-	// Otherwise, proceed to get the remote cluster client and get the Node.
-	remoteClient, err := remote.NewClusterClient(r.Client, cluster)
+	// Create a remote client to delete the node
+	c, err := remote.NewClusterClient(r.Client, cluster, r.scheme)
 	if err != nil {
 		logger.Error(err, "Error creating a remote client for cluster while deleting Machine, won't retry")
 		return nil
 	}
 
-	corev1Remote, err := remoteClient.CoreV1()
-	if err != nil {
-		logger.Error(err, "Error creating a remote client for cluster while deleting Machine, won't retry")
-		return nil
+	node := &corev1.Node{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: name,
+		},
 	}
 
-	return corev1Remote.Nodes().Delete(name, &metav1.DeleteOptions{})
+	if err := c.Delete(ctx, node); err != nil {
+		return errors.Wrapf(err, "error deleting node %s", name)
+	}
+	return nil
 }
 
 // reconcileDeleteExternal tries to delete external references, returning true if it cannot find any.

controllers/machine_controller_noderef.go

@@ -23,11 +23,11 @@ import (
 	"github.com/pkg/errors"
 	apicorev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	corev1 "k8s.io/client-go/kubernetes/typed/core/v1"
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1alpha3"
 	"sigs.k8s.io/cluster-api/controllers/noderefutil"
 	"sigs.k8s.io/cluster-api/controllers/remote"
 	capierrors "sigs.k8s.io/cluster-api/errors"
+	"sigs.k8s.io/controller-runtime/pkg/client"
 )
 
 var (
@@ -65,18 +65,13 @@ func (r *MachineReconciler) reconcileNodeRef(_ context.Context, cluster *cluster
 		return err
 	}
 
-	clusterClient, err := remote.NewClusterClient(r.Client, cluster)
-	if err != nil {
-		return err
-	}
-
-	corev1Client, err := clusterClient.CoreV1()
+	clusterClient, err := remote.NewClusterClient(r.Client, cluster, r.scheme)
 	if err != nil {
 		return err
 	}
 
 	// Get the Node reference.
-	nodeRef, err := r.getNodeReference(corev1Client, providerID)
+	nodeRef, err := r.getNodeReference(clusterClient, providerID)
 	if err != nil {
 		if err == ErrNodeNotFound {
 			return errors.Wrapf(&capierrors.RequeueAfterError{RequeueAfter: 10 * time.Second},
@@ -94,13 +89,15 @@ func (r *MachineReconciler) reconcileNodeRef(_ context.Context, cluster *cluster
 	return nil
 }
 
-func (r *MachineReconciler) getNodeReference(client corev1.NodesGetter, providerID *noderefutil.ProviderID) (*apicorev1.ObjectReference, error) {
+func (r *MachineReconciler) getNodeReference(client client.Client, providerID *noderefutil.ProviderID) (*apicorev1.ObjectReference, error) {
 	logger := r.Log.WithValues("providerID", providerID)
 
 	listOpt := metav1.ListOptions{}
 
 	for {
-		nodeList, err := client.Nodes().List(listOpt)
+		nodeList := apicorev1.NodeList{}
+		// TODO Add a context to this method
+		err := client.List(context.TODO(), &nodeList)
 		if err != nil {
 			return nil, err
 		}

controllers/machine_controller_noderef_test.go

@@ -24,7 +24,6 @@ import (
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
-	fakeclient "k8s.io/client-go/kubernetes/fake"
 	"k8s.io/client-go/kubernetes/scheme"
 	"k8s.io/client-go/tools/record"
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1alpha3"
@@ -70,7 +69,7 @@ func TestGetNodeReference(t *testing.T) {
 		},
 	}
 
-	coreV1Client := fakeclient.NewSimpleClientset(nodeList...).CoreV1()
+	client := fake.NewFakeClientWithScheme(scheme.Scheme, nodeList...)
 
 	testCases := []struct {
 		name       string
@@ -108,7 +107,7 @@ func TestGetNodeReference(t *testing.T) {
 				t.Fatalf("Expected no error parsing provider id %q, got %v", test.providerID, err)
 			}
 
-			reference, err := r.getNodeReference(coreV1Client, providerID)
+			reference, err := r.getNodeReference(client, providerID)
 			if err != nil {
 				if (test.err != nil && !strings.Contains(err.Error(), test.err.Error())) || test.err == nil {
 					t.Fatalf("Expected error %v, got %v", test.err, err)

controllers/machinedeployment_controller.go

@@ -67,8 +67,13 @@ func (r *MachineDeploymentReconciler) SetupWithManager(mgr ctrl.Manager, options
 		WithOptions(options).
 		Complete(r)
 
+	if err != nil {
+		return errors.Wrap(err, "failed setting up with a controller manager")
+	}
+
 	r.recorder = mgr.GetEventRecorderFor("machinedeployment-controller")
-	return err
+
+	return nil
 }
 
 func (r *MachineDeploymentReconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) {

controllers/machineset_controller.go

@@ -30,6 +30,7 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/labels"
+	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/client-go/tools/record"
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1alpha3"
 	"sigs.k8s.io/cluster-api/controllers/external"
@@ -65,6 +66,7 @@ type MachineSetReconciler struct {
 	Log    logr.Logger
 
 	recorder record.EventRecorder
+	scheme   *runtime.Scheme
 }
 
 func (r *MachineSetReconciler) SetupWithManager(mgr ctrl.Manager, options controller.Options) error {
@@ -78,8 +80,14 @@ func (r *MachineSetReconciler) SetupWithManager(mgr ctrl.Manager, options contro
 		WithOptions(options).
 		Complete(r)
 
+	if err != nil {
+		return errors.Wrap(err, "failed setting up with a controller manager")
+	}
+
 	r.recorder = mgr.GetEventRecorderFor("machineset-controller")
-	return err
+
+	r.scheme = mgr.GetScheme()
+	return nil
 }
 
 func (r *MachineSetReconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) {

controllers/machineset_status.go

@@ -21,6 +21,7 @@ import (
 	"fmt"
 
 	"github.com/go-logr/logr"
+	"github.com/pkg/errors"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/labels"
@@ -138,23 +139,14 @@ func updateMachineSetStatus(c client.Client, ms *clusterv1.MachineSet, newStatus
 }
 
 func (r *MachineSetReconciler) getMachineNode(cluster *clusterv1.Cluster, machine *clusterv1.Machine) (*corev1.Node, error) {
-	if cluster == nil {
-		// Try to retrieve the Node from the local cluster, if no Cluster reference is found.
-		node := &corev1.Node{}
-		err := r.Client.Get(context.Background(), client.ObjectKey{Name: machine.Status.NodeRef.Name}, node)
-		return node, err
-	}
-
-	// Otherwise, proceed to get the remote cluster client and get the Node.
-	remoteClient, err := remote.NewClusterClient(r.Client, cluster)
+	c, err := remote.NewClusterClient(r.Client, cluster, r.scheme)
 	if err != nil {
 		return nil, err
 	}
-
-	corev1Remote, err := remoteClient.CoreV1()
+	node := &corev1.Node{}
+	err = c.Get(context.TODO(), client.ObjectKey{Name: machine.Status.NodeRef.Name}, node)
 	if err != nil {
-		return nil, err
+		return nil, errors.Wrapf(err, "error retrieving node %s for machine %s/%s", machine.Status.NodeRef.Name, machine.Namespace, machine.Name)
 	}
-
-	return corev1Remote.Nodes().Get(machine.Status.NodeRef.Name, metav1.GetOptions{})
+	return node, nil
 }