[cfzimmerman]

Reproduces and fixes #342
See that issue if you have questions about this design.

Tests

These tests failed before the second commit.

failures:
    chaos_tests::port_settings::address_cmds_respect_tags
    chaos_tests::port_settings::apply_fails_on_tag_conflict
    chaos_tests::port_settings::settings_apply_respects_tags
    chaos_tests::port_settings::settings_clear_ignores_tags
    chaos_tests::port_settings::settings_rollback_respects_tags

test result: FAILED. 10 passed; 5 failed; 0 ignored; 0 measured; 0 filtered out; finished in 6.80s

• settings_apply_respects_tags is the most important. It exercises the basic enforcement of tag separation. One tag creates an address. Another tag applies an address. The first tag's address still needs to exist. This is the motivating issue between tfportd and sled-agent. This test fails when tag1 lists addresses and sees that its address is missing.
• address_cmds_respect_tags verifies tags are respected by address commands of the type used in swadm, tfportd, and maghemite. CRUD itself works, but the test fails in verification when port_settings_get returns more addresses than link_ipv*_list.
• apply_fails_on_tag_conflict is a weird but meaningful edge case. The passing test create_addr_rejects_overwrites demonstrates that creating an address fails if the address already exists (under another tag). port_settings_apply should allow re-declaring addresses of the same tag, but it should fail when creating an address that belongs to a different tag rather than stealing the address from that tag. Stealing the address complicates deletion, which is implicated if a transaction ever needs to be rolled back. This particular test fails on "Addresses belong to another tag" because stealing is allowed in the current implementation.
• settings_clear_ignores_tags exercises the same issue as settings_apply_respects_tags. If I remove the tag1 check, then this passes. So it's more collateral damage than a legit failure. After all, the test is meant to describe the current behavior rather than declare what it should be.
• settings_rollback_respects_tags fails for various reasons due to rng. But it's usually because tag2 sees tag1's addresses before it has registered anything. A slightly rarer case is tag2 not finding its registered addresses because tag1 overwrote them.

Summary

The main change is in port_settings.rs, where we merge addresses belonging to other tags into the diffable representation.

Context

This PR makes a decision about address tagging in DPD, and it tries to avoid other structural changes. This is stacked below another PR with more bug reproductions and fixes: #374

There's some churn on similar code between the two, but I think keeping them separate is useful. This PR decides what to do about tags, and that PR decides what to do about soft state / asic state divergences amid failure.

Decisions

• Tagged addresses are stored in a 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 than BTreeSet<Ipv*Entry>. I would like a more descriptive Tag type, 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.
• Currently addresses are the only tagged resource on a link.
• If a resource is tagged, it is unconditionally tagged. This was already the case in the Link struct, but that invariant was a bit hidden. Where the dropshot api uses Option<String> as a tag, we unwrap that to "" in api_server.rs. We were already doing this in most/all places, but now it's more visible at the top layer.
• LinkSpec holds addresses for all tags, which is necessary to avoid dropping other tags during port_settings_apply. This contrasts with LinkSettings, which only reflects addresses from a single tag.

Validation

This PR is in a stack. I'll attach Voxel test results to the top PR in the stack.

[cfzimmerman]

Btw once this merges, sled-agent should not need this anymore: https://github.com/oxidecomputer/omicron/blob/6652b9a3a321cb306ea0f2e2328ac789f829f098/sled-agent/scrimlet-reconcilers/src/dpd_reconciler/port_reconciler.rs#L466-L474

It's only required now because sled agent is seeing addresses tagged to tfportd.

@andrewjstone fyi, as a friend and ally of port_settings_* functions.

[cfzimmerman]

This is a strange behavior. But I also don't see a clearly better design under the current API.

• Neither the link nor most of its properties are tagged. So port_settings_clear could delete only the addresses belonging to the tag. But what should it do about non-tagged settings? The current solution of just dropping the link conveniently avoids deciding or caching what a link's default settings should be.
• We could reject "clear" if a link has multiple tags. But that makes this route useless because we expect links to often have multiple tags.
• We could delete this and have caller use link_delete instead. I like this option, but I didn't want this PR to have breaking API changes. The goal right now is just fixing internal issues.

[cfzimmerman]

These move to a global table in #374

[taspelund]

Why the choice to make each address only ever have a single tag? versus something like a BTreeMap<Ipv4Addr, BTreeSet<String>> that could support multiple tags.

