Add support for IPv6 Virtual DNS (#462) #592
Conversation
network/qubes-setup-dnat-to-ns
Outdated
| if dest is None or (vm_nameserver == dest and len(qubesdb_dns) == 0): | ||
| rules += [ | ||
| f"ip{ip46} daddr {vm_nameserver} tcp dport 53 reject with icmp{ip46} type host-unreachable", | ||
| f"ip{ip46} daddr {vm_nameserver} udp dport 53 reject with icmp{ip46} type host-unreachable", | ||
| ] |
There was a problem hiding this comment.
This logic is new for ipv4. It is carried over from #462. (https://github.com/QubesOS/qubes-core-agent-linux/pull/462/files#r1487256297)
Previous version checked explicitly if /qubes-ip//qubes-netvm-primary-dns6 were specifically defined, rather than checking if any dns servers are defined for the family, as is being done here.
It looks like len(qubesdb_dns) == 0 is always false by this point (due to being inside the outer else) so not confident about the condition being correct here.
There was a problem hiding this comment.
Not clear to me under exactly what conditions host-unreachable should be response. Reverted this whole part until there is clarity.
There was a problem hiding this comment.
See the discussion on the original PR: #462
The issue is if you have only IPv4 or only IPv6 DNS - then you wouldn't have address of such kind. And the reject rule is to avoid long timeouts when trying non-existing DNS and immediately fallback to the other one.
If you have a way to test it, try with:
- only IPv4 DNS present
- only IPv6 DNS present
- both present
In all the cases, name resolution should keep working instantly. The tests I added in core-admin PR try to exercise those cases, but I'm not 100% if it will fail on slow fallback...
There was a problem hiding this comment.
Ah, got it, thanks for the pointer.
Makes me think maybe this part could also be broken out separately... Presumably it wouldn't make much difference for users by itself concerning only IPv4 networking and should help with any debugging to have them as separate commits on main.
I probably won't be in a good place to test properly myself until after couple of weeks. For completion I guess "neither present" should also be explicitly tested (against leaks).
3nprob
force-pushed
the
add-ipv6-dnat-to-dns
branch
from
5c98be5 to
1efe22a
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #592 +/- ##
=======================================
Coverage 71.10% 71.10%
=======================================
Files 3 3
Lines 481 481
=======================================
Hits 342 342
Misses 139 139 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This comment was marked as resolved.
This comment was marked as resolved.
* origin/pr/595: chore(network-agent): consistently memoize ip_address stringification perf(network-agent): use cycle instead of manually doubling nameserver list for zip Pull request description: - I guess the existing memoization of `dns_` is for efficiency reasons? This uses the same pattern for the other `ip_address` instance in the string `vm_nameserver`. If nothing else it makes it more consistent. - Use `itertools.cycle` instead of list-doubling to match up `zip` side lengths. - Broken out from #592
|
Now that the smaller PRs are in (the last one is still going through CI), this will need a rebase (dropping those already merged commits) to resolve conflicts. |
3nprob
force-pushed
the
add-ipv6-dnat-to-dns
branch
from
e145853 to
982add7
Compare
Rebased. Ended up squashing all the existing commits on this branch. (lmk if you prefer to retain history during review in situations like this) |
3nprob
force-pushed
the
add-ipv6-dnat-to-dns
branch
2 times, most recently
from
a89ef8f to
77e4542
Compare
|
openQA run is still in progress, but I already see some failures: This job doesn't have IPv6 enabled. |
OpenQA test summaryComplete test suite and dependencies: https://openqa.qubes-os.org/tests/overview?distri=qubesos&version=4.3&build=2025070723-4.3&flavor=pull-requests Test run included the following:
New failures, excluding unstableCompared to: https://openqa.qubes-os.org/tests/overview?distri=qubesos&version=4.3&build=2025061004-4.3&flavor=update
Failed tests50 failures
Fixed failuresCompared to: https://openqa.qubes-os.org/tests/142375#dependencies 12 fixed
Unstable testsPerformance TestsPerformance degradation:7 performance degradations
Remaining performance tests:65 tests
|
|
Not all of the failures are caused by this PR (or the core-admin one), but it seems most of them are. |
3nprob
force-pushed
the
add-ipv6-dnat-to-dns
branch
from
77e4542 to
7658b78
Compare
Fixed in 7658b78. I guess I would expect this one to have been caught in Also added in a little bit of typings for this here. |
This one is still the case. |
3nprob
force-pushed
the
add-ipv6-dnat-to-dns
branch
from
7658b78 to
33f80a9
Compare
- Merge '1cho1ce/add-ipv6-dnat-to-ns' into master - QubesOS#462 - fix: properly assign primary/secondary DNS - fix: check ipv4 dns presence by qdb /qubes-primary-dns instead of /qubes-ip - qubes-setup-dnat-to-ns: unify ipv4/ipv6 firewall rule generation Part of QubesOS/qubes-issues#10038
3nprob
force-pushed
the
add-ipv6-dnat-to-dns
branch
from
33f80a9 to
c0c7d42
Compare
Fixes QubesOS/qubes-issues#10038