bug#26716: Test nginx configuration

[Julien Lepiller <julien@lepiller.eu>]

[-- Attachment #1: Type: text/plain, Size: 6486 bytes --]

Le Sun, 30 Apr 2017 10:29:59 -0500,
Christopher Allan Webber <cwebber@dustycloud.org> a écrit :

> 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 <julien@lepiller.eu>
> > 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 <davet@gnu.org>
> >  ;;; Copyright © 2015, 2016, 2017 Ludovic Courtès <ludo@gnu.org>
> >  ;;; Copyright © 2016 ng0 <ng0@we.make.ritual.n0.is>
> > -;;; Copyright © 2016 Julien Lepiller <julien@lepiller.eu>
> > +;;; Copyright © 2016, 2017 Julien Lepiller <julien@lepiller.eu>
> >  ;;; Copyright © 2017 Christopher Baines <mail@cbaines.net>
> >  ;;;
> >  ;;; 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.
exactly, I added a comment.

> 
> >     (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 <julien@lepiller.eu>
> > 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
> > +@file{/var/run/nginx/logs/error.log}, and after configuration in
> > +@file{/var/log/nginx/error.log}.  The second location can be
> > changed with the +@var{log-directory} 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?
I tried using the -g option to give it some configuration options
(including error_log), but it doesn't seem to change that behaviour, so
I think we'll have to fix nginx to use the same configuration file.

Of course it would be better to fail at reconfigure when the generated
configuration is not correct. That's what I'm trying to do with the
first patch, but that's only one possible mistake.

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


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-gnu-services-nginx-Test-certificate-presence.patch --]
[-- Type: text/x-patch, Size: 2269 bytes --]

From 562bb322253161b5f3b64aa46613cef8fac77292 Mon Sep 17 00:00:00 2001
From: Julien Lepiller <julien@lepiller.eu>
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 | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/gnu/services/web.scm b/gnu/services/web.scm
index b7b2f67f1..47036f42f 100644
--- a/gnu/services/web.scm
+++ b/gnu/services/web.scm
@@ -2,7 +2,7 @@
 ;;; Copyright © 2015 David Thompson <davet@gnu.org>
 ;;; Copyright © 2015, 2016, 2017 Ludovic Courtès <ludo@gnu.org>
 ;;; Copyright © 2016 ng0 <ng0@we.make.ritual.n0.is>
-;;; Copyright © 2016 Julien Lepiller <julien@lepiller.eu>
+;;; Copyright © 2016, 2017 Julien Lepiller <julien@lepiller.eu>
 ;;; Copyright © 2017 Christopher Baines <mail@cbaines.net>
 ;;;
 ;;; This file is part of GNU Guix.
@@ -154,12 +154,16 @@ 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 fails when the certificate file does not exist: it aborts
+         ;; and lets the user fix their configuration.
+         (lstat certificate)
+         (string-append "      ssl_certificate " certificate ";\n"))
        "")
    (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


[-- Attachment #3: 0002-gnu-services-Create-logs-directory.patch --]
[-- Type: text/x-patch, Size: 2072 bytes --]

From a3973400ef1d89eebf42a8f839f7152aa43c5539 Mon Sep 17 00:00:00 2001
From: Julien Lepiller <julien@lepiller.eu>
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
+@file{/var/run/nginx/logs/error.log}, and after configuration in
+@file{/var/log/nginx/error.log}.  The second location can be changed with the
+@var{log-directory} configuration option.
+
 @end deffn
 
 @deffn {Scheme Variable} nginx-service-type
diff --git a/gnu/services/web.scm b/gnu/services/web.scm
index 47036f42f..9f789707e 100644
--- a/gnu/services/web.scm
+++ b/gnu/services/web.scm
@@ -237,6 +237,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
-- 
2.12.2