Do we have a good read on the history behind the tags, or is this more of a decision on how to move forward because we don't really have that history available to consult?

[rcgoodfellow]

Tags were introduced to associate a client with a resource.

https://github.com/oxidecomputer/dendrite-archived/commit/6bae935bfb78c533c07e2e9c00b544178dcec20f

[cfzimmerman]

Do we have a good read on the history behind the tags, or is this more of a decision on how to move forward because we don't really have that history available to consult?

I really considered ditching tags. Some of that decision process is in #342

Here's what convinced me:

1. One motivation behind tagging was to prevent sled-agent from reconciling away swadm addresses. Tags accomplish that. And without tags, sled-agent must know to ignore IPv6 link local addresses that are implicitly owned by tfportd. John G reasonably called that a code smell.
2. Ownership (through tags) is currently kind of a second-class concept in dpd. However, someday we may need an actual authentication scheme for commands. And then it's useful to have an existing ownership model. So if dpd is headed in a direction, it seems more towards tagging/ownership than away from it.

Why the choice to make each address only ever have a single tag? versus something like a BTreeMap<Ipv4Addr, BTreeSet> that could support multiple tags.

Complexity, and I wasn't sure we needed it. Multiple tags per address implicates something like reference counting, which is complex. Was hoping to avoid that if possible. Do you know of any case where multiple tags need to register the same address?

[taspelund]

I think the coexistence of swadm/sled-agent you're already looking at may warrant multiple tags to make some things less awkward.
e.g.
Let's say sled-agent registers IP X, then swadm adds also wants IP X. If there's only one owner, then how can swadm ensure IP X survives even when sled-agent doesn't want it anymore? There would need to be a separate mechanism to query the IPs from dpd, then to re-add IP X after sled-agent deletes it... Which would leave a timing gap between IP X's removal and re-installation.

If multiple parties can register interest in an IP, that defers deletion until both parties have withdrawn theit interest and there's no timing window where the IP is needed but gone.

[cfzimmerman]

That seems reasonable. I'll look into multi-tag ownership on IP addresses.

TODO: implement and link commit

[taspelund]

Don't take my thoughts as a mandate by any means. I think it could help when clients have overlapping interest, but I'm not sure how often we would expect that to happen... even in cases where swadm and sled-agent coexist. So if you feel the complexity isn't justified, that's okay too

[cfzimmerman]

I was not excited about the complexity, but it does seem like a useful feature.

I think the reverse of your example is even more pernicious. Say, for example, someone is debugging a system and uses swadm to create a link and put a configured address on it. Then, every time sled-agent tries to reconcile that same address onto the link, its cmd will be rejected completely because that address already exists under a different owner (swadm). sled-agent will not succeed until someone manually deletes that address with swadm and allows sled-agent to register it.

I've been trying to measure complexity against customer/engineering value, and I think that warrants it. Although this affects many of the tests and implies meaningful changes.

[cfzimmerman]

#374 addresses some of the code duplication in this module.

[rcgoodfellow]

My high-level read of this is the following.

1. Internal functions for IP address creation/deletion now take tags.
2. When the port settings apply endpoint calls these functions it uses said tags, so addresses not created by the port settings apply endpoint cannot be deleted by the port settings apply endpoint. This is what fixes port_settings_apply clobbers link-local addresses added via link_ipv6_create #342
3. The dpd API endpoint for IP address creation takes an optional tag, and uses it for creating addresses when supplied. Thus tfportd will also have tagged address objects.
4. The dpd API endpoint for IP address deletion does not take a tag, so no tag can be supplied to the internal IP address deletion function without an API change. This creates the potential for consumers of that API endpoint such as tfportd to delete addresses it did not create pretty easily.

Is that right? I think addressing (4) is worth the API change to make it harder for clients to delete addresses they did not create - that is assuming the tagging mechanism moves forward with the current design. That does not need to be done in this PR, but is worth a follow on issue.

[taspelund]

It might be worth tweaking the method name or the doc string to call out that this is specific to IPv6 just to remove any possibility for confusion.

Perhaps also in a later commit/PR we could consider using oxnet::UnicastLinkLocalIpv6Addr to enforce this at the type level without needing .map()/.find()

[cfzimmerman]

I agree this is problematic. Might be useful to discuss from a design level. Afaict here's the issue:

This function is used in one place to construct a TfportData instance used by tfportd. It returns the first IPv6 unicast link local address on a link. This breaks down immediately if there are two such addresses on a link. That raises the question, should we allow more than one IPv6 unicast link local address on a link? RFC 4291 allows it, and fwiw illumos supports this.

