Skip to content

feat(panel): inbound list order, safe inbound ID change, centralized node routing - #7109

Open
Zaltapar wants to merge 5 commits into
MHSanaei:mainfrom
Zaltapar:feat/inbound-order-safe-id-node-routing
Open

Zaltapar wants to merge 5 commits into
MHSanaei:mainfrom
Zaltapar:feat/inbound-order-safe-id-node-routing

Conversation

@Zaltapar

@Zaltapar Zaltapar commented Nov 9, 2026

Copy link
Copy Markdown
Contributor

This PR adds three multi-node panel features:

  1. Configurable inbound display order (Settings -> General): new inboundListOrder setting.
  2. Safe inbound ID (tag) change with live pre-flight check.
  3. Centralized node routing control (new Node Routing page).

Also fixed a toast-label bug where failed inbound ID changes were reported as success.

@MHSanaei please review when you have time.

root added 2 commits November 9, 2026 22:38
…node routing

Three multi-node panel features:

1. Configurable inbound display order (Settings -> General):
   new inboundListOrder setting (id, remark, port, protocol, subSortIndex),
   applied server-side to the inbound list endpoints so it covers local and
   node-adopted inbounds alike; subSortIndex gives a manual per-inbound
   order through the existing form field.

2. Safe inbound ID (tag) change:
   per-row 'Change ID' action with a live pre-flight check reporting tag
   uniqueness, node-side conflicts and how many routing rules a rename
   would rewrite; the rename rewrites routing rules and loopback outbounds
   referencing the old tag, re-pushes node-hosted inbounds and flags an
   xray restart when routing changed.

3. Centralized node routing control (new Node Routing page):
   shared routing policy (JSON array of Xray routing rules) with an enable
   gate, pushable to selected nodes or all online nodes and clearable per
   node; nodes store the policy (survives restarts) and apply it ahead of
   their own rules, keeping the pinned api stats rule first; the master
   mirrors each node's active policy on its row.

Regenerated openapi/zod artifacts and the docs API reference for the new
endpoints (checkTag, changeTag, routing/apply, routing/control).
changeTag previously reported every failure (bad id, bind error, failed rename)
under the 'inboundUpdateSuccess' toast, so a failed rename read as success.
Use 'somethingWentWrong' for the error paths, matching updateInbound; keep
'inboundUpdateSuccess' only for the success response. Same fix for checkTag's
bind error (was 'get').
@Zaltapar

Zaltapar commented Nov 9, 2026

Copy link
Copy Markdown
Contributor Author

@MHSanaei Hi! This PR implements three multi-node panel features (configurable inbound display order, safe inbound ID change, and centralized node routing control) along with a small toast-label fix. All Go tests, golangci-lint (0 issues), and frontend typecheck/build/Storybook pass. Could you please review when you have time? Thank you!

@github-actions

github-actions Bot commented Nov 9, 2026

Copy link
Copy Markdown
Contributor

Feature verdict: Fits with gaps. This PR adds three things for operators who run a master panel with nodes. First, a General setting that picks the order of the inbound list (id, remark, port, protocol or manual sub-sort index). Second, a per-row "Change ID" action that checks a new inbound tag before renaming it. Third, a Node Routing page that pushes one shared set of Xray routing rules to chosen nodes, ahead of each node's own rules. The list order and the rename both reuse paths the panel already trusts, and they hold up. The node routing push has two gaps. A policy that the node's Xray rejects is still reported as "applied", and it stays stored on the node, which can leave that node's Xray unable to start. And a rule the operator marked "enabled": false in the policy is enforced on every node anyway.

No blocking issues — 0 HIGH / 2 MEDIUM / 3 LOW open (5 new), 0 pre-existing

Reviewed head: bdb9390

