clnrest: refactoring #6749

tonyaldon · 2023-10-06T08:32:08Z

While working on testcases, I found few bugs in clnrest that were fixed along the way:

clnrest self-signed certificates adds rest-host IP in Subject Alternative Name
Fixed: Only runes which authorise the listclnrest-notifications method are allowed to get notifications

Changelog-Fixed: websocket server notifications are available with readonly runes

Link to #6436.

tests/test_clnrest.py

plugins/clnrest/utilities/generate_certs.py

plugins/clnrest/utilities/rpc_plugin.py

vincenzopalazzo

Looks like you are having fun with pytest and friends :)

I left a couple of comments while reviewing the code

plugins/clnrest/utilities/rpc_routes.py

plugins/clnrest/utilities/shared.py

plugins/clnrest/utilities/rpc_routes.py

tests/test_clnrest.py

doc/getting-started/getting-started/installation.md

plugins/clnrest/clnrest.py

tonyaldon · 2023-10-06T11:53:37Z

Thanks for the review @vincenzopalazzo

It seems better that the logs changed can be seen when INFO level is set. Discussed with Vincenzo Palazzo in the PR ElementsProject#6749.

tonyaldon · 2023-10-11T14:20:37Z

But I am not convinced with the idea to refactor the whole utilities folder and merge everything in clnrest.py directly.

Once we get to the commit e34e631 where we remove the file utilities/shared.py:

because we no longer use set_config function - last function in that file,
and no longer use global variables (last variables in that file) for the startup options because they are already available in the property plugin.options (after the init request)

we are left with the following tree structure in the directory
plugins/clnrest:

.
├── clnrest.py
├── ...
└── utilities
    ├── ...
    ├── generate_certs.py
    ├── rpc_plugin.py
    └── rpc_routes.py

Expect maybe for the functions in generate_certs.py file, the code in rpc_plugin.py and rpc_routes.py no longer contains any utilities.

The file rpc_plugin.py only instantiates the plugin object and declares plugin's options and the file rpc_routes.py only declare 2 routes.

So now the only reason to keep rpc_plugin.py in the utilities directory is in order to require it in rpc_routes.py file in order to use the methods plugin.rpc.call() and plugin.log().

But why not puting those 66 lines of code (rpc_plugin.py and rpc_routes.py) directly in clnrest.py as they are not utilities.

One benefit of this is also to put the most important state of that plugin (which is the plugin object) directly in clnrest.py.

You can also read comment: #6749 (comment)

Finally, when we've removed rpc_plugin.py and rpc_routes.py file, there's no good reason to not incorporate the last 54 lines of code of the file generate_certs.py.

Doing all of this results in 287 LOC in clnrest.py.

And this is really small!

We can compare it to some other plugins if we want:

955 LOC for pay.c,
862 LOC for bcli.c,
1283 LOC for sql.c
...

ps: I'm using cloc utility to count LOC.

tonyaldon · 2023-10-13T08:02:10Z

Hey @ShahanaFarooqui, as discussed yesterday I removed the last refactoring that incorporated the content of the following files utilities/generate_certs.py, utilities/rpc_plugin.py and utilities/rpc_routes.py into clnrest.py file. Normally, this leaves the structure of clnrest plugin in a way you prefer.

If you want, you can now do the modifications you told me about yesterday (I think you have write access to that branch, but tell me if this not the case).

As the PR has evolved, I'm leaving the original proposal here to contextualize it (with minor changes suggested by @vincenzopalazzo):

https://github.com/tonyaldon/lightning/tree/backup-test-clnrest

Dismissing my reviews after rebasing and explaining the reason on my previous comments.

tonyaldon · 2023-10-16T21:16:05Z

Hey @ShahanaFarooqui , I don't know why but if you put requests = "^2.31.0" dependency just after psutil = "^5.9" before the empty line, requests package will be taken into account and you'll be able to run test_clnrest.py test:

$ git diff contrib/pyln-testing/pyproject.toml

diff --git a/contrib/pyln-testing/pyproject.toml b/contrib/pyln-testing/pyproject.toml
index e551acb40..3ceaf279f 100644
--- a/contrib/pyln-testing/pyproject.toml
+++ b/contrib/pyln-testing/pyproject.toml
@@ -21,10 +21,10 @@ pyln-client = ">=23"
 Flask = "^2"
 cheroot = "^8"
 psutil = "^5.9"
+requests = "^2.31.0"
 
 grpcio = { version = "^1", optional = true }
 pyln-grpc-proto = { version = "^0.1", optional = true }
-requests = "^2.31.0"
 
 [tool.poetry.dev-dependencies]
 pyln-client = { path = "../pyln-client", develop = true}

Tell me if it works for you.

ShahanaFarooqui · 2023-10-16T21:25:00Z

Tell me if it works for you.

@tonyaldon Thanks for the suggestion, trying it now.

nepet · 2023-10-23T21:06:52Z

Rebased on master to include the latest test flake fixes.

It is a comment only, nothing to change. Included all other reviewed suggestions.

vincenzopalazzo

Looks like that tests are failing

=========================== short test summary info ============================
FAILED tests/test_clnrest.py::test_clnrest_uses_grpc_plugin_certificates - requests.exceptions.ConnectionError: HTTPSConnectionPool(host='localhost', port=36049): Max retries exceeded with url: /v1/list-methods (Caused by NewConnectionError('<urllib3.connection.HTTPSConnection object at 0x7f0b388eefd0>: Failed to establish a new connection: [Errno 111] Connection refused'))
FAILED tests/test_clnrest.py::test_clnrest_rpc_method - requests.exceptions.ConnectionError: HTTPSConnectionPool(host='127.0.0.1', port=40339): Max retries exceeded with url: /v1/getinfo (Caused by NewConnectionError('<urllib3.connection.HTTPSConnection object at 0x7f9cebc128b0>: Failed to establish a new connection: [Errno 111] Connection refused'))
FAILED tests/test_clnrest.py::test_clnrest_websocket_rune_readonly - socketio.exceptions.ConnectionError: HTTPSConnectionPool(host='127.0.0.1', port=44733): Max retries exceeded with url: /socket.io/?transport=polling&EIO=4&t=1698384416.943965 (Caused by NewConnectionError('<urllib3.connection.HTTPSConnection object at 0x7fe97802fe50>: Failed to establish a new connection: [Errno 111] Connection refused'))
===== 3 failed, 631 passed, 58 skipped, 33 warnings in 1905.51s (0:31:45) ======

tonyaldon · 2023-10-27T11:06:14Z

I'll look at these failing tests.

I can't reproduce these failed tests locally.

But I tried to play with pytest command options to see why the tests are not passing in the case of "Test CLN gcc". And what I found is that when I run the tests with 1, 2 or 3 CPUs out of the 4 that I have, they all pass, BUT when I use the maximum 4 (in my case) some tests failed randomly with the following error:

TimeoutError: timed out

Does anyone know why this happens?

ShahanaFarooqui · 2023-10-30T04:52:26Z

ACK 54cc473

tonyaldon · 2023-10-30T09:08:16Z

@ShahanaFarooqui is it necessary to use the method listclnrest-notifications which doesn't exist in CLN code to verify the rune for websocket notification?

is_valid_rune = verify_rune(plugin, rune, "listclnrest-notifications", None)

Can't we get the same behavior using getinfo instead like this?

is_valid_rune = verify_rune(plugin, rune, "getinfo", None)

ShahanaFarooqui · 2023-10-30T19:22:57Z

@tonyaldon We can use summary method or any method starting with get or starting with list except listdatastore which is equivalent to readonly rune. So yes, in short we can use getinfo method too.

But as suggested by @rustyrussell, using listclnrest-notifications will be less confusing for developers to understand its purpose and we can also utilise it to add/remove some specific features for this kind of rune, if required in future.

endothermicdev

Looks nice. Just a few minor comments.

doc/developers-guide/app-development/rest.md

plugins/clnrest/clnrest.py

plugins/clnrest/utilities/shared.py

plugins/clnrest/utilities/generate_certs.py

doc/getting-started/getting-started/installation.md

- certificate generation - config options validation - log level from 'error' to 'info' - sending method as None instead "" - added `listclnrest-notifications` for websocket server rune method Changelog-Fixed: websocket server notifications are available with restriction of `readonly` runes

endothermicdev · 2023-10-31T16:03:42Z

ACK d4788ed

ShahanaFarooqui · 2023-10-31T17:20:09Z

ACK d4788ed

ShahanaFarooqui

ACK d4788ed

ShahanaFarooqui · 2023-10-11T04:18:48Z

plugins/clnrest/utilities/rpc_plugin.py

-plugin.add_option(name="rest-certs", default=os.getcwd(), description="Path for certificates (for https)", opt_type="string", deprecated=False)
+rest_certs = Path(os.getcwd()) / 'clnrest'
+
+plugin.add_option(name="rest-certs", default=rest_certs.as_posix(), description="Path for certificates (for https)", opt_type="string", deprecated=False)


Configuring the certificate with multiple SAN can be considered an enhancement but:

rest-host option is used for IP address where the server is accessible. So using 127.0.0.1 or rest-host will not make any difference in current cert generation.

Self signed certs should be enough for testing.

For more advanced cert options, user should generate their own certificates as per their preference and configure the location with rest-certs.

