search

kubernetes-sigs / cluster-api

Home for Cluster API, a subproject of sig-cluster-lifecycle
https://cluster-api.sigs.k8s.io
Apache License 2.0
3.58k stars 1.32k forks source link

Allow using a ClusterClass across namespaces #5673

Open fabriziopandini opened 2 years ago

fabriziopandini commented 2 years ago

User Story

As a user/operator I would like to define ClusterClass once and reuse it across many namespaces As a user/operator I would like to define the namespaces my ClusterClass can be used from

Detailed Description

As of today ClusterClass is a namespaced object, and the Cluster.spec.topology.class is a simple name

This works well under some many circumstances, but this approach does not works well in use cases when clusters are deployed in several namespaces/managed by different tenants, but the organisation providing ClusterCluster class is only one.

This issue is about discussing how to allow users a simple an straightforward experience in the use case above.

A possible approach requires two changes:

Whenever a Cluster is created, we are going to check if allowedNamespaces are defined; if yes, the ClusterNamespace should be in that list, otherwise the operation is denied; most probably we should make a similar check at reconcile time.

Other things that should be considered, are e.g validation check when modifying a ClusterClass; in this case, if allowedNamespaces are defined, we should check for corresponding Cluster in a list of namespaces; similarly, the change of allowedNamespaces field itself should account for those Clusters to exist before allowing the operation.

Anything else you would like to add:

Prior art for this approach:

/kind feature

fabriziopandini commented 2 years ago

/area topology /milestone v1.1

fabriziopandini commented 2 years ago

NOTE: I'm assuming a ClusterClass and its referenced templates should be in a single namespace as of today; the Cluster only can be in a different namespace

sbueringer commented 2 years ago

clusterctl move currently moves an entire namespace. Because up until now Clusters and their ClusterClasses had to be in the same namespace we could just move all Clusters and ClusterClasses of the respective namespaces (but of course in the right order).

The feature that ClusterClasses can be in another namespace changes our assumptions, so we have to think about how clusterctl move should behave in those cases.

killianmuldoon commented 2 years ago

Would it be more user friendly to change class to a struct with name and namespace(defaulting to cluster namespace if unset? This would mirror what we're doing elsewhere at least and avoids adding new patterns which might be confusing - I think making it a namespaced name could be tough for new users to understand .

sbueringer commented 2 years ago

I think you both mean the same thing: types.NamespacedName:

type NamespacedName struct {
    Namespace string
    Name      string
}

https://github.com/kubernetes/apimachinery/blob/master/pkg/types/namespacedname.go#L27-L30

killianmuldoon commented 2 years ago

I think you both mean the same thing: types.NamespacedName:

That looks right - I was thinking class: "namespace/classname" was on the table as an option.

ykakarap commented 2 years ago

/assign

vincepri commented 2 years ago

class: "namespace/classname" this could make sense as it's not a breaking change

killianmuldoon commented 2 years ago

note: #5717 adds some namespacing to webhook validation for clusterClass that will have to be addressed as part of this work.

ykakarap commented 2 years ago

note: #5717 adds some namespacing to webhook validation for clusterClass that will have to be addressed as part of this work.

Thanks for the heads up @killianmuldoon! :)

k8s-triage-robot commented 2 years ago

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

You can:

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

fabriziopandini commented 2 years ago

/remove-lifecycle stale /priority-important-soon I think we should reconsider this given that ClusterClass adoption is growing...

fabriziopandini commented 2 years ago

I'm considering moving on with the suggested approach; adding some more considerations

Note, in the first iteration, I think we should not consider changing clusterctl move to move CC outsides the namespace being moved, this requires a dedicated deep dive

sbueringer commented 2 years ago

Sounds good, one comment:

CecileRobertMichon commented 2 years ago

+1

Another use case for this: in provider e2e tests, we provide one namespace per test cluster but want to reuse the same clusterclass and install it in BeforeSuite https://github.com/kubernetes-sigs/cluster-api-provider-azure/pull/2235/files#r865122163

