waku-org / nwaku

Waku node and protocol.
Other
201 stars 53 forks source link

HTTP REST API: Debug and Relay APIs #727

Closed jm-clius closed 2 years ago

jm-clius commented 3 years ago

Background

nim-waku provides a JSON-RPC API for non-Nim clients. The original plan was to have this API be REST-like so that it could eventually map to an HTTP REST API, once a secure web server was implemented in Nim.

With the introduction of nim-presto, this now seems possible. Consequently nimbus-eth2 has added a REST API. nim-waku can follow suit.

Acceptance Criteria

In future we also want to add Store, Filter, Admin and Private API functionality for parity with the existing JSON-RPC API. This worked is tracked here: https://github.com/status-im/nwaku/issues/1076

LNSD commented 2 years ago

From my initial conversation with @jm-clius, one of the requirements for the HTTP RESTful API is to be as consistent as we can with nimbus HTTP REST RPC API. So I did a little analysis on the _nimbus beaconchain. These are some highlights of the nimbus implementation that we should follow in nwaku:

LNSD commented 2 years ago

HTTP RESTful is a widespread way to expose service APIs over HTTP. There is a standardized way of specifying a RESTful API named OpenAPI.

LNSD commented 2 years ago

While doing a first review of the current JSON-RPC API, seems straightforward the mapping between the RPC methods and the new REST API resources. There are some small details to discuss (e.g. keep the /waku/v2/ common path).

For example, the specification of the debug namespace RPC call using the OpenAPI spec format would be:

openapi: 3.1.0
info:
  title: Waku REST API - Debug
  version: 1.0.0

paths:
  /debug/info:  # <-- Path: namespace + resource
    get:
      summary: Get node info
      description: Retrieve information about a Waku v2 node
      tags:
        - Debug
      responses:
        '200':
          description: Information about a Waku v2 node
          content:
            application/json:
              schema:
                $ref: '#/components/schemas/WakuInfo'
        # TODO Specify the error format for 4XX and 5XX error codes

The idea is to have one OpenAPI spec file per "namespace" (e.g. debug, store, filter, etc.). And by using speccy's resolve command, merge the spec files into a single file to generate the client or the spec documentation.

LNSD commented 2 years ago

As one requirement is consistency with the nimbus codebase. So the nim HTTP REST library will be nim-presto. This library is relatively new and no OpenAPI client/server generator exists.

Indeed seems that the OpenAPI generator does not support nim server code generation (see the openAPI server generators list). There is just a "beta" code generator for nim client-side code.

For these reasons, we cannot benefit from this tool to generate the HTTP REST server handlers/stubs. This will have to be done manually. In the future, depending on the number of times the API spec is modified, might be interesting to implement a custom generator that allows us to automate the code boilerplate generation.

jakubgs commented 2 years ago

Consequently nimbus-eth2 has added a REST API.

@jm-clius It's much better than that, nimbus-eth2 has deprecated it's JSON-RPC API, because JSON-RPC is fucking garbage for interacting with as a human being or sysadmin(which arguably might not be as human):

jakubgs commented 2 years ago

As for what I'd want from a good REST API from point of view of a sysadmin, here are some things that come to mind:

Of course some of those things are more important than others.

jm-clius commented 2 years ago

Thanks for this @LNSD!

In general I agree with your approach and think it's a good idea to have the OpenAPI specification - this would make the API much more accessible for nwaku users and allow them to generate client code for whichever environment they're working in (or at least have access to readable, standardised docs).

e.g. keep the /waku/v2/ common path

This was something we added for the JSON-RPC API, but I think it's unlikely that we'll host APIs for different versions of waku on the same server in future and it makes the resource paths quite cumbersome. I'll feel comfortable removing this /waku/v2 from the common path of all resources. @oskarth wdyt? This was a conversation we had when starting work on the JSON-RPC API, but back then we may have imagined hosting v1 and v2 resources on the same host (e.g. a bridge). The v1 API, however, is used as is (without versioning) on the bridge and I don't foresee extending/changing it.

@LNSD, even though path versioning isn't a strictly idiomatic RESTful design, shall we keep the versioning for each sub-API, e.g. /debug/v1/info, or is there an easy alternative? This may just help us extend/change/break things with little impact in future.

For these reasons, we cannot benefit from this tool to generate the HTTP REST server handlers/stubs. This will have to be done manually.

Indeed. Manual code, unfortunately, which also means we'll have to maintain the OpenAPI spec to be sure it always matches what we implemented. A generator will be useful in future - hopefully more people have this need and have started working on this already. :)

jm-clius commented 2 years ago

Good wishlist from @jakubgs!

Although the JSON-RPC API provides a good minimal set of methods to implement, we can (and must) certainly extend with new useful methods (as we go along). Perhaps also worth taking a look at other Waku v2 APIs (e.g. the bindings API) and see if there are methods worth implementing that does not yet exist for nwaku.

LNSD commented 2 years ago

@jm-clius and I have agreed on doing first a prototype porting the Debug API from RPC to REST format in order to discover all the unknown issues.

LNSD commented 2 years ago

Diving more into the nimbus beacon_node REST API, I have realized that JSON Serialization (e.g. request unmarshaling, response marshaling) is not done using NIM's std JSON library. It is done by manually serializing things using the JSONWriter object (from status-im/nim-json-serialization) and by marshaling/unmarshalling data structures "manually" (see: eth2_rest_serialization.nim)

Based on what I can read in the nim-json-serialization repo's README:

Flexible JSON serialization not relying on run-time type information.

I guess that the reason behind this decision is to gain in performance. Maybe @Menduist can shed some more light on this decision.

