✨ 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-10-31T18:34:35Z

[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 Kubernetes 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

enxebre · 2024-11-05T09:49:01Z

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

+
+#### 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.


besides the on admission check which is nice, do we have any rbac recommendation for this?

VAP supports secondary authz for added RBAC control, and VAP binding can be used with label selectors, which addresses #5673 (comment). From the proposal itself, it seems using the policy for added restriction on top of RBAC is within the scope.

Webhook allows to use paramRef of any kind, which can be potentially explored with specific CRD to further restrict access beyond admission, with a controller acting on that resource.

Currently, this is just an example of how an on top policy can be defined (if needed) in k8s 1.30+, where a user may decide to use different policy mechanisms to further restrict access, including a more granular RBAC. I’m thinking to showcase it as an option, but to not enforce any specific solution within this PR.

There is also an RBAC recommendation in https://kccncna2024.sched.com/event/1hoyX, if a talk considered to be one.

neolit123

@Danil-Grigorev did you attempt rebasing a non-namespaced CC to a namespaced CC?
does that work fine?

Danil-Grigorev · 2024-11-07T10:50:48Z

@neolit123 I tried rebase on the clusterClass in the same namespace, and it works fine. Cross-namespace rebases are not possible, as there is validation to prevent namespace change for templates. This seems like a larger change.

neolit123 · 2024-11-07T11:01:33Z

@neolit123 I tried rebase on the clusterClass in the same namespace, and it works fine. Cross-namespace rebases are not possible, as there is validation to prevent namespace change for templates. This seems like a larger change.

if this larger change is not added then it's worth mentioning the limitation in the docs.

Danil-Grigorev · 2024-11-07T16:51:56Z

Maybe to address some comments from the last meeting - extension configs on the initial check seem to work as well, as long as the namespace selector matches.

furkatgofurov7

/lgtm

k8s-ci-robot · 2024-11-11T09:43:48Z

LGTM label has been added.

Git tree hash: f7b15d982fdfb8730cff97c2900ccbd9c661e3d9

fabriziopandini · 2024-11-25T13:23:24Z

api/v1beta1/cluster_types.go

@@ -332,6 +332,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.
+	//


nit

Suggested change

//

fabriziopandini · 2024-11-25T13:28:48Z

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

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


nit

Suggested change

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.

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 `ClusterClass` from the same namespace is permitted in the Cluster rebase procedure.

Also, thinking about:

Class namespace referenced in the Cluster object is equivalent to a cluster being located in the referenced namespace from the validation perspective

I'm not sure if we are surfacing an implementation detail or if we are trying to surface something the users should care about. Could you kindly provide some more context?

fabriziopandini · 2024-11-25T13:33:32Z

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

+  policyName: "cluster-class-ref.cluster.x-k8s.io"
+  validationActions: [Deny]
+  paramRef:
+    name: "ref-list"


might be

Suggested change

name: "ref-list"

name: "allowed-namespaces.cluster-class-ref.cluster.x-k8s.io"

(if we want to keep the "domain like" name, otherwise something with "allowed namespaces")

fabriziopandini · 2024-11-25T13:37:33Z

api/v1beta1/cluster_types.go

+	// The namespace of the ClusterClass object to create the topology.
+	//
+	// +optional
+	ClassNamespace string `json:"classNamespace,omitempty"`


Out of curiosity.
Are we planning to refactor Class/ClassName down the line or keep as it is (top of mind, having name/namespace for an referenced object inlined in the parent struct/not using a nested ref struct is sort of unique in Cluster API).

cc @vincepri @enxebre @JoelSpeed who might have opinions on API modeling as well

In a future version of this API, I'd expect us to move to

class: name: ... namespace: ...

As part of a clean up, this would then be in-keeping, as you say, with other APIs we have

fabriziopandini · 2024-11-25T13:44:45Z

api/v1beta1/cluster_types.go

+}
+
+// GetInfrastructureNamespace returns common namespace for the cluster infrastructure.
+func (c *Cluster) GetInfrastructureNamespace() string {


Considering that this func looks at c.Spec.Topology and not at c.Spec.InfrastructureRef, I'm a little bit confused by "Infrastructure" in the name.

Also, if possible, I would prefer to try to keep API package as clean as possible and not use it as a way to share util between packages (I know we did this in the past in a few cases, let's try to not add more).
E.g. we can think about something under internal/topology, or also duplicate this if could work for me

fabriziopandini · 2024-11-25T13:51:42Z

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 {


Can we add a unit test ensuring that a change in a CC with a given name triggers reconcile only for clusters using it (and not for clusters using CC with the same name, but in another ns)
Possibly, this should cover both cluster with ClassNamespace set and not.

fabriziopandini · 2024-11-25T13:55:42Z

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


Wondering if we should update the index, so it contains both namespace and name of the cluster class

fabriziopandini · 2024-11-25T13:56:28Z

internal/webhooks/clusterclass.go

+
+	referencedClusters := []clusterv1.Cluster{}
+	for _, cluster := range clusters.Items {
+		if cluster.GetInfrastructureNamespace() == clusterClass.Namespace {


Also, in this case, possibly add unit test coverage (similar to clusterClassToCluster above)

fabriziopandini · 2024-11-25T14:12:43Z

api/v1beta1/cluster_types.go

@@ -332,6 +332,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


Q: do we care about differentiating between not set and set to empty string?
(I think not, just double checking)

JoelSpeed · 2024-11-27T10:41:23Z

api/v1beta1/cluster_types.go

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


What is the behaviour when this is not specified? Can you add that to the description here?

JoelSpeed · 2024-11-27T10:43:24Z

api/v1beta1/cluster_types.go

+	// The namespace of the ClusterClass object to create the topology.
+	//
+	// +optional
+	ClassNamespace string `json:"classNamespace,omitempty"`


Please add a minimum and maximum length to all raw string types, since this is a namespace, it follows DNS1123Subdomain and should also be validated as such (there's a regex for this). The maximum length is 253 and the minimum is 1.

Since you have omitempty, you can safely add the MinLength, it will prevent anyone using an unstructured client from persisting classNamespace: "" which would then otherwise not round trip through a structured request

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

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

New changes are detected. LGTM label has been removed.

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

k8s-ci-robot · 2024-11-27T21:30:56Z

@Danil-Grigorev: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-cluster-api-verify-main	`b4b6131`	link	true	`/test pull-cluster-api-verify-main`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

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 removed 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 mentioned this pull request Nov 4, 2024

✨ ClusterClass: ensure templates are created in the Cluster namespace #11366

Merged

enxebre reviewed Nov 5, 2024

View reviewed changes

alexander-demicev mentioned this pull request Nov 5, 2024

Add support for a cross namespace ClusterClass access rancher/turtles#832

Open

neolit123 reviewed Nov 6, 2024

View reviewed changes

Danil-Grigorev force-pushed the class-namespace-addition branch from d450768 to d6d8e33 Compare November 7, 2024 11:44

furkatgofurov7 reviewed Nov 11, 2024

View reviewed changes

k8s-ci-robot assigned furkatgofurov7 Nov 11, 2024

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

Danil-Grigorev mentioned this pull request Nov 11, 2024

[WIP] ✨ Add clusterctl changes for cross-ns CC ref #11395

Open

fabriziopandini reviewed Nov 25, 2024

View reviewed changes

JoelSpeed reviewed Nov 27, 2024

View reviewed changes

Danil-Grigorev added 4 commits November 27, 2024 21:38

Add classNamespace to topology

1dea02a

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

Add documentation on securing cross-namespace access for CC

ccc32d1

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

Lookup Clusters across namespaces

65db336

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

Add a note on cross-ns rebase

5d9542b

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

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

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

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

Add validation on classNamespace, API review addressed

b4b6131

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

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

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 Oct 31, 2024

enxebre Nov 5, 2024 •

edited

Loading

Danil-Grigorev Nov 5, 2024

Danil-Grigorev Nov 14, 2024

neolit123 left a comment

Danil-Grigorev commented Nov 7, 2024

neolit123 commented Nov 7, 2024

Danil-Grigorev commented Nov 7, 2024

furkatgofurov7 left a comment

k8s-ci-robot commented Nov 11, 2024

fabriziopandini Nov 25, 2024

fabriziopandini Nov 25, 2024 •

edited

Loading

fabriziopandini Nov 25, 2024 •

edited

Loading

fabriziopandini Nov 25, 2024 •

edited

Loading

JoelSpeed Nov 27, 2024

fabriziopandini Nov 25, 2024 •

edited

Loading

fabriziopandini Nov 25, 2024 •

edited

Loading

fabriziopandini Nov 25, 2024

fabriziopandini Nov 25, 2024

fabriziopandini Nov 25, 2024

JoelSpeed Nov 27, 2024

JoelSpeed Nov 27, 2024

k8s-ci-robot commented Nov 27, 2024

k8s-ci-robot commented Nov 27, 2024


		#### 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.


		<h1>Cluster rebase across namespaces</h1>

		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.

	name: "ref-list"
	name: "allowed-namespaces.cluster-class-ref.cluster.x-k8s.io"

✨ 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 Oct 31, 2024

enxebre Nov 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neolit123 left a comment

Choose a reason for hiding this comment

Danil-Grigorev commented Nov 7, 2024

neolit123 commented Nov 7, 2024

Danil-Grigorev commented Nov 7, 2024

furkatgofurov7 left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Nov 11, 2024

Choose a reason for hiding this comment

fabriziopandini Nov 25, 2024 • edited Loading

Choose a reason for hiding this comment

fabriziopandini Nov 25, 2024 • edited Loading

Choose a reason for hiding this comment

fabriziopandini Nov 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabriziopandini Nov 25, 2024 • edited Loading

Choose a reason for hiding this comment

fabriziopandini Nov 25, 2024 • 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

k8s-ci-robot commented Nov 27, 2024

k8s-ci-robot commented Nov 27, 2024

Danil-Grigorev commented Oct 30, 2024 •

edited

Loading

enxebre Nov 5, 2024 •

edited

Loading

fabriziopandini Nov 25, 2024 •

edited

Loading

fabriziopandini Nov 25, 2024 •

edited

Loading

fabriziopandini Nov 25, 2024 •

edited

Loading

fabriziopandini Nov 25, 2024 •

edited

Loading

fabriziopandini Nov 25, 2024 •

edited

Loading