Skip to content

Fixes for #5449: Add include directory for custom BIND options #5588

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
maloo1 wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Fixes for #5449: Add include directory for custom BIND options #5588

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c713ac3
Fixes for #5339: Add include directory for custom BIND options
maloo1 Jul 29, 2026
a472bf5
Fixes for #5339: Fix glob
maloo1 Aug 6, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions dns/bind/src/etc/namedb/named-options.conf.d/00-README.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
# Custom BIND Configuration Directory for BIND options
#
# Place your custom BIND options in .conf files in this directory
# Files are included in alphabetical order
#
# Example:
# forwarders port 853 tls Cloudflare-DoT { 1.1.1.2; 1.0.0.2; };
3 changes: 2 additions & 1 deletion dns/bind/src/opnsense/mvc/app/library/OPNsense/System/Status/BindOverrideStatus.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,8 @@ public function __construct()

public function collectStatus()
{
if (count(glob('/usr/local/etc/namedb/named.conf.d/*')) > 1) {
if (count(glob('/usr/local/etc/namedb/named.conf.d/*')) > 1
or count(glob('/usr/local/etc/namedb/named-options.conf.d/*')) > 1) {

@cm-rudolph cm-rudolph Jul 30, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Using a different glob on the directories than in the include statements is a bit odd and leads to a warning message, even though no config file actually gets included.

Yes, it is the same pattern as in #4773, but should be fixed in both places.

@maloo1 maloo1 Jul 31, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Hi @cm-rudolph - Thanks for taking a look, I'm not sure I understand what you mean though? There are two include directories the new one I've added for options on line 12: include "/usr/local/etc/namedb/named-options.conf.d/*.conf";
And the existing one added in #4773 on line 116:
include "/usr/local/etc/namedb/named.conf.d/*.conf";

I have tested the following on my setup:

  1. No added config changes in the include dirs: No warning is displayed
  2. Added a file to the new options include dir: The warning gets displayed
  3. Also added a file to the old include dir: The warning is still displayed
  4. Removed the added file from the new options include dir: The warning is still displayed (because of tge file in the existing include dir)
  5. Removed both added files: No warning is displayed

@cm-rudolph cm-rudolph Jul 31, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Hi @maloo1,

let's assume somebody stores some configuration options in /usr/local/etc/namedb/named-options.conf.d/myoption. The UI would indicate that some manual config probably overwrites config from the UI. But the myoption file isn't loaded, due to the missing .conf file extension.

Line 50 and 51 should read (untested):

        if (count(glob('/usr/local/etc/namedb/named.conf.d/*.conf')) > 1 
            or count(glob('/usr/local/etc/namedb/named-options.conf.d/*.conf')) > 1) {

Best regards
Christian

@maloo1 maloo1 Jul 31, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Oh thank you @cm-rudolph now I understand! I'll update and test tomorrow, but it will need to be > 0 (since in #4773 and my change we were accounting for the README.txt!!)

@maloo1 maloo1 Aug 6, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@cm-rudolph Sorry for the delay on this - I have updated and tested the changes and all works as expected now. Like you said it is cleaner as its only counting what its actually going to load!

@cm-rudolph cm-rudolph Aug 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

No problem. Although I'm not authorised to make a decision regarding this PR, I’d still say: lgtm!

Best regards,
Christian

$this->internalMessage = gettext(
'The configuration contains manual overwrites, these may interfere with the settings configured here.'
);
Expand Down
1 change: 1 addition & 0 deletions dns/bind/src/opnsense/service/templates/OPNsense/Bind/named.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ acl "{{ acl_list.name }}" { {{ acl_list.networks.replace(',', '; ') }}; };
{% endif %}

options {
include "/usr/local/etc/namedb/named-options.conf.d/*.conf";

directory "/usr/local/etc/namedb/working";
pid-file "/var/run/named/pid";
Expand Down