Re: isc-bind service draft

[Chris Marusich]

Hi Oleg,

Oleg Pykhalov <address@hidden> writes:

> I work on isc-bind service.  Currently generation of named.conf is done.
> Ideas and suggestions are welcome!  :-)

Awesome!  Thank you for working on this.  I'm not familiar with BIND
configuration, so I can't really comment much on the particular fields
you've chosen to include in the various configuration objects you've
created.  It'd be nice if someone more familiar with BIND could give it
a look.

> (define-record-type* <bind-options-configuration>

Are these options intended to be used when invoking bind?  If so, maybe
a name like "bind-options" is probably good enough.

>   bind-options-configuration make-bind-options-configuration
>   bind-options-configuration?
>   (user             bind-options-configuration-user          ; string
>                     (default "bind"))
>   (group            bind-options-configuration-group         ; string
>                     (default "bind"))
>   (run-directory    bind-options-configuration-run-directory ; string
>                     (default "/var/run/bind"))
>   (pid-file         bind-options-configuration-pid-file      ; string
>                     (default "/var/run/bind/named.pid"))

For what it's worth, nowadays some distros use /run as the "run
directory" [1].  I don't know if GuixSD has adopted any particular
policy about whether to use /var/run or /run for the default "run
directory".  I don't currently know of any reason why it matters much,
so I think it's fine to use /var/run here.

>   (listen-v4        bind-options-configuration-listen-v4     ; string
>                     (default "0.0.0.0"))
>   (listen-v6        bind-options-configuration-listen-v6     ; string
>                     (default "::"))
>   (listen-port      bind-options-configuration-listen-port   ; integer
>                     (default 53))
>   (allow-recursion? bind-configuration-allow-recursion?      ; list
>                     (default (list "127.0.0.1")))
>   (allow-transfer?  bind-configuration-allow-transfer?       ; list
>                     (default (list "none")))
>   (allow-update?    bind-configuration-allow-update?         ; list
>                     (default (list "none")))
>   (version          bind-configuration-version               ; string
>                     (default "none"))
>   (hostname         bind-configuration-hostname              ; string
>                     (default "none"))

Why not use the system's host name by default?  For example:

   (hostname         bind-configuration-hostname              ; string
                     (default (gethostname)))

>   
>   (server-id bind-configuration-server-id ; string (default "none")))
>
> (define (bind-configuration-statement-string statements)
>   (string-join (list "{" (string-join statements ";\n") "}")))
>

You could also write it like this:

 (define (bind-configuration-statement-string statements)
   (string-append "{" (string-join statements ";\n") "}"))

