Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/3535 multiple jsonrpc urls #3566

Merged

Conversation

640774n6
Copy link
Contributor

@640774n6 640774n6 commented Oct 31, 2021

Resolves #3535

Changes:

  • Added AdditionalRPCUrls option to IJsonRPCConfig
  • Added new class JsonRpcUrl to represent the data parsed from the config format
  • Added new interface IJsonRpcUrlCollection and implementation for managing all of the parsed JsonRpcUrls
  • Refactored JsonRpcContext to include optional JsonRpcUrl
  • Refactored JsonRpcRunner to use urls defined in JsonRpcUrlCollection
  • Refactored JsonRpc Startup to match connection ports to JsonRpcUrls which are passed along through to the service/processor via JsonRpcContext
  • Added port (int) parameter to the CreateClient() method of IWebSocketsModule. Needed to match the connection port to a JsonRpcUrl
  • Refactored IRpcModuleProvider/RpcModuleProvider Check() method to use JsonRpcContext instead of RpcEndpoint. If context.Url is not null: use JsonRpcUrl's enables modules list for check; Else: fallback on default EnabledModules check logic.
  • Updated unit tests to work with refactors

Types of changes

What types of changes does your code introduce?
Put an x in the boxes that apply

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation Update
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Other (please describe):

Testing

Requires testing

  • Yes
  • No

In case you checked yes, did you write tests??

  • Yes
  • No

Comments about testing , should you have some (optional)

  • Adding testing to ensure RpcModuleProvider.Check() uses enabled modules scoped to a url when provided
  • Added tests for JsonRpcUrl
  • Added tests for JsonRpcUrlCollection

Further comments (optional)

First PR to this project (hopefully first of many) so please enlighten me if I missed a step in the process. I tried to follow the existing coding styles in use.

Config file format should be backwards compatible and continue to work without presence of AdditionalRPCUrls config option.

I'm sure there will need to be some iteration to get this right, but figured it was a good starting point to get the conversation going.

Thanks!

@640774n6 640774n6 force-pushed the feature/3535-multiple-jsonrpc-urls branch from a5d4dcb to b2dbad3 Compare October 31, 2021 08:34
Copy link
Member

@dceleda dceleda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the implementation.
I've added a few minor comments.

src/Nethermind/Nethermind.JsonRpc/JsonRpcUrl.cs Outdated Show resolved Hide resolved
src/Nethermind/Nethermind.JsonRpc/JsonRpcUrl.cs Outdated Show resolved Hide resolved

