-
Notifications
You must be signed in to change notification settings - Fork 189
[WIP] feat: add IsValid check for toolsets #490
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Calum Murray <[email protected]>
| ) | ||
| } | ||
|
|
||
| func (t *Toolset) IsValid(k *internalk8s.Kubernetes) bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aljesusg when you have time can you take a look at this method and let me know if this makes sense?
It's supposed to validate that there is a working kiali installation in the target cluster
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes… although I would do the following: since the AuthInfoEndpoint endpoint is already created, I would create a method to call it, as it doesn’t require any parameters and is ‘available’ whether the user is authenticated or not.
|
Will add tests to this next, then it should be good to merge (if people like the approach here) |
|
I believe that this entire functionality requires more thought. This is similar to what's already done in: kubernetes-mcp-server/pkg/api/toolsets.go Line 46 in 0e88935
and related methods/functons. Note This was part of the first spike of the MCP server hence it doesn't make sense in the current state of the cluster. In the early stages of the Kubernetes MCP server this was just enough:
In the current state (multi-cluster + auth support), when a user/client performs a list request (tools, prompts, and so on), the MCP server SHOULD account for everything that applies to that user in its specific scenario:
I'm not convinced about the idea of completely enabling or disabling an entire toolset (as this PR proposes). One of the challenges to do this properly is that go-sdk (or any of the other SDKs) require the tools to be loaded at start-up and don't provide callbacks to provide the tool or resource list dynamically or per session/user (AFAIK, maybe this has changed). In this PR we're getting the derived Kubernetes when reloading the tools, but this will likely fail if there's no global auth for that cluster. I'm really not sure how the problem can be properly tackled. Maybe checking the internals of the SDK and seeing if the list/tools request can be intercepted or something along those lines. |
This PR allows us to dynamically enable/disable toolsets, as well as the targets within tools, based on whether or not prerequisites for the toolset are installed in a given target cluster.
That way, users can enable e.g.
kialitoolset, and if it is only installed in 3/4 clusters, we will only hint targets for those 3 clusters to the MCP client. Similarly, if none of the clusters havekialiinstalled, we will simply disable the toolset and not present it to any clients.