re-implement #654 onto latest main#698
Conversation
| // Use bind_addr from LogicalRouter if specified, otherwise use listen_addr | ||
| // Use bind_addr IP from LogicalRouter (port 0 so OS picks ephemeral), | ||
| // otherwise use listen_addr IP. The source port for outbound connections | ||
| // must not conflict with the dispatcher's listen port. | ||
| let bind_addr = logical_router | ||
| .bind_addr | ||
| .unwrap_or(logical_router.listen_addr); | ||
| .map(|addr| SocketAddr::new(addr.ip(), 0)) | ||
| .unwrap_or_else(|| { | ||
| SocketAddr::new(logical_router.listen_addr.ip(), 0) | ||
| }); |
There was a problem hiding this comment.
note: hard-coding of source port to 0
Do you anticipate the src_port ever being pinned up in tests for your uses of this?
There was a problem hiding this comment.
Do you anticipate the src_port ever being pinned up in tests for your uses of this?
At this point, I don't know. If we were good at anticipating what we'd need in our test suite I wouldn't be writing this PR 😂
A lot of the work that I've done in maghemite over the past few months is adding more flexibility so that it can be used upstream in omicron for testing, so exposing the reality that we're using a SocketAddr here to omicron so it can tweak it as necessary in tests seemed like a reasonable thing to do.
mgd/src/admin.rs
Outdated
| ) -> Result<HttpResponseUpdatedNoContent, HttpError> { | ||
| bgp_admin::create_neighbor(ctx, request.map(Into::into)).await | ||
| } | ||
|
|
There was a problem hiding this comment.
It seems like most of these could be impls provided in the trait definition, i.e. filling in the body in mg-api/src/lib.rs. Not a big deal, but I think would help keep things a little cleaner
| // new stuff after v6 (VERSION_BGP_SRC_ADDR) | ||
| /// Source IP address to bind when establishing outbound TCP connections. | ||
| /// None means the system selects the source address. | ||
| pub src_addr: Option<IpAddr>, |
There was a problem hiding this comment.
Can you add some address-family validation for this into Neighbor.validate_address_families()? That's a nice centralized spot where we can return an error if src_addr isn't the same IP version as Neighbor.host.ip()
There was a problem hiding this comment.
If you're feeling really ambitious, you could add the same validation method to struct UnnumberedNeighbor too, as I noticed we don't have the same validation in place there (I think I have a commit addressing this in #648, but that will likely take a while to get merged).
| pub src_addr: Option<IpAddr>, | ||
| /// Source TCP port to bind when establishing outbound TCP connections. | ||
| #[serde(default)] | ||
| pub src_port: Option<u16>, |
There was a problem hiding this comment.
Would this be better suited as an Option<NonZeroU16>? i.e. what's the difference between None and Some(0)?
There was a problem hiding this comment.
Maybe. It could be reasonable to consider both None and Some(0) as semantically correct:
- If you are creating a
SocketAddrand don't specify a port, it is generally understood that the system will try to pick a random port - If you are creating a
SocketAddrand specify port0, it is generally understood that the system will try to pick a random port - If you are creating a
SocketAddrand specify something other than port0, it is understood that they system should try to bind to the requested port
On the other hand, is there an actual unsafe or unpredictable condition that we avoid by forcing the port to be NonZero if provided?
As the code sits, a caller could just yank whatever ip_address and port that was provided to them and pass it over and get the expected behavior.
If they don't have a port, they can pass None and get the expected behavior.
If we force NonZero, now the caller has to do an additional check before sending over the value, just to get the same outcome as if they passed Some(variable_that_had_zero_as_its_value). In maghemite's code we'd still have to set the port to 0 somewhere in the event that they provide None, so I don't know if we gain anything by forcing the extra check.
Ultimately this is all because we're splitting up the parameters to what is ultimately a SocketAddr under the hood and then exposing those parameters in the API. If we really wanted to simplify it, we could probably have the caller provide a SocketAddr as the src_addr.
There was a problem hiding this comment.
I was mainly pointing this out to show that we would get semantically equivalent outcomes with Option<NonZeroU16> and u16 -- both providing values that equate to 0 -- whereas Option<u16> provides two different values that equate to 0 (Some(0) and None being semantically equivalent).
I don't have super strong opinions here, so I'm happy to accept whatever you think makes the most sense
| // Validate that at least one AF is enabled | ||
| rq.validate_address_families() | ||
| .map_err(Error::InvalidRequest)?; | ||
|
|
taspelund
left a comment
taspelund
left a comment
There was a problem hiding this comment.
Thanks for the last few changes levon, ship it!
internet-diglett
deleted the
levon/make-bgp-dispatch-listen-configurable-v2
branch
2 participants
Re-implements #654 because it was kinda hairy solving the merge conflicts