Validate transit IPs on network interface update #7559
Conversation
This PR adds in some checks when setting transit IPs on a NIC to ensure that we have: * Only unicast addresses. * No loopback addresses. * No duplicates. As discussed in the ticket, violation of any of these isn't going to leave OPTE or CRDB in a broken state -- just a confusing one for end users. Closes #7530.
| @@ -213,3 +214,64 @@ impl super::Nexus { | |||
| } | |||
| } | |||
| } | |||
|
|
|||
| fn validate_transit_ips(ips: &[IpNet]) -> Result<(), Error> { | |||
There was a problem hiding this comment.
I don't know the omicron codebase very well, but most of the logic here (checking for a valid unicast prefix + host bits being unset) seems like it would be useful in other places -- even maybe in maghemite.
Would it maybe make more sense to move this into a couple oxnet helper functions and use validate_transit_ips as a wrapper with context specific logic? (like checking if all the prefixes are the same AF)
e.g.
I could see both an IpNet.valid_unicast() and some kind of prefixes_overlapping() fn being useful in other places.
|
Just one question about potentially making this logic more reusable, but it's not a blocker IMO. LGTM! |
Thanks -- since this isn't time critical, we'll see what we can upstream into oxnet first then. Overlap/contains would probably be uncontroversial, I think this definition of valid unicast isn't overly restrictive either but it might just be application-dependent. |
This PR adds in some checks when setting transit IPs on a NIC to ensure that we have:
As discussed in the ticket, violation of any of these isn't going to leave OPTE or CRDB in a broken state -- just a confusing one for end users. This should make things unambiguous in terms of what how the transit IPs will be handled.
Closes #7530.