[AI-8th] fix: respect virtual host and port in dubbo provider bootstrap#1556
[AI-8th] fix: respect virtual host and port in dubbo provider bootstrap#1556chxbca wants to merge 4 commits into
Conversation
📝 WalkthroughWalkthroughDubbo provider bootstrap now resolves host/port from Changes
Sequence Diagram(s)sequenceDiagram
participant Provider as DubboProviderBootstrap
participant Server as ServerConfig
participant Cache as DubboSingleton (SERVER_MAP)
participant Protocol as ProtocolConfig
participant Registry as Registry/URL builder
Note over Provider,Server: export / copy server fields
Provider->>Server: read host, port, virtualHost, virtualPort
Provider->>Provider: resolveHost(server), resolvePort(server)
Provider->>Cache: getOrCreateProtocolConfig(buildServerCacheKey(resolved, original))
Cache-->>Provider: ProtocolConfig (cached or new)
Provider->>Protocol: copyServerFields(server, protocol) -- sets host/port to resolved values
Provider->>Registry: buildUrls(protocol, resolvedHost, resolvedPort)
Registry-->>Provider: registration URLs (include uniqueId)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
bootstrap/bootstrap-dubbo/src/main/java/com/alipay/sofa/rpc/bootstrap/dubbo/DubboProviderBootstrap.java (1)
206-207: Apply virtual host/port resolution inbuildUrls()for consistency.The method constructs URLs using
server.getHost()andserver.getPort()directly at lines 206-207, but elsewhere in the classcopyServerFields()applies virtual address resolution viaresolveHost()andresolvePort()helpers (lines 135-142). Since both methods handle host/port configuration,buildUrls()should use the same resolution logic to ensure consistent URL construction when virtual addresses are configured.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bootstrap/bootstrap-dubbo/src/main/java/com/alipay/sofa/rpc/bootstrap/dubbo/DubboProviderBootstrap.java` around lines 206 - 207, buildUrls() currently concatenates server.getHost() and server.getPort() directly; change it to use the same virtual address resolution helpers used in copyServerFields(): call resolveHost(...) and resolvePort(...) (the same overloads used there) when constructing the host and port parts so that buildUrls() uses resolved values rather than raw server.getHost()/server.getPort(); keep existing usage of server.getProtocol() and server.getContextPath() unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@bootstrap/bootstrap-dubbo/src/main/java/com/alipay/sofa/rpc/bootstrap/dubbo/DubboProviderBootstrap.java`:
- Around line 206-207: buildUrls() currently concatenates server.getHost() and
server.getPort() directly; change it to use the same virtual address resolution
helpers used in copyServerFields(): call resolveHost(...) and resolvePort(...)
(the same overloads used there) when constructing the host and port parts so
that buildUrls() uses resolved values rather than raw
server.getHost()/server.getPort(); keep existing usage of server.getProtocol()
and server.getContextPath() unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 307847d5-fc64-40e9-a0a1-ba3d6777496f
📒 Files selected for processing (2)
bootstrap/bootstrap-dubbo/src/main/java/com/alipay/sofa/rpc/bootstrap/dubbo/DubboProviderBootstrap.javabootstrap/bootstrap-dubbo/src/test/java/com/alipay/sofa/rpc/bootstrap/dubbo/DubboProviderBootstrapTest.java
There was a problem hiding this comment.
Pull request overview
This PR fixes Dubbo provider address publication/registration in NAT/container/LB/port-mapping scenarios by ensuring virtualHost / virtualPort from ServerConfig are used (with fallbacks) when building Dubbo ProtocolConfig and provider URLs.
Changes:
- Prefer
virtualHostoverhostandvirtualPortoverportinDubboProviderBootstrap.copyServerFields()(with fallbacks). - Update
buildUrls()to use the same resolved host/port. - Add unit tests covering virtual host/port combinations and URL generation.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| bootstrap/bootstrap-dubbo/src/main/java/com/alipay/sofa/rpc/bootstrap/dubbo/DubboProviderBootstrap.java | Resolve host/port using virtualHost/virtualPort and apply to ProtocolConfig + URL construction. |
| bootstrap/bootstrap-dubbo/src/test/java/com/alipay/sofa/rpc/bootstrap/dubbo/DubboProviderBootstrapTest.java | Add tests validating virtual host/port resolution and URL output. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| private void copyServerFields(ServerConfig serverConfig, ProtocolConfig protocolConfig) { | ||
| void copyServerFields(ServerConfig serverConfig, ProtocolConfig protocolConfig) { |
There was a problem hiding this comment.
copyServerFields was changed from private to package-private for test access; the repo commonly annotates such methods with @VisibleForTesting to document intent and discourage production callers from depending on it. Consider adding the annotation (and import) here.
| void copyServerFields(ServerConfig serverConfig, ProtocolConfig protocolConfig) { | ||
| protocolConfig.setId(serverConfig.getId()); | ||
| protocolConfig.setName(serverConfig.getProtocol()); | ||
| protocolConfig.setHost(serverConfig.getHost()); | ||
| protocolConfig.setPort(serverConfig.getPort()); | ||
| protocolConfig.setHost(resolveHost(serverConfig)); | ||
| protocolConfig.setPort(resolvePort(serverConfig)); |
There was a problem hiding this comment.
copyServerFields now derives ProtocolConfig host/port from virtualHost/virtualPort. However, copyServers caches ProtocolConfig instances in DubboSingleton.SERVER_MAP keyed by ServerConfig, and ServerConfig.equals/hashCode only considers protocol/host/port (not virtualHost/virtualPort). This means different ServerConfig instances that share the same bound host/port but differ in virtualHost/virtualPort can collide and reuse a ProtocolConfig with the wrong published address. Consider changing the cache key to include the resolved host/port (or virtualHost/virtualPort), or avoid caching by ServerConfig.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
bootstrap/bootstrap-dubbo/src/test/java/com/alipay/sofa/rpc/bootstrap/dubbo/DubboProviderBootstrapTest.java (1)
219-239: Consider adding an explicit test for same key + conflicting non-address fields.This test validates reuse for identical resolved addresses, but cache behavior is still implicit when two
ServerConfigs share the same cache key and differ in fields likeserialization,threadpool, orthreads. Adding one explicit test would lock expected behavior (reuse-first vs reject-conflict) and prevent silent config bleed regressions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bootstrap/bootstrap-dubbo/src/test/java/com/alipay/sofa/rpc/bootstrap/dubbo/DubboProviderBootstrapTest.java` around lines 219 - 239, Add an explicit unit test in DubboProviderBootstrapTest that covers two ServerConfig instances which resolve to the same cache key (same virtualHost/virtualPort) but differ in non-address fields (e.g., setSerialization(...), setThreadpool(...), setThreads(...)); call dubboProviderBootstrap.getOrCreateProtocolConfig(...) for both and assert the expected policy (either Assert.assertSame to lock reuse-first behavior or assert that a conflict/error is raised depending on the intended contract) so the cache behavior for conflicting non-address fields is explicitly specified and prevented from regressing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@bootstrap/bootstrap-dubbo/src/test/java/com/alipay/sofa/rpc/bootstrap/dubbo/DubboProviderBootstrapTest.java`:
- Around line 163-166: The test in DubboProviderBootstrapTest currently asserts
that the generated URL string starts with "?uniqueId=" which is order-sensitive
and flaky; update the assertions around buildUrls() and the url variable to
parse the URL's query string (or split on '?' and then on '&') and assert that
one of the query parameters equals or starts with "uniqueId=" (i.e., verify
presence of uniqueId key/value rather than its position), using the existing url
from dubboProviderBootstrap.buildUrls().get(0).toString() in the test to locate
and validate the parameter.
---
Nitpick comments:
In
`@bootstrap/bootstrap-dubbo/src/test/java/com/alipay/sofa/rpc/bootstrap/dubbo/DubboProviderBootstrapTest.java`:
- Around line 219-239: Add an explicit unit test in DubboProviderBootstrapTest
that covers two ServerConfig instances which resolve to the same cache key (same
virtualHost/virtualPort) but differ in non-address fields (e.g.,
setSerialization(...), setThreadpool(...), setThreads(...)); call
dubboProviderBootstrap.getOrCreateProtocolConfig(...) for both and assert the
expected policy (either Assert.assertSame to lock reuse-first behavior or assert
that a conflict/error is raised depending on the intended contract) so the cache
behavior for conflicting non-address fields is explicitly specified and
prevented from regressing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7190a399-9f53-47a1-a495-e81aaaae2e6e
📒 Files selected for processing (3)
bootstrap/bootstrap-dubbo/src/main/java/com/alipay/sofa/rpc/bootstrap/dubbo/DubboProviderBootstrap.javabootstrap/bootstrap-dubbo/src/main/java/com/alipay/sofa/rpc/bootstrap/dubbo/DubboSingleton.javabootstrap/bootstrap-dubbo/src/test/java/com/alipay/sofa/rpc/bootstrap/dubbo/DubboProviderBootstrapTest.java
🚧 Files skipped from review as they are similar to previous changes (1)
- bootstrap/bootstrap-dubbo/src/main/java/com/alipay/sofa/rpc/bootstrap/dubbo/DubboProviderBootstrap.java
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
bootstrap/bootstrap-dubbo/src/main/java/com/alipay/sofa/rpc/bootstrap/dubbo/DubboSingleton.java:57
DubboSingleton.destroyAll()currently only destroys the DubboFrameworkModelbut leavesSERVER_MAP/REGISTRY_MAPpopulated. Since this class registers a global destroy hook, it’s safer to also clear these caches during destroy to avoid holding ontoProtocolConfig/ registry config objects across repeated init/destroy cycles (similar to howServerFactory.destroyAll()clears its cache).
/**
* server cache key --> dubbo.ProtocolConfig
*/
final static ConcurrentMap<String, ProtocolConfig> SERVER_MAP = new ConcurrentHashMap<>();
/**
* sofa.RegistryConfig --> dubbo.RegistryConfig
*/
final static ConcurrentMap<RegistryConfig, org.apache.dubbo.config.RegistryConfig> REGISTRY_MAP = new ConcurrentHashMap<>();
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| String buildServerCacheKey(ServerConfig serverConfig) { | ||
| StringBuilder sb = new StringBuilder(64); | ||
| sb.append(serverConfig.getProtocol()).append(':') | ||
| .append(serverConfig.getHost()).append(':') | ||
| .append(serverConfig.getPort()).append(':') | ||
| .append(resolveHost(serverConfig)).append(':') | ||
| .append(resolvePort(serverConfig)); |
There was a problem hiding this comment.
buildServerCacheKey() concatenates fields with : separators, but hosts (and virtual hosts) can contain : (e.g., IPv6, see NetUtils.toIpString() returning InetAddress.getHostAddress()), which makes the key potentially ambiguous and can cause cache-key collisions between different server configs. Consider using an unambiguous encoding (e.g., length-prefixed segments, escaping, or a small value-object key with proper equals/hashCode, or a delimiter that cannot appear in hosts).
| String buildServerCacheKey(ServerConfig serverConfig) { | |
| StringBuilder sb = new StringBuilder(64); | |
| sb.append(serverConfig.getProtocol()).append(':') | |
| .append(serverConfig.getHost()).append(':') | |
| .append(serverConfig.getPort()).append(':') | |
| .append(resolveHost(serverConfig)).append(':') | |
| .append(resolvePort(serverConfig)); | |
| /** | |
| * Append a single part into the server cache key in an unambiguous way. | |
| * Uses a simple length-prefixed encoding: <length>#<value>. | |
| */ | |
| private void appendKeyPart(StringBuilder sb, Object part) { | |
| String value = part == null ? "" : String.valueOf(part); | |
| sb.append(value.length()).append('#').append(value); | |
| } | |
| String buildServerCacheKey(ServerConfig serverConfig) { | |
| StringBuilder sb = new StringBuilder(64); | |
| appendKeyPart(sb, serverConfig.getProtocol()); | |
| appendKeyPart(sb, serverConfig.getHost()); | |
| appendKeyPart(sb, serverConfig.getPort()); | |
| appendKeyPart(sb, resolveHost(serverConfig)); | |
| appendKeyPart(sb, resolvePort(serverConfig)); |
sunhailin-Leo
changed the title
EvenLjj
left a comment
EvenLjj
left a comment
There was a problem hiding this comment.
Code Review
整体思路和代码组织都不错，getOrCreateProtocolConfig 抽得很干净，buildUrls 也同步用了 resolveHost/resolvePort（这个点很容易漏，赞 👍），测试覆盖度也很高。但缓存 key 的设计有一个值得在合并前修掉的隐患，建议 request changes。CI 全绿 ✅。
⚠️ 严重问题
1. buildServerCacheKey 漏字段，可能导致缓存错乱、配置静默被覆盖
String buildServerCacheKey(ServerConfig serverConfig) {
sb.append(serverConfig.getProtocol()).append(':')
.append(serverConfig.getHost()).append(':')
.append(serverConfig.getPort()).append(':')
.append(resolveHost(serverConfig)).append(':')
.append(resolvePort(serverConfig));
return sb.toString();
}但 copyServerFields 实际复制到 ProtocolConfig 的字段还有：id、accepts、serialization、contextPath、ioThreads、threadPoolType、maxThreads、payload、queues、parameters。
问题场景：用户创建两个 ServerConfig，protocol/host/port/virtualHost/virtualPort 完全相同，但一个 serialization=hessian2、另一个 serialization=protobuf（或者线程池/contextPath 不同），后创建的会直接复用前者的 ProtocolConfig，自己的配置被静默丢弃，且没有任何告警。
改造前用 ServerConfig 实例做 key（默认 Object equals 是引用相等），不同实例必然不会冲突，反而是安全的。这次改成 String key 后引入了真正的 collision 风险。
建议（任选其一）：
- 退回到用
ServerConfig实例做 key（最简单，且符合"同一个 ServerConfig 只 export 一次"的隐含契约） - 把所有进入
copyServerFields的字段都纳入 cache key
2. cache key 里 host:port 与 resolveHost:resolvePort 冗余
既然都用 resolved 后的值写到 ProtocolConfig，key 里只保留 resolved 值就够了。raw host/port 既不能区分 ProtocolConfig 内容，又会让"绑不同网卡 / virtual 到同地址"的场景产生两个内容相同的缓存条目。
建议：如果保留 String key 方案，至少改成 protocol:resolveHost:resolvePort。
⚠️ 中等问题
3. virtualPort == 0 / 负数无校验
private Integer resolvePort(ServerConfig serverConfig) {
return serverConfig.getVirtualPort() != null ? serverConfig.getVirtualPort() : serverConfig.getPort();
}ServerConfig.setPort 自身有 isInvalidPort 校验，但 setVirtualPort(Integer) 没有，resolvePort 这里也没补。如果用户误填 setVirtualPort(0) 或 -1，会被原样写到 ProtocolConfig.port，dubbo 启动行为不可控。
建议：resolvePort 加一层 NetUtils.isInvalidPort(virtualPort) 校验，非法时回退到 port 并打 warn 日志。
4. 测试缺关键回归用例
新增的 7 个用例覆盖正向场景已经很全，但没有覆盖"严重问题 1"：同 host/port/virtualHost/virtualPort 但不同 serialization（或 contextPath / threadPool）的 ServerConfig，应当生成两个不同的 ProtocolConfig。补这个用例可以直接暴露上面的缓存 key 缺陷。
5. DubboSingleton.SERVER_MAP 缺清理机制
key 改为 String 后，每次 export 不同 virtual 配置都会新增一个条目，没有 remove 入口。这是改造前就有的问题，本 PR 没让它变更糟，但建议至少挂个 follow-up issue，特别是动态 export/unExport 频繁的场景。
💡 小建议
6. resolveHost / resolvePort 建议挪到 ServerConfig
"如果有 virtual 就用 virtual" 这是 ServerConfig 自身的领域知识，放在 DubboProviderBootstrap 里，其它协议的 bootstrap（bolt / triple / http / rest）后续想 fix 同样问题时无法复用。建议：
// in ServerConfig
public String getEffectiveHost() { ... }
public Integer getEffectivePort() { ... }所有协议 bootstrap 都能直接用，避免各自重写、各自再次引入 bug。
7. 可见性一致性
copyServerFields 加了 @VisibleForTesting 改 package-private OK，但 getOrCreateProtocolConfig 也是 package-private 却没加注解。建议两者一致（要么都加，要么都不加）。
8. cache key 分隔符
现在用 :，未来如果 contextPath 之类带 /、: 的字段加进来易冲突。建议用一个不会出现在合法字段里的分隔符（如 \u0001），或者直接用复合对象作 key。
9. buildUrls 里 version 硬编码（不阻塞本 PR）
.append(getKeyPairs("version", "1.0"))这不是本 PR 引入的问题，但和 copyProvider 里 providerConfig.getParameter(VERSION) 的处理方式不一致。可以另开 PR fix。
✅ 加分项
buildUrls同步使用resolveHost/resolvePort，没有漏掉对外暴露的另一个入口，作者考虑得很周到。getOrCreateProtocolConfig抽离让copyServers主流程清晰，putIfAbsent并发模式正确。StringUtils.isNotBlank而不是isNotEmpty处理 virtualHost，正确避免了配" "空白字符串的坑。- 测试用例 7 个覆盖度高，包括 blank virtualHost、only virtualHost、only virtualPort、cache key 区分/复用，思路严谨。
setUp里加DubboSingleton.SERVER_MAP.clear()保证测试隔离，细节到位。
总体结论
Request changes。整体方向和实现质量没问题，主要阻塞点是 严重问题 1（cache key 漏字段）——改造前用 ServerConfig 实例做 key 是安全的，这次无意中把保护拆掉了。强烈建议合并前：
- 修复 cache key 漏字段问题
- 补一个"同 host/port 但不同 serialization"的回归测试
- 给
virtualPort加非法值校验
可选改进：
4. 考虑把 resolveHost/resolvePort 提到 ServerConfig，方便其它协议复用
|
🎉 Congratulations @chxbca ! Your PR #1556 has been selected as the winner of the Best Newcomer Award 🎁 in our SOFAStack 8th Anniversary Challenge! We are honored to have you make your first-ever contribution to the SOFAStack community. Your work is a fantastic addition, and we’re thrilled to welcome you to the SOFAStack family! 🚀 📩 Next Step — Claim Your Prize: 📱 WeChat: Zzzz_qy521 Cheers, |
Motivation:
DubboProviderBootstrap.copyServerFields()only copiedhostandport, and ignoredvirtualHost/virtualPortfromServerConfig.This causes the Dubbo provider to publish or register the bound address instead of the expected virtual address in NAT, container, load balancer, or port-mapping scenarios.
Fixes #1119.
Modification:
DubboProviderBootstrap.copyServerFields()to prefervirtualHostoverhostDubboProviderBootstrap.copyServerFields()to prefervirtualPortoverportvirtualHostis blank orvirtualPortis nullResult:
Fixes #1119.
Summary by CodeRabbit
New Features
Tests