[bug#54352,v2] services: dnsmasq: Add more options.

* gnu/services/dns.scm (<dnsmasq-configuration>): Add forward-private-reverse-lookup?, strict-order? and additional-cpe-id options.
(dnsmasq-shepherd-service): Pass added options to dnsmasq.
* doc/guix.texi (Guix Services): Document options added to dnsmasq.
---
 doc/guix.texi        |  12 +++
 gnu/services/dns.scm | 178 +++++++++++++++++++++++--------------------
 2 files changed, 109 insertions(+), 81 deletions(-)

Remco van 't Veer schreef op zo 20-03-2022 om 12:44 [+0100]:
> +  (forward-private-reverse-lookup?
> +   dnsmasq-configuration-forward-private-reverse-lookup?
> +                    (default #t))       ;boolean
> +  (strict-order?    dnsmasq-configuration-strict-order?
> +                    (default #f))       ;boolean

It would be nice to verify that these are, in fact, booleans,
using field sanitizers.  See, e.g., ensure-setuid-program-list
in (gnu system).

Greetings,
Maxime.

2022/03/20 12:56, Maxime Devos:

> It would be nice to verify that these are, in fact, booleans,
> using field sanitizers.  See, e.g., ensure-setuid-program-list
> in (gnu system).

I agree but the same could be said about the other fields and types in
this record, and those of other services.  In this case, the names of
the fields ending with "?" should be enough for somebody to realise this
is a boolean, IMHO.  The ";boolean" comments I've added are just me
trying to blend in.

Remco van 't Veer schreef op zo 20-03-2022 om 13:22 [+0100]:
> 2022/03/20 12:56, Maxime Devos:
> 
> > It would be nice to verify that these are, in fact, booleans,
> > using field sanitizers.  See, e.g., ensure-setuid-program-list
> > in (gnu system).
> 
> I agree but the same could be said about the other fields and types in
> this record, and those of other services. 

In the long-term, it would be nice to eventually add error checking to
other services and fields as well.  In the short-term, I would avoid
making error handling worse.

> In this case, the names of the fields ending with "?" should be
> enough for somebody to realise this is a boolean, IMHO. 
> The";boolean" comments I've added are just me trying to blend in.

It's technically sufficient, but it does not make for nice error
messages, see the thread at
<https://lists.gnu.org/archive/html/guix-devel/2022-02/msg00140.html>.

In this particular case, there won't be an error message *at all*,
due to how (if forward-private-reverse-lookup? ... ...) works.

For example, consider the case where someone new to Guile and Guix sees
‘boolean’ in the documentation and then tries using "true" and "false"
(the strings):

  (dnsmasq-configuration
    (forward-private-reverse-lookup? "false"))

Currently, this will silently do the wrong thing.

Greetings,
Maxime.

Remco van 't Veer schreef op zo 20-03-2022 om 12:44 [+0100]:
> +  (strict-order?    dnsmasq-configuration-strict-order?
> +                    (default #f))       ;boolean

The field strict-order? appears to be unused.

Greetings,
Maxime.

Remco van 't Veer schreef op zo 20-03-2022 om 12:44 [+0100]:
> +@item @code{additional-cpe-id} (default: @code{#f})
> +If set, add an arbitrary identifying string to DNS queries which are
> +forwarded upstream.

What is an ‘arbitrary identify string’ here?  What does ‘cpe’ mean? 
How can I know if I need to set this, and how do I know what to set it
to?

Greetings,
Maxime.

Remco van 't Veer schreef op zo 20-03-2022 om 12:44 [+0100]:
> +When true, forces dnsmasq to try each query with each server strictly in
> +the order they appear in @var{servers}.

Can be simplified to:

  When try, dnsmasq queries the servers in the same order as they
  appear in @var{server}.

dnsmasq does this voluntarily on asking, there doesn't appear to be any
forcing here.

Also, what does ‘strict’ mean here?  Is it like the difference between
<= and <?  WDYT of naming it 'query-servers-in-order?'?

Greetings,
Maxime.

> What is an ‘arbitrary identify string’ here?  What does ‘cpe’ mean?
> How can I know if I need to set this, and how do I know what to set it
> to?

I think it stands for "Common Platform Enumerator" but I am not really
sure.  It's used by DNS providers to identify the client so they know
for what account they are resolving.  Looking at the dnsmasq code base
this option used to be called "dns-client-id" which is a lot clearer.  I
could change it to this older name and describe it as above.  WDYT?

2022/03/20 13:31, Maxime Devos:

> The field strict-order? appears to be unused.

Oeps, I'll fix it.

2022/03/20 13:30, Maxime Devos:

> In the long-term, it would be nice to eventually add error checking to
> other services and fields as well.  In the short-term, I would avoid
> making error handling worse.
> [snip]
> Currently, this will silently do the wrong thing.

True, I'll add sanitize code for the boolean fields.

I totally agree with the naming suggestions and will apply them but what
if somebody has been using dnsmasq before?  In that case they will not
recognize the options and which may cause confusion.  I guess the answer
to that is that guix should not propagate the wording mistakes made
upstream?

To help those with prior knowledge of dnsmasq, I could add a config-file
field.  WDYT?

Remco van 't Veer schreef op zo 20-03-2022 om 13:57 [+0100]:
> > What is an ‘arbitrary identify string’ here?  What does ‘cpe’ mean?
> > How can I know if I need to set this, and how do I know what to set
> > it
> > to?
> 
> I think it stands for "Common Platform Enumerator" but I am not
> really sure.  It's used by DNS providers to identify the client so
> they know for what account they are resolving.  Looking at the
> dnsmasq code base this option used to be called "dns-client-id" which
> is a lot clearer.  I could change it to this older name and describe
> it as above.  WDYT?

I'm not familiar enough with DNS to tell which name would be better.
In any case, it seems like you have some material for a description
here.  Additionally, you could add a cross-reference to the relevant
RFC+section number, if you can locate it.

Greetings,
Maxime.

Remco van 't Veer schreef op zo 20-03-2022 om 14:15 [+0100]:
> I totally agree with the naming suggestions and will apply them but what
> if somebody has been using dnsmasq before?  In that case they will not
> recognize the options and which may cause confusion.  I guess the answer
> to that is that guix should not propagate the wording mistakes made
> upstream?

Perhaps some lines like

  ‘This option corresponds to the --foo-bar command line option.’

could be added?

Greetings,
Maxime.

Remco van 't Veer schreef op zo 20-03-2022 om 14:15 [+0100]:
> [...] I could add a config-file field.  WDYT?

I don't think that's necessary but it's certainly an option.
This raises some questions though: if something is specified
in both the fields and the configuration file, what will dnsmasq
use?

Greetings,
Maxime.

Hi Maxime,

Maxime Devos <maximedevos@telenet.be> skribis:

> Remco van 't Veer schreef op zo 20-03-2022 om 12:44 [+0100]:
>> +  (forward-private-reverse-lookup?
>> +   dnsmasq-configuration-forward-private-reverse-lookup?
>> +                    (default #t))       ;boolean
>> +  (strict-order?    dnsmasq-configuration-strict-order?
>> +                    (default #f))       ;boolean
>
> It would be nice to verify that these are, in fact, booleans,
> using field sanitizers.  See, e.g., ensure-setuid-program-list
> in (gnu system).

I think this suggestion is beyond the scope of this review: we’ve never
used sanitizers like this before (or almost), and this particular piece
of code doesn’t use them.

Also, with the recent discussion about the introduction of contracts,
I’d rather wait an use contracts everywhere once they’re available.

Thoughts?

Thanks,
Ludo’.

Ludovic Courtès schreef op ma 21-03-2022 om 16:22 [+0100]:
> I think this suggestion is beyond the scope of this review: we’ve never
> used sanitizers like this before (or almost), and this particular piece
> of code doesn’t use them.
> 
> Also, with the recent discussion about the introduction of contracts,
> I’d rather wait an use contracts everywhere once they’re available.

Seems reasonable to me, given that the specifics weren't discussed yet,
although _everywhere_ (for all procedures, records, ...) seems a bit
much, unless you meant every field of the dnsmasq record.

Greetings,
Maxime.

2022/03/21 19:36, Maxime Devos:

> [[PGP Signed Part:Undecided]]
> Ludovic Courtès schreef op ma 21-03-2022 om 16:22 [+0100]:
>> I think this suggestion is beyond the scope of this review: we’ve never
>> used sanitizers like this before (or almost), and this particular piece
>> of code doesn’t use them.
>>
>> Also, with the recent discussion about the introduction of contracts,
>> I’d rather wait an use contracts everywhere once they’re available.
>
> Seems reasonable to me, given that the specifics weren't discussed yet,
> although _everywhere_ (for all procedures, records, ...) seems a bit
> much, unless you meant every field of the dnsmasq record.

I can add something like the following:

  (define (assert-boolean value)
    (unless (false-if-exception (boolean? value))
      (error-out (format #f "expected a boolean, got: ~s" value)))
    value)

and use it to do

  (sanitize assert-boolean)

on all boolean fields of dnsmasq but I agree with Ludo about this being
a bigger issue which can be solved much more elegantly (including i18n
and source location etc).  In the above I borrowed error-out from knot
in the same file which also doesn't do i18n and source locations.

I'd like to leave it out because the codebase has plenty more unverified
booleans and basic if-statements which go the unexpected route when
passed a "false" string.

2022/03/20 14:20, Maxime Devos:

> Remco van 't Veer schreef op zo 20-03-2022 om 14:15 [+0100]:
>> [...] I could add a config-file field.  WDYT?
>
> I don't think that's necessary but it's certainly an option.
> This raises some questions though: if something is specified
> in both the fields and the configuration file, what will dnsmasq
> use?

Ah, good point.  I'll prepare a new patch when it comes up and work it
out then.

2022/03/20 14:17, Maxime Devos:

> Perhaps some lines like
>
>   ‘This option corresponds to the --foo-bar command line option.’
>
> could be added?

I can't find any precedence in the guix manual about corresponding
command line options like that, so let's not.  Maybe it won't be
confusing and it's pretty easy to find the service definition to find
out.

2022/03/20 14:16, Maxime Devos:

> I'm not familiar enough with DNS to tell which name would be better.
> In any case, it seems like you have some material for a description
> here.  Additionally, you could add a cross-reference to the relevant
> RFC+section number, if you can locate it.

Well, that was a bit of a challenge but I did find out it's a EDNS
field, CPE stands for "Customer-premises Equipment" and that field is in
an IANA unassigned range but, apparently, used by DNS providers to
identify their clients/accounts.  So what do you think about:

  If set, add a CPE (Customer-Premises Equipment) identifier to DNS
  queries which are forwarded upstream.

I don't want to dwell on the unofficial status too much.  Also, I
changed the field name from additional-cpe-id to just cpe-id because
"additional" seems a bit redundant.

WDYT?

Hi,

Remco van 't Veer <remco@remworks.net> skribis:

> 2022/03/21 19:36, Maxime Devos:
>
>> [[PGP Signed Part:Undecided]]
>> Ludovic Courtès schreef op ma 21-03-2022 om 16:22 [+0100]:
>>> I think this suggestion is beyond the scope of this review: we’ve never
>>> used sanitizers like this before (or almost), and this particular piece
>>> of code doesn’t use them.
>>>
>>> Also, with the recent discussion about the introduction of contracts,
>>> I’d rather wait an use contracts everywhere once they’re available.
>>
>> Seems reasonable to me, given that the specifics weren't discussed yet,
>> although _everywhere_ (for all procedures, records, ...) seems a bit
>> much, unless you meant every field of the dnsmasq record.

Sorry, I wasn’t clear.  I mean, when we have contracts in Guix, we can
start using them “everywhere” in Guix, primarily for configuration
records.  But we’re not there yet and how exactly we’ll get there
remains to be seen.

> I can add something like the following:
>
>   (define (assert-boolean value)
>     (unless (false-if-exception (boolean? value))
>       (error-out (format #f "expected a boolean, got: ~s" value)))
>     value)
>
> and use it to do
>
>   (sanitize assert-boolean)

My suggestion is to just do nothing for the dnsmasq record; just leave a
comment about the expected type next to it and in the manual, as is done
for almost every other configuration record.

Eventually we’ll turn that into contracts with good error reporting, I
hope, but that’s not something you can do right now.  :-)

Thanks!

Ludo’.

Just posted a v3.  Changes from v2 to v3:

* renamed field additional-cpe-id to cpe-id
* improved documentation of cpe-id
* renamed field strict-order? to query-servers-in-order?

R.