Merge pull request #7506 from sbueringer/pr-clusterctl-precheck

🌱 clusterctl: implement CRD name precheck

Author: k8s-ci-robot

cmd/clusterctl/api/v1alpha3/annotations.go

@@ -19,4 +19,10 @@ package v1alpha3
 const (
 	// CertManagerVersionAnnotation reports the cert manager version installed by clusterctl.
 	CertManagerVersionAnnotation = "cert-manager.clusterctl.cluster.x-k8s.io/version"
+
+	// SkipCRDNamePreflightCheckAnnotation 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.
+	// Note: Only CRDs that are referenced by core Cluster API CRDs have to comply with the naming scheme.
+	// See the following issue for more information: https://github.com/kubernetes-sigs/cluster-api/issues/5686#issuecomment-1260897278
+	SkipCRDNamePreflightCheckAnnotation = "clusterctl.cluster.x-k8s.io/skip-crd-name-preflight-check"
 )

cmd/clusterctl/client/cluster/installer.go

@@ -25,7 +25,10 @@ import (
 	"github.com/pkg/errors"
 	appsv1 "k8s.io/api/apps/v1"
 	corev1 "k8s.io/api/core/v1"
+	apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
+	apiextensionsv1beta1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
+	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/apimachinery/pkg/util/version"
 	"k8s.io/apimachinery/pkg/util/wait"
@@ -35,8 +38,10 @@ import (
 	clusterctlv1 "sigs.k8s.io/cluster-api/cmd/clusterctl/api/v1alpha3"
 	"sigs.k8s.io/cluster-api/cmd/clusterctl/client/config"
 	"sigs.k8s.io/cluster-api/cmd/clusterctl/client/repository"
+	"sigs.k8s.io/cluster-api/cmd/clusterctl/internal/scheme"
 	"sigs.k8s.io/cluster-api/cmd/clusterctl/internal/util"
 	logf "sigs.k8s.io/cluster-api/cmd/clusterctl/log"
+	"sigs.k8s.io/cluster-api/util/contract"
 )
 
 // ProviderInstaller defines methods for enforcing consistency rules for provider installation.
@@ -53,6 +58,8 @@ type ProviderInstaller interface {
 	// The following checks are performed in order to ensure a fully operational cluster:
 	// - There must be only one instance of the same provider
 	// - All the providers in must support the same API Version of Cluster API (contract)
+	// - All provider CRDs that are referenced in core Cluster API CRDs must comply with the CRD naming scheme,
+	//   otherwise a warning is logged.
 	Validate() error
 
 	// Images returns the list of images required for installing the providers ready in the install queue.
@@ -209,9 +216,75 @@ func (i *providerInstaller) Validate() error {
 			return errors.Errorf("installing provider %q can lead to a non functioning management cluster: the target version for the provider supports the %s API Version of Cluster API (contract), while the management cluster is using %s", components.ManifestLabel(), providerContract, managementClusterContract)
 		}
 	}
+
+	// Validate if provider CRDs comply with the naming scheme.
+	for _, components := range i.installQueue {
+		componentObjects := components.Objs()
+
+		for _, obj := range componentObjects {
+			// Continue if object is not a CRD.
+			if obj.GetKind() != customResourceDefinitionKind {
+				continue
+			}
+
+			gk, err := getCRDGroupKind(obj)
+			if err != nil {
+				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 nil
 }
 
+func getCRDGroupKind(obj unstructured.Unstructured) (*schema.GroupKind, error) {
+	var group string
+	var kind string
+	version := obj.GroupVersionKind().Version
+	switch version {
+	case apiextensionsv1beta1.SchemeGroupVersion.Version:
+		crd := &apiextensionsv1beta1.CustomResourceDefinition{}
+		if err := scheme.Scheme.Convert(&obj, crd, nil); err != nil {
+			return nil, errors.Wrapf(err, "failed to convert %s CustomResourceDefinition %q", version, obj.GetName())
+		}
+		group = crd.Spec.Group
+		kind = crd.Spec.Names.Kind
+	case apiextensionsv1.SchemeGroupVersion.Version:
+		crd := &apiextensionsv1.CustomResourceDefinition{}
+		if err := scheme.Scheme.Convert(&obj, crd, nil); err != nil {
+			return nil, errors.Wrapf(err, "failed to convert %s CustomResourceDefinition %q", version, obj.GetName())
+		}
+		group = crd.Spec.Group
+		kind = crd.Spec.Names.Kind
+	default:
+		return nil, errors.Errorf("failed to read %s CustomResourceDefinition %q", version, obj.GetName())
+	}
+	return &schema.GroupKind{Group: group, Kind: kind}, nil
+}
+
+func validateCRDName(obj unstructured.Unstructured, gk *schema.GroupKind) error {
+	// Return if CRD has skip CRD name preflight check annotation.
+	if _, ok := obj.GetAnnotations()[clusterctlv1.SkipCRDNamePreflightCheckAnnotation]; ok {
+		return nil
+	}
+
+	correctCRDName := contract.CalculateCRDName(gk.Group, gk.Kind)
+
+	// Return if name is correct.
+	if obj.GetName() == correctCRDName {
+		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 "+
+		"CRDs. If not, this warning can be hidden by setting the %q' annotation.", obj.GetName(), correctCRDName, clusterctlv1.SkipCRDNamePreflightCheckAnnotation)
+}
+
 // getProviderContract returns the API Version of Cluster API (contract) for a provider instance.
 func (i *providerInstaller) getProviderContract(providerInstanceContracts map[string]string, provider clusterctlv1.Provider) (string, error) {
 	// If the contract for the provider instance is already known, return it.

cmd/clusterctl/client/cluster/installer_test.go

@@ -21,6 +21,7 @@ import (
 
 	. "github.com/onsi/gomega"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
+	"k8s.io/apimachinery/pkg/runtime/schema"
 
 	clusterctlv1 "sigs.k8s.io/cluster-api/cmd/clusterctl/api/v1alpha3"
 	"sigs.k8s.io/cluster-api/cmd/clusterctl/client/config"
@@ -257,6 +258,49 @@ func Test_providerInstaller_Validate(t *testing.T) {
 	}
 }
 
+func Test_providerInstaller_ValidateCRDName(t *testing.T) {
+	tests := []struct {
+		name    string
+		crd     unstructured.Unstructured
+		gk      *schema.GroupKind
+		wantErr bool
+	}{
+		{
+			name:    "CRD with valid name",
+			crd:     newFakeCRD("clusterclasses.cluster.x-k8s.io", nil),
+			gk:      &schema.GroupKind{Group: "cluster.x-k8s.io", Kind: "ClusterClass"},
+			wantErr: false,
+		},
+		{
+			name:    "CRD with invalid name",
+			crd:     newFakeCRD("clusterclass.cluster.x-k8s.io", nil),
+			gk:      &schema.GroupKind{Group: "cluster.x-k8s.io", Kind: "ClusterClass"},
+			wantErr: true,
+		},
+		{
+			name: "CRD with invalid name but has skip annotation",
+			crd: newFakeCRD("clusterclass.cluster.x-k8s.io", map[string]string{
+				clusterctlv1.SkipCRDNamePreflightCheckAnnotation: "",
+			}),
+			gk:      &schema.GroupKind{Group: "cluster.x-k8s.io", Kind: "ClusterClass"},
+			wantErr: false,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			g := NewWithT(t)
+
+			err := validateCRDName(tt.crd, tt.gk)
+			if tt.wantErr {
+				g.Expect(err).To(HaveOccurred())
+			} else {
+				g.Expect(err).NotTo(HaveOccurred())
+			}
+		})
+	}
+}
+
 type fakeComponents struct {
 	config.Provider
 	inventoryObject clusterctlv1.Provider
@@ -283,7 +327,7 @@ func (c *fakeComponents) InventoryObject() clusterctlv1.Provider {
 }
 
 func (c *fakeComponents) Objs() []unstructured.Unstructured {
-	panic("not implemented")
+	return []unstructured.Unstructured{}
 }
 
 func (c *fakeComponents) Yaml() ([]byte, error) {
@@ -297,3 +341,10 @@ func newFakeComponents(name string, providerType clusterctlv1.ProviderType, vers
 		inventoryObject: inventoryObject,
 	}
 }
+
+func newFakeCRD(name string, annotations map[string]string) unstructured.Unstructured {
+	u := unstructured.Unstructured{}
+	u.SetName(name)
+	u.SetAnnotations(annotations)
+	return u
+}

cmd/clusterctl/client/init.go

@@ -117,6 +117,8 @@ func (c *clusterctlClient) Init(options InitOptions) ([]Components, error) {
 	// Before installing the providers, validates the management cluster resulting by the planned installation. The following checks are performed:
 	// - There should be only one instance of the same provider.
 	// - All the providers must support the same API Version of Cluster API (contract)
+	// - All provider CRDs that are referenced in core Cluster API CRDs must comply with the CRD naming scheme,
+	//   otherwise a warning is logged.
 	if err := installer.Validate(); err != nil {
 		if !options.IgnoreValidationErrors {
 			return nil, err

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

@@ -45,6 +45,9 @@ The default value is 0, meaning that the volume can be detached without any time
 
 ### Other
 
+- clusterctl now emits a warning 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)
 - The Kubernetes default registry has been changed from `k8s.gcr.io` to `registry.k8s.io`. Kubernetes image promotion currently publishes to both registries. Please
   consider publishing manifests which reference the controller images from the new registry (for reference [Cluster API PR](https://github.com/kubernetes-sigs/cluster-api/pull/7478)).
 - e2e tests are upgraded to use Ginkgo v2 (v2.4.0) and Gomega v1.22.1. Providers who use the test framework from this release will also need to upgrade, because Ginkgo v2 can't be imported alongside v1. Please see the [Ginkgo upgrade guide](https://onsi.github.io/ginkgo/MIGRATING_TO_V2), and note:

docs/book/src/reference/labels_and_annotations.md

@@ -20,6 +20,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.   |
 | 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.   |
@@ -45,4 +46,4 @@
 |  machinedeployment.clusters.x-k8s.io/max-replicas  | It is the maximum replicas a deployment can have at a given point, which is machinedeployment.spec.replicas + maxSurge. Used by the underlying machine sets to estimate their proportions in case the deployment has surge replicas. |
 | controlplane.cluster.x-k8s.io/skip-coredns | It explicitly skips reconciling CoreDNS if set. |
 |controlplane.cluster.x-k8s.io/skip-kube-proxy | It explicitly skips reconciling kube-proxy if set.|
-| controlplane.cluster.x-k8s.io/kubeadm-cluster-configuration| It is a machine annotation that stores the json-marshalled string of KCP ClusterConfiguration. This annotation is used to detect any changes in ClusterConfiguration and trigger machine rollout in KCP.|
+| controlplane.cluster.x-k8s.io/kubeadm-cluster-configuration| It is a machine annotation that stores the json-marshalled string of KCP ClusterConfiguration. This annotation is used to detect any changes in ClusterConfiguration and trigger machine rollout in KCP.|