eric/10-wancarp
Watch
1
0
Fork
You've already forked 10-wancarp
 0
Code Issues 1 Pull requests Projects Releases 3 Packages Wiki Activity Actions

Role rule: all-INIT VHIDs leave WAN up on a node that lost CARP (dual WAN) #15

New issue
Closed
opened 2026-10-22 17:36:11 -07:00 by eric · 1 comment
eric commented 2026-10-22 17:36:11 -07:00
Owner

Summary

node_role() treats "every counted VHID is INIT" as undecided and takes no action. When the node loses CARP without passing through BACKUP, its WAN stays UP while the peer promotes itself and brings its own WAN up. That is the dual-WAN state this script exists to prevent.

Where

  • 10-wancarp, node_role() (around line 474), specifically: 
    if (in_array('BACKUP', $states_only, true)) {
    $role = 'BACKUP';
} elseif ($states_only !== [] && array_unique($states_only) === ['MASTER']) {
    $role = 'MASTER';
} else {
    $role = '';
}
  • Rationale text that needs updating along with the code:
    • header comment ROLE RULE (around lines 84–93: "INIT means CARP isn't running on that VIP ... the peer settles it")
    • README How it works table + paragraph ("INIT means CARP isn't running on that VIP ...; the peer settles that")
    • README Boot behaviour note ("while its VHIDs are still INIT the role is undecided, so nothing happens yet")

Failure scenario

FreeBSD sets a VHID to INIT when its parent interface loses link or is admin-down (carp_sc_state() in sys/netinet/ip_carp.c, reason "hardware interface down"). It very likely does the same when net.inet.carp.allow=0, which is what OPNsense's Interfaces → Virtual IPs → Status → Temporarily Disable CARP sets. Confirm that second case on a box.

Setup: HA pair, one LAN CARP VIP (VHID 1), WAN is DHCP with no VIP, $carp_vhids = [] (default). This is the common homelab layout.

  1. The MASTER's LAN cable, switch port or virtio link goes down, or someone clicks "Temporarily Disable CARP" on it.
  2. MASTER: VHID 1 → INIT. Event 1@vtnet1 INIT fires, the script computes role=undecided, and WAN stays UP.
  3. Peer: VHID 1 → MASTER, so the script brings the peer's WAN UP.
  4. Both nodes now have WAN up: duplicate MAC/IP towards the ISP, two DHCP clients, and so on. --reconcile doesn't fix it either, because it reaches the same undecided result every minute.

With multiple VHIDs and preempt on (OPNsense default), the demotion counter pushes the remaining VHIDs to BACKUP, so "BACKUP wins" already covers that case. The hole is when no counted VHID is MASTER and at least one is INIT, most commonly a single counted VHID.

Testing missed it because maintenance-mode failover demotes the node to BACKUP, not INIT.

Suggested fix

Change the rule so that, among counted VHIDs:

Counted VHIDs Role
any BACKUP BACKUP (unchanged)
all MASTER MASTER (unchanged)
none MASTER, at least one INIT (e.g. all INIT) BACKUP (new)
MASTER + INIT mix undecided (unchanged; the node is still MASTER for something)
no counted VHIDs undecided (unchanged)

Trade-offs to note in the docs:

  • If CARP is disabled/INIT on both nodes at once, both WANs go down. That is acceptable: it fails safe, it's an operator action, and with a dead LAN the WAN is useless anyway.
  • At boot, VHIDs are briefly INIT, so a CARP INIT event during boot would now take WAN down until the MASTER/BACKUP event follows. Check this doesn't fight rc.bootup's own interface configuration. The live event path (unlike --reconcile) has no exit_on_bootup() guard; decide whether the INIT→BACKUP mapping should skip during boot, e.g. by checking product::getInstance()->booting().
  • Dry-run with a positional INIT currently maps to $action = '' (around line 709). Decide whether a what-if INIT should now show the BACKUP branch.

Acceptance

  • Off-box test with a stubbed ifconfig -a showing only carp: INIT vhid 1 ...: role=BACKUP.
  • carp: MASTER vhid 1 + carp: INIT vhid 2: still undecided.
  • Header ROLE RULE, README table and Boot behaviour note updated; CHANGELOG [Unreleased] entry under Fixed.
  • On a live pair: pull the MASTER's LAN link (or Temporarily Disable CARP) and confirm WAN goes down on it while the peer takes over.
## Summary `node_role()` treats "every counted VHID is INIT" as **undecided** and takes no action. When the node loses CARP without passing through BACKUP, its WAN stays UP while the peer promotes itself and brings its own WAN up. That is the dual-WAN state this script exists to prevent. ## Where - `10-wancarp`, `node_role()` (around line 474), specifically: ```php if (in_array('BACKUP', $states_only, true)) { $role = 'BACKUP'; } elseif ($states_only !== [] && array_unique($states_only) === ['MASTER']) { $role = 'MASTER'; } else { $role = ''; } ``` - Rationale text that needs updating along with the code: - header comment **ROLE RULE** (around lines 84–93: "INIT means CARP isn't running on that VIP ... the peer settles it") - README **How it works** table + paragraph ("`INIT` means CARP isn't running on that VIP ...; the peer settles that") - README **Boot behaviour** note ("while its VHIDs are still `INIT` the role is undecided, so nothing happens yet") ## Failure scenario FreeBSD sets a VHID to INIT when its parent interface loses link or is admin-down (`carp_sc_state()` in `sys/netinet/ip_carp.c`, reason "hardware interface down"). It very likely does the same when `net.inet.carp.allow=0`, which is what OPNsense's **Interfaces → Virtual IPs → Status → Temporarily Disable CARP** sets. Confirm that second case on a box. Setup: HA pair, one LAN CARP VIP (VHID 1), WAN is DHCP with no VIP, `$carp_vhids = []` (default). This is the common homelab layout. 1. The MASTER's LAN cable, switch port or virtio link goes down, or someone clicks "Temporarily Disable CARP" on it. 2. MASTER: VHID 1 → INIT. Event `1@vtnet1 INIT` fires, the script computes `role=undecided`, and **WAN stays UP**. 3. Peer: VHID 1 → MASTER, so the script brings the peer's WAN UP. 4. Both nodes now have WAN up: duplicate MAC/IP towards the ISP, two DHCP clients, and so on. `--reconcile` doesn't fix it either, because it reaches the same undecided result every minute. With multiple VHIDs and preempt on (OPNsense default), the demotion counter pushes the remaining VHIDs to BACKUP, so "BACKUP wins" already covers that case. The hole is when **no counted VHID is MASTER and at least one is INIT**, most commonly a single counted VHID. Testing missed it because maintenance-mode failover demotes the node to BACKUP, not INIT. ## Suggested fix Change the rule so that, among counted VHIDs: | Counted VHIDs | Role | |---|---| | any BACKUP | BACKUP (unchanged) | | all MASTER | MASTER (unchanged) | | none MASTER, at least one INIT (e.g. all INIT) | **BACKUP** (new) | | MASTER + INIT mix | undecided (unchanged; the node is still MASTER for something) | | no counted VHIDs | undecided (unchanged) | Trade-offs to note in the docs: - If CARP is disabled/INIT on **both** nodes at once, both WANs go down. That is acceptable: it fails safe, it's an operator action, and with a dead LAN the WAN is useless anyway. - At boot, VHIDs are briefly INIT, so a CARP INIT event during boot would now take WAN down until the MASTER/BACKUP event follows. Check this doesn't fight `rc.bootup`'s own interface configuration. The live event path (unlike `--reconcile`) has no `exit_on_bootup()` guard; decide whether the INIT→BACKUP mapping should skip during boot, e.g. by checking `product::getInstance()->booting()`. - Dry-run with a positional `INIT` currently maps to `$action = ''` (around line 709). Decide whether a what-if `INIT` should now show the BACKUP branch. ## Acceptance - Off-box test with a stubbed `ifconfig -a` showing only `carp: INIT vhid 1 ...`: `role=BACKUP`. - `carp: MASTER vhid 1` + `carp: INIT vhid 2`: still undecided. - Header ROLE RULE, README table and Boot behaviour note updated; CHANGELOG `[Unreleased]` entry under Fixed. - On a live pair: pull the MASTER's LAN link (or Temporarily Disable CARP) and confirm WAN goes down on it while the peer takes over.
eric closed this issue 2026-10-23 15:34:42 -07:00
eric commented 2026-10-23 16:07:31 -07:00
Author
Owner

