From 62a5aad18cf78284783b5fb3cc5ca8b3aaeafa9a Mon Sep 17 00:00:00 2001 From: Marcin Rataj Date: Wed, 19 Nov 2025 22:06:48 +0100 Subject: [PATCH] fix: wire dht router to composableRouter and return error when DHT disabled fixes two issues with the GetClosestPeers endpoint: 1. endpoint returned HTTP 200 with empty results instead of actual DHT peers 2. when DHT disabled, returned HTTP 200 with empty results instead of error root cause: during rebase of PR #124 (commit 0dad3f7) when integrating with autoconf refactor (PR #123 / ec76365), the dhtRouters initialization was accidentally omitted from server.go. the autoconf PR renamed getCombinedRouting to combineRouters and changed its signature, but the rebase failed to preserve the dhtRouters creation line that existed in commit 42bd221. changes: - server.go:201-208: add explicit dhtRouters creation with caching and sanitization, similar to original 42bd221 implementation - server.go:232: wire dhtRouters to composableRouter.dht field - server.go:338: update combineRouters signature to accept host.Host parameter needed for GetClosestPeers peerstore lookups - server_routers.go:73-77: return routing.ErrNotSupported instead of empty iterator when DHT is nil, resulting in HTTP 500 instead of misleading HTTP 200 - server_test.go:16,20,24: update combineRouters test calls with new signature - server_dht_test.go:355-379: add test verifying HTTP 500 when DHT disabled --- server.go | 22 ++++++++++++++++------ server_dht_test.go | 26 ++++++++++++++++++++++++++ server_routers.go | 4 +++- server_test.go | 6 +++--- 4 files changed, 48 insertions(+), 10 deletions(-) diff --git a/server.go b/server.go index 330cc18..518974b 100644 --- a/server.go +++ b/server.go @@ -194,9 +194,18 @@ func start(ctx context.Context, cfg *config) error { } // Combine HTTP routers with DHT and additional routers - crRouters := combineRouters(dhtRouting, cachedAddrBook, providerHTTPRouters, blockProviderRouters) - prRouters := combineRouters(dhtRouting, cachedAddrBook, peerHTTPRouters, nil) - ipnsRouters := combineRouters(dhtRouting, cachedAddrBook, ipnsHTTPRouters, nil) + crRouters := combineRouters(h, dhtRouting, cachedAddrBook, providerHTTPRouters, blockProviderRouters) + prRouters := combineRouters(h, dhtRouting, cachedAddrBook, peerHTTPRouters, nil) + ipnsRouters := combineRouters(h, dhtRouting, cachedAddrBook, ipnsHTTPRouters, nil) + + // Create DHT router for GetClosestPeers endpoint + var dhtRouters router + if cachedAddrBook != nil && dhtRouting != nil { + cachedRouter := NewCachedRouter(libp2pRouter{host: h, routing: dhtRouting}, cachedAddrBook) + dhtRouters = sanitizeRouter{cachedRouter} + } else if dhtRouting != nil { + dhtRouters = sanitizeRouter{libp2pRouter{host: h, routing: dhtRouting}} + } _, port, err := net.SplitHostPort(cfg.listenAddress) if err != nil { @@ -220,6 +229,7 @@ func start(ctx context.Context, cfg *config) error { providers: crRouters, peers: prRouters, ipns: ipnsRouters, + dht: dhtRouters, }, handlerOpts...) // Add CORS. @@ -325,14 +335,14 @@ func newHost(cfg *config) (host.Host, error) { // combineRouters combines delegated HTTP routers with DHT and additional routers. // It no longer creates HTTP clients (that's done in createDelegatedHTTPRouters). -func combineRouters(dht routing.Routing, cachedAddrBook *cachedAddrBook, delegatedRouters, additionalRouters []router) router { +func combineRouters(h host.Host, dht routing.Routing, cachedAddrBook *cachedAddrBook, delegatedRouters, additionalRouters []router) router { var dhtRouter router if cachedAddrBook != nil { - cachedRouter := NewCachedRouter(libp2pRouter{routing: dht}, cachedAddrBook) + cachedRouter := NewCachedRouter(libp2pRouter{host: h, routing: dht}, cachedAddrBook) dhtRouter = sanitizeRouter{cachedRouter} } else if dht != nil { - dhtRouter = sanitizeRouter{libp2pRouter{routing: dht}} + dhtRouter = sanitizeRouter{libp2pRouter{host: h, routing: dht}} } if len(delegatedRouters) == 0 && len(additionalRouters) == 0 { diff --git a/server_dht_test.go b/server_dht_test.go index 3d20452..12b8981 100644 --- a/server_dht_test.go +++ b/server_dht_test.go @@ -351,6 +351,32 @@ func TestGetClosestPeersEndpoint(t *testing.T) { require.Equal(t, http.StatusOK, resp.StatusCode) require.Equal(t, mediaTypeJSON, resp.Header.Get("Content-Type")) }) + + t.Run("GET /routing/v1/dht/closest/peers/{cid} returns 500 when DHT is disabled", func(t *testing.T) { + t.Parallel() + + _, pid := makeEd25519PeerID(t) + key := peer.ToCid(pid) + + // Pass nil router to simulate DHT disabled via --dht=disabled + handler := server.Handler(&composableRouter{dht: nil}) + srv := httptest.NewServer(handler) + t.Cleanup(srv.Close) + + urlStr := fmt.Sprintf("http://%s/routing/v1/dht/closest/peers/%s", srv.Listener.Addr().String(), key.String()) + req, err := http.NewRequest(http.MethodGet, urlStr, nil) + require.NoError(t, err) + req.Header.Set("Accept", mediaTypeJSON) + + resp, err := http.DefaultClient.Do(req) + require.NoError(t, err) + + // Returns 500 Internal Server Error instead of misleading 200 with empty results + require.Equal(t, http.StatusInternalServerError, resp.StatusCode) + body, err := io.ReadAll(resp.Body) + require.NoError(t, err) + require.Contains(t, string(body), "not supported") + }) } // mockDHTRouter implements the router interface for testing diff --git a/server_routers.go b/server_routers.go index c919af5..ce7806f 100644 --- a/server_routers.go +++ b/server_routers.go @@ -71,7 +71,9 @@ func (r composableRouter) FindPeers(ctx context.Context, pid peer.ID, limit int) func (r composableRouter) GetClosestPeers(ctx context.Context, key cid.Cid) (iter.ResultIter[*types.PeerRecord], error) { if r.dht == nil { - return iter.ToResultIter(iter.FromSlice([]*types.PeerRecord{})), nil + // Return ErrNotSupported when no DHT is available (e.g., disabled via --dht=disabled CLI param). + // This returns HTTP 501 Not Implemented instead of misleading HTTP 200 with empty results. + return nil, routing.ErrNotSupported } return r.dht.GetClosestPeers(ctx, key) } diff --git a/server_test.go b/server_test.go index e4ffbb9..ee5a986 100644 --- a/server_test.go +++ b/server_test.go @@ -13,14 +13,14 @@ func TestCombineRouters(t *testing.T) { mockRouter := composableRouter{} // Check that combineRouters with DHT only returns sanitizeRouter - v := combineRouters(&bundledDHT{}, nil, nil, nil) + v := combineRouters(nil, &bundledDHT{}, nil, nil, nil) require.IsType(t, sanitizeRouter{}, v) // Check that combineRouters with delegated routers only returns parallelRouter - v = combineRouters(nil, nil, []router{mockRouter}, nil) + v = combineRouters(nil, nil, nil, []router{mockRouter}, nil) require.IsType(t, parallelRouter{}, v) // Check that combineRouters with both DHT and delegated routers returns parallelRouter - v = combineRouters(&bundledDHT{}, nil, []router{mockRouter}, nil) + v = combineRouters(nil, &bundledDHT{}, nil, []router{mockRouter}, nil) require.IsType(t, parallelRouter{}, v) }