client: Replace `unsafe_rpc_expose` with an `RpcMethods` enum #5729

Xanewok · 2020-04-21T17:32:05Z

...which can be either Default, Safe or Unsafe. The idea is to have the
following:

	--rpc-external=false	--rpc-external=true
--rpc-methods=Default		unsafe calls denied
--rpc-methods=Safe	unsafe calls denied	unsafe calls denied
--rpc-methods=Unsafe

Since the previous unsafe-rpc-expose option was confusing.

Fixes #5710

cc @tomusdrw @xlc

I also modified the Compose file since it mentioned the now-removed flag but wasn't sure if that's the right thing to do here; lmk if I should back out the related changes.

parity-cla-bot · 2020-04-21T17:32:10Z

It looks like @Xanewok signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

tomusdrw

Looks good, tiny issues to address. And we should decide on how to deprecate the old options - is there a policy now? CC @bkchr ?

tomusdrw · 2020-04-22T09:59:34Z

client/cli/src/commands/runcmd.rs

-	///
-	/// Same as `--rpc-external`.
-	#[structopt(long = "unsafe-rpc-external")]
-	pub unsafe_rpc_external: bool,


Wait, these options should stay. They suppress the warning in case you are not a validator and allow you to expose the interface in case you are.
Solving the docker case should be via a separate flag, see #5418

On a second though, perhaps it's actually a good idea to remove them and simplify the CLI. I wonder if we should rather deprecate them gracefuly (i.e. keep them for now, and make them an alias of --rpc-external but print a warning when used).

client/cli/src/commands/runcmd.rs

client/cli/src/config.rs

client/service/src/config.rs

Xanewok · 2020-04-23T12:17:29Z

Left out the removal of --unsafe-{rpc,ws}-external flags and only replaced --unsafe-rpc-expose with the --rpc-methods={default,safe,unsafe} enum argument.

For validator nodes, passing --{rpc,ws}-external --rpc-methods=unsafe will not error out and allow us to expose entire RPC method set externally (which IIUC was the original purpose of --unsafe-rpc-expose).

cecton

LGTM

client/cli/src/commands/runcmd.rs

Xanewok · 2020-04-24T09:50:18Z

All green except gitlab-check-polkadot-companion-build:

 warning: build failed, waiting for other jobs to finish...
error[E0061]: this function takes 5 parameters but 4 parameters were supplied
   --> parachain/src/wasm_executor/mod.rs:181:17
    |
181 |       let executor = sc_executor::WasmExecutor::new(
    |  ____________________^
182 | |         sc_executor::WasmExecutionMethod::Interpreted,
183 | |         // TODO: Make sure we don't use more than 1GB: https://github.com/paritytech/polkadot/issues/699
184 | |         Some(1024),
185 | |         HostFunctions::host_functions(),
186 | |         8
187 | |     );
    | |_____^ expected 5 parameters
error[E0061]: this function takes 5 parameters but 6 parameters were supplied
   --> parachain/src/wasm_executor/mod.rs:188:21
    |
