Bug 242125

Summary: dns/dnsdist: Update to 1.4.0
Product: Ports & Packages Reporter: Jørn Åne de Jong <jornane>
Component: Individual Port(s)Assignee: Mateusz Piotrowski <0mp>
Status: Closed FIXED    
Severity: Affects Some People CC: 0mp, ask, cm, cpm, decke, freebsd, niels=freebsd, peter.van.dijk, ports, tremere, woodsb02, yannk
Priority: --- Flags: cpm: maintainer-feedback+
Version: Latest   
Hardware: Any   
OS: Any   
Attachments:
Description Flags
dns/dnsdist patch 2019112101
none
dns/dnsdist patch 2019112102
none
dns/dnsdist patch 2019112103
none
Update to dnsdist 1.4.0
none
Update to dnsdist 1.4.0 (and enable DoH by default)
none
dnsdist 1.4.0 multi-daemon
none
removed error from previous patch
none
same but against rev12
none
same as 209886 but adjusted
none
Update to dnsdist 1.4.0 (fix Leo's patch) none

Description Jørn Åne de Jong 2019-11-21 01:36:12 UTC
Created attachment 209303 [details]
dns/dnsdist patch 2019112101

dns/dnsdist: Update to 1.4.0

  * Update dns/dnsdist to version 1.4.0
  * Add DOH option for DNS over HTTPS (default on)
  * Add dependency on www/h2o when DOH is set

Changelog:

   https://dnsdist.org/changelog.html

QA:

  * portlint: OK (looks fine.)
  * testport: OK (poudriere: 120Ramd64, 120Ri386, only default options tested)
Comment 1 Jørn Åne de Jong 2019-11-21 01:54:16 UTC
Created attachment 209305 [details]
dns/dnsdist patch 2019112102

Made configuration flag for DoH dependent on the DOH option.
Comment 2 Peter van Dijk 2019-11-21 08:48:27 UTC
With 1.4.0, you should no longer need https://svnweb.freebsd.org/ports/head/dns/dnsdist/files/patch-dnsdist-lua-vars.cc?revision=496048&view=markup

I do not know if you still need the other patch.
Comment 3 Jørn Åne de Jong 2019-11-21 11:51:41 UTC
Created attachment 209316 [details]
dns/dnsdist patch 2019112103

You're right, it compiles and runs without these patches.

I'm not sure what patch-ext_luawrapper_include_LuaContext.hpp did, so I can't vouch for its removal, but I did test on 121Ramd64 and it runs a DoH server just fine.

QA:

  * portlint: OK (looks fine.)
  * testport: OK (poudriere: 121Ramd64, 121Ri386, 120Ramd64, 120Ri386, 113Ramd64, 113Ri386, with/without DOH tested on 121Ramd64, only default on others)
Comment 4 Ralf van der Enden 2019-11-21 15:19:58 UTC
Created attachment 209320 [details]
Update to dnsdist 1.4.0


- Makefile is formatted using ports-mgmt/portfmt
- Some configure switches changed from --enable to --with
- Rename FSTRM to DNSTAP
- Add DoH support
- Switch to LuaJIT-Openresty (when LuaJIT is selected instead of LUA)
- Remove unnecessary patch (files/patch-dnsdist-lua-vars.cc)
Comment 5 Jørn Åne de Jong 2019-11-21 17:59:03 UTC
Comment on attachment 209316 [details]
dns/dnsdist patch 2019112103

That seems better, thanks!
Comment 6 Carlos J. Puga Medina freebsd_committer 2019-11-23 18:00:43 UTC
(In reply to Ralf van der Enden from comment #4)

Hi Ralf, 

First of all, thank you very much for the patch :)

Second, are you interested in being the new maintainer and keeping it updated?

Last but not least, please can you explain why did you rename FSTRM to DNSTAP?
Comment 7 Carlos J. Puga Medina freebsd_committer 2019-11-23 18:12:03 UTC
*** Bug 242140 has been marked as a duplicate of this bug. ***
Comment 8 Ralf van der Enden 2019-11-24 16:37:18 UTC
Hi Carlos,

I'm interested in taking over maintainership. I also maintain dns/powerdns and dns/powerdns-recursor.

I renamed FSTRM to DNSTAP, because DNSTap is the feature that uses libfstrm (--enable-dnstap for configure). There's more info about it on https://dnstap.info

IMO it made more sense to (re)name the option after the feature than the used library).

