feat: add syncing_status query parameter to eth/v1/node/health (#5790)

* Add syncing_status query parameter to eth/v1/node/health

* Always validate value of syncing_status

* Add 400 to getHealth error responses

* Set http error status as client response status

* Check status in test cases instead of error code

* Fix appending '?' to URL if query params are omitted

* Use composite reponse type from the server handlers

* Revert "Use composite reponse type from the server handlers"

This reverts commit e1cbec4.

* Remove workaround in generic server test

* Remove request / response from server api type

* Do not forward request / response to API implementation

* Set getHealth status code in route handler

---------

Co-authored-by: Nazar Hussain <nazarhussain@gmail.com>

packages/api/src/beacon/routes/node.ts

@@ -70,6 +70,10 @@ export enum NodeHealth {
   NOT_INITIALIZED_OR_ISSUES = HttpStatusCode.SERVICE_UNAVAILABLE,
 }
 
+export type NodeHealthOptions = {
+  syncingStatus?: number;
+};
+
 /**
  * Read information about the beacon node.
  */
@@ -120,13 +124,13 @@ export type Api = {
   /**
    * Get health check
    * Returns node health status in http status codes. Useful for load balancers.
-   *
-   * NOTE: This route does not return any value
    */
-  getHealth(): Promise<
+  getHealth(
+    options?: NodeHealthOptions
+  ): Promise<
     ApiClientResponse<
       {[HttpStatusCode.OK]: void; [HttpStatusCode.PARTIAL_CONTENT]: void},
-      HttpStatusCode.SERVICE_UNAVAILABLE
+      HttpStatusCode.BAD_REQUEST | HttpStatusCode.SERVICE_UNAVAILABLE
     >
   >;
 };
@@ -150,7 +154,7 @@ export type ReqTypes = {
   getPeerCount: ReqEmpty;
   getNodeVersion: ReqEmpty;
   getSyncingStatus: ReqEmpty;
-  getHealth: ReqEmpty;
+  getHealth: {query: {syncing_status?: number}};
 };
 
 export function getReqSerializers(): ReqSerializers<Api, ReqTypes> {
@@ -171,7 +175,13 @@ export function getReqSerializers(): ReqSerializers<Api, ReqTypes> {
     getPeerCount: reqEmpty,
     getNodeVersion: reqEmpty,
     getSyncingStatus: reqEmpty,
-    getHealth: reqEmpty,
+    getHealth: {
+      writeReq: (options) => ({
+        query: options?.syncingStatus !== undefined ? {syncing_status: options.syncingStatus} : {},
+      }),
+      parseReq: ({query}) => [{syncingStatus: query.syncing_status}],
+      schema: {query: {syncing_status: Schema.Uint}},
+    },
   };
 }
 

packages/api/src/beacon/server/node.ts

@@ -1,11 +1,11 @@
 import {ChainForkConfig} from "@lodestar/config";
 import {Api, ReqTypes, routesData, getReturnTypes, getReqSerializers} from "../routes/node.js";
 import {ServerRoutes, getGenericJsonServer} from "../../utils/server/index.js";
-import {HttpStatusCode} from "../../utils/client/httpStatusCode.js";
 import {ServerApi} from "../../interfaces.js";
 
 export function getRoutes(config: ChainForkConfig, api: ServerApi<Api>): ServerRoutes<ServerApi<Api>, ReqTypes> {
-  // All routes return JSON, use a server auto-generator
+  const reqSerializers = getReqSerializers();
+
   const serverRoutes = getGenericJsonServer<ServerApi<Api>, ReqTypes>(
     {routesData, getReturnTypes, getReqSerializers},
     config,
@@ -18,14 +18,11 @@ export function getRoutes(config: ChainForkConfig, api: ServerApi<Api>): ServerR
     getHealth: {
       ...serverRoutes.getHealth,
       handler: async (req, res) => {
-        try {
-          await api.getHealth();
-          res.raw.writeHead(HttpStatusCode.OK);
-          res.raw.end();
-        } catch (e) {
-          res.raw.writeHead(HttpStatusCode.INTERNAL_SERVER_ERROR);
-          res.raw.end();
-        }
+        const args = reqSerializers.getHealth.parseReq(req);
+        // Note: This type casting is required as per route definition getHealth
+        // does not return a value but since the internal API does not have access
+        // to response object it is required to set the HTTP status code here.
+        res.statusCode = (await api.getHealth(...args)) as unknown as number;
       },
     },
   };

packages/api/src/interfaces.ts

@@ -7,7 +7,7 @@ export type APIClientHandler = (...args: any) => PromiseLike<ApiClientResponse>;
 export type APIServerHandler = (...args: any) => PromiseLike<unknown>;
 
 export type ApiClientSuccessResponse<S extends keyof any, T> = {ok: true; status: S; response: T; error?: never};
-export type ApiClientSErrorResponse<S extends Exclude<HttpStatusCode, HttpSuccessCodes>> = {
+export type ApiClientErrorResponse<S extends Exclude<HttpStatusCode, HttpSuccessCodes>> = {
   ok: false;
   status: S;
   response?: never;
@@ -18,17 +18,16 @@ export type ApiClientResponse<
   E extends Exclude<HttpStatusCode, HttpSuccessCodes> = Exclude<HttpStatusCode, HttpSuccessCodes>,
 > =
   | {[K in keyof S]: ApiClientSuccessResponse<K, S[K]>}[keyof S]
-  | {[K in E]: ApiClientSErrorResponse<K>}[E]
-  | ApiClientSErrorResponse<HttpStatusCode.INTERNAL_SERVER_ERROR>;
+  | {[K in E]: ApiClientErrorResponse<K>}[E]
+  | ApiClientErrorResponse<HttpStatusCode.INTERNAL_SERVER_ERROR>;
 
 export type ApiClientResponseData<T extends ApiClientResponse> = T extends {ok: true; response: infer R} ? R : never;
 
-export type GenericRequestObject = Record<string, unknown>;
-export type GenericResponseObject = {code: (code: number) => void};
+export type GenericOptions = Record<string, unknown>;
 
 export type ServerApi<T extends Record<string, APIClientHandler>> = {
   [K in keyof T]: (
-    ...args: [...args: Parameters<T[K]>, req?: GenericRequestObject, res?: GenericResponseObject]
+    ...args: [...args: Parameters<T[K]>, opts?: GenericOptions]
   ) => Promise<ApiClientResponseData<Resolves<T[K]>>>;
 };
 

packages/api/src/utils/client/client.ts

@@ -26,7 +26,7 @@ export function getFetchOptsSerializer<Fn extends (...args: any) => any, ReqType
     return {
       url: urlFormater(req.params ?? {}),
       method: routeDef.method,
-      query: req.query,
+      query: Object.keys(req.query ?? {}).length ? req.query : undefined,
       body: req.body as unknown,
       headers: req.headers,
       routeId,
@@ -80,14 +80,17 @@ export function generateGenericJsonClient<
         }
       } catch (err) {
         if (err instanceof HttpError) {
-          return {ok: false, error: {code: err.status, message: err.message, operationId: routeId}} as ReturnType<
-            Api[keyof Api]
-          >;
+          return {
+            ok: false,
+            status: err.status,
+            error: {code: err.status, message: err.message, operationId: routeId},
+          } as ReturnType<Api[keyof Api]>;
         }
 
         if (err instanceof TimeoutError) {
           return {
             ok: false,
+            status: HttpStatusCode.INTERNAL_SERVER_ERROR,
             error: {code: HttpStatusCode.INTERNAL_SERVER_ERROR, message: err.message, operationId: routeId},
           } as ReturnType<Api[keyof Api]>;
         }

packages/api/test/unit/beacon/testData/node.ts

@@ -52,7 +52,7 @@ export const testData: GenericServerTestCases<Api> = {
     res: {data: {headSlot: "1", syncDistance: "2", isSyncing: false, isOptimistic: true, elOffline: false}},
   },
   getHealth: {
-    args: [],
+    args: [{syncingStatus: 206}],
     res: undefined,
   },
 };

packages/beacon-node/src/api/impl/node/index.ts

@@ -66,18 +66,29 @@ export function getNodeApi(
       return {data: sync.getSyncStatus()};
     },
 
-    async getHealth(_req, res) {
+    async getHealth(options) {
+      const syncingStatus = options?.syncingStatus;
+
+      if (syncingStatus != null && (syncingStatus < 100 || syncingStatus > 599)) {
+        throw new ApiError(400, `Invalid syncing status code: ${syncingStatus}`);
+      }
+
+      let healthStatus: number;
+
       if (sync.getSyncStatus().isSyncing) {
         // 206: Node is syncing but can serve incomplete data
-        res?.code(routes.node.NodeHealth.SYNCING);
+        healthStatus = syncingStatus ?? routes.node.NodeHealth.SYNCING;
       } else {
         // 200: Node is ready
-        res?.code(routes.node.NodeHealth.READY);
+        healthStatus = routes.node.NodeHealth.READY;
       }
       // else {
       //   503: Node not initialized or having issues
       //   NOTE: Lodestar does not start its API until fully initialized, so this status can never be served
       // }
+
+      // Health status is returned to allow route handler to set it as HTTP status code
+      return healthStatus as unknown as void;
     },
   };
 }

packages/beacon-node/test/e2e/api/impl/beacon/node/endpoints.test.ts

@@ -34,7 +34,7 @@ describe("beacon node api", function () {
         chain: {blsVerifyAllMainThread: true},
       },
       validatorCount,
-      logger: testLogger("Node-A", {level: LogLevel.info}),
+      logger: testLogger("Node-Synced", {level: LogLevel.info}),
     });
     client = getClient({baseUrl: `http://127.0.0.1:${restPort}`}, {config});
   });
