diff mbox

improve nginx-service

Message ID 20161027195949.354cae8e@lepiller.eu
State New
Headers show

Commit Message

Julien Lepiller Oct. 27, 2016, 5:59 p.m. UTC
On Thu, 27 Oct 2016 14:41:18 +0200
ludo@gnu.org (Ludovic Courtès) wrote:

> [...]
>
> What I had in mind was just to add ‘compose’ and ‘extend’ to
> ‘nginx-service-type’ (this is where we define how extensions are
> handled).  There’s a bit of extra bookeeping to do, in particular
> moving the list of vhosts to <nginx-configuration>, as in this
> untested patch:
> 

What do you think of this patch? I only had to add one (or) so the same
config file would be used during test and running (I hope it does not
create another one?).

Actually your patch didn't help me except for syntax simplification.
Your answer was not really helpful, but it forced me to consider what I
really wanted. So my use case would be that I would like to run a
webservice from the store (all configuration should be made via guix).
But my problem is that I don't know how to get the path to the store
that I could pass as the root parameter in nginx-vhost-config.

Also, I don't know what the best solution would be to get a configured
web service in the store. Configuration is usually in a file in the
root directory, and sometimes a default file is already present and you
are supposed to modify it by hand. The issue here is that the store is
read-only. How could you create a configuration file that can be found
in the package's directory (using a guix service)? Would that mean that
I have to copy the whole package and change just one file? Is there
anything better, or do I have no other choice than install it outside of
guix?

Comments

Ludovic Courtès Oct. 30, 2016, 9:46 p.m. UTC | #1
Julien Lepiller <julien@lepiller.eu> skribis:

> On Thu, 27 Oct 2016 14:41:18 +0200
> ludo@gnu.org (Ludovic Courtès) wrote:
>
>> [...]
>>
>> What I had in mind was just to add ‘compose’ and ‘extend’ to
>> ‘nginx-service-type’ (this is where we define how extensions are
>> handled).  There’s a bit of extra bookeeping to do, in particular
>> moving the list of vhosts to <nginx-configuration>, as in this
>> untested patch:
>> 

[...]

> Actually your patch didn't help me except for syntax simplification.
> Your answer was not really helpful,

Fair enough.  :-)

> but it forced me to consider what I really wanted. So my use case
> would be that I would like to run a webservice from the store (all
> configuration should be made via guix).  But my problem is that I
> don't know how to get the path to the store that I could pass as the
> root parameter in nginx-vhost-config.

OK, that doesn’t have much to do with making nginx-service-type
extensible.  But we can do both!

