Merge pull request #8041 from sbueringer/pr-dont-support-crds-with-in…

…valid-name

⚠️ Stop supporting CRDs with invalid names

cmd/clusterctl/client/cluster/installer.go

@@ -229,11 +229,11 @@ func (i *providerInstaller) Validate() error {
 
 			gk, err := getCRDGroupKind(obj)
 			if err != nil {
-				return errors.Wrap(err, "Failed to read group and kind from CustomResourceDefinition")
+				return errors.Wrap(err, "failed to read group and kind from CustomResourceDefinition")
 			}
 
 			if err := validateCRDName(obj, gk); err != nil {
-				logf.Log.Info(err.Error())
+				return err
 			}
 		}
 	}
@@ -279,9 +279,9 @@ func validateCRDName(obj unstructured.Unstructured, gk *schema.GroupKind) error
 		return nil
 	}
 
-	return errors.Errorf("WARNING: CRD name %q is invalid for a CRD referenced in a core Cluster API CRD,"+
-		"it should be %q. Support for CRDs that are referenced in core Cluster API resources with invalid names will be "+
-		"dropped in a future Cluster API release. Note: Please check if this CRD is actually referenced in core Cluster API "+
+	return errors.Errorf("ERROR: CRD name %q is invalid for a CRD referenced in a core Cluster API CRD,"+
+		"it should be %q. Support for CRDs that are referenced in core Cluster API resources with invalid names has been "+
+		"dropped. Note: Please check if this CRD is actually referenced in core Cluster API "+
 		"CRDs. If not, this warning can be hidden by setting the %q' annotation.", obj.GetName(), correctCRDName, clusterctlv1.SkipCRDNamePreflightCheckAnnotation)
 }
 

controlplane/kubeadm/internal/controllers/helpers.go

@@ -148,7 +148,7 @@ func (r *KubeadmControlPlaneReconciler) reconcileExternalReference(ctx context.C
 		return nil
 	}
 
