feat(route): aggregate routes across services when --service-id is omitted#47
Merged
Conversation
…itted API7 EE's /apisix/admin/routes endpoint requires `service_id` under access-token auth, which made `a7 route list -g default` unusable without iterating services manually. List every service in the gateway group and merge their per-service route fetches client-side, dedupe by route ID, and add SERVICE_ID to the table output so the unfiltered view stays meaningful. Passing --service-id keeps the original single-call path. Lifts the existing aggregator out of configutil into a shared pkg/listutil package so `config sync/dump` and `route list` reuse one implementation. Closes #42
|
Warning Review limit reached
More reviews will be available in 9 minutes and 56 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAuto-aggregates routes across services for ChangesRoute listing across services
sequenceDiagram
participant Client
participant RouteListCmd as route list command
participant ListUtil
participant API
Client->>RouteListCmd: route list -g default
RouteListCmd->>RouteListCmd: Parse gateway group and optional label filters
alt serviceID provided
RouteListCmd->>API: GET /apisix/admin/routes?service_id=X
API-->>RouteListCmd: Routes for service X
else serviceID omitted
RouteListCmd->>API: GET /apisix/admin/services?gateway_id=default
API-->>RouteListCmd: Services list
RouteListCmd->>ListUtil: FetchRoutesForServices
loop for each service
ListUtil->>API: GET /apisix/admin/routes?service_id=Y
API-->>ListUtil: Routes for service Y
end
ListUtil-->>RouteListCmd: Aggregated and deduplicated routes
end
RouteListCmd->>RouteListCmd: Apply label value filters
RouteListCmd->>RouteListCmd: Render table with SERVICE_ID column
RouteListCmd-->>Client: Table output
🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
CI golangci-lint flagged S1030 on `[]byte(out.String())`.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR improves the a7 route list UX by allowing a7 route list -g <group> to return routes across all services when --service-id is omitted, working around API7 EE’s access-token requirement to scope /apisix/admin/routes by service_id. It also centralizes the pagination/aggregation logic so both runtime listing and config sync/dump share one implementation.
Changes:
- Implement client-side aggregation for
route listwhen--service-idis omitted, and add aSERVICE_IDcolumn to table output (JSON/YAML output remains[]api.Route). - Introduce
pkg/listutilfor shared pagination and per-service route aggregation; update configutil to use it. - Add/adjust unit tests + e2e test coverage and update the user guide to reflect the new semantics.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| test/e2e/route_test.go | Adds an EE-backed e2e test to validate cross-service aggregation behavior. |
| pkg/listutil/listutil.go | New shared utilities for paginated listing and aggregating routes across services. |
| pkg/cmd/route/list/list.go | Implements aggregated route listing when --service-id is omitted; adds SERVICE_ID to table output. |
| pkg/cmd/route/list/list_test.go | Updates existing tests for the new table column and adds new aggregated-path unit tests (table + JSON). |
| pkg/cmd/config/configutil/configutil.go | Switches config remote-fetch logic to use shared listutil pagination/aggregation helpers. |
| docs/user-guide/route.md | Updates docs to describe default cross-service listing and adjusted --service-id semantics. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![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
🤖 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 `@pkg/cmd/route/list/list.go`:
- Around line 72-75: The bug is that baseQuery (with labelKey) is reused for
both route and service listing so a service-level label filter can exclude
services that host matching routes; change the query usage so the route label is
applied only when listing routes: keep baseQuery for service discovery without
adding labelKey, then build a separate routeQuery (copy of baseQuery plus label
when labelKey != "") and pass routeQuery to the routes listing call while
passing baseQuery (without label) to the services listing call (references:
baseQuery, labelKey and the code that lists services/routes in list.go).
In `@pkg/listutil/listutil.go`:
- Around line 1-6: Add the required Apache 2.0 license header to the top of this
new Go file so it precedes the package declaration; insert the standard
multi-line Apache 2.0 comment block above "package listutil" (keeping the
existing package name and comments intact) to comply with the project's
licensing guidelines.
- Around line 35-40: The generic pagination helper in pkg/listutil/listutil.go
currently swallows API errors by returning (nil, nil) when
cmdutil.IsOptionalResourceError(err) is true after client.Get(path, query);
change the helper to take an explicit allowOptional bool flag (e.g., Add
parameter allowOptional to the pagination function) and only convert
optional-resource errors into (nil, nil) when allowOptional==true, otherwise
return the original error; keep the check using
cmdutil.IsOptionalResourceError(err) and update all call sites to pass
allowOptional=true only for endpoints known to be optional.
🪄 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: 09aaf0b9-120d-4327-9bed-5aedafe85cb7
📒 Files selected for processing (6)
docs/user-guide/route.mdpkg/cmd/config/configutil/configutil.gopkg/cmd/route/list/list.gopkg/cmd/route/list/list_test.gopkg/listutil/listutil.gotest/e2e/route_test.go
Two PR review findings: 1. The label query was added to the shared baseQuery and reused for the /services discovery call, so a service-level label mismatch would drop matching routes from the aggregated result. Build a separate routeQuery that carries the label; keep baseQuery clean for services. 2. The --service-id branch issued a single client.Get and returned the first page only, so a service with more than 500 routes would silently truncate (the aggregated path already used FetchPaginated). Route both paths through listutil.FetchPaginated so they paginate consistently. Adds TestListRoutes_LabelDoesNotFilterServices and TestListRoutes_PaginatesServiceIDPath. Follow-up issue #50 tracks the remaining FetchPaginated error-swallow concern. Refs #42
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/cmd/route/list/list_test.go (1)
381-428: ⚡ Quick winAssert that the discovery path is exercised in this label-isolation test.
At Line 425 onward, the test only checks rendered output. It can still pass if
/servicesdiscovery is accidentally bypassed, which weakens the intended guarantee.✅ Suggested test hardening
func TestListRoutes_LabelDoesNotFilterServices(t *testing.T) { @@ if !strings.Contains(out.String(), "r1") { t.Errorf("expected route r1 in output\noutput:\n%s", out.String()) } + if got := registry.CallCount(http.MethodGet, "/apisix/admin/services"); got != 1 { + t.Errorf("expected /services discovery to be called once, got %d", got) + } + if got := registry.CallCount(http.MethodGet, "/apisix/admin/routes"); got != 1 { + t.Errorf("expected one /routes call for discovered service, got %d", got) + } }🤖 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 `@pkg/cmd/route/list/list_test.go` around lines 381 - 428, The test currently only checks output and may pass if the /services discovery call is skipped; modify TestListRoutes_LabelDoesNotFilterServices to assert that the /apisix/admin/services responder was actually exercised: in the registry.RegisterResponder callback for "/apisix/admin/services" set a local boolean or increment a counter (e.g., servicesCalled) and after calling actionRun(opts) add an assertion that servicesCalled is true (or counter > 0). Keep the existing assertions for the /routes responder and output check; this ensures the discovery path is exercised and the label-isolation behavior is validated.
🤖 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.
Nitpick comments:
In `@pkg/cmd/route/list/list_test.go`:
- Around line 381-428: The test currently only checks output and may pass if the
/services discovery call is skipped; modify
TestListRoutes_LabelDoesNotFilterServices to assert that the
/apisix/admin/services responder was actually exercised: in the
registry.RegisterResponder callback for "/apisix/admin/services" set a local
boolean or increment a counter (e.g., servicesCalled) and after calling
actionRun(opts) add an assertion that servicesCalled is true (or counter > 0).
Keep the existing assertions for the /routes responder and output check; this
ensures the discovery path is exercised and the label-isolation behavior is
validated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8c201166-6e4b-4f56-a4f4-1c8de06bc18f
📒 Files selected for processing (2)
pkg/cmd/route/list/list.gopkg/cmd/route/list/list_test.go
✅ Files skipped from review due to trivial changes (1)
- pkg/cmd/route/list/list.go
The leftover "count [{ occurrences as a proxy for route count" line
described an output-count check that was never written; the call-count
assertion that follows is what actually verifies pagination.
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
a7 route list -g <group>now lists every route in the gateway group when--service-idis omitted, by listing services first and aggregating per-service route fetches client-side (API7 EE's/apisix/admin/routesrequiresservice_idunder access-token auth, so a single unscoped call isn't possible).SERVICE_IDcolumn to the table output so the unfiltered view stays meaningful; JSON/YAML output is unchanged ([]api.Route, which already carriesservice_id).pkg/cmd/config/configutilinto a sharedpkg/listutilpackage soconfig sync/dumpandroute listuse one implementation.--service-idflag description to reflect the new optional semantics.Closes #42
Test plan
go build ./...cleango vet ./...andgo vet -tags=e2e ./test/e2e/...cleango test ./... -count=1all green, including new unit testsTestListRoutes_AcrossServicesandTestListRoutes_AcrossServices_JSON(covers /services + per-service /routes aggregation, dedupe, JSON exporter)TestRoute_ListAcrossServices(creates two services with one route each, asserts both IDs ina7 route list -g default) — requires real API7 EE; will run in the e2e CI job--service-idpath unchanged:TestListRoutes_Table,TestListRoutes_JSON,TestListRoutes_GatewayGroupFrom{Config,Flag}still passNotes
docs/api7ee-api-spec.md); admin-key usage is an internal CP construct and out of scope.configutilbehavior of skipping a service whose route fetch returns the "optional resource" 400/404 (typical for race conditions where a service is deleted mid-iteration). Other errors still propagate.Summary by CodeRabbit
New Features
a7 route listnow lists routes across all services in the gateway group by default.Bug Fixes
--labelis applied to route queries (not to service discovery), producing correct filtered results when listing across services.Documentation
--service-idfilters to a single service.Tests