[vpn-policy-routing] Wish: Preserve ASN lookup output from bundled user scripts

[yitzhaq]

When you're testing rules, you may find yourself frequently reloading VPR until things work the way you want. If you happen to have one of the bundled user scripts enabled, this will cause repeated AS lookups against the third-party service, only to receive a result that with overwhelming likelihood is identical to the previous one. As seen in #125 there can also be other circumstances where VPR will run several times within a short timespan.

With the previous service used for Netflix, this would quite quickly lead to throttling (and thus failure) after just a handful of requests, but even if this should not be the case with the new service, it seems bad form to be asking for seldomly changing data very often, at worst bordering on abuse. You could temporarily disable the user script to prevent this, of course, but then you might forget to re-enable it, and this also wouldn't help in the cases where multiple reloads are triggered. Or if the user script is what you're testing.

Seems to me it would be better if the user script had a means to prevent the unnecessary repeated lookups. As a bonus, this would also mean you wouldn't need to wait for (possible) throttling to end before things start working again. It could perhaps also shave a few milliseconds off the run time (not a significant change, I would imagine), at the cost of not very dynamic data becoming marginally less dynamic.

What if, instead of rm-ing $TARGET_FNAME at the end, it was preserved across runs, and only got cURLed if it didn't exist or had an mtime of $DB_REFRESH_AGE (or some better name), with a default value of 24h? Not cleaning up this tiny file in tmp (until it happens organically anyway) seems an acceptable tradeoff to me.

[stangri]

Thank you for the suggestion. While I love the idea of letting users configure the expiry of the previously downloaded IP addresses and re-download them on service start/reload/restart if they are stale, I'm not sure if this feature is used enough to warrant the additional development within the existing user scripts.

As a compromise, I've added the code to both user scripts to only download the IP addresses if the target file does not exist or is empty and it is now up to the users to remove/comment the rm -f "$TARGET_FNAME" line and control when/how to delete the previously downloaded file (with a cron job for example).

I've also changed the filenames to be more explicitly connected to vpn-policy-routing.

This is the commit link, I'll send PRs momentarily: 480a838

[yitzhaq]

Thanks for another quick fix! This seems a very sensible compromise, I'd say, which certainly accomplishes the main goal. With /var wiping clean on every reboot you effectively get organic expiry that is probably plenty sufficient for the vast majority of users, and there's obviously nothing stopping anyone from doing whatever external expiry they want, with cron or whatever, should they want something more frequent for whatever reason.

BTW, while only marginally on the topic of the Netflix user script, would you consider linking to this gist in the README? While fairly comprehensive, the Method #1 he describes proved, together with your user script, to be the missing piece of the puzzle to get Netflix working even with IPv6 (without any additional dependencies) when using VPN as default gateway.
Or maybe even just a paste-ready config chunk for /etc/config/dhcp with list server entries (I could probably whip something like that up if needed).

[yitzhaq]

Actually, come to think of it, I might suggest having the rm commented out as perhaps a better default? Just in terms of having something that works optimally straight out of the gate (as in not hammering the service under any circumstance) without requiring edits from the user, but which still makes things very easy for anyone wanting to tinker. As mentioned it should still get refreshed after a reboot, even without requiring anything further from the user, so the file wouldn't go stale forever.

[stangri]

I'm hesitant to leave large files in RAM. Thank you for providing the link to the gist, I'll update README today or tomorrow.

[yitzhaq]

Ah, yeah, you're probably right. I'm spoiled with x86_64 and gigs of RAM, which makes it all too easy to forget the constraints many users have. Good looking out!

[yitzhaq]

Although..

# ls -l /var/vpn-policy-routing_tmp_AS2906
-rw-r--r-- 1 root root 2662 Feb 28 16:28 /var/vpn-policy-routing_tmp_AS2906

Don't mean to be going on about this, but 2.6k isn't particularly large, is it? I mean technically I guess you could say that does mean VPR leaves something in memory, but, like, it leaves a PID file too :o)

[stangri]

but 2.6k isn't particularly large, is it?

I agree, it isn't. I thought about keeping it, my concern is that when the user runs restart or reload, they may be expecting the file to be refreshed, but it won't be. Let me sleep on it.

[yitzhaq]

I see your point, but then I think we're quickly sort of moving back towards expiry land.

As a compromise I guess you could delete /var/vpn-policy-routing_tmp* when the user runs stop and maybe also restart (so the script doesn't "leave" anything in memory when it's not "running"), but not on reload (which I guess is still the default action, and should be considered "softer"). Although, if someone were to reuse that namespace in their own user scripts, I suppose this could theoretically lead to unexpected consequences. OTOH, you could argue that this would provide them with some free logic they could then reuse and rely upon in their user scripts. Either way, I'd say you have to consider whether you want to start mixing up stuff in the mother script with the (otherwise standalone) user script(s). Unless the user scripts could somehow read the invoke action from the mother script?

All that said, I do sincerely doubt the list in the current bundled user scripts change more often than the average user reboots their router. It's not like IPv4 subnets just get strewn about anymore :o)

[stangri]

Given recent reports that most ASN-data providers throttle consecutive requests, I'm inclined to change my previous position. I will update the PBR (and PBR WebUI to include the link describing change in behaviour) to retain previously downloaded data.

[stangri]

I believe this has been addressed. Moreover, I was thinking that for the nft-capable version, I may preserve the nft-script itself instead of the downloaded file which will speed up the reload/restart a little bit more. Please reopen if I'm missing something.