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.

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

Add vrf compatibility #562

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

MrSuicideParrot
Copy link

Netavark version v1.8.0 added support for bridges inside VRFs.

Currently, aardvark-dns doesn't work on bridges with a configured VRF. Aardvark-dns attempts to bind to the bidge's IP. However, as the IP is assigned to a VRF, it fails to bind. For a program to be able to bind to a VRF, it must first bind the socket to the VRF where the IP exists. This is done using the SO_BINDTODEVICE socket option.

This pull request adds this capability: before binding to an IP address, it checks if the bridge interface is assigned to a VRF, and if so, it binds the socket to the VRF before attempting to bind the socket to the IP address.

Unfortunately, this feature significantly complicates the logic of binding the DNS server and forces us to use lower-level APIs to set this option. Moreover, to check if a bridge is set to a VRF, I used methods from netavark, thus increasing the number of dependencies of this project.

For these reasons, I don't know if there is interest in merging this pull request, but I did it for at least if anyone encounters this problem and wants it solved, there is already a solution.

Copy link
Contributor

openshift-ci bot commented Jan 19, 2025

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: MrSuicideParrot
Once this PR has been reviewed and has the lgtm label, please assign luap99 for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Copy link

Ephemeral COPR build failed. @containers/packit-build please check.

Add vrf support

Signed-off-by: André Cirne <[email protected]>
Copy link
Member

@Luap99 Luap99 left a comment

Choose a reason for hiding this comment

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

This seems backwards, netavark has already all the info we need so there should be no need at all to lookup any addresses via netlink to figure out if there is a vrf.

I have not looked at how vrf works here and if we really must bind the device instead but if extra info is needed then they must be part of the aardvark-dns config file we read instead.

@@ -35,6 +35,9 @@ nix = { version = "0.29.0", features = ["fs", "signal", "net"] }
libc = "0.2.168"
arc-swap = "1.7.1"
flume = "0.11.1"
netavark = "1.13.1"
Copy link
Member

Choose a reason for hiding this comment

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

We cannot do this here. This dependency would make it very hard for us to cut new releases.


match vrf_index {
Some(vrf_index) => {
let vrf_link_msg = host.get_link(netlink::LinkID::ID(vrf_index)).unwrap();
Copy link
Member

Choose a reason for hiding this comment

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

in general using unwrap() is almost never acceptable, error must be handled properly

@MrSuicideParrot
Copy link
Author

I have tried to bind to the vrf without this option but I haven't had success. There is a tunable kernel option that enables programs to bind to any vrf, but this is only applicable when a program bind to all interfaces (0.0.0.0) and not a single IP as aardvark-dns does.

I understand the problem of adding the netavark as a depency. I'll investigate
how can I add the vrf information to the config file and remove this dependency.

@Luap99
Copy link
Member

Luap99 commented Jan 21, 2025

you need to extend the config file format https://github.com/containers/aardvark-dns/blob/main/config.md#first-line

I guess we could just add the device name on the first line. Then have netavark set this to the vrf name and call bind to device.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants