-
Notifications
You must be signed in to change notification settings - Fork 913
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add clnrest object to plugin manifest #7507
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
few minor comments, overall looks great.
the checks found some extra whitespace.
lightningd/plugin.c:1388:static const char *plugin_rpcmethod_clnrest_add(struct plugin *plugin,
lightningd/plugin.c:1389: const char *buffer,
lightningd/plugin.c:1390: const jsmntok_t *tok,
Extraneous whitespace found
lightningd/plugin.c:1391: struct json_command *cmd)
contrib/msggen/msggen/schema.json
Outdated
"clnrest": { | ||
"type": "object", | ||
"additionalProperties": false, | ||
"required": [], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you add this are any of the fields required? maybe path
and content_type
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yea, this is the response for the help command. So if all of the fields have default values, then these would all be required, yea?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I set all these fields to required in 61c471a
That makes sense right?
lightningd/plugin.c
Outdated
if (pathtok) { | ||
clnrest->path = json_strdup(clnrest, buffer, pathtok); | ||
} else { | ||
clnrest->path = json_strdup(clnrest, buffer, pluginnametok); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: i think technically the 'name' here is the name of the plugin command, not the plugin?
might be worthwhile to add a note somewhere that it defaults to the command name.
am i right in thinking this will make it such that clnrest will dynamically add a GET endpoint for every new plugin method that's added?? incredible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note that if that's the case we might have some default /v1/ etc prefix to add to the name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'name' here is the name of the plugin command, not the plugin
oo yea you're right
am i right in thinking this will make it such that clnrest will dynamically add a GET endpoint for every new plugin method that's added??
Only if that method includes clnrest data in response to get_manifest
. A plugin might want to specify the return content type, but nothing else...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some default /v1/ etc prefix
added this prefix in d52a311
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This manifest is not being reflected in clnrest yet. For example, below method will still require rune header and respond in json format:
@plugin.method("infotext", clnrest_data={"content_type": "text/html", "rune": False})
def info_text(plugin, **kwargs):
"""This methods sends text content!"""
message = "--------------------------------\n"
message += "Id: 0253afcb231426"
message += "\n--------------------------------\n"
message += "Alias: CLN"
message += "\n--------------------------------\n"
message += "Version: v24.05"
message += "\n--------------------------------\n"
return message
We should update file plugins/clnrest/utilities/rpc_routes.py
from line 31 for above changes to reflect in clnrest also.
@rpcns.route("/<rpc_method>")
class RpcMethodResource(Resource):
@rpcns.doc(security=[{"rune": []}])
@rpcns.doc(params={"rpc_method": (f"Name of the RPC method to be called")})
@rpcns.expect(payload_model, validate=False)
@rpcns.response(201, "Success")
@rpcns.response(500, "Server error")
def post(self, rpc_method):
"""Call any valid core lightning method (check list-methods response)"""
try:
rune = request.headers.get("rune", None)
rpc_method = request.view_args.get("rpc_method", None)
rpc_params = request.form.to_dict() if not request.is_json else request.get_json() if len(request.data) != 0 else {}
method_manifest = call_rpc_method(plugin, "help", [rpc_method]).get('help', [])[0].get('clnrest', {})
try:
if method_manifest.get('rune', True):
verify_rune(plugin, rune, rpc_method, rpc_params)
except RpcError as rpc_err:
plugin.log(f"RPC Error: {str(rpc_err.error)}", "info")
return rpc_err.error, 401
except RuneError as rune_err:
plugin.log(f"Rune Error: {str(rune_err)}", "info")
return rune_err.error, 403
try:
response = call_rpc_method(plugin, rpc_method, rpc_params)
if method_manifest.get("content_type", "application/json") == "application/json":
return response, 201
else:
response = make_response(response)
response.headers["Content-Type"] = method_manifest['content_type']
return response
except RpcError as rpc_err:
plugin.log(f"RPC Error: {str(rpc_err.error)}", "info")
return (rpc_err.error, 404) if rpc_err.error["code"] == -32601 else (rpc_err.error, 500)
except Exception as err:
return f"Unable to parse request: {err}", 500
path: str | ||
method: str |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding path and method seems useless as these cannot be changed for clnrest. Path will always be the name of the rpc command (@rpcns.route("/<rpc_method>")
) and method is always POST (def post(self, rpc_method)
). We can add GET, DELETE etc. methods types in clnrest but it seems unnecessary because POST can be used for all types.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking clnrest could be modified to register path and method dynamically. Already been talking to @daywalker90 about adding that feature to his rust version of clnrest (#7509)
This is why I also opened #7508, so that clnrest could dynamically add routes as new plugins start, and remove those routes when plugins stop.
True that POST can be used for all types, but a plugin may want to implement a specific protocol that defines what the method should be for some endpoint. Note "Motivations" above...
61c471a
to
7dc3fe3
Compare
rebased to 7dc3fe3 |
Hi! This is a really intriguing PR: great work!
I think this is not quite as powerful as we want, in general? I think we want to specify what powers the rune has to grant? So instead of runes: boolean, we have something like: e.g. In general, we want to allow MULTIPLE options, and the rune must match at least one? The above example is kinda nonsensical, but it means we test the rune against category=readonly, method=pay etc. Missing (or JSON More difficult, but more powerful! You can have an endpoint whihc you can only access if you have a rune which would let you call "pay" for example. Or, make "method": "clnrest-foo" and you need a specific rune for that! |
Tagged for 25.02! |
Allows a plugin to register custom REST
method
,path
, and returncontent_type
for any of its RPC methods along with arune
flag that signifies whether or not authentication should be applied to this route.Motivation
We would like plugins to be able to customize how they are used via a REST interface such as clnrest. An example use case is a Cashu mint plugin that must follow the Cashu protocol for each of the endpoints, so each RPC method must be mapped to a specific REST route (ie
GET /v1/keys
).Will also help with #7164
What changed?
plugin_rpcmethod_clnrest_add
tolightningd/plugin
which will parse out clnrest data from theget_manifest
response. For each RPC method, the get_manifest response can now contain:Where:
method
: The HTTP method (e.g., "GET")path
: The endpoint path (e.g., "/path/to/me")content_type
: The content type for the return value (e.g., "application/json")rune
: Whether or not this route requires a rune to be accessedjsonrpc_command_add
so that we can check for name collisions and method/path collisions. Will now return false if there is an RPC method with the same name or an RPC method that registers the same route (ie. matching path + method)json_add_help_command
so the output oflightning-cli help
can return clnrest data:clnrest_data
toplugin.py
so thatpyln-client
can add this to theget_manifest
responsetests/plugins/get_manifest.py
to test this functionalityNext steps
I will be submitting another PR that adds two new notifications:
plugin_started
andplugin_stopped
.This will allow other plugins such as clnrest to subscribe to these notifications and dynamically register and drop routes.