Skip to content
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.

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

dnsdist: Add UCI integration, better defaults #25398

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

Conversation

rgacogne
Copy link
Contributor

Maintainer: me / @Habbie
Compile tested: github actions
Run tested: x86_64 docker and TP-Link Archer C7 AC1750 v5 (pre-APK, though)

Description:
This pull request adds UCI integration for DNSdist, making it possible to configure a fair amount of features via UCI. It includes the ability to resolve DNS queries for local domains, getting the name of local devices from the DHCP leases (which is why this PR grants the dnsdist user access to DHCP leases), and Discovery of Designated Resolver (DDR, RFC94621) support to signal DoT and DoH support to clients on the local network.
It also tunes the default settings, using values that make more sense for routers and reduce the memory usage a lot.

Signed-off-by: Remi Gacogne [email protected]

Copy link
Contributor

@Habbie Habbie left a comment

Choose a reason for hiding this comment

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

LGTM if you bump PKG_RELEASE!

@rgacogne
Copy link
Contributor Author

LGTM if you bump PKG_RELEASE!

Oops, good one! Done.

Comment on lines 87 to 97
define Package/dnsdist/postinst/Default
#!/bin/sh
# if we are on a "real" system, IPKG_INSTROOT will be empty,
# otherwise we are in the process of building an image and
# thus on the host system
local root="$${IPKG_INSTROOT}"

if [ ! -e "$${root}/etc/config/dnsdist" ]; then
cp "$${root}/usr/share/dnsdist/simple.uci.conf" "$${root}/etc/config/dnsdist"
fi
endef
Copy link

Choose a reason for hiding this comment

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

I don't think this needs to be a postinst. Conffile handling will ensure that a user's config is not overwritten. The $(INSTALL_CONF) that was removed should work just fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! Given that neither Habbie nor I can remember why we introduced this change, I'm going to sleep on it and very likely revert it tomorrow :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just pushed a commit reverting this change, thanks!

@rgacogne rgacogne requested a review from Habbie November 19, 2024 08:46
@rgacogne
Copy link
Contributor Author

I somehow managed to introduced a typo after testing my changes. Fixed now.

@Habbie
Copy link
Contributor

Habbie commented Nov 19, 2024

I somehow managed to introduced a typo after testing my changes. Fixed now.

oh, very subtle. both sample and simple could have made sense there

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants