Reckless #5647

endothermicdev · 2022-10-06T22:00:05Z

reckless

A Core-Lightning plugin manager

Features

Love Core-Lightning? Want to install and test a CLN plugin with only three words and a command line? Reckless has you covered!

Install a plugin from the lightningd/plugins repository
Add additional source repos to search/install
Automatically install dependencies and verify installation
Dynamically enable/disable reckless-managed plugins
Persists plugin state by managing a config file inherited by lightningd

Commands

reckless install <plugin> searches available repositories, if one matches, clones it, installs dependencies, and tests that the plugin executes and exits normally. If so, the plugin is permanently installed, added to the CLN config, and dynamically activated.

reckless uninstall <plugin> disables the plugin, removes the directory.

reckless search <plugin> looks through all available sources for a plugin matching this name.

reckless enable <plugin>/reckless disable <plugin> dynamically enables/disables the reckless-installed plugin and updates the config to match.

reckless source list list all plugin repositories.

reckless source add <repo url> add another plugin repository for reckless to search or install from.

Let's Get Reckless!

Running the first time will prompt the user that their lightningd's bitcoin config will be appended (or created) to inherit the reckless config file (this config is specific to bitcoin by default.) Management of plugins will subsequently modify this file.

Start with a simple installation such as reckless install summary

Troubleshooting tips

Plugins must be executable. For python plugins, the shebang is invoked, so python3 should be available in your environment. This can be verified with which Python3. The default reckless directory is /home//.lightning/reckless and it should be possible for the lightningd user to execute files located here. If this is a problem, the option flag reckless -d=<my_alternate_dir> may be used to relocate the reckless directory from its default. Consider creating a permanent alias in this case.

For Developers

Tips for making your plugin compatible with reckless install:

Choose a unique plugin name.
The plugin entrypoint is inferred. Naming your plugin executable the same as your plugin name will allow reckless to identify it correctly (file extensions are okay.)
For python plugins, a requirements.txt is the preferred medium for python dependencies. A pyproject.toml will be used as a fallback, but test installation via pip install -e . - Poetry looks for additional files in the working directory, whereas with pip, any references to these will require something like packages = [{ include = "*.py" }] under the [tool.poetry] section.
Additional repository sources may be added with reckless source add https://my.repo.url/here however, https://github.com/lightningd/plugins is included by default. Consider adding your plugin lightningd/plugins to make installation simpler.
If your plugin is located in a subdirectory of your repo with a different name than your plugin, it will likely be overlooked.

To Do

This is the minimum viable product of the utility outlined by Rusty.

Still to come:

Man page
Support for non-python plugins.
Install by specifying url/dir directly.
Possibly support installing a virtual environments for python plugins.

A simple standalone python executable to track plugin repositories, clone to /tmp, install requirements, test plugin runs, then install and enable in lightningd and in the config. Changelog-Added: Reckless - a Core Lightning plugin manager

The user should be informed that their config now has a new source, but but any config files created downstream should be automatically populated.

cdecker

Thanks @endothermicdev, this is a great first iteration of reckless. Since this is the first python built-in tool destined for the end-user I went through it with a fine-toothed comb, so don't worry about the many (optional) comments.

Let me know if I can help, or some of the comments aren't clear 🤗

Makefile

tools/reckless

cdecker · 2022-10-14T15:47:10Z

tools/reckless

+
+if __name__ == '__main__':
+    parser = argparse.ArgumentParser()
+    parser.add_argument(dest='command', help='install/uninstall/search/enable'


Is there a way to customize the error when the command is missing? It'd be very useful for the error to spell out the available commands so we don't have to guess that help will get us that list.

tools/reckless

cdecker · 2022-10-14T15:57:29Z

tools/reckless

+    if cmd[0] == 'add':
+        for src in cmd[1:]:
+            add_source(src)
+    elif cmd[0] == 'remove' or cmd[0] == 'rem' or cmd[0] == 'rm':
+        for src in cmd[1:]:
+            remove_source(src)
+    elif cmd[0] == 'list':
+        list_source()


Should we add a help one? I had to look at the source to find out what the options are. Subparsers are your friend here :-)

tools/reckless

cdecker · 2022-10-14T17:22:53Z

tools/reckless

+def verbose(*args):
+    if not IS_VERBOSE:
+        return
+    print(*args)


Python has quite the powerful logging library at your disposal: https://docs.python.org/3/library/logging.html

cdecker · 2022-10-14T17:24:03Z

tools/reckless

+def lightning_cli_available():
+    clncli = Popen(['lightning-cli'], stdout=PIPE, stderr=PIPE)
+    clncli.wait(timeout=1)
+    if clncli.returncode == 0:
+        return True
+    else:
+        return False


We could also check if we have an executable on the $PATH:

from distutils.spawn import find_executable find_executable('lightning-cli')

Wow, this is so much nicer!

tools/reckless

This enables compatibility with startup_regtest.sh among other uses. The lightning-cli network flag is also set in case there is no config file.

