Tell two networks of the same name apart

A status report with two sections both headed "network LAB" reads as one
network that is somehow working and empty at once. It was two networks:
the same name with different secrets, which is two different networks
that share nothing, because a network's identity is its name *and* its
secret.

Three fixes for the one confusion.

The heading now carries the network id, so the sections are plainly
different things. A name is a label the user chose; the id is the
identity.

Joining a name that is already configured with another secret says so, at
the moment it happens, because that is almost always a mistyped secret
and until now it silently produced an empty network sitting beside a
working one. `status` flags it too, for the case where it already
happened.

And the second network's emptiness now says why. It had no address
because the only configured range was already taken by the first — one
agent has one interface, so an address belongs to one network — and
"nobody else has joined" pointed at the wrong thing entirely. It now
names the range it cannot have, the reason, and the flag that gives it
one of its own.

Nothing was wrong with the connectivity: the working network's tunnel was
up and its ping was answering throughout.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
tsunagi
2026-09-21 20:24:11 +01:00
co-authored by Claude Opus 5
parent 990b9f2e0f
commit 4c84cc9e4b
6 changed files with 160 additions and 28 deletions
+105 -19
View File
@@ -1140,7 +1140,15 @@ async fn status(args: StatusArgs) -> Result<(), Box<dyn std::error::Error>> {
match &observed { match &observed {
Observed::Agent(report) => { Observed::Agent(report) => {
for network in &report.networks { for network in &report.networks {
out.push(network_section(network, &report.endpoint_id)); // Whether another configured network answers to the same
// name, which is what makes two sections look like one.
let shared = report
.networks
.iter()
.filter(|other| other.name == network.name)
.count()
> 1;
out.push(network_section(network, &report.endpoint_id, shared));
} }
} }
// Without an agent there is no live view, but the store still knows // Without an agent there is no live view, but the store still knows
@@ -1306,23 +1314,56 @@ fn member_rows<'a>(network: &'a tsunagi::ipc::NetworkReport, own_id: &str) -> Ve
} }
/// One network: what it is, who is in it, and what has happened since start. /// One network: what it is, who is in it, and what has happened since start.
fn network_section(network: &tsunagi::ipc::NetworkReport, own_id: &str) -> report::Section { fn network_section(
network: &tsunagi::ipc::NetworkReport,
own_id: &str,
name_shared: bool,
) -> report::Section {
use report::{Health, Row, Section}; use report::{Health, Row, Section};
let mut section = Section::new(format!("network {}", network.name)); // The id is in the heading, not only in a row: a name is a label a user
// chose and two networks may share one, so a heading without the id
// reads as one network that is somehow both working and empty.
let mut section = Section::new(format!(
"network {} ({})",
network.name,
short(&network.network_id, 10)
));
section.push(if network.active { section.push(if network.active {
Row::new( Row::new(Health::Good, "state", network.network_id.clone())
Health::Good,
"state",
format!("active {}", network.network_id),
)
} else { } else {
Row::new( Row::new(Health::Degraded, "state", "inactive")
Health::Degraded,
"state",
format!("inactive {}", network.network_id),
)
}); });
if name_shared {
section.push(
Row::new(
Health::Degraded,
"name",
format!(
"another configured network is also called `{}`",
network.name
),
)
.with_note(
"a network is its name *and* its secret, so these two share nothing. \
Usually a mistyped secret; `tsunagi id secret` shows which is which.",
),
);
}
if let Some(conflict) = &network.range_conflict {
section.push(
Row::new(
Health::Degraded,
"range",
format!("cannot use {conflict}: another network here already does"),
)
.with_note(
"one agent has one interface, so an address belongs to one network. \
This one waits to adopt whatever its members settle on; give it \
`--ipv4-range` of its own to propose one.",
),
);
}
if let Some(overlay) = &network.overlay { if let Some(overlay) = &network.overlay {
section.push(Row::new( section.push(Row::new(
@@ -1343,10 +1384,15 @@ fn network_section(network: &tsunagi::ipc::NetworkReport, own_id: &str) -> repor
let rows = member_rows(network, own_id); let rows = member_rows(network, own_id);
let online = rows.iter().filter(|row| row.online()).count(); let online = rows.iter().filter(|row| row.online()).count();
if rows.is_empty() { if rows.is_empty() {
// Why there is nobody, rather than just that there is nobody: the
// two reasons want different actions.
section.push(Row::new( section.push(Row::new(
Health::Info, Health::Info,
"members", "members",
"none known yet; nobody else has joined", match (&network.range, &network.range_conflict) {
(None, Some(_)) => "none: this network has no range to allocate from",
_ => "none known yet; nobody else has joined",
},
)); ));
} else { } else {
section.push(Row::new( section.push(Row::new(
@@ -2317,6 +2363,8 @@ async fn build_report(
.map_or(0, |candidate| candidate.consecutive_failures), .map_or(0, |candidate| candidate.consecutive_failures),
}) })
.collect(), .collect(),
range: network.range.map(|range| range.to_string()),
range_conflict: network.range_conflict.map(|range| range.to_string()),
dial_failures: network.metrics.dial_failures, dial_failures: network.metrics.dial_failures,
handshake_failures: network.metrics.handshake_failures, handshake_failures: network.metrics.handshake_failures,
control_messages: ( control_messages: (
@@ -2580,6 +2628,8 @@ mod status_tests {
failed_dials: 0, failed_dials: 0,
}, },
], ],
range: Some("10.13.37.0/24".into()),
range_conflict: None,
// Everything below happened while the peer was away. // Everything below happened while the peer was away.
dial_failures: 9, dial_failures: 9,
handshake_failures: 0, handshake_failures: 0,
@@ -2592,6 +2642,42 @@ mod status_tests {
} }
} }
#[test]
fn two_networks_with_one_name_are_told_apart_and_flagged() {
// The confusing case: two sections headed identically, one working
// and one empty, read as a single network that is somehow both.
let network = network_after_a_peer_returned();
let mut out = report::Report::new();
out.push(network_section(&network, OWN, true));
let text = out.render(false);
assert!(
text.contains(&format!("network LAB ({})", short(&network.network_id, 10))),
"the heading must identify the network, not just name it:\n{text}"
);
assert!(text.contains("also called `LAB`"), "{text}");
assert!(text.contains("mistyped secret"), "{text}");
}
#[test]
fn a_network_with_no_range_says_that_is_why_it_is_empty() {
// Rather than "nobody else has joined", which points at the wrong
// thing entirely: nobody can join a network with no addresses.
let mut network = network_after_a_peer_returned();
network.peers.clear();
network.members.clear();
network.overlay = None;
network.range = None;
network.range_conflict = Some("10.13.37.0/24".into());
let mut out = report::Report::new();
out.push(network_section(&network, OWN, false));
let text = out.render(false);
assert!(text.contains("no range to allocate from"), "{text}");
assert!(text.contains("another network here already does"), "{text}");
assert!(text.contains("--ipv4-range"), "the fix is named: {text}");
}
#[test] #[test]
fn counters_from_the_past_do_not_grade_the_present() { fn counters_from_the_past_do_not_grade_the_present() {
// A peer that left and returned leaves dial failures and a packet // A peer that left and returned leaves dial failures and a packet
@@ -2600,7 +2686,7 @@ mod status_tests {
// network look broken. // network look broken.
let network = network_after_a_peer_returned(); let network = network_after_a_peer_returned();
let mut out = report::Report::new(); let mut out = report::Report::new();
out.push(network_section(&network, OWN)); out.push(network_section(&network, OWN, false));
assert_eq!(out.worst(), Health::Good, "{}", out.render(false)); assert_eq!(out.worst(), Health::Good, "{}", out.render(false));
let text = out.render(false); let text = out.render(false);
@@ -2649,7 +2735,7 @@ mod status_tests {
}); });
let mut out = report::Report::new(); let mut out = report::Report::new();
out.push(network_section(&network, OWN)); out.push(network_section(&network, OWN, false));
let text = out.render(false); let text = out.render(false);
assert_eq!(out.worst(), Health::Good, "{text}"); assert_eq!(out.worst(), Health::Good, "{text}");
@@ -2665,7 +2751,7 @@ mod status_tests {
network.peers[0].transport = "relay".into(); network.peers[0].transport = "relay".into();
let mut out = report::Report::new(); let mut out = report::Report::new();
out.push(network_section(&network, OWN)); out.push(network_section(&network, OWN, false));
assert_eq!(out.worst(), Health::Degraded, "{}", out.render(false)); assert_eq!(out.worst(), Health::Degraded, "{}", out.render(false));
assert!(out.render(false).contains("relay")); assert!(out.render(false).contains("relay"));
} }
@@ -2676,7 +2762,7 @@ mod status_tests {
network.overlay = Some(overlay(vec![tunnel(ONLINE, None)])); network.overlay = Some(overlay(vec![tunnel(ONLINE, None)]));
let mut out = report::Report::new(); let mut out = report::Report::new();
out.push(network_section(&network, OWN)); out.push(network_section(&network, OWN, false));
let text = out.render(false); let text = out.render(false);
assert_eq!(out.worst(), Health::Degraded, "{text}"); assert_eq!(out.worst(), Health::Degraded, "{text}");
assert!(text.contains("no WireGuard handshake yet"), "{text}"); assert!(text.contains("no WireGuard handshake yet"), "{text}");
@@ -2692,7 +2778,7 @@ mod status_tests {
network.handshake_failures = 4; network.handshake_failures = 4;
let mut out = report::Report::new(); let mut out = report::Report::new();
out.push(network_section(&network, OWN)); out.push(network_section(&network, OWN, false));
let text = out.render(false); let text = out.render(false);
assert_eq!(out.worst(), Health::Degraded, "{text}"); assert_eq!(out.worst(), Health::Degraded, "{text}");
assert!(text.contains("same secret"), "{text}"); assert!(text.contains("same secret"), "{text}");
+34 -7
View File
@@ -48,6 +48,7 @@ use crate::net::EndpointAdapter;
use crate::overlay::PacketCarrier; use crate::overlay::PacketCarrier;
use crate::proto::handshake; use crate::proto::handshake;
use crate::proto::message::ControlMessage; use crate::proto::message::ControlMessage;
use crate::state::Ipv4Range;
use crate::storage::{CacheOutcome, Storage}; use crate::storage::{CacheOutcome, Storage};
use network::{InboundSession, NetCommand, NetworkHandle, RuntimeParams}; use network::{InboundSession, NetCommand, NetworkHandle, RuntimeParams};
@@ -353,18 +354,20 @@ impl Agent {
/// waits to adopt whatever it settles on. One agent has one interface, so /// waits to adopt whatever it settles on. One agent has one interface, so
/// proposing a range it could not route would be worse than having none: /// proposing a range it could not route would be worse than having none:
/// the lowest author's range wins, and the collision would spread. /// the lowest author's range wins, and the collision would spread.
fn reserve_range(&self, network: NetworkId) -> Option<crate::state::Ipv4Range> { fn reserve_range(&self, network: NetworkId) -> (Option<Ipv4Range>, Option<Ipv4Range>) {
let wanted = self.inner.config.overlay_ipv4_range?; let Some(wanted) = self.inner.config.overlay_ipv4_range else {
return (None, None);
};
let reservation = crate::overlay::NetworkRoutes { let reservation = crate::overlay::NetworkRoutes {
range: Some(wanted), range: Some(wanted),
local: None, local: None,
peers: Vec::new(), peers: Vec::new(),
}; };
match self.inner.routes.set_network(network, reservation) { match self.inner.routes.set_network(network, reservation) {
Ok(()) => Some(wanted), Ok(()) => (Some(wanted), None),
Err(err) => { Err(err) => {
tracing::info!(%err, "not proposing a range for this network"); tracing::info!(%err, "not proposing a range for this network");
None (None, Some(wanted))
} }
} }
} }
@@ -456,6 +459,26 @@ impl Agent {
) -> Result<NetworkId> { ) -> Result<NetworkId> {
let keys = NetworkKeys::derive(name, secret); let keys = NetworkKeys::derive(name, secret);
let network_id = keys.network_id(); let network_id = keys.network_id();
// A network's identity is its name *and* its secret, so the same
// name with a different secret is a different network — and one that
// looks identical in anything that shows a name. Almost always a
// mistyped secret, so it is said out loud rather than left to be
// discovered as an empty network sitting beside a working one.
if let Ok(configured) = self.inner.storage.list_networks().await {
for other in configured {
if other.name == *name && other.network_id != network_id {
tracing::warn!(
"`{name}` is already configured with a different secret, as {}. \
Joining with this one adds a second network under the same name; \
they share nothing. Check the secret, or use `tsunagi id secret` \
to see which is which.",
other.network_id
);
}
}
}
self.inner self.inner
.storage .storage
.upsert_network(network_id, name.clone(), secret.clone(), true) .upsert_network(network_id, name.clone(), secret.clone(), true)
@@ -495,6 +518,7 @@ impl Agent {
return Err(Error::NetworkAlreadyActive(network_id)); return Err(Error::NetworkAlreadyActive(network_id));
} }
let (reserved, conflict) = self.reserve_range(network_id);
let handle = network::spawn(RuntimeParams { let handle = network::spawn(RuntimeParams {
keys, keys,
adapter: self.inner.adapter.clone(), adapter: self.inner.adapter.clone(),
@@ -510,7 +534,8 @@ impl Agent {
hostname: self.inner.read_hostname(), hostname: self.inner.read_hostname(),
transport: self.inner.transport.get().cloned(), transport: self.inner.transport.get().cloned(),
device_secret: self.inner.identity.signing_key(), device_secret: self.inner.identity.signing_key(),
ipv4_range: self.reserve_range(network_id), ipv4_range: reserved,
range_conflict: conflict,
}); });
networks.insert(network_id, handle); networks.insert(network_id, handle);
drop(networks); drop(networks);
@@ -658,9 +683,11 @@ impl Agent {
state: NetworkState::Inactive, state: NetworkState::Inactive,
peers: Vec::new(), peers: Vec::new(),
candidates: Vec::new(), candidates: Vec::new(),
// An inactive network has no runtime to ask; the roster comes // An inactive network has no runtime to ask; the roster and
// from one. Empty, not invented. // the range come from one. Empty, not invented.
members: Vec::new(), members: Vec::new(),
range: None,
range_conflict: None,
metrics: NetworkMetrics::default(), metrics: NetworkMetrics::default(),
}); });
} }
+5
View File
@@ -130,6 +130,9 @@ pub(crate) struct RuntimeParams {
/// The IPv4 overlay range this agent would use, if the network has not /// The IPv4 overlay range this agent would use, if the network has not
/// already settled on another one. /// already settled on another one.
pub(crate) ipv4_range: Option<Ipv4Range>, pub(crate) ipv4_range: Option<Ipv4Range>,
/// The range it was configured with but cannot have, because another
/// network on this agent holds it.
pub(crate) range_conflict: Option<Ipv4Range>,
/// How data plane links are opened. `None` disables the data plane. /// How data plane links are opened. `None` disables the data plane.
pub(crate) transport: Option<Arc<dyn PacketTransport>>, pub(crate) transport: Option<Arc<dyn PacketTransport>>,
} }
@@ -1439,6 +1442,8 @@ impl Runtime {
peers, peers,
candidates, candidates,
members, members,
range: self.effective_range(),
range_conflict: self.params.range_conflict,
metrics: self.metrics.clone(), metrics: self.metrics.clone(),
} }
} }
+9
View File
@@ -159,6 +159,15 @@ pub struct NetworkStatus {
pub candidates: Vec<CandidateStatus>, pub candidates: Vec<CandidateStatus>,
/// Members the signed state knows about, whether connected or not. /// Members the signed state knows about, whether connected or not.
pub members: Vec<MemberStatus>, pub members: Vec<MemberStatus>,
/// The overlay range this network uses, once it has one.
pub range: Option<crate::state::Ipv4Range>,
/// The range it could not have, because another network on this agent
/// already holds it.
///
/// One agent has one interface, so an address belongs to one network.
/// This network waits to adopt whatever its members settle on instead of
/// proposing something it could not route.
pub range_conflict: Option<crate::state::Ipv4Range>,
/// Per-network counters. /// Per-network counters.
pub metrics: NetworkMetrics, pub metrics: NetworkMetrics,
} }
+6 -1
View File
@@ -149,7 +149,12 @@ pub struct NetworkReport {
pub handshake_failures: u64, pub handshake_failures: u64,
/// Control messages sent and received. /// Control messages sent and received.
pub control_messages: (u64, u64), pub control_messages: (u64, u64),
/// The overlay, when an IP plugin is running one. /// The overlay range this network uses, once it has one.
pub range: Option<String>,
/// The range it could not have, because another network on this agent
/// already holds it.
pub range_conflict: Option<String>,
/// The overlay, when a protocol is running one.
pub overlay: Option<OverlayReport>, pub overlay: Option<OverlayReport>,
} }
+1 -1
View File
@@ -205,7 +205,7 @@ pub async fn set_hostname(path: impl AsRef<Path>, hostname: &str) -> Result<Stri
/// ///
/// Bump it whenever [`Request`], [`Response`] or anything they contain /// Bump it whenever [`Request`], [`Response`] or anything they contain
/// changes shape. /// changes shape.
pub const CONTROL_PROTOCOL: u32 = u32::from_be_bytes([b'T', b'S', b'N', 5]); pub const CONTROL_PROTOCOL: u32 = u32::from_be_bytes([b'T', b'S', b'N', 6]);
async fn write_message<T: serde::Serialize>(stream: &mut UnixStream, value: &T) -> Result<()> { async fn write_message<T: serde::Serialize>(stream: &mut UnixStream, value: &T) -> Result<()> {
let encoded = postcard::to_stdvec(value) let encoded = postcard::to_stdvec(value)