add tcp support #484
add tcp support #484
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Luap99 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Unit tests failed. @containers/packit-build please check. |
2 similar comments
|
Unit tests failed. @containers/packit-build please check. |
|
Unit tests failed. @containers/packit-build please check. |
| let udp_sock = match UdpSocket::bind(addr).await { | ||
| Ok(s) => s, | ||
| Err(err) => { | ||
| errors.push(AardvarkError::wrap( | ||
| format!("failed to bind udp listener on {addr}"), | ||
| err.into(), | ||
| )); | ||
| continue; | ||
| } | ||
| }; | ||
|
|
||
| let tcp_sock = match TcpListener::bind(addr).await { | ||
| Ok(s) => s, | ||
| Err(err) => { | ||
| errors.push(AardvarkError::wrap( | ||
| format!("failed to bind tcp listener on {addr}"), | ||
| err.into(), | ||
| )); | ||
| continue; | ||
| } | ||
| }; |
There was a problem hiding this comment.
@mheon here I am not sure what is best here
If we fail to bind udp or tcp we will skip that network and bind neither. I am not sure if it is worth to ignore tcp failures and still listen on udp or the other way around. I feel like this is not worth the extra complexity
WDYT?
There was a problem hiding this comment.
I think skipping the network is reasonable. Honestly, I would kind of prefer to shut down Aardvark altogether if we can't take the appropriate ports?
There was a problem hiding this comment.
Well if it is the first launch then we will exit with the right errors but if we just got reloaded via SIGHUP then we just log the error, at this point it is possible that other networks were able to bind.
Exiting means we bring down everything.
Mostly so we can handle lists see the next commit. Signed-off-by: Paul Holzinger <pholzing@redhat.com>
When we fail to bind the first time we are started we must report this back to the user so they know why dns isn't working. In order to capture all errors we need to make use of the multi error type so we can list all bind failures not just the first one. Also change the port to u16 because a port cannot be higher than that it safes us from making a failable conversion later on in the stack. With that and using the patched netavark for the error handling podman shows now this ``` Error: netavark: IO error: Error while applying dns entries: IO error: aardvark-dns failed to start: Error from child process Error starting server failed to bind udp listener on 10.89.1.1:53: IO error: Address already in use (os error 98) failed to bind udp listener on 10.89.0.1:53: IO error: Address already in use (os error 98) ``` The error message is not exactly nice working so we should fix some of that later but at least we not can show the bind errors correctly. Signed-off-by: Paul Holzinger <pholzing@redhat.com>
|
Integration tests failed. @containers/packit-build please check. |
|
looks like the bind with ncat test is flaky (I need to add some sleep there even if that sucks) |
Make sure the bind error is reported as expected. We have to use sleep as nc might take a moment before the port is bound. Signed-off-by: Paul Holzinger <pholzing@redhat.com>
Aardvark-dns will now listen on tcp as well for incoming dns requests. So far this is only the listing side, forwarding is still done via udp which will be added in a follow up. Signed-off-by: Paul Holzinger <pholzing@redhat.com>
This adds support so we connect to upstream resolvers via tcp as well. The logic follows the mode if client connected via udp we forward via udp, if client used tcp we forward via tcp. This is normal behavior because clients typically try udp first and of the message is to large they retry via tcp again. Fixes https://issues.redhat.com/browse/RHEL-17270 Signed-off-by: Paul Holzinger <pholzing@redhat.com>
Luap99
changed the title
|
Ok pushed another commits to do tcp forwarding as well |
|
/lgtm |
openshift-merge-bot
bot
merged commit 0db33bb
into
containers:main
|
Addresses: https://issues.redhat.com/browse/RHEL-17270 |
see commits
Listen and forward via tcp as well not just udp