If we still prefer to add more sofisticated certs functionality then it should be added with new config option like cert-sans/cert-clients to accept array of allowed SANs

The default should still be to use cln-grpc certs/generate basic certs at the default location.

After discussing with Rusty:

cln-grpc certs will be used as first go to option

if certs are not generate by grpc, then clnrest will generate certs

added rest-host is in SAN for clnrest certs

ShahanaFarooqui · 2023-10-11T04:19:59Z

plugins/clnrest/utilities/generate_certs.py



 def generate_certs(plugin, rest_host, certs_path):
-    ca_subject, ca_private_key = generate_ca_cert(rest_host, certs_path)
-    generate_client_server_certs(rest_host, certs_path, ca_subject, ca_private_key)
+    ca_subject, ca_private_key = generate_cert("ca", None, None, rest_host, certs_path)


Fixing log level and refactoring the code for generating the certificate are tiny updated which can be accepted with testing PR as well. But I am not convinced with the idea to refactor the whole utilities folder and merge everything in clnrest.py directly.

Good catch on generate cert refactoring. I was repeating the same code.

@tonyaldon Thanks for refactoring generate certificate method.

ShahanaFarooqui · 2023-10-11T04:20:42Z

plugins/clnrest/pyproject.toml

@@ -6,7 +6,6 @@ authors = ["ShahanaFarooqui <sfarooqui@blockstream.com>"]

 [tool.poetry.dependencies]
 python = "^3.8"
-json5 = "^0.9.14"


I had to add json5 dependency due to some non standard responses received from some requests. json5.loads(str(err)) does not fail with those requests. So json5 dependency is required to handle those responses.

Keeping json5 is essential for some non-standard error messages.

ShahanaFarooqui · 2023-10-11T04:42:48Z

plugins/clnrest/utilities/rpc_routes.py

@@ -41,26 +39,27 @@ class RpcMethodResource(Resource):
    @rpcns.response(500, "Server error")
    def post(self, rpc_method):
        """Call any valid core lightning method (check list-methods response)"""
-        try:


Cool side effect :).

@tonyaldon Thanks, implemented the login to read payload once, as per your suggestion.

doc/developers-guide/app-development/rest.md

plugins/clnrest/utilities/generate_certs.py

doc/getting-started/getting-started/installation.md

plugins/clnrest/utilities/shared.py

plugins/clnrest/utilities/generate_certs.py

vincenzopalazzo self-requested a review October 6, 2023 11:25

vincenzopalazzo added this to the v23.11 milestone Oct 6, 2023

vincenzopalazzo reviewed Oct 6, 2023

View reviewed changes

tests/test_clnrest.py Outdated Show resolved Hide resolved

vincenzopalazzo reviewed Oct 6, 2023

View reviewed changes

plugins/clnrest/utilities/generate_certs.py Outdated Show resolved Hide resolved

vincenzopalazzo reviewed Oct 6, 2023

View reviewed changes

plugins/clnrest/utilities/rpc_plugin.py Outdated Show resolved Hide resolved

vincenzopalazzo previously requested changes Oct 6, 2023

View reviewed changes

tonyaldon added a commit to tonyaldon/lightning that referenced this pull request Oct 8, 2023

clnrest: update logs.

da60faf

It seems better that the logs changed can be seen when INFO level is set. Discussed with Vincenzo Palazzo in the PR ElementsProject#6749.

tonyaldon force-pushed the test-clnrest branch from 4fdc8f3 to da60faf Compare October 8, 2023 15:41

This comment was marked as duplicate.

Sign in to view

tonyaldon mentioned this pull request Oct 11, 2023

clnrest: self-signed certificates error with curl #6761

Open

tonyaldon requested a review from ShahanaFarooqui October 11, 2023 13:04

tonyaldon force-pushed the test-clnrest branch from da60faf to cfeea69 Compare October 13, 2023 07:58

ShahanaFarooqui force-pushed the test-clnrest branch from cfeea69 to 92d8445 Compare October 15, 2023 04:03

ShahanaFarooqui requested a review from cdecker as a code owner October 15, 2023 04:03

ShahanaFarooqui changed the title ~~clnrest: tests, fixes and refactoring~~ clnrest: pyln-tests and refactoring Oct 15, 2023

ShahanaFarooqui force-pushed the test-clnrest branch from 92d8445 to 661beb5 Compare October 16, 2023 18:17

ShahanaFarooqui requested review from vincenzopalazzo and removed request for cdecker October 16, 2023 18:28

ShahanaFarooqui force-pushed the test-clnrest branch from 661beb5 to 651176e Compare October 16, 2023 20:06

ShahanaFarooqui force-pushed the test-clnrest branch from 651176e to b63bec8 Compare October 16, 2023 21:23

ShahanaFarooqui force-pushed the test-clnrest branch from b63bec8 to 5348d25 Compare October 16, 2023 22:12

nepet force-pushed the test-clnrest branch from 7294cd1 to bf576c8 Compare October 23, 2023 21:05

ShahanaFarooqui force-pushed the test-clnrest branch 3 times, most recently from 1a111fa to f12c736 Compare October 24, 2023 22:19

ShahanaFarooqui force-pushed the test-clnrest branch 4 times, most recently from 43752e7 to 7d11019 Compare October 27, 2023 04:54

vincenzopalazzo reviewed Oct 27, 2023

View reviewed changes

ShahanaFarooqui requested a review from endothermicdev October 27, 2023 17:11

ShahanaFarooqui force-pushed the test-clnrest branch 3 times, most recently from 5e4b955 to 54cc473 Compare October 30, 2023 03:28

ShahanaFarooqui changed the title ~~clnrest: pyln-tests and refactoring~~ clnrest: refactoring Oct 30, 2023

tonyaldon mentioned this pull request Oct 30, 2023

Clnrest testcases and removed msat suffix #6828

Merged

endothermicdev reviewed Oct 31, 2023

View reviewed changes

ShahanaFarooqui force-pushed the test-clnrest branch from 54cc473 to 1964f6e Compare October 31, 2023 01:25

tonyaldon added 2 commits October 30, 2023 18:46

doc: update clnrest documentation.

d4788ed

ShahanaFarooqui force-pushed the test-clnrest branch from 1964f6e to d4788ed Compare October 31, 2023 01:47

ShahanaFarooqui approved these changes Oct 31, 2023

View reviewed changes

ShahanaFarooqui merged commit 8a62831 into ElementsProject:master Oct 31, 2023
38 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

clnrest: refactoring #6749

clnrest: refactoring #6749

tonyaldon commented Oct 6, 2023 •

edited by ShahanaFarooqui

Loading

vincenzopalazzo left a comment

tonyaldon commented Oct 6, 2023

This comment was marked as duplicate.

This comment was marked as duplicate.

This comment was marked as duplicate.

This comment was marked as duplicate.

tonyaldon commented Oct 11, 2023 •

edited

Loading

tonyaldon commented Oct 13, 2023 •

edited

Loading

tonyaldon commented Oct 16, 2023

ShahanaFarooqui commented Oct 16, 2023

nepet commented Oct 23, 2023

vincenzopalazzo left a comment

tonyaldon commented Oct 27, 2023 •

edited

Loading

ShahanaFarooqui commented Oct 30, 2023

tonyaldon commented Oct 30, 2023

ShahanaFarooqui commented Oct 30, 2023

endothermicdev left a comment

endothermicdev commented Oct 31, 2023

ShahanaFarooqui commented Oct 31, 2023

ShahanaFarooqui left a comment

ShahanaFarooqui Oct 11, 2023

ShahanaFarooqui Oct 16, 2023

ShahanaFarooqui Oct 11, 2023

ShahanaFarooqui Oct 16, 2023

ShahanaFarooqui Oct 11, 2023

ShahanaFarooqui Oct 16, 2023

ShahanaFarooqui Oct 11, 2023

ShahanaFarooqui Oct 16, 2023

clnrest: refactoring #6749

clnrest: refactoring #6749

Conversation

tonyaldon commented Oct 6, 2023 • edited by ShahanaFarooqui Loading

vincenzopalazzo left a comment

Choose a reason for hiding this comment

tonyaldon commented Oct 6, 2023

This comment was marked as duplicate.

This comment was marked as duplicate.

This comment was marked as duplicate.

This comment was marked as duplicate.

tonyaldon commented Oct 11, 2023 • edited Loading

tonyaldon commented Oct 13, 2023 • edited Loading

tonyaldon commented Oct 16, 2023

ShahanaFarooqui commented Oct 16, 2023

nepet commented Oct 23, 2023

vincenzopalazzo left a comment

Choose a reason for hiding this comment

tonyaldon commented Oct 27, 2023 • edited Loading

ShahanaFarooqui commented Oct 30, 2023

tonyaldon commented Oct 30, 2023

ShahanaFarooqui commented Oct 30, 2023

endothermicdev left a comment

Choose a reason for hiding this comment

endothermicdev commented Oct 31, 2023

ShahanaFarooqui commented Oct 31, 2023

ShahanaFarooqui 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

tonyaldon commented Oct 6, 2023 •

edited by ShahanaFarooqui

Loading

tonyaldon commented Oct 11, 2023 •

edited

Loading

tonyaldon commented Oct 13, 2023 •

edited

Loading

tonyaldon commented Oct 27, 2023 •

edited

Loading