[bug#28769] [PATCH] gnu: services: Add php-fpm.

[Christopher Baines <mail@cbaines.net>]

[-- Attachment #1.1: Type: text/plain, Size: 6700 bytes --]

On Mon, 16 Oct 2017 23:38:50 +0200
nee <nee@cock.li> wrote:

I've also read your subsequent message about fixing the log files, but
I'll reply here, as there is more to reply to.

> Hello, here is an updated version.

...

> > Also, the idea that came to my mind for this is to add php-fpm as a
> > supplementary group for the nginx user. In my mind, this seems the
> > neatest way of giving nginx access to the socket. What do you think?
> > 
> > I suggest this, as I'm not sure the name of the www-data group does a
> > good job of describing that it's controlling access to a socket. Also,
> > giving nginx explicit access to things owned by the php-fpm group could
> > be more secure than using a more generic group, especially as other
> > services that might need access to the socket, could end up getting it
> > because they need access to other things using the www-data group.
> >   
> Using more specialized groups sounds good in general, I changed the
> group name.
> It would be weird to have the nginx user in a bunch of groups for
> software that you did not install though.
> > If the above sounds like a good idea, I think it could be implemented
> > by adding a configurable list of supplementary groups to the
> > nginx-service-type. Then maybe even adding support for configuring this
> > via the service extensions mechanism, then having the
> > php-fpm-service-type extending the nginx-service-type...
> > 
> > The above is definitely to complicated to do all in one go, especially
> > since the nginx-service-type doesn't support anything more complicated
> > than adding server blocks through extension at the moment.
> >   
> This seems to be a solution.

I've now attached a system test for php-fpm, that sets it up with
nginx, creates a basic php file, and checks that it can be requested.

This seems to work, which I was a little surprised at, as I was
suspecting problems with the socket permissions.

I had a look, and while the nginx workers in the test system are not
root, the nginx master process is, so maybe that allows it to work...

Anyway, providing that the test reflects how the service might be used,
it seems like this is a non-issue for now.

> >> @@ -385,3 +397,160 @@ of index files."
> >>  		       (service-extension account-service-type
> >>                                            fcgiwrap-accounts)))
> >>                  (default-value (fcgiwrap-configuration))))

...

> >> +                                         (version-major
> >> (package-version php))
> >> +                                         "-fpm.log")))  
> > 
> > I'm not sure the php is adding much to the log file name, but then
> > again I've never used php... what do you think?
> >   
> For the workers-logfile:
> This is the logfile for the php workers error's
> It only logs errors that were printed with error_log by a php worker
> process.
> Try this example file:
> 
> <?php
> error_log("some error\n"); // should be in the log
> echo "Hello from php"; // should be in the browser
> 
> Then visit the err.php file with curl or a browser using the nginx
> example above.
> 
> Also:
> I forgot to add a setting for the log-file of the php-fpm master
> process. This file should log messages when php-fpm has started or stopped.
> 
> For some reason it stays empty now. I had it log stuff before, when I
> manually killed the process, but I can't reproduce it anymore. I changed
> the permissions to ugo+wrx and it still stays empty. I'm not sure what's
> going on.
> 
> I renamed the default workers-log-file to php7-fpm.www.log as it is
> usually called. The php-fpm log-file is now called php7-fpm.log.

What I think I meant to say here was, I'm not sure the php _version_ is
adding much to the log file name (rather than "I'm not sure the php is
adding").

The php package version is used in a few places, and while I can
imagine this being consistent with other distributions, it does add a
bit of complexity to the default values in the service, and I'm not
sure what benefit it brings?

> >> +  (file          php-fpm-configuration-file ;#f | file-like
> >> +                 (default #f)))

...

> >> +                       (service-extension account-service-type
> >> +                                          php-fpm-accounts)))
> >> +                (default-value (php-fpm-configuration))))  
> > 
> > Filling in the description (a relatively new field on the service type)
> > would be a great addition here.
> >   
> Ah, yes I'm mostly using the web documentation and other services from
> web.scm as reference. Thanks for the update.
> What would be a good value for this field? I just used "The php-fpm
> service-type." for now.

