Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

✨ Add classNamespace to topology #11352

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

✨ Add classNamespace to topology #11352

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
1dea02a
Add classNamespace to topology
Danil-Grigorev Oct 30, 2024
ccc32d1
Add documentation on securing cross-namespace access for CC
Danil-Grigorev Oct 30, 2024
65db336
Lookup Clusters across namespaces
Danil-Grigorev Nov 7, 2024
5d9542b
Add a note on cross-ns rebase
Danil-Grigorev Nov 7, 2024
b4b6131
Add validation on classNamespace, API review addressed
Danil-Grigorev Nov 27, 2024
e16aa76
Add tests for clusterclassToCluster, godoc changes
Danil-Grigorev Dec 3, 2024
3639364
Add ClassNamespace index
Danil-Grigorev Dec 4, 2024
1a88697
Clarify cross-namespace rebase support godoc
Danil-Grigorev Dec 5, 2024
f8e49a0
Use MatchingFields as And logic
Danil-Grigorev Dec 6, 2024
902e582
Fix spelling
Danil-Grigorev Dec 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 15 additions & 1 deletion api/v1beta1/cluster_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Danil-Grigorev marked this conversation as resolved.
Show resolved Hide resolved
// Empty namespace assumes the namespace of the cluster object.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// +optional
fabriziopandini marked this conversation as resolved.
Show resolved Hide resolved
// +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"`
fabriziopandini marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


// The Kubernetes version of the cluster.
Version string `json:"version"`

Expand Down Expand Up @@ -1037,7 +1045,13 @@ 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}

namespace := c.Spec.Topology.ClassNamespace
if c.Spec.Topology == nil || c.Spec.Topology.ClassNamespace == "" {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if c.Spec.Topology == nil || c.Spec.Topology.ClassNamespace == "" {
if namespace == "" {

Above we are returning when c.Spec.Topology == nil

namespace = c.Namespace
}

return types.NamespacedName{Namespace: namespace, Name: c.Spec.Topology.Class}
}

// GetConditions returns the set of conditions for this object.
Expand Down
7 changes: 7 additions & 0 deletions api/v1beta1/zz_generated.openapi.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

7 changes: 7 additions & 0 deletions config/crd/bases/cluster.x-k8s.io_clusters.yaml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

83 changes: 83 additions & 0 deletions docs/book/src/tasks/experimental-features/cluster-class/write-clusterclass.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -438,6 +438,89 @@ 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:
Danil-Grigorev marked this conversation as resolved.
Show resolved Hide resolved
* `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
```

<aside class="note warning">

<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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Danil-Grigorev marked this conversation as resolved.
Show resolved Hide resolved

</aside>

#### 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.
Copy link
Member

@enxebre enxebre Nov 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


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: "allowed-namespaces.cluster-class-ref.cluster.x-k8s.io"
namespace: "default"
parameterNotFoundAction: Deny
---
apiVersion: v1
kind: Secret
metadata:
name: "allowed-namespaces.cluster-class-ref.cluster.x-k8s.io"
namespace: "default"
data:
default: ""
```

## Advanced features of ClusterClass with patches

This section will explain more advanced features of ClusterClass patches.
Expand Down
1 change: 1 addition & 0 deletions internal/apis/core/v1alpha4/conversion.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
1 change: 1 addition & 0 deletions internal/apis/core/v1alpha4/zz_generated.conversion.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 3 additions & 2 deletions internal/controllers/topology/cluster/cluster_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand All @@ -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].GetClassKey().Namespace == clusterClass.Namespace {
requests = append(requests, ctrl.Request{NamespacedName: util.ObjectKey(&clusterList.Items[i])})
}
}
return requests
}
Expand Down
11 changes: 9 additions & 2 deletions internal/webhooks/clusterclass.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems to me that in this method there is not enough information to know cluster namespace due to duality of the namespace reference.

If an index is added, there will be another Client.List call for client.MatchingFields{index.ClusterClassNamespaceField: clusterClass.Namespace}, and here I’m not sure how would an empty field be treated - matching an empty string or matching no selector at all?

If the first one the case, then the method will combine two list responses, and it will be more readable. If not, it will only increase the number of calls.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, seems it is not an issue, as the index can use GetClass() method which resolves ambiguity. Should be added now.

)
if err != nil {
return nil, err
}
return clusters.Items, nil

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

return referencedClusters, nil
}

func getClusterClassVariablesMapWithReverseIndex(clusterClassVariables []clusterv1.ClusterClassVariable) (map[string]*clusterv1.ClusterClassVariable, map[string]int) {
Expand Down