Regtest environments commonly use explicit definition of the config file for lightningd. This can be queried and utilized by default, saving redundant definitions between lightning and reckless.

A more pythonic approach which should also enable additional help context for subcommands.

`reckless help <cmd>` previously called the function docstring. This could be updated to use the subparser help, but would require a strict naming convention or a dictionary. Providing a hint to use the built-in contextual help via the option flag is hopefully sufficient.

More pythonic than returning mixed types.

The goal was to support passing a list to install, enable, etc. in order to improve performance. Passing lists to most of the functions was less practical than iterating through the items from the top level.

This change makes it easier to follow retrieval of parent directories. Additional os.path operations replaced with their pathlib.Path equivalents to keep module usage consistent.

This also simplifies dynamic enable/disable by catching the exception raised when the cli is unable to connect to RPC (lightningd offline or misconfigured relative to reckless).

While loading the appropriate lightningconfig file, it is now checked against the active config file in lightningd. Because a deviation from the default file structure would not be possible, a -conf option is also added to explicitly pass the lightningd config file into reckless.

cdecker

Thanks for the fixups @endothermicdev, I only found a couple of cleanups that could also be done in a separate cleanup PR. I think the user-visible interface looks good, and we can clean up under the covers.

Which brings us to the question of whether you're happy to merge this as is, and I'd add it to the milestone for the release, or whether you'd like to hold it back, and we can include it in two months. WDYT?

cdecker · 2022-11-01T12:30:03Z

tools/reckless

@@ -82,14 +82,14 @@ class InstInfo:

 def create_dir(r: int, directory: str) -> bool:


You could make directory a Path instance, so you avoid converting it multiple times here. But that's more of a cleanup for later and not a blocker here.

cdecker · 2022-11-01T12:31:12Z

tools/reckless

@@ -196,14 +196,14 @@ class RecklessConfig(Config):
    def __init__(self, path: Union[str, None] = None,
                 default_text: Union[str, None] = None):
        if path is None:
-            path = os.path.join(LIGHTNING_DIR, 'reckless',
-                                'bitcoin-reckless.conf')
+            path = Path(LIGHTNING_DIR).joinpath('reckless',


This works also as Path(LIGHTNING_DIR) / 'reckless' / 'bitcoin-reckless.conf' since the Path library overrides __div__ giving it a custom interpretation of concatenation. Makes it look very unix-y :-)

Oh, and the right hand side can be a Path or str allowing you to avoid overly verbose type conversions too :-)

cdecker · 2022-11-01T12:35:56Z

tools/reckless

+    clncli = Popen(cmd, stdout=PIPE, stderr=PIPE)
+    clncli.wait(timeout=timeout)
+    out = clncli.stdout.read().decode()


You could replace these 3 lines with subprocess.check_output() which does the stdout handling by itself, and returns the output as bytes directly. Though since you have the code anyway, it might not be worth it.

See comment below regarding the handling of errors. check_output is likely not worth it here.

One thing that might be worthwhile is to set the text argument to have subprocess decode automatically.

Regarding the use of check_output: that would raise an Exception, potentially making the differentiation of error types harder further down. So you most likely don't want to change that.

cdecker · 2022-11-01T12:56:53Z

tools/reckless

-    cmd = LIGHTNING_CLI_CALL.copy()
-    cmd.extend(['plugin', 'start', path])
-    clncli = Popen(cmd, stdout=PIPE)
-    clncli.wait(timeout=3)


Might be worthwhile encapsulating the RPC into a separate class, or rely on pyln-client to interact with lightningd in a pythonic way.

And of course this is in direct contradiction with my earlier comment 🤦 Feel free to ignore this 😉

cdecker · 2022-11-01T12:58:53Z

tools/reckless

-        RECKLESS_CONFIG.enable_plugin(path)
-        print('{} enabled'.format(plugin_name))
-    else:
-        err = eval(clncli.stdout.read().decode().replace('\n', ''))['message']


Uff, how did I miss eval here? That opens you up to arbitrary code executions should a plugin be malicious it might be able to use this as a way to escalate privileges from an unprivileged user running the node, to a user with more privileges that is managing the node through reckless. Good think you removed it here.

Reckless was failing to install multiple plugins due to git not appreciating the cwd being a now removed dir after the first plugin tmp files were cleaned up.

cdecker

Good fixes, but I think we probably want to lose that last commit.

cdecker · 2022-11-07T17:18:25Z

tools/reckless

    os.chdir(plugin_path)
+    assert os.getcwd() == str(plugin_path)


This is likely unexpected for the user since they'll be dropped into a temporary directory if anything goes wrong. You can use the cwd argument to Popen to have it change the directory in the subprocess before exec'ing the executable, thus leaving the parent process in its current working directory unchanged.

Let's do that in a cleanup PR, since this is just a minor fix.

Makefile

cdecker · 2022-11-07T17:23:29Z

tools/Makefile

