Clnrest: dynamic paths

[daywalker90]

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 GET and POST and arbitrary content-type's for responses. The most specific path is chosen if there are multiple matches.

lightningd is responsible for making sure no ambigous paths like /path/to/<me> and /path/to/<you> are registered.

[niftynei]

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.

[daywalker90]

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?

[niftynei]

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 path explicitly.

[gudnuf]

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

[ShahanaFarooqui]

@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!

[rustyrussell]

@daywalker90 Can you please rebase so we can finally merge this, if you're still happy?

[daywalker90]

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:

add an optional "version" field which defaults to the current CLN rest one. it can be set to 'null' which removes it entirely.

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...

[ShahanaFarooqui]

So if someone could review the C + pyln-client code and add this part from the feedback:

Assigned it to @rustyrussell as he agreed to review (and update if needed) the C and pyln-client code.

[rustyrussell]

Re v1/ path:

1. We can simply stick with v1 today. Handling other urls is tomorrow's problem
2. Make sure the custom handler gets the whole path, including /v1/. That way they don't have to change anything if we start supporting arbitrary prefixes in future.

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:

1. You can certainly make the pyln wrapper do this call automatigically during init as a courtesy.
2. Make sure the path starts with /v1/ (and fail it for now if it doesn't)
3. Make sure clnrest can handle calls to clnrest-register-path before init (save them up until after init), because this could actually happen.
4. Not sure if python plugins should fail gracefully if the call fails. That way they automatically disable themselves if clnrest is disabled for some reason?

[daywalker90]

3. Make sure clnrest can handle calls to clnrest-register-path before init (save them up until after init), because this could actually happen.

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.

[rustyrussell]

3. Make sure clnrest can handle calls to clnrest-register-path before init (save them up until after init), because this could actually happen.

I don't know how to do that, pretty sure this requires some changes to cln-plugin aswell?

Actually, here's how it works at startup:

1. We call getmanifest on everyone, wait until they all respond.
2. We call init on everyone at once.

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).

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.

Yeah, understood. But I think it mainly "just works".