Cheers,
Ralf
Comment 9 Peter van Dijk 2019-11-24 16:48:21 UTC
Hello from PowerDNS upstream! Ralf has been great about keeping dns/powerdns and dns/powerdns-recursor updated, checking with us if anything breaks, and making sure the patches the ports need end up in our upstream tree eventually. Indeed, when I pointed him to this ticket, he mentioned he already had an updated dnsdist port sitting on his disk, as he has shown in this thread.

So, we'd fully support Ralf taking ownership here. Thank you!

(also, yes, calling the option DNSTAP instead of FSTRM makes sense.)
Comment 10 Leo Vandewoestijne 2019-11-25 10:02:59 UTC
As I didn't expect anyone being such a rapid submitting the 1.4.0 update, I've submitted a duplicate PR: PR242140.

I don't know if it's the right moment to suggest a rc script that ignites multiple daemons, which would allow to keep config simple, and so prevent going slowing down.
For example if you run Bind with views, then you don't want to have a rule with countless times "if src addr then / else if addr then / etc".

In a single dnsdist I've used:

function luarule(dq)
  str = dq.localaddr:toString()
  if     string.find("10.0.0.1   2a03::1",   str) then return DNSAction.Pool, "v001"    -- mind that 2a03::1 also matches 2a03::11 and 2a03::111
  elseif string.find("10.0.0.2   2a03::2",   str) then return DNSAction.Pool, "v002"    -- mind that 10.0.0.2 also matches 10.0.0.22 and 10.0.0.222
  elseif string.find("10.0.0.249 2a03::249", str) then return DNSAction.Pool, "v249"    -- so string.find is not the most accurate function to use here 
                                                  else return DNSAction.Pool, "v250"
  end
end
addAction(AllRule(), LuaAction(luarule))

(I'm aware it's not the best code, but it served my need have something working).
In stead of improving config I tougth a multi-daemon RC was a better solution.