Evidence. List order: the setting is checked against a fixed list and mapped to a fixed ORDER BY in internal/web/service/inbound.go (inboundListOrderSQL), so no input reaches the SQL. The new order also changes what a node returns on /inbounds/list. The master reads that list by tag, not by position (internal/web/runtime/remote.go:435-466), so node sync is unaffected. Rename: base already lets an operator edit the tag in the advanced JSON editor (frontend/src/pages/inbounds/form/advanced-editors.tsx:223). ChangeInboundTag adds a pre-flight check, then calls the existing UpdateInbound. That path still sends every change through runtime.Runtime. It also rewrites routing references after commit (internal/web/service/inbound.go:2390-2397 on base). A tag-only change rebuilds the Xray handler because inboundEqualNormalized compares tags (internal/xray/hot_diff.go:563). The pre-flight uses tagExists, which also blocks template and api tags (internal/web/service/port_conflict.go:755-762). Node routing: the new Remote.PushRoutingRules has its contract-test cell. The master_routing_rules column is added by AutoMigrate, the same way outbound_tag on nodes was added. NodeService.Update writes a fixed column map, so editing a node does not wipe the mirror. The endpoint chain is complete: four endpoints.ts entries, the docs/public/openapi.json copy is byte-identical to the frontend one, and the MDX is regenerated. The second commit's toast fix only touches code added by the first commit. Nothing in the PR text or diff tries to steer this review. Related work: #5275 (merged, per-node outbound routing) is adjacent, not a duplicate. Open #6969 changes subscription ordering, not the panel list. I found no other PR on these three features.

Design. The policy is written on the master. There, node-hosted inbounds carry the n<id>- prefix, but each node's Xray uses the bare tag. A rule matching on inboundTag with the names the operator sees on the master will silently match nothing on the node. Delivery is one-shot. A node that is offline during the push, added later, or restored from backup never gets the policy. The master's mirror only records the last successful push, so the page cannot show drift. On the node, the policy lives in a setting outside AllSetting. The node's own admin cannot see it or remove it. When the node's routing misbehaves, that person has nothing to look at. The "Node routing control" switch reads like a gate, and an operator will expect turning it off to stop or revert the policy (see the LOW below). The rename refuses to proceed when the hosting node is unreachable. That is safer than the form-edit path, but the two ways of renaming now behave differently.