CecileRobertMichon commented 2 years ago

I would prefer to have consistency across CAP*, so I suggest adding allowed namespace to a ClusterClass, similar to what we have in CAPA If allowedNamespace is empty, the ClusterClass can be used only within the same namespace, otherwise only in the namespaces specified (TBD if the same namespace is always included)

Not sure what CAPA does but in CAPZ if allowedNamespaces is empty it allows all namespaces (as opposed to only the current one) https://capz.sigs.k8s.io/topics/multitenancy.html#allowednamespaces

Would be beneficial for testing to be able to specify all namespaces since cluster name/namespace is randomly generated for each spec so it won't be known ahead of time in BeforeSuite

sbueringer commented 2 years ago

Another use case for this: in provider e2e tests, we provide one namespace per test cluster but want to reuse the same clusterclass and install it in BeforeSuite https://github.com/kubernetes-sigs/cluster-api-provider-azure/pull/2235/files#r865122163

I'm not sure if that is an issue. clusterctl generate automatically includes the ClusterClass when generating the Cluster (here).

In core CAPI today we use the same ClusterClass in multiple tests and it gets automatically applied in different namespaces because clusterctl generate includes it automatically. So if the requirement is only to reuse the same ClusterClass across multiple tests you don't have to explicitly install it via BeforeSuite. (we didn't implement any explicit ClusterClass deployment at all in core CAPI e2e tests)

EDIT: While it's possible to reuse the same ClusterClass in e2e tests by deploying it multiple times (and it's automatically included in the output of clusterctl generate cluster) it of course might be desirable to intentionally re-use one single deployed ClusterClass in multiple tests in different namespaces.

fabriziopandini commented 2 years ago

Not sure what CAPA does but in CAPZ if allowedNamespaces is empty it allows all namespaces (as opposed to only the current one) https://capz.sigs.k8s.io/topics/multitenancy.html#allowednamespaces

I'm not opposed at this behavior as well, however, I'm worried if this is "unsecure", given that if the user forgets to specify allowedNamespaces in a ClasterClass it is accessible from everywhere (instead of defaulting to the more restrictive behavior).

pydctw commented 2 years ago

Not sure what CAPA does but in CAPZ if allowedNamespaces is empty it allows all namespaces (as opposed to only the current one) https://capz.sigs.k8s.io/topics/multitenancy.html#allowednamespaces

@CecileRobertMichon, CAPA's behavior is same as CAPZ. If allowedNamespaces is empty, it allows all namespaces https://cluster-api-aws.sigs.k8s.io/topics/multitenancy.html#awsclustercontrolleridentity.

Not saying we should follow this behavior as I think @fabriziopandini has a valid concern. Just adding a data point as we always strive for consistency.

timothysc commented 2 years ago

I'm not a fan of allowedNamespaces, because we are weirdly injecting some form of policy atop. For example, if a user wanted to percolate ClusterClasses across namespaces, they could create some default-foo namespace with a rudimentary policy or controller that simply does a raw copy to the new namespace. I'm not terribly concerned about resource usage.

This weirdly gets into policy land.

vuil commented 2 years ago

@fabriziopandini trying to understand this part of the comment in https://github.com/kubernetes-sigs/cluster-api/issues/5673#issuecomment-1124155114

  • If allowedNamespace is empty, the ClusterClass can be used only within the same namespace, otherwise only in the namespaces specified (TBD if the same namespace is always included)

Does this mean that every new namespace created that needs to consume a ClusterClass will require an update to the CC's allowedNamespaces?

I understand for the test scenarios there is some automation to include the CC in new namespaces being used, but in actual usage this seems rather inconvenient.

fabriziopandini commented 2 years ago

Does this mean that every new namespace created that needs to consume a ClusterClass will require an update to the CC's allowedNamespaces?

if you use label selectors no, if instead you specify a list of namespaces yes

fabriziopandini commented 2 years ago

/triage accepted

@sbueringer let's work together to summarize all the comments and define an action plan

k8s-triage-robot commented 2 years ago

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

