Repository navigation
[ISSUE #7652] Support standalone Bootstrap configuration from a local file - #7673
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
|
@Aias00 Both original coordination items are addressed: the file parser uses logged(false) on the included #7677 dependency, and ApolloDataServiceTest matches the version now on master from #7680. I also resolved the current CI/generated-dashboard conflicts against master in c989867, preserving the upstream bundle and retry helper. All 74 selected RuleData and file-mode tests pass locally. Could you please re-review this head and confirm the intended ordering with #7677? The rename dependency remains included; this does not claim that #7677 itself has merged. |
|
Thanks for the update. This PR currently shows merge conflicts with |
Resolve the dashboard modify/delete conflict by retaining the current upstream bundle and index. Preserve the RuleData rename dependency and standalone file configuration changes.
…ertions The initialization signal can reach block() before doFinally releases the handshake request. Await the existing idle barrier before measuring the expired-deadline call, preserving the no-request and zero-pending assertions.
|
This PR still has a merge conflict with Please rebase onto the latest Once the conflict is cleared (and |
Re-review: both previous blocking items are resolved, but
|
| File | Line | Current code |
|---|---|---|
shenyu-plugin/shenyu-plugin-security/shenyu-plugin-key-auth/src/test/java/org/apache/shenyu/plugin/key/auth/KeyAuthAuthenticationHttpTest.java |
151 | rule.setLoged(false); |
shenyu-plugin/shenyu-plugin-security/shenyu-plugin-key-auth/src/test/java/org/apache/shenyu/plugin/key/auth/KeyAuthAuthenticationTest.java |
311 | rule.setLoged(false); |
shenyu-spring-boot-starter/shenyu-spring-boot-starter-plugin/shenyu-spring-boot-starter-plugin-agent-gateway/src/test/java/org/apache/shenyu/springboot/starter/plugin/agent/gateway/KeyAuthMcpTestFixture.java |
310 | .loged(false) on the RuleData builder |
The build stops at shenyu-plugin-key-auth, so the third one has not surfaced in CI yet but will fail the same way once the first two are fixed. Please update all three (setLoged → setLogged, builder loged → logged) and push so pr_build can go green.
⚠️ Please coordinate the merge order with #7677
#7677 ("rename misspelled loged column/field to logged") is still open and currently CONFLICTING. This PR now carries the same rename, so the two will collide in RuleDO/SelectorDO/RuleDTO/SelectorDTO/RuleData, the MyBatis maps and the SQL scripts.
One of them should land first and the other should drop its copy — please agree on the order on the relevant issue rather than merging both. If #7677 lands first, this PR only needs to drop the rename hunks; if this PR lands first, #7677 becomes redundant and should be closed.
Nothing else I reviewed is blocking: FileModeEnvironmentPostProcessor, FileSyncDataConfiguration and FileConfigurationLoader are unchanged in substance from the version I looked at before and still hold up.
Aias00
left a comment
There was a problem hiding this comment.
Re-review: Approved
Thank you for addressing the review comments in commit 076c1f137a (test(file-sync): complete RuleData logging API rename after master sync).
All three call sites identified in the previous review have been properly updated:
KeyAuthAuthenticationHttpTest.java:151:rule.setLogged(false);KeyAuthAuthenticationTest.java:311:rule.setLogged(false);KeyAuthMcpTestFixture.java:310:RuleData.builder()...logged(false)...
CI check results:
pr_buildpassed in 8m47s.- All 53 checks succeeded with 0 failures.
The standalone bootstrap file configuration feature is complete, well tested, and compliant with all project standards. Approved!
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 itsRuleData.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 masterf00c15712, 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: - Validate the complete versioned document before publication: explicit plugin enablement, deterministic identities, references, ordering, matching conditions, rule handles and static HTTP origins. Reject unknown fields, duplicate mapping keys, unsupported metadata/auth entries and unavailable plugins/handlers without echoing configuration values. - Establish exclusive file ownership before clients are created. Suppress the shipped Admin WebSocket URL, heartbeat, WebSocket readiness and external plugin loader; reject other sync sources and explicitly enabled local API/registration/alert writers. Undeclared plugins stay disabled. File mode is disabled by default. - Apply plugins, selectors, rules and upstreams before the server starts; await asynchronous handler work with a bounded timeout. Abort startup on failure, gate traffic until application succeeds, and remove owned configuration on shutdown/failure. - Document the schema, environment placeholders, mounted-file/container launch and switching back to Admin mode. The documented sample is exercised by tests. 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: - 150 selected tests pass, with zero failures/errors/skips: the complete file parser/loader/environment/real-HTTP suites, Apollo data service, RuleData, base-plugin matching, local plugin controller and common plugin subscriber. - The real-HTTP suite starts from the stock Bootstrap settings, forwards to a real upstream, rejects unmatched/local-management requests, checks disabled Admin integrations, restarts with stable identities and fails startup on invalid configuration/handler failure. - The complete 161-module Admin + Bootstrap reactor packages successfully, including test compilation; test execution is skipped for this packaging check. - Checkstyle, Apache RAT andgit diff --checkpass. - Container E2E/native database execution is left to CI; this local follow-up does not claim those suites passed.sh ./mvnw -B -ntp -pl shenyu-spring-boot-starter/shenyu-spring-boot-starter-sync-data-center/shenyu-spring-boot-starter-sync-data-file,shenyu-sync-data-center/shenyu-sync-data-apollo -am test \ '-Dtest=FileConfiguration*Test,FileMode*Test,ApolloDataServiceTest,RuleDataTest,AbstractShenyuPluginTest,LocalPluginControllerTest,CommonPluginDataSubscriberTest' \ -Dsurefire.failIfNoSpecifiedTests=false -DskipRemoteResources=true \ -Djacoco.skip=true -Dmaven.javadoc.skip=true ./mvnw -B -ntp -pl shenyu-admin,shenyu-bootstrap -am package \ -DskipTests -DskipRemoteResources=true -Djacoco.skip=true -Dmaven.javadoc.skip=true ./mvnw -B -ntp -pl shenyu-admin,shenyu-bootstrap -am apache-rat:check -DskipRemoteResources=trueValidation after the October 19 synchronization
Local Java validation used Temurin 21.0.10 through the repository Maven wrapper.
Maintainer review follow-up — October 20
Complete the three missed RuleData API updates identified in review: both key-auth authentication tests now call
setLogged(false), and the MCP key-auth fixture uses.logged(false). A whole-tree Java search finds no remaining calls tosetLoged,getLogedor thelogedbuilder method. Historical SQL migration names and JSON compatibility aliases remain intentional.Validation on Microsoft OpenJDK 17.0.18 through the Maven wrapper: all 44 affected reactor modules compile, and 210 selected tests pass with zero failures/errors/skips. This includes both formerly failing key-auth test classes, key-auth and principal MCP HTTP/security tests, starter wiring, RuleData compatibility, and the complete file parser/loader/environment/HTTP suites. Checkstyle, Apache RAT and
git diff --checkpass.The branch is mergeable with current master. The previously documented merge dependency remains: #7677 should land first; then refresh this branch so its shared rename changes leave this PR's diff. This follow-up does not claim that the new GitHub CI run has completed.