oxidecomputer / oxidecomputer/omicron

Configuration of BFD needs more strict validation

Open
#10,657 0 comments 0 reactions 0 assignees View on GitHub

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:

  1. detection_threshold must be nonzero (should switch to NonZeroU8)
  2. required_rx is a timeout value in microseconds that eventually gets sent on wire as a u32. Any value greater than u32::MAX isn'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.)
  3. BFD peers are uniquely identified by their remote IP address. Vec<BfdPeerConfig> allows multiple entries with the same remote address.
  4. Maghemite's local API is non-optional; omicron converts None into Ipv4Addr::UNSPECIFIED, which is itself incorrect: if remote is an IPv6 address, this should be Ipv6Addr::UNSPECIFIED. But that means None and Some(0.0.0.0) are identical - do we need None at all? (Relevant to the next point: is binding on "all addresses" even a thing we want to expose?)
  5. local controls 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

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.