openwrt
iperf3: add uci server handling
Maintainer: @pprindeville Compile tested: not needed only script changes Run tested: x86_64, APU3, latest openwrt master (iperf3 with uci client/server)
Server iperf3 uci config
config server lan
option enabled '1'
option port '5202'
option iface 'lan'
option redirect '1'
Client iperf3 uci config:
config client lan
option server '192.168.0.53'
option port '5202'
option proto 'ipv4'
option redirect '1'
Execute the command /etc/init.d/perf3 client lan to start a scan against the server und the client device
Description:
As discussed an suggest in the P/R I have extended the iperf3. With the following improvements.
- Add uci hanndling
- Redirect output to
ubus listen luci.iperf3.notify.dataAs for the path, I'm not sure whether we should use luci or something else. Suggestions are welcome ;-)
This commits adds the possibility that iperf3 can be configured via
the uci. The functionality has also been extended so that the output of
iperf3 (server/client) sends a notification event via the ubus to the luci.iperf3.notify.data endpoint.
@stangri @wjowsa Could you also look at my proposal? I think this is a generally valid proposal and would simplify the LuCI. You just have to figure out how to subscribe to the ubus notification in the LuCI and show them.
Then, from my point of view, we would no longer need this patch and could move on. As discussed with @jow- a while a go this has some security issues.
@fecker your proposal looks good and I agree that the frontend might be simpler with the one exception. This approach eliminates using of uhttpd ubus API and server-side events. It means that Luci would have to somehow poll for the output of the iperf3 that might worse UX.
If possible, please send comments to the patch with the description of security issues so I could address them.
@feckert I've had a few comments, maybe @wjowsa would want to implement these after merging your proposal.
@feckert your proposal looks good and I agree that the frontend might be simpler with the one exception. This approach eliminates using of uhttpd ubus API and server-side events.
I have not taken a closer look. But I can also subscribe to the event luci.iperf3.notify.data, can't I? I do the same as you in the LuCI. Only I do the whole thing in the iperf3 init script.
It means that Luci would have to somehow poll for the output of the iperf3 that might worse UX.
That's what we do in the Luci on different places. We add a poll.add function.
If that doesn't work with the ubus event, then we could always poll the log output from iperf3 and display that. That would be even easier, of course. There's not that much data, if that's your concern.
If possible, please send comments to the patch with the description of security issues so I could address them.
Unfortunately, I don't remember. It's been a while since I chatted with him about this topic on IRC. But I could remember that he said that the patch cannot be accepted in this way. I think you have to ask him. Sorry
@feckert your proposal looks good and I agree that the frontend might be simpler with the one exception. This approach eliminates using of uhttpd ubus API and server-side events.
I have not taken a closer look. But I can also subscribe to the event luci.iperf3.notify.data, can't I? I do the same as you in the LuCI. Only I do the whole thing in the iperf3 init script.
It looks the same but it is not the same. You listen for events but I subscribe to an object. More, subscribing to an ubus object is supported by uhttpd so data is easier and in a more elegant way accessible from the browser application.
It means that Luci would have to somehow poll for the output of the iperf3 that might worse UX.
That's what we do in the Luci on different places. We add a poll.add function.
Yes but I think we could start using a new uhttpd ubus API and utilize server-side events instead of polling. This could be a starting point for the unification of applications that read data from the backend - you just create an ubus object on the backend side, subscribe to the object on the frontend side.
If that doesn't work with the ubus event, then we could always poll the log output from iperf3 and display that. That would be even easier, of course. There's not that much data, if that's your concern.
Yes, that is what I would like to avoid :)
@wjowsa Thank you for your explanation, now I understand. We have two possibilities
-
publish/subscribe: publisher lua reference implementation: https://git.openwrt.org/?p=project/ubus.git;a=blob_plain;f=lua/publisher.lua;hb=refs/heads/master subcriber lua reference implementation: https://git.openwrt.org/?p=project/ubus.git;a=blob_plain;f=lua/subscriber.lua;hb=refs/heads/master
-
listen/send send event via ubus:
ubus send luci.iperf3.status '{"message":"<message>"}'listen on events via ubus:ubus listen luci.iperf3.status
As far as I can see, the ubus API only offers the publish/subcribe model currently. Events cannot be output yet over the API, can they? But would be quite useful here in my opinion
For security reasons, I found the following article on the internet. https://exceptionshub.com/http-authorization-header-in-eventsource-server-sent-events.html As far as I can remember, this was also the concern of @jow- .
@feckert Yes, ubus API offers the publish/subcribe model currently. Even if events were part of the API then I think we will end up with the same issue with passing the authentication token / ubus session id.
I know that passing a token through the URL might not look secure enough. What I would like to know is how it is less secure from the way the sid is passed currently from Lucy to the uhttpd.
I know that passing a token through the URL might not look secure enough. What I would like to know is how it is less secure from the way the sid is passed currently from Lucy to the uhttpd.
Your suggested implementation uses the ubus_rpcd session id, which does not change during a session. The timeout is also restarted when a new request is made. The suggestion from the link would generate a token that is valid once per request.
In summary, I would say that the difference is that there is no timeout for the token, but the token is only valid once. This is not the case for the ubus session id. If you have connected once, the session id remains valid until you have logged out or the timeout strikes.
I am not an expert in this field, I can also get this wrong. But these are the things I have found on the subject.
Sorry, I don't know enough about LuCI to say one way or another... I'm on the sidelines waiting for you to converge on consensus and then I'll merge...
@feckert @wjowsa I know the conversation digressed towards WebUI use of the package, this PR does simplify the CLI use quite a bit tho, you should consider merging it.
I have add my latest changes to this pullrequest. @stangri could you please review?
@stangri Would you be so kind again and do a review or an integration test?
Sorry @feckert I will not be able to provide a meaningful response until late next week, if you want to merge it before then, go ahead, even my previous comments weren't about substantial issues.
GitHub WebUI is confusing, I'm still seeing the old code when browsing Files Changed of the PR and the new code when clicking on the hash (https://github.com/openwrt/packages/commit/4f8155e5eaed5f334d5dcfc4578284f85dd265e8).
Great work on that last commit!
@nbd168 Do you have any comments on my change? If not, I would like to merge it.
I did not pursue this any further, as no reviewer was found. I then moved it to my own package feed, as it doesn't necessarily have to be integrated in the iperf3 package. If it is useful, we can integrate it, but I need more feedback.
It is useful for me (instead of having a line in /etc/rc.local).
I cherry-picked this (and changed v3.11 to v3.15). Server mode works well fine (including manual reload when I change the config file); I'm frankly not sure what's the point of client mode, but it doesn't work for me.
root@OpenWrt:~# cat /etc/config/iperf3
config server lan
option enabled '1'
option iface 'lan'
#option interval '1'
#option port '5201'
option redirect '1'
config client door
option server 'door.lan'
option port '5201'
option duration '10'
option proto 'any'
option reverse '0'
option udp '1'
option redirect '1'
root@OpenWrt:~# service iperf3 client door
root@OpenWrt:~# logread -l 1
Wed Nov 29 08:32:56 2023 daemon.err iperf3-ubus-redirect[8121]: iperf3: the client has terminated
root@OpenWrt:~# iperf3 -u -t1 -c door.lan
Connecting to host door.lan, port 5201
[ 5] local 192.168.1.2 port 55522 connected to 192.168.1.25 port 5201
[ ID] Interval Transfer Bitrate Total Datagrams
[ 5] 0.00-1.00 sec 129 KBytes 1.05 Mbits/sec 91
- - - - - - - - - - - - - - - - - - - - - - - - -
[ ID] Interval Transfer Bitrate Jitter Lost/Total Datagrams
[ 5] 0.00-1.00 sec 129 KBytes 1.05 Mbits/sec 0.000 ms 0/91 (0%) sender
[ 5] 0.00-1.00 sec 129 KBytes 1.05 Mbits/sec 0.125 ms 0/91 (0%) receiver
iperf Done.
So client mode works fine too when using redirect '0'. The results of the most recent run are in /var/run/iperf3/logfile-${cfg}.log.
It still seems to me that client mode would be better as a LuCI application rather than a service.
- For server mode. I think the examples in the default config file should be changed. Options that are the default (e.g. port) should be pre-commented out.
- For client mode (if it stays) I guess it makes sense to support all three directions: outbound (no flags), inbound (
--reverse) and both (--bidir) as well as parallel streams.
@feckert
I came to a conclusion that the init.d script should be moved to a separate package (cf. rsync and rsyncd), which is probably what you meant above.
Would it be OK with you if I submitted this modification as a new PR (without client mode). Should I write you as Authored-by, Co-authored-by, or Signed-off-by?
Also, what problem does the ubus redirect script solve? I'm not sure what announcing the results on ubus is good for.
Thanks!