clnrest: replace with rust plugin #7509

daywalker90 · 2024-07-31T12:18:16Z

This is intended to be a drop-in replacement for @ShahanaFarooqui's clnrest.py. Python dependency management, especially in the context of CLN is not easy. Quite a few people struggled to setup CLN + python dependencies on modern unix OS. E.g. on debian 12 you have great choices like pip install --user --break-system-packages -r requirements.txt (you are only running CLN with this user right?) or hard-coding the PATH and PYTHON_PATH variables in your systemd service.

Installing a binary is easier, so @chrisguida bribed me into rewriting clnrest.py in rust.

In a future PR i intend to support creating dynamic REST routes for other plugins using @gudnuf PR's #7507 and #7508

vincenzopalazzo

Concept ACK

rustyrussell · 2024-08-01T00:01:11Z

Nice! Might take a while to reach feature parity, I've asked @ShahanaFarooqui to review...

niftynei

some early feedback, based on looking through the tests and running startup_regtest locally. still need to go through the rust code

i had two patches to make startup_regtest work, see daywalker90#6

plugins/clnrest-plugin/src/certs.rs

niftynei · 2024-08-05T19:50:33Z

plugins/clnrest-plugin/src/certs.rs

+    server_params
+        .distinguished_name
+        .push(rcgen::DnType::CommonName, "cln rest server");
+    if let Ok(ip) = rest_host.parse::<IpAddr>() {


i'm pretty new to rust so apologies if this is obvious, but what happens in the case where the rest_host string fails to parse as an IpAddr?

Then the code block after will not execute. if let Ok(ip) means "get the Ok type into ip". I could do something with the error by appending a else block.

The if let syntax here:

if let Ok(something) = some_operation() { println!("Hello {}", something) } else {}

is equivalent to

match some_operation() { Ok(something) => println!("Hello {}", something), Err(_e) => {} }

NOtice that the match statement allows us to extract the error, which I'm not sure we'd do in the if let case.

niftynei · 2024-08-05T19:51:45Z

plugins/clnrest-plugin/src/certs.rs

+    let ca_key = KeyPair::generate()?;
+    let ca_cert = ca_params.self_signed(&ca_key)?;
+
+    fs::create_dir_all(certs_path)?;


what happens if this fails?

The ? will propagate the error and in this case right to the main function where there is another ? after generate_certificates which will stop the plugin with the error message from fs::create_dir_all. When does this error:

"This function will return an error in the following situations, but is not limited to just these cases:

If any directory in the path specified by path does not already exist and it could not be created otherwise. The specific error conditions for when a directory is being created (after it is determined to not exist) are outlined by [fs::create_dir].
Notable exception is made for situations where any of the directories specified in the path could not be created as it was being created concurrently. Such cases are considered to be successful. That is, calling create_dir_all concurrently from multiple threads or processes is guaranteed not to fail due to a race condition with itself."

niftynei · 2024-08-05T19:52:14Z

plugins/clnrest-plugin/src/certs.rs

+    fs::write(
+        certs_path.join("client-key.pem"),
+        client_key.serialize_pem().as_bytes(),
+    )?;


what happens if this fails?

A ? at the end of a function call that can error means that the error wil propagate up. In this case directly to the main function and the plugin would stop with the error message generated by fs::write

tests/test_clnrest.py

niftynei · 2024-08-05T20:58:59Z

tests/test_clnrest.py

@@ -251,7 +251,7 @@ def test_clnrest_large_response(node_factory):
 # to complain with the errors F811 like this "F811 redefinition of
 # unused 'message'".

-def notifications_received_via_websocket(l1, base_url, http_session, rpc_method='invoice', rpc_params=[100000, 'label', 'description']):
+def notifications_received_via_websocket(l1, base_url, http_session, rpc_method='invoice', rpc_params=[100000, 'label', 'description'], expect_error=None):


nice addition!

niftynei · 2024-08-05T20:59:50Z

tests/test_clnrest.py

@@ -390,10 +401,10 @@ def test_clnrest_numeric_msat_notification(node_factory):
    rune_clnrest_notifications = l2.rpc.createrune(restrictions=[["method=listclnrest-notifications"]])['rune']
    http_session.headers.update({"rune": rune_clnrest_notifications})
    notifications = notifications_received_via_websocket(l1, base_url, http_session, 'pay', [inv['bolt11']])
-    filtered_notifications = [n for n in notifications if 'invoice_creation' in n]
+    filtered_notifications = [n for n in notifications if 'invoice_payment' in n]


what's the rationale for changing this notification keyword?

The python version of clnrest was only sending notifications every 1 second, while the rust version sends them immediately. In the tests it only starts listening to notifications when calling notifications_received_via_websocket so it would miss the notification from the invoice creation before while the python version would still catch it because of the queue.

tests/test_clnrest.py

niftynei · 2024-08-05T21:03:57Z

tests/test_clnrest.py

+
+
+def test_clnrest_websocket_upgrade_header(node_factory):
+    """Test that not setting an upgrade header leads to rejection"""


is it not possible to call the http endpoints w/o using a websocket? or is there a subset of RPCs that require websocket connections to work?

this seems like a change in behavior from the python client.

The comment may be misleading i think. It is possible to call the http endpoints without using a websocket. In fact the websocket in clnrest is only for listening to notifications.

I wrote this test because in order to differentiate what a client wants when calling the root path i'm checking the upgrade header. Every websocket client i tested (RTL + ZEUS) included that but not the client used in the tests.

Not setting this header (as in this test) makes the request look like a regular http request and in this case will return a redirect to the swagger ui which the socketio client interprets as an unexpected response.

I'd say in practice this does not make a change in behaviour since a normal websocket client sets the upgrade header.

I only wrote this tests because i had a bug in an earlier version where not setting the upgrade header would circumvent the rune check for notifications.

daywalker90 · 2024-08-08T09:46:33Z

Thanks for the quick feedback @niftynei ! If you are new to rust i can recommend https://doc.rust-lang.org/book/

daywalker90 · 2024-08-13T14:44:54Z

Built ontop of #7544 after getting merged on master. Fixes compatibility with multi options.

niftynei · 2024-08-20T16:34:25Z

as a nixos user, i am very interested in this move away from python!

chrisguida · 2024-09-20T22:43:40Z

Feature request: log which runes were used for which calls. That way an organization can track which users authorized which transactions by giving different runes to different users.

daywalker90 · 2024-09-22T10:47:00Z

Feature request: log which runes were used for which calls. That way an organization can track which users authorized which transactions by giving different runes to different users.

I've done it like this:

INFO    plugin-clnrest: Authorized `ysx3Jj5nOUWk3qujMHD8qo5Lcc0UlsXoDyXuhEC4_a49MA==` access to `invoice` with params: `{\"amount_msat\":1000,\"description\":\"owiegh\",\"label\":\"owigerjh\"}`

Also rebased on master and fixed tests for Valgrind (had to wait for stuff longer because Valgrind is so slow).

chrisguida · 2024-09-23T20:43:18Z

Awesome!!! Hmm, I'm realizing that these will be sensitive to have in the logs... is there any utility to clean stuff like this out?

Or maybe we can put in the db instead?

chrisguida · 2024-09-23T20:44:12Z

Oh actually you can reference the runes by their index instead of their actual value, that might be the way to go?

Changelog-Changed: clnrest is now a rust plugin

Just check for the presence of the clnrest binary now. Also update the string to check to the actual REST startup string

If you re-run a node several times, the log file fills with info from previous runs. To avoid looking at old logs, only parse the most recent run's logs when looking for the magic CLN rest startup/deactivated strings Changelog-Fixed: startup-regtest.sh now only inspects the most recent run's logs for the active status of the clnrest plugin

daywalker90 · 2024-09-24T12:10:07Z

I've replaced the actual rune with the rune id (at the cost of an extra rpc call to showrunes) and formatted the string a bit better for people who want to parse it:

"Authorized rune_id:`{}` access to method:`{}` with params:`{}`"

chrisguida · 2024-10-04T18:18:19Z

This plugin has been added to https://github.com/lightningd/plugins#available-plugins if anyone wants to try it out!

See "clnrest-rs"

daywalker90 force-pushed the clnrest-rs branch 2 times, most recently from 82996a7 to 0663de9 Compare July 31, 2024 13:10

vincenzopalazzo reviewed Jul 31, 2024

View reviewed changes

daywalker90 force-pushed the clnrest-rs branch 2 times, most recently from 3579c8d to dfecf91 Compare July 31, 2024 15:06

gudnuf mentioned this pull request Jul 31, 2024

add clnrest object to plugin manifest #7507

Open

rustyrussell requested a review from ShahanaFarooqui August 1, 2024 00:00

daywalker90 mentioned this pull request Aug 5, 2024

Clnrest: dynamic paths #7529

Open

niftynei reviewed Aug 8, 2024

View reviewed changes

daywalker90 force-pushed the clnrest-rs branch 2 times, most recently from f77ec03 to 1b04270 Compare August 8, 2024 09:44

This comment was marked as resolved.

Sign in to view

daywalker90 mentioned this pull request Aug 8, 2024

plugins: rust clnrest plugin #7541

Closed

daywalker90 force-pushed the clnrest-rs branch from 1b04270 to 6c4bea0 Compare August 13, 2024 14:43

daywalker90 force-pushed the clnrest-rs branch from 6c4bea0 to 956abee Compare August 13, 2024 14:56

daywalker90 force-pushed the clnrest-rs branch 2 times, most recently from d9eb409 to 6af4816 Compare September 22, 2024 10:16

daywalker90 and others added 3 commits September 24, 2024 10:35

clnrest: replace with rust plugin

992b68c

Changelog-Changed: clnrest is now a rust plugin

startup-regtest: look for clnrest binary on startup

8aae5db

Just check for the presence of the clnrest binary now. Also update the string to check to the actual REST startup string

daywalker90 force-pushed the clnrest-rs branch from 6af4816 to 855bce6 Compare September 24, 2024 12:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

clnrest: replace with rust plugin #7509

clnrest: replace with rust plugin #7509

daywalker90 commented Jul 31, 2024

vincenzopalazzo left a comment

rustyrussell commented Aug 1, 2024

niftynei left a comment

niftynei Aug 5, 2024

daywalker90 Aug 8, 2024

cdecker Aug 8, 2024

niftynei Aug 5, 2024

daywalker90 Aug 8, 2024

niftynei Aug 5, 2024

daywalker90 Aug 8, 2024

niftynei Aug 5, 2024

niftynei Aug 5, 2024

daywalker90 Aug 8, 2024

niftynei Aug 5, 2024

daywalker90 Aug 8, 2024

daywalker90 Aug 8, 2024

daywalker90 commented Aug 8, 2024

This comment was marked as resolved.

daywalker90 commented Aug 13, 2024

niftynei commented Aug 20, 2024

chrisguida commented Sep 20, 2024

daywalker90 commented Sep 22, 2024 •

edited

Loading

chrisguida commented Sep 23, 2024

chrisguida commented Sep 23, 2024

daywalker90 commented Sep 24, 2024

chrisguida commented Oct 4, 2024



		def test_clnrest_websocket_upgrade_header(node_factory):
		"""Test that not setting an upgrade header leads to rejection"""

clnrest: replace with rust plugin #7509

Are you sure you want to change the base?

clnrest: replace with rust plugin #7509

Conversation

daywalker90 commented Jul 31, 2024

vincenzopalazzo left a comment

Choose a reason for hiding this comment

rustyrussell commented Aug 1, 2024

niftynei 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

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

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

daywalker90 commented Aug 8, 2024

This comment was marked as resolved.

daywalker90 commented Aug 13, 2024

niftynei commented Aug 20, 2024

chrisguida commented Sep 20, 2024

daywalker90 commented Sep 22, 2024 • edited Loading

chrisguida commented Sep 23, 2024

chrisguida commented Sep 23, 2024

daywalker90 commented Sep 24, 2024

chrisguida commented Oct 4, 2024

daywalker90 commented Sep 22, 2024 •

edited

Loading