public bool Equals(JsonRpcUrl other)
{
if (ReferenceEquals(null, other))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[minor]
if(other == null) {return false;}
?

if (url.RpcEndpoint == RpcEndpoint.None)
{
if (_logger.IsWarn)
_logger.Warn($"Additional JSON RPC URL '{url}' has web socket endpoint type and web sockets are not enabled; skipping...");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think Info would be better in this case.


if (context.Url != null) return context.Url.EnabledModules.Contains(result.ModuleType, StringComparer.InvariantCultureIgnoreCase) ? ModuleResolution.Enabled : ModuleResolution.Disabled;

return _enabledModules.Contains(result.ModuleType) == true ? ModuleResolution.Enabled : ModuleResolution.Disabled;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[minor] Why adding "== true" ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm actually not sure how that got in there. I usually don't do == true unless it's a nullable bool. It also wasn't in that line to begin with. Anyways, I will fix it.

return _enabledModules.Contains(result.ModuleType) ? ModuleResolution.Enabled : ModuleResolution.Disabled;
if ((result.Availability & context.RpcEndpoint) == RpcEndpoint.None) return ModuleResolution.EndpointDisabled;

if (context.Url != null) return context.Url.EnabledModules.Contains(result.ModuleType, StringComparer.InvariantCultureIgnoreCase) ? ModuleResolution.Enabled : ModuleResolution.Disabled;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should return ModuleResolution.EndpointDisabled for this case if it's not defined for the URL.

{
JsonRpcUrl jsonRpcUrl = _jsonRpcUrlCollection.FirstOrDefault(x => x.Port == port && x.RpcEndpoint.HasFlag(RpcEndpoint.WebSocket));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if I like how we configure these additional URLs and then actually we use only ports.
Maybe the config should contain only ports instead of scheme+host+port ?
@MarekM25 ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if I like how we configure these additional URLs and then actually we use only ports. Maybe the config should contain only ports instead of scheme+host+port ? @MarekM25 ?

No, it shouldn't contain only ports. We want to be able to set up the engine module for localhost only. However, other RPC modules could be public. That is the goal of this issue.

Copy link
Contributor Author

@640774n6 640774n6 Nov 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, since the urls are pass in to UseUrls() in the JsonRpcRunner.cs WebHostBuilder.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is guaranteed not to be null?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also potentially slow please use dictionary

@640774n6
Copy link
Contributor Author

640774n6 commented Nov 2, 2021

@dceleda Pushed code review suggestions

@@ -97,9 +97,9 @@ public ModuleResolution Check(string methodName, JsonRpcContext context)

if ((result.Availability & context.RpcEndpoint) == RpcEndpoint.None) return ModuleResolution.EndpointDisabled;

if (context.Url != null) return context.Url.EnabledModules.Contains(result.ModuleType, StringComparer.InvariantCultureIgnoreCase) ? ModuleResolution.Enabled : ModuleResolution.Disabled;
if (context.Url != null) return context.Url.EnabledModules.Contains(result.ModuleType, StringComparer.InvariantCultureIgnoreCase) ? ModuleResolution.Enabled : ModuleResolution.EndpointDisabled;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sorry I wasn't specific enough. I meant more smth like this:

            if (context.Url != null && !context.Url.EnabledModules.Contains(result.ModuleType, StringComparer.InvariantCultureIgnoreCase))
            {              
                return ModuleResolution.EndpointDisabled;
            }

The last line returns if a module is enabled/disabled at all. The preceding lines indicate if the module is enabled/disabled at the context level.

Copy link
Contributor Author

@640774n6 640774n6 Nov 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't it determine enabled/disabled solely off of the context enabled modules if context.Url is not null?

If I'm reading that logic right, I think it would end up hitting the last line to determine if it is enabled again using _enabledModules which comes from JsonRpcConfig.EnabledModules which is only there as a fallback for when Url is null (not http/websocket).

Copy link
Member

@dceleda dceleda Nov 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question. We have this logic in JsonRpcService

            return result switch
            {
                ModuleResolution.Unknown => ((int?) ErrorCodes.MethodNotFound, $"Method {methodName} is not supported"),
                ModuleResolution.Disabled => (ErrorCodes.InvalidRequest, $"{methodName} found but the containing module is disabled, consider adding module to JsonRpcConfig.EnabledModule"),
                ModuleResolution.EndpointDisabled => (ErrorCodes.InvalidRequest, $"{methodName} found but disabled for {context.RpcEndpoint}"),
                _ => (null, null)
            };

Which indicates that the Disabled switch is linked with JsonRpcConfig.EnabledModules .
I can imagine that we have a module enabled in this config (EnabledModules) but it's not enabled for the URL - then we want to return EndpointDisabled .
The question is how the system should behave if the module is NOT listed in EnabledModules but it's listed in the URL config. What is the priority? Should we still switch it ON for the URL or it should be disabled in general ?
@MarekM25 , @LukaszRozmej ?

Copy link
Contributor Author

@640774n6 640774n6 Nov 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it helps, the implementation was made with the requirement of keeping this backwards compatible, so JsonRpcConfig.Host/Port/WebSocketPort/EnabledModules is used to create the first url in the url collection. This way the config will still work as it did before these changes if a user does not want to use the AdditonalRpcUrls functionality.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed with @MarekM25 - both configurations (the old and the new one) are independent. So, it's good as it is - we can ignore my last suggestion :)

Copy link
Contributor Author

@640774n6 640774n6 Nov 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK cool. So just to confirm, the latest commit (changing it to return EndpointDisabled instead of Disabled), is correct?

Copy link
Contributor Author

@640774n6 640774n6 Nov 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm just thinking that following the same logic, EndpointDisabled would imply that the specific endpoint is disabled (net.peerCount, but not net.listening for example), when in fact it is the entire module (net) for that url, which is what Disabled represents. If that is the case, the error message in JsonRpcService might need to be reworded.

@LukaszRozmej
Copy link
Member

Hi @640774n6!
First let me thank you for fantastic contribution! Its great and the task wasn't simple.

Still I managed to find some things to potentially change which I left in code comments.
Maybe we also can add a RPC test that invokes something on non-default url, to prove it works correctly?

Other way we will definitely use this code.

src/Nethermind/Nethermind.JsonRpc/JsonRpcConfig.cs Outdated Show resolved Hide resolved
Comment on lines 340 to 341
ModuleResolution.Disabled => (ErrorCodes.InvalidRequest, $"{methodName} found but the containing module is disabled, consider adding module to JsonRpcConfig.EnabledModule"),
ModuleResolution.EndpointDisabled => (ErrorCodes.InvalidRequest, $"{methodName} found but disabled for {context.RpcEndpoint}"),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to enhance messages

Copy link
Contributor Author

@640774n6 640774n6 Nov 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. Anyone have any suggestions? If not, I can try and come up with something.

Comment on lines 29 to 30
private const string HttpEndpointValue = "http";
private const string WebSocketEndpointValue = "ws";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could use Uri.UriSchemeXXX instead of duplicating values, but they are standard so it doesn't matter. I wonder If we should support https and wss here to just for ease of use.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, that would be cleaner, but Uri.UriSchemeWs and Uri.UriScheme.Wss do not appear to be available until net6.0, but NethermindEth is currently targeting net5.0.

dotnet/runtime#35180

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was checking dissasembly and didn't catch they are internal 🤦‍♂️
image


string url = parts[0];
if (!Uri.TryCreate(url, UriKind.Absolute, out Uri? uri) ||
uri.Scheme != "http" ||
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Support http, https, ws, wss values based on Uri.UriSchemeXXX

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can add wss, and https but I don't think we can use Uri.UriSchemeXXX for all of them for the same reason in my comment on the previous MR review suggestion.

Copy link
Contributor Author

@640774n6 640774n6 Nov 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the actual scheme of the urls we pass into UseUrls() on the web host builder of JsonRpcRunner.cs needs to be http or https. I don't think ws or wss would work.

The format in AdditionalRpcUrls can allow the same url to be enabled for http/https or ws/wss like so: "https://127.0.0.1:1234|https,ws|eth,net,admin".

The only thing the web socket code does with it is check the port of the incoming request and makes sure a websocket RpcEndpoint flag is set for a url with that port. It doesn't care about the scheme of the url.

if (!Uri.TryCreate(url, UriKind.Absolute, out Uri? uri) ||
uri.Scheme != "http" ||
uri.Segments.Count() > 1 ||
uri.Port <= 0)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't Uri class handle this in parsing?

Copy link
Contributor Author

@640774n6 640774n6 Nov 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • uri.Scheme != "http": can be changed to allow https, but as mentioned in the comment above, wss and ws urls won't work with web host builder UseUrls().
  • uri.Segments.Count() > 1: ensures we don't get Uris with path components like: "http://127.0.0.1:80/something/else".
  • uri.Port <= 0: I just tested and Uri.TryCreate does parse correctly when port is 0 which should be invalid. So: "http://127.0.0.1:0" is successfully parsed. Negative numbers are not allowed by the parser, so I can just do a zero check to ensure it is not zero.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't we allow subpaths?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Web builder's UseUrls() doesn't allow subpaths. You would have to lookup the url based on port and path in app.Use(). Port would no longer be unique as you could have multiple urls with with the same port but different subpaths which might be kind of weird, but could be done if this is desired.

&& ctx.Connection.LocalPort == jsonRpcConfig.WebSocketsPort,
app.UseWhen(ctx =>
ctx.WebSockets.IsWebSocketRequest &&
jsonRpcUrlCollection.Any(x => x.Port == ctx.Connection.LocalPort && x.RpcEndpoint.HasFlag(RpcEndpoint.WebSocket)),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is potentially slow, please create a Dictionary of websocket supported jsonRcpUrl's

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. I will refactor JsonRpcUrlCollection to be backed by a Dictionary<int, JsonRpcUrl>() with port being the key.

@@ -144,13 +147,14 @@ long SerializeTimeoutException(IJsonRpcService service, Stream resultStream)
await ctx.Response.WriteAsync("Nethermind JSON RPC");
}

if (ctx.Connection.LocalPort == jsonRpcConfig.Port && ctx.Request.Method == "POST")
JsonRpcUrl jsonRpcUrl = jsonRpcUrlCollection.FirstOrDefault(x => x.Port == ctx.Connection.LocalPort && x.RpcEndpoint.HasFlag(RpcEndpoint.Http));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is potentially slow, please create a Dictionary of http supported jsonRcpUrl's

@@ -144,13 +147,14 @@ long SerializeTimeoutException(IJsonRpcService service, Stream resultStream)
await ctx.Response.WriteAsync("Nethermind JSON RPC");
}

if (ctx.Connection.LocalPort == jsonRpcConfig.Port && ctx.Request.Method == "POST")
JsonRpcUrl jsonRpcUrl = jsonRpcUrlCollection.FirstOrDefault(x => x.Port == ctx.Connection.LocalPort && x.RpcEndpoint.HasFlag(RpcEndpoint.Http));
if (ctx.Request.Method == "POST" && jsonRpcUrl != null)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor: reverse order of checks (or with dictionary use TryGetValue)

{
JsonRpcUrl jsonRpcUrl = _jsonRpcUrlCollection.FirstOrDefault(x => x.Port == port && x.RpcEndpoint.HasFlag(RpcEndpoint.WebSocket));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also potentially slow please use dictionary

@640774n6 640774n6 requested a review from LukaszRozmej November 7, 2021 07:31
@640774n6
Copy link
Contributor Author

640774n6 commented Nov 7, 2021

I took a pass at resolving the code review comments. I also attempted to reword the ModuleResolution.Disabled message, but will definitely not be offended if someone has something better 😄

I wouldn't mind adding an RPC test to ensure the new functionality works, but I am not sure where I would do that. This sounds like an integration test and I couldn't locate where this should be added.

Thanks!

Comment on lines 340 to 341
ModuleResolution.Disabled => (ErrorCodes.InvalidRequest, $"{methodName} found but the containing module is disabled for the url, consider adding module for additional url in JsonRpcConfig.AdditionalRpcUrls or to JsonRpcConfig.EnabledModules"),
ModuleResolution.EndpointDisabled => (ErrorCodes.InvalidRequest, $"{methodName} found but disabled for {context.RpcEndpoint}"),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, one thing still to add into the message now would be the url (or maybe just port?) for which it is disabled, so user can see that maybe he is querying wrong one.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah for sure. I will do that.

Comment on lines 29 to 30
private const string HttpEndpointValue = "http";
private const string WebSocketEndpointValue = "ws";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was checking dissasembly and didn't catch they are internal 🤦‍♂️
image


Add(url.Port, url);
}
catch (Exception)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we handle only FormatException here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It needs to catch any exception that JsonRpcUrl.Parse() can throw to allow it to recover, log the warning that the url parse failed, then skip it, and continue parsing the next url. Maybe the warning should be reworded from "not formatted correctly" to "could not be parsed"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

JsonRpcUrl.Parse() could also catch other exceptions and throw FormatExceptions to keep it consistent

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refactored to make JsonRpcUrl.Parse() thrown exceptions consistent by using FormatException; Also passing on relevant and helpful format exception messages in the log.

@640774n6
Copy link
Contributor Author

640774n6 commented Nov 8, 2021

Tested passing additional urls using the command line args. Looks like using a comma as part of the url format doesn't work because the config already does a Split(',') as AdditionalRPCUrls is a collection. I don't think a semi-colon is used by the config, so I vote we use a semi-colon as a delimiter for the RpcEndpoint part and the EnabledModules part.

For example instead of "http://127.0.0.1:8888|http,ws|net,web3,admin" it would be "http://127.0.0.1:8888|http;ws|net;web3;admin".

…at to support command line args; Hardened JsonRpcUrl parsing; Updated tests
@640774n6
Copy link
Contributor Author

640774n6 commented Nov 8, 2021

I believe this is ready for another review pass. Eventually we'll get it right 😄

  • JsonRpcUrl format now using semi-colon instead of comma for RpcEndpoints/EnabledModules as comma is already used by config.
  • JsonRpcUrl now throws FormatExceptions for any format related problems.
  • JsonRpcUrlCollection logs FormatException messages if a url throws FormatException providing more context to the user.
  • ModuleResolution.Disabled now includes url string in reworded log message.
  • Updated tests.

Copy link
Member

@LukaszRozmej LukaszRozmej left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good

ModuleResolution.Disabled => (ErrorCodes.InvalidRequest, $"{methodName} found but the containing module is disabled, consider adding module to JsonRpcConfig.EnabledModules"),
ModuleResolution.EndpointDisabled => (ErrorCodes.InvalidRequest, $"{methodName} found but disabled for {rpcEndpoint}"),
ModuleResolution.Disabled => (ErrorCodes.InvalidRequest, $"{methodName} found but the containing module is disabled for the url '{context.Url?.ToString() ?? string.Empty}', consider adding module in JsonRpcConfig.AdditionalRpcUrls for additional url, or to JsonRpcConfig.EnabledModules for default url"),
ModuleResolution.EndpointDisabled => (ErrorCodes.InvalidRequest, $"{methodName} found but disabled for {context.RpcEndpoint}"),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should add this url details to EndpointDisabled too

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Url added to EndpointDisabled message in latest commit

_logger.Info($"Additional JSON RPC URL packed value '{additionalRpcUrl}' format error: {fe.Message}; skipping...");
continue;
}
catch (Exception e)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still, do we want to catch any exception here? Is it really needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My original thinking was to allow it to recover from any exception on a per url basis so that if something unexpected occurs for one url, it doesn't stop it from parsing the rest of the urls. I guess we should only recover & continue from expected exceptions (FormatException). I can clean that up.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cleaned up catch-all in latest commit

@LukaszRozmej
Copy link
Member

@640774n6 Cool! Great code! We will do a bit of internal testing before merging as JSON RPC is crucial functionality.

The only thing that potentially could make this better is adding some more tests around this 'routing' of modules and endpoints.

@LukaszRozmej LukaszRozmej merged commit 6281f3f into NethermindEth:master Nov 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[TheMerge] Support more than one url for Json RPC/WebSockets
5 participants