search

kubeflow / mpi-operator

Kubernetes Operator for MPI-based applications (distributed training, HPC, etc.)
https://www.kubeflow.org/docs/components/training/mpi/
Apache License 2.0
430 stars 216 forks source link

Running in a subset of namespaces #620

Open emsixteeen opened 7 months ago

emsixteeen commented 7 months ago

Currently it doesn't seem possible to run the mpi-operator in a subset of namespaces. It's either all namespaces or a specific (single) namespace.

This limitation looks like it comes from Kubernetes itself, where [generated] Informers are either scoped to a single namespace using <factory>.WithNamespace(...), or if no namespace is provider, defaults to metav1.NamespaceAll.

An example (real-world) use-case for running the mpi-operator in subset of namespaces is when it's deployed to a Kubernetes cluster with tightly controlled cluster-wide permissions. E.g. when obtaining cluster-wide access to Secrets is non-starter. In such a case, it's still possible to create a namespace-local RoleBinding, whereby access can be granted to the Service Account running the mpi-operator for namespace-local secrets. However, because the mpi-operator operates only in either all namespaces or a single namespace, using namespace-local RoleBindings isn't expandable beyond a single namespace.

kuizhiqing commented 7 months ago

It can be implemented with

mpiJobInformer.Informer().AddEventHandler(cache.FilteringResourceEventHandler{FilterFunc: ...

in the enqeue process.

If we need it, I'd like to contribute.

emsixteeen commented 7 months ago

It seems that cache.FilteringResourceEventHandler will filter items once they have already been [successfully] listed / retrieved from the API server.

The use-case / issue here is when the mpi-operator might not have permissions to list items in some namespace to begin with.

kuizhiqing commented 7 months ago

As a feature of functionality, FilteringResourceEventHandler is the right way to run controller in a subset of namespaces, while the RBAC staffs is quite another thing.

emsixteeen commented 7 months ago

@kuizhiqing totally agree that filtering is the way to go – assuming that RBAC permissions already allows for resources to be listed in all namespaces.

The current use-case / problem is that the Kubernetes cluster has a really restrictive RBAC configuration. With that being the case, because the controller is unable to list certain objects in the namespace to begin with (such as Secrets), getting to to filtering can never happen (classic chicken-and-egg situation: to filter we need to list, but to list, we need RBAC permissions, and RBAC is restricting those operations).

In trying to solve this issue, I came across https://github.com/maistra/xns-informer, which (using codegen) creates a multi-namespace informer out of existing informers.

It seems that all the informers it generates are API-compatible with client-go and mpi-operator informers, and I've done some testing to validate that here: https://github.com/emsixteeen/mpi-operator/tree/xns-informers.

On the bright side, using these generated multi-namespace informers looks like it can solve this issue, but on the darker side, bringing in this third-party library, along with all it's dependencies and codegen is [probably] non-starter!

kuizhiqing commented 7 months ago

It would be better to contribute to client-go project for the informer part if it worth doing so.

emsixteeen commented 7 months ago

Doing a bit of light research^1 on the topic leads me to believe that there is no appetite for doing this in client-go 😞 ...

emsixteeen commented 7 months ago

Been doing a bit more thinking about this and I'm wondering if having a level of abstraction / indirection when creating Informers could be an acceptable approach? 😕

In other words abstract the creation of an Informer to something more pluggable. When creating an Informer (such as here, instead of calling <package>.NewSharedInformerFactoryWithOptions(), it would instead call a function that would return the desired <package>.SharedInformerFactory interface. That way if one wanted to use something like generated multi-namespace Informers in place of the default Informer, as long as the interface returned was API compatible, then it could work.

I'll work on mocking something up to validate this idea, and [hopefully] update the issue with a prototype ...

emsixteeen commented 7 months ago

Looks like making informers pluggable is a workable concept!

I've created a branch^1 and associated project ^2 to test it ...