>
> (define-record-type* <bind-zone-configuration>
>   bind-zone-configuration make-bind-zone-configuration
>   bind-zone-configuration?
>   (network bind-zone-configuration-network  ; string
>            (default '()))
>   (class   bind-zone-configuration-class    ; string
>     (default '()))
>   (type    bind-zone-configuration-type     ; string
>            (default '()))
>   (file    bind-zone-configuration-filename ; string
>            (default '())))
>
> (define-record-type* <bind-configuration-file>
>   bind-configuration-file make-bind-configuration-file
>   bind-configuration-file?
>
>   ;; <bind-options-configuration>
>   (config-options bind-configuration-file-config-options
>                   (default (bind-options-configuration)))
>
>   ;; list of <bind-zone-configuration>
>   (config-zones   bind-configuration-file-config-zones
>                   (default (list (bind-zone-configuration
>                                   (network "localhost")
>                                   (class   "IN")
>                                   (type    "master")
>                                   (file    "localhost.zone"))
>                                  (bind-zone-configuration
>                                   (network "0.0.127.in-addr.arpa")
>                                   (class   "IN")
>                                   (type    "master")
>                                   (file    "127.0.0.zone"))
>                                  (bind-zone-configuration
>                                   (network (string-append 
> "1.0.0.0.0.0.0.0.0.0."
>                                                           
> "0.0.0.0.0.0.0.0.0.0."
>                                                           
> "0.0.0.0.0.0.0.0.0.0."
>                                                           "0.0.ip6.arpa"))
>                                   (class   "IN")
>                                   (type    "master")
>                                   (file    "localhost.ip6.zone"))
>                                  (bind-zone-configuration
>                                   (network "255.in-addr.arpa")
>                                   (class   "IN")
>                                   (type    "master")
>                                   (file    "empty.zone"))
>                                  (bind-zone-configuration
>                                   (network "0.in-addr.arpa")
>                                   (class   "IN")
>                                   (type    "master")
>                                   (file    "empty.zone"))
>                                  (bind-zone-configuration
>                                   (network ".")
>                                   (class   "IN")
>                                   (type    "master")
>                                   (file    "root.hint"))))))

What is the intended behavior of these defaults?  In what situations
will they work, and in what situations will they not?  It might be good
to put a comment in that explains the intended default behavior and why
it is reasonable.

> (define-record-type* <bind-configuration>
>   bind-configuration make-bind-configuration
>   bind-configuration?
>   (config-file bind-configuration-config-file
>                (default (bind-configuration-file)))
>   (package     bind-configuration-package ; <package>
>                (default bind)))
>
> (define-syntax option
>   (syntax-rules ()
>     ((_ key value) (if value
>                        (list "    " (string-join (list key value)) ";" "\n")
>                        '()))))

Does this need to be a macro?  By the way, you could use string-append
here, too, to make it simpler.

> (define-syntax key/value
>   (syntax-rules ()
>     ((_ (key value) rest ...)
>      (append (option key value)
>              (key/value rest ...)))
>     ((_) '())))

Does this need to be a macro?

> (define (emit-bind-zones-config zone)
>   (match zone
>     (($ <bind-zone-configuration> network class type file)
>      (list (string-join `(,(string-join (list "zone"
>                                               (string-append "\""
>                                                              network
>                                                              "\"")
>                                               class "{\n"))
>                           ,@(key/value ("type" type)
>                                        ("file" file))
>                           "};\n")
>                         "")))))
>
> (define (emit-bind-options-config options)
>   (match options
>     (($ <bind-options-configuration> user _ run-directory pid-file
>                                      listen-v4 listen-v6 listen-port
>                                      allow-recursion? allow-transfer?
>                                      allow-update?
>                                      version hostname server-id)

Some of these slots (e.g., listen-v4) appear to be un-used.  Instead of
listing positional slots by name, maybe it would be better to bind the
entire <bind-options-configuration> to a variable, and then use the
accessor procedures (e.g., bind-options-configuration-listen-v4) to get
just the attributes you need.  This has the benefit of being more
resilient to refactorings which change the order of fields in the
record, also.  I realize that a lot of the code in Guix relies on
positional matching of slots like this, so I don't mind if you keep it
as-is, but consider my suggestion as food for thought.

>      `("options {\n"
>        ,@(key/value ("directory" run-directory)
>                     ("pid-file" pid-file)
>                     ("allow-recursion"
>                      (bind-configuration-statement-string allow-recursion?))
>                     ("allow-transfer"
>                      (bind-configuration-statement-string allow-transfer?))
>                     ("allow-update"
>                      (bind-configuration-statement-string allow-update?))
>                     ("version" version)
>                     ("hostname" hostname)
>                     ("server-id" server-id))
>        "};\n"))))
>
> (define-gexp-compiler (bind-configuration-compiler
>                        (file <bind-configuration>) system target)
>   (match file
>     (($ <bind-configuration> config-file)
>      (match config-file
>        (($ <bind-configuration-file> config-options config-zones)
>         (apply text-file* "named.conf"
>                (append (fold append '() (map emit-bind-zones-config 
> config-zones))
>                        (emit-bind-options-config config-options))))))))
>

Is it necessary to define a gexp compiler here?  I would have thought we
could just invoke plain-file or text-file instead (see (guix)
G-Expressions in the Guix manual).  Why can't we?  Other services do
this; for example, see the service definitions in gnu/services/mail.scm.

Also, is it possible for a user to pass in an existing configuration
file to be used verbatim, or included somewhere in the config?  Having
an "escape hatch" like that seems useful for most services; perhaps it
could be useful here, too.

Footnotes: 
[1]  
https://unix.stackexchange.com/questions/13972/what-is-this-new-run-filesystem

-- 
Chris

signature.asc
Description: PGP signature