Follow-up audit: the boot check never fired

Found in an audit of the fixes against opnsense/core (3684ed7), fixed in 222180a.

Finding. node_booting() tested method_exists('product', 'booting'). In core, product (config.inc) has no booting() method: it is served by __call(), which returns the stored value or null. method_exists() doesn't see magic methods, so on a real firewall the check was always false and the script treated the box as never booting. An INIT event during boot (every VHID starts INIT) therefore became a live BACKUP run that called interface_reset('wan') while rc.bootup was bringing WAN up. The following MASTER event would have re-upped it, but that is exactly the clash this fix said it avoided. The harness missed it because its product stub declared booting() as a real method.

Fix.

  • node_booting() now calls product::getInstance()->booting() directly, cast to bool, the same call core's exit_on_bootup() makes.
  • The harness's product stub now uses __call() like core. With that stub, the old script fails all-INIT during boot is undecided and changes nothing; the fixed one passes.
  • The dry-run preflight now reports boot check product::booting() present; booting=no. (or an error if the class is missing), so the live pre-check in #21 no longer needs a separate php -r step. The old pre-check there used method_exists() too and would have printed bool(false) on a healthy box; it has been replaced.

The live boot checks in #21 (section C) are the ones that confirm this.

## Follow-up audit: the boot check never fired Found in an audit of the fixes against opnsense/core (`3684ed7`), fixed in 222180a. **Finding.** `node_booting()` tested `method_exists('product', 'booting')`. In core, `product` (`config.inc`) has no `booting()` method: it is served by `__call()`, which returns the stored value or `null`. `method_exists()` doesn't see magic methods, so on a real firewall the check was always false and the script treated the box as never booting. An INIT event during boot (every VHID starts INIT) therefore became a live BACKUP run that called `interface_reset('wan')` while `rc.bootup` was bringing WAN up. The following MASTER event would have re-upped it, but that is exactly the clash this fix said it avoided. The harness missed it because its `product` stub declared `booting()` as a real method. **Fix.** - `node_booting()` now calls `product::getInstance()->booting()` directly, cast to bool, the same call core's `exit_on_bootup()` makes. - The harness's `product` stub now uses `__call()` like core. With that stub, the old script fails `all-INIT during boot is undecided and changes nothing`; the fixed one passes. - The dry-run preflight now reports `boot check product::booting() present; booting=no.` (or an error if the class is missing), so the live pre-check in #21 no longer needs a separate `php -r` step. The old pre-check there used `method_exists()` too and would have printed `bool(false)` on a healthy box; it has been replaced. The live boot checks in #21 (section C) are the ones that confirm this.
Sign in to join this conversation.
No Branch/Tag specified
Branches Tags
main
1.1.1
1.1.0
1.0.0
Labels
Clear labels   
bug
Something is not working

  
duplicate
This issue or pull request already exists

  
enhancement
New feature

  
help wanted
Need some help

  
invalid
Something is wrong

  
question
More information is needed

  
wontfix
This won't be fixed

No labels
bug
duplicate
enhancement
help wanted
invalid
question
wontfix
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
eric/10-wancarp#15
No description provided.