Repository navigation
Fixes #7356: Reconcile websocket data sync across standalone admin nodes sharing one database - #7358
Conversation
Aias00
left a comment
There was a problem hiding this comment.
Requesting changes on the current head 8579b0c7ff6712ae9a9d53b7ee6acd04e78af5ec.
[HIGH] Empty REFRESH/MYSELF snapshots still do not clear stale gateway cache, so the PR does not satisfy the #7356 deletion/empty-group acceptance criteria. WebsocketDataReconciler can now send data: [] for a changed group (shenyu-admin/src/main/java/org/apache/shenyu/admin/listener/websocket/WebsocketDataReconciler.java:202-211), but the bootstrap websocket side drops empty payloads before dispatch (shenyu-sync-data-center/shenyu-sync-data-websocket/src/main/java/org/apache/shenyu/plugin/sync/data/websocket/handler/AbstractDataHandler.java:63-67). Even if that guard is removed, the common plugin subscriber also returns on empty self-refresh for plugin/selector/rule (shenyu-plugin/shenyu-plugin-base/src/main/java/org/apache/shenyu/plugin/base/cache/CommonPluginDataSubscriber.java:127-132, 155-160, 182-187). This means deleting the final rule/selector/metadata/etc. on one standalone Admin node can make the reconciler emit an empty refresh, while the local Bootstrap attached to another Admin keeps stale cache until reconnect/manual full sync. Please make empty REFRESH/MYSELF carry enough scope to clear the affected namespace/group cache and add bootstrap-side regression coverage for the stale-cache deletion case.
[HIGH] The reconciler can leak AI proxy API keys into admin INFO logs. It reconciles AI_PROXY_API_KEY (WebsocketDataReconciler.java:248-249), whose payload contains proxyApiKey and realApiKey (shenyu-common/src/main/java/org/apache/shenyu/common/dto/ProxyApiKeyData.java:25-27; built in AiProxyApiKeyServiceImpl.java:357-360). WebsocketCollector.send logs the full message at INFO (shenyu-admin/src/main/java/org/apache/shenyu/admin/listener/websocket/WebsocketCollector.java:303), but maskSensitive only masks top-level apiKey/realApiKey fields (WebsocketCollector.java:360-378), not nested data[].realApiKey / data[].proxyApiKey. Please avoid logging full sync messages or add recursive redaction with tests for nested AI proxy key payloads.
[MEDIUM] The digest used to decide whether to push is order-sensitive (WebsocketDataReconciler.java:202-204), but several loaders used by reconciliation do not enforce deterministic ordering. Examples include app auth (AppAuthServiceImpl#listAllByNamespaceId via app-auth-sqlmap.xml), metadata (MetaDataServiceImpl#listAllByNamespaceId via meta-data-sqlmap.xml), and AI proxy keys (AiProxyApiKeyServiceImpl#listAllByNamespaceId). An unchanged DB snapshot can therefore be hashed differently if row order changes, causing periodic false refresh pushes. Please sort snapshots by stable keys before hashing/sending or add explicit ORDER BY to every reconciler query, with a regression test that reordering unchanged data does not trigger a second push.
Local verification: git diff --check origin/master...HEAD passed; ./mvnw -pl shenyu-admin -Dtest=WebsocketDataReconcilerTest test passed; ./mvnw -pl shenyu-sync-data-center/shenyu-sync-data-websocket -Dtest=PluginDataHandlerTest,SelectorDataHandlerTest,RuleDataHandlerTest,MetaDataHandlerTest test passed and currently confirms empty payloads are treated as no-op. Current GitHub CI also has failing/cancelled IT/e2e jobs on this head.
Aias00
left a comment
There was a problem hiding this comment.
Requesting changes on the current head. The previous blockers still apply: empty REFRESH/MYSELF snapshots do not clear stale bootstrap caches, AI proxy key payloads can still be logged without recursive redaction, and the digest remains order-sensitive for loaders without deterministic ordering. CI is also failing on this head (IT/e2e/k8s-examples). Please fix those issues and rerun CI before this can be approved.
…standalone-admin-reconciliation # Conflicts: # shenyu-admin/src/main/java/org/apache/shenyu/admin/service/AiProxyApiKeyService.java # shenyu-admin/src/main/java/org/apache/shenyu/admin/service/AppAuthService.java # shenyu-admin/src/main/java/org/apache/shenyu/admin/service/DiscoveryUpstreamService.java # shenyu-admin/src/main/java/org/apache/shenyu/admin/service/MetaDataService.java # shenyu-admin/src/main/java/org/apache/shenyu/admin/service/impl/MetaDataServiceImpl.java
Aias00
left a comment
There was a problem hiding this comment.
This is a sensible approach to a real problem: standalone admin nodes sharing one database cannot see each other's Spring events or WebSocket sessions. The digest-per-(namespace, group) design, advancing the cursor only after a successful load and push, skipping cluster followers, jittering the initial delay, and scheduleWithFixedDelay to avoid overlapping runs are all good choices, and WebsocketDataReconcilerTest gives solid coverage.
Requesting changes on three points:
1. The snapshot wipes the gateway cache globally, with no namespace scoping
doSnapshot for PLUGIN / SELECTOR / RULE calls refreshSelectorDataAll() / refreshRuleDataAll() / refreshPluginDataAll(), which reach BaseDataCache.getInstance().cleanSelectorData() and friends. I checked BaseDataCache: it is a flat ConcurrentMap<String, ...> with no namespace partitioning at all, and cleanSelectorData() clears everything (only cleanSelectorDataSelf(list) is scoped to a list).
So pushing a snapshot for namespace A erases namespace B's cached configuration, and B is only restored when B's own digest changes and B receives a push — which may not happen for a long time. Even in a single-namespace deployment this is a full flush of the cache whenever a group changes. The snapshot needs to be scoped to the namespace it describes, or BaseDataCache needs namespace-aware cleaning.
2. Five of the eight groups have no delete semantics, but are still marked fullSnapshot
Only PLUGIN, SELECTOR and RULE override doSnapshot. APP_AUTH, META_DATA, PROXY_SELECTOR, DISCOVER_UPSTREAM and AI_PROXY_API_KEY inherit the default doSnapshot -> doRefresh(dataList), which only refreshes the rows present and never removes rows missing from the payload. Since the reconciler pushes all eight groups with fullSnapshot = true, deletions in those five groups can never converge — stale entries stay on the gateway indefinitely. Please either implement a real replace for those groups, or restrict reconciliation to the groups that support it and document the boundary.
3. Defaults and digest cost
Reconciliation.enabled defaults to true with a 60s interval, so every standalone admin polls all eight groups from the database every minute by default — including single-admin deployments that gain nothing from it. Consider defaulting to false, or at least documenting the added load.
Separately, the digest computation serializes every row twice: .sorted(Comparator.comparing(GsonUtils.getInstance()::toJson)) recomputes the key on each comparison (roughly O(n log n) Gson calls), and then .map(GsonUtils.getInstance()::toJson).sorted() serializes everything again. Mapping to strings once and sorting those would halve the work.
Non-blocking: this PR and #7360 both modify DividePluginTest.java (+18/-1) and add the same LoggingRuleSyncTest.java (+79) in the rocketmq e2e case; whichever merges first will force a rebase on the other.
Aias00
left a comment
There was a problem hiding this comment.
Re-reviewed after the new commits — all three points I raised are addressed, and I appreciate the shape of the fix.
-
Namespace scoping:
refreshPluginDataNamespace/refreshSelectorDataNamespace/refreshRuleDataNamespacenow filterBaseDataCachebynamespaceIdand unsubscribe only the matching rows, instead of callingcleanSelectorData()-style global clears.AbstractDataHandler.handleSnapshotthreads the namespace through, and the defaultdoSnapshotfails loudly rather than silently half-applying. -
Group scope: the reconciler now iterates
List.of(PLUGIN, SELECTOR, RULE)rather than all eight groups, so groups without authoritative replace semantics are no longer pushed asfullSnapshot. That was my main correctness worry — a "complete snapshot" that cannot delete can never converge. -
Defaults and cost:
Reconciliation.enablednow defaults tofalse, and the digest maps each row to JSON once before sorting instead of re-serializing inside the comparator.
One documentation ask: the PR description still says eight configuration groups; please update it to reflect that reconciliation covers plugin, selector and rule only, and note that custom PluginDataSubscriber implementations must implement the three namespace methods (they throw UnsupportedOperationException by default) before enabling this.
Fixes #7356
Background
Standalone Admin nodes sharing one database do not share Spring events or websocket sessions. Opt-in reconciliation lets each node update its connected gateways when another node changes the database.
Changes
shenyu.sync.websocket.reconciliation.enabled=false). Shared-database standalone deployments must opt in; only active namespaces are polled, with a default 60-second interval and initial jitter.Verification
git diff --check origin/masterpassed.6f1608134is pushed; current-head checks are pending, not yet verified green. The preceding head failed storage e2e with an external HTTPBin 502 response and an endpoint-not-found assertion. Retried using a content-identical commit; no assertions or production behavior were relaxed.Compatibility
Custom PluginDataSubscriber implementations must implement namespace-scoped replacement before enabling this feature; the default rejects unsupported snapshots rather than clearing the cache globally. Upgrade Admin and gateways together before enabling reconciliation: older gateways do not implement the namespace-marked snapshot protocol. Gateways are configured for a single connection namespace; this change scopes snapshot deletion rather than introducing multi-namespace routing or changing existing cache keys.