Re: [PATCH 2/2] gnu: Add openvpn service.

[ludo@gnu.org (Ludovic Courtès)]

Julien Lepiller <julien@lepiller.eu> skribis:

> * gnu/services/vpn.scm: New file.
> * gnu/local.mk (GNU_SYSTEM_MODULES): Add it.
> * doc/guix.texi (VPN Services): New section.

Woow, neat!

Overall LGTM!  The following comments are about cosmetic issues, but I
think it’s best to get them right.

> +@node VPN Services
> +@subsubsection VPN Services
> +@cindex VPN
> +
> +The @code{(gnu services vpn)} module provides the following sevices:

Please provide a few sentences of context and index entries, like:

  @cindex VPN (virtual private network)
  @cindex virtual private network (VPN)
  The @code{(gnu services vpn)} module provides services related to
  @dfn{virtual private networks} (VPNs).  It provides a @emph{client}
  service---allowing your machine to connect to a VPN, as well as
  @emph{server} functionality---where your machine hosts a VPN.  Both
  use @uref{https://openvpn.net/, OpenVPN}.

> +@deffn {Scheme Procedure} openvpn-client-service @
> +       [#:config (openvpn-client-configuration)]
> +
> +Return a service that runs @var{openvpn}, a VPN daemon, as a client.

@command{openvpn} (or @command{openvpnd}, whatever it’s called), or
OpenVPN.  @var is for variables.

> +@deffn {Scheme Procedure} openvpn-server-service @
> +       [#:config (openvpn-server-configuration)]
> +
> +Return a service that runs @var{openvpn}, a VPN daemon, as a server.

Ditto.

> +            openvpn-ccd-configuration
> +            generate-openvpn-client-documentation
> +            generate-openvpn-server-documentation
> +))

No hanging parens please.  :-)

> +(define (uglify-field-name name)
> +  (match name
> +         ('verbosity "verb")
> +         (_ (let ((str (symbol->string name)))
> +              (if (string-suffix? "?" str)
> +                  (substring str 0 (1- (string-length str)))
> +                  str)))))

The indentation is off in several places.  Could you run:

  ./etc/indent-code.el gnu/services/vpn.scm

(You need to have ‘emacs’ or ‘emacs-minimal’ installed but this is
non-interactive.)

> +(define (create-ccd-directory val)
> +  (let ((files (map (lambda (ccd)
> +                      (list (openvpn-ccd-configuration-name ccd)
> +                        (with-output-to-string
> +                          (lambda ()
> +                            (serialize-configuration
> +                              ccd openvpn-ccd-configuration-fields)))))

Please add a docstring.  It’s not obvious what’s happening here.

> +                    val)))
> +    (computed-file "ccd"
> +      (with-imported-modules '((guix build utils))
> +        #~(begin
> +          (use-modules (guix build utils))
> +          (mkdir-p #$output)
> +            (for-each
> +              (lambda (ccd)
> +                (call-with-output-file (string-append #$output "/" (car ccd))
> +                  (lambda (port) (display (car (cdr ccd)) port))))
> +              '#$files))))))

Please use ‘match’ instead of (car (cdr ccd)):

  (lambda (port)
    (match ccd
      ((_ (thing _ ...))
       (display thing port))))

Of course you can use an identifier more descriptive than ‘thing’.  ;-)

> +   (server-ipv6
> +     (cidr6 #f)
> +     "A CIDR notation specifying the ipv6 subnet inside the virtual network.")

It would be ideal if you could expand “CIDR” the first type.

Also, in comments and docstrings in the whole file (and thus in
guix.texi):

  s/ipv6/IPv6/
  s/udp/UDP/
  s/tcp/TCP/
  s/diffie-hellman/Diffie-Hellman/
  s/Openvpn/OpenVPN/

> +(define (openvpn-shepherd-service role)
> +  (lambda (config)
> +    (let* ((config-file (openvpn-config-file role config))
> +           ;                #$(serialize-configuration config
> +           ;                    (match role
> +           ;                      ('server openvpn-server-configuration-fields)
> +           ;                      ('client openvpn-client-configuration-fields)))))

Please remove the comment.

> +              (start #~(make-forkexec-constructor
> +                         (list (string-append #$openvpn "/sbin/openvpn")
> +                               "--writepid" #$pid-file "--config" #$config-file
> +                               "--daemon")))

Add #:pid-file #$pid-file, so that shepherd does the right thing.

> +(define %openvpn-activation
> +  #~(begin
> +      (mkdir-p "/var/run/openvpn")))

‘begin’ can be omitted.

Could you send an updated patch?

Besides, could you think of a system test that would allow us to test
both services?  Perhaps a single config that has both the OpenVPN server
and client running?  Thoughts?

Thank you!

Ludo’.