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

Rob-NY · 2022-10-17T15:45:20Z

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

ann0see · 2022-10-17T18:49:09Z

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

Rob-NY · 2022-10-17T19:14:56Z

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.

hoffie

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

pljones · 2022-10-17T20:08:27Z

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 · 2022-10-17T20:09:17Z

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.

Rob-NY · 2022-10-18T00:04:24Z

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.

src/rpcserver.cpp

hoffie · 2022-10-25T06:10:05Z

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 · 2022-11-02T00:17:57Z

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 · 2022-11-03T12:19:26Z

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

ann0see

Thanks. Some minor comments

ann0see · 2022-11-03T13:02:20Z

src/main.cpp

+            // out on malformed addresses not caught here.
+
+            QHostAddress InetAddr;
+            if ( !InetAddr.setAddress ( strJsonRpcBindIP ) )


We should introduce a method which checks IP validity e.g via Regex

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.

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

docs/JSON-RPC.md

Rob-NY · 2022-11-04T19:33:05Z

@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 · 2022-11-04T19:44:57Z

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

hoffie · 2022-11-07T22:07:57Z

src/main.cpp

+        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 );
+            }
+        }


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

I'll refactor taking into account @pljones comment about the port being specified as well.

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 · 2022-11-07T22:43:00Z

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 · 2022-11-09T00:12:01Z

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.

hoffie

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

@ann0see

IPv6 was tested successfully by @ann0see in jamulussoftware#2917 (comment)

pljones · 2022-11-10T19:19:53Z

src/main.cpp

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


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 :).

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.

References: jamulussoftware#2917 (comment) Co-authored-by: Peter L Jones <peter@drealm.info>

ann0see · 2023-07-24T20:39:24Z

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

hoffie added this to the Release 3.10.0 milestone Oct 17, 2022

hoffie added the needs documentation PRs requiring documentation changes or additions label Oct 17, 2022

hoffie reviewed Oct 17, 2022

View reviewed changes

src/main.cpp Outdated Show resolved Hide resolved

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 force-pushed the server_bind_address branch from 205cc4b to f0e4a2d Compare October 17, 2022 23:53

pljones reviewed Oct 18, 2022

View reviewed changes

src/rpcserver.cpp Show resolved Hide resolved

Rob-NY force-pushed the server_bind_address branch from f0e4a2d to f030a32 Compare October 22, 2022 14:01

pljones approved these changes Oct 24, 2022

View reviewed changes

Rob-NY force-pushed the server_bind_address branch from f030a32 to be51d63 Compare November 1, 2022 23:59

Rob-NY closed this Nov 2, 2022

Rob-NY reopened this Nov 2, 2022

Rob-NY force-pushed the server_bind_address branch from 70d839d to e81b343 Compare November 3, 2022 12:29

ann0see reviewed Nov 3, 2022

View reviewed changes

Rob-NY force-pushed the server_bind_address branch from 9c103c8 to b29580f Compare November 4, 2022 16:50

hoffie reviewed Nov 7, 2022

View reviewed changes

Add option to specify bind address for RPC server (IPv4 Only)

22dd69b

Rob-NY force-pushed the server_bind_address branch from 5f6e9c5 to 22dd69b Compare November 9, 2022 01:03

hoffie approved these changes Nov 10, 2022

View reviewed changes

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

JSON-RPC: Remove IPv6-untested warning from --jsonrpcbindip docs

2878ad9

IPv6 was tested successfully by @ann0see in jamulussoftware#2917 (comment)

hoffie mentioned this pull request Nov 10, 2022

JSON-RPC: Remove IPv6-untested warning from --jsonrpcbindip docs / improve empty address warning #2958

Merged

5 tasks

pljones reviewed Nov 10, 2022

View reviewed changes

hoffie added a commit to hoffie/jamulus that referenced this pull request Nov 10, 2022

JSON-RPC: Improve empty-bind-address warning for --jsonrpcbindip

d9ef061

References: jamulussoftware#2917 (comment) Co-authored-by: Peter L Jones <peter@drealm.info>

ann0see removed the needs documentation PRs requiring documentation changes or additions label Jul 24, 2023

pljones assigned Rob-NY Jul 29, 2023

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Rob-NY commented Oct 17, 2022 •

edited by ann0see

Loading

ann0see commented Oct 17, 2022

Rob-NY commented Oct 17, 2022

hoffie left a comment

pljones commented Oct 17, 2022

hoffie commented Oct 17, 2022

Rob-NY commented Oct 18, 2022

hoffie commented Oct 25, 2022

Rob-NY commented Nov 2, 2022

Rob-NY commented Nov 3, 2022 •

edited

Loading

ann0see left a comment

ann0see Nov 3, 2022

Rob-NY Nov 3, 2022

pljones Nov 10, 2022

Rob-NY commented Nov 4, 2022

ann0see commented Nov 4, 2022

hoffie Nov 7, 2022

Rob-NY Nov 7, 2022

Rob-NY Nov 9, 2022

hoffie commented Nov 7, 2022

Rob-NY commented Nov 9, 2022

hoffie left a comment

pljones Nov 10, 2022

hoffie Nov 10, 2022

ann0see commented Jul 24, 2023

	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." ) );

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

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

Conversation

Rob-NY commented Oct 17, 2022 • edited by ann0see Loading

Checklist

ann0see commented Oct 17, 2022

Rob-NY commented Oct 17, 2022

hoffie left a comment

Choose a reason for hiding this comment

pljones commented Oct 17, 2022

hoffie commented Oct 17, 2022

Rob-NY commented Oct 18, 2022

hoffie commented Oct 25, 2022

Rob-NY commented Nov 2, 2022

Rob-NY commented Nov 3, 2022 • edited Loading

ann0see left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rob-NY commented Nov 4, 2022

ann0see commented Nov 4, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hoffie commented Nov 7, 2022

Rob-NY commented Nov 9, 2022

hoffie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ann0see commented Jul 24, 2023

Rob-NY commented Oct 17, 2022 •

edited by ann0see

Loading

Rob-NY commented Nov 3, 2022 •

edited

Loading