✨ Add classNamespace to topology #11352

Danil-Grigorev · 2024-10-30T09:46:21Z

What this PR does / why we need it:

Adding classNamespace variable to the cluster topology, which allows to point to a ClusterClass in a different namespace. This field is dormant, and is used for differentiation only.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Related to #5673

/area clusterclass

salasberryfin

Thanks @Danil-Grigorev! I added a small suggestion but I think this looks good.

docs/book/src/tasks/experimental-features/cluster-class/write-clusterclass.md

salasberryfin · 2024-10-31T10:04:51Z

Thanks @Danil-Grigorev

/lgtm

k8s-ci-robot · 2024-10-31T10:04:57Z

LGTM label has been added.

Git tree hash: c4b418743a4683ae623b3264aae7636c41ad5675

salasberryfin · 2024-10-31T10:05:46Z

/assign @chrischdi

k8s-ci-robot · 2024-11-27T21:03:11Z

New changes are detected. LGTM label has been removed.

fabriziopandini · 2024-11-28T11:09:34Z

api/v1beta1/cluster_types.go

@@ -509,6 +509,14 @@ 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.


Wondering if we should default to current namespace ...

Pros: it is explicit and consistent to what we do for other references
Cons: it add "noises" to the containing struct while we don't have a clean nesting yet

A compromise might be to keep it empty for now and start defaulting when we will introduce a nested struct
@JoelSpeed opinions?

The namespace of the object can never change, so, defaulting this value to the current namespace would make sense, and make the API explicit. I can't foresee us ever, in the future, wanting to default this to a specific namespace, as we have no concept of a core namespace.

That said, I could see a use case where a cluster admin wants to default this field themselves. In the future they could leverage MutatingAdmissionPolicy to set the namespace always to a certain namespace where they keep their classes. Could defaulting this (since it would have to be a webhook, or MAP) then cause interference with any logic the cluster admin has there? They'd have to make sure that their defaulting went before ours 🤔

I think I would err on the side here, or not defaulting it and just explaining that the empty namespace means that the namespace of the cluster is leveraged, as it is now.

Makes sense to me, thx for the explanation Joel!

api/v1beta1/cluster_types.go

fabriziopandini · 2024-11-28T11:19:04Z

docs/book/src/tasks/experimental-features/cluster-class/write-clusterclass.md

+
+<h1>Cluster rebase across namespaces</h1>
+
+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.


Might be I'm wrong, but I did not found a place where we are enforcing "Changing classNamespace is not supported".
Also, out of curiosity, why are we introducing this limitation?

That’s mainly due to tests performed on that operation locally. It seems when changing classNamespace, many referenced template components change namespace too, as they have to preserve ownership references, and instead of rebase performing minimal modifications of the resource, it has to switch resources entirely. This seemed as a larger scope change.

I’ll add a validation on the field modifications.

Thanks clarification
It would be great if we can add the same note as a godoc explanation for check above

I composed a clarification message on the field. It seems there is no need for additional tests as TestLocalObjectTemplatesAreCompatible is already covering template namespace changes.

I added another comment already but I didn't see validation for this. To be honest I'm really wondering what's going on here. Rebase across namespaces shouldn't lead to problems

What do you mean with: "it has to switch resources entirely"?

fabriziopandini · 2024-11-28T11:31:07Z

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),


Thinking a little bit more about this, I think we change this index so performance in the webhook are optimized and also the resulting code will be more readable (both here and in clusterClassToCluster)

fabriziopandini · 2024-11-28T11:32:02Z

internal/controllers/topology/cluster/cluster_controller.go

