Add new NetIf variant to NatResolver for IP resolution via network interface #10922
Conversation
crates/net/nat/src/lib.rs
Outdated
| @@ -185,6 +189,7 @@ pub async fn external_addr_with(resolver: NatResolver) -> Option<IpAddr> { | |||
| match resolver { | |||
| NatResolver::Any | NatResolver::Upnp | NatResolver::PublicIp => resolve_external_ip().await, | |||
| NatResolver::ExternalIp(ip) => Some(ip), | |||
| NatResolver::NetIf => resolve_net_if_ip(DEFAULT_NET_IF_NAME).ok(), | |||
There was a problem hiding this comment.
@emhane I've decided to return None here if resolution fails given IP resolution here is touted as best effort similarly to how both resolve_external_ip() above behaves in that there is no Err if that fails.
Don't think we should be changing the signature of this method to Result<Option<IpAddr>, NetInterfaceError> since that would leak resolver specific impl detail to consumers. LMKWYT
There was a problem hiding this comment.
defo think sig should change. becomes hard to use this tool otherwise if error isn't printed to log. the error could be logged here too, but it's wrong to use thereth::cli target here, and users should be able to read the error without enabling target reth::net. @shekhirin will error log level be written to stdout regardless of target in default node logging setting?
There was a problem hiding this comment.
Yeah think I'd much rather log an error here or lower, than force consumers of external_addr_with to handle a NetInterfaceError even if their NatResolver impl is something unrelated WDYT
Similar reasoning to how resolve_external_ip() discards the error, would be nice to have it log an error instead
There was a problem hiding this comment.
ye, would like it to panic already in cli as well if the error is logged, so if none is returned when the net if ip resolution flag is passed
please open an issue for resolve_external_ip() not logging error
Co-authored-by: Emilia Hane <[email protected]>
crates/net/nat/src/lib.rs
Outdated
| @@ -48,6 +49,9 @@ pub enum NatResolver { | |||
| PublicIp, | |||
| /// Use the given [`IpAddr`] | |||
| ExternalIp(IpAddr), | |||
| /// Resolve external IP via the network interface. | |||
| #[cfg(not(target_os = "windows"))] | |||
There was a problem hiding this comment.
I'd like to not feature gate this variant
There was a problem hiding this comment.
There was a problem hiding this comment.
https://docs.rs/if-addrs/0.13.3/x86_64-pc-windows-msvc/if_addrs/fn.get_if_addrs.html
this also exists for windows,
…h into garwah-add-netif-natresolver
| error!("Failed to resolve network interface IP: {}", err); | ||
| None |
There was a problem hiding this comment.
@emhane Alternatively we could panic!(....) instead here if I'm understanding #10922 (comment) correctly. Have decided to just log an error here but LMKWYT
There was a problem hiding this comment.
this should just return None if this fails
There was a problem hiding this comment.
Yes this is the behavior that I had originally #10922 (comment)
There was a problem hiding this comment.
Do we want to log an error if this fails?
doesn't hurt but not strictly required
There was a problem hiding this comment.
Right so does the above current diff SGTY? We log an error and return None on failure. Just checking your comment here #10922 (comment) wasn't referring to something else
| error!("Failed to resolve network interface IP: {}", err); | ||
| None |
There was a problem hiding this comment.
this should just return None if this fails
There was a problem hiding this comment.
what's missing is another arm here:
reth/crates/net/nat/src/lib.rs
Line 93 in 03e89b1
Closes #10832