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

Allow announcing extra routes through DHCPv4 #1734

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

gwenya
Copy link
Contributor

@gwenya gwenya commented Mar 7, 2025

This PR adds a new option ipv4.dhcp.static-routes for both bridge and ovn networks. The value of the option is directly passed through to dnsmasq and ovsdb respectively, to be used as DHCP option 121.

@github-actions github-actions bot added the Documentation Documentation needs updating label Mar 7, 2025
@@ -0,0 +1,32 @@
test_network() {

Check failure

Code scanning / shellcheck

SC2148 Error test

Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.
@gwenya gwenya force-pushed the dhcp-static-routes branch 4 times, most recently from 024cd42 to 67a8054 Compare March 7, 2025 15:42
@gwenya
Copy link
Contributor Author

gwenya commented Mar 7, 2025

something is wrong with the tests I wrote but I can't figure out how to run them locally to properly figure out the issue :/

@stgraber
Copy link
Member

stgraber commented Mar 7, 2025

I think it can probably just be ipv4.dhcp.routes, no need for static- in the name

@stgraber
Copy link
Member

stgraber commented Mar 7, 2025

something is wrong with the tests I wrote but I can't figure out how to run them locally to properly figure out the issue :/

That's fine, we don't pay for the Github runners ;)

Just push some debug commits to dump the network config, instance config, full output of the routing table in the container, ...

It may be as simple as the tiny busybox image on the Ubuntu 22.04 runners not supporting option 121.

@gwenya gwenya force-pushed the dhcp-static-routes branch from fdb4a59 to c8dc273 Compare March 7, 2025 21:10
@gwenya
Copy link
Contributor Author

gwenya commented Mar 7, 2025

I think we might not be running a dhcp4 client (or server?) at all in the tests, is that possible? I just did a run with default settings, and not only does the machine not have an ipv4 on its eth0, incus network list-leases shows that there aren't any ipv4 leases.

The only other test that looks at the leases is network.sh, and that one only tests static leases.

I'm not sure if and how it is possible to test this in the github runners, and right now I'm having trouble testing it on my laptop because any bridge networks I create don't get dhcp at all for some reason (works fine on ovn though, so probably a dnsmasq issue)

@gwenya
Copy link
Contributor Author

gwenya commented Mar 7, 2025

oh wait nvm the issue with my bridges locally is just firewall >.<

@gwenya
Copy link
Contributor Author

gwenya commented Mar 7, 2025

looking into udhcpc, it appears that it doesn't configure anything itself, and instead just passes the dhcp information it receives to a script (/usr/share/udhcpc/default.script by default). So even after running udhcpc in the test image, no addresses or routes are installed because there is no script for it.
I'm thinking of either providing a script that just prints out the information this test cares about so that it can be grep'ed, or just try to grep the output of udhcpc.

@gwenya
Copy link
Contributor Author

gwenya commented Mar 8, 2025

I now have the tests with bridge network passing.

OCI get skipped because offline mode, and OVN isn't available on the test runners so I made them skip if the ovn network creation fails (I think we don't have a way of detecting if OVN is available).

@gwenya gwenya force-pushed the dhcp-static-routes branch 6 times, most recently from a29a1b3 to 85917ee Compare March 8, 2025 13:09
@gwenya gwenya marked this pull request as ready for review March 8, 2025 13:46
@gwenya gwenya requested a review from stgraber as a code owner March 8, 2025 13:46
@gwenya
Copy link
Contributor Author

gwenya commented Mar 9, 2025

I think this probably needs an API extension as well, should I just add one?

@gwenya gwenya force-pushed the dhcp-static-routes branch from 85917ee to 722a2d4 Compare March 9, 2025 09:02
@gwenya gwenya marked this pull request as draft March 9, 2025 09:06
@gwenya
Copy link
Contributor Author

gwenya commented Mar 9, 2025

I think there is still a bug with the ovn networks, it seems these routes now override the route to the uplink dns server, and I'm gonna have to dig a bit deeper to figure out what is going on there, and why we are routing the uplink DNS in the first place.

Edit: nvm, we can ignore the route because it is caught by the default route anyways, and it's just added by some dhcp clients for some reason rather than actively being sent. I'm still trying to figure out why the uplink DNS server is used rather than the OVN one though.

@gwenya gwenya force-pushed the dhcp-static-routes branch from 722a2d4 to fddbb2d Compare March 9, 2025 13:26
@gwenya gwenya marked this pull request as ready for review March 9, 2025 13:29
stgraber and others added 2 commits March 9, 2025 23:33
@stgraber stgraber force-pushed the dhcp-static-routes branch from fddbb2d to c2e945f Compare March 10, 2025 03:36
@github-actions github-actions bot added the API Changes to the REST API label Mar 10, 2025
@stgraber stgraber force-pushed the dhcp-static-routes branch from c2e945f to c17870e Compare March 10, 2025 03:37
@stgraber stgraber changed the title static routes option for dhcp Allow announcing extra routes through DHCPv4 Mar 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Changes to the REST API Documentation Documentation needs updating
Development

Successfully merging this pull request may close these issues.

2 participants