feat: Multi Cluster Support #348
Conversation
Signed-off-by: Calum Murray <[email protected]>
|
So far, this PR only adds the ClusterProvider interface and the implementation for a kubeconfig provider. The idea is that the provider can be used to get a Manager instance for the cluster requested in the tool provider. Barring any feedback against this approach, I will continue with adding the cluster parameter to tools whenever the number of clusters is > 1, and using the provider interface to get the correct manager to each tool. |
Signed-off-by: Calum Murray <[email protected]>
Signed-off-by: Calum Murray <[email protected]>
Signed-off-by: Calum Murray <[email protected]>
Signed-off-by: Calum Murray <[email protected]>
Signed-off-by: Calum Murray <[email protected]>
This comment was marked as resolved.
This comment was marked as resolved.
Signed-off-by: Calum Murray <[email protected]>
Signed-off-by: Calum Murray <[email protected]>
Signed-off-by: Calum Murray <[email protected]>
Signed-off-by: Calum Murray <[email protected]>
Signed-off-by: Calum Murray <[email protected]>
Signed-off-by: Calum Murray <[email protected]>
Signed-off-by: Calum Murray <[email protected]>
Signed-off-by: Calum Murray <[email protected]>
|
@manusa I tried to keep the changes separated out by commit to make reviewing easier, let me know if I should squash them |
|
It's looking good, let me review it locally and propose some changes (if applicable) as a PR to your branch. |
Signed-off-by: Calum Murray <[email protected]>
|
I'm adding a few changes to the branch and restructuring a bit. I'll try to have a PR to your branch ready later today. |
|
We can squash everything together once the feature is ready. |
Signed-off-by: Calum Murray <[email protected]>
|
Hui, note I'm working on this: https://github.com/Cali0707/kubernetes-mcp-server/compare/multi-cluster-auth-config...marcnuri-forks:kubernetes-mcp-server:review/mutli-cluster-config?expand=1 |
|
Thanks @manusa ! |
* nit: rename contexts_list to configuration_contexts_list Besides the conventional naming, it helps LLMs understand the context of the tool by providing a certain level of hierarchy. Signed-off-by: Marc Nuri <[email protected]> * fix(mcp): ToolMutator doesn't rely on magic strings Signed-off-by: Marc Nuri <[email protected]> * refactor(api): don't expose ManagerProvider to toolsets Signed-off-by: Marc Nuri <[email protected]> * test(mcp): configuration_contexts_list basic tests Signed-off-by: Marc Nuri <[email protected]> * test(toolsets): revert edge-case test This test should not be touched. Signed-off-by: Marc Nuri <[email protected]> * test(toolsets): add specific metadata tests for multi-cluster Signed-off-by: Marc Nuri <[email protected]> * fix(mcp): ToolFilter doesn't rely on magic strings (partially) Signed-off-by: Marc Nuri <[email protected]> * test(api): IsClusterAware and IsTargetListProvider default values Signed-off-by: Marc Nuri <[email protected]> * test(mcp): revert unneeded changes in mcp_tools_test.go Signed-off-by: Marc Nuri <[email protected]> --------- Signed-off-by: Marc Nuri <[email protected]>
There was a problem hiding this comment.
LGTM, thx!
Originally posted in Cali0707#1
The tool mutator is a great way to tackle the multi-cluster problem with minimal impact on the toolsets 🙌
The Kubernetes module architecture is now quite complex, Provider->Manager->Kubernetes.
We might look into merging Provider and Manager together in the future.
The InOpenShift function is making less sense now, especially under the new multi-cluster scenario where some clusters might be openshift while others not.
We should plan for removal. This should also simplify the GetTools-related logic.
In the auth section, I'm not convinced about the changes to KubernetesApiTokenVerifier.
However, this will likely be removed. In the token exchange scenario this wasn't relevant any more.
The output for the configuration_contexts_list tool should be changed to something LLM friendlier and maybe parseable.
I'll probably add some follow-up tasks based on how we agree to proceed.
|
@alexeykazakov would you mind giving a look and see if this would work for your current use-case? 🙏 |
Signed-off-by: Calum Murray <[email protected]>
Signed-off-by: Calum Murray <[email protected]>
|
@Cali0707 @manusa Latest enhancements work great! Only comment is what @alexeykazakov already mentioned related to the documentation |
|
I'll merge now since the PR is quite complex as it is
To be updated in a follow-up. Also maybe add an opt-out option for multi-cluster before cutting the release. |
4 participants
Fixes #83
Supersedes closes #325
This PR builds on the approach taken in #325, but makes it more generic so that we can configure how clients select the cluster based on the cluster provider strategy (currently supporting in-cluster and kubeconfig).
There is a
contexts_listtool added as well, which is conditionally enabled when there are more than 15 contexts in the kubeconfig, as that is when we switch from using an enum parameter to a general string parameter.