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

JSON-RPC: Add option to specify bind address for server #2917

Merged
merged 1 commit into from
Nov 10, 2022

Conversation

Rob-NY
Copy link
Contributor

@Rob-NY Rob-NY commented Oct 17, 2022

Short description of changes

Adds a command line switch to specify the bind address for the RPC server. Today this address is hard-coded to the loopback interface. The loopback remains the default, but can be overridden with this new command line option.

CHANGELOG: RPC: Added new command line switch (--jsonrpcbindip) to specify the bind address for the RPC server.

Context: Fixes an issue?

Does not fix a reported issue, but does address that there is no way to change the bind address without recompiling.

Does this change need documentation? What needs to be documented and how?

If accepted, will update doc repo.

Status of this Pull Request

What is missing until this pull request can be merged?

Tested on Linux only.

Checklist

  • I've verified that this Pull Request follows the general code principles
  • I tested my code and it does what I want
  • My code follows the style guide
  • I waited some time after this Pull Request was opened and all GitHub checks completed without errors.
  • I've filled all the content above

@hoffie hoffie added this to the Release 3.10.0 milestone Oct 17, 2022
@hoffie hoffie added the needs documentation PRs requiring documentation changes or additions label Oct 17, 2022
@ann0see
Copy link
Member

ann0see commented Oct 17, 2022

This could also make --jsonrpcport redundant, if it supports a port too.

@Rob-NY
Copy link
Contributor Author

Rob-NY commented Oct 17, 2022

This could also make --jsonrpcport redundant, if it supports a port too.

It does not support a port at this time. Moreover, I did not change the fact that specifying the port is the switch that turns on/off RPC. This is how it was before, just defaulting to the loopback.

Copy link
Member

@hoffie hoffie left a comment

Choose a reason for hiding this comment

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

Minor nit, looks good to me in general and works for me. Thanks!

I know that we might not be consistent regarding this in the existing code base, but it might make sense to error out if --jsonrpcbind(ip) is given, but --jsonrpcport isn't (i.e., JSON-RPC is not enabled at all). Otherwise the option is ignored silently.

src/main.cpp Outdated Show resolved Hide resolved
@pljones
Copy link
Collaborator

pljones commented Oct 17, 2022

Same approach as --port and --serverbindip. One sets the listening port (but doesn't interfere with the listening address), the other sets the listening address (but doesn't touch the listening port). Maybe have it as --jsonrpcbindip (i.e. with the ip) to match the server one.

@hoffie
Copy link
Member

hoffie commented Oct 17, 2022

This could also make --jsonrpcport redundant, if it supports a port too.

I thought about it as well, but considering that it does break CLI flag compatibility (which would be ok, as this is still experimental), it would not be consistent with the existing flags for handling ports/bind addresses either (--serverbindip). Therefore, I'd be in favor of doing what this PR does already.

@hoffie hoffie changed the title Add option to specify bind address for RPC server JSON-RPC: Add option to specify bind address for server Oct 17, 2022
@Rob-NY
Copy link
Contributor Author

Rob-NY commented Oct 18, 2022

Same approach as --port and --serverbindip. One sets the listening port (but doesn't interfere with the listening address), the other sets the listening address (but doesn't touch the listening port). Maybe have it as --jsonrpcbindip (i.e. with the ip) to match the server one.

Pushed an update to have the RPC IP and Port match the naming convention for server.

@hoffie
Copy link
Member

hoffie commented Oct 25, 2022

I'm mostly happy with this PR, but unless someone opposes, I'd prefer to have the validation implemented which is mentioned here:

I know that we might not be consistent regarding this in the existing code base, but it might make sense to error out if --jsonrpcbind(ip) is given, but --jsonrpcport isn't (i.e., JSON-RPC is not enabled at all). Otherwise the option is ignored silently.

@Rob-NY
Copy link
Contributor Author

Rob-NY commented Nov 2, 2022

I'm mostly happy with this PR, but unless someone opposes, I'd prefer to have the validation implemented which is mentioned here:

I know that we might not be consistent regarding this in the existing code base, but it might make sense to error out if --jsonrpcbind(ip) is given, but --jsonrpcport isn't (i.e., JSON-RPC is not enabled at all). Otherwise the option is ignored silently.

Pushed an update with additional error checking both for "empty" addresses and using QHostAddress as is done consistently elsewhere. This should wrap up this PR, let me know if otherwise.

@Rob-NY Rob-NY closed this Nov 2, 2022
@Rob-NY Rob-NY reopened this Nov 2, 2022
@Rob-NY
Copy link
Contributor Author

Rob-NY commented Nov 3, 2022

Docs updated in the code base (python doc-generation script) and regenerated the rpc md file. Squashed all commits.

Copy link
Member

@ann0see ann0see left a comment

Choose a reason for hiding this comment

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

Thanks. Some minor comments

// out on malformed addresses not caught here.

QHostAddress InetAddr;
if ( !InetAddr.setAddress ( strJsonRpcBindIP ) )
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 introduce a method which checks IP validity e.g via Regex

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're probably right -- an IP address validation function used through the codebase. I think that is out of scope for this particular PR, however.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think using the same methods as for Server Bind IP will be okay for now.

docs/JSON-RPC.md Outdated Show resolved Hide resolved
@Rob-NY
Copy link
Contributor Author