All interfaces are required to have at least one Link-Local unicast
address ... A single interface may also have multiple IPv6 addresses of any type
(unicast, anycast, and multicast) or scope.
RFC 4291

If we allow multiple, then a function returning an Option<Ipv6Addr> is incoherent, and either dpd or tfportd absorbs the complexity of a link possibly having more than one IPv6 unicast link local address. I'm inclined to make tfportd handle this. Its job can be "ensure the link has the IPv6 link local address(es) I want", but it can leave other addresses alone.

TODO: implement and link the commit

[taspelund]

My understanding from (past, fuzzy) conversations with Ry is that we don't intend for operators to manually configure link-local addresses, so there should really only ever be one (which is derived from the link's MAC via SLAAC/EUI-64). It seems preferable to me to enforce that invariant (or at least abstract away the lack of enforcement) in a single place. tfportd seems like a natural place to do that, but I still don't feel as comfortable with the dendrite codebase as I'd like, so I'm not 100% confident that's the best spot.

[rcgoodfellow]

Yes. There should only ever be one link local on an interface and that link local should be EUI-64 derived from the physical MAC address.

[taspelund]

For something like this where None effectively operates as force=true, I think we may be better served by adding a helper enum that effectively spells this out more clearly without relying on comments for clarity around meaning.
e.g.

enum TagSelector {
    Single(String),
    All,
}

[cfzimmerman]

Good point. This Option<String> pattern is used all over these three PRs. I'll transition that to an enum.

TODO: implement and replace stale docs

[taspelund]

This feels like it might be a good use case for proptest.

Not necessarily something that needs to be done in this PR, but something to think about

[cfzimmerman]

Agreed that dpd could use some prop testing, although yes I'm inclined to make that its own project. This was just to avoid hard coded address strings in individual tests, which would presumably be kind of distracting.

[cfzimmerman]

Update: I experimented with proptests on this stack but scrapped that. Multiple places in DPD will benefit from proptesting. I think the port settings API and basically everything involving table ops. Preferred plan is to wait for Zeeshan's multicast proptests and then run a standalone project proptesting the general DPD API.

[taspelund]

Could maybe tuck this behind #[cfg(test)] or rename to TEST_LINK_CREATE so it's obvious we wouldn't want this in prod code (e.g. as an impl Default for LinkCreate`)

[cfzimmerman]

Sgtm. Renamed to TEST_LINK_CONF.

Edit: ... in the branch above. The two have diverged so far at this point, I'm inclined to sunset this PR and include these commits in the parent PR.

[taspelund]

Would it be simpler to use .cloned() here instead of deref/copying addr and converting &str to String each time?

[taspelund]

Same feedback for the other instances below

[cfzimmerman]

I'd prefer to only clone after filter. We could put this work inside filter_map, but rustfmt makes that one more LOC than this.

It's mapping (&IpAddr, &String) to (IpAddr, String), so I don't think cloned is a drop in replacement: https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&gist=e4cada359f5cb2fbd5af2031bf7e8be4

Do you know of a better way to do this?

[taspelund]

I guess not. I was thinking .cloned() would clone the values inside the tuple rather than the tuple itself, but that doesn't seem to be the case

[taspelund]

Overall I'm happy with this PR. I've got some nits here and there, but nothing I'd consider to be a blocker

[nicolaskagami]

One thing I'm concerned about is the change in semantics from the "stealing" to outright rejection in the case of collision with a different existing tag. An alternative would be a "best effort" application of all entries that don't conflict, which would more closely match the current behavior (with the exception of the collided entry).

I've been thinking a lot about the reconciler pattern and this is one of many things it could opine about or at least present good practices for. Tags are a great tool for solving some of these conflict problems and I think we should have a discussion about their use.

[cfzimmerman]

One thing I'm concerned about is the change in semantics from the "stealing" to outright rejection in the case of collision with a different existing tag. An alternative would be a "best effort" application of all entries that don't conflict, which would more closely match the current behavior (with the exception of the collided entry).

I've been thinking a lot about the reconciler pattern and this is one of many things it could opine about or at least present good practices for. Tags are a great tool for solving some of these conflict problems and I think we should have a discussion about their use.

Per Trey's comment, the new design is to reference count tags. So two tags can both declare that they want the same address on an interface, and the address is only deleted from the asic when the final tag deletes it. Thoughts on that?