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

[Ludovic Courtès]

Julien Lepiller <address@hidden> 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.

> address@hidden VPN Services
> address@hidden VPN Services
> address@hidden 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}.

> address@hidden {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.

> address@hidden {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’.