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.

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

📖 Namespace separation proposal #11691

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

Conversation

marek-veber
Copy link

What type of PR is this?:

/kind documentation

What this PR does / why we need it:

This PR serves as a starting point for the discussion about running multiple instances and defining which installation will watch which namespace.

See issues:

@k8s-ci-robot k8s-ci-robot added kind/documentation Categorizes issue or PR as related to documentation. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-area PR is missing an area label labels Jan 15, 2025
@k8s-ci-robot k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 15, 2025
@k8s-ci-robot
Copy link
Contributor

[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 assign enxebre for approval. 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:

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

@k8s-ci-robot
Copy link
Contributor

Hi @marek-veber. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

Copy link
Contributor

@elmiko elmiko left a comment

Choose a reason for hiding this comment

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

thanks @marek-veber , the design makes sense to me, i have some questions about various details.

a provisioning cluster which is provisioned cluster at the same time

also, i'm finding this phrase to be slightly confusing, is there a clearer way to state it?


## Motivation
Our motivation is to have a provisioning cluster which is provisioned cluster at the same time while using hierarchical structure of clusters.
Two namespaces are used by management cluster and the rest of namespaces are watched by CAPI manager to manage other managed clusters.
Copy link
Contributor

Choose a reason for hiding this comment

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

i think these sentences could be a little clearer, i'm not fully understanding the motivation.

Copy link
Member

Choose a reason for hiding this comment

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

+1
TBH this seems more a description of a solution (use a ns as a tenancy boundary), with some assumption how organizations should use this feature (two namespaces, why two?)

Let's please expand on the problem we are trying to solve in this paragraph, so we can then properly understand the solution down below

Copy link
Author

Choose a reason for hiding this comment

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

Thanks, I used @serngawy 's suggested version here.

* https://github.com/kubernetes-sigs/cluster-api/issues/11193

### Goals
We need to extend the existing feature to limit watching on specified namespace.
Copy link
Contributor

Choose a reason for hiding this comment

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

just to be clear this is extending the existing feature for supporting multiple instances of cluster-api?

Copy link
Contributor

@nrb nrb Jan 16, 2025

Choose a reason for hiding this comment

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

I think it's extending the existing --namespace option to allow more than 1 namespace to be watched, like L95 of this document.

Copy link
Author

Choose a reason for hiding this comment

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

Would be such a formulation better:

1. extend the existing "cache configuration" feature `--namespace=<ns>` (limit watching a single namespace) to limit watching on multiple namespaces.
2. add new feature to watch on all namespaces except selected ones.
3. then we run multiple CAPI controller instances:
   - each watching only specified namespaces: `capi1-system`, …, `capi$(N-1)-system`
   - and the last resort instance to watch the rest of namespaces excluding the namespaces already watched by previously mentioned instances

?

We need to extend the existing feature to limit watching on specified namespace.
We need to run multiple CAPI controller instances:
- each watching only specified namespaces: `capi1-system`, …, `capi$(N-1)-system`
- and the last resort instance to watch the rest of namespaces excluding the namespaces already watched by previously mentioned instances
Copy link
Contributor

Choose a reason for hiding this comment

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

is there prior art on cluster-api controllers watching multiple namespaces? (just curious)

Copy link
Contributor

Choose a reason for hiding this comment

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

https://github.com/kubernetes-sigs/cluster-api/blob/main/main.go#L170-L171 shows that we can take in a single namespace, which eventually gets specified as a controller-runtime cache.Option.DefaultNamespaces option (https://github.com/kubernetes-sigs/cluster-api/blob/main/main.go#L346).

The documentation for that field (https://github.com/kubernetes-sigs/controller-runtime/blob/main/pkg/cache/cache.go#L182-L193) implies to me that multiple namespaces are supported, but I'm not familiar with the implementation, and I know that somewhat recently @sbueringer and @fabriziopandini put a lot of effort into making sure caching was optimized.

As I understand things right now, CAPI's controllers will watch either 1 namespace or all namespaces. Watching a number of namespaces between those two extremes is either not supported or well understood right now, based on my interpretation of what was said in the community meeting.

Copy link
Contributor

Choose a reason for hiding this comment

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

Watching a number of namespaces between those two extremes is either not supported or well understood right now, based on my interpretation of what was said in the community meeting.

ack, thanks @nrb, this is what i was wondering.

Copy link
Member

@fabriziopandini fabriziopandini Jan 28, 2025

Choose a reason for hiding this comment

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

My personal opinion is that we don't really know if the existing --namespace and --watch-filter flags are working today (also when filtering by one ns), because AFAIK no one really took time to dig into foundational problems like sharding in caches, web hooks in a multi instance deployment, version skew and probably more

Copy link
Author

Choose a reason for hiding this comment

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

My personal opinion is that we don't really know if the existing --namespace and --watch-filter flags are working today (also when filtering by one ns), because AFAIK no one really took time to dig into foundational problems like sharding in caches, web hooks in a multi instance deployment, version skew and probably more

@fabriziopandini: I checked (on v1.8) that both of them are working. I'll check for 1.9 as well. Based on @JoelSpeed's [comment](#11370 (comment)), I assume that event filtering will consume more resources than the approach of configuring the cache using the --namespace=<ns> option.

The extension to multiple namespaces, as suggested by @enxebre here, is also working on both v1.8 and v1.9 (as I checked on my PR ).

Copy link
Member

@fabriziopandini fabriziopandini Feb 4, 2025

Choose a reason for hiding this comment

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

@fabriziopandini: I checked (on v1.8) that both of them are working

I don't doubt a CAPI instance can filter objects in reconcile, but everything else about running multiple instances of CAPI is undefined because foundational problems are not addressed.

Trying to work around those foundational issues e.g. by running a single instance of web hooks, already proved to be very fragile in the past, this is why we dropped first class support in clusterctl & CAPI.

And even ignoring those problem, the lack of a proper sharding of memory/caching/informers + the additional operational complexity TBH makes unclear the advantages/sustainability of of this model.

Last but least, let's not forget that we don't have any E2E test and probably also no unit test for those features

Copy link
Author

Choose a reason for hiding this comment

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

I checked that our use case is functional in upstream (version 1.9) as well.

I also tested running one CAPI instance based on the 1.8 branch and another based on the current upstream/main branch, and I found it functional too.

I understand @fabriziopandini 's arguments and agree that all of them are fully relevant, but sometimes the obscure use of CAPI is necessary. 😔 See e.g.: https://kubernetes.slack.com/archives/C8TSNPY4T/p1723645630888909

I promise to create some E2E tests for it.

Copy link
Member

@fabriziopandini fabriziopandini Feb 12, 2025

Choose a reason for hiding this comment

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

I understand @fabriziopandini 's arguments and agree that all of them are fully relevant, but sometimes the obscure use of CAPI is necessary. 😔 See e.g.: https://kubernetes.slack.com/archives/C8TSNPY4T/p1723645630888909

Unfortunately the link doesn't work for me, so cannot follow up properly

Copy link
Author

@marek-veber marek-veber Feb 12, 2025

Choose a reason for hiding this comment

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

I created a user story base on this Slack discussion here: https://github.com/marek-veber/cluster-api/blob/namespace-separation-proposal/docs/proposals/20241127-namespace-separation.md#story-5---two-different-versions-of-capi-out-of-scope

As an example I prepared a deployment with:

  1. the 1st instance in the namespace capi1-system
    • upstream based image (1.9) with patches:
      • to enable use --namespace=<..> option multiple times
      • and adding --excluded-namespace=<..>
    • see quay.io/mveber/cluster-api/cluster-api-controller-amd64:dev
  2. the 2nd instance in the namespace capi2-system
    • a downstream of capi based on 1.8 with patches:
      • to enable use --namespace=<..> option multiple times
      • and adding --excluded-namespace=<..>
    • see quay.io/mveber/ose-cluster-kube-cluster-api-rhel9-operator:v4.18-ns-separation

See the deploy-capi2.sh (inside muti-capi-deployment.TGZ) for the deployment of two included helm charts into a kind cluster.

I checked that both CAPI instances can be used for provisioning clusters into AWS using a capa provider.

Copy link
Author

Choose a reason for hiding this comment

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

But that’s just one user story for a complex (obscure) environment.

There are other cases where --excluded-namespace=<..> and multiple --namespace=<..> can be used within a single CAPI instance scenario.

```

### User Stories
We need to deploy two CAPI instances in the same cluster and divide the list of namespaces to assign some well known namespaces to be watched from the first instance and rest of them to assign to the second instace.
Copy link
Contributor

Choose a reason for hiding this comment

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

this user story helps me to understand the motivation a little better, it might be nice to have some of this language in that section too.

Copy link
Author

Choose a reason for hiding this comment

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

Is this (@serngawy 's version) better?:

## Motivation
For multi-tenant environment a cluster is used as provision-er using different CAPI providers using CAPI requires careful consideration of namespace isolation
to maintain security and operational boundaries between tenants. In such setups, it is essential to configure the CAPI controller instances
to either watch or exclude specific groups of namespaces based on the isolation requirements.
This can be achieved by setting up namespace-scoped controllers or applying filters, such as label selectors, to define the namespaces each instance should monitor.
By doing so, administrators can ensure that the activities of one tenant do not interfere with others, while also reducing the resource overhead by limiting the scope of CAPI operations.
This approach enhances scalability, security, and manageability, making it well-suited for environments with strict multi-tenancy requirements.
Our motivation is to have a provisioning cluster that also serves as a provisioned cluster, leveraging a hierarchical structure of clusters.
Two namespaces are used by the management cluster, while the remaining namespaces are monitored by the CAPI manager to oversee other managed clusters.

### User Stories
We need to deploy two CAPI instances in the same cluster and divide the list of namespaces to assign some well known namespaces to be watched from the first instance and rest of them to assign to the second instace.

#### Story 1 - RedHat Hierarchical deployment using CAPI
Copy link
Contributor

Choose a reason for hiding this comment

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

i don't think this is specific to Red Hat, it seems anyone who is doing this type of heirarchical deployment could gain benefit from this enhancement.

i do think it's nice to include the links to the Red Hat jiras.

Copy link
Author

Choose a reason for hiding this comment

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

fixed

A service account will be created for each namespace with CAPI instance.
In the simple deployment example we are considering that all CAPI-instances will share the one cluster role `capi-aggregated-manager-role` so all CAPI's service accounts will be bound using then cluster role binding `capi-manager-rolebinding`.
We can also use multiple cluster roles and grant the access more granular only to the namespaces watched by the instance.

Copy link
Contributor

Choose a reason for hiding this comment

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

will all the controllers use the same cloud credentials or will each namespace have its own cloud credentials?

Copy link
Author

Choose a reason for hiding this comment

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

I'm using the common ClusterRole with access to watch all namespaces (as defined in the default CAPI deployment) in the example. However, we can define multiple ClusterRoles, one for each instance, and when an instance is watching only selected namespaces, we can restrict access to just those namespaces.

@enxebre
Copy link
Member

enxebre commented Jan 16, 2025

I think overall the motivation behind this proposal should be something like "Enable adoption in advance multi tenant scenarios".

Use case:
As a Service Provider/Consumer I own a management cluster that allocates and manages the lifecycle of Kubernetes clusters powered by CAPI using at least 2 different paradigms.
Paradigm 1:
Each cluster of type 1 runs each own suite of capi controllers targeting a particular namespace as a hidden implementation engine. Don't use webhooks. Motivations:

  • Granular versioning lifecycling
  • Granular Logging and forwarding mechanism
  • Granular metrics
  • Granular resource consumption budget
  • Security requirements
    -- Per cluster Network policies isolation
    -- Per cluster Cloud provider creds isolation
    -- Per cluster Kubeconfig access isolation

Paradigm 2:
Each cluster of type 2 is managed by a common centralized suite of CAPI controllers with different requirements/constraints to the listed above.

For both paradigms to coexist, paradigm 2 wants a way to restrict its scope and not be aware of CAPI resources owned by paradigm1.

cc @fabriziopandini @sbueringer @serngawy I'm catching up with yesterday community call so wanted to share my thoughts on the topic. I agree with the concerns raised on the call. Thinks that I think could be explored:

Use watchFilterValue
Use RBAC
Add some flexibility to let the manager cache to filter out

<!-- END doctoc generated TOC please keep comment here to allow auto update -->

## Summary
We need to run multiple CAPI instances in one cluster and divide the namespaces to be watched by given instances.
Copy link
Contributor

@serngawy serngawy Jan 16, 2025

Choose a reason for hiding this comment

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

lets add the ability to run a single CAPI instance that can watch group of namespaces OR exclude group of namespaces from been watched.

Copy link
Author

Choose a reason for hiding this comment

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

added

* https://github.com/kubernetes-sigs/cluster-api/pull/11370 add the new commandline option `--excluded-namespace=<ns1, …>`

## Motivation
Our motivation is to have a provisioning cluster which is provisioned cluster at the same time while using hierarchical structure of clusters.
Copy link
Contributor

@serngawy serngawy Jan 16, 2025

Choose a reason for hiding this comment

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

Motivation:

For multi-tenant environment a cluster is used as provision-er using different CAPI providers.
Using CAPI requires careful consideration of namespace isolation to maintain security and operational boundaries between tenants. In such setups, it is essential to configure the CAPI controller instances to either watch or exclude specific groups of namespaces based on the isolation requirements. This can be achieved by setting up namespace-scoped controllers or applying filters, such as label selectors, to define the namespaces each instance should monitor. By doing so, administrators can ensure that the activities of one tenant do not interfere with others, while also reducing the resource overhead by limiting the scope of CAPI operations. This approach enhances scalability, security, and manageability, making it well-suited for environments with strict multi-tenancy requirements.

Copy link
Author

Choose a reason for hiding this comment

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

used

- and the last resort instance to watch the rest of namespaces excluding the namespaces already watched by previously mentioned instances

This change is only a small and strait forward update of the existing feature to limit watching on specified namespace by commandline `--namespace <ns>`

Copy link
Contributor

Choose a reason for hiding this comment

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

Lets clarify the goal is to let the CAPI instance able to watch/exclude group of namespaces which will enhances the scalability, security, and manageability in multi tenant environment

Copy link
Author

Choose a reason for hiding this comment

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

Now it is:

### Goals
There are some restrictions while using multiple providers, see: https://cluster-api.sigs.k8s.io/developer/core/support-multiple-instances
But we need to:

1. extend the existing "cache configuration" feature `--namespace=<ns>` (limit watching a single namespace) to limit watching on multiple namespaces.
2. add new feature to watch on all namespaces except selected ones.
3. then we run multiple CAPI controller instances:
   - each watching only specified namespaces: `capi1-system`, …, `capi$(N-1)-system`
   - and the last resort instance to watch the rest of namespaces excluding the namespaces already watched by previously mentioned instances

ok?

@nrb
Copy link
Contributor

nrb commented Jan 16, 2025

/area provider/core

@k8s-ci-robot k8s-ci-robot added area/provider/core Issues or PRs related to the core provider and removed do-not-merge/needs-area PR is missing an area label labels Jan 16, 2025
@nrb
Copy link
Contributor

nrb commented Jan 16, 2025

I like @enxebre's explanation of paradigms.

As I've understood the goals, another way of stating 2 paradigms could be a CAPI install for self-managing the cluster (paradigm 1) and a CAPI install managing everything else that users may create (paradigm 2).

In this scenario, the self-managing CAPI install wants to watch exactly 1 namespace (let's call it ns/internal-capi), and I think that's covered with the --namespace option today.

The CAPI installation for everything else, then, would like to watch all namespaces except ns/internal-capi. I don't think this option is supported today, and would be served by implementing #11193 in some fashion.

Copy link
Member

@fabriziopandini fabriziopandini left a comment

Choose a reason for hiding this comment

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

I left a first round of comments of summary and motivations.

I might be wrong, but my impression is that the core problem we are trying to solve here is multi tenancy in core CAPI, while sharding, either by namespaces or by other criteria, is one of the solution to solve this problem (e.g for multi-tenacy in infrastructure providers we chosed a different approach, Single Controller Multitenancy, unfortunately this was never properly documented in CAPI, rif #4514).

If, I'm correct in assuming that multi-tenancy is the problem this proposal is solving, then my best suggestion is to clearly define what model of multi-tenancy this document is proposing, because I think that the current definition we have in the glossary doesn't match what we are discussing here.

@enxebre comment is definitely a step in the right direction to clarify why we are doing this work (thank you!); I have a few questions on paradigm 1, but probably the definition above will help me also to answers to those questions.

edited 20th of January

Another option - might be less confusing - is use "sharding" as a key term for this proposal / for considerations about assigning subset of CAPI clusters to different instances of CAPI controllers running on the same cluster (and keep the current definition of multi-tenancy in CAPI)

* https://github.com/kubernetes-sigs/cluster-api/pull/11370 add the new commandline option `--excluded-namespace=<ns1, …>`

## Motivation
Our motivation is to have a provisioning cluster which is provisioned cluster at the same time while using hierarchical structure of clusters.
Copy link
Member

@fabriziopandini fabriziopandini Jan 17, 2025

Choose a reason for hiding this comment

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

might be we need a glossary section if we are introducing new terms like provisioning cluster, provisioned cluster, hierarchical structure of clusters.

Copy link
Author

Choose a reason for hiding this comment

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

How about choosing the right terminology? (Sorry, my fault):

  • Provisioning cluster → Management cluster
  • Provisioned cluster → Managed cluster

Does this make it clearer?

Copy link
Member

Choose a reason for hiding this comment

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

Using Management cluster instead of Provisioning cluster seems a step in the right direction.

Managed cluster is still confusing to me, is it a managed kubernetes offering like AKS, GKE etc (see https://cluster-api.sigs.k8s.io/reference/glossary#managed-kubernetes)? or it is what we call workload cluster? might be what you are trying to convey is a self-hosted cluster (a management cluster that manage a workload cluster representing itself), but I'm really confused.

Also struggling to put this in the context of a hierarchical structure of clusters (we don't have something similar)

Copy link
Contributor

Choose a reason for hiding this comment

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

To clarify the distinction between the management/Provisioning cluster and the Provisioned/managed cluster, it's helpful to understand that this follows a typical Hub-Spoke architecture model in OCM. In this multi-namespace separation model, which we are proposing for the OCM use case, the Hub cluster (management/Provisioning) is responsible for creating other clusters. The Spoke cluster (Provisioned/managed) is a cluster created by the Hub cluster. In addition to cluster creation, the Hub cluster also deploys workloads and policies to the Spoke clusters.
For example, the Hub cluster applies different policies to Custom Resources (CRs) created under the aws-prod and aws-stage namespaces. Meanwhile, the CAPI instance running in the same Hub cluster will only create clusters (CAPI CRs) within those same aws-prod and aws-stage namespaces.

hope that helps clarify things a bit.

Copy link
Member

@fabriziopandini fabriziopandini Feb 6, 2025

Choose a reason for hiding this comment

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

Thanks for the clarification.

Unfortunately, every product has its own terminology and we cannot pick one for every proposal, this will be very confusing (e.g. a CAPI management cluster, is called Provisioning cluster in OCM, supervisor cluster in VCF etc. A workload cluster in CAPI is called provisioned or managed cluster in OCM, guest cluster in VCF etc).

IMO we should stick to the glossary/terms that exists in CAPI (management and workload cluster) and that we are using consistently across the proposal and the documentation.

Similarly, I would recommend to not refer to or to not give for granted concepts or practices from a specific product documentation (like e.g. Hub and scope in OCM), because CAPI is used in many (sometime surprising) ways.

Our motivation is to have a provisioning cluster which is provisioned cluster at the same time while using hierarchical structure of clusters.
Two namespaces are used by management cluster and the rest of namespaces are watched by CAPI manager to manage other managed clusters.

Our enhancement is also widely required many times from the CAPI community:
Copy link
Member

Choose a reason for hiding this comment

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

Those are two issues from the same person, we can quote them as a reference, but I struggle a little bit to find them representative of other's member needs (one issue is stale, the other has just few comments)

FYI I'm aware of other persons occasionally asking about this, but ultimately no one stepped up to tackle this point before now.

Also, please note that our controllers today have a --watch-filter flag, as well there are issues like #7775, which are the expression of different need/requirement about how sharding of CAPI clusters should work.

Ideally this work should address both requirements, or at least it should be documented how this proposal does not prevent making progress on the other approach if and when someone will step up to work on it.

Copy link
Author

Choose a reason for hiding this comment

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

Fixed

Copy link
Member

Choose a reason for hiding this comment

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

Just for double checking before the next pass, did you also addressed

Ideally this work should address both requirements, or at least it should be documented how this proposal does not prevent making progress on the other approach if and when someone will step up to work on it.


## Motivation
Our motivation is to have a provisioning cluster which is provisioned cluster at the same time while using hierarchical structure of clusters.
Two namespaces are used by management cluster and the rest of namespaces are watched by CAPI manager to manage other managed clusters.
Copy link
Member

Choose a reason for hiding this comment

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

+1
TBH this seems more a description of a solution (use a ns as a tenancy boundary), with some assumption how organizations should use this feature (two namespaces, why two?)

Let's please expand on the problem we are trying to solve in this paragraph, so we can then properly understand the solution down below

Comment on lines 54 to 72
We want and consider:
- each CAPI instance:
- is running in separate namespace and is using its own service account
- can select by command the line arguments the list of namespaces:
- to watch - e.g.: `--namespace <ns1> --namespace <ns2>`
- to exclude from watching - e.g.: `--excluded-namespace <ns1> --excluded-namespace <ns2>`
- we are not supporting multiple versions of CAPI
- all running CAPI-instances:
- are using the same container image (same version of CAPI)
- are sharing global resources:
- CRDs:
- cluster.x-k8s.io:
- addons.cluster.x-k8s.io: clusterresourcesetbindings, clusterresourcesets
- cluster.x-k8s.io: clusterclasses, clusters, machinedeployments, machinehealthchecks, machinepools, machinesets, machines
- ipam.cluster.x-k8s.io: ipaddressclaims, ipaddresses
- runtime.cluster.x-k8s.io: extensionconfigs
- NOTE: the web-hooks are pointing from the CRDs into the first instance only
- the `ClusterRole/capi-aggregated-manager-role`
- the `ClusterRoleBinding/capi-manager-rolebinding` to bind all service accounts for CAPI instances (e.g. `capi1-system:capi-manager`, ..., `capiN-system:capi-manager`) to the `ClusterRole`
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 it is me, but I struggle a little bit in understanding if this list is about a problem statement, or if it is about details of a solution or some constraint we have/were introduced by this proposal.

Frankly speaking, I would suggest to keep in the summary only a short sentence about the solution, and move those details down in the proposal, where possibly we will have some more context to explain why some decisions.

Copy link
Author

Choose a reason for hiding this comment

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

done

superseded-by:
---

# Support running multiple instances of the same provider, each one watching different namespaces
Copy link
Member

Choose a reason for hiding this comment

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

if possible, let's keep the title of the PR in sync with the title of the proposal ("namespace separation" is a little bit confusing awhen read without context).

Also wondering if the title should have something about multi-tenancy or sharding, but I'm not entirely sure at this stage

- can select by command the line arguments the list of namespaces:
- to watch - e.g.: `--namespace <ns1> --namespace <ns2>`
- to exclude from watching - e.g.: `--excluded-namespace <ns1> --excluded-namespace <ns2>`
- we are not supporting multiple versions of CAPI
Copy link
Contributor

Choose a reason for hiding this comment

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

There will at times inevitably be some drift between versions, how much skew can there be at runtime before this causes an issue? Is patch level skew ok?

Copy link
Author

Choose a reason for hiding this comment

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

Good question. I think we’ll be fine as long as no CRD changes are required between the parallel running instances.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think that CAPI can guarantee that multiple versions of the same provider can work together. Basically this means we would end up with CRDs and webhooks of one version having to work with another version of the controller

(Nobody considers today if a specific version of CAPI can work with the older or newer versions of the CRDs and webhooks)

- to exclude from watching - e.g.: `--excluded-namespace <ns1> --excluded-namespace <ns2>`
- we are not supporting multiple versions of CAPI
- all running CAPI-instances:
- are using the same container image (same version of CAPI)
Copy link
Contributor

Choose a reason for hiding this comment

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

This and line 60 say the same thing

- cluster.x-k8s.io: clusterclasses, clusters, machinedeployments, machinehealthchecks, machinepools, machinesets, machines
- ipam.cluster.x-k8s.io: ipaddressclaims, ipaddresses
- runtime.cluster.x-k8s.io: extensionconfigs
- NOTE: the web-hooks are pointing from the CRDs into the first instance only
Copy link
Contributor

Choose a reason for hiding this comment

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

How does one define first here? Do we mean the first installed? What if an admin wanted it not to be in some random namespace because that happened to be the first deployed? Would it not be better to recommend having an instance of the CAPI controllers running separately just to serve webhooks?

Can CAPI controllers run as just webhooks? Could we disable all controllers within a binary to achieve something like this?

Copy link
Author

Choose a reason for hiding this comment

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

I think we don’t have to be too strict. 😉
If:

  • The CRD definitions come from the newest version (based on all running instances).
  • The conversion webhooks are handled by the newest instance version.
  • The CRDs remain compatible with the oldest instance version.
    ... then it might just work. 😊

Copy link
Author

Choose a reason for hiding this comment

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

I created such a deployment with two different instances (one OC downstream based on capi 1.8, the second upstream based 1.9). See https://kubernetes.slack.com/archives/C8TSNPY4T/p1738954981484819?thread_ts=1723645630.888909&cid=C8TSNPY4T

* the code change is only extending an existing hash with one item to multiple items
* the maintenance complexity shouldn't be extended here
* add the new commandline option `--excluded-namespace=<ens1, …>` to define list of excluded namespaces
* the code change is only setting an option `Cache.Options.DefaultFieldSelector` to disable matching with any of specified namespace's names
Copy link
Contributor

Choose a reason for hiding this comment

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

For those who aren't familiar with this option, it may be good to explain what this is and how it works. IIUC this means there's a API server side filter that means we don't return any results from the specified namespace here right?

Edit: I see this further down in the doc, a link might be handy

Copy link
Author

Choose a reason for hiding this comment

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

E.g. this:

  * the code [change](https://github.com/kubernetes-sigs/cluster-api/pull/11370/files#diff-c4604297ff388834dc8c6de30c847f50548cd6dd4b2f546c433b234a27ad4631R263) is only setting an option `Cache.Options.DefaultFieldSelector` to disable matching with any of specified namespace's names

?

....
}
```
and then the namespaces contained in `watchNamespacesList` will be excluded from watching by the instance.
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the rbac implication of this field selector, do I not need RBAC on that namespace?

@marek-veber marek-veber force-pushed the namespace-separation-proposal branch from a153b24 to 4aa1b44 Compare January 20, 2025 15:10
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 20, 2025
@marek-veber marek-veber force-pushed the namespace-separation-proposal branch from 4aa1b44 to c796bdf Compare January 29, 2025 12:25
@fabriziopandini
Copy link
Member

Looks like the PR have unrelated commits

@marek-veber marek-veber force-pushed the namespace-separation-proposal branch from c796bdf to 595f4df Compare February 11, 2025 19:07
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 11, 2025
@marek-veber
Copy link
Author

Looks like the PR have unrelated commits

Thanks, fixed.

@marek-veber marek-veber force-pushed the namespace-separation-proposal branch from 595f4df to 39f305f Compare February 12, 2025 17:04
@marek-veber marek-veber force-pushed the namespace-separation-proposal branch from 39f305f to 9f0c1d7 Compare February 12, 2025 17:15
Two distinct paradigms coexist to address different operational and security requirements.

### Paradigm 1: Isolated Cluster Management
Each Kubernetes cluster operates its own suite of CAPI controllers, targeting specific namespaces as a hidden implementation engine.
Copy link
Member

Choose a reason for hiding this comment

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

I think most controllers including building blocks like CRD's are setup in a way to have one (or multiple but one active via leader election) instances/deployments per Cluster.

IMHO this is how kubernetes is built, just like there's only a single version of kube-controller-manager (except during upgrades), but not different kube-controller-managers per namespace.

Do you have examples of controllers which are targeting specific namespaces?

Copy link
Member

Choose a reason for hiding this comment

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

In this model each namespace represents a managed cluster boundary, each cluster is driven by their own controllers and CRs within that namespace. This model satisfies the requirements above (Key Features)

Copy link
Contributor

Choose a reason for hiding this comment

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

The provider contracts explain that a --namespace flag should be supported by providers, and I believe that is designed to support this use case.

IIUC, you could configure the validation/defaulting webhooks in the same way too? The only piece that must be shared are conversion webhooks?


### Paradigm 1: Isolated Cluster Management
Each Kubernetes cluster operates its own suite of CAPI controllers, targeting specific namespaces as a hidden implementation engine.
This paradigm avoids using webhooks and prioritizes isolation and granularity.
Copy link
Member

Choose a reason for hiding this comment

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

I don't get the point about prioritisation.

If we are in a situation that one cluster is stuck getting fixed for too long because there are too many others, then we have a scaling issue which should be solved instead.

- Reduces management overhead through centralization.
- Prioritizes ease of use and scalability over strict isolation.

The addition of the new command-line option `--excluded-namespace=<ns1, …>` is proposed in this PR [#11370](https://github.com/kubernetes-sigs/cluster-api/pull/11370).
Copy link
Member

Choose a reason for hiding this comment

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

I don't get the use-case of using --excluded-namespace, who manages clusters in that namespace? Or why exclude namespaces if nothing is supposed to be in there?

Shouldn't this be part of Kubernetes and as simple as (I think this does not work this way today):

  • Using rbac to allow a controller (its service account) to only access the resources it needs?

Copy link
Member

Choose a reason for hiding this comment

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

I'd expect this use case to be addressable by rbac that's a great point.

(I think this does not work this way today)

what would prevent it from working?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure (might be wrong), if we can have rbac to limit privilege for a specific namespace same time we have privilege over all namespaces and future namespaces. Otherwise we need to specify rbac for each namespace.

Copy link
Author

Choose a reason for hiding this comment

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

ad https://kubernetes.io/docs/reference/access-authn-authz/rbac/#role-and-clusterrole cite: An RBAC Role or ClusterRole contains rules that represent a set of permissions. Permissions are purely additive (there are no "deny" rules).

Copy link
Contributor

Choose a reason for hiding this comment

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

The issue here is that the informer will try to watch all namespaces, and lack of RBAC for a namespace will just mean that the informer breaks.

You would need a way to tell the informer (through a field selector perhaps?) that the informer should ignore resources in a particular namespace. I expect though, that the field selector solution doesn't integrate well with RBAC and you'd still need the RBAC to list/watch all namespaces even if the API server never returned an item from a particular namespace


### User Stories
#### Story 1 - Hierarchical deployment using CAPI:
In [OCM](https://github.com/open-cluster-management-io/ocm) environment there is `hub Cluster` managing multiple `klusterlets` (managed clusters/ spoke clusters).
Copy link
Member

Choose a reason for hiding this comment

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

Question: is the "hub Cluster" created by Cluster API? And is that Cluster managing itself?

Did you consider using a solution like KCP for tenancy instead (Note: this is not KubeadmControlPlane)?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah "strict multi-tenancy" in a regular Kubernetes cluster is really a stretch. I would say impossible as soon as cluster-wide objects like CRDs, Mutating/ValidatingWebhookConfigurations, ExtensionConfigs, all sorts of other cluster-wide objects that infrastructure providers have are used, but might depend on the definition of "strict multi-tenancy".

#### Story 1 - Hierarchical deployment using CAPI:
In [OCM](https://github.com/open-cluster-management-io/ocm) environment there is `hub Cluster` managing multiple `klusterlets` (managed clusters/ spoke clusters).

See the `Cluster namespace` term definition on [this](https://open-cluster-management.io/docs/concepts/architecture/) page, cite:
Copy link
Member

Choose a reason for hiding this comment

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

Please use cluster-api wording.

Copy link
Member

Choose a reason for hiding this comment

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

Agree. Honestly. I don't know how to interpret this. Can we please translate this into upstream Cluster API terms so that folks that are not familiar with OCM can understand it?

* last-resort instance (deployed in `capiR-system`) is watching the rest of namespaces


#### Story 5 - Two different versions of CAPI (out of scope):
Copy link
Member

Choose a reason for hiding this comment

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

Let's remove this if it is out of scope and maybe best to add as non-goal as from discussions this seems not to be archievable.

Copy link
Member

Choose a reason for hiding this comment

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

Agree. If it's out of scope, it should be a non-goal not a user story

We need to limit the list of namespaces to watch. It's possible to do this now, but only on one namespace and we need to watch on multiple namespaces by one instance.

#### Story 3 - Centralized Cluster Management
We need to exclude the list of namespaces from watch to reduces management overhead through centralization.
Copy link
Member

Choose a reason for hiding this comment

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

Please clarify this story.

Why exclude when wanting a centralized management?
Under centralized I e.g. understand having a single instance of Cluster API running.

We need to exclude the list of namespaces from watch to reduces management overhead through centralization.

#### Story 4 - combination of Isolated and Centralized Cluster Management
We need to deploy multiple CAPI instances in the same cluster and divide the list of namespaces to assign certain well-known namespaces to be watched from the given instances and define an instance to watch on the rest of them.
Copy link
Member

Choose a reason for hiding this comment

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

That's a solution, whats the user story?

Copy link
Member

Choose a reason for hiding this comment

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

generally speaking can we please format stories as "As a Persona I want to X so..."

The extension to enable the existing command-line option `--namespace=<ns1, …>` define multiple namespaces is proposed in this PR [#11397](https://github.com/kubernetes-sigs/cluster-api/pull/11397).

### Paradigm 2: Centralized Cluster Management
This paradigm manages multiple Kubernetes clusters using a shared, centralized suite of CAPI controllers. It is designed for scenarios with less stringent isolation requirements.
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this how Clsuter API already today works by default?

Copy link
Member

Choose a reason for hiding this comment

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

I would say both models are default capi, they just represent different API consumption choices. But this is the typical consumption model yeh

@enxebre
Copy link
Member

enxebre commented Feb 13, 2025

@marek-veber would relying on existing watchFilterValue and labling resources at creation time explicitly satisfy all user stories in this proposal?
If that's the case then fixing watchFilterValue to move out from the predicate filtering level to be computed by the manager cache via DefaultFieldSelector: fields.AndSelectors( might be a possible path forward.
I think we should fix that in any case anyways.

Two distinct paradigms coexist to address different operational and security requirements.

### Paradigm 1: Isolated Cluster Management
Each Kubernetes cluster operates its own suite of CAPI controllers, targeting specific namespaces as a hidden implementation engine.
Copy link
Member

Choose a reason for hiding this comment

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

I really have trouble parsing this. Is "Kubernetes cluster" a management or workload cluster here?

Management cluster doesn't seem to make sense because that would mean there is only one instance of all CAPI providers? (which I think is not what you are aiming for)

Workload cluster would mean the workload cluster operates the CAPI providers, which also doesn't seem to make sense?

Maybe we mean that we'll run one suite of CAPI controllers for every workload cluster?

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed, I think this needs re-wording. My understanding is the same as Stefan's last point, on the management cluster, there is a distinct CAPI suite of controllers, per workload cluster


### Paradigm 1: Isolated Cluster Management
Each Kubernetes cluster operates its own suite of CAPI controllers, targeting specific namespaces as a hidden implementation engine.
This paradigm avoids using webhooks and prioritizes isolation and granularity.
Copy link
Member

Choose a reason for hiding this comment

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

I don't think CAPI can guarantee that it works without its webhooks

Copy link
Contributor

Choose a reason for hiding this comment

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

Mutating/ValidatingWebhooKConfiguration can be configured to operate only on specific namespaces using a namespaceSelector, so I'm not sure if deploying 1 suite per workload cluster needs to have webhooks disabled, does it?

Conversion webhooks cannot operate this way and are global, but sharing the CRD across multiple different versions feels like a whole other conversation?


**Key Features**:

- **Granular Lifecycle Management**: Independent versioning and upgrades for each cluster's CAPI components.
Copy link
Member

Choose a reason for hiding this comment

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

It won't work to run different versions of CAPI on the same management cluster.

We only have one set of CRDs. If you run a CAPI version that doesn't match the version of the CRDs really bad things will happen

Copy link
Contributor

Choose a reason for hiding this comment

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

If you run a CAPI version that doesn't match the version of the CRDs really bad things will happen

I think there could be an argument against this statement. The Gateway API folks are dealing with lots of similar issues at the moment and there are things that can be done to make sure this isn't an issue (though not necessarily easy)

  • Defaulting/Validation/Controllers should all be from the same version +/- CAPIs support (version skew across upgrades)
    • So if webhooks are within the namespace, this is fine and the same as any other CAPI deployment
  • The CRD must be compatible with the version of all controllers
    • This means the newest version of the controllers is likely to dictate the version of the CRD
    • It puts a backwards limit on the support for older controllers, when we have multiple schema versions, we can support a wide range, when one is dropped, any controller relying on that version must have been upgraded first
    • There's a performance impact because conversion webhooks may be used heavily in a scenario like this
    • Conversion webhooks are singletons, these should match the version of the CRD
    • Dead fields are a thing, and a real risk. There are solutions that could be built to prevent the use of dead fields within a namespace if we knew which fields were supported at the controllers version of the namespace

There's probably more I've missed

I'm think a blanket "bad things will happen" is probably too broad of a statement, and I'd be interested if you have specific examples of bad things you'd be concerned about?


- **Granular Lifecycle Management**: Independent versioning and upgrades for each cluster's CAPI components.
- **Logging and Metrics**: Per-cluster logging, forwarding, and metric collection.
- **Resource Isolation**: Defined resource budgets for CPU, memory, and storage on a per-cluster basis.
Copy link
Member

Choose a reason for hiding this comment

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

Because I have no idea how this is implemented I don't know what the implications of this are for Cluster API

(similar for the security requirements below)


**Characteristics**:

- Operates under simplified constraints compared to [Paradigm 1](#paradigm-1-isolated-cluster-management).
Copy link
Member

Choose a reason for hiding this comment

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

What does simplified mean? This is very fuzzy

(I don't know in which way the constraints here are simplified compared to the above)

* last-resort instance (deployed in `capiR-system`) is watching the rest of namespaces


#### Story 5 - Two different versions of CAPI (out of scope):
Copy link
Member

Choose a reason for hiding this comment

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

Agree. If it's out of scope, it should be a non-goal not a user story

#### Story 1 - Hierarchical deployment using CAPI:
In [OCM](https://github.com/open-cluster-management-io/ocm) environment there is `hub Cluster` managing multiple `klusterlets` (managed clusters/ spoke clusters).

See the `Cluster namespace` term definition on [this](https://open-cluster-management.io/docs/concepts/architecture/) page, cite:
Copy link
Member

Choose a reason for hiding this comment

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

Agree. Honestly. I don't know how to interpret this. Can we please translate this into upstream Cluster API terms so that folks that are not familiar with OCM can understand it?

- can select by command the line arguments the list of namespaces:
- to watch - e.g.: `--namespace <ns1> --namespace <ns2>`
- to exclude from watching - e.g.: `--excluded-namespace <ns1> --excluded-namespace <ns2>`
- we are not supporting multiple versions of CAPI
Copy link
Member

Choose a reason for hiding this comment

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

I don't think that CAPI can guarantee that multiple versions of the same provider can work together. Basically this means we would end up with CRDs and webhooks of one version having to work with another version of the controller

(Nobody considers today if a specific version of CAPI can work with the older or newer versions of the CRDs and webhooks)


### User Stories
#### Story 1 - Hierarchical deployment using CAPI:
In [OCM](https://github.com/open-cluster-management-io/ocm) environment there is `hub Cluster` managing multiple `klusterlets` (managed clusters/ spoke clusters).
Copy link
Member

Choose a reason for hiding this comment

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

Yeah "strict multi-tenancy" in a regular Kubernetes cluster is really a stretch. I would say impossible as soon as cluster-wide objects like CRDs, Mutating/ValidatingWebhookConfigurations, ExtensionConfigs, all sorts of other cluster-wide objects that infrastructure providers have are used, but might depend on the definition of "strict multi-tenancy".

- each watching only specified namespaces: `capi1-system`, …, `capi$(N-1)-system`
- and the last resort instance to watch the rest of namespaces excluding the namespaces already watched by previously mentioned instances

### Non-Goals/Future Work
Copy link
Member

Choose a reason for hiding this comment

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

I assume clusterctl support is out of scope?

If yes, we should:

  • add it to non-goals
  • provide instructions on how to deploy and operate CAPI in both paradigms

Two distinct paradigms coexist to address different operational and security requirements.

### Paradigm 1: Isolated Cluster Management
Each Kubernetes cluster operates its own suite of CAPI controllers, targeting specific namespaces as a hidden implementation engine.
Copy link
Contributor

Choose a reason for hiding this comment

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

The provider contracts explain that a --namespace flag should be supported by providers, and I believe that is designed to support this use case.

IIUC, you could configure the validation/defaulting webhooks in the same way too? The only piece that must be shared are conversion webhooks?

Two distinct paradigms coexist to address different operational and security requirements.

### Paradigm 1: Isolated Cluster Management
Each Kubernetes cluster operates its own suite of CAPI controllers, targeting specific namespaces as a hidden implementation engine.
Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed, I think this needs re-wording. My understanding is the same as Stefan's last point, on the management cluster, there is a distinct CAPI suite of controllers, per workload cluster


### Paradigm 1: Isolated Cluster Management
Each Kubernetes cluster operates its own suite of CAPI controllers, targeting specific namespaces as a hidden implementation engine.
This paradigm avoids using webhooks and prioritizes isolation and granularity.
Copy link
Contributor

Choose a reason for hiding this comment

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

Mutating/ValidatingWebhooKConfiguration can be configured to operate only on specific namespaces using a namespaceSelector, so I'm not sure if deploying 1 suite per workload cluster needs to have webhooks disabled, does it?

Conversion webhooks cannot operate this way and are global, but sharing the CRD across multiple different versions feels like a whole other conversation?


**Key Features**:

- **Granular Lifecycle Management**: Independent versioning and upgrades for each cluster's CAPI components.
Copy link
Contributor

Choose a reason for hiding this comment

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

If you run a CAPI version that doesn't match the version of the CRDs really bad things will happen

I think there could be an argument against this statement. The Gateway API folks are dealing with lots of similar issues at the moment and there are things that can be done to make sure this isn't an issue (though not necessarily easy)

  • Defaulting/Validation/Controllers should all be from the same version +/- CAPIs support (version skew across upgrades)
    • So if webhooks are within the namespace, this is fine and the same as any other CAPI deployment
  • The CRD must be compatible with the version of all controllers
    • This means the newest version of the controllers is likely to dictate the version of the CRD
    • It puts a backwards limit on the support for older controllers, when we have multiple schema versions, we can support a wide range, when one is dropped, any controller relying on that version must have been upgraded first
    • There's a performance impact because conversion webhooks may be used heavily in a scenario like this
    • Conversion webhooks are singletons, these should match the version of the CRD
    • Dead fields are a thing, and a real risk. There are solutions that could be built to prevent the use of dead fields within a namespace if we knew which fields were supported at the controllers version of the namespace

There's probably more I've missed

I'm think a blanket "bad things will happen" is probably too broad of a statement, and I'd be interested if you have specific examples of bad things you'd be concerned about?

- Reduces management overhead through centralization.
- Prioritizes ease of use and scalability over strict isolation.

The addition of the new command-line option `--excluded-namespace=<ns1, …>` is proposed in this PR [#11370](https://github.com/kubernetes-sigs/cluster-api/pull/11370).
Copy link
Contributor

Choose a reason for hiding this comment

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

The issue here is that the informer will try to watch all namespaces, and lack of RBAC for a namespace will just mean that the informer breaks.

You would need a way to tell the informer (through a field selector perhaps?) that the informer should ignore resources in a particular namespace. I expect though, that the field selector solution doesn't integrate well with RBAC and you'd still need the RBAC to list/watch all namespaces even if the API server never returned an item from a particular namespace

Comment on lines +154 to +156
- We don't need to support for multiple CAPI versions, but:
- All instances must be compatible with the deployed CRDs.
- CRDs are deployed only for the newest CAPI instance (selecting one instance with the newest version).
Copy link
Contributor

Choose a reason for hiding this comment

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

In what order are components of CAPI upgraded today? There will always be a small period where the CRDs are either newer or older during upgrades, so I assume we already have to solve the problems of a small version skew, which would mean it's probably "safe" running a small version skew for the long term provided there's consistency between the validation, defaulting and controllers

- We don't need to support for multiple CAPI versions, but:
- All instances must be compatible with the deployed CRDs.
- CRDs are deployed only for the newest CAPI instance (selecting one instance with the newest version).
- All conversion webhooks in CRDs point to the newest CAPI instance.
Copy link
Contributor

Choose a reason for hiding this comment

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

It would probably be safer to assert that CRDs and their conversion are handled separately (as a separate binary) rather than running them alongside controllers and potentially "moving around" if one becomes newer

- All instances must be compatible with the deployed CRDs.
- CRDs are deployed only for the newest CAPI instance (selecting one instance with the newest version).
- All conversion webhooks in CRDs point to the newest CAPI instance.
- `MutatingWebhookConfiguration` and `ValidatingWebhookConfiguration` are deployed only for the newest CAPI instance and point to it.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is likely to cause issues. The mutating webhook may add a field, that the controller does not understand, which it may then try to remove (through a deserialize/serialize loop), and that could then flip back and forth

Webhooks for mutation and validation should be tied to the same version as the controllers operating on the resources

- CRDs are deployed only for the newest CAPI instance (selecting one instance with the newest version).
- All conversion webhooks in CRDs point to the newest CAPI instance.
- `MutatingWebhookConfiguration` and `ValidatingWebhookConfiguration` are deployed only for the newest CAPI instance and point to it.
- If instances use different API versions, conversion must be handled correctly by the conversion webhooks.
Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed, this is an issue we'd have to solve, commonly called dead fields as far as I've seen.

IMO, preventing the addition of dead fields in a namespace where the controllers wouldn't understand them is likely the safest approach.

Provided all code (default/validation/controller) in the namespace is at the same level, the only actor who would be able to add a dead field would be the user

- `ipam.cluster.x-k8s.io`: `ipaddressclaims`, `ipaddresses`
- `runtime.cluster.x-k8s.io`: `extensionconfigs`
- NOTE: Webhooks from CRDs point to `Service/capi-webhook-service` of the newest instance only.
- MutatingWebhookConfiguration and ValidatingWebhookConfiguration point to `Service/capi-webhook-service` of the newest instance only.
Copy link
Contributor

Choose a reason for hiding this comment

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

As above, I think this poses a risk

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/provider/core Issues or PRs related to the core provider cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/documentation Categorizes issue or PR as related to documentation. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants