refactor(nacos): migrate from SDK v1 to v2, remove v1 dependency#982
refactor(nacos): migrate from SDK v1 to v2, remove v1 dependency#982Aias00 wants to merge 8 commits into
Conversation
Migrate all Nacos SDK imports from the deprecated v1.1.3 (2020) to v2.3.2, consolidating on a single version and eliminating dependency bloat. Key changes: - Updated all import paths from nacos-sdk-go to nacos-sdk-go/v2 - Changed SubscribeCallback signature from []SubscribeService to []Instance (v2 delivers Instance directly in callbacks, no manual conversion needed) - Removed generateInstance() helper functions that converted SubscribeService to Instance (3 files: service_listener.go, listener.go, nacos.go) - Removed fromSubscribeServiceToServiceInstance() in springcloud/nacos.go, now using fromInstanceToServiceInstance() directly in the callback - Added @@ group prefix stripping for ServiceName in springcloud callback (previously handled by removed conversion function) - Removed references to model.Instance.Valid field (removed in v2) - Added ServerHealthy() and CloseClient() methods to test mock to satisfy v2's INamingClient interface - Updated all test data from SubscribeService to Instance type Files modified: pkg/remote/nacos/client.go (imports only) pkg/configcenter/nacos_load.go (imports only) pkg/adapter/dubboregistry/registry/nacos/ (5 files) pkg/adapter/springcloud/servicediscovery/nacos/ (1 file) pkg/adapter/llmregistry/registry/nacos/ (3 files) go.mod / go.sum (v1 removed) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR migrates the codebase from github.com/nacos-group/nacos-sdk-go v1 to github.com/nacos-group/nacos-sdk-go/v2, updating imports and adapting to the v2 subscription callback API.
Changes:
- Switched Nacos SDK imports across adapters/config/remote clients from v1 to v2.
- Updated Nacos subscribe callbacks from
[]SubscribeServiceto[]Instanceand removed now-unneeded conversion helpers. - Updated unit tests and mocks to match the v2 client interfaces.
Reviewed changes
Copilot reviewed 12 out of 13 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/remote/nacos/client.go | Updates Nacos SDK imports to v2 for naming client usage. |
| pkg/configcenter/nacos_load.go | Updates Nacos SDK imports to v2 for config client usage. |
| pkg/adapter/springcloud/servicediscovery/nacos/nacos.go | Adapts subscribe callback signature to v2 Instance and adjusts service name parsing logic. |
| pkg/adapter/llmregistry/registry/nacos/registry.go | Updates Nacos SDK imports to v2 for naming registry integration. |
| pkg/adapter/llmregistry/registry/nacos/listener.go | Updates callback types to v2 Instance and removes SubscribeService→Instance conversion. |
| pkg/adapter/llmregistry/registry/nacos/listener_test.go | Updates mocks/tests for v2 types and adds methods required by updated client interfaces. |
| pkg/adapter/dubboregistry/registry/nacos/service_listener.go | Updates callback types to v2 Instance and removes conversion helper. |
| pkg/adapter/dubboregistry/registry/nacos/registry.go | Updates Nacos SDK imports to v2 for Dubbo registry integration. |
| pkg/adapter/dubboregistry/registry/nacos/interface_listener.go | Updates Nacos SDK imports to v2 for listener integration. |
| pkg/adapter/dubboregistry/registry/nacos/application_service_listener.go | Updates callback types to v2 Instance and uses Instance directly. |
| pkg/adapter/dubboregistry/registry/nacos/application_listener.go | Updates Nacos SDK imports to v2 for application listener integration. |
| go.mod | Removes v1 nacos-sdk-go requirement and keeps v2. |
| go.sum | Removes v1.1.3 checksum entries and keeps v2 entries. |
Comments suppressed due to low confidence (1)
pkg/adapter/dubboregistry/registry/nacos/application_service_listener.go:1
handleServiceName(per the provided context) returns""when there is no"@@"in the input. This code unconditionally overwritesservices[i].ServiceName, which can blank out valid service names and break downstream logic that relies onInstance.ServiceName. Fix by changinghandleServiceNameto return the originalserviceNamewhen no group prefix is present, or only overwriteServiceNamewhen the split actually finds a suffix.
/*
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // v2 subscribe callback receives Instance directly (was SubscribeService in v1) | ||
| // ServiceName may contain group prefix like "DEFAULT_GROUP@@service-name", strip it | ||
| serviceName := service.ServiceName | ||
| if tmp := strings.Split(serviceName, "@@"); len(tmp) == 2 { | ||
| serviceName = tmp[1] | ||
| } | ||
|
|
||
| instance := fromInstanceToServiceInstance(serviceName, service) |
There was a problem hiding this comment.
Already addressed in commit 233d147b — the @@ prefix strip now uses strings.Cut(serviceName, "@@") (split once, no extra allocation). See nacos.go Callback.
| key := instance.GetUniqKey() | ||
| newInstanceMap[instance.GetUniqKey()] = instance |
There was a problem hiding this comment.
Already addressed in commit 233d147b — key := instance.GetUniqKey() is now computed once and reused for the map write (newInstanceMap[key] = instance).
…qKey call - Replace strings.Split with strings.Cut for group prefix stripping (cleaner idiom, avoids unnecessary allocation) - Fix duplicate instance.GetUniqKey() call by reusing key variable Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #982 +/- ##
===========================================
+ Coverage 26.29% 27.28% +0.98%
===========================================
Files 275 274 -1
Lines 21985 21993 +8
===========================================
+ Hits 5782 6000 +218
+ Misses 15590 15370 -220
- Partials 613 623 +10
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
- Add tests for handleServiceName function (@@ prefix stripping) - Add tests for toNacosInstance function - Add tests for Callback skipping disabled instances - Add tests for fromInstanceToServiceInstance function - Add tests for generateURL function Covers the new code paths from the v1 to v2 SDK migration.
Split imports into separate blocks: - Standard library imports - Third-party imports
| "github.com/nacos-group/nacos-sdk-go/clients/naming_client" | ||
| nacosConstant "github.com/nacos-group/nacos-sdk-go/common/constant" | ||
| "github.com/nacos-group/nacos-sdk-go/vo" | ||
| "github.com/nacos-group/nacos-sdk-go/v2/clients" |
There was a problem hiding this comment.
[P1] 补齐 Nacos 1.x 与 gRPC 端口的升级门禁
这不是纯 API 替换。Nacos 官方兼容说明明确：2.x 客户端不兼容 1.x 服务端，并且客户端会通过主端口 +1000（默认 8848 → 9848）建立 gRPC；存在防火墙、容器映射或代理时必须额外开放该 TCP 端口。当前仓库的 Nacos 配置/文档仍只要求 8848，docs/ai/registry*.md 甚至仍示例导入 v1 vo，也没有声明最低服务端版本，因此现有 Nacos 1.x 或只开放 8848 的部署会在升级后运行期失联。请保留兼容路径，或明确最低 Nacos 2.x、补充 9848/TCP 转发与升级文档，并增加真实 Nacos 2.x 的连接验证。官方依据：https://github.com/nacos-group/nacos-group.github.io/blob/5e9d9ca33caeac97ebef7146f6f3f2b3a82793f6/src/content/docs/v2/en/upgrading/200-compatibility.md
There was a problem hiding this comment.
已处理（commit 0e94c1ee）。未保留 v1 兼容回退路径（会重新引入 v1 依赖，与本次迁移目的相悖），改为按你给的第二个方案：
- 在
docs/ai/registry.md/registry_CN.md顶部新增 Nacos 2.x 服务端要求 说明：v2 客户端不兼容 1.x 服务端，最低要求 2.x（建议 2.2.0+）。 - 明确 gRPC 端口 = 主端口 + 1000（8848→9848），并要求防火墙/容器映射/反代场景同时开放
8848/TCP与9848/TCP，否则运行期失联。docs/ai/mcp/mcp.md/mcp_CN.md同步补充。 - 修正示例中过时的 v1 导入
nacos-sdk-go/vo→nacos-sdk-go/v2/vo。
真实 2.x 连接验证受限于 CI 无 Nacos 实例，未新增 e2e；关闭路径已用 mock 断言。
| return true | ||
| } | ||
|
|
||
| func (m *mockNacosClient) CloseClient() {} |
There was a problem hiding this comment.
[P1] 生产关闭路径没有消费 v2 的 CloseClient
这里为满足 v2 INamingClient 补了空实现，但生产生命周期仍只做 listener/unsubscribe：LLM DoUnsubscribe、Spring Cloud 的 Nacos 包装器以及配置中心都没有调用 CloseClient()。v2 的实现会在该方法中执行 gRPC Shutdown() 并取消内部 context；仓库现有 MCP v2 适配器也在关闭时显式关闭 naming/config client。当前遗漏会让 Stop/Apply 或优雅退出后的连接、重试协程继续存活。请在所有本次迁移的客户端创建点补齐可达的关闭接口，在取消订阅后调用它，并用 mock 断言关闭发生。
There was a problem hiding this comment.
已处理（commit 0e94c1ee）。在所有本次迁移的客户端创建点补齐了可达的关闭接口，取消订阅/停止后调用 CloseClient()，并用 mock 断言关闭发生：
- llmregistry：
NacosRegistry.DoUnsubscribe在停掉 listener 后调client.CloseClient()；抽出newNacosRegistryWithClient供测试注入 mock，新增TestDoUnsubscribeClosesClient断言CloseClient被调用 1 次。 - springcloud：
nacosServiceDiscovery.Unsubscribe末尾调n.client.Close()；client 字段抽象为nacosNamingClient接口（*nacos.NacosClient仍满足），mock 记录Close，新增TestUnsubscribeClosesClient。 - configcenter：给
ConfigClient/Load接口与ConfigManager加Close()，NacosConfig.Close透传 SDKCloseClient，TestDefaultConfigLoad_Close用 mock 断言转发。 - dubboregistry：原
DoUnsubscribe是panic("implement me")，改为真实关闭（Listener.Close+client.CloseClient），与ZKRegistry对齐。 - pkg/remote/nacos：
NacosClient.Close暴露namingClient.CloseClient()。
SDK 的 CloseClient 内部有 isClosed 守卫，幂等，重复 Stop 安全。
| if _, after, ok := cutServiceName(serviceName, "@@"); ok { | ||
| serviceName = after | ||
| } | ||
| assert.Equal(t, tt.expected, serviceName) |
There was a problem hiding this comment.
[P2] 这条测试没有执行生产 Callback
当前用测试文件里的 cutServiceName 复制了一遍分隔逻辑，生产代码即使删除前缀处理、改错分隔符或没有把结果传给 listener，这条测试仍会通过。请让 mock 记录收到的 ServiceInstance，直接调用 nacosServiceDiscovery.Callback 并断言 ServiceName，然后删除这份复制实现。
There was a problem hiding this comment.
已处理（commit 0e94c1ee）。删除了复制的 cutServiceName 实现。TestCallback_ServiceNameWithGroupPrefix 现在直接调用生产 nacosServiceDiscovery.Callback：mock listener (mockServiceEventListener) 记录收到的 ServiceInstance，断言 ServiceName 已剥离 @@ 前缀。这样若生产代码删掉前缀处理、改错分隔符或没把结果传给 listener，测试都会失败。
…ack test Address AlexStocks' [P1]/[P2] review on PR apache#982 (Nacos SDK v1→v2 migration). [P1] Close path consumes v2 CloseClient The v2 clients hold a gRPC connection + internal retry goroutines that survive Unsubscribe; the migration added empty CloseClient() to test mocks but never called it in production. Wire CloseClient into every migrated client's shutdown path and assert with mocks: - llmregistry: NacosRegistry.DoUnsubscribe closes the naming client after stopping the listener (newNacosRegistryWithClient extracted for mock injection; TestDoUnsubscribeClosesClient). - springcloud: nacosServiceDiscovery.Unsubscribe closes the naming client; client field abstracted behind a nacosNamingClient interface so a mock can record Close (TestUnsubscribeClosesClient). - configcenter: add Close() to ConfigClient/Load interfaces + ConfigManager; NacosConfig.Close delegates to the SDK (TestDefaultConfigLoad_Close). - dubboregistry: replace the panicking DoUnsubscribe with a real close (listener.Close + CloseClient), matching ZKRegistry. - pkg/remote/nacos: NacosClient.Close exposes namingClient.CloseClient. [P1] Nacos 2.x compatibility & docs v2 client is incompatible with 1.x servers and uses gRPC port = main port + 1000 (8848→9848). Document the minimum server version (2.x) and the 9848/TCP requirement in docs/ai/registry(.md/_CN.md) and docs/ai/mcp/mcp(.md/_CN.md), and fix the stale v1 SDK import (nacos-sdk-go/vo → /v2/vo) in the examples. [P2] springcloud test drives production Callback Replace the copied cutServiceName separator logic with a test that calls the real nacosServiceDiscovery.Callback and asserts the stripped ServiceName via a listener mock that records received ServiceInstances. Copilot nits (strings.Cut, duplicate GetUniqKey) were already fixed in 233d147; no further change needed. Verified: go build ./..., go vet, go test on all affected packages. Co-Authored-By: Claude <noreply@anthropic.com>
CI's imports-formatter runs with the default -bl=true (split import modules with a blank line), so testify/assert and the pkg/* imports must be in separate groups. The previous commit put them in one group, failing the "Check Import Formatting" job on PR apache#982. Co-Authored-By: Claude <noreply@anthropic.com>
Add unit tests for the v2 close paths introduced in 0e94c1e so the new shutdown code is not uncovered (Codecov flagged 22 missing lines). Each new Close/DoUnsubscribe method is now exercised with a mock that records the close call: - dubboregistry: mockNamingClient + TestDoUnsubscribeClosesClient (covers listener close, svc-listener close loop, and client.CloseClient) and TestDoUnsubscribe_NoListener (error path that replaced the panic). - pkg/remote/nacos: TestNacosClient_Close (delegates to namingClient.CloseClient). - configcenter: TestNacosConfig_Close (delegates + nil-client no-op). - pkg/config: TestConfigManager_Close (forwards to loader + nil-load no-op). All new Close methods report 100% function coverage locally. Formatter run with CI's -bl=true to keep import grouping consistent. Co-Authored-By: Claude <noreply@anthropic.com>
|
Summary
Migrate all Nacos SDK usage from the deprecated v1.1.3 (2020) to v2.3.2, consolidating on a single version and eliminating dependency bloat and potential security issues.
Problem
The project had both
github.com/nacos-group/nacos-sdk-go v1.1.3andgithub.com/nacos-group/nacos-sdk-go/v2 v2.3.2as dependencies. Onlypkg/adapter/mcpserver/registry/nacos/used v2; 5 other packages still used v1. The v1 SDK is outdated (2020) with known security issues and API incompatibilities.Changes
Import path updates (all nacos-using files)
All imports changed from
github.com/nacos-group/nacos-sdk-go/...togithub.com/nacos-group/nacos-sdk-go/v2/...Breaking API change: SubscribeCallback signature
func(services []model.SubscribeService, err error)func(services []model.Instance, err error)The
model.SubscribeServicetype no longer exists in v2 — the callback receives[]model.Instancedirectly.Code simplifications
generateInstance()helper functions (3 files) that convertedSubscribeService → Instance— no longer needed since v2 callback providesInstancedirectlyfromSubscribeServiceToServiceInstance()in springcloud/nacos.go, now usingfromInstanceToServiceInstance()directly@@group prefix stripping forServiceNamein springcloud callback (previously handled by the removed conversion function)model.Instance.Validfield (removed in v2 SDK)Test updates
SubscribeServicetoInstancetypeServerHealthy()andCloseClient()methods to test mock to satisfy v2'sINamingClientinterfaceDependency cleanup
go.mod: removedgithub.com/nacos-group/nacos-sdk-go v1.1.3direct dependencygo.sum: cleaned up v1 entriesFiles Modified (13 files, net -40 lines)
pkg/remote/nacos/client.gopkg/configcenter/nacos_load.gopkg/adapter/dubboregistry/registry/nacos/registry.gopkg/adapter/dubboregistry/registry/nacos/application_listener.gopkg/adapter/dubboregistry/registry/nacos/interface_listener.goapplication_service_listener.go[]SubscribeService→[]Instance, removegenerateInstance()service_listener.go[]SubscribeService→[]Instance, removegenerateInstance(), drop.Validlistener.go[]SubscribeService→[]Instance, removegenerateInstance(), drop.Validspringcloud/nacos.go[]SubscribeService→[]Instance, removefromSubscribeServiceToServiceInstance(), add@@prefix striplistener_test.goSubscribeService→Instance, addServerHealthy()+CloseClient()go.mod/go.sumVerification
go build ./...— passesgo vet ./...— no warningsgo mod tidy— clean, only v2 remainsgo test ./pkg/adapter/llmregistry/registry/nacos/...— all 8 tests passgo test ./pkg/adapter/mcpserver/registry/nacos/...— passesgo test ./pkg/configcenter/...— passes