This patch contains what I was playing with:
https://bz-attachments.freebsd.org/attachment.cgi?id=209318
consider it code for inspiration: I'm not fully sure I've truly used it, plus wonder if it really does what I wish to accomplish (or if it's the correct way to do so).

I will try to do some testing later today.
If you have input in the meanwhile - or know of better methods/examples, then RSVP.
Comment 11 Leo Vandewoestijne 2019-11-26 16:21:39 UTC
Still working on it / verifying if it does what it should.

Ahead of that I'd like to mention the rc script could already be improved with minor changes:

+actual_command="%%PREFIX%%/sbin/${name} -u ${dnsdist_priv_user} -g ${dnsdist_priv_group} --supervised"
 command=/usr/sbin/daemon
-actual_command=/usr/local/sbin/${name}
-command_args="-c -f -r -P ${pidfile} ${actual_command} -u ${dnsdist_priv_user} -g ${dnsdist_priv_group} --supervised"
+command_args="-c -f -r -P ${pidfile} -- ${actual_command}"

Such that you unlikely will send wrong parameters to the other command.
Comment 12 Sascha Biberhofer 2019-12-01 13:24:19 UTC
(In reply to Ralf van der Enden from comment #4)

This patch has been working fine for me. Is DoH support going to stay a non-default option? I'd really prefer to enable DoH by default since dnsdist is one of the few projects able to serve DNS-over-TLS as well as DNS-over-HTTPS, especially since powerdns for example delegates this functionality pretty explicitly to dnsdist.
Comment 13 Ralf van der Enden 2019-12-02 08:44:34 UTC
Created attachment 209591 [details]
Update to dnsdist 1.4.0 (and enable DoH by default)
Comment 14 Ralf van der Enden 2019-12-02 08:46:42 UTC
Also cleaned up the Makefile a bit. DoH was enabled by default, but the OPTIONS knob for DoH pulled in the necessary library (h2o) if enabled.
Comment 15 Leo Vandewoestijne 2019-12-03 14:05:37 UTC
Created attachment 209658 [details]
dnsdist 1.4.0 multi-daemon

+ includes everything of patch-id 209591 of Ralf (I applied it before starting my changes)
+ maintainer change
+ minor improvements in the rc script
+ a major improvement in the rc script; allowing multiple daemons
+ start using -C and %%ETCDIR%% causing the default path for dnsdist.conf changes to usually /usr/local/etc/dnsdist/ in which you can store single or multiple config files, includes, key-files for DNSCrypt, key/certs for DoH, etc.

For the multi-daemon part I peaked at the NSD port maintained by Jaap Akkerhuis.

Of course I've tested it running it single and multiple, and checked (succesfully) if all processes / pid.files all started/stopped cleanly.
Comment 16 Leo Vandewoestijne 2019-12-10 13:47:26 UTC
Created attachment 209826 [details]
removed error from previous patch

While adding my own patch to my production environments I found a mistake; GNU_CONFIGURE went missing for unknown reason.
This patch keeps it as intended by Ralf.
Comment 17 Leo Vandewoestijne 2019-12-12 10:23:01 UTC
Created attachment 209886 [details]
same but against rev12

I applied my own patch and got rejected, then discovered PORTREVISION 12,
so this is all the same as before, but adapted to that.
Comment 18 Ben Woods freebsd_committer 2019-12-21 00:10:29 UTC
Hi Carlos,
Are you ok if I commit this and make Raif the maintainer?
Regards,
Ben
Comment 19 Yann Kerherve 2020-01-15 17:50:39 UTC
There is a bug in that patch, {pidfile} should be ${pidfile}
Comment 20 Leo Vandewoestijne 2020-01-15 23:13:28 UTC
Created attachment 210781 [details]
same as 209886 but adjusted

Same as 209886 but adjusted:
 - {pidfile} / ${pidfile}
 - MAINTAINER
 - and MASTER_SITES accordingly
and removed
 - # REQUIRE: LOGIN  from the rc script (I've misread the manual on this)
Comment 21 Ralf van der Enden 2020-01-22 08:53:42 UTC
Created attachment 210948 [details]
Update to dnsdist 1.4.0 (fix Leo's patch)


After applying Leo's diff I found out the port didn't build anymore. After fixing that, discovered the rc.d script looks in etc/dnsdist, but he didn't change anything else (like add --sysconfigdir to configure, create the directory and install the patch to that destination).

Note: basis for this one is Leo's diff (which in turn was based on mine), with additional fixes.
Comment 22 Ralf van der Enden 2020-01-22 08:54:24 UTC
Hopefully this can get committed now, since I've been getting questions from users when it'll be available.
Comment 23 Mateusz Piotrowski freebsd_committer 2020-02-04 09:15:39 UTC
I'll take a look.
Comment 24 commit-hook freebsd_committer 2020-02-04 10:39:02 UTC
A commit references this bug:

Author: 0mp
Date: Tue Feb  4 10:38:45 UTC 2020
New revision: 525141
URL: https://svnweb.freebsd.org/changeset/ports/525141

Log:
  dns/dnsdist: Update to 1.4.0

  - Lint Makefile
  - Some configure switches changed from --enable to --with
  - Rename FSTRM to DNSTAP
  - Add DoH support
  - Switch to LuaJIT-Openresty (when LUAJIT is selected instead of LUA)
  - Remove unnecessary patch (files/patch-dnsdist-lua-vars.cc)
  - Major improvement in the rc script: allowing multiple daemons
  - Start using -C and %%ETCDIR%% causing the default path for dnsdist.conf
    changes to usually /usr/local/etc/dnsdist/ in which you can store single
    or multiple config files, includes, key-files for DNSCrypt, key/certs for
    DoH, etc.

  Also, change maintainer to Ralf van der Enden.

  Changelog:
  https://dnsdist.org/changelog.html

  PR:		242125
  Submitted by:	J?rn ?ne de Jong, Ralf van der Enden, Leo Vandewoestijne
  Reviewed by:	cpm@, Sascha Biberhofer, Yann Kerherve
  Approved by:	maintainer

Changes:
  head/dns/dnsdist/Makefile
  head/dns/dnsdist/distinfo
  head/dns/dnsdist/files/dnsdist.in
  head/dns/dnsdist/files/patch-dnsdist-lua-vars.cc
  head/dns/dnsdist/pkg-plist
Comment 25 Mateusz Piotrowski freebsd_committer 2020-02-04 10:39:40 UTC
Thanks everyone!