🌱 Conversion of references should use a CR client

This changeset introduces some deprecations for functions that are still creating the metadata client through a restconfig each time the function is called. The controller runtime client supports retrieving metadata only objects; this change deprecates the old functions and creates new ones that use a CR client. Signed-off-by: Vince Prignano <[email protected]>
kubernetes-sigs · Aug 26, 2021 · 6c13858 · 6c13858
1 parent b0e4c8b
commit 6c13858
Show file tree

Hide file tree

Showing 11 changed files with 81 additions and 38 deletions.
diff --git a/controllers/cluster_controller.go b/controllers/cluster_controller.go
@@ -30,7 +30,6 @@ import (
 	"k8s.io/apimachinery/pkg/api/meta"
 	"k8s.io/apimachinery/pkg/runtime"
 	kerrors "k8s.io/apimachinery/pkg/util/errors"
-	"k8s.io/client-go/rest"
 	"k8s.io/client-go/tools/record"
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1alpha4"
 	"sigs.k8s.io/cluster-api/controllers/external"
@@ -68,7 +67,6 @@ type ClusterReconciler struct {
 	Client           client.Client
 	WatchFilterValue string
 
-	restConfig      *rest.Config
 	recorder        record.EventRecorder
 	externalTracker external.ObjectTracker
 }
@@ -91,7 +89,6 @@ func (r *ClusterReconciler) SetupWithManager(ctx context.Context, mgr ctrl.Manag
 	}
 
 	r.recorder = mgr.GetEventRecorderFor("cluster-controller")
-	r.restConfig = mgr.GetConfig()
 	r.externalTracker = external.ObjectTracker{
 		Controller: controller,
 	}

diff --git a/controllers/cluster_controller_phases.go b/controllers/cluster_controller_phases.go
@@ -66,7 +66,7 @@ func (r *ClusterReconciler) reconcilePhase(_ context.Context, cluster *clusterv1
 func (r *ClusterReconciler) reconcileExternal(ctx context.Context, cluster *clusterv1.Cluster, ref *corev1.ObjectReference) (external.ReconcileOutput, error) {
 	log := ctrl.LoggerFrom(ctx)
 
-	if err := utilconversion.ConvertReferenceAPIContract(ctx, r.Client, r.restConfig, ref); err != nil {
+	if err := utilconversion.UpdateReferenceAPIContract(ctx, r.Client, ref); err != nil {
 		return external.ReconcileOutput{}, err
 	}
 

diff --git a/controllers/machine_controller.go b/controllers/machine_controller.go
@@ -30,7 +30,6 @@ import (
 	kerrors "k8s.io/apimachinery/pkg/util/errors"
 	"k8s.io/apimachinery/pkg/util/wait"
 	"k8s.io/client-go/kubernetes"
-	"k8s.io/client-go/rest"
 	"k8s.io/client-go/tools/record"
 	"k8s.io/klog/v2"
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1alpha4"
@@ -81,7 +80,6 @@ type MachineReconciler struct {
 	WatchFilterValue string
 
 	controller      controller.Controller
-	restConfig      *rest.Config
 	recorder        record.EventRecorder
 	externalTracker external.ObjectTracker
 }
@@ -116,7 +114,6 @@ func (r *MachineReconciler) SetupWithManager(ctx context.Context, mgr ctrl.Manag
 	r.controller = controller
 
 	r.recorder = mgr.GetEventRecorderFor("machine-controller")
-	r.restConfig = mgr.GetConfig()
 	r.externalTracker = external.ObjectTracker{
 		Controller: controller,
 	}

diff --git a/controllers/machine_controller_phases.go b/controllers/machine_controller_phases.go
@@ -89,7 +89,7 @@ func (r *MachineReconciler) reconcilePhase(_ context.Context, m *clusterv1.Machi
 func (r *MachineReconciler) reconcileExternal(ctx context.Context, cluster *clusterv1.Cluster, m *clusterv1.Machine, ref *corev1.ObjectReference) (external.ReconcileOutput, error) {
 	log := ctrl.LoggerFrom(ctx, "cluster", cluster.Name)
 
-	if err := utilconversion.ConvertReferenceAPIContract(ctx, r.Client, r.restConfig, ref); err != nil {
+	if err := utilconversion.UpdateReferenceAPIContract(ctx, r.Client, ref); err != nil {
 		return external.ReconcileOutput{}, err
 	}
 

diff --git a/controllers/machinedeployment_controller.go b/controllers/machinedeployment_controller.go
@@ -26,7 +26,6 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/labels"
 	kerrors "k8s.io/apimachinery/pkg/util/errors"
-	"k8s.io/client-go/rest"
 	"k8s.io/client-go/tools/record"
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1alpha4"
 	"sigs.k8s.io/cluster-api/util"
@@ -57,8 +56,7 @@ type MachineDeploymentReconciler struct {
 	Client           client.Client
 	WatchFilterValue string
 
-	recorder   record.EventRecorder
-	restConfig *rest.Config
+	recorder record.EventRecorder
 }
 
 func (r *MachineDeploymentReconciler) SetupWithManager(ctx context.Context, mgr ctrl.Manager, options controller.Options) error {
@@ -94,7 +92,6 @@ func (r *MachineDeploymentReconciler) SetupWithManager(ctx context.Context, mgr
 	}
 
 	r.recorder = mgr.GetEventRecorderFor("machinedeployment-controller")
-	r.restConfig = mgr.GetConfig()
 	return nil
 }
 
@@ -202,12 +199,12 @@ func (r *MachineDeploymentReconciler) reconcile(ctx context.Context, cluster *cl
 	}
 
 	// Make sure to reconcile the external infrastructure reference.
-	if err := reconcileExternalTemplateReference(ctx, r.Client, r.restConfig, cluster, &d.Spec.Template.Spec.InfrastructureRef); err != nil {
+	if err := reconcileExternalTemplateReference(ctx, r.Client, cluster, &d.Spec.Template.Spec.InfrastructureRef); err != nil {
 		return ctrl.Result{}, err
 	}
 	// Make sure to reconcile the external bootstrap reference, if any.
 	if d.Spec.Template.Spec.Bootstrap.ConfigRef != nil {
-		if err := reconcileExternalTemplateReference(ctx, r.Client, r.restConfig, cluster, d.Spec.Template.Spec.Bootstrap.ConfigRef); err != nil {
+		if err := reconcileExternalTemplateReference(ctx, r.Client, cluster, d.Spec.Template.Spec.Bootstrap.ConfigRef); err != nil {
 			return ctrl.Result{}, err
 		}
 	}

diff --git a/controllers/machineset_controller.go b/controllers/machineset_controller.go
@@ -28,7 +28,6 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/labels"
 	kerrors "k8s.io/apimachinery/pkg/util/errors"
-	"k8s.io/client-go/rest"
 	"k8s.io/client-go/tools/record"
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1alpha4"
 	"sigs.k8s.io/cluster-api/controllers/external"
@@ -71,8 +70,7 @@ type MachineSetReconciler struct {
 	Tracker          *remote.ClusterCacheTracker
 	WatchFilterValue string
 
-	recorder   record.EventRecorder
-	restConfig *rest.Config
+	recorder record.EventRecorder
 }
 
 func (r *MachineSetReconciler) SetupWithManager(ctx context.Context, mgr ctrl.Manager, options controller.Options) error {
@@ -108,7 +106,6 @@ func (r *MachineSetReconciler) SetupWithManager(ctx context.Context, mgr ctrl.Ma
 	}
 
 	r.recorder = mgr.GetEventRecorderFor("machineset-controller")
-	r.restConfig = mgr.GetConfig()
 	return nil
 }
 
@@ -184,12 +181,12 @@ func (r *MachineSetReconciler) reconcile(ctx context.Context, cluster *clusterv1
 	}
 
 	// Make sure to reconcile the external infrastructure reference.
-	if err := reconcileExternalTemplateReference(ctx, r.Client, r.restConfig, cluster, &machineSet.Spec.Template.Spec.InfrastructureRef); err != nil {
+	if err := reconcileExternalTemplateReference(ctx, r.Client, cluster, &machineSet.Spec.Template.Spec.InfrastructureRef); err != nil {
 		return ctrl.Result{}, err
 	}
 	// Make sure to reconcile the external bootstrap reference, if any.
 	if machineSet.Spec.Template.Spec.Bootstrap.ConfigRef != nil {
-		if err := reconcileExternalTemplateReference(ctx, r.Client, r.restConfig, cluster, machineSet.Spec.Template.Spec.Bootstrap.ConfigRef); err != nil {
+		if err := reconcileExternalTemplateReference(ctx, r.Client, cluster, machineSet.Spec.Template.Spec.Bootstrap.ConfigRef); err != nil {
 			return ctrl.Result{}, err
 		}
 	}
@@ -668,12 +665,12 @@ func (r *MachineSetReconciler) getMachineNode(ctx context.Context, cluster *clus
 	return node, nil
 }
 
-func reconcileExternalTemplateReference(ctx context.Context, c client.Client, restConfig *rest.Config, cluster *clusterv1.Cluster, ref *corev1.ObjectReference) error {
+func reconcileExternalTemplateReference(ctx context.Context, c client.Client, cluster *clusterv1.Cluster, ref *corev1.ObjectReference) error {
 	if !strings.HasSuffix(ref.Kind, clusterv1.TemplateSuffix) {
 		return nil
 	}
 
-	if err := utilconversion.ConvertReferenceAPIContract(ctx, c, restConfig, ref); err != nil {
+	if err := utilconversion.UpdateReferenceAPIContract(ctx, c, ref); err != nil {
 		return err
 	}
 

diff --git a/controllers/topology/controller.go b/controllers/topology/controller.go
@@ -23,7 +23,6 @@ import (
 	"github.com/pkg/errors"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/types"
-	"k8s.io/client-go/rest"
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1alpha4"
 	"sigs.k8s.io/cluster-api/api/v1alpha4/index"
 	"sigs.k8s.io/cluster-api/controllers/external"
@@ -52,7 +51,6 @@ type ClusterReconciler struct {
 	// thus allowing to optimize reads for templates or provider specific objects in a managed topology.
 	UnstructuredCachingClient client.Client
 
-	restConfig      *rest.Config
 	externalTracker external.ObjectTracker
 }
 
@@ -75,7 +73,6 @@ func (r *ClusterReconciler) SetupWithManager(ctx context.Context, mgr ctrl.Manag
 		return errors.Wrap(err, "failed setting up with a controller manager")
 	}
 
-	r.restConfig = mgr.GetConfig()
 	r.externalTracker = external.ObjectTracker{
 		Controller: c,
 	}

diff --git a/controllers/topology/util.go b/controllers/topology/util.go
@@ -48,7 +48,7 @@ func (r *ClusterReconciler) getReference(ctx context.Context, ref *corev1.Object
 	if ref == nil {
 		return nil, errors.New("reference is not set")
 	}
-	if err := utilconversion.ConvertReferenceAPIContract(ctx, r.Client, r.restConfig, ref); err != nil {
+	if err := utilconversion.UpdateReferenceAPIContract(ctx, r.Client, ref); err != nil {
 		return nil, err
 	}
 

diff --git a/exp/controllers/machinepool_controller.go b/exp/controllers/machinepool_controller.go
@@ -26,7 +26,6 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	kerrors "k8s.io/apimachinery/pkg/util/errors"
-	"k8s.io/client-go/rest"
 	"k8s.io/client-go/tools/record"
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1alpha4"
 	"sigs.k8s.io/cluster-api/controllers/external"
@@ -61,7 +60,6 @@ type MachinePoolReconciler struct {
 	Client           client.Client
 	WatchFilterValue string
 
-	config           *rest.Config
 	controller       controller.Controller
 	recorder         record.EventRecorder
 	externalWatchers sync.Map
@@ -95,7 +93,6 @@ func (r *MachinePoolReconciler) SetupWithManager(ctx context.Context, mgr ctrl.M
 
 	r.controller = c
 	r.recorder = mgr.GetEventRecorderFor("machinepool-controller")
-	r.config = mgr.GetConfig()
 	return nil
 }
 

diff --git a/util/conversion/conversion.go b/util/conversion/conversion.go
@@ -58,6 +58,44 @@ var (
 // the Custom Resource Definition and looks which one is the stored version available.
 //
 // The object passed as input is modified in place if an updated compatible version is found.
+func UpdateReferenceAPIContract(ctx context.Context, c client.Client, ref *corev1.ObjectReference) error {
+	log := ctrl.LoggerFrom(ctx)
+	gvk := ref.GroupVersionKind()
+
+	metadata, err := util.GetGVKMetadata(ctx, c, gvk)
+	if err != nil {
+		log.Info("Cannot retrieve CRD with metadata only client, falling back to slower listing", "err", err.Error())
+		// Fallback to slower and more memory intensive method to get the full CRD.
+		crd, err := util.GetCRDWithContract(ctx, c, gvk, contract)
+		if err != nil {
+			return err
+		}
+		metadata = &metav1.PartialObjectMetadata{
+			TypeMeta:   crd.TypeMeta,
+			ObjectMeta: crd.ObjectMeta,
+		}
+	}
+
+	chosen, err := getLatestAPIVersionFromContract(metadata)
+	if err != nil {
+		return err
+	}
+
+	// Modify the GroupVersionKind with the new version.
+	if gvk.Version != chosen {
+		gvk.Version = chosen
+		ref.SetGroupVersionKind(gvk)
+	}
+
+	return nil
+}
+
+// ConvertReferenceAPIContract takes a client and object reference, queries the API Server for
+// the Custom Resource Definition and looks which one is the stored version available.
+//
+// The object passed as input is modified in place if an updated compatible version is found.
+//
+// Deprecated: Use UpdateReferenceAPIContract instead.
 func ConvertReferenceAPIContract(ctx context.Context, c client.Client, restConfig *rest.Config, ref *corev1.ObjectReference) error {
 	log := ctrl.LoggerFrom(ctx)
 	gvk := ref.GroupVersionKind()
@@ -76,17 +114,11 @@ func ConvertReferenceAPIContract(ctx context.Context, c client.Client, restConfi
 		}
 	}
 
-	// If there is no label, return early without changing the reference.
-	supportedVersions, ok := metadata.Labels[contract]
-	if !ok || supportedVersions == "" {
-		return errors.Errorf("cannot find any versions matching contract %q for CRD %v as contract version label(s) are either missing or empty", contract, metadata.Name)
+	chosen, err := getLatestAPIVersionFromContract(metadata)
+	if err != nil {
+		return err
 	}
 
-	// Pick the latest version in the slice and validate it.
-	kubeVersions := util.KubeAwareAPIVersions(strings.Split(supportedVersions, "_"))
-	sort.Sort(kubeVersions)
-	chosen := kubeVersions[len(kubeVersions)-1]
-
 	// Modify the GroupVersionKind with the new version.
 	if gvk.Version != chosen {
 		gvk.Version = chosen
@@ -96,6 +128,21 @@ func ConvertReferenceAPIContract(ctx context.Context, c client.Client, restConfi
 	return nil
 }
 
+func getLatestAPIVersionFromContract(metadata metav1.Object) (string, error) {
+	labels := metadata.GetLabels()
+
+	// If there is no label, return early without changing the reference.
+	supportedVersions, ok := labels[contract]
+	if !ok || supportedVersions == "" {
+		return "", errors.Errorf("cannot find any versions matching contract %q for GVK %v as contract version label(s) are either missing or empty", contract, metadata.GetName())
+	}
+
+	// Pick the latest version in the slice and validate it.
+	kubeVersions := util.KubeAwareAPIVersions(strings.Split(supportedVersions, "_"))
+	sort.Sort(kubeVersions)
+	return kubeVersions[len(kubeVersions)-1], nil
+}
+
 // MarshalData stores the source object as json data in the destination object annotations map.
 // It ignores the metadata of the source object.
 func MarshalData(src metav1.Object, dst metav1.Object) error {

diff --git a/util/util.go b/util/util.go
@@ -416,6 +416,18 @@ func HasOwner(refList []metav1.OwnerReference, apiVersion string, kinds []string
 	return false
 }
 
+// GetGVKMetadata retrieves a CustomResourceDefinition metadata from the API server using partial object metadata.
+//
+// This function is greatly more efficient than GetCRDWithContract and should be preferred in most cases.
+func GetGVKMetadata(ctx context.Context, c client.Client, gvk schema.GroupVersionKind) (*metav1.PartialObjectMetadata, error) {
+	meta := &metav1.PartialObjectMetadata{}
+	meta.SetName(fmt.Sprintf("%s.%s", flect.Pluralize(strings.ToLower(gvk.Kind)), gvk.Group))
+	if err := c.Get(ctx, client.ObjectKeyFromObject(meta), meta); err != nil {
+		return meta, errors.Wrap(err, "failed to retrieve metadata from GVK resource")
+	}
+	return meta, nil
+}
+
 // GetCRDWithContract retrieves a list of CustomResourceDefinitions from using controller-runtime Client,
 // filtering with the `contract` label passed in.
 // Returns the first CRD in the list that matches the GroupVersionKind, otherwise returns an error.
@@ -445,6 +457,8 @@ func GetCRDWithContract(ctx context.Context, c client.Client, gvk schema.GroupVe
 // GetCRDMetadataFromGVK retrieves a CustomResourceDefinition metadata from the API server using client-go's metadata only client.
 //
 // This function is greatly more efficient than GetCRDWithContract and should be preferred in most cases.
+//
+// Deprecated: Use GetGVKMetadata instead.
 func GetCRDMetadataFromGVK(ctx context.Context, restConfig *rest.Config, gvk schema.GroupVersionKind) (*metav1.PartialObjectMetadata, error) {
 	// Make sure a rest config is available.
 	if restConfig == nil {