bug#26716: Test nginx configuration

[Christopher Allan Webber]

Julien Lepiller writes:

> Hi, here are two patches to react to Christopher's experience. I added
> two simple tests that check the presence of the certificate and the key
> passed to nginx configuration.
>
> If the error log file cannot be created at startup, error messages
> about the configuration file are logged only on stderr. The second
> patch makes sure the log file can be created.

Cool!

> From 53f98d79c5888f402ae8698ce61433e67f9b6015 Mon Sep 17 00:00:00 2001
> From: Julien Lepiller <address@hidden>
> Date: Sun, 30 Apr 2017 11:17:02 +0200
> Subject: [PATCH 1/2] gnu: services: nginx: Test certificate presence.
>
> * gnu/services/web.scm (default-nginx-server-config): Test certificate
> presence when https is requested at configure time.
> ---
>  gnu/services/web.scm | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/gnu/services/web.scm b/gnu/services/web.scm
> index b7b2f67f1..a13534c84 100644
> --- a/gnu/services/web.scm
> +++ b/gnu/services/web.scm
> @@ -2,7 +2,7 @@
>  ;;; Copyright © 2015 David Thompson <address@hidden>
>  ;;; Copyright © 2015, 2016, 2017 Ludovic Courtès <address@hidden>
>  ;;; Copyright © 2016 ng0 <address@hidden>
> -;;; Copyright © 2016 Julien Lepiller <address@hidden>
> +;;; Copyright © 2016, 2017 Julien Lepiller <address@hidden>
>  ;;; Copyright © 2017 Christopher Baines <address@hidden>
>  ;;;
>  ;;; This file is part of GNU Guix.
> @@ -154,12 +154,14 @@ of index files."
>                           (nginx-server-configuration-server-name server))
>                          ";\n"
>     (if (nginx-server-configuration-ssl-certificate server)
> -       (string-append "      ssl_certificate "
> -                      (nginx-server-configuration-ssl-certificate server) 
> ";\n")
> +       (let ((certificate (nginx-server-configuration-ssl-certificate 
> server)))
> +         (lstat certificate)
> +         (string-append "      ssl_certificate " certificate ";\n"))
>         "")

So is the goal here that it will raise an exception if it doesn't exist,
like so?

  ERROR: In procedure lstat: No such file or directory: "/tmp/no-such-file"

That does seem like useful information to spit out.

Maybe add a comment before the lstat explaining the call?  If I didn't
know that's why lstat was being used here I would have been confused.

>     (if (nginx-server-configuration-ssl-certificate-key server)
> -       (string-append "      ssl_certificate_key "
> -                      (nginx-server-configuration-ssl-certificate-key 
> server) ";\n")
> +       (let ((key (nginx-server-configuration-ssl-certificate-key server)))
> +         (lstat certificate)
> +         (string-append "      ssl_certificate_key " key ";\n"))
>         "")
>     "      root " (nginx-server-configuration-root server) ";\n"
>     "      index " (config-index-strings (nginx-server-configuration-index 
> server)) ";\n"
> --
> 2.12.2
>
>>From 85de5d18aec10900accd146746ea72902a6147dc Mon Sep 17 00:00:00 2001
> From: Julien Lepiller <address@hidden>
> Date: Sun, 30 Apr 2017 11:51:12 +0200
> Subject: [PATCH 2/2] gnu: services: Create logs directory.
>
> * gnu/services/web.scm (nginx-activation): Create logs directory so nginx can
> log its startup messages before it loads its configuration.
> ---
>  doc/guix.texi        | 9 +++++++++
>  gnu/services/web.scm | 3 +++
>  2 files changed, 12 insertions(+)
>
> diff --git a/doc/guix.texi b/doc/guix.texi
> index 0d334e302..957ce2bab 100644
> --- a/doc/guix.texi
> +++ b/doc/guix.texi
> @@ -13316,6 +13316,15 @@ used to specify the list of @dfn{server blocks} 
> required on the host and
>  blocks} to configure.  For this to work, use the default value for
>  @var{config-file}.
>
> +At startup, @command{nginx} has not yet read its configuration file, so it
> +uses a default file to log error messages.  If it fails to load its
> +configuration file, that is where error messages are logged.  After the
> +configuration file is loaded, the default error log file changes as per
> +configuration.  In our case, startup error messages can be found in
> address@hidden/var/run/nginx/logs/error.log}, and after configuration in
> address@hidden/var/log/nginx/error.log}.  The second location can be changed 
> with the
> address@hidden configuration option.
> +
>  @end deffn
>
>  @deffn {Scheme Variable} nginx-service-type
> diff --git a/gnu/services/web.scm b/gnu/services/web.scm
> index a13534c84..0c9d31043 100644
> --- a/gnu/services/web.scm
> +++ b/gnu/services/web.scm
> @@ -235,6 +235,9 @@ of index files."
>           (mkdir-p (string-append #$run-directory "/fastcgi_temp"))
>           (mkdir-p (string-append #$run-directory "/uwsgi_temp"))
>           (mkdir-p (string-append #$run-directory "/scgi_temp"))
> +         ;; Start-up logs. Once configuration is loaded, nginx switches to
> +         ;; log-directory.
> +         (mkdir-p (string-append #$run-directory "/logs"))
>           ;; Check configuration file syntax.
>           (system* (string-append #$nginx "/sbin/nginx")
>                    "-c" #$(or config-file

Oh, that's interesting.  So in my experience earlier, it was proably
*trying* to log some information, and failing I guess.

It would be even nicer if they wrote to the same file by default, but I
guess this probably isn't easy to do without actually patching nginx
itself, which isn't likely worth it... is that right?

With the comment issue resolved, and assuming there's no clean way to
get nginx to write to the same error file we normally use by default, it
seems good to me!

 - Chris