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..ce38df7 100644 --- a/pkg/listutil/listutil.go +++ b/pkg/listutil/listutil.go @@ -16,10 +16,14 @@ 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). +// 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 +38,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 +65,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 +80,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..e73d071 --- /dev/null +++ b/pkg/listutil/listutil_test.go @@ -0,0 +1,159 @@ +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) + } + reg.Verify(t) +} + +// 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") + } + reg.Verify(t) +} + +// 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) + } + reg.Verify(t) +} + +// 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) + } + reg.Verify(t) +} + +// 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") + } + reg.Verify(t) +} + +// 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) + } + reg.Verify(t) +} + +// 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) + } + reg.Verify(t) +}