-	if err := utilconversion.UpdateReferenceAPIContract(ctx, r.Client, r.APIReader, ref); err != nil {
+	if err := utilconversion.UpdateReferenceAPIContract(ctx, r.Client, ref); err != nil {
 		return err
 	}
 

controlplane/kubeadm/internal/controllers/suite_test.go

@@ -38,7 +38,7 @@ var (
 			Kind:       "CustomResourceDefinition",
 		},
 		ObjectMeta: metav1.ObjectMeta{
-			Name: "genericmachinetemplate.generic.io",
+			Name: "genericmachinetemplates.generic.io",
 			Labels: map[string]string{
 				"cluster.x-k8s.io/v1beta1": "v1",
 			},

docs/book/src/developer/providers/v1.3-to-v1.4.md

@@ -22,6 +22,7 @@ maintainers of providers and consumers of our Go API.
 
 ### Removals
 
+- `util/conversion.GetCRDWithContract` has been removed.
 - `clusterctl backup` has been removed.
 - `clusterctl restore` has been removed.
 - `api/v1beta1.MachineHealthCheckSuccededCondition` condition type has been removed.
@@ -34,6 +35,7 @@ maintainers of providers and consumers of our Go API.
 
 ### API Changes
 
+- `util/conversion.UpdateReferenceAPIContract` dropped the `APIReader` parameter because it's not required anymore as we now only handle CRDs with compliant names.
 - `Backup(options BackupOptions) error` in the Client interface has been removed.
 - `Restore(options RestoreOptions) error` in the Client interface has been removed.
 - `cmd/clusterctl/client.RolloutOptions` has been removed, `RolloutRestartOptions`, `RolloutPauseOptions` , `RolloutResumeOptions`, and `RolloutUndoOptions` have been added instead.
@@ -53,6 +55,9 @@ maintainers of providers and consumers of our Go API.
 
 ### Other
 
+- clusterctl now emits an error for provider CRDs which don't comply with the CRD naming conventions. This warning can be skipped for resources not referenced by Cluster API
+  core resources via the `clusterctl.cluster.x-k8s.io/skip-crd-name-preflight-check` annotation. The contracts specify:
+  > The CRD name must have the format produced by sigs.k8s.io/cluster-api/util/contract.CalculateCRDName(Group, Kind)
 - `clusterctl upgrade apply` no longer requires a namespace when updating providers. It is now optional and in a future release it will be deprecated. The new syntax is `[namespace/]provider:version`.
 - `WatchDeploymentLogs` is changed to `WatchDeploymentLogsByName`, it works same as before. Another function `WatchDeploymentLogsByLabelSelector` is added to stream logs of deployment by label selector.
 - Cluster API controllers are now using an explicit security context by default.

docs/book/src/reference/labels_and_annotations.md

@@ -21,7 +21,7 @@
 
 | Annotation                                                       | Note                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                        |
 |:-----------------------------------------------------------------|:------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
-| clusterctl.cluster.x-k8s.io/skip-crd-name-preflight-check        | Can be placed on provider CRDs, so that clusterctl doesn't emit a warning if the CRD doesn't comply with Cluster APIs naming scheme. Only CRDs that are referenced by core Cluster API CRDs have to comply with the naming scheme.                                                                                                                                                                                                                                                                                                                          |
+| clusterctl.cluster.x-k8s.io/skip-crd-name-preflight-check        | Can be placed on provider CRDs, so that clusterctl doesn't emit an error if the CRD doesn't comply with Cluster APIs naming scheme. Only CRDs that are referenced by core Cluster API CRDs have to comply with the naming scheme.                                                                                                                                                                                                                                                                                                                           |
 | unsafe.topology.cluster.x-k8s.io/disable-update-class-name-check | It can be used to disable the webhook check on update that disallows a pre-existing Cluster to be populated with Topology information and Class.                                                                                                                                                                                                                                                                                                                                                                                                            |
 | cluster.x-k8s.io/cluster-name                                    | It is set on nodes identifying the name of the cluster the node belongs to.                                                                                                                                                                                                                                                                                                                                                                                                                                                                                 |
 | cluster.x-k8s.io/cluster-namespace                               | It is set on nodes identifying the namespace of the cluster the node belongs to.                                                                                                                                                                                                                                                                                                                                                                                                                                                                            |

exp/internal/controllers/machinepool_controller_phases.go

@@ -105,7 +105,7 @@ func (r *MachinePoolReconciler) reconcilePhase(mp *expv1.MachinePool) {
 func (r *MachinePoolReconciler) reconcileExternal(ctx context.Context, cluster *clusterv1.Cluster, m *expv1.MachinePool, ref *corev1.ObjectReference) (external.ReconcileOutput, error) {
 	log := ctrl.LoggerFrom(ctx)
 
-	if err := utilconversion.UpdateReferenceAPIContract(ctx, r.Client, r.APIReader, ref); err != nil {
+	if err := utilconversion.UpdateReferenceAPIContract(ctx, r.Client, ref); err != nil {
 		return external.ReconcileOutput{}, err
 	}
 

internal/controllers/cluster/cluster_controller_phases.go

@@ -67,7 +67,7 @@ func (r *Reconciler) reconcilePhase(_ context.Context, cluster *clusterv1.Cluste
 func (r *Reconciler) reconcileExternal(ctx context.Context, cluster *clusterv1.Cluster, ref *corev1.ObjectReference) (external.ReconcileOutput, error) {
 	log := ctrl.LoggerFrom(ctx)
 
-	if err := utilconversion.UpdateReferenceAPIContract(ctx, r.Client, r.APIReader, ref); err != nil {
+	if err := utilconversion.UpdateReferenceAPIContract(ctx, r.Client, ref); err != nil {
 		return external.ReconcileOutput{}, err
 	}
 

internal/controllers/clusterclass/clusterclass_controller.go

@@ -166,7 +166,7 @@ func (r *Reconciler) reconcile(ctx context.Context, clusterClass *clusterv1.Clus
 		// Check if the template reference is outdated, i.e. it is not using the latest apiVersion
 		// for the current CAPI contract.
 		updatedRef := ref.DeepCopy()
-		if err := conversion.UpdateReferenceAPIContract(ctx, r.Client, r.APIReader, updatedRef); err != nil {
+		if err := conversion.UpdateReferenceAPIContract(ctx, r.Client, updatedRef); err != nil {
 			errs = append(errs, err)
 		}
 		if ref.GroupVersionKind().Version != updatedRef.GroupVersionKind().Version {

internal/controllers/machine/machine_controller_phases.go

@@ -91,7 +91,7 @@ func (r *Reconciler) reconcilePhase(_ context.Context, m *clusterv1.Machine) {
 func (r *Reconciler) reconcileExternal(ctx context.Context, cluster *clusterv1.Cluster, m *clusterv1.Machine, ref *corev1.ObjectReference) (external.ReconcileOutput, error) {
 	log := ctrl.LoggerFrom(ctx)
 
-	if err := utilconversion.UpdateReferenceAPIContract(ctx, r.Client, r.APIReader, ref); err != nil {
+	if err := utilconversion.UpdateReferenceAPIContract(ctx, r.Client, ref); err != nil {
 		return external.ReconcileOutput{}, err
 	}
 

internal/controllers/machinedeployment/machinedeployment_controller.go

@@ -212,12 +212,12 @@ func (r *Reconciler) reconcile(ctx context.Context, cluster *clusterv1.Cluster,
 	}
 
 	// Make sure to reconcile the external infrastructure reference.
-	if err := reconcileExternalTemplateReference(ctx, r.Client, r.APIReader, 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.APIReader, 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
 		}
 	}
@@ -399,12 +399,12 @@ func (r *Reconciler) shouldAdopt(md *clusterv1.MachineDeployment) bool {
 	return !util.HasOwner(md.OwnerReferences, clusterv1.GroupVersion.String(), []string{"Cluster"})
 }
 
-func reconcileExternalTemplateReference(ctx context.Context, c client.Client, apiReader client.Reader, 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.UpdateReferenceAPIContract(ctx, c, apiReader, ref); err != nil {
+	if err := utilconversion.UpdateReferenceAPIContract(ctx, c, ref); err != nil {
 		return err
 	}
 

internal/controllers/machineset/machineset_controller.go

@@ -228,12 +228,12 @@ func (r *Reconciler) reconcile(ctx context.Context, cluster *clusterv1.Cluster,
 	}
 
 	// Make sure to reconcile the external infrastructure reference.
-	if err := reconcileExternalTemplateReference(ctx, r.Client, r.APIReader, 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.APIReader, 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
 		}
 	}
@@ -830,12 +830,12 @@ func (r *Reconciler) getMachineNode(ctx context.Context, cluster *clusterv1.Clus
 	return node, nil
 }
 
-func reconcileExternalTemplateReference(ctx context.Context, c client.Client, apiReader client.Reader, 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.UpdateReferenceAPIContract(ctx, c, apiReader, ref); err != nil {
+	if err := utilconversion.UpdateReferenceAPIContract(ctx, c, ref); err != nil {
 		return err
 	}
 

util/conversion/conversion.go

@@ -37,7 +37,6 @@ import (
 	runtimeserializer "k8s.io/apimachinery/pkg/runtime/serializer"
 	"k8s.io/apimachinery/pkg/util/json"
 	"k8s.io/client-go/kubernetes/scheme"
-	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/conversion"
 
@@ -59,29 +58,18 @@ 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.
-// NOTE: In case CRDs are named incorrectly, this func is using an APIReader instead of the regular client to list CRDs
-// to avoid implicitly creating an informer for CRDs which would lead to high memory consumption.
-func UpdateReferenceAPIContract(ctx context.Context, c client.Client, apiReader client.Reader, ref *corev1.ObjectReference) error {
-	log := ctrl.LoggerFrom(ctx)
+// NOTE: This version depends on CRDs being named correctly as defined by contract.CalculateCRDName.
+func UpdateReferenceAPIContract(ctx context.Context, c client.Client, ref *corev1.ObjectReference) error {
 	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, apiReader, gvk, contract)
-		if err != nil {
-			return err
-		}
-		metadata = &metav1.PartialObjectMetadata{
-			TypeMeta:   crd.TypeMeta,
-			ObjectMeta: crd.ObjectMeta,
-		}
+		return errors.Wrapf(err, "failed to update apiVersion in ref")
 	}
 
 	chosen, err := getLatestAPIVersionFromContract(metadata)
 	if err != nil {
-		return err
+		return errors.Wrapf(err, "failed to update apiVersion in ref")
 	}
 
 	// Modify the GroupVersionKind with the new version.

util/util.go

@@ -450,32 +450,6 @@ func GetGVKMetadata(ctx context.Context, c client.Client, gvk schema.GroupVersio
 	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.
-func GetCRDWithContract(ctx context.Context, c client.Reader, gvk schema.GroupVersionKind, contract string) (*apiextensionsv1.CustomResourceDefinition, error) {
-	crdList := &apiextensionsv1.CustomResourceDefinitionList{}
-	for {
-		if err := c.List(ctx, crdList, client.Continue(crdList.Continue), client.HasLabels{contract}); err != nil {
-			return nil, errors.Wrapf(err, "failed to list CustomResourceDefinitions for %v", gvk)
-		}
-
-		for i := range crdList.Items {
-			crd := crdList.Items[i]
-			if crd.Spec.Group == gvk.Group &&
-				crd.Spec.Names.Kind == gvk.Kind {
-				return &crd, nil
-			}
-		}
-
-		if crdList.Continue == "" {
-			break
-		}
-	}
-
-	return nil, errors.Errorf("failed to find a CustomResourceDefinition for %v with contract %q", gvk, contract)
-}
-
 // KubeAwareAPIVersions is a sortable slice of kube-like version strings.
 //
 // Kube-like version strings are starting with a v, followed by a major version,