-
Notifications
You must be signed in to change notification settings - Fork 976
Clnrest: dynamic paths #7529
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
Clnrest: dynamic paths #7529
Conversation
2e2d66e to
8c16178
Compare
plugins/clnrest-plugin/src/main.rs
Outdated
| @@ -131,7 +153,7 @@ async fn main() -> Result<(), anyhow::Error> { | |||
| "/v1", | |||
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 v1 to all custom endpoints might prove problematic if someone's attempting to match an external spec.
In the case of cashu, which is currently v1, this is fine but in the case that cashu moves to v2 this will cause problems.
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.
That's a very good point. The versions are not correlated and could change independently. Do we need to add a version field to the clnrest manifest data?
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.
it's also possible to have specs that don't include the version part of the path!
I think there's two options here:
- add an optional "version" field which defaults to the current CLN rest one. it can be set to 'null' which removes it entirely.
- don't add the version to the endpoint but instead have rpcs add it to their
pathexplicitly.
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.
also would be problematic for cashu if clnrest moves to v2, but cashu does not
add an optional "version" field which defaults to the current CLN rest one. it can be set to 'null' which removes it entirely.
This would help maintain a static endpoint if clnrest bumps its version
|
@daywalker90 Thank you for the PR! I’m moving it to the next milestone since it enhances our current clnrest plugin. For this release, our priority is to smoothly replace the Python plugin with the Rust version. Once that’s done, we can introduce this and other enhancements in future releases. Apologies for the delay! |
|
@daywalker90 Can you please rebase so we can finally merge this, if you're still happy? |
6e8c391 to
9cc1ff8
Compare
|
Ok so i rebased this but it's not ready to merge. niftynei's feedback regarding versioning is not yet addressed, i remember vividly that there was something wrong with the rust code aswell but can't remember and i don't even know what gudnuf didn't finish in #7507. So if someone could review the C + pyln-client code and add this part from the feedback:
that would help alot. Remember i was contacted by gudnuf with this, i just wanted to do the rust side of clnrest, not design the new manifest. I don't know what people need... |
Assigned it to @rustyrussell as he agreed to review (and update if needed) the C and pyln-client code. |
|
Re v1/ path:
But more importantly, I think you don't need to put the clnrest path in the manifest; that's a bit weird. Just implement a "clnrest-register-path" call directly in clnrest! Points:
|
I don't know how to do that, pretty sure this requires some changes to cln-plugin aswell? If i could do that the "clnrest-register-path" call could take in everything and we could drop the whole manifest approach. But that's why we chose that approach because nobody thought we could catch all pre-init calls. |
Actually, here's how it works at startup:
This means you will never get called before init if you're not dynamically loaded (the call will get queued behind init), BUT you can get called before init returns (if you're doing async stuff, for example). (Note: there's an important exception for db hooks, which are called from lightningd itself, but they're magic and special).
Yeah, understood. But I think it mainly "just works". |
|
I would love to see this merged for the cln release early H1 2026.... |
9cc1ff8 to
5a7de0c
Compare
|
Ok i took in all the feedback and noticed: hey! i can do that.
The new command takes a I dropped both I had to rearrange some routing logic to make it all work and after finally understanding that axum layers are like onions where the last layer gets applied first. That means unauthorized PR also includes a little refactoring of structs to a new file and some minor drive-by fixes of unnecessary clones and deprecated dependencies. Let me know what you think! |
|
Oh and about this:
turns out
|
bb6a7ae to
bd4b2d6
Compare
|
I've changed the behaviour of not setting the rune parameter as per the discussion in the CLN meeting. If you call It was also discussed to not add any methods to delete routes in this PR and saving routes is also out of scope for this PR. The question remains if the clnrest plugin should stay |
bd4b2d6 to
72b68a5
Compare
ShahanaFarooqui
left a comment
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.
Dynamic plugin
Dynamic plugin loading makes REST functionality optional, ensuring zero resource consumption for users who don’t require REST capabilities. For now, plugins that require full readiness before handling messages can work around this by awaiting all asynchronous initialization before returning from init. We can revisit this approach if a compelling use case arises in the future.
Documentation location
Should clnrest-register-path be listed in the JSON-RPC documentation, or would it be better to add it to doc/developers-guide/plugin-development/a-day-in-the-life-of-a-plugin.md, or document it in a new file for the clnrest passthrough (like doc/developers-guide/plugin-development/json-rpc-passthrough.md)?
Rename rune to rune_restrictions
Rename the rune argument to rune_restrictions in clnrest-register-path to avoid confusion between rune values and restriction rules, and to clearly indicate the argument’s purpose.
Add rune_required argument
- Type: Boolean
- Default:
True - Behavior:
True: ReturnNot authorized: Missing runewhen the rune header is absentFalse: Allow requests without a rune (useful for read-onlyGETendpoints such as/version,/info, etc.)- Should
rune_required: Falsebe allowed only whenmethod_type == GET, and default toTruefor all other methods?
Add HTTP method types
- Valid values:
GET,POST,PUT,PATCH,DELETE - Default:
POST - Benefits:
- Semantic clarity - intent is immediately obvious
- RESTful design - enables resource-oriented API patterns
- Safety guarantees -
GEToperations are read-only and idempotent - Caching optimization - intermediaries can safely cache
GETresponses - Path reusability - a single path can support multiple operations:
GET /user/123-> Retrieve user detailsPOST /user/123-> Create a new userPUT /user/123-> Update an existing userDELETE /user/123-> Remove the user
Content negotiation headers
I suggest explicitly supporting content negotiation headers for completeness, but we can handle this as a separate enhancement in a future PR too.
-
Suggested arguments:
content_type_header: Add this argument to specify the request body formataccept_header: Add this argument to specify the desired response format
-
Supported values:
application/jsonapplication/xmlapplication/yamltext/plaintext/htmlapplication/x-www-form-urlencoded
-
Current issues:
application/json,application/xml, andapplication/yamlwork as expected- Requests with
Accept: text/plain,text/html, orapplication/x-www-form-urlencodedreturn 406 Not Acceptable - When the
Acceptheader is missing, responses default to a JSON string, requiring additional client-side parsing for text, HTML, or form-encoded use cases
-
Benefits:
- Production readiness - explicit declarations are preferable to implicit assumptions
- Future-proofing - enables content negotiation and graceful API evolution without breaking changes
- Plugin flexibility - gives plugin authors control over input and output formats
3f47da8 to
0887f06
Compare
|
Thank you for your feedback, Shahana!
Right, i was just thinking about other plugins. They need to deal with a suddenly appearing/disappearing clnrest if it stays "dynamic" and we don't remember their dynamic routes.
You are talking about the
done
done, it's now
Difficult to say, maybe get some feedback from people who are going to use this?
We could do this for dynamic routes, but there is a problem: How do you pass on the http method to the rpc method? A
If you think explicit declarations are preferred and clnrest should reject supported but not declared content types i can easily do that.
That is expected since we currently don't support
That is not that unusual i think, some REST servers don't even accept |
[Shahana]: After discussing with @rustyrussell, we agreed to leave this as-is for now. While there are solutions to address this issue, we'll implement them if a real-world use case requires it.
[Shahana]: Per @rustyrussell 's feedback, Additional recommendations:
[Shahana]: Let's keep
[Shahana]: I would prefer a separate rpc_method for each "http_method+path" combination as a cleaner approach. We have two options for that:
[Shahana]: I don't expect explicit declarations for all content types (the default JSON response handling is perfect). However, when clients explicitly declare common content types in headers (like text/plain or text/html), and the plugin developer is prepared to handle and respond in those formats, we shouldn't return a # This works - defaults to JSON
dynamic_res = http_session.post(
base_url + "/return/json",
headers={"Rune": rune},
data={'data': 'Returns JSON'},
verify=ca_cert
)
dynamic_res.raise_for_status()
assert dynamic_res.json()["data"] == "Returns JSON"
# Throws 500 Server Error (should work - explicit JSON content-type)
dynamic_res = http_session.post(
base_url + "/return/json",
headers={"Rune": rune, 'Content-type': 'application/json'},
data={'data': 'Returns JSON'},
verify=ca_cert
)
dynamic_res.raise_for_status()
assert dynamic_res.json()["data"] == "Returns JSON"
# Throws 406 Client Error
dynamic_res = http_session.post(
base_url + "/return/text",
headers={"Rune": rune, 'Content-type': 'text/plain'},
data="I am returning text",
verify=ca_cert
)
dynamic_res.raise_for_status()
assert dynamic_res.text == "I am returning text"
# Throws 406 Client Error
dynamic_res = http_session.post(
base_url + "/return/html",
headers={"Rune": rune, 'Content-type': 'text/html'},
data="<h1>I am returning HTML</h1>",
verify=ca_cert
)
dynamic_res.raise_for_status()
assert dynamic_res.text == "<h1>I am returning HTML</h1>"The server should accept these common content types and pass the raw content to the plugin method as a parameter, allowing the plugin developer to handle it appropriately and return the response in the format the client expects.
[Shahana]: If we can easily provide this feature, why limit content format support to just JSON? If supporting common formats like
[Shahana]: Yeah, this one was specifically for Content-Type. My bad for lumping them together 😛.
[Shahana]: If this can be implemented easily, it is good to have. But, if the implementation is complex, the current setup is reasonable too. Thank you for working through all this feedback! The PR is looking great. ALMOST THERE!!! 👏👏👏 |
0887f06 to
1d5296d
Compare
Changelog-Added: clnrest: add clnrest-register-path rpc method to register dynamic paths
1d5296d to
37ed9ba
Compare
|
It's now
We have a passing test for this, you can't do it like that: When you pass a dict to data=, requests sends it as form-encoded data (application/x-www-form-urlencoded), which conflicts with your header. You must use I have a commit ready to support any |
[Shahana]: Ah, that’s a perfect excuse for not testing while sleepy 😄. As long as it works, we are good! 🚀
[Shahana]: Let’s keep it as is for now and address this if a future requirement arises. |
ShahanaFarooqui
left a comment
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.
ACK 37ed9ba.
|
This is glorious. 🧡 |
Naughtyknucks
left a comment
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.
G
based on #7507, #7508 and #7509
Adds the capability for clnrest to handle dynamic paths registered via a plugin's manifest. Supports the http methods
GETandPOSTand arbitrarycontent-type's for responses. The most specific path is chosen if there are multiple matches.lightningdis responsible for making sure no ambigous paths like/path/to/<me>and/path/to/<you>are registered.