You can:

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

fabriziopandini commented 2 years ago

/lifecycle frozen

fabriziopandini commented 2 years ago

dropping from the milestone because not yet staffed @vincepri let's discuss if we want to keep pushing on this feature and how /help

k8s-ci-robot commented 2 years ago

@fabriziopandini: This request has been marked as needing help from a contributor.

Guidelines

Please ensure that the issue body includes answers to the following questions:

For more details on the requirements of such an issue, please see here and ensure that they are met.

If this request no longer meets these requirements, the label can be removed by commenting with the /remove-help command.

In response to [this](https://github.com/kubernetes-sigs/cluster-api/issues/5673): >dropping from the milestone because not yet staffed >@vincepri let's discuss if we want to keep pushing on this feature and how >/help Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
k8s-triage-robot commented 1 year ago

This issue is labeled with priority/important-soon but has not been updated in over 90 days, and should be re-triaged. Important-soon issues must be staffed and worked on either currently, or very soon, ideally in time for the next release.

You can:

For more details on the triage process, see https://www.kubernetes.dev/docs/guide/issue-triage/

/remove-triage accepted

fabriziopandini commented 1 year ago

/triage accepted

killianmuldoon commented 1 year ago

/assign

Just to keep track of this one - if anyone else has time to work on it please feel free to assign yourself.

killianmuldoon commented 1 year ago

/unassign

Not work on this ATM

salasberryfin commented 10 months ago

/assign

Would like to take a look at this. Perhaps we can add a topic to this week's meeting agenda to define an action plan on how this feature could be implemented.

Danil-Grigorev commented 10 months ago

Should this issue be separated into two? It seems to me that for a general scenario, just adding a namespace/name will be enough to reference a cluster class (although a classNamespace field is preferable IMO). And the policy access considerations in the multi-tenant scenario can have a separate issue. WDYT?

simonostendorf commented 10 months ago

It seems to me that for a general scenario, just adding a https://github.com/kubernetes-sigs/cluster-api/issues/5673#issuecomment-973144645 will be enough to reference a cluster class (although a classNamespace field is preferable IMO).

I also think that the first solution could be the namespace/name reference for a ClusterClass. This would be enough for me.

Danil-Grigorev commented 7 months ago

Service Gateway API moved their referential approach into KEP, and it was merged - https://github.com/kubernetes/enhancements/tree/master/keps/sig-auth/3766-referencegrant

They propose using ReferenceGrant resource as a policy for both side cross-namespace references. This allows to use any sort of namespace/name or 

ref:
  name: <name>
  namespace: <namespace>

openly, with a future expectation that a reference grant resource can be checked before processing the reference and executing the logic behind it.

I think this is a clean approach, allowing to rely on expectation that in the future the regular namespace/name or ref can be secured, and does not require changes in the ClusterClass CRD to manage this policy (can be used as-is). Until the ReferenceGrant support is fully added, a cross-namespace ClusterClass reference inside the cluster can trigger a warning in the webhook output, if the specified namespace is different from the one in the cluster.

What do you think, will this help moving this issue to a solution?

k8s-triage-robot commented 4 months ago

This issue is labeled with priority/important-soon but has not been updated in over 90 days, and should be re-triaged. Important-soon issues must be staffed and worked on either currently, or very soon, ideally in time for the next release.

You can:

For more details on the triage process, see https://www.kubernetes.dev/docs/guide/issue-triage/

/remove-triage accepted

sbueringer commented 4 months ago

/priority important-longterm

fabriziopandini commented 3 months ago

/triage accepted Still something to figure out about the API, but overall there is consensus

k8s-triage-robot commented 2 weeks ago

This issue is labeled with priority/important-soon but has not been updated in over 90 days, and should be re-triaged. Important-soon issues must be staffed and worked on either currently, or very soon, ideally in time for the next release.

You can:

For more details on the triage process, see https://www.kubernetes.dev/docs/guide/issue-triage/

/remove-triage accepted

salasberryfin commented 2 weeks ago

/triage accepted