node: serve JSON-RPC on all unused HTTP paths

[renaynay]

This PR fixes an issue where an RPC call that is not on the root path results in a 404 Not Found error. Instead, the request will be pattern-matched against the mux router to see if any registered paths match the request path, and if not, the server will fall back to JSON-RPC. This will allow RPC requests with an additional path to still be served.

[holiman]

cc @ryanschneider

[ryanschneider]

lgtm at first blush but am just checking it in my phone atm. One thing I like is this completely removes the .RequestURI comparison which I also noticed was causing query strings to fail to route to the RPC server (e.g. a POST to /?foo=bar would 404).

My only concern is if sending to the mux first as a precheck beforehand has any sort of performance regression, I'll try out this change in the morning and see if it has any obvious change it performance/pprof on one of our canary nodes.

[ryanschneider]

I ran a mix of random state-related RPCs from a sample of production traffic against our slightly forked version of geth both without and with this patch applied, and actually your PR ran faster than the old one:

Baseline:

Summary:
  Total:	30.0628 secs
  Slowest:	1.5710 secs
  Fastest:	0.0004 secs
  Average:	0.0173 secs
  Requests/sec:	2887.2512

Latency distribution:
  10% in 0.0032 secs
  25% in 0.0060 secs
  50% in 0.0102 secs
  75% in 0.0198 secs
  90% in 0.0352 secs
  95% in 0.0479 secs
  99% in 0.1243 secs

With this PR merged in:

Summary:
  Total:	30.0280 secs
  Slowest:	0.6459 secs
  Fastest:	0.0004 secs
  Average:	0.0137 secs
  Requests/sec:	3649.6590

Latency distribution:
  10% in 0.0025 secs
  25% in 0.0050 secs
  50% in 0.0090 secs
  75% in 0.0165 secs
  90% in 0.0272 secs
  95% in 0.0377 secs
  99% in 0.0820 secs

Anyways I think the key takeaway is that this definitely does not have any appreciable negative impact on RPC performance. I suspect the speed boost was due to the short nature of the test and the distinct possibility that the node was processing more complex blocks or devp2p traffic or what not during the first run.

So in short great work @renaynay and lgtm! Thanks @holiman for letting me know about this PR, it'll be one less patch for us to keep in our fork going forward.

[renaynay]

This is great to hear, thanks for the information @ryanschneider

[fjl]

We discussed this a bit last couple days, and I'm not super happy to include this change. It does work, but the behavior also gets a bit unpredictable. For example, if you enable GraphQL, JSON-RPC will not be served on /graphql and RPC requests sent to that path will trigger a GraphQL-specific error. But without GraphQL, JSON-RPC requests against that path will work, and GraphQL requests sent to that will return a JSON-RPC error.

I think it would be better to restrict JSON-RPC to a single path. @ryanschneider AFAIK, your issue with this was mostly that AWS load balancing does not support changing the path. Can you give us some insight where the path is coming from in your case? I'm hoping it can be solved in a different way.

[ryanschneider]

Without going too deep into specifics, we have multiple groups of nodes behind a single load balancer. While this isn't exactly how we are setup, imagine that some of our mainnet archive nodes are on a URL like http://internal-loadbalancer/mainnet/archive. Since AWS ALBs don't support rewriting the request, the request to the node is still POST /mainnet/archive. This worked up to 1.9.12 but required us to patch versions .13 and higher.

It could be worked around by running a separate local reverse proxy on the node (e.g. nginx with a rewrite rule) but that seems like a lot of extra devops surface when we already have a proxy in front of the internal ALB in our stack. Or we could work around the ALB limitation by using Host headers or DNS names instead of URL paths. The issue isn't really that there aren't other ways to do it, it's that having geth behave like it did before .12 is the least significant change, though I do understand your concern about unexpected behavior.

What about a compromise where the "any path" behavior only works if h.mux is "empty" (that is no calls to .RegisterHandler have been made)? That would behave just like .12 when graphQL is disabled, and trigger the new behavior only once graphQL or some other new handler is added. And, as mentioned, the PR fixes the odd behavior that POST /?foo=bar returns a 404 as well (because of the overly strict .RequestURI check vs. parsing the URL.Path).

[ryanschneider]

@fjl or @renaynay any thoughts on this compromise approach #21826 (comment) ?

What about a compromise where the "any path" behavior only works if h.mux is "empty" (that is no calls to .RegisterHandler have been made)? That would behave just like .12 when graphQL is disabled, and trigger the new behavior only once graphQL or some other new handler is added. And, as mentioned, the PR fixes the odd behavior that POST /?foo=bar returns a 404 as well (because of the overly strict .RequestURI check vs. parsing the URL.Path).

[fjl]

Sorry for not replying, still thinking about this.

[fjl]

I have a better idea. Instead of serving JSON-RPC requests on all unmatched paths, we can add a flag that lets you set the path of the RPC handler (or path prefix, if necessary). @ryanschneider this would solve your case, since the path is specific to each geth instance, right?

[ryanschneider]

@fjl ya that would work. We would probably run with a wildcard prefix like / since we tend to want to move geth nodes around without having to reconfigure them so as long as that was a supported prefix this idea would work for us.

[renaynay]

Okay sounds good. On holiday now but will implement this ASAP.

[ryanschneider]

Awesome, so I'm not 100% familiar with this code but I think that this approach can actually lead to a simpler codebase, since the cleanest thing to do would probably be "register" the Graph QL, GraphQL UI, HTTP JSONRPC and Websocket JSONRPC handlers all the same way, which could allow httpHandler.ServeHTTP to just be a mux lookup (though probably still with a special case for isWebsocket).

[renaynay]

Option 1: https://github.com/renaynay/go-ethereum/pull/31/files
Option 2: (replace mux router with gorilla mux) renaynay#32

@fjl

[fjl]

Closing in favor of #22184