luci-proto-bfd: add BFD protocol handler - #8969
Conversation
Allow users to configure bfdd peers through luci. Signed-off-by: Wolf480pl <wolf480@interia.pl>
Formality Check: FailedWe checked this pull request against the contribution guidelines. Here is what needs your attention: 🛑 CRITICAL ERRORS
Tip Do not close this pull request to make corrections. Instead, modify your existing commits (e.g. Something broken? Consider reporting an issue. |
Unlike in openwrt/packages, here the pull request template explicitly says
Since S.O.B. must match Author name, I thought this means commit author also may be a nickname. Is that not the case? This is a nickname I've been using on the internet for 10+ years, and contributed to other projects using it. Not some random account created yesterday. |
openwrt-ai
left a comment
There was a problem hiding this comment.
Reviewed 1 new commit; 7 inline comments. Backend coupling checks out: every UCI option here (peer_address, local_address, multihop, vrf_name, detect_multiplier, receive_interval, transmit_interval, echo_mode, echo_interval) is read by proto_bfd_init_config/proto_bfd_setup in net/bfdd/files/bfd.sh from openwrt/packages#30364, and that dependency is called out in the PR description as required.
The main functional issue is the link-local zone suffix added in local_address's load(), which the option's own datatype rejects.
Generated by Claude Code
| }, | ||
|
|
||
| getPackageName: function() { | ||
| return 'proto-bfd'; |
There was a problem hiding this comment.
There is no proto-bfd package. getPackageName() is documented as "the name of the package to download, required for the protocol to function" (network.js:2464-2477),`` and in openwrt/packages#30364 the netifd handler (/lib/netifd/proto/bfd.sh) is installed by the `bfdd` package itself — which is also what `LUCI_DEPENDS:=+bfdd` in the Makefile refers to.
| return 'proto-bfd'; | |
| return 'bfdd'; |
Generated by Claude Code
| function addLinkName(addr) { | ||
| if (addr.startsWith("fe8")) { | ||
| addr = addr + "%" + linkName; | ||
| } | ||
| return addr; | ||
| } |
There was a problem hiding this comment.
The zone-suffixed values this produces cannot pass the option's own datatype. ip6addr("nomask") matches against /^([0-9a-fA-F:.]+)$/ (validation.js:533-538),`` so % is rejected — picking any link-local entry from this dropdown fails validation with "valid IP address" and the form cannot be saved.
Either drop the %<link> suffix here, or replace the datatype on line 29 with a custom validate() that strips the zone before delegating to the address check.
Separately, the prefix test is too narrow: link-local is fe80::/10 (fe8, fe9, fea, feb), so startsWith("fe8") only covers fe80::/16.
Generated by Claude Code
|
|
||
| o = s.taboption('general', form.Flag, 'multihop', _("Multihop"), _("Allow BFD packats to traverse multiple hops. Use when BFD peer is not a direct neighbour of this router.")); | ||
|
|
||
| o = s.taboption('general', form.Value, 'local_address', _("Local IPv4 address"), _("The local IP address used to communicate with the BFD peer (mandatory on multihop).")); |
There was a problem hiding this comment.
The label says IPv4 but the datatype on line 29 accepts IPv6 as well, and the load() below populates the dropdown with getIP6Addrs() too. Looks like a leftover from luci-proto-gre, which is IPv4-only.
| o = s.taboption('general', form.Value, 'local_address', _("Local IPv4 address"), _("The local IP address used to communicate with the BFD peer (mandatory on multihop).")); | |
| o = s.taboption('general', form.Value, 'local_address', _("Local IP address"), _("The local IP address used to communicate with the BFD peer (mandatory on multihop).")); |
Generated by Claude Code
| o = s.taboption('general', form.Value, 'local_address', _("Local IPv4 address"), _("The local IP address used to communicate with the BFD peer (mandatory on multihop).")); | ||
| o.rmempty = false; | ||
| o.datatype = 'or(ip4addr("nomask"),ip6addr("nomask"))'; | ||
| o.depends('multihop', '1'); |
There was a problem hiding this comment.
Gating on multihop makes local_address unreachable for single-hop sessions, but the backend does not treat it that way: bfd.sh in openwrt/packages#30364 passes ${local_address:+-l "$local_address"} unconditionally, and bfdd's own bfdd.template.json documents local-address as "mandatory on multihop" — i.e. permitted, just not required, otherwise. The option's own help text says the same thing. Is hiding it for single-hop intentional? As written a user with several addresses on the bound device has no way to pin the source address for a single-hop session.
Same question for vrf_name on line 56 — bfd.sh emits vrf-name regardless of multihop.
Generated by Claude Code
There was a problem hiding this comment.
bfdd itself ignores vrf_name and local_address when multihop is is disabled https://github.com/rzalamena/bfdd/blob/921bfa05b0e0020eb910cffc2a6edf37543b84cf/bfd.c#L775-L792
It's an inconsistence between bfdd's documentation and implementation, though I guess I should pick one interpretation and stick to it instead of carrying the inconsistency into openwrt.
| o.rmempty = false; | ||
| o.datatype = 'or(ip4addr("nomask"),ip6addr("nomask"))'; | ||
|
|
||
| o = s.taboption('general', form.Flag, 'multihop', _("Multihop"), _("Allow BFD packats to traverse multiple hops. Use when BFD peer is not a direct neighbour of this router.")); |
There was a problem hiding this comment.
nit: typo "packats" — this string goes into the translation catalogs, so it is worth fixing before merge.
| o = s.taboption('general', form.Flag, 'multihop', _("Multihop"), _("Allow BFD packats to traverse multiple hops. Use when BFD peer is not a direct neighbour of this router.")); | |
| o = s.taboption('general', form.Flag, 'multihop', _("Multihop"), _("Allow BFD packets to traverse multiple hops. Use when BFD peer is not a direct neighbour of this router.")); |
Line 70 has a related one: "Minimum number of milliseconds between BFD packet we transmit" should be "packets".
Generated by Claude Code
|
|
||
| renderFormOptions: function(s) { | ||
| var o; | ||
| var proto = this; |
There was a problem hiding this comment.
nit: proto is never used in this function.
| var proto = this; |
Same for the tools.widgets as widgets import on line 4 — no widget from it is referenced (unlike luci-proto-gre, which uses widgets.NetworkSelect).
Generated by Claude Code
| return addrs4.concat(addrs6) | ||
| }).sort() |
There was a problem hiding this comment.
nit: missing statement terminators; every other statement in this file is semicolon-terminated.
| return addrs4.concat(addrs6) | |
| }).sort() | |
| return addrs4.concat(addrs6); | |
| }).sort(); |
Generated by Claude Code
Pull request details
Description
Add luci-proto-bfd - a BFD protocol handler, so that users can configure bfdd peers through luci.
This depends on openwrt/packages#30364
Screenshot or video of changes
Maintainer
Not sure... I'm adding myself (@Wolf480pl) as the maintainer of the luci-protocol-bfd package, but @lucize maintains the bfdd package on openwrt/packages side
Tested on
OpenWrt version: OpenWrt 24.10.2 r28739-d9340319c6
LuCI version: openwrt-24.10 branch (25.340.26705~d88390b)
Web browser(s): Firefox 145.0.2 (64-bit)
(Once I get a separate router for testing, I'll try testing on 25.12 and master as well)
Checklist
(Nice to have) Includes what Issue it closes (e.g. openwrt/luci#issue-number).