From eae0d1da1ed628e7ce18b347349459a08113c8f3 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Fri, 17 Nov 2023 10:08:06 +1030 Subject: [PATCH] lightningd: fix up deprecated `rest-port`, `rest-protocol`, `rest-host` and `rest-certs` option if we would otherwise fail. Since these worked in v23.08, we can't just rename them. So if they are used and unclaimed, we should rename them internally (if they're claimed, it's probably clightning-rest, and we should *NOT* touch them!). Signed-off-by: Rusty Russell Changelog-Deprecated: Plugins: `clnrest` parameters `rest-port`, `rest-protocol`, `rest-host` and `rest-certs`: prefix `cln` to them --- lightningd/options.c | 35 ++++++++++++++++++++++++++++ tests/plugins/clnrest-use-options.py | 18 ++++++++++++++ tests/test_clnrest.py | 33 ++++++++++++++++++++++++++ 3 files changed, 86 insertions(+) create mode 100755 tests/plugins/clnrest-use-options.py diff --git a/lightningd/options.c b/lightningd/options.c index 3ace67d97..dddd5d736 100644 --- a/lightningd/options.c +++ b/lightningd/options.c @@ -1811,8 +1811,43 @@ void handle_early_opts(struct lightningd *ld, int argc, char *argv[]) logging_options_parsed(ld->log_book); } +/* Free *str, set *str to copy with `cln` prepended */ +static void prefix_cln(const char **str STEALS) +{ + const char *newstr = tal_fmt(tal_parent(*str), "cln%s", *str); + tal_free(*str); + *str = newstr; +} + +/* Due to a conflict between the widely-deployed clightning-rest plugin and + * our own clnrest plugin, and people wanting to run both, in v23.11 we + * renamed some options. This breaks perfectly working v23.08 deployments who + * don't care about clightning-rest, so we work around it here. */ +static void fixup_clnrest_options(struct lightningd *ld) +{ + for (size_t i = 0; i < tal_count(ld->configvars); i++) { + struct configvar *cv = ld->configvars[i]; + + /* These worked for v23.08 */ + if (!strstarts(cv->configline, "rest-port=") + && !strstarts(cv->configline, "rest-protocol=") + && !strstarts(cv->configline, "rest-host=") + && !strstarts(cv->configline, "rest-certs=")) + continue; + /* Did some (plugin) claim it? */ + if (opt_find_long(cv->configline, &cv->optarg)) + continue; + log_unusual(ld->log, "Option %s deprecated in v23.11, renaming to cln%s", + cv->configline, cv->configline); + prefix_cln(&cv->configline); + } +} + void handle_opts(struct lightningd *ld) { + if (ld->deprecated_apis) + fixup_clnrest_options(ld); + /* Now we know all the options, finish parsing and finish * populating ld->configvars with cmdline. */ parse_configvars_final(ld->configvars, true, ld->developer); diff --git a/tests/plugins/clnrest-use-options.py b/tests/plugins/clnrest-use-options.py new file mode 100755 index 000000000..1c520bcaf --- /dev/null +++ b/tests/plugins/clnrest-use-options.py @@ -0,0 +1,18 @@ +#!/usr/bin/env python3 +"""Register rest-port to test that we don't "fix" it.""" + +from pyln.client import Plugin + + +plugin = Plugin() + + +@plugin.init() +def init(configuration, options, plugin): + print(f"rest-port is {plugin.get_option('rest-port')}") + + +plugin.add_option('rest-port', None, "Parameter to clash with clnrest.py deprecated one") + + +plugin.run() diff --git a/tests/test_clnrest.py b/tests/test_clnrest.py index a9b3daea9..195880d02 100644 --- a/tests/test_clnrest.py +++ b/tests/test_clnrest.py @@ -3,6 +3,7 @@ from fixtures import * # noqa: F401,F403 from pyln.testing.utils import env, TEST_NETWORK from pyln.client import Millisatoshi import unittest +import os import requests from pathlib import Path from requests.adapters import HTTPAdapter @@ -433,3 +434,35 @@ def test_clnrest_http_headers(node_factory): headers={'Origin': 'http://192.168.1.10:1010'}, verify=ca_cert) assert response.headers['Access-Control-Allow-Origin'] == 'http://192.168.1.10:1010' + + +def test_clnrest_old_params(node_factory): + """Test that we handle the v23.08-style parameters""" + rest_port = str(reserve()) + rest_host = '127.0.0.1' + base_url = f'https://{rest_host}:{rest_port}' + l1 = node_factory.get_node(options={'rest-port': rest_port, + 'rest-host': rest_host, + 'allow-deprecated-apis': True}) + # This might happen really early! + l1.daemon.logsearch_start = 0 + l1.daemon.wait_for_logs([r'UNUSUAL lightningd: Option rest-port=.* deprecated in v23\.11, renaming to clnrest-port', + r'UNUSUAL lightningd: Option rest-host=.* deprecated in v23\.11, renaming to clnrest-host']) + l1.daemon.wait_for_log(r'plugin-clnrest.py: REST server running at ' + base_url) + + # Now try one where a plugin (e.g. clightning-rest) registers the option. + plugin = os.path.join(os.path.dirname(__file__), 'plugins/clnrest-use-options.py') + l2 = node_factory.get_node(options={'rest-port': rest_port, + 'rest-host': rest_host, + 'plugin': plugin, + 'allow-deprecated-apis': True}) + + l2.daemon.logsearch_start = 0 + # We still rename this one, since it's for clnrest. + assert l2.daemon.is_in_log(r'UNUSUAL lightningd: Option rest-host=.* deprecated in v23\.11, renaming to clnrest-host') + + # This one does not get renamed! + assert not l2.daemon.is_in_log(r'UNUSUAL lightningd: Option rest-port=.* deprecated in v23\.11, renaming to clnrest-host') + assert [p for p in l2.rpc.plugin('list')['plugins'] if 'clnrest.py' in p['name']] == [] + assert l2.daemon.is_in_log(r'plugin-clnrest.py: Killing plugin: disabled itself at init: `clnrest-port` option is not configured') + assert l2.daemon.is_in_log(rf'clnrest-use-options.py: rest-port is {rest_port}')