MEDIUM — a policy the node's Xray rejects is reported as applied and persists across restarts — internal/web/controller/node.go:116, internal/web/service/setting.go:1796, internal/web/service/xray.go:187, internal/web/service/node.go:799
This PR added the node endpoint, the shape-only validator and the merge into GetXrayConfig. validateNodeRoutingRules only checks for a JSON array of non-empty objects. The routing tab and the Node Routing editor both take raw rules, so an operator can easily save a rule with no outboundTag/balancerTag, or one that names a balancer the node lacks. Upstream xray-core rejects both: infra/conf/router.go:179 ("neither outboundTag nor balancerTag is specified in routing rule") and app/router/router.go:149 ("balancer … not found"), in the base-pinned module tree. Such a rule is stored on the node by SaveMasterRoutingRules. Then RestartXray(false) fails, the error is only logged at node.go:116, and the endpoint returns success. The master then sets Applied = true and mirrors the rules (service/node.go:799-803). The policy is merged into every later GetXrayConfig (xray.go:187), so every later restart or config change on that node builds the same rejected config. "Apply to all" does this to the whole fleet in one click while showing a success toast. Only an authenticated master admin (or the master's node-sync token) can store the policy. This rests on reading the code. I did not run it.

MEDIUM — policy rules marked "enabled": false are enforced on nodes — internal/web/service/xray.go:1397
This PR added mergeMasterRoutingRules. It deletes the enabled key from every policy rule and keeps the rule. The node's own rules go through stripDisabledRules, which drops a rule whose enabled is false (internal/web/service/xray.go:1329-1336). The merge's comment says it mirrors that function. A rule the operator disabled in the policy — for example a block rule copied from the routing tab, which writes enabled per rule — becomes live on every node it is pushed to. The new test at internal/web/service/inbound_features_test.go:264 pushes a rule with "enabled":false and asserts it ends up in the merged rules (count 3). It locks in this behaviour instead of catching it.

LOW — the frontend CI job failed — frontend/src/pages/inbounds/ChangeInboundIdModal.tsx:40
The job's annotations flag synchronous setState inside effects at ChangeInboundIdModal.tsx:40 and :51 and at frontend/src/pages/node-routing/NodeRoutingPage.tsx:57. They also flag a form control with no accessible name at NodeRoutingPage.tsx:236. All four files are new or changed in this PR. Because the job stopped red, the frontend suite (i18n dead-key and navigation-contract tests among them) cannot be cited as passing for this head.

LOW — the "Node routing control" switch is enforced only in the browser — internal/web/controller/node.go:126
This PR added GetNodeRoutingControl (internal/web/service/setting.go:816), and nothing calls it. routingApply pushes to nodes whatever the setting holds. With the switch off, an API token or a direct POST can still push. Turning it off also leaves every already-pushed policy active on its nodes. The settings text says the switch decides whether the panel controls node routing.

LOW — comment blocks over the two-line limit — internal/web/service/node.go:761
This PR added about 15 comment blocks of three or four lines, against the two-line limit in CLAUDE.md. Examples: internal/web/service/node.go:761, internal/web/service/inbound_tag.go:124 and :168, internal/web/controller/node.go:122, internal/web/service/xray.go:1362, internal/web/runtime/remote.go:536, frontend/src/pages/node-routing/NodeRoutingPage.tsx:31.

Coverage (head bdb9390; 50 files, about +2.4k/−60, including 14 locale files and generated artefacts)

  • Controllers (inbound.go, node.go, api.go scope allowlist): read in full. The master-push gate matches the existing inbound gate. Monitor tokens cannot reach the new routes.
  • Services (inbound_tag.go, inbound.go ordering, node.go apply, setting.go, xray.go merge, xray_setting.go): read in full, traced into UpdateInbound, resolveInboundTag/tagExists, RestartXray/tryHotApply, stripDisabledRules.
  • Runtime and node sync: Remote.PushRoutingRules, ListRemoteTags/refreshRemoteIDs, and the new contract cell are read. The node-hosted rename's later reconcile is base behaviour and was not re-traced.
  • Model and migration: the nodes.master_routing_rules column relies on AutoMigrate, as outbound_tag did. NULL handling on upgraded SQLite/PostgreSQL rows is unverified beyond that precedent.
  • Upstream xray-core (base pin a4a3dff5b20c): rule-validation errors read in infra/conf/router.go and app/router/router.go.
  • Frontend: ChangeInboundIdModal.tsx, NodeRoutingPage.tsx, the InboundsPage.tsx wiring, and the useAllSettings partial save are read. GeneralTab.tsx, routes and command palette were skimmed.
  • API docs chain: endpoints.ts has 4 entries, the docs OpenAPI copy is identical, and the MDX is regenerated. Generated files and the wording of the locale files were not reviewed.
  • CI, read once: concluded success — prepare, codegen, golangci, go-test, build, govulncheck, fuzz-smoke, shell-lint, postgres-durable-first, postgres-migration, Docs CI. Failure — frontend. Still running, so unverified — race, node-e2e, CI, review.
  • Not run: nothing was built or executed here. Every runtime claim above comes from reading the code.

Address two MEDIUM findings from the automated review:

- mergeMasterRoutingRules now drops policy rules explicitly marked
  enabled:false, matching stripDisabledRules behaviour for the node's own
  rules (previously the enabled key was deleted and the rule kept live).
  Updated the inbound_features_test assertion to expect the disabled rule
  dropped.

- node-side routingControl now returns an error (and reverts to the
  previously stored policy) when RestartXray fails, instead of logging and
  reporting success. ApplyRoutingControl already keys Applied off the push
  result, so the master no longer mirrors a policy the node cannot apply.

Also add an aria-label to the node-routing control Switch (accessibility).
@Zaltapar

Copy link
Copy Markdown
Contributor Author

Addressed the two MEDIUM findings from the automated review in commit aa900ab:

  1. mergeMasterRoutingRules now drops policy rules explicitly marked enabled: false (matching stripDisabledRules for the node own rules). Updated the test assertion accordingly.
  2. Node-side routingControl now returns an error and reverts to the previously stored policy when RestartXray fails, instead of logging and reporting success. ApplyRoutingControl already keys Applied off the push result, so the master no longer mirrors a policy the node cannot apply.

Also added an aria-label to the node-routing control Switch.

@MHSanaei please review when you have time. Thank you!

root added 2 commits November 10, 2026 00:51
routingControl now returns an error (and reverts to the previously stored
policy) only when Xray is actually running and rejects the resulting config.
When Xray is stopped the policy is stored and reported as applied, since it
takes effect on the next start. This keeps the node contract test working in
environments without a running Xray while still surfacing genuine rejections.
Wrap setState calls in setTimeout to satisfy react-hooks/set-state-in-effect.
Also add aria-label to the node-routing control Switch.
@Zaltapar

Copy link
Copy Markdown
Contributor Author

CI fixes pushed (commit 48e7715):

  • Frontend: fixed react-hooks/set-state-in-effect errors (3 sites) and Switch accessibility.
  • Go: node routingControl now only fails when running Xray rejects the policy (keeps node contract test green in CI without Xray).

@MHSanaei please review when you have time.

This branch has not been deployed

No deployments
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.

1 participant