Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add bind_addr for udp stream #2191

Closed
wants to merge 3 commits into from
Closed

Add bind_addr for udp stream #2191

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
026fa65
Add bind_addr for udp stream
hainesc Apr 22, 2024
8218469
Add bind_addr for udp stream
hainesc Apr 22, 2024
12956a4
Fix codeliness
hainesc Apr 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
12 changes: 11 additions & 1 deletion crates/proto/src/udp/udp_client_stream.rs
View file
Open in desktop
Expand Up @@ -36,6 +36,7 @@ where
MF: MessageFinalizer,
{
name_server: SocketAddr,
bind_addr: Option<SocketAddr>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on the discussion in #2190, this should probably be IPaddr instead. I do think we should still consider how we want to deal with IPv4 vs IPv6...

timeout: Duration,
is_shutdown: bool,
signer: Option<Arc<MF>>,
Expand Down Expand Up @@ -100,6 +101,7 @@ impl<S: UdpSocket + Send + 'static, MF: MessageFinalizer> UdpClientStream<S, MF>
) -> UdpClientConnect<S, MF> {
UdpClientConnect {
name_server,
bind_addr: None,
timeout,
signer,
creator: Arc::new(|local_addr: _, server_addr: _| {
Expand Down Expand Up @@ -127,6 +129,7 @@ impl<S: UdpSocket + Send + 'static, MF: MessageFinalizer> UdpClientStream<S, MF>
) -> UdpClientConnect<S, MF> {
UdpClientConnect {
name_server,
bind_addr,
timeout,
signer,
creator: Arc::new(move |local_addr: _, server_addr: _| {
Expand All @@ -151,12 +154,14 @@ impl<S: DnsUdpSocket + Send, MF: MessageFinalizer> UdpClientStream<S, MF> {
/// * `creator` - function that binds a local address to a newly created UDP socket
pub fn with_creator(
name_server: SocketAddr,
bind_addr: Option<SocketAddr>,
signer: Option<Arc<MF>>,
timeout: Duration,
creator: UdpCreator<S>,
) -> UdpClientConnect<S, MF> {
UdpClientConnect {
name_server,
bind_addr,
timeout,
signer,
creator,
Expand Down Expand Up @@ -233,11 +238,14 @@ impl<S: DnsUdpSocket + Send + 'static, MF: MessageFinalizer> DnsRequestSender
);
let creator = self.creator.clone();
let addr = message.addr();
let bind_addr = self.bind_addr;

S::Time::timeout::<Pin<Box<dyn Future<Output = Result<DnsResponse, ProtoError>> + Send>>>(
self.timeout,
Box::pin(async move {
let socket: S = NextRandomUdpSocket::new_with_closure(&addr, creator).await?;
let socket: S =
NextRandomUdpSocket::new_with_bind_addr_and_closure(&addr, &bind_addr, creator)
.await?;
send_serial_message_inner(message, message_id, verifier, socket, recv_buf_size)
.await
}),
Expand Down Expand Up @@ -275,6 +283,7 @@ where
MF: MessageFinalizer,
{
name_server: SocketAddr,
bind_addr: Option<SocketAddr>,
timeout: Duration,
signer: Option<Arc<MF>>,
creator: UdpCreator<S>,
Expand All @@ -288,6 +297,7 @@ impl<S: Send + Unpin, MF: MessageFinalizer> Future for UdpClientConnect<S, MF> {
// TODO: this doesn't need to be a future?
Poll::Ready(Ok(UdpClientStream::<S, MF> {
name_server: self.name_server,
bind_addr: self.bind_addr,
is_shutdown: false,
timeout: self.timeout,
signer: self.signer.take(),
Expand Down
23 changes: 18 additions & 5 deletions crates/proto/src/udp/udp_stream.rs
View file
Open in desktop
Expand Up @@ -264,12 +264,25 @@ impl<S: UdpSocket + 'static> NextRandomUdpSocket<S> {

impl<S: DnsUdpSocket> NextRandomUdpSocket<S> {
/// Create a future with generator
#[allow(dead_code)]
pub(crate) fn new_with_closure(name_server: &SocketAddr, func: UdpCreator<S>) -> Self {
let bind_address = match *name_server {
SocketAddr::V4(..) => SocketAddr::new(IpAddr::V4(Ipv4Addr::new(0, 0, 0, 0)), 0),
SocketAddr::V6(..) => {
SocketAddr::new(IpAddr::V6(Ipv6Addr::new(0, 0, 0, 0, 0, 0, 0, 0)), 0)
}
Self::new_with_bind_addr_and_closure(name_server, &None, func)
}

/// Create a future with generator
pub(crate) fn new_with_bind_addr_and_closure(
name_server: &SocketAddr,
bind_addr: &Option<SocketAddr>,
func: UdpCreator<S>,
) -> Self {
let bind_address = match bind_addr {
Some(ba) => *ba,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally, I prefer more expressive names to shortening them like this, Some(bind_addr) => *bind_addr, is more readable IMO.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the logic below, do we need to check that the bind_addr matches the family of the name_server? That is, if this is a v4 address, but the name_server is a v6 address, than this UDP socket will either fail, or never get a response, maybe it will fail quickly with a "no route to host", though I'm not sure off the top of my head.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, I will take a look what other(such as std lib) do on this problem.

None => match *name_server {
SocketAddr::V4(..) => SocketAddr::new(IpAddr::V4(Ipv4Addr::new(0, 0, 0, 0)), 0),
SocketAddr::V6(..) => {
SocketAddr::new(IpAddr::V6(Ipv6Addr::new(0, 0, 0, 0, 0, 0, 0, 0)), 0)
}
},
};
Self {
name_server: *name_server,
Expand Down
1 change: 1 addition & 0 deletions crates/resolver/src/name_server/connection_provider.rs
View file
Open in desktop
Expand Up @@ -284,6 +284,7 @@ impl<P: RuntimeProvider> ConnectionProvider for GenericConnector<P> {
};
let stream = UdpClientStream::with_creator(
config.socket_addr,
config.bind_addr,
None,
options.timeout,
Arc::new(closure),
Expand Down