bfdd: update and add protocol handler - #30364
Conversation
This includes an upstream fix for gcc-10+ compatibility, so we can drop our gcc-10 patch. It also includes a fix for a crash when dynamically adding BFD sessions with a label that was in use before. Signed-off-by: Wolf Pl <[email protected]>
Update to upstream version containing improvements to bfdctl, including bugfixes and new CLI flags that make it possible to dynamically add peer definitions to bfdd, without going through the static config. These features will be needed for a bfd protocol handler for netifd. Signed-off-by: Wolf Pl <[email protected]>
Add a netifd protocol handler for bfdd. It's intended to be used as a second protocol on a device that already has a protocol that can't detect its up-ness on its own (eg. wireguard). When bfdd detects the link to be up or down, it the protocol handler will propagate that status to the bfd interface in netifd, which will cause its routes (and possibly other settings) to be applied or retracted in response. The interface's device is used both as a device to which the settings are applied, and a device to which the bfd session is bound. This might not be optimal for multihop BFD, but I wanted to keep things simple for now. Maybe adding a way to separately specify each of these devices will need to be added in the future. Signed-off-by: Wolf Pl <[email protected]>
Formality Check: Suggestions AvailableWe checked this pull request against the contribution guidelines. Here is what needs your attention:
|
It was reset to one in the commit that updated the version. I can put both of those changes in one commit and avoid bumping PKG_RELEASE - please let me know if that's preferred. |
|
can't test this as I mainly use the frr-bfd now |
would it make sense to add myself as another maintainer once I start using this package as part of my long-term setup? |
|
yes, please do it! |
openwrt-ai
left a comment
openwrt-ai
left a comment
There was a problem hiding this comment.
Reviewed 3 commits. Nothing here is a hard blocker beyond what the PR already acknowledges (it is a draft pending the upstream bfdd PR); the missing peer_address validation and the teardown ordering are the two I would want resolved before merge.
Commit checks
6410a8d"bfdd: add netifd protocol handler" — the diff matches the message, but the body has a stray word and a double space: "When bfdd detects the link to be up or down, it the protocol handler will propagate that status to the bfd interface". Worth fixing while the branch is still being rebased.
On the PKG_RELEASE warning from the formality bot: your reasoning in the thread is right — 87d02ad resets it to 1 with the version bump and 6410a8d increments it for the added files. That is the documented behaviour for this repo, so no change needed on that account.
Generated by Claude Code
| PKG_RELEASE:=2 | ||
|
|
||
| PKG_SOURCE_VERSION:=c54534beb524afc3972039f57b56ec65332b43f7 | ||
| # TODO: update to a commit from master once https://github.com/rzalamena/bfdd/bfdd/pulls/6 is merged |
There was a problem hiding this comment.
The URL in this TODO is malformed and 404s — it has the repo name twice and pulls instead of pull. The PR description links the correct one (https://github.com/rzalamena/bfdd/pull/6).
| # TODO: update to a commit from master once https://github.com/rzalamena/bfdd/bfdd/pulls/6 is merged | |
| # TODO: update to a commit from master once https://github.com/rzalamena/bfdd/pull/6 is merged |
Generated by Claude Code
| json_get_vars detect_multiplier receive_interval transmit_interval echo_mode echo_interval | ||
|
|
There was a problem hiding this comment.
peer_address is never validated, but bfdd.template.json documents peer-address as mandatory. If the user forgets option peer_address, line 67 runs bfdctl -a -i <dev> -p "" ..., bfdctl exits non-zero, and netifd will keep re-running setup in a restart loop instead of telling the user what is wrong. Other handlers in the feed guard this with proto_notify_error + proto_block_restart, e.g. tayga-proto.sh:20.
| json_get_vars detect_multiplier receive_interval transmit_interval echo_mode echo_interval | |
| json_get_vars detect_multiplier receive_interval transmit_interval echo_mode echo_interval | |
| if [ -z "$peer_address" ]; then | |
| proto_notify_error "$config" "MISSING_PEER_ADDRESS" | |
| proto_block_restart "$config" | |
| return 1 | |
| fi | |
The template also marks local-address as mandatory for multihop, so a matching check under [ "$multihop" = "1" ] would be worth adding too.
Generated by Claude Code
| if ! pidof bfdd; then | ||
| service bfdd start | ||
| fi |
There was a problem hiding this comment.
Two problems here:
pidof bfddprints the matched PIDs on stdout, and proto handlers run under netifd, so every singleifupof a bfd interface writes a stray PID line into the log. Redirect it.service bfdd starthands off to procd and returns immediately — it does not wait for bfdd to create its control socket. Thebfdctlinvocation at line 66 runs right after, so on the very first setup (cold boot, or bfdd not yet started) it can fail to connect and the whole setup fails. Either poll for the control socket before continuing, or make the interface depend on bfdd being started at boot and onlyproto_notify_error/proto_block_restartwhen it is not running.
| if ! pidof bfdd; then | |
| service bfdd start | |
| fi | |
| if ! pidof bfdd >/dev/null; then | |
| service bfdd start | |
| fi |
Separately, worth deciding deliberately: this silently starts a service the user may have stopped or disabled on purpose.
Generated by Claude Code
There was a problem hiding this comment.
it can fail to connect and the whole setup fails
Yes, and netifd will keep restarting the protocol handler until it succeeds.
I don't see what polling for startup accomplishes that the current implementation doesn't.
Separately, worth deciding deliberately: this silently starts a service the user may have stopped or disabled on purpose.
Just like the protocol handler for xl2tpd
| [ -z "$transmit_interval" ] || json_add_string "transmit-interval" "$transmit_interval" | ||
| [ -z "$echo_interval" ] || json_add_string "echo-interval" "$echo_interval" | ||
|
|
||
| json_add_boolean "echo-mode" "$echo_mode" |
There was a problem hiding this comment.
This one option escapes the guard pattern the comment on lines 53-56 sets up. When the user does not set option echo_mode, json_get_vars leaves $echo_mode empty and jshn serialises an empty boolean as false, so the key is present and the daemon default is overridden rather than used — exactly the case the comment says you want to avoid. It happens to be harmless today because echo-mode defaults to false anyway, but it will silently diverge if that default ever changes.
| json_add_boolean "echo-mode" "$echo_mode" | |
| [ -z "$echo_mode" ] || json_add_boolean "echo-mode" "$echo_mode" |
Generated by Claude Code
There was a problem hiding this comment.
echo_mode is not an interval, that comment doesn't apply to it
bfdd's struct bpc_config has no bpc_has_echomode that could be used to signify bpf_echomode's absence
| # first kill the bfdctl -M that listens for events, | ||
| # since it holds refcount on the bfd session, preventing it from being deleted | ||
| proto_kill_command "$config" | ||
| # then remove the session | ||
| bfdctl -d -L "$config" |
There was a problem hiding this comment.
The comment claims an ordering that proto_kill_command does not actually give you. proto_kill_command is a ubus call network.interface kill_command — it asks netifd to deliver a signal and returns as soon as ubus acknowledges it; it does not wait for bfdctl -M to reap. So bfdctl -d -L "$config" on line 79 can easily run while the monitor still holds the refcount, which is precisely the failure the comment says it is avoiding, and the session is then left behind in bfdd — a subsequent ifup would hit a stale label.
Did you observe this working reliably, or is it timing-dependent on your setup? If it is not guaranteed, the teardown probably needs to wait for the monitor to exit (poll on the pid) or retry the delete.
Also worth noting: if bfdd is not running at teardown time, bfdctl -d fails and its error goes to the log with no handling.
Generated by Claude Code
| up=1 | ||
| fi | ||
|
|
||
| echo "bfd-event: config=$config up=$up ifname=$ifname" |
There was a problem hiding this comment.
nit: this reads like debug scaffolding — it fires on every BFD state change and lands in the log as an unattributed bare line. If the logging is wanted, route it through logger so it is tagged and can be filtered; otherwise drop it.
| echo "bfd-event: config=$config up=$up ifname=$ifname" | |
| logger -t bfd-event "config=$config up=$up ifname=$ifname" |
Generated by Claude Code
|
|
||
| up=0 | ||
| if [ "$state" = "up" ]; then | ||
| up=1 |
There was a problem hiding this comment.
nit: this file indents with 2 spaces while the sibling file added in the same commit (bfd.sh) and the package's existing files/bfdd.init both use 4. Worth making the two new files agree.
| up=1 | |
| up=1 |
(For what it's worth, every other netifd proto handler in the feed — net/tayga/files/tayga-proto.sh, net/openvpn/files/lib/netifd/proto/openvpn.sh, net/bonding/files/lib/netifd/proto/bonding.sh — indents with tabs, so tabs would be the more conventional choice for bfd.sh too. Not a blocker either way, just pick one.)
Generated by Claude Code
3 participants
📦 Package Details
Maintainer: @lucize
Description:
Add netifd protocol handler for bfdd, after updating to a version of bfdd that has the changes necessary to make that possible.
That version is from my pull request to the upstream repo: rzalamena/bfdd#6
so I suppose merging that PR is blocking for this one.
The idea behind this protocol handler is that if you have an interface of a protocol that, on its own, can't detect if it's up or down (eg. wireguard), you can add a second interface of bfd protocol on the same device, and it will only go up once bfdd detects that the other end is reachable. Then you can configure routes on this bfd interface, and netifd will only enable those once the interface goes up.
🧪 Run Testing Details
(Once I get a separate router for testing, I'll try testing on 25.12 and master as well)
✅ Formalities
If your PR contains a patch:
It doesn't