Respect address tags in port_settings_* - #370
Conversation
- Tagged port_settings_apply can delete addresses from another tag. - Tagged port_settings_get returns addresses belonging to other tags. - Tagged port_settings_clear can delete addresses from another tag.
Scope link address CRUD to user-provided tags. Much of this is already implied by the dropshot API.
It was not used in the previous version, and implying support for tags is incorrect. port_settings_clear just deletes links from a port.
c586154 to
73b4a3c
Compare
| /// - Port settings apply away the tag2 addresses | ||
| /// - The tag1 addresses should still be on the link | ||
| #[tokio::test] | ||
| async fn addr_ns_persistent_create() -> anyhow::Result<()> { |
There was a problem hiding this comment.
This failed before the second commit. addr_ns_spot_delete passed before changes.
cfzimmerman
changed the title
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
94ace38 to
e6c362d
Compare
| self.ipv6 | ||
| .iter() | ||
| .find(|entry| (entry.addr.segments()[0] & 0xffc0) == 0xfe80) | ||
| .map(|entry| entry.addr) | ||
| self.ipv6.keys().find(|addr| addr.is_unicast_link_local()).copied() |
There was a problem hiding this comment.
This change isn't strictly necessary, but the previous closure did exactly what is_unicast_link_local does. I don't see a compelling reason not to use std here.
cfzimmerman
changed the title
e6c362d to
fba64cc
Compare
cfzimmerman
left a comment
cfzimmerman
left a comment
There was a problem hiding this comment.
🤖 This message was written by Claude
Review of the tag-isolation fix. The core change — BTreeSet<Ipv*Entry> to BTreeMap<Ipv*Addr, String> — is the right call: it makes "one address has exactly one owner" a property of the type instead of a custom Ord that compared on addr while pretending to be a set of entries. That old shape is what made #342 possible, and the range() rewrite of the paginated list falls out much cleaner as a result. Carrying the owner tag on each LinkSpec entry so an apply can reconstruct other tags' addresses is a sound fix for the modify path without adding a second source of truth. The rollback closures in addr_del_v* correctly restore the original tag rather than ctx.tag, which is easy to get wrong.
It's worth recording that the headline fix is real and observable in the consumer: omicron/sled-agent/scrimlet-reconcilers/src/dpd_reconciler/port_reconciler.rs:466 strips link-local v6 out of its change computation with the comment "dont consider link local addresses in change computation" — a workaround for exactly the pre-PR behavior where a tagged apply clobbered tfportd's fe80::. Post-PR that filter should be redundant, since sled-agent already calls port_settings_get(port, DPD_TAG).
Four things I'd want resolved before this leaves draft:
- Link deletion is still completely tag-blind — a tagged apply that omits a link destroys it and every other tag's addresses on it. Same class of bug as #342, reached through a different door.
port_settings_applysilently discards a caller's claim on an address owned by another tag, which puts a reconciler into a permanent no-progress loop.- Untagged
port_settings_getnow returns nothing instead of everything, on the existing API version with no bump. delete_ipv*_address_lockedreturnsOk(())for a delete it deliberately skipped, with a rollback hazard behind it.
Plus two test issues and a nit. Three things I checked and cleared: is_unicast_link_local() is exactly equivalent to the old segments()[0] & 0xffc0 == 0xfe80 mask and is stable (verified on rustc 1.90); the Bound::Excluded(addr) pagination rewrite preserves ordering because the old Ipv4Entry: Ord compared only the address; and the port_settings_clear_v2 shim's claim that v13 callers "can use the new version identically" is accurate — the old endpoint already deleted every link regardless of tag, so ignoring query preserves real behavior rather than changing it.
Second opinion: the design skeptic
1. What customer problem is this actually solving? A concrete one, and the workaround in omicron is the proof. tfportd owns link-local addresses and sled-agent owns routable ones on the same links; today a tagged port_settings_apply from either deletes the other's. On a real rack that is a link-local address disappearing out from under ddm mid-reconcile, with no error anywhere — the daemon that lost its address quietly stops working until its next pass. sled-agent's response was to stop looking at link-local addresses entirely, which is a reasonable patch on a bug it could not fix from its side. Fixing it properly in dpd lets that filter be deleted.
2. Does the value warrant the complexity? For the map change, comfortably — it removes a concept (an entry whose equality secretly ignores half its fields) rather than adding one. from_settings(settings, tag, master) is where the new cost sits. It introduces an invariant a future reader must hold: a LinkSpec built from settings contains addresses the caller never mentioned, and the caller's own claim loses to an existing foreign owner. Two non-obvious rules encoded in an extend with a filter, neither stated at the call site. A doc line on from_settings saying "foreign entries win" would pay for itself; renaming master to current would help more, since master suggests authority when it means "what is on the switch right now."
3. What is the simplest robust alternative? Three candidates, cheapest first.
Do nothing: not viable, the bug is real, filed, and already worked around downstream.
Fix it one layer down — make the tag a real type. You flag this in the PR body and defer it; deferring is right, but for a sharper reason than scope. The moment Tag is a newtype, the "" sentinel for "untagged" has to be named, and that is a genuine design decision: is untagged a distinct owner, or does it mean "anyone"? Today the answer is smeared across unwrap_or("") in api_server.rs and != tag comparisons in port_settings.rs, and it currently means "a distinct owner named empty-string" — which is why a tagged client can never adopt an untagged address, and why the untagged-GET finding below exists at all. That is defensible, but nobody decided it. Deciding it is the prerequisite for the newtype, not the other way round.
Fix the caller: worth naming because it is the alternative that does not exist. There is no caller-side fix — the whole point of the tag is that the callers are mutually unaware daemons. dpd is the only place this can be fixed, which is the argument for closing the link-deletion hole here rather than leaving a second gap of the same shape.
The uncomfortable question. Four of the findings below share one pattern: the server does less than the caller asked and reports success. Tag isolation only pays off if a client can tell when its request was refused — otherwise you have replaced "my address got deleted" with "my address was never created," which is quieter but no easier to debug at 3am, and in the reconciler case turns a visible failure into an invisible infinite loop. Before this merges I'd want one sentence in the API docs stating what a caller is entitled to assume when port_settings_apply returns Ok on a link another tag also writes to.
| self.links.modify = links_to_mod | ||
| .map(|id| { | ||
| let settings_link = (&settings.links[&id.0]).into(); | ||
| let switch_link = ctx.link_spec(*id).expect( | ||
| let before = ctx.link_spec(*id).expect( | ||
| "link existence is guaranteed by the locked link map", | ||
| ); | ||
| (id, settings_link, switch_link) | ||
| let conf = &settings.links[&id.0]; | ||
| let after = | ||
| LinkSpec::from_settings(conf, ctx.tag, Some(&before)); |
There was a problem hiding this comment.
🤖 This message was written by Claude
Link deletion is still tag-blind, so the #342 bug survives one level up.
This is the fix for the modify path — from_settings(conf, ctx.tag, Some(&before)) carefully preserves other tags' addresses. But the delete path just above (links_to_del at line 218, self.links.delete at 229-237) got no equivalent treatment: it is still switch_links.difference(&settings_links), and remove_link then iterates the whole spec.ipv4/spec.ipv6 map — built from LinkSpec::from(&Link), i.e. every tag's addresses — and deletes them all.
Concretely: tfportd creates link 0 on qsfp0 and registers its link-local. sled-agent calls port_settings_apply?tag=sled-agent with settings naming only link 1. Link 0 is on the switch and absent from sled-agent's body, so it lands in links.delete and tfportd's address goes with it. That is #342 through a different door, and the new tests don't cover it — addr_ns_persistent_create applies an empty addrs for a link that is still present in the body, which exercises the modify path only.
There may be no clean answer today, since links carry no tag of their own — only their addresses do. But the options deserve an explicit decision rather than silence: skip deletion of a link whose addresses are all foreign; refuse the apply with a conflict; or state in the API docs that a tagged apply owns the port's link set outright and only addresses are tag-scoped. Note you already updated port_settings_clear's doc to be honest about this ("Deletes all the links belonging to a port") — port_settings_apply deserves the same. If it's out of scope, fine, but then #342 shouldn't be closed here without a follow-up issue naming this case.
| if let Some(master) = master { | ||
| ipv4.extend( | ||
| master | ||
| .ipv4 | ||
| .iter() | ||
| .filter(|(_, entry_tag)| entry_tag.as_str() != tag) | ||
| .map(|(addr, entry_tag)| (*addr, entry_tag.to_string())), | ||
| ); | ||
|
|
||
| ipv6.extend( | ||
| master | ||
| .ipv6 | ||
| .iter() | ||
| .filter(|(_, entry_tag)| entry_tag.as_str() != tag) | ||
| .map(|(addr, entry_tag)| (*addr, entry_tag.to_string())), | ||
| ); | ||
| } |
There was a problem hiding this comment.
🤖 This message was written by Claude
A caller's claim on an address owned by another tag is silently discarded, which livelocks a reconciler.
extend overwrites on key collision, so ordering matters: every address from l.addrs goes in with the caller's tag first, then this block overwrites any that a different tag already owns. The result is after[addr] == before[addr], the modify_link filter (!ipv4_before.contains_key(addr)) skips it, and port_settings_apply returns Ok — while the address the caller asked for is still owned by someone else. The response body, built by Link::settings(tag), omits it entirely.
The failure mode is worse than a lost write. sled-agent's port_reconciler diffs desired against what port_settings_get(port, DPD_TAG) returns. If the desired config names an address tfportd owns, every pass sees current ≠ desired, re-applies, gets the same silent no-op, and never converges — ports_to_apply never drains and nothing is logged. A hard error would surface in one pass; this hides forever.
It's also inconsistent with the spot-add path: create_ipv4_address_locked returns DpdError::Exists for exactly this conflict, and your own new test asserts that it does. Same conflict, two answers depending on the endpoint. Given the PR summary says "the same address cannot be registered by multiple peers," the bulk path should say so too — collect collisions and fail the apply naming the address and its current owner.
Separately: master reads as "the authoritative one," but it holds current switch state. current describes it better, and from_settings deserves a doc line saying foreign entries win — that rule is not guessable from the signature.
| let path = path.into_inner(); | ||
| let query = query.into_inner(); | ||
| let port_id = path.port_id; | ||
| let tag = query.tag.as_deref().unwrap_or(""); |
There was a problem hiding this comment.
🤖 This message was written by Claude
This collapses "no tag supplied" into "the empty tag", which turns an untagged port_settings_get into a blank response.
Previously get_port_settings took the Option through and the old impl From<&Link> for LinkSettings added every address unconditionally, so a bare GET /port/qsfp0/settings showed the whole picture. Now the None becomes "", and Link::settings (line 2958) filters *t == tag — so an untagged GET returns only addresses registered with the literal empty tag, which in practice is none of them. An operator or support engineer curling this endpoint to debug a configured port sees an empty addrs list on every link, and there is no longer any way to ask for the unfiltered view.
sled-agent is unaffected — it always passes DPD_TAG (port_reconciler.rs:131) — so this lands on humans and swadm, not the control plane. But note the versioning asymmetry: port_settings_clear got a v14 bump for its semantic change while this one rides on the existing VERSION_ALLOW_DDM_TRAFFIC.. range, so a v13 client gets new port_settings_get semantics with no signal that anything changed.
Carrying Option<&str> through to Link::settings and treating None as unfiltered keeps the debug view and costs almost nothing. Same unwrap_or("") at line 1845 for port_settings_apply is fine by comparison — there "" is a real owner to write under, not a filter.
| btree_map::Entry::Occupied(slot) | ||
| if tag.is_some_and(|t| t != slot.get()) => | ||
| { | ||
| // Don't delete an addr that belongs to another tag. | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
🤖 This message was written by Claude
Returning Ok(()) here reports success for a delete that deliberately did nothing — and sets a rollback trap.
The doc comment above says "Returns Err if the address is not found," but there are two ways to not delete and only one is an error. A caller passing Some(tag) cannot distinguish "deleted" from "refused, not yours."
Today every Some(tag) caller is internal to port_settings.rs and reads the tag out of before, so the arm is unreachable — which is exactly what makes it a trap. If it ever does fire inside addr_del_v4, that function has already called rb.wind(... create_ipv4_address_locked(addr, tag)). A later failure in the same transaction then unwinds into a create against an address that was never removed, which returns DpdError::Exists and converts an ordinary recoverable failure into DpdError::Unwind / ROLLBACK_FAILURE_ERROR_CODE. A silent no-op becomes a poisoned rollback.
Make the outcome explicit: return DpdError::NoSuchAddress, add a distinct "owned by another tag" error, or change the signature to DpdResult<bool> so callers must look. Same applies to delete_ipv6_address_locked at 1223.
| .into_iter() | ||
| .filter_map(|entry| { | ||
| (entry.tag == self.v4_entry.tag) | ||
| .then(|| IpAddr::from(entry.addr)) | ||
| }) | ||
| .chain(v6.items.into_iter().filter_map(|entry| { | ||
| (entry.tag == self.v4_entry.tag) | ||
| .then(|| IpAddr::from(entry.addr)) | ||
| })) |
There was a problem hiding this comment.
🤖 This message was written by Claude
The v6 branch filters on the v4 entry's tag.
Line 807 and line 811 both read self.v4_entry.tag; the second is inside the IPv6 arm and should be self.v6_entry.tag. TestAddrs::new gives both entries the same tag, so this passes today and will keep passing — which is the problem. The moment a test wants to check that v4 and v6 registrations under different tags stay isolated, this helper will silently report success without testing it, and that is precisely the scenario this PR exists to protect.
While here: Verify::Exhaustive asserts items.len() != 1 rather than comparing against the expected set. It will start failing for a confusing reason as soon as anything else lands on the link — a harness-created link-local, say — and the message won't say what was actually there. Comparing sorted vectors is about the same amount of code and gives a usable failure.
| let address = path.address; | ||
| switch | ||
| .delete_ipv4_address(port_id, link_id, address) | ||
| .delete_ipv4_address(port_id, link_id, address, None) |
There was a problem hiding this comment.
🤖 This message was written by Claude
Spot delete passes None, so it still bypasses tags entirely.
This endpoint now has tag plumbing available and explicitly opts out, which leaves link_ipv4_delete as a way for any client to delete any other client's address — the same isolation hole as #342, on a different endpoint. The PR body lists spot deletion as fine, and the create side genuinely is (create_ipv4_address threads entry.tag correctly), but delete takes no tag in the API at all.
I'm not arguing for a behavior change here — adding tag scoping would be an API break needing its own version bump, and swadm operators plausibly want the override. But the None is doing load-bearing work with no explanation. A short comment saying this endpoint is intentionally tag-agnostic, and why, stops the next reader from filing it as the bug it resembles. Same for the v6 case at line 1256.
| pub fn unique_ipv4(&mut self) -> Ipv4Addr { | ||
| Self::roll_unique(&mut self.claimed, || { | ||
| Ipv4Addr::from_bits(self.rng.random()) | ||
| }) | ||
| } | ||
|
|
||
| /// Returns a random IPv6 address that this instance | ||
| /// has never created before. | ||
| pub fn unique_ipv6(&mut self) -> Ipv6Addr { | ||
| Self::roll_unique(&mut self.claimed, || { | ||
| Ipv6Addr::from_bits(self.rng.random()) | ||
| }) | ||
| } |
There was a problem hiding this comment.
🤖 This message was written by Claude
Unconstrained random addresses will eventually generate ones that aren't valid link addresses.
Ipv4Addr::from_bits(rng.random()) draws uniformly from the whole 32-bit space, so roughly 1 in 8 generated v4 addresses lands in 224.0.0.0/3 (multicast and reserved), plus smaller odds of 0.0.0.0/8, 127.0.0.0/8 and 255.255.255.255. On the v6 side about 1 in 256 lands in ff00::/8 and about 1 in 1024 in fe80::/10.
The link-local case is the one I'd worry about: it isn't merely invalid, it's meaningful. Link::link_local() returns the first link-local address on the link, so a randomly drawn fe80:: can silently become the link's link-local and change behavior unrelated to the test.
It's seeded, so today's runs are deterministic and evidently pass — which makes the seed load-bearing in a way nothing records. Adding a test, reordering existing ones, or changing the seed can start drawing a multicast address and produce a failure that looks nothing like its cause. Drawing from the documentation ranges instead — 198.51.100.0/24 and 2001:db8::/32, randomising host bits — keeps the uniqueness property and removes the class entirely.
(Minor: #[cfg(test)] on mod util_tests in a tests/ file is redundant — integration tests are already compiled with --test.)
| btree_map::Entry::Occupied(_) => Err(DpdError::Exists(format!( | ||
| "IP address {addr} already exists", | ||
| ))), |
There was a problem hiding this comment.
🤖 This message was written by Claude
Nit, but cheap given what this PR is for: now that every address has an owner, "IP address {addr} already exists" omits the one fact the operator needs. When sled-agent gets this for an address tfportd owns, the message should say so — slot.get() is right there. Same for the v6 copy at 1159.
|
fyi human reviewers, if not self-evident, I'm experimenting with claude review infrastructure. |
1 participant
Reproduces and fixes #342
Tests demonstrate
port_settings_applycan delete addresses from another tag.port_settings_getreturns addresses belonging to other tags.port_settings_clearfundamentally does not respect tags. I'm proposing a new version with the same behavior but no tag parameter.Summary
BTreeMap<Ipv*Addr, String>. So an address may necessarily only be added to a link by a single tag. I believe this was the previous behavior too, but it's more self-enforcing thanBTreeSet<Ipv*Entry>. I would like a more descriptiveTagtype, but that implies a lot of API changes and felt out of scope for this PR. Right now we're just trying to make the current API work as expected.Linkstruct, but that invariant was a bit hidden. Where the dropshot api usesOption<String>as a tag, we unwrap that to""inapi_server.rs. We were already doing this in most/all places, but now it's more visible at the top layer.LinkSpecholds addresses for all tags, which is necessary to avoid dropping other tags duringport_settings_apply. This contrasts withLinkSettings, which only reflects addresses from a single tag.