+tools/pip-install:
+	@if which $(PIP_EXE) >> /dev/null; then \
+		echo "installing python dependencies"; \
+		$(PIP_EXE) $(PIP_ARGS) install -r tools/requirements.txt ; \
+	else \
+		(echo "Pip not installed, skipping python dependencies") ; \
+	fi


Ok, I see where this comes from, but it most likely isn't what we want:

Developer dependencies are tracked in pyproject.toml

There is currently no user dependencies (other than libraries from the distro), so we need something here. On the other hand the users will not be the ones to run Makefiles, so we better look for some better option. Alternatively we could just use urllib3 and not have dependencies at all.

Potentially reckless could be a completely separate tool that we distribute on PyPI where it can have its own dependencies, and users would just be a pip install reckless away from being able to run it.

I'm certainly open to a better option for installing python dependencies for the user (or at least prompting a pip install.) I think we'll want that at some point. Let's just switch to urllib3 for the time being? Distributing reckless via PyPI is fine, but I would still prefer it to be updated and working by default with the latest CLN release if possible.

endothermicdev added 2 commits October 6, 2022 14:49

reckless: it turns out the warning is a bit much.

e1a8c15

The user should be informed that their config now has a new source, but but any config files created downstream should be automatically populated.

endothermicdev added the plugin label Oct 6, 2022

endothermicdev added this to the v22.11 milestone Oct 6, 2022

cdecker reviewed Oct 14, 2022

View reviewed changes

endothermicdev added 5 commits October 19, 2022 08:54

reckless: use the lightning path when invoking lightning-cli

8589067

This enables compatibility with startup_regtest.sh among other uses. The lightning-cli network flag is also set in case there is no config file.

reckless: detect pip3 or pip

8fc1911

reckless: use config that was explicitly passed to lightningd

db8cbce

Regtest environments commonly use explicit definition of the config file for lightningd. This can be queried and utilized by default, saving redundant definitions between lightning and reckless.

reckless: use argparse subparsers

1fd1b58

A more pythonic approach which should also enable additional help context for subcommands.

endothermicdev marked this pull request as draft October 21, 2022 18:23

endothermicdev added 7 commits October 21, 2022 13:52

reckless: raise exception or early termination instead of returning None

19bb3f5

More pythonic than returning mixed types.

reckless: add type hints

0ecd0a0

reckless: refactor argument list handling.

dacd4c8

The goal was to support passing a list to install, enable, etc. in order to improve performance. Passing lists to most of the functions was less practical than iterating through the items from the top level.

reckless: replace os.path with pathlib operations

104d234

This change makes it easier to follow retrieval of parent directories. Additional os.path operations replaced with their pathlib.Path equivalents to keep module usage consistent.

reckless: multiline string style cleanup

1866ee8

reckless: add function for lightning-cli calls

68b5bb4

This also simplifies dynamic enable/disable by catching the exception raised when the cli is unable to connect to RPC (lightningd offline or misconfigured relative to reckless).

endothermicdev force-pushed the reckless branch from b89911a to 1e8ea76 Compare October 26, 2022 22:25

reckless: analyze repositories with urlparse

2ce8d67

cdecker approved these changes Nov 1, 2022

View reviewed changes

endothermicdev added 2 commits November 2, 2022 18:05

Reckless: add man page

c8460f0

reckless: fix git clone issue with removed dir

6725934

Reckless was failing to install multiple plugins due to git not appreciating the cwd being a now removed dir after the first plugin tmp files were cleaned up.

endothermicdev force-pushed the reckless branch from 37d64ca to 6725934 Compare November 2, 2022 23:06

endothermicdev marked this pull request as ready for review November 4, 2022 01:21

cdecker requested changes Nov 7, 2022

View reviewed changes

endothermicdev force-pushed the reckless branch from dedf801 to 6725934 Compare November 7, 2022 22:13

cdecker approved these changes Nov 8, 2022

View reviewed changes

cdecker merged commit 341d73f into ElementsProject:master Nov 8, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reckless #5647

Reckless #5647

endothermicdev commented Oct 6, 2022

cdecker left a comment

cdecker Oct 14, 2022

cdecker Oct 14, 2022

cdecker Oct 14, 2022

cdecker Oct 14, 2022

endothermicdev Oct 14, 2022

cdecker left a comment

cdecker Nov 1, 2022

cdecker Nov 1, 2022

cdecker Nov 1, 2022

cdecker Nov 1, 2022

cdecker Nov 1, 2022

cdecker Nov 1, 2022

cdecker Nov 1, 2022

cdecker Nov 1, 2022

cdecker Nov 1, 2022

cdecker left a comment

cdecker Nov 7, 2022

cdecker Nov 7, 2022

endothermicdev Nov 7, 2022

		@@ -82,14 +82,14 @@ class InstInfo:

		def create_dir(r: int, directory: str) -> bool:

Reckless #5647

Reckless #5647

Conversation

endothermicdev commented Oct 6, 2022

reckless

Features

Commands

Let's Get Reckless!

Troubleshooting tips

For Developers

To Do

cdecker 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

cdecker 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

cdecker 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