diff --git a/cmd/server/config.go b/cmd/server/config.go index cd767c54..e259a31e 100644 --- a/cmd/server/config.go +++ b/cmd/server/config.go @@ -142,14 +142,17 @@ type Config struct { // GOMEMLIMIT environment variable, when set, takes precedence. Runtime *RuntimeConfig `json:"runtime,omitempty"` GeoFilter *GeoFilterConfig `json:"geo_filter,omitempty"` - // GeoFilterAppliesToNodeList restores the pre-opt-in behavior where - // configuring geo_filter alone made GET /api/nodes exclude out-of- - // polygon nodes by default (no ?geoFilter=1 needed). Off by default — - // deployments that already relied on the old always-on filtering - // (added under #730) can set this to keep it; new deployments get the - // non-surprising default of geo_filter not silently changing what the - // node list/map returns. - GeoFilterAppliesToNodeList bool `json:"geoFilterAppliesToNodeList,omitempty"` + // GeoFilterExemptNodeList opts a deployment OUT of the long-standing + // #730 behavior where configuring geo_filter also makes GET /api/nodes + // (and therefore the live map) exclude out-of-polygon nodes by + // default. Off by default — an existing config.json with geo_filter + // already set (i.e. every deployment predating this field) keeps + // getting exactly the filtering it always had; a deployment that wants + // geo_filter purely for foreign_advert classification/analytics, + // without also decluttering the node list/map, sets this to true. + // Per-request ?geoFilter=0 / ?geoFilter=1 overrides either default for + // that one call. + GeoFilterExemptNodeList bool `json:"geoFilterExemptNodeList,omitempty"` Areas map[string]AreaEntry `json:"areas,omitempty"` diff --git a/cmd/server/nodes_geofilter_optin_test.go b/cmd/server/nodes_geofilter_optin_test.go index cbdd310b..e2f54bd8 100644 --- a/cmd/server/nodes_geofilter_optin_test.go +++ b/cmd/server/nodes_geofilter_optin_test.go @@ -6,12 +6,14 @@ import ( "testing" ) -// Regression test: configuring geo_filter alone must not change what -// GET /api/nodes returns. Before this fix, setting geo_filter silently -// hid every node outside the polygon that hadn't yet been re-tagged -// foreign_advert=1 by the ingestor (which only happens on that node's -// next ADVERT) — including the live map, which lists straight off this -// endpoint. The filter is now opt-in via ?geoFilter=1. +// Regression test: geo_filter's node-list-declutter behavior (#730) is +// preserved by default for every deployment that already had geo_filter +// configured — GeoFilterExemptNodeList (default false) only lets a NEW +// adopter of geo_filter (using it purely for foreign_advert classification/ +// analytics) opt OUT of also hiding out-of-polygon nodes from +// GET /api/nodes (and therefore the live map, which lists straight off +// this endpoint). Per-request ?geoFilter=0/1 overrides either default for +// a single call. func TestHandleNodes_GeoFilterExcludedByDefault(t *testing.T) { apiKey := "a-strong-api-key-for-testing" srv, router, _ := setupGeoFilterServer(t, apiKey) @@ -40,30 +42,19 @@ func TestHandleNodes_GeoFilterExcludedByDefault(t *testing.T) { } return out } - - t.Run("default request returns every node regardless of geo_filter", func(t *testing.T) { - req := httptest.NewRequest("GET", "/api/nodes?limit=50", nil) + get := func(qs string) map[string]bool { + t.Helper() + req := httptest.NewRequest("GET", "/api/nodes?limit=50"+qs, nil) w := httptest.NewRecorder() router.ServeHTTP(w, req) if w.Code != 200 { t.Fatalf("expected 200, got %d", w.Code) } - got := names(w) - for _, want := range []string{"InsideNode", "OutsideUntagged", "OutsideTagged"} { - if !got[want] { - t.Errorf("expected %s in default (unfiltered) response, got %v", want, got) - } - } - }) + return names(w) + } - t.Run("geoFilter=1 excludes untagged out-of-polygon nodes but keeps foreign-tagged ones", func(t *testing.T) { - req := httptest.NewRequest("GET", "/api/nodes?limit=50&geoFilter=1", nil) - w := httptest.NewRecorder() - router.ServeHTTP(w, req) - if w.Code != 200 { - t.Fatalf("expected 200, got %d", w.Code) - } - got := names(w) + t.Run("default request preserves the long-standing #730 filtering (deployments predating this field see no change)", func(t *testing.T) { + got := get("") if !got["InsideNode"] { t.Error("expected InsideNode (within polygon) to be present") } @@ -71,29 +62,44 @@ func TestHandleNodes_GeoFilterExcludedByDefault(t *testing.T) { t.Error("expected OutsideTagged (foreign_advert=1) to be present even though it's outside the polygon — #730") } if got["OutsideUntagged"] { - t.Error("expected OutsideUntagged (outside polygon, not yet foreign-tagged) to be excluded when geoFilter=1") + t.Error("expected OutsideUntagged (outside polygon, not foreign-tagged) to be excluded by default, matching the pre-existing #730 behavior") } }) - t.Run("GeoFilterAppliesToNodeList=true restores the pre-opt-in always-on behavior without needing ?geoFilter=1", func(t *testing.T) { - srv.cfg.GeoFilterAppliesToNodeList = true - defer func() { srv.cfg.GeoFilterAppliesToNodeList = false }() + t.Run("geoFilter=0 forces the filter off for a single request", func(t *testing.T) { + got := get("&geoFilter=0") + if !got["OutsideUntagged"] { + t.Error("expected OutsideUntagged to be present when explicitly overriding with geoFilter=0") + } + }) - req := httptest.NewRequest("GET", "/api/nodes?limit=50", nil) - w := httptest.NewRecorder() - router.ServeHTTP(w, req) - if w.Code != 200 { - t.Fatalf("expected 200, got %d", w.Code) - } - got := names(w) - if !got["InsideNode"] { - t.Error("expected InsideNode (within polygon) to be present") - } - if !got["OutsideTagged"] { - t.Error("expected OutsideTagged (foreign_advert=1) to be present even though it's outside the polygon — #730") + t.Run("geoFilter=1 is a no-op restating the default (still excludes untagged, keeps foreign-tagged)", func(t *testing.T) { + got := get("&geoFilter=1") + if !got["InsideNode"] || !got["OutsideTagged"] { + t.Error("expected InsideNode and OutsideTagged to remain present") } if got["OutsideUntagged"] { - t.Error("expected OutsideUntagged to be excluded by default when GeoFilterAppliesToNodeList=true, without needing ?geoFilter=1") + t.Error("expected OutsideUntagged to still be excluded with geoFilter=1") + } + }) + + t.Run("GeoFilterExemptNodeList=true makes the default request return everything, without needing ?geoFilter=0", func(t *testing.T) { + srv.cfg.GeoFilterExemptNodeList = true + defer func() { srv.cfg.GeoFilterExemptNodeList = false }() + + got := get("") + if !got["InsideNode"] || !got["OutsideTagged"] || !got["OutsideUntagged"] { + t.Errorf("expected every node present when GeoFilterExemptNodeList=true, got %v", got) + } + }) + + t.Run("geoFilter=1 still overrides GeoFilterExemptNodeList=true for a single request", func(t *testing.T) { + srv.cfg.GeoFilterExemptNodeList = true + defer func() { srv.cfg.GeoFilterExemptNodeList = false }() + + got := get("&geoFilter=1") + if got["OutsideUntagged"] { + t.Error("expected OutsideUntagged to be excluded when geoFilter=1 overrides an exempt deployment") } }) } diff --git a/cmd/server/openapi.go b/cmd/server/openapi.go index 36c12843..a1f335d2 100644 --- a/cmd/server/openapi.go +++ b/cmd/server/openapi.go @@ -80,7 +80,7 @@ func routeDescriptions() map[string]routeMeta { QueryParams: []paramMeta{ {Name: "role", Description: "Filter by node role", Type: "string"}, {Name: "status", Description: "Filter by status (active/stale/offline)", Type: "string"}, - {Name: "geoFilter", Description: "Set to \"1\" to exclude nodes outside the configured geo_filter (unless foreign_advert-tagged). Opt-in — omitting it returns every node regardless of geo_filter, unless the deployment's config.json sets geoFilterAppliesToNodeList=true to restore the pre-opt-in always-on behavior.", Type: "string"}, + {Name: "geoFilter", Description: "Overrides the deployment's geo_filter node-list default for this one request: \"1\" excludes nodes outside the configured geo_filter (unless foreign_advert-tagged), \"0\" returns every node regardless. Omitting it uses the deployment default — geo_filter applies to the node list unless config.json sets geoFilterExemptNodeList=true.", Type: "string"}, }}, "GET /api/nodes/search": {Summary: "Search nodes", Description: "Search nodes by name or public key prefix.", Tag: "nodes", QueryParams: []paramMeta{{Name: "q", Description: "Search query", Type: "string", Required: true}}}, "GET /api/nodes/bulk-health": {Summary: "Bulk node health", Description: "Returns health status for all nodes in one call.", Tag: "nodes"}, diff --git a/cmd/server/routes.go b/cmd/server/routes.go index 513a7773..168ed63c 100644 --- a/cmd/server/routes.go +++ b/cmd/server/routes.go @@ -1426,20 +1426,23 @@ func (s *Server) handleNodes(w http.ResponseWriter, r *http.Request) { } } } - // Opt-in by default (?geoFilter=1), unless the deployment has - // GeoFilterAppliesToNodeList set to restore the pre-existing always-on - // behavior. Configuring geo_filter alone must not, by default, change - // what /api/nodes (and therefore the live map, which lists straight off - // this endpoint) returns — any node outside the polygon and not YET - // foreign_advert-tagged (which only happens on that node's next ADVERT - // after geo_filter was configured) would otherwise silently vanish from - // every view the moment geo_filter was set, with no per-request way to - // see them anyway. A deployment that intentionally relies on the old - // #730 declutter behavior can set GeoFilterAppliesToNodeList to keep - // it. geo_filter's own ingestor-side tagging (and the explicit - // prune-geo-filter admin flow) are unaffected either way — this only - // gates the passive declutter view. - applyGeoFilter := s.cfg.GeoFilterAppliesToNodeList || q.Get("geoFilter") == "1" + // geo_filter applies to the node list (and therefore the live map, + // which lists straight off this endpoint) BY DEFAULT when configured — + // the long-standing #730 declutter behavior. Every deployment that + // already had geo_filter set before GeoFilterExemptNodeList existed + // keeps getting exactly that, unchanged: the field is absent from + // their config.json, which decodes to false, which preserves the + // default. A deployment that wants geo_filter purely for + // foreign_advert classification/analytics — without also hiding + // out-of-polygon nodes from the map — opts out via + // GeoFilterExemptNodeList. ?geoFilter=0 / ?geoFilter=1 overrides + // either default for a single request. geo_filter's own ingestor-side + // tagging (and the explicit prune-geo-filter admin flow) are + // unaffected either way — this only gates the passive declutter view. + applyGeoFilter := !s.cfg.GeoFilterExemptNodeList + if v := q.Get("geoFilter"); v != "" { + applyGeoFilter = v == "1" + } if s.cfg.GeoFilter != nil && applyGeoFilter { filtered := nodes[:0] for _, node := range nodes {