Skip to content

unbound: make blocklist additions/removals dynamic to prevent a restart#5747

Merged
AdSchellevis merged 3 commits intoopnsense:masterfrom
swhite2:unbound_general
May 9, 2022
Merged

unbound: make blocklist additions/removals dynamic to prevent a restart#5747
AdSchellevis merged 3 commits intoopnsense:masterfrom
swhite2:unbound_general

Conversation

@swhite2
Copy link
Member

@swhite2 swhite2 commented Apr 28, 2022

This PR contains an optimization for the way unbound can be updated in its' configuration
without the need for restarting Unbound and by extension a loss of internet connectivity.

Hopefully this will pave the way for optimizations on other aspects of Unbounds' configuration as well.
We could for example look into making the provided wrapper function more generic.

The GUI has also been modified slightly to inform the user of the amount of RRs added/removed.

This commit contains an optimization for the way unbound can be updated in its' configuration
without the need for restarting Unbound and by extension a loss of internet connectivity.

Hopefully this will pave the way for optimizations on other aspects of Unbounds' configuration as well.
We could for example look into making the provided wrapper function more generic.

The GUI has also been modified slightly to inform the user of the amount of RRs added/removed.
@fichtner
Copy link
Member

Hopefully this will pave the way for optimizations on other aspects of Unbounds' configuration as well.
We could for example look into making the provided wrapper function more generic.

Yes and no. What would be nice would be ACL updates but that isn't supported by unbound-control. This likely applies to other situations as well where it would be nice but wasn't implemented. ;)

@fichtner fichtner requested a review from AdSchellevis April 29, 2022 06:45
@swhite2
Copy link
Member Author

swhite2 commented Apr 29, 2022

Hopefully this will pave the way for optimizations on other aspects of Unbounds' configuration as well.
We could for example look into making the provided wrapper function more generic.

Yes and no. What would be nice would be ACL updates but that isn't supported by unbound-control. This likely applies to other situations as well where it would be nice but wasn't implemented. ;)

True, but unbound-control contains a wealth of other functionality which theoretically allows us to drastically reduce the number of restarts Unbound requires. This matters especially when millions of DNSBL records need to be loaded in every time. On top of that, we could trim the legacy codebase of functionality (such as overrides) we deemed too complex to put in a template and make it the responsibility of a configd action.

A downside I see would be the fact that 'Apply' wouldn't really apply the entire configuration anymore, but may be addressed through either documentation or a general 'force restart' option. This is also the reason I renamed the dnsbl button to Download & Apply.

AdSchellevis added a commit that referenced this pull request May 8, 2022
Changed the following minor items:

o gettext() for human readable reponse message
o simplify comparison loop (only new or diff are actually the same operation)
o replace one-liner split into a loop with validation in case an empty record exists (or something that doesn't fit the pattern)
o remove optional (but always) set -f option
@AdSchellevis
Copy link
Member

@swhite2 feedback and proposed changes in https://github.com/opnsense/core/tree/unbound_PR5747 (052fc62)

AdSchellevis and others added 2 commits May 9, 2022 08:40
Changed the following minor items:

o gettext() for human readable reponse message
o simplify comparison loop (only new or diff are actually the same operation)
o replace one-liner split into a loop with validation in case an empty record exists (or something that doesn't fit the pattern)
o remove optional (but always) set -f option
@swhite2
Copy link
Member Author

swhite2 commented May 9, 2022

@AdSchellevis Thanks for the review. Cherry-picked it and changed the following:

  1. Small spelling fix
  2. Strip more aggressively
  3. Do removals before additions, to prevent case insensitive duplicates from being added first, only to be removed next.

@AdSchellevis AdSchellevis merged commit 73a062c into opnsense:master May 9, 2022
@AdSchellevis
Copy link
Member

@swhite2 thanks!

type:script
type:script_output
message:Updating Unbound DNSBLs
description:Update Unbound DNSBLs
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@swhite2 @AdSchellevis due to review for 22.1.7 I saw that this is also used for a cron job due to the attached description. will it even continue to work? if not we need to split into dnsbl and dnsbl.cron ...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fichtner cron will ignore the output as far as I know

fichtner pushed a commit that referenced this pull request Jun 7, 2022
…rt (#5747)

(cherry picked from commit 73a062c)
(cherry picked from commit 1f1502a)
(cherry picked from commit 292b701)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants