unbound: check if a restart is required, reload after dnsbl update

[kulikov-a]

Important notices
Before you add a new report, we ask you kindly to acknowledge the following:

[+] I have read the contributing guide lines at https://github.com/opnsense/core/blob/master/CONTRIBUTING.md

[+] I have searched the existing issues and I'm convinced that mine is new.

Is your feature request related to a problem? Please describe.
Hello everyone.
For now:

• if link on any interface goes down and up, unbound updates the hosts records and restarts. Even if the address has not changed or is static. The restart can take a while if large dnsbl list is in use, so clients practically lose the Internet for some time.
• unbound does not restart after updating the dnsbl list(s) ( pluginctl unbound restart . typo? -s missed ?)
  https://forum.opnsense.org/index.php?topic=20356.0

Describe the solution you'd like
could you consider changes like this:

• check if the host_entries.conf file has actually changed before unbound restart
  (something like kulikov-a@d799447).

• change actions_unbound.conf [dnsbl] command to
  /usr/local/opnsense/scripts/unbound/download_blacklists.py && configctl unbound reload
  ( pluginctl -s unbound restart works too. but reload is far more fatser than restart. and imho reload is enough after dnsbl update. UPDATE! in my case difference in time for reload vs restart was due to a failed unbound-anchor launch. not by the procedure itself)
  Thanks!

Describe alternatives you've considered
A clear and concise description of any alternative solutions or features you've considered.

Additional context
Add any other context or screenshots about the feature request here.

[AdSchellevis]

@kulikov-a the dnsbl part looks like a bug indeed, 3facaaa should fix it. Adding more logic in an attempt to "restart unbound" properly I'm not really a fan of. In my opinion most of these restart actions on interface changes shouldn't happen at all, since the service should take care of its own issues. When bound to a dynamic interface one could argue that using a port forward to an internal address would be a safer choice (and is a pattern most other vendors advise as well to prevent cluttering services).

Short term I'm not sure if we can detach unbound from the interface reload actions, there's more weird cluttering of interface addresses in it at the moment.

[kulikov-a]

@AdSchellevis thanks!
not detach, just skip unnecessary action.
I'm not a fan either, but I think that changing host entries through a unbound-control in this part is also not a nifty way (
so may be at least replace killbypid with "reload" (for me it 3 times faster) in unbound_hosts_generate()?
(not tested it yet. but should work imo)

and sorry, сould you please explain where I am wrong in #4497 ? so not to make mistakes in the future

[AdSchellevis]

@kulikov-a I'm not sure what you mean with the killbypid, but if you add a PR, I'll gladly take a look.

about #4497, did I say it's an improper request? If I did, I'll probably have to apologise . I think most of the backend components do support alternative separators, I can't remember how it's organised in the tokeniser to be honest.

[kulikov-a]

@AdSchellevis
yes, sure. i will test it in VM and make a PR for it
about #4497 : no no. I just decided that if there are no responses, then I either made a crooked request, or poorly described the proposals.

[AdSchellevis]

@kulikov-a certainly not, we're just quite busy.

[kulikov-a]

@AdSchellevis
oh! sorry for the impatience

[kulikov-a]

My apologies and clarifications. With a successful launch of unbound-anchor before launching unboud, there is no noticeable time difference between restart and reload (sorry. should have found out before creating a FR. just old PIX droped dns-packets > 512 bytes). another question is whether it is necessary to start unbound-anchor at all if DNSSEC not enabled on onbound.

[kulikov-a]

@fichtner thanks!
@AdSchellevis @fichtner
is it possible to offer an option not to use import of dnsbls via .conf inclusion? can load into an already working inbound using unbound-control local_datas command
quickly tested by adding
/usr/local/sbin/unbound-control -c /var/unbound/unbound.conf local_datas < /var/unbound/etc/testDNSBL.list
in the end of unbound_start.sh
(change a download script a bit to produce suitable list)
result: no downtime at all at restart. the server serves requests while loading the full 102MB dnsbl list
of course, still need to test in different circumstances (links down\up, ip change etc), if you are ready to consider such a transition

[AdSchellevis]

@kulikov-a we're using a similar approach in https://github.com/opnsense/core/blob/master/src/opnsense/scripts/dns/unbound_dhcpd.py if I'm not mistaken. I'm not perse agains loading the data using unbound-control when unbound itself starts fast enough when the configuration is already there. (in which case you can maintain the database while initial content is more or less complete). It does however complicate loading a bit, since irrelevant items need to be removed as well, which can be challenging.

[kulikov-a]

@AdSchellevis
wow! did not know that you made dhcp use the unbound-control. (I do not use OPN DHCP and thought it would still restart unbound on every client). awesome!
I don't know how to manage changes in such a large list faster than just flushing and loading a new one. For now unbound is restarting or reloading to load the dnsbl. and (if dnsbls enabled) reloads list at every restart\reload). so I suggest only to load the list after the start of the unbound so that there is no service downtime

[kulikov-a]

@AdSchellevis
So my suggestion does not change any logic or events that trigger a restart, HUP or reload of unbound (harmless?). Only changes the moment of loading dnsbls from .conf-inclusion to unbound-control loading.
small bonus: with this approach, unbound is not at risk of stopping working if there is a garbage entry in dnsbl (as in this example: https://forum.opnsense.org/index.php?topic=20284.0). unbound-control just skips this entry and continues loading the list:
unbound[79396] | [79396:0] error: error parsing local-data at 98 'ister.co.uk/2014/10/07/adobe_digital_editions_4_caught_snooping_into_ebook_collections_of_users/ A 0.0.0.0': Label length overflow
can you please look at the code changes when you have time?
master...kulikov-a:patch-2

[AdSchellevis]

@kulikov-a just open a PR and I'll try to take a look, in case of the blacklists this might be safer indeed. Better not call configd from configd by the way, although it works, we try to avoid nesting.

[Bramzor]

Ran into this "bug" today.
Alternative can be to just place a warning that a manual restart is required after updating dnsbl selection?

[kulikov-a]

@Bramzor
fixed by 3facaaa
(and before I noticed and mentioned this)

[fichtner]

@kulikov-a can this be closed?

[kulikov-a]

Yes, thanks!
Closing..
PR for moving dnsbls from .conf to control is still on hold

[fichtner]

I promise to take a look now that 21.1-RC1 is out we have master branch for experiments again :)

[kulikov-a]

I'm not in a hurry, just the PR was mentioned in this FR )
Thanks again!