fix: respect virtual host and port in dubbo provider bootstrap#1556
fix: respect virtual host and port in dubbo provider bootstrap#1556chxbca wants to merge 4 commits intosofastack:masterfrom
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
| String url = dubboProviderBootstrap.buildUrls().get(0).toString(); | ||
| Assert.assertTrue(url.startsWith("dubbo://10.0.0.1:80/" + DemoService.class.getName())); | ||
| Assert.assertTrue(url.contains("?uniqueId=")); | ||
| } |
There was a problem hiding this comment.
uniqueId assertion is order-sensitive and can become flaky.
Lines 165 and 183 require uniqueId to be the first query parameter (?uniqueId=). If parameter order changes, the test fails even when uniqueId exists.
Proposed test hardening
- Assert.assertTrue(url.contains("?uniqueId="));
+ Assert.assertTrue(url.contains("uniqueId="));
...
- Assert.assertTrue(url.contains("?uniqueId="));
+ Assert.assertTrue(url.contains("uniqueId="));Also applies to: 181-184
🤖 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 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.
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