Conversation
eb5da59 to
cba04f0
Compare
|
Hi @Cali0707 thanks for the review. You're right. We are still working on it, which is why we marked it as WIP, and it's likely we'll find something else. We won't be able to verify everything until Kiali 2.24 is released next Monday. |
69abe42 to
0217d00
Compare
Signed-off-by: Alberto Gutierrez <aljesusg@gmail.com>
Signed-off-by: Alberto Gutierrez <aljesusg@gmail.com>
Signed-off-by: Alberto Gutierrez <aljesusg@gmail.com>
3ea7cf9 to
6fcba0a
Compare
|
Hi @Cali0707 ! This isr eady. Can we run it? |
Signed-off-by: Alberto Gutierrez <aljesusg@gmail.com>
2d5d4cf to
fcbcaf8
Compare
|
/run-mcpchecker kiali |
mcpchecker MCP Evaluation ResultsCommit:
|
Cali0707
left a comment
There was a problem hiding this comment.
Overall this refactor looks fine to me. I also understand the concerns around duplicated logic for these tools.
I guess my only two worries would be around if there will be any issues with versioning this MCP server vs. the kiali one (what happens if the kiali backend updates and has different params, but the user doesn't update the MCP server or vice-versa?), as well as how the kiali endpoint mixes with the multi cluster support
One interesting approach we have seen downstream is having the toolset be an external dependency that is imported into the server, maybe that would be another option that would at least solve the versioning issues?
| method = http.MethodGet | ||
| } | ||
| func (k *Kiali) ExecuteRequest(ctx context.Context, endpoint string, arguments map[string]any) (string, error) { | ||
| ApiCallURL, err := k.validateAndGetURL(endpoint) |
There was a problem hiding this comment.
@aljesusg does this approach work with any of the multi cluster work in the MCP server? Or is this going to always go to a specific kiali instance?
Ready
Summary
/api/chat/mcp/*endpoints instead of implementing the logic locallyMotivation
Kiali's UI already ships a built-in chatbot that uses the same MCP tool semantics. Maintaining two independent implementations of the same tools — one in the Kiali backend and one here in the MCP server — created several problems:
What changed
Architecture
Before: each tool handler built REST API calls to multiple Kiali endpoints, parsed JSON responses, computed derived data (health rollups, graph merges), and formatted output.
After: each tool handler is a thin wrapper that POSTs to
<kiali-url>/api/chat/mcp/<tool_name>with the tool arguments as JSON. Kiali's backend owns all data fetching, aggregation, and LLM-optimized formatting.Deleted (~3,000 lines)
pkg/kiali/: 15 files removed —health_calculation.go(626 lines),get_mesh_graph.go,graph.go,traces.go,logs.go,services.go,workloads.go,istio.go,mesh.go,namespaces.go,validations.go,types.go,endpoints.go, etc.pkg/toolsets/kiali/: 7 files removed —get_mesh_graph.go,get_metrics.go,get_resource_details.go,get_traces.go,helpers.go,helpers_test.go,logs.go,manage_istio_config.goAdded
pkg/toolsets/kiali/tools/: 9 thin tool handlers (~60–90 lines each) that define the tool schema and delegate tokiali.ExecuteRequest()pkg/toolsets/kiali/tools/endpoints.go: centralized MCP endpoint path constantspkg/toolsets/kiali/internal/defaults/: shared default values for tool parameterspkg/kiali/tests/: endpoint coverage test ensuring every endpoint constant is wired to a tool implementation, plus backend contract test scaffoldingImproved test coverage
kiali_test.go: no-config initialization, empty endpoint, non-2xx error handling (with/without body), JSON body serialization, header assertions (Content-Type,X-Kubernetes-MCP-Server, POST method), Bearer token edge cases (empty, already-prefixed)config_test.go: directValidate()tests for nil config, empty URL, invalid URL, URL without scheme, HTTP vs HTTPS certificate requirementspkg/mcp/kiali_test.go: rewritten to test the new architecture (single POST per tool, MCP endpoint paths, JSON argument forwarding)Benefits
Test plan
make testpasses (all 25 packages green)