In my opinion, if performance is a concern in nwaku's node REST API, then we should go with the JSONWriter approach. Otherwise, the std/json option should be enough. What do you think @jm-clius?

Menduist commented 2 years ago

The std json library is notoriously unoptimized and hard to use properly

The nim-*-ser repos instead rely on automatic marshaling and efficient parsing / dumping

They're sometime a bit of pain to use, since they rely on a lot of "magic" (generics, templates, macro), but the code generally end up cleaner and faster

Your call though, for a quick prototype std json is good enough :)

jm-clius commented 2 years ago

Mmm. I agree that for our first end-to-end POC using std/json would be good enough - we've been using std/json on the JSON-RPC API without issue. I don't foresee that we'd need a high performant REST API in the short term, but we'll probably make things easier for ourselves down the line if we follow nimbus in using nim-json-serialization earlier rather than later.

I may not have a good grasp of how much extra (initial) effort this would be? If it's merely a lot of copying and pasting I'd say go ahead, but if we think that it will be weeks of extra work I'd rather stay with the simpler std/json option.

arnetheduck commented 2 years ago

Nimbus beacon_node exposes the "control interface" in two forms: RPC and REST

RPC was removed recently (not that it matters greatly for this issue but ..)

There is a standardized way of specifying a RESTful API named OpenAPI.

The API that nimbus-eth2 implements is standardised / documented here: https://github.com/ethereum/beacon-APIs (including yaml specs from which in theory code could be generated)

std/json

Summary of performance and implementation bugs: https://github.com/status-im/nimbus-eth2/issues/2430 (we, and most of the Nim community, stays as far away from this module as possible :))

Serialization issues tend to be tricky to fix "afterwards" because you need to concurrently upgrade producers and consumers - eth2_rest_serialization looks messy but the mess is there so that we don't "accidentally" break the serialized output and make it incompatible.

arnetheduck commented 2 years ago

API versioning

in eth2, the key point was to version each request separately - this has worked well so far - the other important point to specify is an upgrade policy: what changes are allowed to each endpoint which don't require a version upgrade - adding "optional" fields tends to be the common ground for most people.,

LNSD commented 2 years ago

I have been busy with the JSON serialization part. I am still learning Nim ๐Ÿ˜

I have implemented the serialization/deserialization part of the POC using the nim-json-serialization library as nimbus does and based on the facts/comments that @arnetheduck and @Menduist have provided above.

I have opened a Draft PR to get feedback about this. See: #988

LNSD commented 2 years ago

Update on this issue state. The REST POC is almost finished:

LNSD commented 2 years ago

For the record. During the POC development, some limitations were found:

E2E endpoint testing

Typically, the preferred way of testing the HTTP REST handlers logic is by doing unit tests and providing a test double (usually a mock or a stub) for the "dependencies" of the handler. This approach enables testing the logic in an isolated way and also the corner cases (e.g. using fault injection techniques).

The debug info handler, /debug/v1/info endpoint, depends on WakuNode type. Mocking the whole node implementation is not feasible. So we have decided to follow the same approach used to test the JSON-RPC: testing the different endpoints in an End-to-end fashion.

nim-presto rest macro

The nim-presto library client approach is based on the rest macro. One can define the client method in a declarative way:

proc debugInfoV1*(): DebugWakuInfo {.rest, endpoint: "/debug/v1/info", meth: HttpMethod.MethodGet.}

Together with the annotated method, the deserialization logic between the openArray[byes] and the client method return type should be provided too:

proc decodeBytes*(t: typedesc[DebugWakuInfo], data: openArray[byte], contentType: string): RestResult[DebugWakuInfo] =
  if MediaType.init(contentType) != MIMETYPE_JSON:
    error "Unsupported respose contentType value", contentType = contentType
    return err("Unsupported response contentType")

  let decoded = ?decodeFromJsonBytes(DebugWakuInfo, data)
  return ok(decoded) 

This macro implementation has a limitation: a constant cannot be used as endpoint value while declaring the client method. It only accepts a string literal. See nim-presto's client.nim#l156-L169.

LNSD commented 2 years ago

The POC PR is ready to review here: #988

LNSD commented 2 years ago

Next step: Port a complex API from JSON-RPC to REST (See Relay API at 16/WAKU2-RPC).

LNSD commented 2 years ago

The following issue has surfaced in the nwaku discord channel: https://github.com/status-im/nwaku/issues/754

The canary/health-check service may be part of the work after we support all the current JSON-RPC APIs with the new REST API.

In the case there are business requirements to increase the #754 issue priority, we could tackle this issue as part of the next release.

jakubgs commented 2 years ago

The canary/health-check service may be part of the work after we support all the current JSON-RPC APIs with the new REST API.

The canary has nothing to do with JSON-RPC or REST API. It's supposed to check availability and functionality of a Waku node, not availability of Waku node JSON-RPC or REST API.

LNSD commented 2 years ago

Now we support besides Debug API also the Relay API. Two PR have been open with the latest changes:

There is still some integration work pending between these changes and the current Waku node logic (e.g. documentation, configuration options, rest server startup, etc.). Having these two APIs, the next step is to integrate them with the waku v2 node code so we can start testing it in the nightly builds ๐Ÿ˜

LNSD commented 2 years ago

In order to start testing the REST API endpoints, the integration work between the REST HTTP server and the Waku node has been advanced. Please check the following PR for details: #1022

jm-clius commented 2 years ago

Basic Debug, Relay functionality added and being released in v0.11. Remaining work for subsequent releases tracked here: https://github.com/status-im/nwaku/issues/1076

jakubgs commented 2 years ago

Are there any docs on the available API calls?