https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36304 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|In Discussion |Failed QA --- Comment #5 from David Cook <dcook@prosentient.com.au> --- (In reply to Nicholas van Oudtshoorn from comment #4)
Created attachment 163973 [details] [review] Support for externally updated proxy lists
Thanks for the feedback, David!
Thanks for being receptive!
It makes sense to move it into the koha-conf.xml file, and this updated patch has done so. It includes a sample bash script that can be used to keep an external list file up to date. This can also easily be extended to automatically update other providers, hence I have taken away the cloudflare name, although cloudflare is referenced in a comment in koha-conf.xml
You'll want to add the config block to "debian/templates/koha-conf-site.xml.in" as well. (I think it could be worthwhile to say in the comment that the list is a plain-text with an IP (range) per line, but not a blocker.)
Currently, RealIP was re-generating the trusted_proxies list on every call to get_real_ip. This patch also uses Koha's caching system to prevent reloading from the config file and from the external list on every call, which should speed things up just a little bit.
Oh yikes. I remember reading through this recently, and I saw "$self->trusted_proxy()" in prepare_app(), and that list is passed to get_real_ip(), so I figured it was just determining the list at startup time, but it looks like it's unused currently and it really is fetching it on every call! Double yikes... I think we'd want to split the changes here. I think a new ticket to fix RealIP.pm to use the $self->trusted_proxy() set up in "prepare_app", and we could also set that up to use the cache to further improve performance. The change to RealIP.pm looks like it has some copy/pasted code. I'd suggest adding the list IPs to @trusted_proxies() rather than having 2 separate blocks calling Net::Netmask->new2().
The sample bash script will restart koha-common if it is running, and also clear the caches of all koha-instances.
Since this one is Cloudflare specific, I think it would be worthwhile including that in the script name (e.g. "update_cloudflare_proxies_list.sh). I think it might be overkill restarting the whole koha-common service, since we really just need to restart the Plack in this case. You'd want to clear the cache first before running koha-plack --reload/--restart as well I believe. -- Overall, I think it's looking pretty good though. -- You are receiving this mail because: You are watching all bug changes.