fix: refactored mcp server auto discovery logic and fix some issue #2382
Conversation
# Conflicts: # pkg/ingress/kube/configmap/mcp_server.go # registry/nacos/mcpserver/watcher.go
Codecov ReportAttention: Patch coverage is
❌ Your patch status has failed because the patch coverage (0.00%) is below the target coverage (50.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #2382 +/- ##
===========================================
+ Coverage 35.91% 46.03% +10.12%
===========================================
Files 69 81 +12
Lines 11576 13018 +1442
===========================================
+ Hits 4157 5993 +1836
+ Misses 7104 6679 -425
- Partials 315 346 +31
🚀 New features to boost your workflow:
|
![lingma-agents[bot]](https://avatars.githubusercontent.com/in/1198481?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔍 代码评审报告
🎯 评审意见概览
| 严重度 | 数量 | 说明 |
|---|---|---|
| 🔴 Blocker | 0 | 阻断性问题，需立即修复。例如：系统崩溃、关键功能不可用或严重安全漏洞。 |
| 🟠 Critical | 6 | 严重问题，高优先级修复。例如：核心功能异常或性能瓶颈影响用户体验。 |
| 🟡 Major | 4 | 主要问题，建议修复。例如：非核心功能缺陷或代码维护性较差。 |
| 🟢 Minor | 5 | 次要问题，酌情优化。例如：代码格式不规范或注释缺失。 |
总计： 15 个问题
📋 评审意见详情
💡 单文件建议
以下是文件级别的代码建议，聚焦于代码的可读性、可维护性和潜在问题。
📄 api/networking/v1/mcp_bridge.proto (1 💬)
- 新增字段未添加注释说明 (L74-L75)
🔹 pkg/ingress/config/ingress_config.go (1 💬)
- 新增的TLS配置合并存在空指针风险 (L805-L814)
🔹 registry/mcp_model.go (5 💬)
- 修复 WasmPluginConfig 结构体中 JSON 标签的语法错误 (L39)
- 修复 RequestTemplate 结构体中关键字段的可选性配置错误 (L75-L76)
- 修复 RequestTemplateHeaders 结构体中关键字段的可选性配置错误 (L85-L86)
- 修复 ToolDescription 结构体中 inputSchema 字段的可选性配置错误 (L134)
- 修复 ToolsMeta 结构体中 JSON 标签的大小写不一致问题 (L144-L146)
🔹 registry/nacos/mcpserver/util.go (4 💬)
🔹 registry/nacos/mcpserver/watcher_test.go (2 💬)
- 提取重复的JSON配置字符串以减少代码冗余 (L114-L171)
- 改进测试断言以增强配置验证的可读性 (L554-L556)
🔹 registry/nacos/v2/watcher.go (2 💬)
- MCP服务错误处理信息不充分 (L118-L121)
- MCP服务配置参数存在冗余定义 (L296-L300)
🚀 跨文件建议
以下是对代码架构和设计的综合分析，聚焦于跨文件交互、系统一致性和潜在优化空间。
🔍 1. MCP服务器配置与Nacos注册中心v2/v3版本处理逻辑存在架构不一致性
在registry/nacos/v2/watcher.go与registry/nacos/mcpserver/watcher.go中发现MCP服务器功能的实现存在架构分层问题。Nacos v2/v3的通用注册逻辑与MCP专用逻辑存在重叠，导致配置参数传递不统一（如McpServerExportDomains在v2 watcher中通过option传递，而在mcpserver模块中直接硬编码)。这种架构分层混乱可能导致后续维护时出现配置覆盖问题和版本兼容性风险。
🔍 2. 依赖升级导致潜在版本冲突风险
在go.mod中升级nacos-sdk-go/v2到v2.3.2时，未清理旧的版本替换指令（如删除了github.com/luoxiner/nacos-sdk-go/v2的replace）。同时新引入的github.com/stretchr/objx未评估与现有依赖的兼容性。
📌 关键代码：
github.com/nacos-group/nacos-sdk-go/v2 v2.3.2
# 旧的replace指令已被删除但未验证是否完全清理
🔍 3. MCP服务器配置监听存在资源泄漏风险
在registry/nacos/mcpserver/util.go的CancelListenToServer方法中，未正确清理服务订阅监听器。当服务实例数量较多时，未调用namingClient.Unsubscribe可能导致连接累积，观察到代码中存在条件判断缺失：只有当ctx.serviceInfo存在时才执行取消订阅操作。
📌 关键代码：
if ctx.serviceInfo != nil {
err := n.namingClient.Unsubscribe(...)
}🔍 4. 跨模块配置参数传递存在重复实现
MCP服务器配置参数（如McpServerExportDomains和McpServerBaseUrl）在registry/nacos/v2/watcher.go和registry/mcp_model.go中存在重复解析逻辑。建议将配置解析抽象为公共模块以避免代码冗余。
📌 关键代码：
WithMcpExportDomains(registry.McpServerExportDomains),
WithMcpBaseUrl(registry.McpServerBaseUrl),McpServerRule struct {
Server *ServerConfig `json:"server,omitempty"`
}🔍 5. HTTPS协议支持存在安全配置缺失
在registry/nacos/mcpserver/watcher.go的generateDrForSSEService方法中，为HTTPS协议生成的DestinationRule未配置证书验证策略。当前实现仅设置mode: SIMPLE但未指定证书来源，可能导致TLS连接不安全。
📌 关键代码：
return &v1alpha3.DestinationRule{
TrafficPolicy: &v1alpha3.TrafficPolicy{
Tls: &v1alpha3.ClientTLSSettings{
Mode: v1alpha3.ClientTLSSettings_SIMPLE,
},
},
}🔍 6. DNS解析服务的端点地址未进行有效性验证
在registry/nacos/v2/watcher.go的generateServiceEntry方法中，当服务地址为DNS名称时，未验证域名可达性。当前仅通过IP格式判断区分DNS服务，但未实现DNS解析失败时的回退机制或健康检查。
📌 关键代码：
if !isValidIP(service.Ip) {
isDnsService = true
}🔍 7. 跨模块错误处理信息不充分
在registry/nacos/mcpserver/watcher.go的buildMcpServerForMcpServer等关键方法中，未对核心操作（如配置解析、服务发现）添加详细错误日志。现有错误处理仅返回通用错误信息，难以进行故障定位。
📌 关键代码：
if vs == nil {
return nil
}🔍 8. 元数据存储结构存在潜在性能隐患
在api/networking/v1/mcp_bridge.proto中新增的metadata map<string, InnerMap>结构采用嵌套map实现，未评估大规模元数据存储时的性能影响。InnerMap结构的序列化/反序列化可能成为性能瓶颈。
📌 关键代码：
message InnerMap {
map<string, string> inner_map = 1;
}
🔍 9. 服务更新通知机制存在竞态条件
在registry/nacos/mcpserver/watcher.go的Stop方法中，停止监听操作与配置清理未使用相同的锁保护。w.mutex和subMutex的混合使用可能导致并发操作时的状态不一致。
📌 关键代码：
w.mutex.Lock()
defer w.mutex.Unlock()
// 直接操作未加锁的subMutex保护的资源🔍 10. MCP服务器路由规则存在路径匹配漏洞
在registry/nacos/mcpserver/watcher.go的虚拟服务构建逻辑中，路由路径匹配未包含基础路径的精确匹配。当McpServerBaseUrl为非空时，/mcp-server/abc这样的路径可能无法正确匹配/mcp-server的精确规则。
📌 关键代码：
mergePath = strings.TrimSuffix(w.McpServerBaseUrl, "/") + mergePath
// 缺少对基础路径本身的精确匹配处理💡 小贴士
与 lingma-agents 交流的方式
📜 直接回复评论
直接回复本条评论，lingma-agents 将自动处理您的请求。例如：
-
在当前代码中添加详细的注释说明。
-
请详细介绍一下你说的 LRU 改造方案，并使用伪代码加以说明。
📜 在代码行处标记
在文件的特定位置创建评论并 @lingma-agents。例如：
-
@Lingma-Agent 分析这个方法的性能瓶颈并提供优化建议。
-
@Lingma-Agent 对这个方法生成优化代码。
📜 在讨论中提问
在任何讨论中 @lingma-agents 来获取帮助。例如：
-
@Lingma-Agent 请总结上述讨论并提出解决方案。
-
@Lingma-Agent 请根据讨论内容生成优化代码。
|
@Erica177 Please resolve the CR comments of lingma agent. |
CH3CHO
left a comment
CH3CHO
left a comment
There was a problem hiding this comment.
还剩下面两个文件没看完，先加这些评论。
- registry/nacos/mcpserver/watcher.go
- registry/nacos/mcpserver/util.go
Erica177
changed the title
…libaba#2382) Co-authored-by: johnlanni <[email protected]>
4 participants
Ⅰ. Describe what this PR did
1.refactored the code nacos mcp server auto discovered, extracting the subscription logic into a separate util function
2.add ut
3.update go mod for nacos-go-sdk
4.fix some issue
5.updated MCP Bridge CRD with added fields
Ⅱ. Does this pull request fix one issue?
#2360
#2283
#2250
#2248
#2173
Ⅲ. Why don't you add test cases (unit test/integration test)?
Ⅳ. Describe how to verify it
Ⅴ. Special notes for reviews