支持规则同步到nacos - #3635
Conversation
|
zhangyunlong seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
BitDive Runtime Review - PR #3635PR: #3635 SummaryThis PR rewires all five rule-management controllers (Flow V2, Degrade, ParamFlow, Authority, System) from The dominant symptom is silent data loss. The Nacos server address is hardcoded to Beyond the cross-cutting data-loss issue, each controller has its own behavior change worth flagging:
Latency on every endpoint jumps 14×–450× (e.g. Fix before merge:
Runtime Verification Matrix
* Stable relative to the other controllers — the same Nacos-side silent-data-loss pattern still applies. How to read BitDive evidenceBehavior Δ in the matrix and the red/green
GitHub strips Review scope and validation coverage
Out of scope for runtime review: CI/CD assets; frontend Change #1 - FlowControllerV2 silent bean hijackThe V2 flow controller keeps the same Behavior contract# GET /v2/flow/rules?app=sentinel-dashboard — runtime contract
- HTTP 200 OK {success:true, data:[]}
+ HTTP 200 OK {success:true, data:[]} (silent — Nacos unreachable)
- bean: @Component('flowRuleDefaultProvider') = FlowRuleApiProvider
+ bean: @Component('flowRuleDefaultProvider') = FlowRuleNacosProvider
- call tree: FlowRuleApiProvider.getRules → SentinelApiClient.fetchFlowRuleOfMachine → fetchRules → fetchItems → fetchItemsAsync → executeCommand (6 nested, 14 child calls)
+ call tree: FlowRuleNacosProvider.getRules → ConfigService.getConfig (3s timeout, empty data, 2 child calls)
- write: InMemoryRuleRepositoryAdapter.saveAll([])
+ write: InMemoryRuleRepositoryAdapter.saveAll([]) (retained — empty list both sides)
- data source: Sentinel client library over HTTP (172.17.0.2:8719)
+ data source: Nacos ConfigService (private LAN IP 192.168.234.59:8848)
- time: 221ms
+ time: 3070ms (14x slowdown)
# first divergence: FlowRuleNacosProvider.getRules()
- apiQueryMachineRules → FlowRuleApiProvider.getRules → SentinelApiClient chain
+ apiQueryMachineRules → FlowRuleNacosProvider.getRules → ConfigService.getConfig (3s timeout → empty)Scenario matrix
Trace evidence
Contract delta
Key trace deltaFirst meaningful divergence:
Change #2 - DegradeController API contract breakThe degrade controller query method drops Behavior contract# GET /degrade/rules.json?app=sentinel-dashboard&ip=127.0.0.1&port=8719 — runtime contract
- HTTP 200 OK {success:true, data:[]}
+ HTTP 200 OK {success:true, data:[]} (silent — Nacos unreachable)
- method args: [app='sentinel-dashboard', ip='172.17.0.2', port=8719] (3 args)
+ method args: [app='sentinel-dashboard'] (1 arg — ip/port dropped from signature)
- validation: AppManagement.isValidMachineOfApp(app, ip, port)
+ validation: REMOVED (no ip/port to validate)
- call tree: SentinelApiClient.fetchDegradeRuleOfMachine → executeCommand (direct HTTP)
+ call tree: DegradeRuleNacosProvider.getRules → ConfigService.getConfig (3s timeout)
- write: InMemoryRuleRepositoryAdapter.saveAll([])
+ write: InMemoryRuleRepositoryAdapter.saveAll([]) (retained under different parent path)
- mutation: publishRules returns boolean
+ mutation: publishRules throws Exception (code-only)
- time: 8.8ms
+ time: 3010ms (342x slowdown)
# first divergence: DegradeRuleNacosProvider.getRules()Scenario matrix
Trace evidence
Contract delta
Key trace deltaFirst meaningful divergence:
Change #3 - ParamFlowRuleController major simplification + write addedThe most aggressive refactor: method signature drops Behavior contract# GET /paramFlow/rules?app=sentinel-dashboard&ip=127.0.0.1&port=8719 — runtime contract
- HTTP 200 OK {success:true, data:[]}
+ HTTP 200 OK {success:true, data:[]} (silent — Nacos unreachable)
- method args: [app, ip, port] (3 args)
+ method args: [app] (1 arg — ip/port dropped)
- pre-call: checkIfSupported (Sentinel client version check)
+ pre-call: REMOVED
- async: CompletableFuture.supplyAsync → SentinelApiClient chain
+ sync: ParamFlowRuleNacosProvider.getRules → ConfigService.getConfig (blocking 3s)
- write: no saveAll on query path
+ write: InMemoryRuleRepositoryAdapter.saveAll([]) ADDED
- call tree: 11 child calls
+ call tree: 2 child calls
- time: 11.7ms
+ time: 3009ms (257x slowdown)
# first divergence: InMemoryRuleRepositoryAdapter.saveAll() added in AFTERScenario matrix
Trace evidence
Contract delta
Key trace deltaFirst meaningful divergence:
Change #4 - AuthorityRuleController saveAll removedThe only controller where Behavior contract# GET /authority/rules?app=sentinel-dashboard&ip=127.0.0.1&port=8719 — runtime contract
- HTTP 200 OK {success:true, data:[]}
+ HTTP 200 OK {success:true, data:[]} (silent — Nacos unreachable)
- method args: [app, ip, port] (3 args — signature UNCHANGED, validation retained)
+ method args: [app, ip, port] (3 args — signature UNCHANGED)
- validation: checkBasicParams → isValidMachineOfApp (retained)
+ validation: checkBasicParams → isValidMachineOfApp (retained)
- write: InMemoryRuleRepositoryAdapter.saveAll([]) called
+ write: saveAll REMOVED from query path
- call tree: SentinelApiClient.fetchAuthorityRulesOfMachine chain (14 child calls)
+ call tree: AuthorityRuleNacosProvider.getRules (4 child calls)
- time: 6.68ms
+ time: 3013ms (450x slowdown)
# first divergence: AppManagement.getDetailApp() (environmental drift — hostname/version/heartbeat)
# structural change: AuthorityRuleNacosProvider.getRules replacing SentinelApiClient chainBaseline note: The first divergence flagged by the trace comparison is Scenario matrix
Trace evidence
Contract delta
Key trace deltaFirst code divergence (after environmental drift):
Change #5 - SystemController cleanest migrationThe best-implemented migration of the five: Behavior contract# GET /system/rules.json?app=sentinel-dashboard&ip=127.0.0.1&port=8719 — runtime contract
- HTTP 200 OK {success:true, data:[]}
+ HTTP 200 OK {success:true, data:[]} (silent — Nacos unreachable)
- method args: [app, ip, port] (signature UNCHANGED)
+ method args: [app, ip, port] (signature UNCHANGED)
- validation: checkBasicParams → isValidMachineOfApp (retained)
+ validation: same (retained)
- provider: SentinelApiClient.fetchSystemRuleOfMachine
+ provider: SystemRuleNacosProvider.getRules → ConfigService.getConfig
- post-process: sets ip/port on each returned rule entity (code-only)
+ post-process: same code path (code-only — data=[] means no entities post-processed at runtime)
- write: InMemoryRuleRepositoryAdapter.saveAll([])
+ write: same (retained)
- time: 8.5ms
+ time: 3011ms (354x slowdown)
# first divergence: AppManagement.getDetailApp() (environmental drift — hostname/version/heartbeat)
# structural change: SystemRuleNacosProvider.getRules replacing SentinelApiClient chainScenario matrix
Trace evidence
Contract delta
Key trace deltaFirst code divergence (after environmental drift):
Follow-Ups
RecommendationRequest changes. The Nacos integration pattern is sound and |
|
CLA Not Signed The Contributor License Agreement (CLA) check is currently pending on this PR ( @zackzhangCN please sign the CLA via the CLA assistant badge in the comment above, or visit https://cla-assistant.io/alibaba/Sentinel. Once signed, the Automated check by github-manager-bot |
oss-sentinel-ai
left a comment
oss-sentinel-ai
left a comment
There was a problem hiding this comment.
Summary
This PR rewires the five dashboard rule controllers (Flow V2, Degrade, ParamFlow, Authority, System) from direct Sentinel client HTTP calls to Nacos ConfigService, adding ten provider/publisher/config classes and promoting sentinel-datasource-nacos to compile scope. The direction is reasonable, but in the current state the integration ships broken defaults and several silent-failure paths, so it needs another round of changes. Note that the earlier runtime review on this PR has already confirmed the blocking behaviors (silent empty-rule responses from the hardcoded private Nacos address, silent bean swap via @Component("flowRuleDefaultProvider") name reuse, dropped ip/port parameters, and inconsistent repository.saveAll handling across controllers); the static pass below adds issues not covered there.
Findings
- [Critical] sentinel-dashboard/src/main/resources/application.properties:27-30 — Nacos credentials (
username=nacos/password=nacos) and a private LAN address are committed to the repository and become the silent default for every deployment. - [Warning] rule/nacos/*Publisher.java (FlowRuleNacosPublisher.java:52) —
publishConfig(...)return value ignored in all five publishers; afalseresult means the rules were not persisted while the dashboard still reports success. - [Warning] rule/nacos/FlowRuleNacosProvider.java:45-49 — unreachable Nacos returns null after the 3s timeout and is converted to an empty rule list with
success=true; "unreachable" is indistinguishable from "no rules" (mechanism behind the runtime-observed silent data loss). - [Warning] rule/nacos/FlowRuleNacosProvider.java:52 — entities built via
fromFlowRule(app, null, null, rule)have no id/ip/port; ids are only assigned when the controller re-saves viarepository.saveAll, which this PR removes from AuthorityRuleController, so authority rules fetched from Nacos cannot be updated/deleted via the/{id}endpoints. - [Warning] sentinel-dashboard/pom.xml:103 — scope promotion plus unconditional
NacosConfigmakes a Nacos server a hard requirement for every dashboard deployment, with no opt-out to the previous behavior. - [Info] The minified build artifact
sentinel-dashboard/src/main/webapp/resources/dist/js/app.jsis committed; if the frontend change is intentional, please include rebuild instructions or the regenerated source artifacts so reviewers can reproduce it. - [Info] No unit/integration tests for the ten new Nacos classes (also flagged in the runtime review).
Suggestions
- Externalize all Nacos connection settings (env vars or external config), remove committed credentials, and fail loudly at startup or query time when the server is unreachable instead of returning empty data as success.
- Check the boolean result of
publishConfigin every publisher and surface failures to the caller. - Restore (or deliberately version) the
ip/portquery contract inDegradeController/ParamFlowRuleController, and keep thesaveAll/entity-id handling consistent across all five controllers. - Avoid reusing the
flowRuleDefaultProviderbean name for the Nacos provider; wire the new provider explicitly so the switch is visible. - Gate the Nacos wiring behind a configuration property so deployments without Nacos keep working as before.
Note
The CLA is not signed yet on this PR (see the CLA reminder comment), so it cannot be approved regardless of review outcome — please sign it via the CLA assistant link first.
Automated review by github-manager-bot
| spring.cloud.nacos.config.server-addr=192.168.234.59:8848 | ||
| spring.cloud.nacos.config.namespace=public | ||
| spring.cloud.nacos.config.username=nacos | ||
| spring.cloud.nacos.config.password=nacos No newline at end of file |
There was a problem hiding this comment.
[Critical] Nacos credentials (username/password) are committed to the repository, along with a private LAN address that becomes the silent default for every dashboard deployment. Even if these are lab defaults, shipped credentials are a security anti-pattern. Suggestion: keep only placeholders here (or remove the block), resolve real values from environment variables / external config at deployment time, and fail fast at startup when the address is still a placeholder instead of silently timing out on every rule query.
| List<FlowRule> flowRules = rules.stream() | ||
| .map(FlowRuleEntity::toRule) | ||
| .collect(Collectors.toList()); | ||
| configService.publishConfig(app + NacosConfigUtil.FLOW_DATA_ID_POSTFIX, |
There was a problem hiding this comment.
[Warning] The boolean result of configService.publishConfig(...) is ignored. Nacos returns false when the publish does not take effect (e.g. cas/permission issues) without throwing, so the dashboard would report success to the user while the rule set was not persisted — silent rule loss on the mutation path. This pattern is the same in all five new publishers (Flow/Degrade/ParamFlow/Authority/System); please check the return value and throw/log on failure.
| @Override | ||
| public List<FlowRuleEntity> getRules(String appName) throws Exception { | ||
| String rules = configService.getConfig(appName + NacosConfigUtil.FLOW_DATA_ID_POSTFIX, | ||
| NacosConfigUtil.GROUP_ID, 3000); |
There was a problem hiding this comment.
[Warning] When Nacos is unreachable, getConfig returns null after the 3s timeout and this code path converts it into an empty rule list with success=true — 'Nacos unreachable' becomes indistinguishable from 'no rules configured' (this is exactly the silent-data-loss behavior observed in the runtime review). Consider distinguishing the two, e.g. by checking config existence first or surfacing client errors, so operators get a loud failure instead of a healthy-looking empty list.
| return new ArrayList<>(); | ||
| } | ||
| List<FlowRule> flowRules = converter.convert(rules); | ||
| return flowRules.stream() |
There was a problem hiding this comment.
[Warning] FlowRuleEntity.fromFlowRule(appName, null, null, rule) leaves id, ip, port, gmtCreate/gmtModified unset. The in-memory repository assigns an id only when the controller re-saves the fetched list (repository.saveAll), but this PR removes that call from AuthorityRuleController — so authority entities fetched from Nacos have no id and cannot be updated/deleted through the /{id} endpoints. Please keep entity identity handling consistent across all five controllers (and document the ip/port semantics now that rules are app-scoped instead of machine-scoped).
| </dependency> | ||
|
|
||
| <!-- for Nacos rule publisher sample --> | ||
| <!-- for Nacos rule publisher --> |
There was a problem hiding this comment.
[Warning] Promoting sentinel-datasource-nacos from test to compile scope plus the unconditional @Configuration NacosConfig means every dashboard deployment now hard-depends on a Nacos server, with no switch to keep the previous direct-API behavior for the five rewired controllers. Consider gating the Nacos wiring behind a property (e.g. @ConditionalOnProperty) so deployments without Nacos are not forced into it.
4 participants
支持规则同步到nacos。
包括：流控，熔断，热点，系统，授权