Skip to content

Fixes #7356: Reconcile websocket data sync across standalone admin nodes sharing one database - #7358

Merged
Aias00 merged 15 commits into
apache:masterfrom
BobSong-dev:fix/7356-websocket-standalone-admin-reconciliation
Oct 1, 2026
Merged

Aias00 merged 15 commits into
apache:masterfrom
BobSong-dev:fix/7356-websocket-standalone-admin-reconciliation

Conversation

@BobSong-dev

@BobSong-dev BobSong-dev commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor

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

  • Reconcile PLUGIN, SELECTOR and RULE only. These groups now remove cached rows belonging to the snapshot namespace through subscriber deletion callbacks, then apply the new rows. An empty snapshot removes stale rows without clearing other namespaces. Validate both the envelope and row namespaces before mutation.
  • Keep APP_AUTH, META_DATA, PROXY_SELECTOR, DISCOVER_UPSTREAM and AI_PROXY_API_KEY on their existing sync paths; they are not reconciled by this task. Gateways still follow current master semantics for ordinary unmarked REFRESH/MYSELF messages, including empty refreshes.
  • Default reconciliation to disabled (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.
  • Serialize each row once, sort serialized rows for a stable digest and reuse them in the outgoing snapshot; unchanged row ordering does not cause another push.
  • Recursively redact API key fields from sync-message logs, including nested proxy keys. Do not log malformed messages verbatim.
  • Preserve upstream full-sync/readiness behavior and include only the independently authorized RocketMQ test synchronization fix.

Verification

  • Local: 63 targeted Admin tests passed; 30 plugin-base tests and 82 websocket tests passed. After adding the nonempty cross-namespace replacement case, all 13 WebsocketDataHandlerTest cases passed in a separate rerun. Zero failures/errors/skips in these test runs; Checkstyle passed.
  • Local: Apache RAT passed for Admin, common, plugin-base, sync-api and websocket modules; git diff --check origin/master passed.
  • GitHub CI: head 6f1608134 is 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.
  • Not run locally: Docker-based integration/e2e or full-project tests.

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.

@Aias00 Aias00 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Aias00 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

BobSong-dev added 8 commits September 30, 2026 17:13
…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 Aias00 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Aias00 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed after the new commits — all three points I raised are addressed, and I appreciate the shape of the fix.

  1. Namespace scoping: refreshPluginDataNamespace / refreshSelectorDataNamespace / refreshRuleDataNamespace now filter BaseDataCache by namespaceId and unsubscribe only the matching rows, instead of calling cleanSelectorData()-style global clears. AbstractDataHandler.handleSnapshot threads the namespace through, and the default doSnapshot fails loudly rather than silently half-applying.

  2. 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 as fullSnapshot. That was my main correctness worry — a "complete snapshot" that cannot delete can never converge.

  3. Defaults and cost: Reconciliation.enabled now defaults to false, 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.

@Aias00
Aias00 merged commit a448dcd into apache:master Oct 1, 2026
39 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] WebSocket sync does not converge across standalone Admin nodes sharing a database

2 participants