-
Notifications
You must be signed in to change notification settings - Fork 461
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
Feature/3535 multiple jsonrpc urls #3566
Conversation
a5d4dcb
to
b2dbad3
Compare
There was a problem hiding this 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.
|
||
public bool Equals(JsonRpcUrl other) | ||
{ | ||
if (ReferenceEquals(null, other)) |
There was a problem hiding this comment.
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..."); |
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[minor] Why adding "== true" ?
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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)); |
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
@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; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
Hi @640774n6! Still I managed to find some things to potentially change which I left in code comments. Other way we will definitely use this code. |
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}"), |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
private const string HttpEndpointValue = "http"; | ||
private const string WebSocketEndpointValue = "ws"; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
||
string url = parts[0]; | ||
if (!Uri.TryCreate(url, UriKind.Absolute, out Uri? uri) || | ||
uri.Scheme != "http" || |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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)), |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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)); |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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)); |
There was a problem hiding this comment.
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
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! |
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}"), |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
private const string HttpEndpointValue = "http"; | ||
private const string WebSocketEndpointValue = "ws"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
||
Add(url.Port, url); | ||
} | ||
catch (Exception) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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"
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
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
I believe this is ready for another review pass. Eventually we'll get it right 😄
|
There was a problem hiding this 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}"), |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
…ointDisabled message
@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. |
Resolves #3535
Changes:
Types of changes
What types of changes does your code introduce?
Put an
x
in the boxes that applyTesting
Requires testing
In case you checked yes, did you write tests??
Comments about testing , should you have some (optional)
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!