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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support matching IPv6 in LocalDnsServer #2386

Merged
merged 26 commits into from Jan 9, 2020
Merged

Conversation

@Mygod
Copy link
Contributor

Mygod commented Dec 13, 2019

This fixes confusions once and for all in like fa3d4e7.

Also updates China IP address blocks.

P.S. I am not sure what is the most authentic source for IP address blocks. If it is not ipverse.net, feel free to change it. 馃槢

This fixes confusions once and for all in like fa3d4e7.

Also updates China IP address blocks.
@Mygod Mygod added the enhancement label Dec 13, 2019
@Mygod Mygod requested a review from madeye Dec 13, 2019
@Mygod

This comment has been minimized.

Copy link
Contributor Author

Mygod commented Dec 13, 2019

P.S. Tested locally and it bypasses baidu.com correctly but not ip.cn. For IPv6, tested ipv6.google.com and ipv6.jmu.edu.cn and works as expected except that Chrome really does not seem to like NAT6 addresses.

@Mygod

This comment has been minimized.

Copy link
Contributor Author

Mygod commented Dec 13, 2019

@madeye We should probably add these updates to bypass-china.acl and bypass-lan-china.acl as well. What do you think if we just extract bypass IPs from ACL file?

@madeye

This comment has been minimized.

Copy link
Contributor

madeye commented Dec 13, 2019

Yeah, extracting bypass IPs from ACL files should be better

@Mygod Mygod force-pushed the Mygod:local-dns-ipv6 branch from d8df34b to da4113c Dec 14, 2019
@Mygod

This comment has been minimized.

Copy link
Contributor Author

Mygod commented Dec 14, 2019

@madeye Done. It seems to be working alright but now start up takes ~1s extra.

@Mygod Mygod force-pushed the Mygod:local-dns-ipv6 branch from da4113c to 55b960f Dec 14, 2019
@Mygod Mygod force-pushed the Mygod:local-dns-ipv6 branch from 9b43066 to cd744cb Dec 14, 2019
@Mygod

This comment has been minimized.

Copy link
Contributor Author

Mygod commented Dec 14, 2019

Hostname matching in LocalDnsServer might also add ~80ms of delay per DNS request. Unfortunately the pattern matching also creates a lot of garbage objects for every DNS request.

@madeye

This comment has been minimized.

Copy link
Contributor

madeye commented Dec 14, 2019

Is it possible that we define a special section, e.g. [china_ip_range] in ACL? If so we can still use the raw ip range?

It should help to improve the overall perf.

@madeye
madeye approved these changes Dec 14, 2019
Copy link
Contributor

madeye left a comment

Generally, I don't think the overhead of local DNS resolver here really matters. 80ms should be much smaller than a normal TCP query.

So, LGTM.

@Mygod

This comment has been minimized.

Copy link
Contributor Author

Mygod commented Dec 14, 2019

Oh I meant that I also implemented the full hostname matching in LocalDnsServer which was not there before. The worse part about this is a lot of Matcher objects are being created so the garbage collector will kick in very frequently if there are many DNS queries which could cause battery drain. If that part is removed the performance should be pretty good.

For the start up performance hit, profiling shows that the ~1 second comes mainly from SortedList in Acl.

@madeye

This comment has been minimized.

Copy link
Contributor

madeye commented Dec 14, 2019

I see.

BTW, can we try some matcher like https://github.com/google/re2j and compile all rules into one a|b|c|d...? Then we can keep only one matcher during the whole lifetime of a VPN session.

@Mygod

This comment has been minimized.

Copy link
Contributor Author

Mygod commented Dec 14, 2019

That might work but we need to remove almost all groups in our regex, otherwise it is still going to use a lot of memory. Currently almost every rule has (^|\.) at the beginning.

Also we probably also need to optimize Acl.

@madeye

This comment has been minimized.

Copy link
Contributor

madeye commented Dec 14, 2019

Let's make the improvement a feature work.

IMO, for domain ACL, wildcard like rules should work for almost all the cases.

@Mygod

This comment has been minimized.

Copy link
Contributor Author

Mygod commented Dec 14, 2019

Good news: now hostname matching takes <1ms.
Bad news: Initialization takes 3s. :)

EDIT: Sorry apparently matching causes stack overflow.

Mygod added 2 commits Dec 14, 2019
@Mygod

This comment has been minimized.

Copy link
Contributor Author

Mygod commented Dec 14, 2019

If we only allow googleapi rules as before, this change causes start up to be only 16x slower. (reading acl is taking the majority of the time) We can mitigate start up slowdown by putting it into async.

@Mygod

This comment has been minimized.

Copy link
Contributor Author

Mygod commented Dec 14, 2019

Did some more testing. On average, ACL set up time is ~100ms (~400ms for full ACL), hostname lookup is <1ms (~50ms if we use full ACL), IPv4 subnet lookup 2ms and IPv6 subnet lookup <1ms subnet lookup is now negligible with binary search with ~300ms preprocessing. While these time is negligible in terms of user experience, full ACL lookup can accumulate into battery drain since it is ~50ms of CPU time for every query.

@madeye

This comment has been minimized.

Copy link
Contributor

madeye commented Dec 14, 2019

Cool!

@Mygod Mygod mentioned this pull request Dec 18, 2019
Mygod added 5 commits Dec 18, 2019
8M on <1GB RAM devices, 64M otherwise.
Use re2
@madeye

This comment has been minimized.

Copy link
Contributor

madeye commented Jan 2, 2020

I guess we can merge this now? @Mygod

@Mygod

This comment has been minimized.

Copy link
Contributor Author

Mygod commented Jan 2, 2020

@madeye

This comment has been minimized.

Copy link
Contributor

madeye commented Jan 2, 2020

No hurry...

@Mygod Mygod merged commit ce361da into shadowsocks:master Jan 9, 2020
1 of 2 checks passed
1 of 2 checks passed
Codacy/PR Quality Review Not up to standards. This pull request quality could be better.
Details
ci/circleci Your tests passed on CircleCI!
Details
@Mygod Mygod deleted the Mygod:local-dns-ipv6 branch Jan 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can鈥檛 perform that action at this time.