@@ -65,9 +65,8 @@ describe("beacon node api", function () {
     });
 
     it("should return 'el_offline' as 'true' when EL not available", async () => {
-      // Close first instance
-      await bn.close();
-      bn = await getDevBeaconNode({
+      const portElOffline = 9597;
+      const bnElOffline = await getDevBeaconNode({
         params: {
           ...chainConfigDef,
           // eslint-disable-next-line @typescript-eslint/naming-convention
@@ -82,18 +81,19 @@ describe("beacon node api", function () {
           api: {
             rest: {
               enabled: true,
-              port: restPort,
+              port: portElOffline,
             },
           },
           chain: {blsVerifyAllMainThread: true},
         },
         validatorCount: 5,
-        logger: testLogger("Node-A", {level: LogLevel.info}),
+        logger: testLogger("Node-EL-Offline", {level: LogLevel.info}),
       });
+      const clientElOffline = getClient({baseUrl: `http://127.0.0.1:${portElOffline}`}, {config});
 
       // To make BN communicate with EL, it needs to produce some blocks and for that need validators
       const {validators} = await getAndInitDevValidators({
-        node: bn,
+        node: bnElOffline,
         validatorClientCount: 1,
         validatorsPerClient: validatorCount,
         startIndex: 0,
@@ -102,12 +102,75 @@ describe("beacon node api", function () {
       // Give node sometime to communicate with EL
       await sleep(chainConfigDef.SECONDS_PER_SLOT * 2 * 1000);
 
-      const res = await client.node.getSyncingStatus();
+      const res = await clientElOffline.node.getSyncingStatus();
       ApiError.assert(res);
 
       expect(res.response.data.elOffline).to.eql(true);
 
       await Promise.all(validators.map((v) => v.close()));
+      await bnElOffline.close();
+    });
+  });
+
+  describe("getHealth", () => {
+    const portSyncing = 9598;
+
+    let bnSyncing: BeaconNode;
+    let clientSyncing: Api;
+
+    before(async () => {
+      bnSyncing = await getDevBeaconNode({
+        params: chainConfigDef,
+        options: {
+          // Node won't consider itself synced without peers
+          sync: {isSingleNode: false},
+          api: {
+            rest: {
+              enabled: true,
+              port: portSyncing,
+            },
+          },
+          chain: {blsVerifyAllMainThread: true},
+        },
+        validatorCount,
+        logger: testLogger("Node-Syncing", {level: LogLevel.info}),
+      });
+      clientSyncing = getClient({baseUrl: `http://127.0.0.1:${portSyncing}`}, {config});
+      // Must at least wait for one slot else node considers itself synced pre/at genesis
+      await sleep(chainConfigDef.SECONDS_PER_SLOT * 1000);
+    });
+
+    after(async () => {
+      await bnSyncing.close();
+    });
+
+    it("should return 200 status code if node is ready", async () => {
+      const res = await client.node.getHealth();
+      expect(res.status).to.equal(200);
+    });
+
+    it("should return 206 status code if node is syncing", async () => {
+      const res = await clientSyncing.node.getHealth();
+      expect(res.status).to.equal(206);
+    });
+
+    it("should return custom status code from 'syncing_status' query parameter if node is syncing", async () => {
+      const statusCode = 204;
+      const res = await clientSyncing.node.getHealth({syncingStatus: statusCode});
+      expect(res.status).to.equal(statusCode);
+    });
+
+    it("should only use status code from 'syncing_status' query parameter if node is syncing", async () => {
+      const res = await client.node.getHealth({syncingStatus: 204});
+      expect(res.status).to.equal(200);
+    });
+
+    it("should return 400 status code if value of 'syncing_status' query parameter is invalid", async () => {
+      const res = await clientSyncing.node.getHealth({syncingStatus: 99});
+      expect(res.status).to.equal(400);
+
+      const resp = await clientSyncing.node.getHealth({syncingStatus: 600});
+      expect(resp.status).to.equal(400);
     });
   });
 });