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

[Julien Lepiller]

On Fri, 13 Jan 2017 18:58:18 +0100
address@hidden (Ludovic Courtès) wrote:

> 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?

This should correct everything you reported.

> 
> 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?

I don't think you can connect a client to itself, but you could run two
machines, one client and one server. Is it possible?

> 
> Thank you!
> 
> Ludo’.

0001-gnu-Add-openvpn-service.patch
Description: Text Data