oxidecomputer / oxidecomputer/omicron
Configuration of BFD needs more strict validation
Nobody has claimed this yet.
- Dominant language
- Rust
- Stars
- 572
- Forks
- 97
- Avg merge
- 2d 12h
- Merged PRs (30d)
- 96
Description
This came up while writing property-based tests for BFD reconciliation as a part of the #10167 work. I think we currently assume operators are doing something reasonable with these settings, but proptests tend to do unreasonable things and expose gaps in validation; I wanted to jot these down here as a "should be fixed at some point".
Currently, RackNetworkConfig contains the complete BFD configuration as a Vec of peers: https://github.com/oxidecomputer/omicron/blob/b260af8d29767fce5d9f990d06271072a435eb7a/sled-agent/types/versions/src/stronger_bgp_unnumbered_types/early_networking.rs#L581-L583
each of which is defined as: https://github.com/oxidecomputer/omicron/blob/b260af8d29767fce5d9f990d06271072a435eb7a/sled-agent/types/versions/src/initial/early_networking.rs#L153-L163
Things that are allowed by these structures that shouldn't be:
detection_thresholdmust be nonzero (should switch toNonZeroU8)required_rxis a timeout value in microseconds that eventually gets sent on wire as au32. Any value greater thanu32::MAXisn't representable, so maghemite has to either cast it to a u32 (incorrectly wrapping around, as it does today on main) or saturate it. (I'm not sure whether 0 is a reasonable value here.)- BFD peers are uniquely identified by their remote IP address.
Vec<BfdPeerConfig>allows multiple entries with the same remote address. - Maghemite's
localAPI is non-optional; omicron convertsNoneintoIpv4Addr::UNSPECIFIED, which is itself incorrect: ifremoteis an IPv6 address, this should beIpv6Addr::UNSPECIFIED. But that meansNoneandSome(0.0.0.0)are identical - do we needNoneat all? (Relevant to the next point: is binding on "all addresses" even a thing we want to expose?) localcontrols the local address on which mgd will bind a socket; allowing any arbitrary IP here is not correct. Completely nonsense IPs (e.g., IPs that aren't associated with any uplink port) will fail to bind, but there are other IPs that will succeed to bind that we presumably don't want to allow (e.g., internal addresses like the underlay or bootstrap addrs).
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start with RackNetworkConfig and BfdPeerConfig in sled-agent/types/versions/src/stronger_bgp_unnumbered_types/early_networking.rs and initial/early_networking.rs, then read the conversion in nexus/src/app/background/tasks/bfd.rs. Review the property-based BFD reconciliation work from #10167 and decide the intended semantics for each listed constraint, especially local address binding and duplicate peers. Done means the configuration rejects or safely handles every invalid case identified here, with coverage for the chosen behavior.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- rust
- Domain
- networking
- Issue type
- Bug
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 42/100