If the vhost’s root directory is immutable, you can of course add it to
the store via ‘computed-file’ or similar, and then write:

  (nginx-vhost-configuration
    (root #$(computed-file "root" …)))

If it’s mutable, as is often going to be the case, you can simply pass
its file name:

  (nginx-vhost-configuration
    (root "/var/www/the-service"))

In that case, it’s up to you to make sure that /var/www/the-service
exists and contains the right thing.

Does that answer your question?

> Also, I don't know what the best solution would be to get a configured
> web service in the store. Configuration is usually in a file in the
> root directory, and sometimes a default file is already present and you
> are supposed to modify it by hand. The issue here is that the store is
> read-only. How could you create a configuration file that can be found
> in the package's directory (using a guix service)? Would that mean that
> I have to copy the whole package and change just one file? Is there
> anything better, or do I have no other choice than install it outside of
> guix?

All the daemons (almost all of them) started on GuixSD get their config
file from the store.  When you wrote:

  #~(system* #$(file-append nginx "/bin/nginx") "-c" #$config-file)

that translated to:

  (system* "/gnu/store/…/bin/nginx" "-c" "/gnu/store/…-config")

where /gnu/store/…-config is a file that was generated using
‘computed-file’ or similar.

I think the same approach should work for a Web service, but without
being more specific I can’t tell.

> From 25a296057969a35b86ea7371577504c43bf96334 Mon Sep 17 00:00:00 2001
> From: Julien Lepiller <julien@lepiller.eu>
> Date: Thu, 27 Oct 2016 19:47:27 +0200
> Subject: [PATCH] service: Make nginx-service extensible
>
> gnu/services/web.scm (nginx-service-type): Make extensible

[...]

> +@deffn [Scheme Variable] nginx-service-type
          ^
Should be braces.

> +This is the type for the @code{nginx} web server.

“nginx” (the proper name, not the command name.)

>    (nginx         nginx-configuration-nginx)         ;<package>
>    (log-directory nginx-configuration-log-directory) ;string
>    (run-directory nginx-configuration-run-directory) ;string
> +  (vhosts        nginx-configuration-vhosts)        ;list of <nginx-vhost-configuration>

Please add a default value like at
<https://lists.gnu.org/archive/html/guix-devel/2016-10/msg01380.html>.

I hindsight, I wonder why I put that one-element list as the default;
shouldn’t it be the empty list?

>    (file          nginx-configuration-file))         ;string | file-like
>  
>  (define (config-domain-strings names)
> @@ -102,11 +104,13 @@ of index files."
>     "      server_name " (config-domain-strings
>                           (nginx-vhost-configuration-server-name vhost))
>                          ";\n"
> -   (if (nginx-vhost-configuration-ssl-certificate vhost)
> +   (if (and (nginx-vhost-configuration-https-port vhost)
> +            (nginx-vhost-configuration-ssl-certificate vhost))
>         (string-append "      ssl_certificate "
>                        (nginx-vhost-configuration-ssl-certificate vhost) ";\n")
>         "")
> -   (if (nginx-vhost-configuration-ssl-certificate-key vhost)
> +   (if (and (nginx-vhost-configuration-https-port vhost)
> +            (nginx-vhost-configuration-ssl-certificate-key vhost))

Could you remove these changes?  I think they are unrelated.

>  (define* (nginx-service #:key (nginx nginx)
>                          (log-directory "/var/log/nginx")
>                          (run-directory "/var/run/nginx")
> -                        (vhost-list (list (nginx-vhost-configuration)))
> -                        (config-file
> -                         (default-nginx-config log-directory run-directory vhost-list)))
> +                        (vhost-list (list (nginx-vhost-configuration
> +                                            (https-port #f))))

Please remove the ‘https-port’ setting.

Apart from that it LGTM.  Could you send an updated patch?

Thanks!

Ludo’.
Hartmut Goebel Nov. 2, 2016, 8:22 a.m. UTC | #2
Hi,
> If the vhost’s root directory is immutable, you can of course add it to
> the store via ‘computed-file’ or similar, and then write:
>
>   (nginx-vhost-configuration
>     (root #$(computed-file "root" …)))
Related question: I've but the content of some example website into a
package. How can I use that package's directory as "root"? I tried
these, but none worked:

   (root (package-file demo-website))
   (root #$(file-append demo-website "/"))
   (root (assoc-ref demo-website "out"))
   (root #$demo-website)
Ludovic Courtès Nov. 3, 2016, 2:54 p.m. UTC | #3
Hartmut Goebel <h.goebel@crazy-compilers.com> skribis:

> Hi,
>> If the vhost’s root directory is immutable, you can of course add it to
>> the store via ‘computed-file’ or similar, and then write:
>>
>>   (nginx-vhost-configuration
>>     (root #$(computed-file "root" …)))

Oops, there shouldn’t be a #$ here since we’re not inside #~.

> Related question: I've but the content of some example website into a
> package. How can I use that package's directory as "root"? I tried
> these, but none worked:
>
>    (root (package-file demo-website))
>    (root #$(file-append demo-website "/"))
>    (root (assoc-ref demo-website "out"))
>    (root #$demo-website)

Assuming ‘demo-website’ is bound to a package object, you should be able
to do:

  (root (file-append demo-website "/"))

or simply:

  (root demo-website)

The #$ above are invalid since #$ is meant to be used within #~.
Likewise, the first argument to ‘assoc-ref’ must be an association list,
which ‘demo-website’ is not.  As for ‘package-file’, it’s not enough: it
returns the file name of the package, but doesn’t make the package a
dependency of the vhost config.

HTH!

Ludo’.
Hartmut Goebel Nov. 3, 2016, 10:38 p.m. UTC | #4
Am 03.11.2016 um 15:54 schrieb Ludovic Courtès:
> Assuming ‘demo-website’ is bound to a package object, you should be able
> to do:
>
>   (root (file-append demo-website "/"))
>
> or simply:
>
>   (root demo-website)

Hmm, this does not work for me:


ice-9/boot-9.scm:702:27: In procedure map:
ice-9/boot-9.scm:702:27: In procedure string-append: Wrong type
(expecting string): #<package taler-demo-landing-page@0.0.0-1.105bea68
./taler/packages/demo.scm:100 3e2a600>


I tried with current master branch.
Ludovic Courtès Nov. 4, 2016, 1:21 p.m. UTC | #5
Hartmut Goebel <h.goebel@crazy-compilers.com> skribis:

> Am 03.11.2016 um 15:54 schrieb Ludovic Courtès:
>> Assuming ‘demo-website’ is bound to a package object, you should be able
>> to do:
>>
>>   (root (file-append demo-website "/"))
>>
>> or simply:
>>
>>   (root demo-website)
>
> Hmm, this does not work for me:
>
>
> ice-9/boot-9.scm:702:27: In procedure map:
> ice-9/boot-9.scm:702:27: In procedure string-append: Wrong type
> (expecting string): #<package taler-demo-landing-page@0.0.0-1.105bea68
> ./taler/packages/demo.scm:100 3e2a600>
>
>
> I tried with current master branch.

Doh!  Indeed, ‘default-nginx-vhost-config’ must be rewritten to do
something like:

  #~(string-append #$foo #$bar …)

instead of:

  (string-append foo bar …)

More precisely, it could ‘string-append’ all the string literals yet use
a #~(string-append …) gexp for ‘root’.

Julien?  :-)

Ludo’.
diff mbox

Patch

From 25a296057969a35b86ea7371577504c43bf96334 Mon Sep 17 00:00:00 2001
From: Julien Lepiller <julien@lepiller.eu>
Date: Thu, 27 Oct 2016 19:47:27 +0200
Subject: [PATCH] service: Make nginx-service extensible

gnu/services/web.scm (nginx-service-type): Make extensible
---
 doc/guix.texi        | 15 ++++++++++++++-
 gnu/services/web.scm | 39 +++++++++++++++++++++++++++++----------
 2 files changed, 43 insertions(+), 11 deletions(-)

diff --git a/doc/guix.texi b/doc/guix.texi
index 1293b8b..b6e62b3 100644
--- a/doc/guix.texi
+++ b/doc/guix.texi
@@ -10399,7 +10399,7 @@  The @code{(gnu services web)} module provides the following service:
        [#:log-directory ``/var/log/nginx''] @
        [#:run-directory ``/var/run/nginx''] @
        [#:vhost-list (list (nginx-vhost-configuration))] @
-       [#:config-file]
+       [#:config-file @code{#f}]
 
 Return a service that runs @var{nginx}, the nginx web server.
 
@@ -10415,6 +10415,19 @@  this to work, use the default value for @var{config-file}.
 
 @end deffn
 
+@deffn [Scheme Variable] nginx-service-type
+This is the type for the @code{nginx} web server.
+
+This service can be extended to add more vhosts than the default one.
+
+@example
+(simple-service 'my-extra-vhost nginx-service-type
+                (list (nginx-vhost-configuration (https-port #f)
+                                                 (root "/var/www/extra-website"))))
+@end example
+
+@end deffn
+
 @deftp {Data Type} nginx-vhost-configuration
 Data type representing the configuration of an nginx virtual host.
 This type has the following parameters:
diff --git a/gnu/services/web.scm b/gnu/services/web.scm
index 59e1e54..0fb1a0b 100644
--- a/gnu/services/web.scm
+++ b/gnu/services/web.scm
@@ -28,6 +28,7 @@ 
   #:use-module (guix records)
   #:use-module (guix gexp)
   #:use-module (ice-9 match)
+  #:use-module (srfi srfi-1)
   #:export (nginx-configuration
             nginx-configuration?
             nginx-vhost-configuration
@@ -67,6 +68,7 @@ 
   (nginx         nginx-configuration-nginx)         ;<package>
   (log-directory nginx-configuration-log-directory) ;string
   (run-directory nginx-configuration-run-directory) ;string
+  (vhosts        nginx-configuration-vhosts)        ;list of <nginx-vhost-configuration>
   (file          nginx-configuration-file))         ;string | file-like
 
 (define (config-domain-strings names)
@@ -102,11 +104,13 @@  of index files."
    "      server_name " (config-domain-strings
                          (nginx-vhost-configuration-server-name vhost))
                         ";\n"
-   (if (nginx-vhost-configuration-ssl-certificate vhost)
+   (if (and (nginx-vhost-configuration-https-port vhost)
+            (nginx-vhost-configuration-ssl-certificate vhost))
        (string-append "      ssl_certificate "
                       (nginx-vhost-configuration-ssl-certificate vhost) ";\n")
        "")
-   (if (nginx-vhost-configuration-ssl-certificate-key vhost)
+   (if (and (nginx-vhost-configuration-https-port vhost)
+            (nginx-vhost-configuration-ssl-certificate-key vhost))
        (string-append "      ssl_certificate_key "
                       (nginx-vhost-configuration-ssl-certificate-key vhost) ";\n")
        "")
@@ -148,7 +152,7 @@  of index files."
 
 (define nginx-activation
   (match-lambda
-    (($ <nginx-configuration> nginx log-directory run-directory config-file)
+    (($ <nginx-configuration> nginx log-directory run-directory vhosts config-file)
      #~(begin
          (use-modules (guix build utils))
 
@@ -164,17 +168,25 @@  of index files."
          (mkdir-p (string-append #$run-directory "/scgi_temp"))
          ;; Check configuration file syntax.
          (system* (string-append #$nginx "/sbin/nginx")
-                  "-c" #$config-file "-t")))))
+                   "-c" #$(or config-file
+                              (default-nginx-config log-directory
+                                run-directory vhosts))
+                   "-t")))))
 
 (define nginx-shepherd-service
   (match-lambda
-    (($ <nginx-configuration> nginx log-directory run-directory config-file)
+    (($ <nginx-configuration> nginx log-directory run-directory vhosts config-file)
      (let* ((nginx-binary (file-append nginx "/sbin/nginx"))
             (nginx-action
              (lambda args
                #~(lambda _
                    (zero?
-                    (system* #$nginx-binary "-c" #$config-file #$@args))))))
+                    (system* #$nginx-binary "-c" #$(or config-file
+                                                       (default-nginx-config
+                                                         log-directory
+                                                         run-directory
+                                                         vhosts))
+                             #$@args))))))
 
        ;; TODO: Add 'reload' action.
        (list (shepherd-service
@@ -192,14 +204,20 @@  of index files."
                        (service-extension activation-service-type
                                           nginx-activation)
                        (service-extension account-service-type
-                                          (const %nginx-accounts))))))
+                                          (const %nginx-accounts))))
+                (compose concatenate)
+                (extend (lambda (config vhosts)
+                          (nginx-configuration
+                            (inherit config)
+                            (vhosts (append (nginx-configuration-vhosts config)
+                                            vhosts)))))))
 
 (define* (nginx-service #:key (nginx nginx)
                         (log-directory "/var/log/nginx")
                         (run-directory "/var/run/nginx")
-                        (vhost-list (list (nginx-vhost-configuration)))
-                        (config-file
-                         (default-nginx-config log-directory run-directory vhost-list)))
+                        (vhost-list (list (nginx-vhost-configuration
+                                            (https-port #f))))
+                        (config-file #f))
   "Return a service that runs NGINX, the nginx web server.
 
 The nginx daemon loads its runtime configuration from CONFIG-FILE, stores log
@@ -209,4 +227,5 @@  files in LOG-DIRECTORY, and stores temporary runtime files in RUN-DIRECTORY."
             (nginx nginx)
             (log-directory log-directory)
             (run-directory run-directory)
+            (vhosts vhost-list)
             (file config-file))))
-- 
2.10.1