Reject `octet-stream` content type when we expect `json`

[eserilev]

Issue Addressed

Closes #5855

Proposed Changes

The issue is a side effect from https://github.com/sigp/lighthouse/pull/4575 . We ignore content type headers for endpoints that expect a json request body. Theres really only two content-type headers that are relevant to us json and octet-stream.

This PR adds the following logic: If an endpoint that only supports json attempts to serve a request with an octet-stream content type header, it will reject the request with a 415 status code. All other content-type headers are left unchecked.

[eserilev]

after some manual testing were still not returning a 415 status code. trying to figure out why

[eserilev]

The main problem was due to the way warp chains errors and how we check for those errors. Heres an example error response that gets fed into warp_utils::handle_rejection

Rejection([UnsupportedMediaType("The request's content-type is not supported"), CustomBadRequest("Unsupported endpoint version: v1"), CustomBadRequest("Unsupported endpoint version: v1"), CustomBadRequest("Unsupported endpoint version: v1"), CustomBadRequest("Unsupported endpoint version: v1")])

We have a chain of if-else statements in handle_rejection that checks which rejection is contained in the error. Depending on the order of the if-else's , different error messages may be returned to the client. I resolved the issue by moving the unsupported media type check higher up in the if-else chain.

Additionally I've added a test to check for a 415 response error on an endpoint that expects a json request body. This test should hopefully catch any regressions that may potentially be introduced down the line.

I should have tested this earlier before opening the PR, sorry about that!

[eserilev]

also tested a few endpoints manually. seems to be working as intended

[nflaig]

also tested a few endpoints manually. seems to be working as intended

Happy to test this if you can provide me an image, running this with Lodestar in ssz-only mode should cover most (relevant) apis

[eserilev]

@nflaig maybe we can try testing in kurtosis? I can try spinning up a lodestar + lighthouse testnet w/ lodestar in ssz only mode

[nflaig]

@nflaig maybe we can try testing in kurtosis? I can try spinning up a lodestar + lighthouse testnet w/ lodestar in ssz only mode

That was my idea, just need a lighthouse image

This is the config that works with lighthouse latest but adding vc_extra_params will break attestations, fetching duties , etc.

participants:
  # Lighthouse
  - el_type: geth
    el_image: ethereum/client-go:stable
    cl_type: lodestar
    cl_image: nflaig/lodestar:ssz-api
    vc_type: lighthouse
    vc_image: sigp/lighthouse:latest
    count: 1
  - el_type: geth
    el_image: ethereum/client-go:stable
    cl_type: lighthouse
    cl_image: sigp/lighthouse:latest
    vc_type: lodestar
    vc_image: nflaig/lodestar:ssz-api
   # this will configure Lodestar to send ssz requests for all routes, Lighthouse (and spec) does not
   # support this but if proper 415 status code is returned Lodestar will retry with json and there should
   # be no erros on Lodestar VC side

   # vc_extra_params:
   #  - --http.requestWireFormat=ssz
    count: 1

see complete config

[eserilev]

@nflaig thanks for the config! heres a lighthouse image you can use to test these changes: eserilev/lighthouse-415:latest. i'll also be spinning up a testnet shortly.

[nflaig]

eserilev/lighthouse-415:latest

thanks for the image but I had issues testing with it as it's arm64 only, and I noticed my image is amd64 only ...

Maybe you can give it a try, have build a multiarch image (nflaig/lodestar:ssz-api-multi) and a arm64 only image (nflaig/lodestar:ssz-api-arm64)

[eserilev]

hey @nflaig I finally got around to running these changes on kurtosis. I ended up using this grafana dashboard to view vc metrics. I think the relevant graph is this one

Now that lighthouse returns 415's, when appropriate, I'm not seeing any rest api error metrics. When i ran a kurtosis testnet without the fixes in this PR, i was seeing some error metrics get populated in that graph.

[nflaig]

Now that lighthouse returns 415's, when appropriate, I'm not seeing any rest api error metrics. When i ran a kurtosis testnet without the fixes in this PR, i was seeing some error metrics get populated in that graph.

Awesome, thanks for testing. No data on the panel is a great indicator that it works for all apis

[jimmygchen]

@mergify queue

[mergify[bot]]

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at 69d84e785b9dad3789b613837019a67730c5b475