Respect address tags in port_settings_apply

[cfzimmerman]

Reproduces: #342

Tests demonstrate

• 🐛 Tagged port_settings_apply can delete addresses from another tag.
• 🐛 Tagged port_settings_get returns addresses belonging to other tags.
• ❓ TODO: Tagged port_settings_clear affects addresses belonging to other tags?
• 🆗 Spot deletion is fine. I added a test out of curiosity, but that logic is fine.
• 🆗 The same address cannot be registered by multiple peers. Afaik that's especially ok in our usage because tfportd will only register link local addresses, and sled-agent will only register routable addresses.

Summary:

• Require a tag arg when creating the diffable link representation. Only include addresses in the LinkSpec and LinkSettings that match the tag.
• Where tags are currently optional, use unwrap_or("") as a default. We were already doing this in multiple places.

[cfzimmerman]

This failed before the second commit. addr_ns_spot_delete passed before changes.

[cfzimmerman]

🤖 CLAUDE REVIEW

Review: dendrite#370 — Filter addresses by tag in LinkSpec constructor

Base: main @ 2475028. Head: 881403e. Two commits, 4 files.

The core change is right: making LinkSpec tag-scoped stops a port_settings_apply
from one client silently deleting another client's addresses, and it also fixes a
latent rollback bug (addr_del_v4's unwind used to re-create a foreign address
stamped with the applier's tag, losing the original owner). cargo check -p dpd --all-targets and cargo check -p dpd-client --features chaos --tests both pass on
helios.

My concern is that the namespace is only half-built. apply and clear are now
tag-scoped, but get, link deletion, and the per-address endpoints are not — and the
one in-tree consumer that does read-compare-write against this API (omicron's
DpdPortReconciler) reads through the un-scoped side.

---

1. get_port_settings takes a tag and ignores it — omicron's port reconciler can no longer converge

dpd/src/port_settings.rs:703

get_port_settings now threads a tag into the Context, but the only consumer of
ctx.tag on the read path is Context::link_spec, which
get_port_settings_locked never calls. It builds its reply from
LinkSettings::from(&*link) (dpd/src/api_server.rs:2952), which unions every
link.ipv4 / link.ipv6 entry regardless of tag. So after this PR the read side and
the write side of the same endpoint family disagree about what "this tag's settings"
means.

That asymmetry lands directly on omicron. PortReconciler::dpd_get_current_settings
(sled-agent/scrimlet-reconcilers/src/dpd_reconciler/port_reconciler.rs:131) calls
port_settings_get(port, OMICRON_DPD_TAG), converts the reply to
DiffablePortSettings (dropping only unicast link-locals), and diffs it against
RackNetworkConfig. Concretely, with any foreign-tagged non-link-local address on a
managed link:

• Non-convergence. get reports the foreign address, the diff says "drift",
  the port goes into to_apply, and the follow-up
  port_settings_apply(port, OMICRON_DPD_TAG, …) now computes its diff over the
  tag-filtered spec, finds nothing to do, and returns Ok. Next reconciliation
  pass: identical. The port never appears in unchanged, and dpd gets a settings
  apply on every pass, forever. Before this PR the apply deleted the foreign
  address and the loop terminated.
• Hard failure, if the foreign address happens to be one omicron wants. The
  filtered switch spec omits it, so the diff plans an add;
  create_ipv4_address_locked (dpd/src/link.rs:1052) tests link.ipv4.contains()
  and Ipv4Entry's PartialEq/Ord compare address only
  (dpd-types/versions/src/impls/port.rs:33-52), so it returns DpdError::Exists,
  the whole transaction rolls back, and the port sits in apply_failures permanently.

Fix: filter by ctx.tag in get_port_settings_locked, or drop the parameter and be
explicit that get is unscoped. Either way it should not be silently accepted and
discarded — and whichever you pick, the omicron reconciler's expectations want a
second look before this lands in a release omicron picks up.

2. Link deletion is still un-scoped, so the tag filter leaks ASIC table entries

dpd/src/port_settings.rs:378

calculate_links still derives links_to_del from ctx.link_map.port_links(port_id)
— every link on the port, no tag involved — but remove_link now walks a
tag-filtered spec.ipv4/spec.ipv6. So the link goes away while the addresses
belonging to other tags never get port_ip::ipv4_delete/ipv6_delete called on them.

Nothing downstream picks up the slack. remove_link sets delete_me and triggers
the reconciler; link_reconcile (dpd/src/link.rs:1813, :1871) calls
unplumb_link — which clears the uplink, MAC and ASIC port but never touches
PortAddrIpv4/PortAddrIpv6 — and then LinkMap::delete_link
(dpd/src/link.rs:98), which is a bare HashMap::remove. Note that
Switch::delete_link (dpd/src/link.rs:704) does drain the tables first; the
port_settings path bypasses it.

Reachable path today: tfportd puts a link-local IPv6 tagged tfportd on every link
it sees (tfportd/src/ports.rs:257). Omicron calls
port_settings_clear(qsfp, OMICRON_DPD_TAG) when a port leaves RackNetworkConfig
(port_reconciler.rs:183), and wicketd's preflight teardown applies an empty
PortSettings under the same tag
(wicketd/src/preflight_check/uplink.rs:808). Both now delete the link and leave
tfportd's two table entries programmed with no owning Link. For v6 the leaked
drop_key is {dst_addr, in_port: val 0, mask 0} (dpd/src/table/port_ip.rs:83) —
a switch-wide blackhole for that address that survives until dpd restarts, and that
will collide if the link is recreated and tfportd re-adds the same address.

Pre-PR the unfiltered spec deleted everything, so this is a regression introduced
here, not a pre-existing hole. Either scope link deletion to the tag too, or have
remove_link drain all addresses (not just the tag's) since the link itself is going
away.

3. A foreign-tagged address in the desired set now fails the whole apply

dpd/src/port_settings.rs:113, dpd/src/port_settings.rs:518

Called out inline above but worth its own line, because the fix is different. Because
address identity ignores the tag, LinkSpec::from_link filtering it out does not make
the address available — it just makes the diff plan an add that is guaranteed to
fail. There's no way for a client to tell "someone else owns this address" from a
generic Exists, and the failure takes the entire atomic apply with it. Worth
deciding explicitly what should happen when two tags want the same address: a
distinct error naming the owning tag, or treating a same-address different-tag entry
as satisfied. Right now it's an accident of Ipv4Entry: Eq.

4. The per-address endpoints are not tag-scoped, so the namespace isn't enforced

dpd/src/link.rs:1102, dpd/src/link.rs:1204

delete_ipv4_address_locked/delete_ipv6_address_locked build
Ipv4Entry { tag: String::new(), addr } and rely on address-only equality, so
link_ipv4_delete/link_ipv6_delete will happily remove an address owned by another
tag. tfportd already does exactly this: ensure_address_match
(tfportd/src/ports.rs:240) deletes whatever link_local dpd reports for a link,
with no tag check.

That's pre-existing, but this PR is the one establishing "addresses are namespaced by
tag", and the new addr_ns_spot_delete test only ever deletes its own addresses —
so it reads as if the property is verified when the enforcement isn't there. Either
scope the delete endpoints or say plainly in the API docs that tags are advisory
outside of port_settings.

5. The tag's meaning changed for port_settings_* but the API docs and versioning didn't

dpd/src/api_server.rs:1845, dpd-api/src/lib.rs:1649-1741

query.tag was previously write-only — a label stamped on entries this call created.
It is now also a filter that determines which existing state the call can even see.
unwrap_or("") additionally collapses "no tag supplied" into "the empty-tag
namespace", so a caller that omits the parameter now manages only untagged addresses
instead of all of them. That's a material behavioural change to
port_settings_apply/clear/get with no doc-comment update on any of the three
endpoints and no version bump (the schema is unchanged, so the generated clients
won't notice). At minimum the doc comments should state that the tag scopes the
address set.

6. Test coverage misses the case that actually breaks (finding 2)

dpd-client/tests/chaos_tests/port_settings.rs:532

addr_ns_persistent_create deliberately keeps the link alive across the second apply
(it passes the link with addrs: Vec::new()), and addr_ns_spot_delete never removes
a link at all. Neither test exercises port_settings_clear, nor an apply that drops
the link from the settings map — which is precisely where the isolation falls over.
A third case (tag1 addresses on a link, tag2 clears the port, assert tag1's ASIC
entries are gone rather than orphaned) would have caught finding 2. Worth adding
alongside the fix.

7. IpRng draws from the whole address space

dpd-client/tests/chaos_tests/util.rs:52

Ipv4Addr::from_bits(rng.random()) / Ipv6Addr::from_bits(rng.random()) can produce
0.0.0.0, 127/8, 224/4, ::, ff00::/8, or a fe80::/10 link-local. dpd
doesn't validate any of these today (link_ipv4_create goes straight to
create_ipv4_address), so the current seeds are fine — but the values are only stable
for this rand version, and a rand uprev silently redraws every address. Link-local
v6 in particular is special-cased by tfportd and by omicron's diff. Cheap insurance:
generate within 203.0.113.0/24 and fd00::/8 like the rest of the file does, or
mask the high bits.

Nits

• dpd-client/tests/chaos_tests/port_settings.rs:12-13: the file already imports
  init_harness via use super::harness::{…} and link_list_ipv4 via
  use super::util::{…}; the two new use crate::chaos_tests::… lines plus
  harness::init_harness(…) at the call sites are a second, inconsistent style in
  the same file.
• addr_ns_spot_delete's doc comment has a stray sentence break: "This should fail.
  because the address already exists".
• addr_ns_spot_delete asserts the duplicate-tag rejection for v4 only; the v6 case
  goes through a separate code path (create_ipv6_address_locked) and is untested.