mirror of
https://github.com/jmcorgan/fips.git
synced 2026-10-06 03:28:24 +00:00
fix(config): validate node.netmon.*
Two checks, both about the detector being able to do its job at all rather than taste in numbers. A zero `poll_interval_secs` was silently clamped to one second, so a typo produced a node polling twenty times more often than configured and saying nothing about it. It is now refused, and only while detection is enabled, since a disabled detector imposes no constraint on its own knobs. A `debounce_ms` whose worst case — the full `MAX_DEBOUNCE_ROUNDS` of settling — meets or exceeds `link_dead_timeout_secs` is refused too, because the liveness reaper would tear the peering down before the change was ever reported: the machinery would run and could not help. The constant is shared with the detector rather than restated, so the two cannot drift. Both multiplications saturate. The operands are operator-supplied `u64`s, and an overflow would wrap to a small number and silently accept the very configuration this refuses. Documented in the reference, because neither refusal was predictable from it. The debounce one couples two keys in different blocks, so an operator shortening `link_dead_timeout_secs` for fast failover can make an untouched `debounce_ms` illegal and meet a hard startup refusal citing a netmon key they never set. The multiplier its own error message uses is now on the page. A test asserts the shipped defaults still validate, which is the failure mode a cross-field check invites. Another asserts the refusal message carries no run of literal spaces: it is the whole diagnostic for the only refusal reachable by editing `debounce_ms`, substring assertions cannot see how it reads, and rustfmt does not touch string literals — a continuation join had already left twenty-two of them mid-sentence.
This commit is contained in:
committed by
Johnathan Corgan
parent
957e05b283
commit
47f8f01e2f
@@ -202,8 +202,22 @@ an interface arriving or leaving — and rebinds the send path immediately.
|
||||
| Parameter | Type | Default | Description |
|
||||
|-----------|------|---------|-------------|
|
||||
| `node.netmon.enabled` | bool | `true` | Whether medium-change detection runs |
|
||||
| `node.netmon.poll_interval_secs` | u64 | `5` | How often the path to each peer is sampled (backstop period where an event-driven backend exists) |
|
||||
| `node.netmon.debounce_ms` | u64 | `250` | How long to wait for the picture to settle before acting (`0` disables) |
|
||||
| `node.netmon.poll_interval_secs` | u64 | `5` | How often the path to each peer is sampled (backstop period where an event-driven backend exists). **Must be at least 1 while `enabled`; `0` is refused at startup.** |
|
||||
| `node.netmon.debounce_ms` | u64 | `250` | How long to wait for the picture to settle before acting (`0` disables). **Refused at startup when `debounce_ms × 8` reaches `node.link_dead_timeout_secs`** — see below. |
|
||||
|
||||
Both refusals stop the node rather than degrade it, so they are worth knowing
|
||||
before they are met.
|
||||
|
||||
A handover is ridden out for up to **8** settling rounds (`MAX_DEBOUNCE_ROUNDS`)
|
||||
of `debounce_ms` each before a change is reported. If that worst case reaches
|
||||
`node.link_dead_timeout_secs`, the liveness reaper tears the peering down before
|
||||
the detector ever reports, so the machinery runs and cannot help — the node
|
||||
refuses to start rather than run in that shape. At the shipped defaults the
|
||||
margin is wide (8 × 250 ms = 2 s against 30 s), but the constraint couples two
|
||||
keys in different blocks: **shortening `link_dead_timeout_secs` for fast
|
||||
failover can make an untouched `debounce_ms` illegal.** The refusal names both
|
||||
values and the multiplier.
|
||||
|
||||
|
||||
Established UDP peers use a per-peer `connect()`-ed socket for the send fast
|
||||
path. `connect(2)` makes the kernel resolve the route once and pin the local
|
||||
|
||||
@@ -1100,6 +1100,44 @@ impl Config {
|
||||
}
|
||||
}
|
||||
|
||||
// Medium-change detection. Both checks are about the detector being
|
||||
// able to do its job at all, not about taste in numbers.
|
||||
let netmon = &self.node.netmon;
|
||||
if netmon.enabled {
|
||||
if netmon.poll_interval_secs == 0 {
|
||||
return Err(ConfigError::Validation(
|
||||
"`node.netmon.poll_interval_secs` must be at least 1; it is the backstop \
|
||||
period behind the kernel event source, and the only detection signal at \
|
||||
all on a platform without one"
|
||||
.to_string(),
|
||||
));
|
||||
}
|
||||
// A handover is ridden out for up to `MAX_DEBOUNCE_ROUNDS` rounds
|
||||
// of `debounce_ms` before the change is reported. If that can
|
||||
// outlast the liveness timeout, the reaper tears the peering down
|
||||
// first and the detector never gets to rebind anything — the
|
||||
// machinery runs and cannot help.
|
||||
// Saturating: both operands are operator-supplied `u64`s, and an
|
||||
// overflow here would wrap to a small number and silently accept
|
||||
// the very configuration this refuses.
|
||||
let worst_case_debounce_ms = netmon
|
||||
.debounce_ms
|
||||
.saturating_mul(u64::from(crate::node::netmon::MAX_DEBOUNCE_ROUNDS));
|
||||
let dead_timeout_ms = self.node.link_dead_timeout_secs.saturating_mul(1000);
|
||||
if dead_timeout_ms > 0 && worst_case_debounce_ms >= dead_timeout_ms {
|
||||
return Err(ConfigError::Validation(format!(
|
||||
"`node.netmon.debounce_ms` = {} can hold a report for up to {}ms across \
|
||||
{} settling rounds, which meets or exceeds \
|
||||
`node.link_dead_timeout_secs` = {}s: the peering would be reaped \
|
||||
before the medium change was ever acted on",
|
||||
netmon.debounce_ms,
|
||||
worst_case_debounce_ms,
|
||||
crate::node::netmon::MAX_DEBOUNCE_ROUNDS,
|
||||
self.node.link_dead_timeout_secs,
|
||||
)));
|
||||
}
|
||||
}
|
||||
|
||||
let native = &self.node.native_api;
|
||||
// Both floors refuse a node that would start, answer every setup call
|
||||
// and then drop every datagram a peer sent. A zero `backlog` makes the
|
||||
@@ -2332,6 +2370,65 @@ node:
|
||||
assert!(config.node.discovery.is_none());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_a_zero_netmon_poll_interval_is_refused() {
|
||||
// It was silently clamped to 1s, so a typo produced a node that polled
|
||||
// twenty times more often than asked and said nothing about it.
|
||||
let mut config = Config::default();
|
||||
config.node.netmon.poll_interval_secs = 0;
|
||||
|
||||
let err = config.validate().expect_err("validation should fail");
|
||||
assert!(err.to_string().contains("poll_interval_secs"), "{}", err);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_a_zero_netmon_poll_interval_is_allowed_when_detection_is_off() {
|
||||
// Nothing reads it, so refusing the node over it would be pedantry.
|
||||
let mut config = Config::default();
|
||||
config.node.netmon.enabled = false;
|
||||
config.node.netmon.poll_interval_secs = 0;
|
||||
|
||||
config
|
||||
.validate()
|
||||
.expect("a disabled detector imposes no constraint on its own knobs");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_a_debounce_that_outlasts_the_dead_timeout_is_refused() {
|
||||
// The detector rides out a handover for up to MAX_DEBOUNCE_ROUNDS
|
||||
// rounds before reporting. If that can exceed the liveness timeout the
|
||||
// peering is reaped first and the detector cannot help — the node runs
|
||||
// the machinery and still takes the outage it was meant to prevent.
|
||||
let mut config = Config::default();
|
||||
config.node.link_dead_timeout_secs = 30;
|
||||
// 8 rounds x 4000ms = 32s > 30s.
|
||||
config.node.netmon.debounce_ms = 4000;
|
||||
|
||||
let err = config.validate().expect_err("validation should fail");
|
||||
let msg = err.to_string();
|
||||
assert!(msg.contains("debounce_ms"), "{}", msg);
|
||||
assert!(msg.contains("link_dead_timeout_secs"), "{}", msg);
|
||||
// This message is the whole diagnostic for the only refusal an operator
|
||||
// reaches by editing `node.netmon.debounce_ms`, and substring
|
||||
// assertions cannot see how it reads. A run of spaces mid-sentence is
|
||||
// what a continuation join leaves behind, and rustfmt does not touch
|
||||
// string literals, so nothing else would catch it.
|
||||
assert!(
|
||||
!msg.contains(" "),
|
||||
"the refusal message has a run of literal spaces in it: {:?}",
|
||||
msg
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_the_default_netmon_block_validates() {
|
||||
// The shipped defaults must not be a config the node refuses to start
|
||||
// on, which is the failure mode a cross-field check invites.
|
||||
Config::default()
|
||||
.validate()
|
||||
.expect("the default configuration must validate");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_validate_transport_advert_requires_nostr_enabled() {
|
||||
let mut config = Config::default();
|
||||
|
||||
@@ -159,7 +159,7 @@ use crate::identity::NodeAddr;
|
||||
/// change again; riding it out coalesces the burst into one event. Bounded so
|
||||
/// an interface that flaps continuously still produces events rather than
|
||||
/// starving the handler forever.
|
||||
const MAX_DEBOUNCE_ROUNDS: u32 = 8;
|
||||
pub(crate) const MAX_DEBOUNCE_ROUNDS: u32 = 8;
|
||||
|
||||
/// Minimum spacing between two reported changes.
|
||||
///
|
||||
|
||||
Reference in New Issue
Block a user