ipset netlink rewrite #2179
ipset netlink rewrite #2179
Conversation
In the process, found a segfault from assuming the Res field of proxyCtx is non-nil. I added a defensive check as I'm not sure if it's guaranteed to be non-nil in regular execution in this context.
This matches dnsmasq behavior and the alternative is not really useful. See http://thekelleys.org.uk/gitweb/?p=dnsmasq.git;a=blob;f=src/forward.c;hb=f60fea1fb0a288011f57a25dfb653b8f6f8b46b9#l588
Now also checks the root domain allowing catchall ipsets and fixes a bug that overwrote bound ipsets for a domain if the domain was in multiple config lines.
This is useful for negating more general domain rules. For example, example.com/MostOfExample host.example.com/ would put all subdomains of example.com in the ipset MostOfExample except for resolutions at and below host.example.com.
Resolves read/write and write/write races on the cache maps present since feature introduction.
Codecov Report
@@ Coverage Diff @@
## master #2179 +/- ##
==========================================
- Coverage 34.19% 33.68% -0.52%
==========================================
Files 73 74 +1
Lines 8978 9111 +133
==========================================
- Hits 3070 3069 -1
- Misses 5511 5646 +135
+ Partials 397 396 -1
Continue to review full report at Codecov.
|
Hm, I will add this to Wiki then.
We'll see how Travis tests go. In theory, ipset functionality was not supposed to work outside of Linux, we just need to exclude it properly. |
dsheets
force-pushed
the
ipset-netlink
branch
from
5e489fb to
8674f02
Compare
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
This is a squash of all commits in #2179.
|
Apologies for the long wait. We've rebased your changes and mostly checked them in. We'll readapt your integration test in a following change, probably sometime during the v0.106.0 cycle. |
Merge in DNS/adguard-home from 2179-ipset-subdomains to master Closes AdguardTeam#2179. Squashed commit of the following: commit de17caa Author: Ainar Garipov <A.Garipov@AdGuard.COM> Date: Fri Jan 29 18:34:46 2021 +0300 dnsforward: imp code, docs commit e5ab957 Author: Ainar Garipov <A.Garipov@AdGuard.COM> Date: Tue Jan 26 20:43:31 2021 +0300 dnsforward: imp code commit 2b84d27 Author: David Sheets <sheets@alum.mit.edu> Date: Tue Oct 6 16:34:06 2020 +0100 dnsforward: support subdomain matching in ipset This is a squash of all commits in AdguardTeam#2179.
This PR supersedes #2165 and implements the proposed rewrite of the ipset functionality on top of netlink sockets.
This change improves 'difficult' (10 addresses to be inserted) request performance by 50x, removes the need for root permissions to invoke the
ipsetexecutable (replacing it with a new requirement forCAP_NET_ADMIN), quiets the logspam from ipv4/ipv6 ipset mismatches, fixes a couple of semantics issues, and fixes a crashing race condition. Additionally, this changeset paves the way for some exciting future ipset-based features and, as far as I know, makes AGH the best choice for personal DNS-based firewalls.Unfortunately, nothing is free. I've introduced some new dependencies, made the ipset tests harder to run, and I don't know what the cross-platform story is:
New dependencies mdlayher/netlink, ti-mo/netfilter, and digineo/go-ipset are all necessary for this functionality. vishvananda/netns has been added to the tests in order to run the ipset creation/query/addition/destruction operations in a network namespace to keep the root network namespace clean. As creating a network namespace requires root permissions, I'm not sure this is worth the extra hassle and I'm considering removing this functionality.
Integration tests I converted all of the ipset tests to integration tests now as they do I/O during initialization to check whether the referenced ipsets exist and what their properties are. Various things could be done to unintegrate the tests so that at least some of them can be run without
CAP_NET_ADMINand without risking leaking test ipsets into the user's ipset namespace. I don't really know what to do here as I don't know what your test infrastructure looks like. I think at the least there will need to be some kind of privileged test run in CI that uses the appropriate build flags to pick up the tests.Cross-platform I have no idea... I only have access to Linux.
Please let me know how you'd like to proceed. I'm happy to do some more work to get this accepted. I haven't tested this in 'production' yet but that will probably happen in the next couple days.
Thanks!