-
Notifications
You must be signed in to change notification settings - Fork 11
Fix DNS resolution in ephemeral guests #167
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
Conversation
d5e8558 to
f693004
Compare
cgwalters
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with this overall if you are; a few nits.
That said, an integration test (run as part of just test-integration ephemeral) would be both easy and IMO mandatory for changes like this.
542253f to
8b859ad
Compare
dcc3ad4 to
9996db4
Compare
|
|
9996db4 to
5e28fd5
Compare
Ah yes that relates to #22 - basically all of |
cgwalters
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One nit otherwise looks sane (though I'd reiterate we probably really do want a bigger fix of --net=host per the issue, but this helps for now)
5e28fd5 to
57af585
Compare
57af585 to
eb89500
Compare
|
Reworking this PR as I think it will require a few more changes. |
5cf80aa to
3bf0686
Compare
3bf0686 to
0cd76fc
Compare
|
This one's just timing out on test_to_disk_for_image_quay_io_centos_bootc_centos_bootc_stream9 test but should be good to go though |
0cd76fc to
998adc2
Compare
QEMU's slirp reads /etc/resolv.conf from the container namespace, which contains unreachable bridge DNS servers. On systemd-resolved hosts, it only has 127.0.0.53 (stub resolver). Read upstream DNS servers from host's /run/systemd/resolve/resolv.conf, pass them to the container, and write /etc/resolv.conf before starting QEMU. Signed-off-by: gursewak1997 <gursmangat@gmail.com>
998adc2 to
b91195d
Compare
|
So I get this from every run now:
Looks like what's happening is when I'm on my work VPN the DNS is switched over to 10.x which matches In general I would say we probably want to get sooner rather than later out of the business of parsing DNS at all and ensuring that libvirt is handling networking consistently. |
PR #167 introduced DNS filtering that excluded all private IP addresses (10.x, 172.16-31.x, 192.168.x, fc00::/7) assuming they would be unreachable from QEMU's slirp networking. However, this breaks VPN scenarios where private DNS servers are actually reachable. This change removes the overly aggressive private IP filtering, now only filtering out localhost and link-local addresses. Private network DNS servers are allowed through since they may be reachable (e.g., via VPN or air-gapped networks). If they're actually unreachable, DNS will fail naturally, which is better than prematurely filtering them out. Also downgraded the fallback warning from WARN to debug level since falling back to public DNS is a normal case, not an error condition. Assisted-by: Claude Code (Sonnet 4.5) Signed-off-by: gursewak1997 <gursmangat@gmail.com>
A fix for now: #182 |
PR #167 introduced DNS filtering that excluded all private IP addresses (10.x, 172.16-31.x, 192.168.x, fc00::/7) assuming they would be unreachable from QEMU's slirp networking. However, this breaks VPN scenarios where private DNS servers are actually reachable. This change removes the overly aggressive private IP filtering, now only filtering out localhost and link-local addresses. Private network DNS servers are allowed through since they may be reachable (e.g., via VPN or air-gapped networks). If they're actually unreachable, DNS will fail naturally, which is better than prematurely filtering them out. Also downgraded the fallback warning from WARN to debug level since falling back to public DNS is a normal case, not an error condition. Assisted-by: Claude Code (Sonnet 4.5) Signed-off-by: gursewak1997 <gursmangat@gmail.com>
PR #167 introduced DNS filtering that excluded all private IP addresses (10.x, 172.16-31.x, 192.168.x, fc00::/7) assuming they would be unreachable from QEMU's slirp networking. However, this breaks VPN scenarios where private DNS servers are actually reachable. This change removes the overly aggressive private IP filtering, now only filtering out localhost and link-local addresses. Private network DNS servers are allowed through since they may be reachable (e.g., via VPN or air-gapped networks). If they're actually unreachable, DNS will fail naturally, which is better than prematurely filtering them out. Also downgraded the fallback warning from WARN to debug level since falling back to public DNS is a normal case, not an error condition. Assisted-by: Claude Code (Sonnet 4.5) Signed-off-by: gursewak1997 <gursmangat@gmail.com>
PR #167 introduced DNS filtering that excluded all private IP addresses (10.x, 172.16-31.x, 192.168.x, fc00::/7) assuming they would be unreachable from QEMU's slirp networking. However, this breaks VPN scenarios where private DNS servers are actually reachable. This change removes the overly aggressive private IP filtering, now only filtering out localhost and link-local addresses. Private network DNS servers are allowed through since they may be reachable (e.g., via VPN or air-gapped networks). If they're actually unreachable, DNS will fail naturally, which is better than prematurely filtering them out. Also downgraded the fallback warning from WARN to debug level since falling back to public DNS is a normal case, not an error condition. Assisted-by: Claude Code (Sonnet 4.5) Signed-off-by: gursewak1997 <gursmangat@gmail.com>
PR #167 introduced DNS filtering that excluded all private IP addresses (10.x, 172.16-31.x, 192.168.x, fc00::/7) assuming they would be unreachable from QEMU's slirp networking. However, this breaks VPN scenarios where private DNS servers are actually reachable. This change removes the overly aggressive private IP filtering, now only filtering out localhost and link-local addresses. Private network DNS servers are allowed through since they may be reachable (e.g., via VPN or air-gapped networks). If they're actually unreachable, DNS will fail naturally, which is better than prematurely filtering them out. Also downgraded the fallback warning from WARN to debug level since falling back to public DNS is a normal case, not an error condition. Assisted-by: Claude Code (Sonnet 4.5) Signed-off-by: gursewak1997 <gursmangat@gmail.com>
PR #167 introduced DNS filtering that excluded all private IP addresses (10.x, 172.16-31.x, 192.168.x, fc00::/7) assuming they would be unreachable from QEMU's slirp networking. However, this breaks VPN scenarios where private DNS servers are actually reachable. This change removes the overly aggressive private IP filtering, now only filtering out localhost and link-local addresses. Private network DNS servers are allowed through since they may be reachable (e.g., via VPN or air-gapped networks). If they're actually unreachable, DNS will fail naturally, which is better than prematurely filtering them out. Also downgraded the fallback warning from WARN to debug level since falling back to public DNS is a normal case, not an error condition. Assisted-by: Claude Code (Sonnet 4.5) Signed-off-by: gursewak1997 <gursmangat@gmail.com>
cgwalters
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assisted-by: OpenCode (Claude Sonnet 4)
AI-generated review based on REVIEW.md guidelines.
Comments prefixed with "AI:" are unedited AI output.
Post-merge cross-check review
The key issue (filtering private network DNS servers) was already identified and fixed in follow-up commit 8f8d301. Overall this is a well-structured fix with good documentation and testing.
|
|
||
| // Filter out localhost, link-local, and private network addresses | ||
| // QEMU runs in user networking mode (slirp) inside a container, which cannot | ||
| // reach private network addresses (10.x.x.x, 172.16-31.x.x, 192.168.x.x for IPv4, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AI: Important: The filtering of private network addresses (!ip.is_private() for IPv4, ULA check for IPv6) will break DNS for many enterprise/VPN users whose DNS servers are on private networks. Consider only filtering loopback and link-local. (Note: This was fixed in follow-up commit 8f8d301)
| "-c", | ||
| r#" | ||
| if command -v curl >/dev/null 2>&1; then | ||
| curl -sS --max-time 10 https://quay.io/v2/ >/dev/null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AI: (low) This test requires external network access to quay.io, which may fail in air-gapped CI environments or during quay.io outages. Consider documenting this as an integration test requirement, or adding a skip mechanism for offline testing.
| pub host_dns_servers: Option<Vec<String>>, | ||
| } | ||
|
|
||
| /// Parse DNS servers from resolv.conf format content |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AI: Good structure - having parse_resolv_conf take &str makes it unit-testable without filesystem dependencies. Consider adding unit tests for edge cases (empty file, malformed lines, mixed IPv4/IPv6).
|
⬆️ was a test case for bootc-dev/infra#64 |
|
I like how it pointed out an issue and also mentioned |
Configure QEMU user-mode networking to use host DNS servers from /etc/resolv.conf instead of the default 10.0.2.3, which doesn't work when QEMU runs inside containers.