That is ok, but it could probably be more useful. I think ideally it
would describe more about what the service offers.

Users might encounter this when searching for services for example:

→ guix system search php
name: php-fpm
location: gnu/services/web.scm:607:2
extends: shepherd-root activate account
description: The php-fpm service-type.
relevance: 5


> >> +(define* (nginx-php-location
> >> +          #:key
> >> +          (nginx-package nginx)

...

> > Overall, I think this is great. Excellent use of record types, gexps
> > and match expressions. I think it would be good to think more on the
> > accounts issue before merging, but that is all that comes to mind
> > currently.
> >   
> Good to hear, thank you! :)
> 
> > Also, if you feel like it, as service test would be a great addition to
> > this patch. There is a test for nginx already in gnu/tests/web.scm, and
> > I think you could get most of the benefit by having a test for nginx
> > with php-fpm, as that would give you some coverage over the php-fpm
> > service, as well as the nginx configuration.
> >   
> That sounds great I love the idea of system tests and will have a look
> at it later.
> 
> I also added a line to copyright headers of each file. Except for the
> utils.scm file, because that change is trivial copy pasta.

I've attached a patch containing a couple of copy+paste fixes, and a
system test.

It would be good to get your opinion on the system test, does it test
the right things?

If you like the look of it, I'd suggest including those changes in the
commit that adds the service, and then sending the updated patches.
I've included a changelog in the commit message.

Now that there is a system test, and the php-fpm service seems to work
fine with the nginx service, I think that means that doing anything
extra with accounts can be done later.



[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.2: 0001-Add-fixes-and-system-test-for-PHP-FPM.patch --]
[-- Type: text/x-patch, Size: 5779 bytes --]

From cb37c6e0353cebcdab81280a15d618e8b3ccd631 Mon Sep 17 00:00:00 2001
From: Christopher Baines <mail@cbaines.net>
Date: Thu, 2 Nov 2017 18:39:21 +0000
Subject: [PATCH] Add fixes and system test for PHP-FPM

Probably best to fixup this in to the commit that adds the service.

* gnu/tests/web.scm (%make-php-fpm-http-root, %php-fpm-nginx-server-blocks,
  %php-fpm-os, %test-php-fpm): New variables.
  (run-php-fpm-test): New procedure.
---
 gnu/services/web.scm |   4 +-
 gnu/tests/web.scm    | 109 ++++++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 110 insertions(+), 3 deletions(-)

diff --git a/gnu/services/web.scm b/gnu/services/web.scm
index d30f3af2a..168c23cce 100644
--- a/gnu/services/web.scm
+++ b/gnu/services/web.scm
@@ -114,7 +114,7 @@
             php-fpm-static-process-manager-configuration?
             php-fpm-static-process-manager-configuration-max-children
             
-            <php-fpm-static-process-manager-configuration>
+            <php-fpm-on-demand-process-manager-configuration>
             php-fpm-on-demand-process-manager-configuration
             make-php-fpm-on-demand-process-manager-configuration
             php-fpm-on-demand-process-manager-configuration?
@@ -490,7 +490,7 @@ of index files."
   (max-children         php-fpm-static-process-manager-configuration-max-children
                         (default 5)))
 
-(define-record-type* <php-fpm-static-process-manager-configuration>
+(define-record-type* <php-fpm-on-demand-process-manager-configuration>
   php-fpm-on-demand-process-manager-configuration
   make-php-fpm-on-demand-process-manager-configuration
   php-fpm-on-demand-process-manager-configuration?
diff --git a/gnu/tests/web.scm b/gnu/tests/web.scm
index 3fa272c67..bf8435711 100644
--- a/gnu/tests/web.scm
+++ b/gnu/tests/web.scm
@@ -1,5 +1,6 @@
 ;;; GNU Guix --- Functional package management for GNU
 ;;; Copyright © 2017 Ludovic Courtès <ludo@gnu.org>
+;;; Copyright © 2017 Christopher Baines <mail@cbaines.net>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -27,7 +28,8 @@
   #:use-module (gnu services networking)
   #:use-module (guix gexp)
   #:use-module (guix store)
-  #:export (%test-nginx))
+  #:export (%test-nginx
+            %test-php-fpm))
 
 (define %index.html-contents
   ;; Contents of the /index.html file served by nginx.
@@ -132,3 +134,108 @@ HTTP-PORT."
    (name "nginx")
    (description "Connect to a running NGINX server.")
    (value (run-nginx-test))))
+
+\f
+;;;
+;;; PHP-FPM
+;;;
+
+(define %make-php-fpm-http-root
+  ;; Create our server root in /srv.
+  #~(begin
+      (mkdir "/srv")
+      (call-with-output-file "/srv/index.php"
+        (lambda (port)
+          (display "<?php phpinfo(); ?>\n" port)))))
+
+(define %php-fpm-nginx-server-blocks
+  (list (nginx-server-configuration
+         (root "/srv")
+         (locations
+          (list (nginx-php-location)))
+         (http-port 8042)
+         (https-port #f)
+         (ssl-certificate #f)
+         (ssl-certificate-key #f))))
+
+(define %php-fpm-os
+  ;; Operating system under test.
+  (simple-operating-system
+   (dhcp-client-service)
+   (service php-fpm-service-type)
+   (service nginx-service-type
+            (nginx-configuration
+             (server-blocks %php-fpm-nginx-server-blocks)))
+   (simple-service 'make-http-root activation-service-type
+                   %make-php-fpm-http-root)))
+
+(define* (run-php-fpm-test #:optional (http-port 8042))
+  "Run tests in %PHP-FPM-OS, which has nginx running and listening on
+HTTP-PORT, along with php-fpm."
+  (define os
+    (marionette-operating-system
+     %php-fpm-os
+     #:imported-modules '((gnu services herd)
+                          (guix combinators))))
+
+  (define vm
+    (virtual-machine
+     (operating-system os)
+     (port-forwardings `((8080 . ,http-port)))))
+
+  (define test
+    (with-imported-modules '((gnu build marionette)
+                             (guix build utils))
+      #~(begin
+          (use-modules (srfi srfi-11) (srfi srfi-64)
+                       (gnu build marionette)
+                       (web uri)
+                       (web client)
+                       (web response))
+
+          (define marionette
+            (make-marionette (list #$vm)))
+
+          (mkdir #$output)
+          (chdir #$output)
+
+          (test-begin "php-fpm")
+
+          (test-assert "php-fpm running"
+            (marionette-eval
+             '(begin
+                (use-modules (gnu services herd))
+                (match (start-service 'php-fpm)
+                  (#f #f)
+                  (('service response-parts ...)
+                   (match (assq-ref response-parts 'running)
+                     ((pid) (number? pid))))))
+             marionette))
+
+          (test-eq "nginx running"
+            'running!
+            (marionette-eval
+             '(begin
+                (use-modules (gnu services herd))
+                (start-service 'nginx)
+                'running!)
+             marionette))
+
+          (test-equal "http-get"
+            200
+            (let-values (((response text)
+                          (http-get "http://localhost:8080/index.php"
+                                    #:decode-body? #t)))
+              (response-code response)))
+
+          (test-end)
+          
+          (exit (= (test-runner-fail-count (test-runner-current)) 0)))))
+
+  (gexp->derivation "php-fpm-test" test))
+
+(define %test-php-fpm
+  (system-test
+   (name "php-fpm")
+   (description "Test PHP-FPM through nginx.")
+   (value (run-php-fpm-test))))
-- 
2.14.2


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 963 bytes --]