Bug 1297215 - Review Request: dnsdist - A highly DNS-, DoS- and abuse-aware loadbalancer
Review Request: dnsdist - A highly DNS-, DoS- and abuse-aware loadbalancer
Status: POST
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Roman Tsisyk
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2016-01-10 12:49 EST by Sander Hoentjen
Modified: 2016-03-28 19:29 EDT (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed:
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
roman: fedora‑review+


Attachments (Terms of Use)

  None (edit)
Description Sander Hoentjen 2016-01-10 12:49:57 EST
Spec URL: https://fedorapeople.org/~tjikkun/dnsdist/dnsdist.spec
SRPM URL: https://fedorapeople.org/~tjikkun/dnsdist/dnsdist-1.0.0-0.1.alpha1.fc23.src.rpm
Description: dnsdist is a highly DNS-, DoS- and abuse-aware loadbalancer. Its goal in life is to route traffic to the best server, delivering top performance to legitimate users while shunting or blocking abusive traffic.
Fedora Account System Username: tjikkun
Comment 1 Roman Tsisyk 2016-01-12 01:50:27 EST
Thanks your for you spec!

> #%license COPYING

License is mandatory for all new packages [1].
Your ticket has been fixed in the upstream, please enable
https://github.com/PowerDNS/pdns/pull/3202

[1]: https://fedoraproject.org/wiki/Packaging:LicensingGuidelines#License_Text

--------------

> BuildRequires: systemd-units

=> BuildRequires: systemd

systemd-units has been merged into systemd package [2].

[2]: https://fedoraproject.org/wiki/Packaging:Systemd#Filesystem_locations

--------------

> # install systemd unit file
> %{__install} -D -p -m 644 contrib/%{name}.service %{buildroot}%{_unitdir}/%{name}.service

+ Requires(post): systemd
+ Requires(preun): systemd
+ Requires(postun): systemd
+
+ %post
+ %systemd_post %{name}.service
+
+ %preun
+ %systemd_preun %{name}.service
+
+ %postun
+ %systemd_postun_with_restart %{name}.service

[3] https://fedoraproject.org/wiki/Packaging:ScriptletSnippets#New_Packages

--------------

> %{_mandir}/man1/%{name}.1.gz

> When installing man pages, note that they should be installed uncompressed as the build system will compress them as needed. The compression method may change, so it is important to reference the pages in the %files section with a pattern that takes this into account [4]

[4]: https://fedoraproject.org/wiki/Packaging:Guidelines#Manpages

>  /usr/bin/install -c -m 644 dnsdist.1 '/builddir/build/BUILDROOT/dnsdist-1.0.0-0.1.alpha1.fc24.x86_64/usr/share/man/man1'

--------------

dnsdist-1.0.0-alpha1/html/js/jquery-1.8.3.min.js
dnsdist-1.0.0-alpha1/html/js/jsrender.js
dnsdist-1.0.0-alpha1/html/js/moment.min.js
dnsdist-1.0.0-alpha1/html/js/purl.js
dnsdist-1.0.0-alpha1/html/js/rickshaw.min.js
dnsdist-1.0.0-alpha1/html/js/underscore-min.js
...

Obfuscated (=compiled) JavaScript looks like a binary for me:

> When you encounter prebuilt binaries in a package you MUST:
>
> Remove all pre-built program binaries and program libraries in %prep prior to the building of the package.
> Ask upstream to remove the binaries in their next release. [5]

[5] https://fedoraproject.org/wiki/Packaging:Guidelines#No_inclusion_of_pre-built_binaries_or_libraries

I'm not so experienced with Fedora packaging yet, but some other distros (you know) blame for all these *.min.js VERY strongly, so upstream might have problems with other packages as well. Please correct me if I'm wrong.

--------------

%dir %{_sysconfdir}/%{name}/

It would be nice to include an example of configuration file.

--------------

Note: No Requires: %{name}%{?_isa} = %{version}-%{release} in dnsdist-debuginfo

--------------

> -I/builddir/build/BUILD/dnsdist-1.0.0-alpha1/ext/mbedtls/include

mbedtls.i686 : Light-weight cryptographic and SSL/TLS library
mbedtls.x86_64 : Light-weight cryptographic and SSL/TLS library
mbedtls-utils.x86_64 : Utilities for mbedtls
mbedtls-static.i686 : Static files for mbedtls
mbedtls-static.x86_64 : Static files for mbedtls
mbedtls-devel.i686 : Development files for mbedtls
mbedtls-devel.x86_64 : Development files for mbedtls
mbedtls-doc.noarch : Documentation files for mbedtls

--------------

^I--disable-silent-rules \$
  --enable-dnscrypt \$
  --enable-libsodium \$
^I--with-lua \$

(`:set list` in vim)
--------------

> Summary: A highly DNS-, DoS- and abuse-aware loadbalancer

Remove the "A" article from Summary, it looks better in listings without.
It is not mandatory and is not mentioned in the guidelines, but I was noticed about that couple times by Zbigniew Jędrzejewski-Szmek.

--------------

Please try to use koji to ensure that package compiles on all supported architectures:
koji build --scratch rawhide pdns-4.0.0-0.1.alpha1.fc24.src.rpm

--------------

What the difference between https://github.com/PowerDNS/pdns and this software?
Shouldn't dnsdist to be added as a subpackage of existing pdns package?

--------------

It seems that pdns packages also uses yahttp.
Probably yahttp needs their own package.

--------------

License and unclear relation with existing pdns package are major problems here.
Comment 2 Sander Hoentjen 2016-01-12 14:23:41 EST
(In reply to Roman Tsisyk from comment #1)
> Thanks your for you spec!
Thanks for reviewing!
> 
> > #%license COPYING
> 
> License is mandatory for all new packages [1].
> Your ticket has been fixed in the upstream, please enable
> https://github.com/PowerDNS/pdns/pull/3202
> 
> [1]:
> https://fedoraproject.org/wiki/Packaging:LicensingGuidelines#License_Text
> 
Will fix this with the release of second alpha, expected soon
> --------------
> 
> > BuildRequires: systemd-units
> 
> => BuildRequires: systemd
> 
> systemd-units has been merged into systemd package [2].
> 
> [2]: https://fedoraproject.org/wiki/Packaging:Systemd#Filesystem_locations
Yes but for EPEL 6 you need systemd-units right? So that works on al releases. If I need to make it conditional I can of course.
> 
> --------------
> 
> > # install systemd unit file
> > %{__install} -D -p -m 644 contrib/%{name}.service %{buildroot}%{_unitdir}/%{name}.service
> 
> + Requires(post): systemd
> + Requires(preun): systemd
> + Requires(postun): systemd
> +
> + %post
> + %systemd_post %{name}.service
> +
> + %preun
> + %systemd_preun %{name}.service
> +
> + %postun
> + %systemd_postun_with_restart %{name}.service
> 
> [3] https://fedoraproject.org/wiki/Packaging:ScriptletSnippets#New_Packages
> 
Will fix
> --------------
> 
> > %{_mandir}/man1/%{name}.1.gz
> 
> > When installing man pages, note that they should be installed uncompressed as the build system will compress them as needed. The compression method may change, so it is important to reference the pages in the %files section with a pattern that takes this into account [4]
> 
> [4]: https://fedoraproject.org/wiki/Packaging:Guidelines#Manpages
> 
> >  /usr/bin/install -c -m 644 dnsdist.1 '/builddir/build/BUILDROOT/dnsdist-1.0.0-0.1.alpha1.fc24.x86_64/usr/share/man/man1'
> 
Ok, will fix
> --------------
> 
> dnsdist-1.0.0-alpha1/html/js/jquery-1.8.3.min.js
> dnsdist-1.0.0-alpha1/html/js/jsrender.js
> dnsdist-1.0.0-alpha1/html/js/moment.min.js
> dnsdist-1.0.0-alpha1/html/js/purl.js
> dnsdist-1.0.0-alpha1/html/js/rickshaw.min.js
> dnsdist-1.0.0-alpha1/html/js/underscore-min.js
> ...
> 
> Obfuscated (=compiled) JavaScript looks like a binary for me:
> 
> > When you encounter prebuilt binaries in a package you MUST:
> >
> > Remove all pre-built program binaries and program libraries in %prep prior to the building of the package.
> > Ask upstream to remove the binaries in their next release. [5]
> 
> [5]
> https://fedoraproject.org/wiki/Packaging:Guidelines#No_inclusion_of_pre-
> built_binaries_or_libraries
> 
> I'm not so experienced with Fedora packaging yet, but some other distros
> (you know) blame for all these *.min.js VERY strongly, so upstream might
> have problems with other packages as well. Please correct me if I'm wrong.

I will look into this
> 
> --------------
> 
> %dir %{_sysconfdir}/%{name}/
> 
> It would be nice to include an example of configuration file.
> 
Ok, will do that
> --------------
> 
> Note: No Requires: %{name}%{?_isa} = %{version}-%{release} in
> dnsdist-debuginfo
> 
> --------------
> 
> > -I/builddir/build/BUILD/dnsdist-1.0.0-alpha1/ext/mbedtls/include
> 
> mbedtls.i686 : Light-weight cryptographic and SSL/TLS library
> mbedtls.x86_64 : Light-weight cryptographic and SSL/TLS library
> mbedtls-utils.x86_64 : Utilities for mbedtls
> mbedtls-static.i686 : Static files for mbedtls
> mbedtls-static.x86_64 : Static files for mbedtls
> mbedtls-devel.i686 : Development files for mbedtls
> mbedtls-devel.x86_64 : Development files for mbedtls
> mbedtls-doc.noarch : Documentation files for mbedtls
> 
ok, will look into unbundling
> --------------
> 
> ^I--disable-silent-rules \$
>   --enable-dnscrypt \$
>   --enable-libsodium \$
> ^I--with-lua \$
> 
> (`:set list` in vim)
Good catch
> --------------
> 
> > Summary: A highly DNS-, DoS- and abuse-aware loadbalancer
> 
> Remove the "A" article from Summary, it looks better in listings without.
> It is not mandatory and is not mentioned in the guidelines, but I was
> noticed about that couple times by Zbigniew Jędrzejewski-Szmek.
> 
ok
> --------------
> 
> Please try to use koji to ensure that package compiles on all supported
> architectures:
> koji build --scratch rawhide pdns-4.0.0-0.1.alpha1.fc24.src.rpm
> 
will do
> --------------
> 
> What the difference between https://github.com/PowerDNS/pdns and this
> software?
> Shouldn't dnsdist to be added as a subpackage of existing pdns package?
> 
dnsdist lives in the pdns tree, but it is a fully seperate thing. https://www.powerdns.com/nluug/2015%20nluug%20powerdns%20dnsdist.pdf
"Open source, vendor neutral - it is not “PowerDNS Dist”"
> --------------
> 
> It seems that pdns packages also uses yahttp.
> Probably yahttp needs their own package.
Will look into this
> 
> --------------
> 
> License and unclear relation with existing pdns package are major problems
> here.
Hope I have resolved these, will try to have a new spec ready soon.
Comment 3 Sander Hoentjen 2016-01-14 15:47:30 EST
Just as an update: I am working with upstream to fix some of the issues.
Comment 4 Roman Tsisyk 2016-01-18 00:58:01 EST
> Just as an update: I am working with upstream to fix some of the issues.

OK.
Comment 5 Sander Hoentjen 2016-01-29 04:49:38 EST
Spec URL: https://fedorapeople.org/~tjikkun/dnsdist/dnsdist.spec
SRPM URL: https://fedorapeople.org/~tjikkun/dnsdist/dnsdist-1.0.0-0.2.alpha1.fc23.src.rpm

This is actually not alpha1 but closer to what alpha2 will be, so at the moment you cannot verify source, but this is more to show my progress. Once alpha2 gets released I will generate a new src.rpm
Comment 7 Roman Tsisyk 2016-02-06 03:28:30 EST
(In reply to Sander Hoentjen from comment #6)
> Spec URL: https://fedorapeople.org/~tjikkun/dnsdist/dnsdist.spec
> SRPM URL:
> https://fedorapeople.org/~tjikkun/dnsdist/dnsdist-1.0.0-0.3.alpha2.fc23.src.
> rpm
> 
> This should resolve the issues in comment 1

ACK. I'll try to review the new version on this weekend.
Comment 8 Roman Tsisyk 2016-02-08 02:10:45 EST
+ Fixed: license file is present, %license macro is used
+ Fixed: BuildRequires: systemd instead of systemd-utils
+ Fixed: %post %preun %postun scriptlets for systemd were added
+ Fixed: man pages installed uncompressed
+ Fixed: RPM spec now compiles JS from sources during %build stage
+ Fixed: bundled mbedtls is not used anymore
+ Fixed: pdns and dnsdist relation is now clear for me
+ license is acceptable (GPLv2)
+ BuildRequires  are OK
+ %check is present and all tests pass
+ mock builds [1] are ok
+ installs and works properly [2]
+ rpmlint is OK - only false-positive spelling-error

Package is APPROVED.

Further things to consider (up to you):

* it would be nice to include a sample configuration file
* bundled yahttp is used at least by pdns and your package, probably
  yahttp needs its own package

[1] tested on fedora-rawhide-x86_64 and fedora-23-armv7hl
[2] tested on fedora23-armv7hl with a configuration file from
https://github.com/PowerDNS/pdns/blob/master/pdns/dnsdistconf.lua
Comment 9 Gwyn Ciesla 2016-02-08 10:01:38 EST
Package request has been approved: https://admin.fedoraproject.org/pkgdb/package/dnsdist
Comment 10 Mike McCune 2016-03-28 19:29:58 EDT
This bug was accidentally moved from POST to MODIFIED via an error in automation, please see mmccune@redhat.com with any questions

Note You need to log in before you can comment on or make changes to this bug.