feat(xray): merge user-provided API services instead of overwriting#60
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughAdds routing-rule and balancer-management capabilities: new protobuf messages/RPCs, a RoutingBackend interface implemented by the xray backend via a gRPC RoutingServiceClient, REST and RPC controller endpoints exposing these operations, middleware gating, and xray API-service sanitization with default observatory injection. ChangesRouting Feature
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant RESTService as REST/RPC Service
participant RoutingBackend
participant XrayHandler
participant RoutingServiceClient as Xray gRPC RoutingService
Client->>RESTService: AddRoutingRule / ListRoutingRules / TestRoute request
RESTService->>RoutingBackend: asRoutingBackend(backend)
RoutingBackend-->>RESTService: RoutingBackend or Unimplemented error
RESTService->>XrayHandler: delegate call (via *Xray routing methods)
XrayHandler->>RoutingServiceClient: gRPC call (AddRule/ListRule/TestRoute/...)
RoutingServiceClient-->>XrayHandler: gRPC response or error
XrayHandler-->>RESTService: converted common.* response or error
RESTService-->>Client: protobuf response or mapped HTTP/gRPC error
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
CP3 spec-compliance review fixes: - Register the 6 RoutingService RPCs in backendMethods so they get the same controlling-client + started gating as their stats.go neighbors. - Return codes.InvalidArgument for malformed AddRoutingRule JSON (was Unknown).
xray's Router.AddRule unwraps the TypedMessage and requires a *router.Config; a bare *router.RoutingRule is rejected with "config type error". Send the built config (carrying the single parsed rule) instead, and assert the *router.Config instance type in the unit test. Fixes AddRoutingRule on both gRPC and REST.
Add REST handlers for the 6 routing operations under /routing, registered in the gated private group (controlling-client + started middleware), reusing the RoutingBackend capability gate and the gRPC-code-to-HTTP error mapping used by the stats handlers. Enable RoutingService in the test config and add unit (capability gate) + integration (list/add/remove, malformed->400, route wiring) coverage.
The proto bool defaulted to false, and xray treats AddRule(shouldAppend=false) as "reset all rules + balancers". Renamed the field to should_reset so the wire default (false) is the non-destructive append; handlers pass !should_reset to the backend. A direct caller that omits the field no longer wipes the routing table. The REST integration test now asserts a default add appends (both rules survive).
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
controller/rest/routing.go (1)
36-157: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRepeated backend-resolution/error-handling boilerplate across all six handlers.
Each handler repeats the identical
rb, err := s.routingBackend(); if err != nil { writeRoutingError(w, err); return }pattern. Extracting a small wrapper would reduce duplication and the chance of divergence as more routing endpoints are added.♻️ Example wrapper
+func (s *Service) withRoutingBackend(w http.ResponseWriter, fn func(rb backend.RoutingBackend) error) { + rb, err := s.routingBackend() + if err != nil { + writeRoutingError(w, err) + return + } + if err := fn(rb); err != nil { + writeRoutingError(w, err) + } +} + func (s *Service) ListRoutingRules(w http.ResponseWriter, r *http.Request) { - rb, err := s.routingBackend() - if err != nil { - writeRoutingError(w, err) - return - } - - resp, err := rb.ListRoutingRules(r.Context()) - if err != nil { - writeRoutingError(w, err) - return - } - - common.SendProtoResponse(w, resp) + s.withRoutingBackend(w, func(rb backend.RoutingBackend) error { + resp, err := rb.ListRoutingRules(r.Context()) + if err != nil { + return err + } + common.SendProtoResponse(w, resp) + return nil + }) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@controller/rest/routing.go` around lines 36 - 157, The six routing handlers in Service all duplicate the same backend lookup and error handling before doing their request-specific work. Extract that repeated `s.routingBackend()` plus `writeRoutingError`/return flow into a small shared helper or wrapper around `ListRoutingRules`, `GetBalancerInfo`, `TestRoute`, `AddRoutingRule`, `RemoveRoutingRule`, and `OverrideBalancerTarget`, then have each handler call it so only the per-endpoint body remains. Keep the existing behavior for backend resolution failures and preserve the current proto request parsing and response sending inside each handler.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/xray/routing.go`:
- Around line 25-34: The snapshot in routingHandler() is not sufficient to
protect the delegated RPC path because Shutdown() can clear RoutingServiceClient
while backend/xray/api/routing.go is still dereferencing it. Update
Xray.routingHandler() and the routing API call path so the same synchronization
covers both the guard and the client dereference, either by holding x.mu.RLock
through the delegated call or by making handler/client shutdown use the same
lock consistently.
In `@controller/rest/routing.go`:
- Around line 52-94: The routing endpoints in GetBalancerInfo and TestRoute are
reading a request body while still being used as GET handlers, which is
incompatible with many clients and intermediaries. Update the route registration
and handler expectations so these endpoints use POST instead, keeping the
existing body parsing in common.ReadProtoBody and the backend calls to
rb.GetBalancerInfo and rb.TestRoute unchanged. Ensure any client-facing docs or
route definitions match the new method.
In `@controller/rpc/routing.go`:
- Around line 55-61: Update Service.TestRoute so it matches the other routing
handlers by wrapping errors from routingBackend/TestRoute with
common.InterceptNotFound. Right now TestRoute returns backend errors directly,
which leaves “not found” cases as Unknown; apply the same error handling pattern
used by the other RPC methods in this service so backend missing-route failures
surface as gRPC NotFound.
---
Nitpick comments:
In `@controller/rest/routing.go`:
- Around line 36-157: The six routing handlers in Service all duplicate the same
backend lookup and error handling before doing their request-specific work.
Extract that repeated `s.routingBackend()` plus `writeRoutingError`/return flow
into a small shared helper or wrapper around `ListRoutingRules`,
`GetBalancerInfo`, `TestRoute`, `AddRoutingRule`, `RemoveRoutingRule`, and
`OverrideBalancerTarget`, then have each handler call it so only the
per-endpoint body remains. Keep the existing behavior for backend resolution
failures and preserve the current proto request parsing and response sending
inside each handler.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d1d4e0de-6c18-4212-bfda-39925416cce2
⛔ Files ignored due to path filters (2)
common/service.pb.gois excluded by!**/*.pb.gocommon/service_grpc.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (19)
.gitignorebackend/backend.gobackend/xray/api/base.gobackend/xray/api/routing.gobackend/xray/api/routing_test.gobackend/xray/config.gobackend/xray/config.jsonbackend/xray/config_test.gobackend/xray/routing.gobackend/xray/routing_test.gocommon/service.protocontroller/rest/rest_test.gocontroller/rest/routing.gocontroller/rest/routing_test.gocontroller/rest/service.gocontroller/rpc/middleware.gocontroller/rpc/middleware_test.gocontroller/rpc/routing.gocontroller/rpc/routing_test.go
| func (x *Xray) routingHandler() (handler, error) { | ||
| x.mu.RLock() | ||
| h := x.handler | ||
| started := x.core != nil && x.core.Started() | ||
| x.mu.RUnlock() | ||
| if !started || h == nil { | ||
| return nil, errors.New("xray not started") | ||
| } | ||
| return h, nil | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== backend/xray/routing.go ==\n'
sed -n '1,220p' backend/xray/routing.go
printf '\n== backend/xray/api/base.go ==\n'
sed -n '1,220p' backend/xray/api/base.go
printf '\n== backend/xray/api/routing.go ==\n'
sed -n '1,260p' backend/xray/api/routing.go
printf '\n== search for lifecycle and locking around Xray ==\n'
rg -n "type Xray|func .*Stop|func .*Restart|RoutingServiceClient|Close\(|handler =|mu\.|Started\(" backend/xray -g '!**/*_test.go'Repository: PasarGuard/node
Length of output: 13606
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== backend/xray/xray.go ==\n'
sed -n '1,240p' backend/xray/xray.go
printf '\n== backend/xray/core.go (Stop/Restart area) ==\n'
sed -n '260,400p' backend/xray/core.go
printf '\n== backend/xray/latency.go ==\n'
sed -n '1,120p' backend/xray/latency.goRepository: PasarGuard/node
Length of output: 9658
TOCTOU race between the guard and the RPC call.
routingHandler() only protects the snapshot; Shutdown() can nil RoutingServiceClient while backend/xray/api/routing.go is still doing client := *x.RoutingServiceClient, which can panic the process. Keep the read lock across the delegated call, or synchronize handler close with the same lock.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/xray/routing.go` around lines 25 - 34, The snapshot in
routingHandler() is not sufficient to protect the delegated RPC path because
Shutdown() can clear RoutingServiceClient while backend/xray/api/routing.go is
still dereferencing it. Update Xray.routingHandler() and the routing API call
path so the same synchronization covers both the guard and the client
dereference, either by holding x.mu.RLock through the delegated call or by
making handler/client shutdown use the same lock consistently.
| func (s *Service) GetBalancerInfo(w http.ResponseWriter, r *http.Request) { | ||
| rb, err := s.routingBackend() | ||
| if err != nil { | ||
| writeRoutingError(w, err) | ||
| return | ||
| } | ||
|
|
||
| var request common.BalancerInfoRequest | ||
| if err := common.ReadProtoBody(r.Body, &request); err != nil { | ||
| http.Error(w, err.Error(), http.StatusBadRequest) | ||
| return | ||
| } | ||
|
|
||
| resp, err := rb.GetBalancerInfo(r.Context(), request.GetTag()) | ||
| if err != nil { | ||
| writeRoutingError(w, err) | ||
| return | ||
| } | ||
|
|
||
| common.SendProtoResponse(w, resp) | ||
| } | ||
|
|
||
| func (s *Service) TestRoute(w http.ResponseWriter, r *http.Request) { | ||
| rb, err := s.routingBackend() | ||
| if err != nil { | ||
| writeRoutingError(w, err) | ||
| return | ||
| } | ||
|
|
||
| var request common.TestRouteRequest | ||
| if err := common.ReadProtoBody(r.Body, &request); err != nil { | ||
| http.Error(w, err.Error(), http.StatusBadRequest) | ||
| return | ||
| } | ||
|
|
||
| resp, err := rb.TestRoute(r.Context(), &request) | ||
| if err != nil { | ||
| writeRoutingError(w, err) | ||
| return | ||
| } | ||
|
|
||
| common.SendProtoResponse(w, resp) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does the HTTP/1.1 spec (RFC 9110) allow request bodies on GET requests, and do reverse proxies/CDNs commonly strip them?
💡 Result:
RFC 9110, the current standard for HTTP semantics, does not strictly prohibit request bodies in GET requests, but it strongly discourages them [1][2]. The specification notes that while HTTP message framing is independent of the method, content in a GET request has no defined semantics and cannot alter the meaning or target of the request [1][2]. RFC 9110 explicitly states that a client "SHOULD NOT generate content in a GET request" unless it is communicating directly with an origin server that is known to support and require such a request for a specific purpose [1][2]. Furthermore, it warns that many implementations may reject GET requests containing a body, potentially closing the connection due to security concerns, such as the risk of request smuggling [1][2]. In practice, reverse proxies, CDNs, and various HTTP frameworks frequently strip, ignore, or reject GET requests that include a body [3][4]. Because HTTP methods like GET are historically and widely assumed to be "safe" and "idempotent" without request bodies, many intermediaries (load balancers, WAFs, and proxy servers like Nginx or Zuul) are configured to automatically discard or drop the body of a GET request [2][5][6]. Consequently, using a body with GET is not interoperable on the open web and is likely to result in broken communication or request rejection when passing through intermediaries [2][4].
Citations:
- 1: https://www.http-probe.com/docs/body/get-with-cl-body/
- 2: https://lists.w3.org/Archives/Public/public-webapps-github/2023Sep/0580.html
- 3: https://www.baeldung.com/cs/http-get-with-body
- 4: https://medium.com/@uday410ry/cant-use-get-with-a-requestbody-fd64dcbd7be5
- 5: https://stackoverflow.com/questions/74368064/can-cloudflare-or-nginx-strip-the-body-of-an-inbound-request
- 6: Zuul: Request body is dropped if HTTP method is GET spring-cloud/spring-cloud-netflix#1964
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant files first
git ls-files controller/rest | sed -n '1,120p'
echo
echo "== routing.go outline =="
ast-grep outline controller/rest/routing.go --view expanded || true
echo
echo "== service.go outline =="
ast-grep outline controller/rest/service.go --view expanded || true
echo
echo "== Search for the two handlers and their registrations =="
rg -n "GetBalancerInfo|TestRoute|routingGroup\.(Get|Post|Put)\(" controller/rest -n -A3 -B3
echo
echo "== Read the relevant slices =="
sed -n '1,180p' controller/rest/service.go
echo
sed -n '1,180p' controller/rest/routing.goRepository: PasarGuard/node
Length of output: 11461
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== search for routing endpoints across repo =="
rg -n "/routing/(balancer|test)|GetBalancerInfo|TestRoute|BalancerInfoRequest|TestRouteRequest" .
echo
echo "== search for API docs mentioning GET bodies or these endpoints =="
rg -n "GET.*body|request body|/routing/balancer|/routing/test|balancer.*test route|test route" README* docs* .github* controller common backend 2>/dev/null || trueRepository: PasarGuard/node
Length of output: 12210
Move these routing endpoints off GET controller/rest/service.go:61-63
GetBalancerInfo and TestRoute both parse a request body while being registered as GET. That breaks standard fetch()/Workers clients and can be dropped by intermediaries; switch them to POST or move the inputs into query/path parameters.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@controller/rest/routing.go` around lines 52 - 94, The routing endpoints in
GetBalancerInfo and TestRoute are reading a request body while still being used
as GET handlers, which is incompatible with many clients and intermediaries.
Update the route registration and handler expectations so these endpoints use
POST instead, keeping the existing body parsing in common.ReadProtoBody and the
backend calls to rb.GetBalancerInfo and rb.TestRoute unchanged. Ensure any
client-facing docs or route definitions match the new method.
| func (s *Service) TestRoute(ctx context.Context, request *common.TestRouteRequest) (*common.RouteResult, error) { | ||
| rb, err := s.routingBackend() | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| return rb.TestRoute(ctx, request) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the routing service and backend interfaces.
git ls-files | rg '(^|/)controller/rpc/routing\.go$|(^|/)backend.*\.go$|(^|/)common.*\.go$|(^|/)routing.*\.go$'
echo '--- controller/rpc/routing.go ---'
cat -n controller/rpc/routing.go | sed -n '1,220p'
echo '--- search for InterceptNotFound and TestRoute implementations ---'
rg -n 'InterceptNotFound|TestRoute\(' controller backend common -g '!**/vendor/**'
echo '--- routing backend interface / implementations ---'
rg -n 'type .*RoutingBackend|interface .*RoutingBackend|func .*TestRoute\(' . -g '!**/vendor/**'
echo '--- proto/doc references for TestRoute / error semantics ---'
rg -n 'TestRoute|not found|ShouldReset|Append' . -g '!**/vendor/**'Repository: PasarGuard/node
Length of output: 18486
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- controller/rest/routing.go ---'
cat -n controller/rest/routing.go | sed -n '1,180p'
echo '--- backend/backend.go ---'
cat -n backend/backend.go | sed -n '1,120p'
echo '--- backend/xray/routing.go ---'
cat -n backend/xray/routing.go | sed -n '1,120p'
echo '--- backend/xray/api/routing.go ---'
cat -n backend/xray/api/routing.go | sed -n '1,220p'
echo '--- common/helper.go ---'
cat -n common/helper.go | sed -n '90,130p'Repository: PasarGuard/node
Length of output: 16810
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- RPC routing tests ---'
cat -n controller/rpc/routing_test.go | sed -n '1,260p'
echo '--- backend/xray routing tests ---'
cat -n backend/xray/routing_test.go | sed -n '1,220p'
echo '--- search for plain "not found." errors around routing/backends ---'
rg -n '"not found\.|not found\.' backend controller common -g '!**/vendor/**'
echo '--- search for status.NotFound in routing backend path ---'
rg -n 'codes\.NotFound|status\.Error\(codes\.NotFound|status\.Errorf\(codes\.NotFound' backend controller common -g '!**/vendor/**'Repository: PasarGuard/node
Length of output: 3196
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- controller/rpc/rpc_test.go (relevant section) ---'
cat -n controller/rpc/rpc_test.go | sed -n '150,240p'
echo '--- controller/rpc/middleware.go (status handling around routing methods) ---'
cat -n controller/rpc/middleware.go | sed -n '220,320p'
echo '--- search for TestRoute-specific error assertions in controller/rpc ---'
rg -n 'TestRoute|NotFound|InterceptNotFound' controller/rpc -g '!**/vendor/**'Repository: PasarGuard/node
Length of output: 8442
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- uses of InterceptNotFound in tests and handlers ---'
rg -n 'InterceptNotFound\(' . -g '!**/vendor/**' -g '*_test.go'
echo '--- routing-related tests around error mapping ---'
rg -n 'NotFound|Unknown|TestRoute|AddRoutingRule|RemoveRoutingRule|OverrideBalancerTarget' controller/rpc controller/rest backend/xray -g '*_test.go' -g '!**/vendor/**'
echo '--- any tests for common.InterceptNotFound ---'
rg -n 'InterceptNotFound|not found\.' common -g '*_test.go' -g '!**/vendor/**'Repository: PasarGuard/node
Length of output: 209
Wrap TestRoute errors with common.InterceptNotFound controller/rpc/routing.go:55-60 — this handler should match the other routing RPCs (and the REST endpoint) so backend "not found." errors surface as gRPC NotFound instead of Unknown.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@controller/rpc/routing.go` around lines 55 - 61, Update Service.TestRoute so
it matches the other routing handlers by wrapping errors from
routingBackend/TestRoute with common.InterceptNotFound. Right now TestRoute
returns backend errors directly, which leaves “not found” cases as Unknown;
apply the same error handling pattern used by the other RPC methods in this
service so backend missing-route failures surface as gRPC NotFound.
…ce panic XrayHandler.Close nilled the service client fields, so an RPC racing Shutdown (guard passed, then Close ran) dereferenced a nil client and panicked — and the gRPC server chain has no recovery interceptor, so that took the process down. Closing the grpc.ClientConn is enough: in-flight and later calls now fail cleanly with a connection-closing error. Also covers the same latent race in the stats/handler client paths. Addresses CodeRabbit review on PasarGuard#60 (TOCTOU in routingHandler); fixed at the client-lifetime level instead of holding x.mu across delegated network calls, which would block Shutdown behind in-flight RPCs.
GetBalancerInfo and TestRoute parse protobuf request bodies but were registered as GET. RFC 9110 discourages GET bodies, fetch()-style clients cannot send them, and intermediaries may drop them. Switch both to POST while the API is still unreleased; handlers are unchanged. Addresses CodeRabbit review on PasarGuard#60. The pre-existing /stats GET-with-body endpoints are left as-is (released surface, out of scope here).
TestRoute was the only routing RPC not wrapping backend errors with common.InterceptNotFound, so an identical error could map to NotFound on REST (writeRoutingError applies it) but Unknown on gRPC. Behaviorally a no-op for today's xray errors, but keeps the two transports consistent. Addresses CodeRabbit review on PasarGuard#60.
get_balancer_info and test_route send protobuf request bodies, which GET does not reliably support through clients and intermediaries. Switch both to POST in lockstep with the node's route change (PasarGuard/node#60).
|
Addressed in df7817c, bb9a461, afaa792 (+ lockstep bridge change in 1. TOCTOU race in 2. GET with request body on 3. 4. Nitpick: extract a |
* feat(proto): add RoutingService messages + RPCs and regenerate stubs Mirror the node's service.proto: 9 routing messages and 6 NodeService RPCs (field names/numbers identical for wire compatibility). Regenerated with the pinned toolchain. * feat: add routing RPC wrappers over gRPC and REST Expose the 6 RoutingService operations on the PasarGuardNode interface (list/get-balancer/test-route/add/remove/override). gRPC calls the RoutingService client; REST calls the node's /routing endpoints. Routing is xray-only, so a non-xray backend surfaces NodeAPIError(501). Documented in the README. * fix: default add_routing_rule to append (safe should_reset) Mirror the node proto rename should_append -> should_reset so the wire default (false) appends rather than resetting the router (xray clears all rules + balancers when shouldAppend is false). The bridge method now takes should_reset=False; pass should_reset=True to replace all rules. * fix: serialize routing mutations under the node lock add_routing_rule / remove_routing_rule / override_balancer_target (gRPC + REST) now run under _node_lock like the other state-changing ops (start/stop/sync), so a routing change can't race a concurrent core restart. Read-only routing methods (list/get_balancer/test_route) stay lock-free, consistent with the other reads. Addresses CodeRabbit review on #16. * fix: use POST for routing endpoints that carry request bodies get_balancer_info and test_route send protobuf request bodies, which GET does not reliably support through clients and intermediaries. Switch both to POST in lockstep with the node's route change (PasarGuard/node#60).
2 participants
What
Makes operator-selected Xray gRPC API services both honored and usable by
the node. Three parts:
Config.ApplyAPIused to hard-code the APIservices to
HandlerService/LoggerService/StatsServiceand discardanything set on
api.services. It now merges the required services withvalid user-provided extras.
ObservatoryServiceno longer crashes the core. Enabling it now injects afunctional default
observatoryapp when the operator configured none.RoutingServiceis now usable. Adds sixNodeServicerouting RPCs (overboth gRPC and REST) so clients (the panel, via the bridge) can use it.
Why
Operators want to enable optional Xray API services (
RoutingService,ObservatoryService,ReflectionService) from their core config. Three gapsexisted: required services were silently dropped if the operator set
api.services; enablingObservatoryServicecrashed xray on startup; and evenwhen
RoutingServicewas enabled the node had no RPCs to use it.HandlerService/StatsService(users + traffic) andLoggerServicestaymandatory.
How
API services merge
sanitizeAPIServices: starts from the required set, then appends valid userextras canonicalized against an allowlist mirrored from xray-core
(
infra/conf/api.go), deduped case-insensitively, and sorted for deterministicoutput.
backend, consistent with the rest of
ApplyAPI's defensive normalization.api.listenis intentionally left empty: the API is exposed only via theloopback, source-restricted
API_INBOUND. Honoring a user-supplied listen wouldopen a second, unguarded gRPC entry point.
TestSanitizeAPIServicespins the expected names so drift breaks the test.ObservatoryService crash fix
ObservatoryServiceis enabled; with noobservatory/burstObservatoryapp thecore exits (
core: not all dependencies are resolved).ApplyAPInow injects afunctional default observatory (probing eligible outbounds) only when the service
is enabled and the operator configured neither app. Operator-supplied config is
respected; default (no optional services) behavior is unchanged.
GetOutboundsLatencythen returns data via the existing
/debug/varsreader.RoutingService RPCs
NodeService, served over both gRPC and REST:ListRoutingRules,GetBalancerInfo,TestRoute,AddRoutingRule,RemoveRoutingRule,OverrideBalancerTarget.RoutingBackendcapability interface implemented only by*xray.Xray; otherbackends (WireGuard) return
codes.Unimplemented/ HTTP 501. Includes the gRPCclient wiring and node⇄xray-core type mapping;
AddRoutingRuleaccepts a JSONrule (same shape as a
routing.rules[]entry).should_reset(defaultfalse=append, keeping existing rules);
trueresets the router (clears all rules +balancers) before adding. xray's underlying flag treats false as "reset", so
the wire default is inverted to the non-destructive behavior.
Intentionally excluded
SubscribeRoutingStats— xray registersRoutingServicewith a nil statschannel, so it always errors.
ObservatoryServicegRPC client (latency is already served via/debug/vars)and no
ReflectionServicewrapper (it's grpc server reflection for tooling).Tests
backend/xray/config_test.go: services merge / dedupe / drop-unknown; noapisection still yields exactly the required services; observatory injected when
enabled (eligible + empty-selector), respected when operator-supplied, nil when
disabled.
backend/xray/api/routing_test.go: node⇄xray routing type mapping andJSON-rule →
*router.Configconversion (incl.InvalidArgumenton malformed JSON).controller/rpc+controller/rest: capability gate returnsUnimplemented/ 501;REST integration test covers list/add/remove, malformed → 400, and that a default
add appends (existing rules survive).
make testpasses locally; the observatory crash fix is verified live on the nodevia
xray run -test(broken config → exit 23; injected-observatory config →Configuration OK).Related
Summary by CodeRabbit
New Features
Bug Fixes
Chores