bug#26716: Test nginx configuration

[Julien Lepiller]

Le Sun, 30 Apr 2017 10:29:59 -0500,
Christopher Allan Webber <address@hidden> 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 <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.
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 <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?
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

0001-gnu-services-nginx-Test-certificate-presence.patch
Description: Text Data

0002-gnu-services-Create-logs-directory.patch
Description: Text Data