188 |     let res = executor.call_in_wasm(
    |                        ^^^^^^^^^^^^ expected 5 parameters
error: aborting due to 3 previous errors
Some errors have detailed explanations: E0061, E0433.
For more information about an error, try `rustc --explain E0061`.
error: could not compile `polkadot-parachain`.

Let me retrigger the CI

cecton

CLI is purrfect 🐱

gavofyork · 2020-04-30T13:47:34Z

@Xanewok needs resolving and possibly a Polkadot companion PR submitting

bkchr · 2020-05-04T12:08:08Z

@Xanework status?

Xanewok · 2020-05-04T22:54:06Z

Will resolve this today, sorry for the delay.

…

On Mon, 4 May 2020 at 14:08, Bastian Köcher ***@***.***> wrote: @Xanework status? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#5729 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAXTFXOKXEQ4KA6WHYFP4A3RP2V3PANCNFSM4MNPDX4Q> .

Co-Authored-By: Cecile Tonglet <cecile.tonglet@cecton.com>

Xanewok · 2020-05-05T12:49:04Z

Checks are all green and as far as I understand, this does not affect Polkadot since the changes here touch CLI mostly, which are contained already as a flattened base struct in the RunCmd.

@gnunicorn is this good to merge?

gnunicorn · 2020-05-05T12:53:21Z

@Xanewok please do not force-push, it reverts history and makes it harder to review – just use regular merges, thus a reviewer can use the "Review only changes since you've last seen" to speed up the process. We squash-merge anyways, all merge commit are going away then.

Xanewok · 2020-05-05T12:57:08Z

Will do. I'm am used to rebasing on master + merging all of the commits from a feature branch and so wanted to avoid multiple merge commits but we're squashing anyway so there won't be any clutter in the first place 👍 Sorry about that.

Xanewok · 2020-05-05T13:00:26Z

To facilitate the review, the only bits needed resolving where import directives:

and the rest stayed the same.

bkchr · 2020-05-05T14:59:10Z

client/cli/src/commands/run_cmd.rs

-	/// Please do this if you know what you're doing.
-	#[structopt(long = "unsafe-rpc-expose")]
-	pub unsafe_rpc_expose: bool,
+	/// Defaults to `Safe` (allow only a safe subset of RPC methods) if RPC is


I find this documentation reads very confusing.

You describe the possible values, but not the Default variant.

I find it strikes a good balance without being very explicit - variants of Safe and Unsafe are explained (which can be explicitly picked and the behaviour is as explained) and Default, well, defaults to one of them, depending on the situation as listed.

Do you have any concrete example on how you'd like to improve it?

The docs don't specify what happens with Default though, right?

You say that it defaults to Safe, which is not true. It will default to Safe, when the RPC port is exposed externally.

The user can see all the values this CLI accepts by looking into --help and the user also sees the default value. The docs should describe that this argument is doing, so that the user can decide which value he wants.

Declare which RPC methods should be exposed under different conditions: - `Default`: Expose safe & unsafe RPC methods when the RPC is only reachable on localhost. If the RPC port is reachable on all interfaces only the safe RPC methods are exposed. - `Safe`: Only expose safe RPC methods under any conditions. - `Unsafe`: Expose safe & unsafe RPC methods under any condition.

Something like this?

/// RPC methods to expose. /// /// The `Unsafe` variant allows every RPC method to be called, while the /// `Safe` variant only allows for its safe subset. /// If `Default` variant is specified, it acts as `Safe` if RPC is served /// externally, e.g. when `--{rpc,ws}-external` is passed, or as `Unsafe` otherwise.

Does this read better?

EDIT: @bkchr We raced with a reply, let me reply to the above in a sec

@Xanewok sounds good to me.

@bkchr I liked your list approach, so I went with that instead (Also needed verbatim_doc_comment or the list would get wrapped, otherwise).

This now renders as:

--rpc-methods <METHOD SET> RPC methods to expose. - `Unsafe`: Exposes every RPC method. - `Safe`: Exposes only a safe subset of RPC methods, denying unsafe RPC methods. - `Default`: Acts as `Safe` if RPC is served externally, e.g. when `--{rpc,ws}-external` is passed, otherwise acts as `Unsafe`. [default: Default] [possible values: Default, Safe, Unsafe]

Now that I read about it, the Default variant name does seem off... Especially if structopt injects the [default: Default] for us.

Maybe something like Protected (or Auto?) would be a better name? Or should we just leave it for now?

Auto sounds good as it handles it for you automatically :D

(auto is also more common for a CLI)

Xanewok · 2020-05-05T19:53:43Z

The polkadot companion build is failing but it looks like it's unrelated:

   Compiling polkadot-runtime-common v0.7.33 (/builds/parity/substrate/polkadot/runtime/common)
error[E0277]: the trait bound `&polkadot_parachain::primitives::UpwardMessage: core::iter::ExactSizeIterator` is not satisfied
   --> /builds/parity/substrate/polkadot/runtime/common/src/parachains.rs:917:71
    |
917 |             upward_messages.iter().for_each(|m| RelayDispatchQueue::append(id, m));
    |                                                                                ^ the trait `core::iter::ExactSizeIterator` is not implemented for `&polkadot_parachain::primitives::UpwardMessage`
    |
    = note: required by `attestations::sp_api_hidden_includes_decl_storage::hidden_include::StorageMap::append`
error[E0308]: mismatched types
   --> /builds/parity/substrate/polkadot/runtime/common/src/parachains.rs:917:40
    |
917 |             upward_messages.iter().for_each(|m| RelayDispatchQueue::append(id, m));
    |                                                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected `()`, found enum `core::result::Result`
    |
    = note: expected unit type `()`
                    found enum `core::result::Result<(), &'static str>`
error: aborting due to 2 previous errors
Some errors have detailed explanations: E0277, E0308.

...by printing to stderr the stderr of the command. This is normally suppressed for succesful tests but not for failing ones - if that's the case then it's useful to see the test failure reason inline rather than having to execute the command separately ourselves.

Xanewok · 2020-05-05T20:20:09Z

Pushed the Default variant rename to Auto.

bin/node/cli/tests/build_spec_works.rs

bkchr · 2020-05-06T09:01:13Z

The polkadot companion build is failing but it looks like it's unrelated:

   Compiling polkadot-runtime-common v0.7.33 (/builds/parity/substrate/polkadot/runtime/common)
error[E0277]: the trait bound `&polkadot_parachain::primitives::UpwardMessage: core::iter::ExactSizeIterator` is not satisfied
   --> /builds/parity/substrate/polkadot/runtime/common/src/parachains.rs:917:71
    |
917 |             upward_messages.iter().for_each(|m| RelayDispatchQueue::append(id, m));
    |                                                                                ^ the trait `core::iter::ExactSizeIterator` is not implemented for `&polkadot_parachain::primitives::UpwardMessage`
    |
    = note: required by `attestations::sp_api_hidden_includes_decl_storage::hidden_include::StorageMap::append`
error[E0308]: mismatched types
   --> /builds/parity/substrate/polkadot/runtime/common/src/parachains.rs:917:40
    |
917 |             upward_messages.iter().for_each(|m| RelayDispatchQueue::append(id, m));
    |                                                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected `()`, found enum `core::result::Result`
    |
    = note: expected unit type `()`
                    found enum `core::result::Result<(), &'static str>`
error: aborting due to 2 previous errors
Some errors have detailed explanations: E0277, E0308.

That is because you don't have latest Substrate master in.

Xanewok added A0-please_review Pull request needs code review. M6-rpcapi labels Apr 21, 2020

Xanewok requested a review from cecton as a code owner April 21, 2020 17:32

Xanewok force-pushed the tweak-rpc-unsafe branch from 5f8a3e9 to da72329 Compare April 21, 2020 19:22

tomusdrw approved these changes Apr 22, 2020

View reviewed changes

tomusdrw added A5-grumble and removed A0-please_review Pull request needs code review. labels Apr 22, 2020

Xanewok force-pushed the tweak-rpc-unsafe branch from 77159c5 to 98394ff Compare April 23, 2020 12:06

tomusdrw added A8-mergeoncegreen and removed A5-grumble labels Apr 23, 2020

tomusdrw approved these changes Apr 23, 2020

View reviewed changes

gnunicorn requested a review from tomusdrw April 23, 2020 13:42

cecton reviewed Apr 23, 2020

View reviewed changes

client/cli/src/commands/runcmd.rs Outdated Show resolved Hide resolved

client/cli/src/commands/runcmd.rs Outdated Show resolved Hide resolved

Xanewok closed this Apr 24, 2020

Xanewok reopened this Apr 24, 2020

Xanewok force-pushed the tweak-rpc-unsafe branch from 8574bb7 to 4852d1a Compare April 24, 2020 09:52

Xanewok requested a review from cecton April 24, 2020 09:53

cecton reviewed Apr 24, 2020

View reviewed changes

Xanewok and others added 3 commits May 5, 2020 13:19

client: Only warn against exposing externally unsafe RPC method set

8d58af6

Apply suggestions from code review

ce63918

Co-Authored-By: Cecile Tonglet <cecile.tonglet@cecton.com>

Xanewok force-pushed the tweak-rpc-unsafe branch from 2f3789e to ce63918 Compare May 5, 2020 11:22

gnunicorn requested a review from cecton May 5, 2020 12:51

bkchr reviewed May 5, 2020

View reviewed changes

cli: Rephrase doc comment for rpc_methods config

3ffaefe

Xanewok added 2 commits May 5, 2020 22:17

Rename RpcMethods::{Default => Auto} variant

63fd770

bkchr approved these changes May 6, 2020

View reviewed changes

bin/node/cli/tests/build_spec_works.rs Outdated Show resolved Hide resolved

Update bin/node/cli/tests/build_spec_works.rs

0816008

bkchr merged commit 24486f5 into paritytech:master May 6, 2020

Xanewok deleted the tweak-rpc-unsafe branch May 6, 2020 09:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

client: Replace `unsafe_rpc_expose` with an `RpcMethods` enum #5729

client: Replace `unsafe_rpc_expose` with an `RpcMethods` enum #5729

Xanewok commented Apr 21, 2020

parity-cla-bot commented Apr 21, 2020

tomusdrw left a comment

tomusdrw Apr 22, 2020

tomusdrw Apr 22, 2020

Xanewok commented Apr 23, 2020 •

edited

Loading

cecton left a comment

Xanewok commented Apr 24, 2020

cecton left a comment

gavofyork commented Apr 30, 2020

bkchr commented May 4, 2020

Xanewok commented May 4, 2020 via email

Xanewok commented May 5, 2020

gnunicorn commented May 5, 2020

Xanewok commented May 5, 2020

Xanewok commented May 5, 2020

bkchr May 5, 2020

bkchr May 5, 2020

Xanewok May 5, 2020

gavofyork May 5, 2020

bkchr May 5, 2020

Xanewok May 5, 2020 •

edited

Loading

bkchr May 5, 2020

Xanewok May 5, 2020 •

edited

Loading

bkchr May 5, 2020

cecton May 6, 2020

Xanewok commented May 5, 2020

Xanewok commented May 5, 2020

bkchr commented May 6, 2020

client: Replace unsafe_rpc_expose with an RpcMethods enum #5729

client: Replace unsafe_rpc_expose with an RpcMethods enum #5729

Conversation

Xanewok commented Apr 21, 2020

parity-cla-bot commented Apr 21, 2020

tomusdrw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Xanewok commented Apr 23, 2020 • edited Loading

cecton left a comment

Choose a reason for hiding this comment

Xanewok commented Apr 24, 2020

cecton left a comment

Choose a reason for hiding this comment

gavofyork commented Apr 30, 2020

bkchr commented May 4, 2020

Xanewok commented May 4, 2020 via email

Xanewok commented May 5, 2020

gnunicorn commented May 5, 2020

Xanewok commented May 5, 2020

Xanewok commented May 5, 2020

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

Choose a reason for hiding this comment

Xanewok May 5, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Xanewok May 5, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Xanewok commented May 5, 2020

Xanewok commented May 5, 2020

bkchr commented May 6, 2020

client: Replace `unsafe_rpc_expose` with an `RpcMethods` enum #5729

client: Replace `unsafe_rpc_expose` with an `RpcMethods` enum #5729

Xanewok commented Apr 23, 2020 •

edited

Loading

Xanewok May 5, 2020 •

edited

Loading

Xanewok May 5, 2020 •

edited

Loading