From 1dea02a1af47dac66d4dd4453d01c69fa4346e51 Mon Sep 17 00:00:00 2001 From: Danil-Grigorev Date: Wed, 30 Oct 2024 10:35:41 +0100 Subject: [PATCH 01/10] Add classNamespace to topology Signed-off-by: Danil-Grigorev --- api/v1beta1/cluster_types.go | 17 ++++++++++++++++- api/v1beta1/zz_generated.openapi.go | 7 +++++++ config/crd/bases/cluster.x-k8s.io_clusters.yaml | 4 ++++ internal/apis/core/v1alpha4/conversion.go | 1 + .../core/v1alpha4/zz_generated.conversion.go | 1 + 5 files changed, 29 insertions(+), 1 deletion(-) diff --git a/api/v1beta1/cluster_types.go b/api/v1beta1/cluster_types.go index b573429366fa..4b546af73b52 100644 --- a/api/v1beta1/cluster_types.go +++ b/api/v1beta1/cluster_types.go @@ -509,6 +509,11 @@ type Topology struct { // The name of the ClusterClass object to create the topology. Class string `json:"class"` + // The namespace of the ClusterClass object to create the topology. + // + // +optional + ClassNamespace string `json:"classNamespace,omitempty"` + // The Kubernetes version of the cluster. Version string `json:"version"` @@ -1037,7 +1042,17 @@ func (c *Cluster) GetClassKey() types.NamespacedName { if c.Spec.Topology == nil { return types.NamespacedName{} } - return types.NamespacedName{Namespace: c.GetNamespace(), Name: c.Spec.Topology.Class} + + return types.NamespacedName{Namespace: c.GetInfrastructureNamespace(), Name: c.Spec.Topology.Class} +} + +// GetInfrastructureNamespace returns common namespace for the cluster infrastructure. +func (c *Cluster) GetInfrastructureNamespace() string { + if c.Spec.Topology == nil || c.Spec.Topology.ClassNamespace == "" { + return c.Namespace + } + + return c.Spec.Topology.ClassNamespace } // GetConditions returns the set of conditions for this object. diff --git a/api/v1beta1/zz_generated.openapi.go b/api/v1beta1/zz_generated.openapi.go index 23f3ecfd8d9c..0f9c9cef455a 100644 --- a/api/v1beta1/zz_generated.openapi.go +++ b/api/v1beta1/zz_generated.openapi.go @@ -4461,6 +4461,13 @@ func schema_sigsk8sio_cluster_api_api_v1beta1_Topology(ref common.ReferenceCallb Format: "", }, }, + "classNamespace": { + SchemaProps: spec.SchemaProps{ + Description: "The namespace of the ClusterClass object to create the topology.", + Type: []string{"string"}, + Format: "", + }, + }, "version": { SchemaProps: spec.SchemaProps{ Description: "The Kubernetes version of the cluster.", diff --git a/config/crd/bases/cluster.x-k8s.io_clusters.yaml b/config/crd/bases/cluster.x-k8s.io_clusters.yaml index 4ad16356145b..0deba724fe95 100644 --- a/config/crd/bases/cluster.x-k8s.io_clusters.yaml +++ b/config/crd/bases/cluster.x-k8s.io_clusters.yaml @@ -928,6 +928,10 @@ spec: description: The name of the ClusterClass object to create the topology. type: string + classNamespace: + description: The namespace of the ClusterClass object to create + the topology. + type: string controlPlane: description: controlPlane describes the cluster control plane. properties: diff --git a/internal/apis/core/v1alpha4/conversion.go b/internal/apis/core/v1alpha4/conversion.go index cd69ca35d2ba..8e2ed513d13d 100644 --- a/internal/apis/core/v1alpha4/conversion.go +++ b/internal/apis/core/v1alpha4/conversion.go @@ -42,6 +42,7 @@ func (src *Cluster) ConvertTo(dstRaw conversion.Hub) error { if dst.Spec.Topology == nil { dst.Spec.Topology = &clusterv1.Topology{} } + dst.Spec.Topology.ClassNamespace = restored.Spec.Topology.ClassNamespace dst.Spec.Topology.Variables = restored.Spec.Topology.Variables dst.Spec.Topology.ControlPlane.Variables = restored.Spec.Topology.ControlPlane.Variables diff --git a/internal/apis/core/v1alpha4/zz_generated.conversion.go b/internal/apis/core/v1alpha4/zz_generated.conversion.go index 14df0dfc9112..f5c0433a653d 100644 --- a/internal/apis/core/v1alpha4/zz_generated.conversion.go +++ b/internal/apis/core/v1alpha4/zz_generated.conversion.go @@ -1757,6 +1757,7 @@ func Convert_v1alpha4_Topology_To_v1beta1_Topology(in *Topology, out *v1beta1.To func autoConvert_v1beta1_Topology_To_v1alpha4_Topology(in *v1beta1.Topology, out *Topology, s conversion.Scope) error { out.Class = in.Class + // WARNING: in.ClassNamespace requires manual conversion: does not exist in peer-type out.Version = in.Version out.RolloutAfter = (*metav1.Time)(unsafe.Pointer(in.RolloutAfter)) if err := Convert_v1beta1_ControlPlaneTopology_To_v1alpha4_ControlPlaneTopology(&in.ControlPlane, &out.ControlPlane, s); err != nil { From ccc32d171b8748e3d362d666dfd4873c77c0a9ab Mon Sep 17 00:00:00 2001 From: Danil-Grigorev Date: Wed, 30 Oct 2024 16:04:09 +0100 Subject: [PATCH 02/10] Add documentation on securing cross-namespace access for CC Signed-off-by: Danil-Grigorev --- .../cluster-class/write-clusterclass.md | 75 +++++++++++++++++++ 1 file changed, 75 insertions(+) diff --git a/docs/book/src/tasks/experimental-features/cluster-class/write-clusterclass.md b/docs/book/src/tasks/experimental-features/cluster-class/write-clusterclass.md index 1c722cfcf5f9..e9dea671cf24 100644 --- a/docs/book/src/tasks/experimental-features/cluster-class/write-clusterclass.md +++ b/docs/book/src/tasks/experimental-features/cluster-class/write-clusterclass.md @@ -438,6 +438,81 @@ spec: template: "{{ .cluster.name }}-{{ .machinePool.topologyName }}-{{ .random }}" ``` +### Defining a custom namespace for ClusterClass object + +As a user, I may need to create a `Cluster` from a `ClusterClass` object that exists only in a different namespace. To uniquely identify the `ClusterClass`, a `NamespaceName` ref is constructed from combination of: +* `cluster.spec.topology.classNamespace` - namespace of the `ClusterClass` object. +* `cluster.spec.topology.class` - name of the `ClusterClass` object. + +Example of the `Cluster` object with the `name/namespace` reference: + +```yaml +apiVersion: cluster.x-k8s.io/v1beta1 +kind: Cluster +metadata: + name: my-docker-cluster + namespace: default +spec: + topology: + class: docker-clusterclass-v0.1.0 + classNamespace: default + version: v1.22.4 + controlPlane: + replicas: 3 + workers: + machineDeployments: + - class: default-worker + name: md-0 + replicas: 4 + failureDomain: region +``` + +#### Securing cross-namespace reference to the ClusterClass + +It is often desirable to restrict free cross-namespace `ClusterClass` access for the `Cluster` object. This can be implemented by defining a [`ValidatingAdmissionPolicy`](https://kubernetes.io/docs/reference/access-authn-authz/validating-admission-policy/) on the `Cluster` object. + +An example of such policy may be: + +```yaml +apiVersion: admissionregistration.k8s.io/v1 +kind: ValidatingAdmissionPolicy +metadata: + name: "cluster-class-ref.cluster.x-k8s.io" +spec: + failurePolicy: Fail + paramKind: + apiVersion: v1 + kind: Secret + matchConstraints: + resourceRules: + - apiGroups: ["cluster.x-k8s.io"] + apiVersions: ["v1beta1"] + operations: ["CREATE", "UPDATE"] + resources: ["clusters"] + validations: + - expression: "!has(object.spec.topology.classNamespace) || object.spec.topology.classNamespace in params.data" +--- +apiVersion: admissionregistration.k8s.io/v1 +kind: ValidatingAdmissionPolicyBinding +metadata: + name: "cluster-class-ref-binding.cluster.x-k8s.io" +spec: + policyName: "cluster-class-ref.cluster.x-k8s.io" + validationActions: [Deny] + paramRef: + name: "ref-list" + namespace: "default" + parameterNotFoundAction: Deny +--- +apiVersion: v1 +kind: Secret +metadata: + name: "ref-list" + namespace: "default" +data: + default: "" +``` + ## Advanced features of ClusterClass with patches This section will explain more advanced features of ClusterClass patches. From 65db336e2fbaf0aef2ff1ea7fde1f8951edaad32 Mon Sep 17 00:00:00 2001 From: Danil-Grigorev Date: Thu, 7 Nov 2024 12:23:16 +0100 Subject: [PATCH 03/10] Lookup Clusters across namespaces Signed-off-by: Danil-Grigorev --- .../topology/cluster/cluster_controller.go | 5 +++-- internal/webhooks/clusterclass.go | 11 +++++++++-- 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/internal/controllers/topology/cluster/cluster_controller.go b/internal/controllers/topology/cluster/cluster_controller.go index ef8faedfc244..b45da6f49259 100644 --- a/internal/controllers/topology/cluster/cluster_controller.go +++ b/internal/controllers/topology/cluster/cluster_controller.go @@ -373,7 +373,6 @@ func (r *Reconciler) clusterClassToCluster(ctx context.Context, o client.Object) ctx, clusterList, client.MatchingFields{index.ClusterClassNameField: clusterClass.Name}, - client.InNamespace(clusterClass.Namespace), ); err != nil { return nil } @@ -382,7 +381,9 @@ func (r *Reconciler) clusterClassToCluster(ctx context.Context, o client.Object) // create a request for each of the clusters. requests := []ctrl.Request{} for i := range clusterList.Items { - requests = append(requests, ctrl.Request{NamespacedName: util.ObjectKey(&clusterList.Items[i])}) + if clusterList.Items[i].GetInfrastructureNamespace() == clusterClass.Namespace { + requests = append(requests, ctrl.Request{NamespacedName: util.ObjectKey(&clusterList.Items[i])}) + } } return requests } diff --git a/internal/webhooks/clusterclass.go b/internal/webhooks/clusterclass.go index 65fa7a2d69cd..49bc76976212 100644 --- a/internal/webhooks/clusterclass.go +++ b/internal/webhooks/clusterclass.go @@ -380,12 +380,19 @@ func (webhook *ClusterClass) getClustersUsingClusterClass(ctx context.Context, c clusters := &clusterv1.ClusterList{} err := webhook.Client.List(ctx, clusters, client.MatchingFields{index.ClusterClassNameField: clusterClass.Name}, - client.InNamespace(clusterClass.Namespace), ) if err != nil { return nil, err } - return clusters.Items, nil + + referencedClusters := []clusterv1.Cluster{} + for _, cluster := range clusters.Items { + if cluster.GetInfrastructureNamespace() == clusterClass.Namespace { + referencedClusters = append(referencedClusters, cluster) + } + } + + return referencedClusters, nil } func getClusterClassVariablesMapWithReverseIndex(clusterClassVariables []clusterv1.ClusterClassVariable) (map[string]*clusterv1.ClusterClassVariable, map[string]int) { From 5d9542b702f50c494dd403d01940cdee663cd4f5 Mon Sep 17 00:00:00 2001 From: Danil-Grigorev Date: Thu, 7 Nov 2024 12:37:02 +0100 Subject: [PATCH 04/10] Add a note on cross-ns rebase Signed-off-by: Danil-Grigorev --- .../cluster-class/write-clusterclass.md | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/docs/book/src/tasks/experimental-features/cluster-class/write-clusterclass.md b/docs/book/src/tasks/experimental-features/cluster-class/write-clusterclass.md index e9dea671cf24..10fe2568610b 100644 --- a/docs/book/src/tasks/experimental-features/cluster-class/write-clusterclass.md +++ b/docs/book/src/tasks/experimental-features/cluster-class/write-clusterclass.md @@ -467,6 +467,14 @@ spec: failureDomain: region ``` + + #### Securing cross-namespace reference to the ClusterClass It is often desirable to restrict free cross-namespace `ClusterClass` access for the `Cluster` object. This can be implemented by defining a [`ValidatingAdmissionPolicy`](https://kubernetes.io/docs/reference/access-authn-authz/validating-admission-policy/) on the `Cluster` object. From b4b6131f183d51f029e26da76cc044af49e771ec Mon Sep 17 00:00:00 2001 From: Danil-Grigorev Date: Wed, 27 Nov 2024 21:38:24 +0100 Subject: [PATCH 05/10] Add validation on classNamespace, API review addressed Signed-off-by: Danil-Grigorev --- api/v1beta1/cluster_types.go | 15 +++++++-------- config/crd/bases/cluster.x-k8s.io_clusters.yaml | 3 +++ .../cluster-class/write-clusterclass.md | 6 +++--- .../topology/cluster/cluster_controller.go | 2 +- internal/webhooks/clusterclass.go | 2 +- 5 files changed, 15 insertions(+), 13 deletions(-) diff --git a/api/v1beta1/cluster_types.go b/api/v1beta1/cluster_types.go index 4b546af73b52..41a19fb3e2bd 100644 --- a/api/v1beta1/cluster_types.go +++ b/api/v1beta1/cluster_types.go @@ -510,8 +510,11 @@ type Topology struct { Class string `json:"class"` // The namespace of the ClusterClass object to create the topology. - // + // Empty namespace assumes the namespace of the cluster object. // +optional + // +kubebuilder:validation:MinLength=1 + // +kubebuilder:validation:MaxLength=253 + // +kubebuilder:validation:Pattern="^[a-z0-9](?:[-a-z0-9]*[a-z0-9])?(?:\\.[a-z0-9](?:[-a-z0-9]*[a-z0-9])?)*$" ClassNamespace string `json:"classNamespace,omitempty"` // The Kubernetes version of the cluster. @@ -1043,16 +1046,12 @@ func (c *Cluster) GetClassKey() types.NamespacedName { return types.NamespacedName{} } - return types.NamespacedName{Namespace: c.GetInfrastructureNamespace(), Name: c.Spec.Topology.Class} -} - -// GetInfrastructureNamespace returns common namespace for the cluster infrastructure. -func (c *Cluster) GetInfrastructureNamespace() string { + namespace := c.Spec.Topology.ClassNamespace if c.Spec.Topology == nil || c.Spec.Topology.ClassNamespace == "" { - return c.Namespace + namespace = c.Namespace } - return c.Spec.Topology.ClassNamespace + return types.NamespacedName{Namespace: namespace, Name: c.Spec.Topology.Class} } // GetConditions returns the set of conditions for this object. diff --git a/config/crd/bases/cluster.x-k8s.io_clusters.yaml b/config/crd/bases/cluster.x-k8s.io_clusters.yaml index 0deba724fe95..b141266dd6b3 100644 --- a/config/crd/bases/cluster.x-k8s.io_clusters.yaml +++ b/config/crd/bases/cluster.x-k8s.io_clusters.yaml @@ -931,6 +931,9 @@ spec: classNamespace: description: The namespace of the ClusterClass object to create the topology. + maxLength: 253 + minLength: 1 + pattern: ^[a-z0-9](?:[-a-z0-9]*[a-z0-9])?(?:\.[a-z0-9](?:[-a-z0-9]*[a-z0-9])?)*$ type: string controlPlane: description: controlPlane describes the cluster control plane. diff --git a/docs/book/src/tasks/experimental-features/cluster-class/write-clusterclass.md b/docs/book/src/tasks/experimental-features/cluster-class/write-clusterclass.md index 10fe2568610b..f9384befed58 100644 --- a/docs/book/src/tasks/experimental-features/cluster-class/write-clusterclass.md +++ b/docs/book/src/tasks/experimental-features/cluster-class/write-clusterclass.md @@ -471,7 +471,7 @@ spec:

Cluster rebase across namespaces

-Class namespace referenced in the `Cluster` object is equivalent to a cluster being located in the referenced namespace from the validation perspective. Changing `classNamespace` is not allowed, while using a different `CluterClass` from the same namespace is permitted in the Cluster rebase procedure. +Changing `classNamespace` is not supported in rebase procedure, while chanding `class` reference to a different `ClusterClass` from the same namespace performs regular `Cluster` rebase procedure. @@ -508,14 +508,14 @@ spec: policyName: "cluster-class-ref.cluster.x-k8s.io" validationActions: [Deny] paramRef: - name: "ref-list" + name: "allowed-namespaces.cluster-class-ref.cluster.x-k8s.io" namespace: "default" parameterNotFoundAction: Deny --- apiVersion: v1 kind: Secret metadata: - name: "ref-list" + name: "allowed-namespaces.cluster-class-ref.cluster.x-k8s.io" namespace: "default" data: default: "" diff --git a/internal/controllers/topology/cluster/cluster_controller.go b/internal/controllers/topology/cluster/cluster_controller.go index b45da6f49259..9eb7a4bc1c4a 100644 --- a/internal/controllers/topology/cluster/cluster_controller.go +++ b/internal/controllers/topology/cluster/cluster_controller.go @@ -381,7 +381,7 @@ func (r *Reconciler) clusterClassToCluster(ctx context.Context, o client.Object) // create a request for each of the clusters. requests := []ctrl.Request{} for i := range clusterList.Items { - if clusterList.Items[i].GetInfrastructureNamespace() == clusterClass.Namespace { + if clusterList.Items[i].GetClassKey().Namespace == clusterClass.Namespace { requests = append(requests, ctrl.Request{NamespacedName: util.ObjectKey(&clusterList.Items[i])}) } } diff --git a/internal/webhooks/clusterclass.go b/internal/webhooks/clusterclass.go index 49bc76976212..0ffab383e3c9 100644 --- a/internal/webhooks/clusterclass.go +++ b/internal/webhooks/clusterclass.go @@ -387,7 +387,7 @@ func (webhook *ClusterClass) getClustersUsingClusterClass(ctx context.Context, c referencedClusters := []clusterv1.Cluster{} for _, cluster := range clusters.Items { - if cluster.GetInfrastructureNamespace() == clusterClass.Namespace { + if cluster.GetClassKey().Namespace == clusterClass.Namespace { referencedClusters = append(referencedClusters, cluster) } } From e16aa76ccb936f3016352ad4307ee65c7752db04 Mon Sep 17 00:00:00 2001 From: Danil-Grigorev Date: Tue, 3 Dec 2024 15:45:27 +0100 Subject: [PATCH 06/10] Add tests for clusterclassToCluster, godoc changes Signed-off-by: Danil-Grigorev --- api/v1beta1/cluster_types.go | 8 +-- api/v1beta1/zz_generated.openapi.go | 2 +- .../crd/bases/cluster.x-k8s.io_clusters.yaml | 6 +- .../cluster/cluster_controller_test.go | 68 +++++++++++++++++++ util/test/builder/builders.go | 15 ++-- 5 files changed, 87 insertions(+), 12 deletions(-) diff --git a/api/v1beta1/cluster_types.go b/api/v1beta1/cluster_types.go index 41a19fb3e2bd..d8cc308e0f01 100644 --- a/api/v1beta1/cluster_types.go +++ b/api/v1beta1/cluster_types.go @@ -17,6 +17,7 @@ limitations under the License. package v1beta1 import ( + "cmp" "fmt" "net" "strings" @@ -511,6 +512,7 @@ type Topology struct { // The namespace of the ClusterClass object to create the topology. // Empty namespace assumes the namespace of the cluster object. + // Class namespace changes are not supported by the rebase procedure. // +optional // +kubebuilder:validation:MinLength=1 // +kubebuilder:validation:MaxLength=253 @@ -1046,11 +1048,7 @@ func (c *Cluster) GetClassKey() types.NamespacedName { return types.NamespacedName{} } - namespace := c.Spec.Topology.ClassNamespace - if c.Spec.Topology == nil || c.Spec.Topology.ClassNamespace == "" { - namespace = c.Namespace - } - + namespace := cmp.Or(c.Spec.Topology.ClassNamespace, c.Namespace) return types.NamespacedName{Namespace: namespace, Name: c.Spec.Topology.Class} } diff --git a/api/v1beta1/zz_generated.openapi.go b/api/v1beta1/zz_generated.openapi.go index 0f9c9cef455a..625b3ba2332b 100644 --- a/api/v1beta1/zz_generated.openapi.go +++ b/api/v1beta1/zz_generated.openapi.go @@ -4463,7 +4463,7 @@ func schema_sigsk8sio_cluster_api_api_v1beta1_Topology(ref common.ReferenceCallb }, "classNamespace": { SchemaProps: spec.SchemaProps{ - Description: "The namespace of the ClusterClass object to create the topology.", + Description: "The namespace of the ClusterClass object to create the topology. Empty namespace assumes the namespace of the cluster object. Class namespace changes are not supported by the rebase procedure.", Type: []string{"string"}, Format: "", }, diff --git a/config/crd/bases/cluster.x-k8s.io_clusters.yaml b/config/crd/bases/cluster.x-k8s.io_clusters.yaml index b141266dd6b3..665796ce5d9c 100644 --- a/config/crd/bases/cluster.x-k8s.io_clusters.yaml +++ b/config/crd/bases/cluster.x-k8s.io_clusters.yaml @@ -929,8 +929,10 @@ spec: topology. type: string classNamespace: - description: The namespace of the ClusterClass object to create - the topology. + description: |- + The namespace of the ClusterClass object to create the topology. + Empty namespace assumes the namespace of the cluster object. + Namespace changes are not supported by rebase procedure. maxLength: 253 minLength: 1 pattern: ^[a-z0-9](?:[-a-z0-9]*[a-z0-9])?(?:\.[a-z0-9](?:[-a-z0-9]*[a-z0-9])?)*$ diff --git a/internal/controllers/topology/cluster/cluster_controller_test.go b/internal/controllers/topology/cluster/cluster_controller_test.go index 61fc1d8dd810..fd139066a194 100644 --- a/internal/controllers/topology/cluster/cluster_controller_test.go +++ b/internal/controllers/topology/cluster/cluster_controller_test.go @@ -52,6 +52,7 @@ import ( var ( clusterName1 = "cluster1" clusterName2 = "cluster2" + clusterName3 = "cluster3" clusterClassName1 = "class1" clusterClassName2 = "class2" infrastructureMachineTemplateName1 = "inframachinetemplate1" @@ -854,6 +855,19 @@ func setupTestEnvForIntegrationTests(ns *corev1.Namespace) (func() error, error) Build()). Build() + // Cross ns referencing cluster + cluster3 := builder.Cluster(ns.Name, clusterName3). + WithTopology( + builder.ClusterTopology(). + WithClass(clusterClass.Name). + WithClassNamespace("other"). + WithMachineDeployment(machineDeploymentTopology2). + WithMachinePool(machinePoolTopology2). + WithVersion("1.21.0"). + WithControlPlaneReplicas(1). + Build()). + Build() + // Setup kubeconfig secrets for the clusters, so the ClusterCacheTracker works. cluster1Secret := kubeconfig.GenerateSecret(cluster1, kubeconfig.FromEnvTestConfig(env.Config, cluster1)) cluster2Secret := kubeconfig.GenerateSecret(cluster2, kubeconfig.FromEnvTestConfig(env.Config, cluster2)) @@ -876,6 +890,7 @@ func setupTestEnvForIntegrationTests(ns *corev1.Namespace) (func() error, error) clusterClassForRebase, cluster1, cluster2, + cluster3, cluster1Secret, cluster2Secret, } @@ -1577,3 +1592,56 @@ func TestReconciler_ValidateCluster(t *testing.T) { }) } } + +func TestClusterClassToCluster(t *testing.T) { + utilfeature.SetFeatureGateDuringTest(t, feature.Gates, feature.ClusterTopology, true) + g := NewWithT(t) + + ns, err := env.CreateNamespace(ctx, "cluster-reconcile-namespace") + g.Expect(err).ToNot(HaveOccurred()) + + // Create the objects needed for the integration test: + cleanup, err := setupTestEnvForIntegrationTests(ns) + g.Expect(err).ToNot(HaveOccurred()) + + // Defer a cleanup function that deletes each of the objects created during setupTestEnvForIntegrationTests. + defer func() { + g.Expect(cleanup()).To(Succeed()) + }() + + tests := []struct { + name string + clusterClass *clusterv1.ClusterClass + expected []reconcile.Request + }{ + { + name: "ClusterClass change should request reconcile for the referenced class", + clusterClass: builder.ClusterClass(ns.Name, clusterClassName1).Build(), + expected: []reconcile.Request{ + {NamespacedName: client.ObjectKeyFromObject(builder.Cluster(ns.Name, clusterName1).Build())}, + {NamespacedName: client.ObjectKeyFromObject(builder.Cluster(ns.Name, clusterName2).Build())}, + }, + }, + { + name: "ClusterClass with no matching name and namespace should not trigger reconcile", + clusterClass: builder.ClusterClass("other", clusterClassName2).Build(), + expected: []reconcile.Request{}, + }, + { + name: "Different ClusterClass with matching name and namespace should trigger reconcile", + clusterClass: builder.ClusterClass("other", clusterClassName1).Build(), + expected: []reconcile.Request{ + {NamespacedName: client.ObjectKeyFromObject(builder.Cluster(ns.Name, clusterName3).Build())}, + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(*testing.T) { + r := &Reconciler{Client: env.GetClient()} + + requests := r.clusterClassToCluster(ctx, tt.clusterClass) + g.Expect(requests).To(Equal(tt.expected)) + }) + } +} diff --git a/util/test/builder/builders.go b/util/test/builder/builders.go index dd2ae1dc881f..762b418fe835 100644 --- a/util/test/builder/builders.go +++ b/util/test/builder/builders.go @@ -114,7 +114,7 @@ func (c *ClusterBuilder) Build() *clusterv1.Cluster { // ClusterTopologyBuilder contains the fields needed to build a testable ClusterTopology. type ClusterTopologyBuilder struct { - class string + class, classNamespace string workers *clusterv1.WorkersTopology version string controlPlaneReplicas int32 @@ -136,6 +136,12 @@ func (c *ClusterTopologyBuilder) WithClass(class string) *ClusterTopologyBuilder return c } +// WithClassNamespace adds the passed classNamespace to the ClusterTopologyBuilder. +func (c *ClusterTopologyBuilder) WithClassNamespace(ns string) *ClusterTopologyBuilder { + c.classNamespace = ns + return c +} + // WithVersion adds the passed version to the ClusterTopologyBuilder. func (c *ClusterTopologyBuilder) WithVersion(version string) *ClusterTopologyBuilder { c.version = version @@ -181,9 +187,10 @@ func (c *ClusterTopologyBuilder) WithVariables(vars ...clusterv1.ClusterVariable // Build returns a testable cluster Topology object with any values passed to the builder. func (c *ClusterTopologyBuilder) Build() *clusterv1.Topology { t := &clusterv1.Topology{ - Class: c.class, - Workers: c.workers, - Version: c.version, + Class: c.class, + ClassNamespace: c.classNamespace, + Workers: c.workers, + Version: c.version, ControlPlane: clusterv1.ControlPlaneTopology{ Replicas: &c.controlPlaneReplicas, MachineHealthCheck: c.controlPlaneMHC, From 36393649203f8ef50ee6784f4024b13d3ea58697 Mon Sep 17 00:00:00 2001 From: Danil-Grigorev Date: Wed, 4 Dec 2024 13:57:35 +0100 Subject: [PATCH 07/10] Add ClassNamespace index Signed-off-by: Danil-Grigorev --- api/v1beta1/index/cluster.go | 23 +++++++ api/v1beta1/index/index.go | 4 ++ .../topology/cluster/cluster_controller.go | 10 +-- .../topology/cluster/suite_test.go | 5 +- internal/webhooks/clusterclass.go | 15 ++--- internal/webhooks/clusterclass_test.go | 67 +++++++++++++++++++ 6 files changed, 110 insertions(+), 14 deletions(-) diff --git a/api/v1beta1/index/cluster.go b/api/v1beta1/index/cluster.go index d61d8c36f90d..226145db9753 100644 --- a/api/v1beta1/index/cluster.go +++ b/api/v1beta1/index/cluster.go @@ -30,6 +30,8 @@ import ( const ( // ClusterClassNameField is used by the Cluster controller to index Clusters by ClusterClass name. ClusterClassNameField = "spec.topology.class" + // ClusterClassNamespaceField is used by the Cluster controller to index Clusters by ClusterClass namespace. + ClusterClassNamespaceField = "spec.topology.classNamespace" ) // ByClusterClassName adds the cluster class name index to the @@ -44,6 +46,18 @@ func ByClusterClassName(ctx context.Context, mgr ctrl.Manager) error { return nil } +// ByClusterClassNamespace adds the cluster class namespace index to the +// managers cache. +func ByClusterClassNamespace(ctx context.Context, mgr ctrl.Manager) error { + if err := mgr.GetCache().IndexField(ctx, &clusterv1.Cluster{}, + ClusterClassNamespaceField, + ClusterByClusterClassClassNamespace, + ); err != nil { + return errors.Wrap(err, "error setting index field") + } + return nil +} + // ClusterByClusterClassClassName contains the logic to index Clusters by ClusterClass name. func ClusterByClusterClassClassName(o client.Object) []string { cluster, ok := o.(*clusterv1.Cluster) @@ -55,3 +69,12 @@ func ClusterByClusterClassClassName(o client.Object) []string { } return nil } + +// ClusterByClusterClassClassNamespace contains the logic to index Clusters by ClusterClass namespace. +func ClusterByClusterClassClassNamespace(o client.Object) []string { + cluster, ok := o.(*clusterv1.Cluster) + if !ok { + panic(fmt.Sprintf("Expected Cluster but got a %T", o)) + } + return []string{cluster.GetClassKey().Namespace} +} diff --git a/api/v1beta1/index/index.go b/api/v1beta1/index/index.go index c6a17bf175bc..b5f0e7d9df55 100644 --- a/api/v1beta1/index/index.go +++ b/api/v1beta1/index/index.go @@ -39,6 +39,10 @@ func AddDefaultIndexes(ctx context.Context, mgr ctrl.Manager) error { if err := ByClusterClassName(ctx, mgr); err != nil { return err } + + if err := ByClusterClassNamespace(ctx, mgr); err != nil { + return err + } } if feature.Gates.Enabled(feature.MachinePool) { diff --git a/internal/controllers/topology/cluster/cluster_controller.go b/internal/controllers/topology/cluster/cluster_controller.go index 9eb7a4bc1c4a..6afaf19bc763 100644 --- a/internal/controllers/topology/cluster/cluster_controller.go +++ b/internal/controllers/topology/cluster/cluster_controller.go @@ -24,6 +24,7 @@ import ( "github.com/go-logr/logr" "github.com/pkg/errors" apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/fields" "k8s.io/apimachinery/pkg/types" kerrors "k8s.io/apimachinery/pkg/util/errors" "k8s.io/client-go/tools/record" @@ -372,7 +373,10 @@ func (r *Reconciler) clusterClassToCluster(ctx context.Context, o client.Object) if err := r.Client.List( ctx, clusterList, - client.MatchingFields{index.ClusterClassNameField: clusterClass.Name}, + client.MatchingFieldsSelector{Selector: fields.AndSelectors( + fields.OneTermEqualSelector(index.ClusterClassNameField, clusterClass.Name), + fields.OneTermEqualSelector(index.ClusterClassNamespaceField, clusterClass.Namespace), + )}, ); err != nil { return nil } @@ -381,9 +385,7 @@ func (r *Reconciler) clusterClassToCluster(ctx context.Context, o client.Object) // create a request for each of the clusters. requests := []ctrl.Request{} for i := range clusterList.Items { - if clusterList.Items[i].GetClassKey().Namespace == clusterClass.Namespace { - requests = append(requests, ctrl.Request{NamespacedName: util.ObjectKey(&clusterList.Items[i])}) - } + requests = append(requests, ctrl.Request{NamespacedName: util.ObjectKey(&clusterList.Items[i])}) } return requests } diff --git a/internal/controllers/topology/cluster/suite_test.go b/internal/controllers/topology/cluster/suite_test.go index 715f4a52bd47..32a41031689e 100644 --- a/internal/controllers/topology/cluster/suite_test.go +++ b/internal/controllers/topology/cluster/suite_test.go @@ -60,11 +60,14 @@ func TestMain(m *testing.M) { if err := index.AddDefaultIndexes(ctx, mgr); err != nil { panic(fmt.Sprintf("unable to setup index: %v", err)) } - // Set up the ClusterClassName index explicitly here. This index is ordinarily created in + // Set up the ClusterClassName and ClusterClassNamespace index explicitly here. This index is ordinarily created in // index.AddDefaultIndexes. That doesn't happen here because the ClusterClass feature flag is not set. if err := index.ByClusterClassName(ctx, mgr); err != nil { panic(fmt.Sprintf("unable to setup index: %v", err)) } + if err := index.ByClusterClassNamespace(ctx, mgr); err != nil { + panic(fmt.Sprintf("unable to setup index: %v", err)) + } } setupReconcilers := func(ctx context.Context, mgr ctrl.Manager) { clusterCache, err := clustercache.SetupWithManager(ctx, mgr, clustercache.Options{ diff --git a/internal/webhooks/clusterclass.go b/internal/webhooks/clusterclass.go index 0ffab383e3c9..d89394051356 100644 --- a/internal/webhooks/clusterclass.go +++ b/internal/webhooks/clusterclass.go @@ -25,6 +25,7 @@ import ( corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/fields" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/sets" "k8s.io/apimachinery/pkg/util/validation" @@ -379,20 +380,16 @@ func (webhook *ClusterClass) classNamesFromMPWorkerClass(w clusterv1.WorkersClas func (webhook *ClusterClass) getClustersUsingClusterClass(ctx context.Context, clusterClass *clusterv1.ClusterClass) ([]clusterv1.Cluster, error) { clusters := &clusterv1.ClusterList{} err := webhook.Client.List(ctx, clusters, - client.MatchingFields{index.ClusterClassNameField: clusterClass.Name}, + client.MatchingFieldsSelector{Selector: fields.AndSelectors( + fields.OneTermEqualSelector(index.ClusterClassNameField, clusterClass.Name), + fields.OneTermEqualSelector(index.ClusterClassNamespaceField, clusterClass.Namespace), + )}, ) if err != nil { return nil, err } - referencedClusters := []clusterv1.Cluster{} - for _, cluster := range clusters.Items { - if cluster.GetClassKey().Namespace == clusterClass.Namespace { - referencedClusters = append(referencedClusters, cluster) - } - } - - return referencedClusters, nil + return clusters.Items, nil } func getClusterClassVariablesMapWithReverseIndex(clusterClassVariables []clusterv1.ClusterClassVariable) (map[string]*clusterv1.ClusterClassVariable, map[string]int) { diff --git a/internal/webhooks/clusterclass_test.go b/internal/webhooks/clusterclass_test.go index 49b8928529ad..639b9dc71643 100644 --- a/internal/webhooks/clusterclass_test.go +++ b/internal/webhooks/clusterclass_test.go @@ -92,6 +92,7 @@ func TestClusterClassDefaultNamespaces(t *testing.T) { fakeClient := fake.NewClientBuilder(). WithScheme(fakeScheme). WithIndex(&clusterv1.Cluster{}, index.ClusterClassNameField, index.ClusterByClusterClassClassName). + WithIndex(&clusterv1.Cluster{}, index.ClusterClassNamespaceField, index.ClusterByClusterClassClassNamespace). Build() // Create the webhook and add the fakeClient as its client. @@ -1866,6 +1867,7 @@ func TestClusterClassValidation(t *testing.T) { fakeClient := fake.NewClientBuilder(). WithScheme(fakeScheme). WithIndex(&clusterv1.Cluster{}, index.ClusterClassNameField, index.ClusterByClusterClassClassName). + WithIndex(&clusterv1.Cluster{}, index.ClusterClassNamespaceField, index.ClusterByClusterClassClassNamespace). Build() // Pin the compatibility version used in variable CEL validation to 1.29, so we don't have to continuously refactor @@ -2513,6 +2515,7 @@ func TestClusterClassValidationWithClusterAwareChecks(t *testing.T) { WithScheme(fakeScheme). WithObjects(tt.clusters...). WithIndex(&clusterv1.Cluster{}, index.ClusterClassNameField, index.ClusterByClusterClassClassName). + WithIndex(&clusterv1.Cluster{}, index.ClusterClassNamespaceField, index.ClusterByClusterClassClassNamespace). Build() // Create the webhook and add the fakeClient as its client. @@ -2527,6 +2530,70 @@ func TestClusterClassValidationWithClusterAwareChecks(t *testing.T) { } } +func TestGetClustersUsingClusterClass(t *testing.T) { + // NOTE: ClusterTopology feature flag is disabled by default, thus preventing to create or update ClusterClasses. + // Enabling the feature flag temporarily for this test. + utilfeature.SetFeatureGateDuringTest(t, feature.Gates, feature.ClusterTopology, true) + + topology := builder.ClusterTopology().WithClass("class1") + + tests := []struct { + name string + clusterClass *clusterv1.ClusterClass + clusters []client.Object + expectErr bool + expectClusters []client.Object + }{ + { + name: "ClusterClass should return clusters referencing it", + clusterClass: builder.ClusterClass("default", "class1").Build(), + clusters: []client.Object{ + builder.Cluster("default", "cluster1").WithTopology(topology.Build()).Build(), + builder.Cluster("default", "cluster2").Build(), + builder.Cluster("other", "cluster2").WithTopology(topology.DeepCopy().WithClassNamespace("default").Build()).Build(), + builder.Cluster("other", "cluster3").WithTopology(topology.Build()).Build(), + }, + expectClusters: []client.Object{ + builder.Cluster("default", "cluster1").Build(), + builder.Cluster("other", "cluster2").Build(), + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + + // Sets up the fakeClient for the test case. + fakeClient := fake.NewClientBuilder(). + WithScheme(fakeScheme). + WithObjects(tt.clusters...). + WithIndex(&clusterv1.Cluster{}, index.ClusterClassNameField, index.ClusterByClusterClassClassName). + WithIndex(&clusterv1.Cluster{}, index.ClusterClassNamespaceField, index.ClusterByClusterClassClassNamespace). + Build() + + // Create the webhook and add the fakeClient as its client. + webhook := &ClusterClass{Client: fakeClient} + clusters, err := webhook.getClustersUsingClusterClass(ctx, tt.clusterClass) + if tt.expectErr { + g.Expect(err).To(HaveOccurred()) + return + } + g.Expect(err).ToNot(HaveOccurred()) + + clusterKeys := []client.ObjectKey{} + for _, c := range clusters { + clusterKeys = append(clusterKeys, client.ObjectKeyFromObject(&c)) + } + expectedKeys := []client.ObjectKey{} + for _, c := range tt.expectClusters { + expectedKeys = append(expectedKeys, client.ObjectKeyFromObject(c)) + } + g.Expect(clusterKeys).To(Equal(expectedKeys)) + }) + } +} + func TestValidateAutoscalerAnnotationsForClusterClass(t *testing.T) { tests := []struct { name string From 1a88697438d1e89c1f59bd0e2ccba6f41c8f7f7f Mon Sep 17 00:00:00 2001 From: Danil-Grigorev Date: Thu, 5 Dec 2024 13:55:36 +0100 Subject: [PATCH 08/10] Clarify cross-namespace rebase support godoc Signed-off-by: Danil-Grigorev --- api/v1beta1/cluster_types.go | 6 +++--- api/v1beta1/zz_generated.openapi.go | 2 +- config/crd/bases/cluster.x-k8s.io_clusters.yaml | 6 +++--- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/api/v1beta1/cluster_types.go b/api/v1beta1/cluster_types.go index d8cc308e0f01..907df8d0be5b 100644 --- a/api/v1beta1/cluster_types.go +++ b/api/v1beta1/cluster_types.go @@ -510,9 +510,9 @@ type Topology struct { // The name of the ClusterClass object to create the topology. Class string `json:"class"` - // The namespace of the ClusterClass object to create the topology. - // Empty namespace assumes the namespace of the cluster object. - // Class namespace changes are not supported by the rebase procedure. + // The namespace of the ClusterClass object to create the topology. Empty namespace assumes the namespace of the cluster object. + // Class namespace changes are not supported by the rebase procedure, as different CC namespace uses namespace-local templates. + // Cluster templates namespace modification is not allowed. // +optional // +kubebuilder:validation:MinLength=1 // +kubebuilder:validation:MaxLength=253 diff --git a/api/v1beta1/zz_generated.openapi.go b/api/v1beta1/zz_generated.openapi.go index 625b3ba2332b..47c5cdd84813 100644 --- a/api/v1beta1/zz_generated.openapi.go +++ b/api/v1beta1/zz_generated.openapi.go @@ -4463,7 +4463,7 @@ func schema_sigsk8sio_cluster_api_api_v1beta1_Topology(ref common.ReferenceCallb }, "classNamespace": { SchemaProps: spec.SchemaProps{ - Description: "The namespace of the ClusterClass object to create the topology. Empty namespace assumes the namespace of the cluster object. Class namespace changes are not supported by the rebase procedure.", + Description: "The namespace of the ClusterClass object to create the topology. Empty namespace assumes the namespace of the cluster object. Class namespace changes are not supported by the rebase procedure, as different CC namespace uses namespace-local templates. Cluster templates namespace modification is not allowed.", Type: []string{"string"}, Format: "", }, diff --git a/config/crd/bases/cluster.x-k8s.io_clusters.yaml b/config/crd/bases/cluster.x-k8s.io_clusters.yaml index 665796ce5d9c..63a036e7b2f5 100644 --- a/config/crd/bases/cluster.x-k8s.io_clusters.yaml +++ b/config/crd/bases/cluster.x-k8s.io_clusters.yaml @@ -930,9 +930,9 @@ spec: type: string classNamespace: description: |- - The namespace of the ClusterClass object to create the topology. - Empty namespace assumes the namespace of the cluster object. - Namespace changes are not supported by rebase procedure. + The namespace of the ClusterClass object to create the topology. Empty namespace assumes the namespace of the cluster object. + Class namespace changes are not supported by the rebase procedure, as different CC namespace uses namespace-local templates. + Cluster templates namespace modification is not allowed. maxLength: 253 minLength: 1 pattern: ^[a-z0-9](?:[-a-z0-9]*[a-z0-9])?(?:\.[a-z0-9](?:[-a-z0-9]*[a-z0-9])?)*$ From f8e49a0dc516ad1d33e7e9aaf27c7fee290c8eaf Mon Sep 17 00:00:00 2001 From: Danil-Grigorev Date: Fri, 6 Dec 2024 15:36:07 +0100 Subject: [PATCH 09/10] Use MatchingFields as And logic Signed-off-by: Danil-Grigorev --- .../controllers/topology/cluster/cluster_controller.go | 9 ++++----- internal/webhooks/clusterclass.go | 9 ++++----- 2 files changed, 8 insertions(+), 10 deletions(-) diff --git a/internal/controllers/topology/cluster/cluster_controller.go b/internal/controllers/topology/cluster/cluster_controller.go index 6afaf19bc763..4d3119d7758b 100644 --- a/internal/controllers/topology/cluster/cluster_controller.go +++ b/internal/controllers/topology/cluster/cluster_controller.go @@ -24,7 +24,6 @@ import ( "github.com/go-logr/logr" "github.com/pkg/errors" apierrors "k8s.io/apimachinery/pkg/api/errors" - "k8s.io/apimachinery/pkg/fields" "k8s.io/apimachinery/pkg/types" kerrors "k8s.io/apimachinery/pkg/util/errors" "k8s.io/client-go/tools/record" @@ -373,10 +372,10 @@ func (r *Reconciler) clusterClassToCluster(ctx context.Context, o client.Object) if err := r.Client.List( ctx, clusterList, - client.MatchingFieldsSelector{Selector: fields.AndSelectors( - fields.OneTermEqualSelector(index.ClusterClassNameField, clusterClass.Name), - fields.OneTermEqualSelector(index.ClusterClassNamespaceField, clusterClass.Namespace), - )}, + client.MatchingFields{ + index.ClusterClassNameField: clusterClass.Name, + index.ClusterClassNamespaceField: clusterClass.Namespace, + }, ); err != nil { return nil } diff --git a/internal/webhooks/clusterclass.go b/internal/webhooks/clusterclass.go index d89394051356..ab0e842e0400 100644 --- a/internal/webhooks/clusterclass.go +++ b/internal/webhooks/clusterclass.go @@ -25,7 +25,6 @@ import ( corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/fields" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/sets" "k8s.io/apimachinery/pkg/util/validation" @@ -380,10 +379,10 @@ func (webhook *ClusterClass) classNamesFromMPWorkerClass(w clusterv1.WorkersClas func (webhook *ClusterClass) getClustersUsingClusterClass(ctx context.Context, clusterClass *clusterv1.ClusterClass) ([]clusterv1.Cluster, error) { clusters := &clusterv1.ClusterList{} err := webhook.Client.List(ctx, clusters, - client.MatchingFieldsSelector{Selector: fields.AndSelectors( - fields.OneTermEqualSelector(index.ClusterClassNameField, clusterClass.Name), - fields.OneTermEqualSelector(index.ClusterClassNamespaceField, clusterClass.Namespace), - )}, + client.MatchingFields{ + index.ClusterClassNameField: clusterClass.Name, + index.ClusterClassNamespaceField: clusterClass.Namespace, + }, ) if err != nil { return nil, err From 902e58252226150973f774167bad454f13a827ee Mon Sep 17 00:00:00 2001 From: Danil Grigorev Date: Tue, 10 Dec 2024 11:40:15 +0100 Subject: [PATCH 10/10] Fix spelling Co-authored-by: Christian Schlotter --- .../experimental-features/cluster-class/write-clusterclass.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/book/src/tasks/experimental-features/cluster-class/write-clusterclass.md b/docs/book/src/tasks/experimental-features/cluster-class/write-clusterclass.md index f9384befed58..990d638888b6 100644 --- a/docs/book/src/tasks/experimental-features/cluster-class/write-clusterclass.md +++ b/docs/book/src/tasks/experimental-features/cluster-class/write-clusterclass.md @@ -440,7 +440,7 @@ spec: ### Defining a custom namespace for ClusterClass object -As a user, I may need to create a `Cluster` from a `ClusterClass` object that exists only in a different namespace. To uniquely identify the `ClusterClass`, a `NamespaceName` ref is constructed from combination of: +As a user, I may need to create a `Cluster` from a `ClusterClass` object that exists only in a different namespace. To uniquely identify the `ClusterClass`, a `NamespacedName` ref is constructed from combination of: * `cluster.spec.topology.classNamespace` - namespace of the `ClusterClass` object. * `cluster.spec.topology.class` - name of the `ClusterClass` object. @@ -471,7 +471,7 @@ spec:

Cluster rebase across namespaces

-Changing `classNamespace` is not supported in rebase procedure, while chanding `class` reference to a different `ClusterClass` from the same namespace performs regular `Cluster` rebase procedure. +Changing `classNamespace` is not supported in rebase procedure, while changing `class` reference to a different `ClusterClass` from the same namespace performs regular `Cluster` rebase procedure.