From 3bd64c399068cb6e801c961dc11d3d998f0a21fd Mon Sep 17 00:00:00 2001 From: Abhishek Choudhary Date: Tue, 26 May 2026 16:16:38 +0800 Subject: [PATCH 1/4] fix(listutil): gate optional-resource swallow behind explicit flag FetchPaginated previously converted any IsOptionalResourceError (400/404) into (nil, nil) for every caller. The lenience was inherited from configutil's private helper, where it was intentional for stream_routes, protos and secret_providers in non-stream deployments; promoting the helper to a shared package generalized it to consumers that need strict behavior: * route list aggregation across services silently dropped a deleted service's 404 and rendered a short table with no error. * config dump / config sync trusted (nil, nil) on services / consumers / ssl / global_rules, producing partial output that the diff engine would then treat as remote deletions or duplicate creates against an out-of-sync gateway. Add an explicit allowOptional bool to FetchPaginated and thread it through FetchRoutesForServices. Set true at the three configutil call sites that genuinely tolerate the endpoint being absent (stream_routes, protos, secret_providers); set false everywhere else. Closes #50 --- pkg/cmd/config/configutil/configutil.go | 16 +-- pkg/cmd/route/list/list.go | 6 +- pkg/listutil/listutil.go | 24 ++-- pkg/listutil/listutil_test.go | 152 ++++++++++++++++++++++++ 4 files changed, 178 insertions(+), 20 deletions(-) create mode 100644 pkg/listutil/listutil_test.go diff --git a/pkg/cmd/config/configutil/configutil.go b/pkg/cmd/config/configutil/configutil.go index 406fe25..cde1f89 100644 --- a/pkg/cmd/config/configutil/configutil.go +++ b/pkg/cmd/config/configutil/configutil.go @@ -112,38 +112,38 @@ func FetchRemoteConfig(client *api.Client, gatewayGroup string) (*api.ConfigFile query["gateway_group_id"] = gatewayGroup } - services, err := listutil.FetchPaginated[api.Service](client, "/apisix/admin/services", query) + services, err := listutil.FetchPaginated[api.Service](client, "/apisix/admin/services", query, false) if err != nil { return nil, err } // API7 EE requires service_id when listing routes with access tokens. // Fetch routes per service and aggregate. - routes, err := listutil.FetchRoutesForServices(client, services, query) + routes, err := listutil.FetchRoutesForServices(client, services, query, false) if err != nil { return nil, err } - consumers, err := listutil.FetchPaginated[api.Consumer](client, "/apisix/admin/consumers", query) + consumers, err := listutil.FetchPaginated[api.Consumer](client, "/apisix/admin/consumers", query, false) if err != nil { return nil, err } - ssl, err := listutil.FetchPaginated[api.SSL](client, "/apisix/admin/ssls", query) + ssl, err := listutil.FetchPaginated[api.SSL](client, "/apisix/admin/ssls", query, false) if err != nil { return nil, err } - globalRules, err := listutil.FetchPaginated[api.GlobalRule](client, "/apisix/admin/global_rules", query) + globalRules, err := listutil.FetchPaginated[api.GlobalRule](client, "/apisix/admin/global_rules", query, false) if err != nil { return nil, err } - streamRoutes, err := listutil.FetchPaginated[api.StreamRoute](client, "/apisix/admin/stream_routes", query) + streamRoutes, err := listutil.FetchPaginated[api.StreamRoute](client, "/apisix/admin/stream_routes", query, true) if err != nil { return nil, err } - protos, err := listutil.FetchPaginated[api.Proto](client, "/apisix/admin/protos", query) + protos, err := listutil.FetchPaginated[api.Proto](client, "/apisix/admin/protos", query, true) if err != nil { return nil, err } - secrets, err := listutil.FetchPaginated[api.Secret](client, "/apisix/admin/secret_providers", query) + secrets, err := listutil.FetchPaginated[api.Secret](client, "/apisix/admin/secret_providers", query, true) if err != nil { return nil, err } diff --git a/pkg/cmd/route/list/list.go b/pkg/cmd/route/list/list.go index 2fe17b0..f677493 100644 --- a/pkg/cmd/route/list/list.go +++ b/pkg/cmd/route/list/list.go @@ -125,12 +125,12 @@ func fetchRoutes(client *api.Client, baseQuery map[string]string, serviceID, lab if serviceID != "" { routeQuery["service_id"] = serviceID - return listutil.FetchPaginated[api.Route](client, "/apisix/admin/routes", routeQuery) + return listutil.FetchPaginated[api.Route](client, "/apisix/admin/routes", routeQuery, false) } - services, err := listutil.FetchPaginated[api.Service](client, "/apisix/admin/services", baseQuery) + services, err := listutil.FetchPaginated[api.Service](client, "/apisix/admin/services", baseQuery, false) if err != nil { return nil, err } - return listutil.FetchRoutesForServices(client, services, routeQuery) + return listutil.FetchRoutesForServices(client, services, routeQuery, false) } diff --git a/pkg/listutil/listutil.go b/pkg/listutil/listutil.go index dc74dc3..89d8939 100644 --- a/pkg/listutil/listutil.go +++ b/pkg/listutil/listutil.go @@ -16,10 +16,15 @@ const defaultPageSize = 500 // FetchPaginated fetches all items from a paginated API7 EE list endpoint. // API7 EE returns ListResponse[T] with .List []T directly (no ListItem wrapper). -// Returns (nil, nil) when the endpoint signals that the resource is unavailable -// (e.g., stream mode disabled, endpoint not exposed); callers that need stricter -// behavior should inspect the returned error themselves. -func FetchPaginated[T any](client *api.Client, path string, extraQuery map[string]string) ([]T, error) { +// +// When allowOptional is true, the helper converts API errors that mark a +// resource as unavailable (400/404, see cmdutil.IsOptionalResourceError) into +// (nil, nil). This is only appropriate for endpoints that genuinely may not be +// in use on the target gateway (stream_routes, protos, secret_providers, +// plugin_metadata). All other callers must pass false so transient failures +// or contract changes surface as errors instead of silently shrinking the +// result. +func FetchPaginated[T any](client *api.Client, path string, extraQuery map[string]string, allowOptional bool) ([]T, error) { page := 1 var items []T @@ -34,7 +39,7 @@ func FetchPaginated[T any](client *api.Client, path string, extraQuery map[strin body, err := client.Get(path, query) if err != nil { - if cmdutil.IsOptionalResourceError(err) { + if allowOptional && cmdutil.IsOptionalResourceError(err) { return nil, nil } return nil, err @@ -61,9 +66,10 @@ func FetchPaginated[T any](client *api.Client, path string, extraQuery map[strin // route in a gateway group must iterate services and merge. // // baseQuery is merged into each per-service request (e.g. `gateway_group_id`). -// A service whose route fetch returns an "optional resource" error (400/404) -// is skipped, matching the behavior of FetchPaginated. -func FetchRoutesForServices(client *api.Client, services []api.Service, baseQuery map[string]string) ([]api.Route, error) { +// allowOptional is threaded into the underlying FetchPaginated call; pass +// false unless the caller has a specific reason to tolerate per-service +// 400/404 responses. +func FetchRoutesForServices(client *api.Client, services []api.Service, baseQuery map[string]string, allowOptional bool) ([]api.Route, error) { seen := make(map[string]bool) var allRoutes []api.Route for _, svc := range services { @@ -75,7 +81,7 @@ func FetchRoutesForServices(client *api.Client, services []api.Service, baseQuer q[k] = v } q["service_id"] = svc.ID - routes, err := FetchPaginated[api.Route](client, "/apisix/admin/routes", q) + routes, err := FetchPaginated[api.Route](client, "/apisix/admin/routes", q, allowOptional) if err != nil { return nil, err } diff --git a/pkg/listutil/listutil_test.go b/pkg/listutil/listutil_test.go new file mode 100644 index 0000000..0b5d38f --- /dev/null +++ b/pkg/listutil/listutil_test.go @@ -0,0 +1,152 @@ +package listutil + +import ( + "errors" + "net/http" + "testing" + + "github.com/api7/a7/pkg/api" + "github.com/api7/a7/pkg/httpmock" +) + +func newTestClient(reg *httpmock.Registry) *api.Client { + return api.NewClient(reg.GetClient(), "http://api.local") +} + +// TestFetchPaginated_StrictPropagates404 confirms that with allowOptional=false +// a 404 from the upstream surfaces as an error instead of being silently +// converted to (nil, nil). +func TestFetchPaginated_StrictPropagates404(t *testing.T) { + reg := &httpmock.Registry{} + reg.Register(http.MethodGet, "/apisix/admin/services", httpmock.StringResponse(http.StatusNotFound, `{"error_msg":"not found"}`)) + + client := newTestClient(reg) + items, err := FetchPaginated[api.Service](client, "/apisix/admin/services", nil, false) + if err == nil { + t.Fatalf("expected error, got items=%v", items) + } + if items != nil { + t.Errorf("expected nil items on error, got %v", items) + } + var apiErr *api.APIError + if !errors.As(err, &apiErr) || apiErr.StatusCode != http.StatusNotFound { + t.Errorf("expected *api.APIError with status 404, got %T: %v", err, err) + } +} + +// TestFetchPaginated_StrictPropagates400 confirms 400 is also propagated when +// allowOptional=false. A transient 400 on services/consumers must not be +// swallowed because config sync would then plan destructive operations +// against the unintentionally-empty result. +func TestFetchPaginated_StrictPropagates400(t *testing.T) { + reg := &httpmock.Registry{} + reg.Register(http.MethodGet, "/apisix/admin/consumers", httpmock.StringResponse(http.StatusBadRequest, `{"error_msg":"bad request"}`)) + + client := newTestClient(reg) + if _, err := FetchPaginated[api.Consumer](client, "/apisix/admin/consumers", nil, false); err == nil { + t.Fatal("expected error to propagate when allowOptional=false") + } +} + +// TestFetchPaginated_OptionalSwallows404 confirms the opt-in lenient path +// still works: stream_routes / protos / secret_providers callers pass +// allowOptional=true and expect (nil, nil) when the endpoint signals the +// resource is not in use. +func TestFetchPaginated_OptionalSwallows404(t *testing.T) { + reg := &httpmock.Registry{} + reg.Register(http.MethodGet, "/apisix/admin/stream_routes", httpmock.StringResponse(http.StatusNotFound, `{"error_msg":"not found"}`)) + + client := newTestClient(reg) + items, err := FetchPaginated[api.StreamRoute](client, "/apisix/admin/stream_routes", nil, true) + if err != nil { + t.Fatalf("expected nil error with allowOptional=true, got %v", err) + } + if items != nil { + t.Errorf("expected nil items, got %v", items) + } +} + +// TestFetchPaginated_OptionalSwallows400 mirrors the 404 case: stream mode +// disabled commonly returns 400, and allowOptional=true callers expect it +// suppressed. +func TestFetchPaginated_OptionalSwallows400(t *testing.T) { + reg := &httpmock.Registry{} + reg.Register(http.MethodGet, "/apisix/admin/protos", httpmock.StringResponse(http.StatusBadRequest, `{"error_msg":"stream disabled"}`)) + + client := newTestClient(reg) + items, err := FetchPaginated[api.Proto](client, "/apisix/admin/protos", nil, true) + if err != nil { + t.Fatalf("expected nil error with allowOptional=true, got %v", err) + } + if items != nil { + t.Errorf("expected nil items, got %v", items) + } +} + +// TestFetchPaginated_OptionalStillPropagatesOther5xx confirms allowOptional +// only relaxes the 400/404 contract; a 500 must still surface. +func TestFetchPaginated_OptionalStillPropagatesOther5xx(t *testing.T) { + reg := &httpmock.Registry{} + reg.Register(http.MethodGet, "/apisix/admin/protos", httpmock.StringResponse(http.StatusInternalServerError, `{"error_msg":"boom"}`)) + + client := newTestClient(reg) + if _, err := FetchPaginated[api.Proto](client, "/apisix/admin/protos", nil, true); err == nil { + t.Fatal("expected 500 to propagate even with allowOptional=true") + } +} + +// TestFetchRoutesForServices_StrictPropagatesPerServiceError confirms that a +// 404 on one service's /routes is not silently dropped when allowOptional is +// false. This is the race scenario from issue #50: if a service is deleted +// between enumeration and the per-service /routes fetch, the user must see +// an error rather than a quietly-shortened list. +func TestFetchRoutesForServices_StrictPropagatesPerServiceError(t *testing.T) { + reg := &httpmock.Registry{} + reg.RegisterResponder(http.MethodGet, "/apisix/admin/routes", func(r *http.Request) (httpmock.Response, error) { + switch r.URL.Query().Get("service_id") { + case "svc-a": + return httpmock.JSONResponse(`{"total":1,"list":[{"id":"r-a","service_id":"svc-a"}]}`), nil + case "svc-gone": + return httpmock.StringResponse(http.StatusNotFound, `{"error_msg":"not found"}`), nil + default: + return httpmock.JSONResponse(`{"total":0,"list":[]}`), nil + } + }) + + client := newTestClient(reg) + services := []api.Service{{ID: "svc-a"}, {ID: "svc-gone"}} + routes, err := FetchRoutesForServices(client, services, nil, false) + if err == nil { + t.Fatalf("expected error, got routes=%v", routes) + } + var apiErr *api.APIError + if !errors.As(err, &apiErr) || apiErr.StatusCode != http.StatusNotFound { + t.Errorf("expected *api.APIError with status 404, got %T: %v", err, err) + } +} + +// TestFetchRoutesForServices_OptionalSkipsPerServiceError documents that the +// lenient path is still wired through for callers that explicitly opt in. +func TestFetchRoutesForServices_OptionalSkipsPerServiceError(t *testing.T) { + reg := &httpmock.Registry{} + reg.RegisterResponder(http.MethodGet, "/apisix/admin/routes", func(r *http.Request) (httpmock.Response, error) { + switch r.URL.Query().Get("service_id") { + case "svc-a": + return httpmock.JSONResponse(`{"total":1,"list":[{"id":"r-a","service_id":"svc-a"}]}`), nil + case "svc-gone": + return httpmock.StringResponse(http.StatusNotFound, `{"error_msg":"not found"}`), nil + default: + return httpmock.JSONResponse(`{"total":0,"list":[]}`), nil + } + }) + + client := newTestClient(reg) + services := []api.Service{{ID: "svc-a"}, {ID: "svc-gone"}} + routes, err := FetchRoutesForServices(client, services, nil, true) + if err != nil { + t.Fatalf("expected no error with allowOptional=true, got %v", err) + } + if len(routes) != 1 || routes[0].ID != "r-a" { + t.Errorf("expected only svc-a's routes, got %+v", routes) + } +} From c0d56f4e496c6e9121a71aa519f44cc9da6d593e Mon Sep 17 00:00:00 2001 From: Abhishek Choudhary Date: Wed, 27 May 2026 09:46:22 +0800 Subject: [PATCH 2/4] fixup: address review on PR #52 - Drop plugin_metadata from the FetchPaginated doc comment; it never goes through FetchPaginated (configutil.fetchPluginMetadata hand-rolls /plugins/list + /plugin_metadata/{name}). - Call reg.Verify(t) in every listutil test so a regression that short-circuits before issuing the HTTP request can't pass silently. --- pkg/listutil/listutil.go | 7 +++---- pkg/listutil/listutil_test.go | 7 +++++++ 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/pkg/listutil/listutil.go b/pkg/listutil/listutil.go index 89d8939..ce38df7 100644 --- a/pkg/listutil/listutil.go +++ b/pkg/listutil/listutil.go @@ -20,10 +20,9 @@ const defaultPageSize = 500 // When allowOptional is true, the helper converts API errors that mark a // resource as unavailable (400/404, see cmdutil.IsOptionalResourceError) into // (nil, nil). This is only appropriate for endpoints that genuinely may not be -// in use on the target gateway (stream_routes, protos, secret_providers, -// plugin_metadata). All other callers must pass false so transient failures -// or contract changes surface as errors instead of silently shrinking the -// result. +// in use on the target gateway (stream_routes, protos, secret_providers). +// All other callers must pass false so transient failures or contract changes +// surface as errors instead of silently shrinking the result. func FetchPaginated[T any](client *api.Client, path string, extraQuery map[string]string, allowOptional bool) ([]T, error) { page := 1 var items []T diff --git a/pkg/listutil/listutil_test.go b/pkg/listutil/listutil_test.go index 0b5d38f..e73d071 100644 --- a/pkg/listutil/listutil_test.go +++ b/pkg/listutil/listutil_test.go @@ -32,6 +32,7 @@ func TestFetchPaginated_StrictPropagates404(t *testing.T) { if !errors.As(err, &apiErr) || apiErr.StatusCode != http.StatusNotFound { t.Errorf("expected *api.APIError with status 404, got %T: %v", err, err) } + reg.Verify(t) } // TestFetchPaginated_StrictPropagates400 confirms 400 is also propagated when @@ -46,6 +47,7 @@ func TestFetchPaginated_StrictPropagates400(t *testing.T) { if _, err := FetchPaginated[api.Consumer](client, "/apisix/admin/consumers", nil, false); err == nil { t.Fatal("expected error to propagate when allowOptional=false") } + reg.Verify(t) } // TestFetchPaginated_OptionalSwallows404 confirms the opt-in lenient path @@ -64,6 +66,7 @@ func TestFetchPaginated_OptionalSwallows404(t *testing.T) { if items != nil { t.Errorf("expected nil items, got %v", items) } + reg.Verify(t) } // TestFetchPaginated_OptionalSwallows400 mirrors the 404 case: stream mode @@ -81,6 +84,7 @@ func TestFetchPaginated_OptionalSwallows400(t *testing.T) { if items != nil { t.Errorf("expected nil items, got %v", items) } + reg.Verify(t) } // TestFetchPaginated_OptionalStillPropagatesOther5xx confirms allowOptional @@ -93,6 +97,7 @@ func TestFetchPaginated_OptionalStillPropagatesOther5xx(t *testing.T) { if _, err := FetchPaginated[api.Proto](client, "/apisix/admin/protos", nil, true); err == nil { t.Fatal("expected 500 to propagate even with allowOptional=true") } + reg.Verify(t) } // TestFetchRoutesForServices_StrictPropagatesPerServiceError confirms that a @@ -123,6 +128,7 @@ func TestFetchRoutesForServices_StrictPropagatesPerServiceError(t *testing.T) { if !errors.As(err, &apiErr) || apiErr.StatusCode != http.StatusNotFound { t.Errorf("expected *api.APIError with status 404, got %T: %v", err, err) } + reg.Verify(t) } // TestFetchRoutesForServices_OptionalSkipsPerServiceError documents that the @@ -149,4 +155,5 @@ func TestFetchRoutesForServices_OptionalSkipsPerServiceError(t *testing.T) { if len(routes) != 1 || routes[0].ID != "r-a" { t.Errorf("expected only svc-a's routes, got %+v", routes) } + reg.Verify(t) } From 603125bc30f2065b214cc26e8959f81ee5da519e Mon Sep 17 00:00:00 2001 From: Abhishek Choudhary Date: Wed, 27 May 2026 09:59:32 +0800 Subject: [PATCH 3/4] trigger ci From 11363b4db023d15c430a28d601c206549efb4ec4 Mon Sep 17 00:00:00 2001 From: Abhishek Choudhary Date: Wed, 27 May 2026 10:00:38 +0800 Subject: [PATCH 4/4] chore: trigger CI after base retarget to master