Rob-NY commented Nov 4, 2022

@ann0see @hoffie - I think something happened with the auto checks. The Mac legacy "was cancelled". Let me know if there's something I need to do.

@ann0see
Copy link
Member

ann0see commented Nov 4, 2022

Probably that's an outrage due to the old macOS version it's using

src/main.cpp Outdated
Comment on lines 807 to 818
else
{
// This means of testing the validity of IP addresses is far from perfect but
// we do it here as an upfront check. The downstream network calls will error
// out on malformed addresses not caught here.

QHostAddress InetAddr;
if ( !InetAddr.setAddress ( strJsonRpcBindIP ) )
{
qCritical() << qUtf8Printable ( QString ( "The JSON-RPC address specified is not valid, exiting. " ) );
exit ( 1 );
}
}
Copy link
Member

Choose a reason for hiding this comment

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

We can avoid deep nesting here by dropping else (we'll never reach that code due to the exit before.

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'll refactor taking into account @pljones comment about the port being specified as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FWIW, the reason this was in an if..else block was to manage the variable scope for InetAddr. Even if I pull it out and make the "else" its own block, I need to (or should) bracket it to protect that scope.

@hoffie
Copy link
Member

hoffie commented Nov 7, 2022

I'm mostly happy with this PR, but unless someone opposes, I'd prefer to have the validation implemented which is mentioned here:

I know that we might not be consistent regarding this in the existing code base, but it might make sense to error out if --jsonrpcbind(ip) is given, but --jsonrpcport isn't (i.e., JSON-RPC is not enabled at all). Otherwise the option is ignored silently.

Pushed an update with additional error checking both for "empty" addresses and using QHostAddress as is done consistently elsewhere. This should wrap up this PR, let me know if otherwise.

I think there might have been a misunderstanding. We're now checking if the user has provided an empty IP argument. That's nice to have, but not what I meant with the above suggestion.

What I meant was:

$ ./Jamulus -s -n --jsonrpcbindip 127.0.0.1
[...]
- JSON-RPC binding to: 127.0.0.1

This might be confusing as it seems like JSON-RPC is enabled, while it isn't (as no --jsonrpcport was given, which controls to enabling).

I was suggesting something like:

$ ./Jamulus -s -n --jsonrpcbindip 127.0.0.1
[...]
Ignoring --jsonrpcbindip as no --jsonrpcport was given.

As this is a new feature, we could be even more harsh and log it as an error and exit to make such usage errors obvious (there is no risk of breaking anything at this stage).

The Mac legacy "was cancelled".

I've restarted that job. It was indeed due to the MacOS 10.15 "brown out" phase by Github (see #2773).

@Rob-NY
Copy link
Contributor Author

Rob-NY commented Nov 9, 2022

I was suggesting something like:

$ ./Jamulus -s -n --jsonrpcbindip 127.0.0.1
[...]
Ignoring --jsonrpcbindip as no --jsonrpcport was given.

Given that command line options are processed left to right, and that a default rpcbindip always exists, doing something like this would require additional flags during switch processing and additional logic after all switches were processed.

Alternatively, the way the default RPC IP is defined and assigned would need to change.

Frankly, for me at this point, for such a very basic capability and small audience, it just isn't worth it.

I can change the language to say "JSON-RPC: would bind to x.x.x.x, enabled when port specified." or something like it if that helps clarify things.

Copy link
Member

@hoffie hoffie left a comment

Choose a reason for hiding this comment

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

@Rob-NY Fair points, I'm fine with that. Thanks again for this PR!

@hoffie hoffie merged commit 18dd1b1 into jamulussoftware:master Nov 10, 2022
hoffie added a commit to hoffie/jamulus that referenced this pull request Nov 10, 2022

if ( strJsonRpcBindIP.trimmed().isEmpty() )
{
qCritical() << qUtf8Printable ( QString ( "JSON-RPC is enabled but no bind address was provided, exiting." ) );
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the wording of the "objection" is slightly misleading here. --serverbindip was passed with a parameter, otherwise GetStringArgument would have failed earlier. So

Suggested change
qCritical() << qUtf8Printable ( QString ( "JSON-RPC is enabled but no bind address was provided, exiting." ) );
qCritical() << qUtf8Printable ( QString ( "JSON-RPC is enabled but bind address provided is invalid, exiting." ) );

It might even be useful to do if ( !InetAddr.setAddress ( strJsonRpcBindIP ) ), like for Server Bind IP.

I'd also question whether "exiting" is in keeping with the other checks we do... it's a bit hit and miss, I guess. I may even have said do it like this :).

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 the wording of the "objection" is slightly misleading here.

I'll squeeze that improvement into #2958 and rename it to be more generic.

I'd also question whether "exiting" is in keeping with the other checks we do... it's a bit hit and miss, I guess. I may even have said do it like this :).

I would have certainly supported that (maybe I did ;)). I think we should fail early for new features, even if we didn't do so previously and don't want to make those existing checks more strict for compatibility reasons for now.

hoffie added a commit to hoffie/jamulus that referenced this pull request Nov 10, 2022
@ann0see ann0see removed the needs documentation PRs requiring documentation changes or additions label Jul 24, 2023
@ann0see
Copy link
Member

ann0see commented Jul 24, 2023

Dropped needs documentation as jamulussoftware/jamuluswebsite#871 was merged a while ago.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants