Repository navigation
[ISSUE #7652] Support standalone Bootstrap configuration from a local file - #7673
yangjj-iso wants to merge 11 commits into
Conversation
Aias00
left a comment
There was a problem hiding this comment.
Review — Standalone Bootstrap file configuration
Thanks for this — it's a large, well-structured contribution and the security posture of FileModeEnvironmentPostProcessor is genuinely good work. I verified the full head tree against the current master and found two blocking issues, both of which are collisions with other in-flight PRs rather than defects in your logic.
🔴 Blocking 1 — FileConfigurationParser.java:194 will not compile once #7677 lands
shenyu-sync-data-center/shenyu-sync-data-file/src/main/java/org/apache/shenyu/sync/data/file/FileConfigurationParser.java line 194:
RuleData.builder()... .enabled(...).sort(sort).matchMode(matchMode(entry, at)).loged(false).matchRestful(false)On master today, RuleData still declares the field as loged (shenyu-common/src/main/java/org/apache/shenyu/common/dto/convert/rule/RuleData.java:39 — private Boolean loged;), so this compiles. But #7677 (open) renames it to logged across the whole tree. SelectorData was already renamed in v2.5.0 (SelectorData.java:47 — private Boolean logged;), which is why line 173 of your own parser is already correct:
SelectorData.builder()... .logged(false).continued(true)... // line 173 — correctSo the moment #7677 merges, this line breaks the build for the file-sync module. Note the asymmetry is easy to miss because it is not uniform within your own file.
Please rebase onto #7677 (or at minimum change loged → logged and add a note) so the two land in a compilable order. Merging #7673 before #7677 is also fine, but then #7677 must carry the fix — worth calling out in whichever PR merges second.
🔴 Blocking 2 — ApolloDataServiceTest collides with the already-approved #7680
shenyu-sync-data-center/shenyu-sync-data-apollo/src/test/java/org/apache/shenyu/sync/data/apollo/ApolloDataServiceTest.java line 283. You and #7680 are fixing the same master bug in the same two lines by different means.
The underlying defect: AbstractNodeDataSyncService#getDataKeys() returns {"test-plugin", "selector-id", ...} style arrays and AbstractNodeDataSyncService only calls setId(keys[4]) / setName(...) on one of them, leaving getName() null.
- #7680 changes the input key to
shenyu/test-plugin/selector-idand assertsgetId()equalsselector-id. - #7673 keeps the input and asserts
getId()equalstest-proxy.
Both are defensible readings of intent, but they touch identical lines, so this will be a guaranteed merge conflict. Since #7680 is already approved, please rebase onto it and drop your duplicate fix rather than keeping both. If you believe the test-proxy assertion is the more faithful one, that is a separate discussion to have on #7680 — but this PR should not carry a second, divergent fix for the same defect.
✅ What I verified as correct
I went through the whole module against the dependency sources; the design holds up.
FileModeEnvironmentPostProcessor
- Ordering is right:
ConfigDataEnvironmentPostProcessor.ORDER + 1places you afterConfigDataEnvironmentPostProcessorhas populatedshenyu.sync.*fromapplication.yaml, soenvironment.getProperty(key, "false")genuinely sees user configuration rather than the bootstrap default. You are early enough (EnvironmentPostProcessor) that no bean has been created yet, which is what makes the hard fail safe. - The bootstrap-default carve-out (
ws://localhost:9095/websocket) is exactly the right refinement — it distinguishes "user left the shipped default alone" from "user explicitly configured another sync source". Without it, every fresh standalone deployment would be rejected. - Rejecting before overriding to
falseis the correct ordering; theIllegalStateExceptionmessage naming the offending channel is actionable.
FileSyncDataConfiguration
environment.getPropertySources().contains("shenyuFileMode")at lines 66-68 is a good guard against the post-processor not having registered (e.g. when the starter is on the classpath butspring.factoriesregistration was stripped or overridden by a customspring.factories). This avoids a confusing NPE-less misconfiguration.
Filtered plugin subscriber
CommonPluginDataSubscriberholdshandlerMapas an instance field (shenyu-plugin-base/.../CommonPluginDataSubscriber.java:53), so constructing a second filtered subscriber does not corrupt the shared admin-side instance. Correct.- I checked the startup path:
updateCacheDatacallshandlerPluginbeforecachePluginData/notifyPluginData, andsubscribeDataHandler's catch rethrows whileInitialSyncApplication.isActive()is true. YourSmartInitializingSingleton.afterSingletonsInstantiated()runs beforefinishRefresh(), hence before Netty binds — so a bad file fails startup rather than after the port is open. The extra 503 filter is reasonable defence in depth.
FileConfigurationParser strictness
- Rejecting unknown fields, multiple documents, and >1 MiB files; resolving placeholders before inserting into the node (so a placeholder value cannot alter the document's shape); and not echoing values in diagnostics — all good, and the diagnostic messages keep the
atpath which is what makes them usable. - I diffed the documented defaults against
DivideRuleHandleand they line up exactly:timeout= 3000 =Constants.TIME_OUT(shenyu-common/.../Constants.java:317),retry= 3,loadBalance=random,headerMaxSize/requestMaxSize= 0 (no initial value in the DTO, so 0 is the right "unset" default).ContextMappingRuleHandle.addPrefixis genuinely aString(ContextMappingRuleHandle.java:29-33), so requiring a leading/matchesREADME.md:87.
FileConfigurationLoader
InitialSyncApplication.run(this::apply).get(timeout.toMillis(), MILLISECONDS)correctly blocks startup on initial sync, andclose()unsubscribes in reverse dependency order (upstreams → rules → selectors → plugins) with per-entry cleanup isolation. Correct shape.
⭐ E2E infrastructure improvements (this is the part I'd most like to keep)
These are easy to overlook because they are "just CI", but they fix real, recurring pain:
- Local
httpbinfixture — the compose network aliashttpbin.orgplus a healthcheck removes the externalhttpbin.orgdependency. This is directly relevant: while reviewing #7677 I found the e2e storage suite red onendpoint '/anything/xx/yy' not exists, which traced back to external-site flakiness. You've eliminated that class of failure at the source. - RocketMQ consumer hardening —
DefaultLitePullConsumer, unique group and request id,seekToBegin, and toleratingTOPIC_NOT_EXISTare all the right changes for a suite that needs to be deterministic from a cold topic. WaitForHelperTest— replacing a wall-clock-dependent test with a localHttpServer+ latch, and asserting the delayed request actually arrived, turns a timing guess into a deterministic assertion. Much better.- Dropping
mvndfor./mvnwinci.yml— justified by daemon disconnect flakiness.
Summary
Request changes for the two cross-PR collisions above; nothing else I found is blocking. Please rebase onto #7680 and reconcile the RuleData field name with #7677, and I'll re-review.
…inting null-safe (apache#7589)
Integrate apache#7677 and preserve current master changes. Use the renamed builder in the file parser, assert logging defaults, and adapt the new base-plugin tests. The duplicate Apollo selector test fix was dropped when rebasing onto master, which already contains apache#7680.
d611e62 to
1079512
Compare
Fixes #7652.
Bootstrap can start from an external declarative YAML/JSON file and serve HTTP routes without Admin, an Admin database or an external configuration center. The opt-in
shenyu-spring-boot-starter-sync-data-fileis included in Bootstrap and reuses the existing configuration DTOs, subscribers, plugin handlers and request chain.Merge dependency: #7677 must land first. This branch includes its
RuleData.loggedrename at6ffa0d8107b182d3ff03c8082500cc0671b5b649, and the file parser uses.logged(false). Selector and rule logging defaults are asserted by the parser test. The rename's SQL/Admin changes remain attributable to #7677 and will leave this PR's diff when that dependency lands.The branch is rebased onto master
f00c15712, which already includes #7680. The competing Apollo selector-deletion test commit has been dropped;ApolloDataServiceTest.javais identical to master. Integration preserves the latest Admin dashboard/CORS changes and adapts the new base-plugin tests to the renamed setter.Behavior:
Initial scope is static HTTP routing through global/divide/uri/nettyHttpClient/response, with optional contextPath. Metadata/auth entries and other declared plugins are rejected. Hot reload and multi-source merging are outside this change.
The existing shared E2E repairs remain: a healthchecked local HTTP upstream for storage/cluster scenarios, RocketMQ log consumption with unique request/group IDs and topic/queue readiness, Spring Cloud route readiness using the actual controller response, and a local HTTP server for wait-helper tests. PR builds use the Maven wrapper.
Validation on Java 17:
git diff --checkpass.