Fix missing Allow LAN access toggle

[Pawloland]

Fix missing Allow LAN access toggle when exit node is activated for all devices older than android 13 - API 33.
Instead of relying on android SDK, manually calculate required ranges.
Fixes tailscale/tailscale#16187

[Pawloland]

@kari-ts @barnstar @nickoneill @bradfitz,
Hello, obligatory sorry for ping! It's been a while, and I am not sure if the issuebot is holding my PR back from your review. I linked the issue I found and fixed it. Could you review it? Thanks :)

[kari-ts]

Some initial thoughts:
-can we still use ExcludeRoute where supported?
-can we add some sort of guardrail to prevent route explosion (maybe something like, log the number of routes and don't proceed if the computed route set is too big)
-can we add tests? (I know this is a little hypocritical given the embarrassingly sparse test coverage, but we are trying to be better about this!)

[Pawloland]

It is possible to use exclude route method for supported android versions, but since it doesn't work for all of them supported by tailscale app, and manual approach does work for all, I thought it is better to stop using it everywhere, to avoid confusion.

Regarding route explosion, I am not sure what it means. Do you mean having too many of addresses printed out in logcat, instead of just the count of them? I suppose I can remove logging them, but it was usefull when comparing with the tool I mentioned in linked issue.
If you instead meant, preventing the set of routes being applied, when the count of them in a set is too big, then I would say no. It would undermine the whole reason why they are calculated in the first place, which is to fit into the old interface of android. If only some of them were to be applied then the behavior would be different, than it is now on API 33+, and if none of them were to be applied, then it would simply do nothing.

The algorithm for calculating the CIDR ranges tries to create minimal number of them, with smallest possible masks, to cover the whole allowed address space.

About tests, I don't know how, since the code is in go aar, not in kotlin. I saw that the makefile has a target to run android code tests with Gradle but that doesn't cover go code, correct?

[Pawloland]

@kari-ts what do you say?

[kari-ts]

We prefer using ExcludeRoutes where possible because it avoids having to emulate exclusion by expanding the allowed set into many included routes and lets the platform handle subtraction internally, which is more robust and closer to how routing is intended to work.

It is possible to use exclude route method for supported android versions, but since it doesn't work for all of them supported by tailscale app, and manual approach does work for all, I thought it is better to stop using it everywhere, to avoid confusion.

I'm not concerned about too many lines in logcat; on pre-33 Android we have to express “allowed minus excluded” as an include list, and that list can blow up in size. I'm concerned that the computed include-route list becomes so large that VpnService.Builder (or underlying netd/kernel plumbing) can’t accept it, or it becomes unreliable to establish. My preference here is to fail fast and log the error.

Regarding route explosion, I am not sure what it means. Do you mean having too many of addresses printed out in logcat, instead of just the count of them? I suppose I can remove logging them, but it was usefull when comparing with the tool I mentioned in linked issue. If you instead meant, preventing the set of routes being applied, when the count of them in a set is too big, then I would say no. It would undermine the whole reason why they are calculated in the first place, which is to fit into the old interface of android. If only some of them were to be applied then the behavior would be different, than it is now on API 33+, and if none of them were to be applied, then it would simply do nothing.

rangeToCIDRs gives a minimal cover for each resulting range, but route explosion can still happen because subtraction fragments the allowed space into many ranges, and each fragment boundary can require ~O(bits) CIDRs to represent. So “minimal” doen't guarantee “small”, especially when the disallowed set is large/sparse.

The algorithm for calculating the CIDR ranges tries to create minimal number of them, with smallest possible masks, to cover the whole allowed address space.

https://github.com/tailscale/tailscale-android/blob/main/libtailscale/ifaceparse/ifaceparse_test.go might be helpful

About tests, I don't know how, since the code is in go aar, not in kotlin. I saw that the makefile has a target to run android code tests with Gradle but that doesn't cover go code, correct?

[Pawloland]

Ok, I will add conditional usage of ExcludeRoutes back, but before I want to know how to go about the other issues.

Regarding route explosion and setting a ceiling, what should it be? On https://developer.android.com/reference/android/net/VpnService.Builder I didn't see any documented mention of hard ceiling constrain, so I thought there isn't any. What should be considered an explosion? Tens of thousands of entries, thousands, hundreds? It is also untestable, because if such a ceiling exists or not, but is undocumented, then it is undefined behavior, possibly in kernel and probably is vendor specific. Setting such a ceiling in go code would be just an educated guess, which may not hold on every phone out there, unless the threshold is set dramatically low, which would make the feature useless and fail fast in most of the scenarios. Additionally from API specification there doesn't seem to be any exception that could be thrown that indicates that a route was rejected (other than invalid format of a passed route). I don't like the idea of setting an arbitrary ceiling, especially too low one, because it can prematurely stop the connection, even if it would work fine.

I propose a limit of 5000 routes, because in real conditions when there are usually up to 8 subnets to be excluded (wifi, celular, ethernet, which can be x2 because of possible dualstack ipv4 and ipv6 + localhost) the resulting ranges set was big but probably not that big and working alright on my device.

As for tests, they are just run manually, locally on developer's PC, correct? I didn't find any CI for go tests, nor any makefile target for it.