@@ -371,7 +370,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 {


Note for self: this seems still open no matter if it reports outdated

docs/book/src/tasks/experimental-features/cluster-class/write-clusterclass.md

Danil-Grigorev · 2024-12-18T17:11:46Z

@chrischdi @fabriziopandini Can I get another review? All comments are addressed here.

k8s-ci-robot · 2025-01-07T19:49:50Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from chrischdi. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Signed-off-by: Danil-Grigorev <[email protected]>

Co-authored-by: Christian Schlotter <[email protected]>

Signed-off-by: Danil-Grigorev <[email protected]>

sbueringer

Thank you for working on this and thx for the patience. Sorry for the delay

sbueringer · 2025-01-10T08:50:18Z

api/v1beta1/cluster_types.go

@@ -517,6 +518,15 @@ 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, as different CC namespace uses namespace-local templates.


I don't understand this limitation.

Example:

Before rebase: CC1 in test1 namespace referencing templates in test1 namespace
After rebase: CC2 in test2 namespace referencing templates in test2 namespace

Why doesn't this work?

EDIT: I also didn't find anything in our webhook that blocks changing the namespace, maybe this godoc comment is just outdated

Related: #11352 (comment)

sbueringer · 2025-01-10T08:50:59Z

api/v1beta1/cluster_types.go

@@ -517,6 +518,15 @@ 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, as different CC namespace uses namespace-local templates.
+	// Cluster templates namespace modification is not allowed.


What is Cluster templates referring to? To me this comment is mostly confusing to be honest

sbueringer · 2025-01-10T08:54:22Z

api/v1beta1/cluster_types.go

+	// 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


Doesn't MinLength=1 conflict with "Empty namespace assumes the namespace of the cluster object"?

I assume valid values are / should be:

not set => which is what "empty namespace" is referring to

set to a string with length between 1 and 253

Not valid:

setting it to an empty string ("")

If I'm correct, maybe we should simply update the comment above to: "If namespace is not set we assume the namespace of the Cluster object"

P.S. I'm aware that when an end user sets the classNamespace field to "", the field will be removed by our mutating webhook on the Cluster object (because of the omitempty) and then passes the OpenAPI schema validation. Nonetheless it's probably cleaner to use "not set" vs. "empty string" (also because empty string is forbidden by the OpenAPI schema)

I think this was originally a suggestion from Joel. Cross-referencing it here for the context: #11352 (comment) I validated it, and it works great. Typically an ”” value will be stored on the resource if it is once set (even via a controller), but here it blocks the round trip on API server via MinLength and falls back to +optional, removing the field, thus making it use class metadata.namespace instead.

This is what allowed to add defaulting of the value in the E2E tests followup, suggested in #11395 (comment), allowing to greatly reduce templates duplication.

Okay. Can we modify the comment to say "If the namespace is empty or not set ..." ?

sbueringer · 2025-01-10T08:59:12Z

api/v1beta1/cluster_types.go

+	// +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])?)*$"


If there's a specific place where we got this regex from, let's please mention it (I guess it's sort of the "standard" regexp to validate Kubernetes object names?)

sbueringer · 2025-01-10T09:10:17Z

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),


I'm not sure I'm following the discussion (but I also don't know the previous version of the code).

But I was also wondering why we don't simply use one index (e.g. ClusterClassRefField) that has the format namespace/name?

As ClusterClassNameField index is always used together with the ClusterClassNamespaceField index I don't see the need to maintain two indices.

k8s-ci-robot added area/clusterclass Issues or PRs related to clusterclass cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Oct 30, 2024

Danil-Grigorev changed the title ~~Add classNamespace to topology~~ ✨ Add classNamespace to topology Oct 30, 2024

k8s-ci-robot requested review from jackfrancis and sbueringer October 30, 2024 09:46

Danil-Grigorev force-pushed the class-namespace-addition branch 2 times, most recently from ae1cb19 to 43cb995 Compare October 30, 2024 11:59

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Oct 30, 2024

Danil-Grigorev force-pushed the class-namespace-addition branch 2 times, most recently from 2bddeeb to 352fd66 Compare October 30, 2024 15:20

salasberryfin reviewed Oct 31, 2024

View reviewed changes

docs/book/src/tasks/experimental-features/cluster-class/write-clusterclass.md Outdated Show resolved Hide resolved

Danil-Grigorev force-pushed the class-namespace-addition branch from 352fd66 to bd8a56f Compare October 31, 2024 09:51

k8s-ci-robot assigned salasberryfin Oct 31, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 31, 2024

k8s-ci-robot assigned chrischdi Oct 31, 2024

Danil-Grigorev changed the title ~~✨ Add classNamespace to topology~~ [WIP] ✨ Add classNamespace to topology Oct 31, 2024

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 31, 2024

Danil-Grigorev force-pushed the class-namespace-addition branch from bd8a56f to 4bbb218 Compare October 31, 2024 18:34

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 31, 2024

k8s-ci-robot requested review from chrischdi and salasberryfin October 31, 2024 18:34

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 31, 2024

Danil-Grigorev force-pushed the class-namespace-addition branch from 4bbb218 to d450768 Compare October 31, 2024 18:44

Danil-Grigorev changed the title ~~[WIP] ✨ Add classNamespace to topology~~ ✨ Add classNamespace to topology Oct 31, 2024

k8s-ci-robot requested a review from furkatgofurov7 November 27, 2024 21:03

Danil-Grigorev force-pushed the class-namespace-addition branch from bbbddcf to b4b6131 Compare November 27, 2024 21:25

fabriziopandini reviewed Nov 28, 2024

View reviewed changes

Danil-Grigorev mentioned this pull request Nov 28, 2024

v1beta2: Class Namespace API change #11500

Open

Danil-Grigorev force-pushed the class-namespace-addition branch 4 times, most recently from c581444 to 3639364 Compare December 4, 2024 13:28

chrischdi reviewed Dec 10, 2024

View reviewed changes

docs/book/src/tasks/experimental-features/cluster-class/write-clusterclass.md Outdated Show resolved Hide resolved

docs/book/src/tasks/experimental-features/cluster-class/write-clusterclass.md Outdated Show resolved Hide resolved

Danil-Grigorev force-pushed the class-namespace-addition branch from 902e582 to 2730042 Compare January 7, 2025 19:49

Danil-Grigorev mentioned this pull request Jan 7, 2025

✨ clusterctl move support for a cross namespace ClusterClass reference #11649

Open

Danil-Grigorev and others added 11 commits January 8, 2025 22:00

Add classNamespace to topology

1fb1287

Signed-off-by: Danil-Grigorev <[email protected]>

Add documentation on securing cross-namespace access for CC

d8d4132

Signed-off-by: Danil-Grigorev <[email protected]>

Lookup Clusters across namespaces

80f2faf

Signed-off-by: Danil-Grigorev <[email protected]>

Add a note on cross-ns rebase

0c17213

Signed-off-by: Danil-Grigorev <[email protected]>

Add validation on classNamespace, API review addressed

01bebff

Signed-off-by: Danil-Grigorev <[email protected]>

Add tests for clusterclassToCluster, godoc changes

24bcb75

Signed-off-by: Danil-Grigorev <[email protected]>

Add ClassNamespace index

1b6e9ec

Signed-off-by: Danil-Grigorev <[email protected]>

Clarify cross-namespace rebase support godoc

18b4348

Signed-off-by: Danil-Grigorev <[email protected]>

Use MatchingFields as And logic

81e6316

Signed-off-by: Danil-Grigorev <[email protected]>

Fix spelling

8969890

Co-authored-by: Christian Schlotter <[email protected]>

Make TestClusterClassToCluster independent on request order

f1456bd

Signed-off-by: Danil-Grigorev <[email protected]>

Danil-Grigorev force-pushed the class-namespace-addition branch 2 times, most recently from eb540d6 to 03780fe Compare January 9, 2025 21:05

Link and naming fixes

1ae95a5

Signed-off-by: Danil-Grigorev <[email protected]>

Danil-Grigorev force-pushed the class-namespace-addition branch from 03780fe to 1ae95a5 Compare January 9, 2025 21:07

sbueringer reviewed Jan 10, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Add classNamespace to topology #11352

✨ Add classNamespace to topology #11352

Danil-Grigorev commented Oct 30, 2024 •

edited

Loading

salasberryfin left a comment

salasberryfin commented Oct 31, 2024

k8s-ci-robot commented Oct 31, 2024

salasberryfin commented Oct 31, 2024

k8s-ci-robot commented Nov 27, 2024

fabriziopandini Nov 28, 2024

JoelSpeed Nov 28, 2024

sbueringer Jan 10, 2025

fabriziopandini Nov 28, 2024

Danil-Grigorev Nov 28, 2024

fabriziopandini Dec 2, 2024

Danil-Grigorev Dec 5, 2024

sbueringer Jan 10, 2025 •

edited

Loading

fabriziopandini Nov 28, 2024

fabriziopandini Nov 28, 2024

Danil-Grigorev commented Dec 18, 2024

k8s-ci-robot commented Jan 7, 2025

sbueringer left a comment

sbueringer Jan 10, 2025 •

edited

Loading

sbueringer Jan 10, 2025

sbueringer Jan 10, 2025

Danil-Grigorev Jan 10, 2025

sbueringer Jan 10, 2025

sbueringer Jan 10, 2025

sbueringer Jan 10, 2025 •

edited

Loading


		<h1>Cluster rebase across namespaces</h1>

		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.

✨ Add classNamespace to topology #11352

Are you sure you want to change the base?

✨ Add classNamespace to topology #11352

Conversation

Danil-Grigorev commented Oct 30, 2024 • edited Loading

salasberryfin left a comment

Choose a reason for hiding this comment

salasberryfin commented Oct 31, 2024

k8s-ci-robot commented Oct 31, 2024

salasberryfin commented Oct 31, 2024

k8s-ci-robot commented Nov 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbueringer Jan 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Danil-Grigorev commented Dec 18, 2024

k8s-ci-robot commented Jan 7, 2025

sbueringer left a comment

Choose a reason for hiding this comment

sbueringer Jan 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbueringer Jan 10, 2025 • edited Loading

Choose a reason for hiding this comment

Danil-Grigorev commented Oct 30, 2024 •

edited

Loading

sbueringer Jan 10, 2025 •

edited

Loading

sbueringer Jan 10, 2025 •

edited

Loading

sbueringer Jan 10, 2025 •

edited

Loading