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

[nee]

Hello, here is an updated version.
I'm weirdly having trouble with getting the log-file to log something
(see below), everything else should be addressed.

Am 13.10.2017 um 23:37 schrieb Christopher Baines:
> On Mon, 9 Oct 2017 23:54:24 +0200
> nee <address@hidden> wrote:
> 
>> Subject: [PATCH 2/2] gnu: services: Add php-fpm.
> 
> Hey, I've never used php-fpm, but I'll have a go at reviewing this :)
> 
>> * gnu/services/web.scm (<php-fpm-configuration>,
>>   <php-fpm-process-manager-configuration>): New record types.
>>   (php-fpm-configuration?,
>>    php-fpm-process-manager-configuration?,
>>    php-fpm-service-type,
>>    nginx-php-location): New procedures.
>> * doc/guix.texi (Web-Services): document php-fpm service.
> 
> Very minor, but I'd suggest "(Web Services): Document the php-fpm
> service." here.
> 
Okay, I changed the first letter to capital case and from the other
patch 'function' to 'procedure'.

>> ---
>>  doc/guix.texi        |  93 +++++++++++++++++++++++++++
>>  gnu/services/web.scm | 173
>> ++++++++++++++++++++++++++++++++++++++++++++++++++- 2 files changed,
>> 264 insertions(+), 2 deletions(-)
>>
>> diff --git a/doc/guix.texi b/doc/guix.texi
>> index f0a59a6b4..ed4336f64 100644
>> --- a/doc/guix.texi
>> +++ b/doc/guix.texi
>> @@ -14529,6 +14529,99 @@ capability also has to be configured on the
>> front-end as well. @end deftp
>>  
>>  
>> address@hidden php-fpm
>> +PHP-FPM (FastCGI Process Manager) is an alternative PHP FastCGI 
>> implementation
>> +with some additional features useful for sites of any size.
> 
> If these additional features are worth mentioning, it would be best to
> be specific as to what they are, and what benefit they provide.
> 
Okay I copied the list of features form the php-fpm homepage.
I didn't add configuration for all of the features, tough.

There is some advanced stuff with pools that use different php.inis,
chroot, and using sendmail to email errors.
I'm not much of a php user and I don't know how to test everything.
I want to get a basic service working at first.

>> address@hidden {Scheme Variable} php-fpm-service-type
>> +A Service type for @code{php-fpm}.
>> address@hidden defvr
>> +
> 
> ...
> 
>> address@hidden {Data type} php-fpm-process-manager-configuration
>> +Data Type for php-fpm worker process limits.
>> address@hidden @asis
>> address@hidden @code {type} (default: @code{"dynamic"})
>> address@hidden @asis
>> address@hidden @code{"dynamic"}
>> +Spare worker processes are kept around based on the set 
>> @code{php-fpm-process-manager-configuration} limits.
>> address@hidden @code{"static"}
>> +A static number of worker processes is created.
>> address@hidden @code{"ondemand"}
>> +Worker processes are only created on demand.
>> address@hidden table
>> address@hidden @code {max-children} (default: @code{5})
>> +Maximum of worker processes. Applies when the type is @code{"static"}, 
>> @code{"dynamic"}, or @code{"ondemand"}.
>> address@hidden @code {start-servers} (default: @code{2})
>> +How many worker processes should be started on start-up. Only applies when 
>> type is @code{"dynamic"}.
>> address@hidden @code {min-spare-servers} (default: @code{1})
>> +How many spare worker processes should be kept around at minimum. Only 
>> applies when type is @code{"dynamic"}.
>> address@hidden @code {max-spare-servers} (default: @code{3})
>> +How many spare worker processes should be kept around at maximum. Only 
>> applies when type is @code{"dynamic"}.
>> address@hidden @code {process-idle-timeout} (default: @code{10})
>> +The time in seconds after which a process with no requests is killed. Only 
>> applies when type is @code{"ondemand"}.
>> address@hidden table
>> address@hidden deftp
> 
> Reading this makes me think that maybe having record types per process
> manager types might be useful, e.g.
> <php-fpm-dynamic-process-manager-configuration>
> <php-fpm-static-process-manager-configuration>
> <php-fpm-process-manager-configuration>
> 
Good point! That makes misconfiguration less likely.
I changed it to be that way.


>> address@hidden {Scheme Variable} nginx-php-fpm-location
>> +A helper function to quickly add php to an
>> @code{nginx-server-configuration}. address@hidden defvr
>> +
>> +A simple services setup for nginx with php can look like this:
>> address@hidden
>> +(services (cons* (dhcp-client-service)
>> +                 (service php-fpm-service-type
>> +                          (php-fpm-configuration))
> 
> The default-value for the service type means that you can omit the
> (php-fpm-configuration) here, as (service php-fpm-service-type) should
> work.
> 
Okay, I changed the example.

>> +                 (service nginx-service-type
>> +                          (nginx-server-configuration
>> +                           (server-name '("example.com"))
>> +                           (root "/srv/http/")
>> +                           (locations
>> +                            (list (nginx-php-location)))
>> +                           (https-port #f)
>> +                           (ssl-certificate #f)
>> +                           (ssl-certificate-key #f)))
>> +                 %base-services))
>> address@hidden example
>> +
>> +
>>  @node DNS Services
>>  @subsubsection DNS Services
>>  @cindex DNS (domain name system)
>> diff --git a/gnu/services/web.scm b/gnu/services/web.scm
>> index 9d713003c..fd63b15bb 100644
>> --- a/gnu/services/web.scm
>> +++ b/gnu/services/web.scm
>> @@ -26,8 +26,11 @@
>>    #:use-module (gnu system shadow)
>>    #:use-module (gnu packages admin)
>>    #:use-module (gnu packages web)
>> +  #:use-module (gnu packages php)
>>    #:use-module (guix records)
>>    #:use-module (guix gexp)
>> +  #:use-module ((guix utils) #:select (version-major))
>> +  #:use-module ((guix packages) #:select (package-version))
>>    #:use-module (srfi srfi-1)
>>    #:use-module (ice-9 match)
>>    #:export (<nginx-configuration>
>> @@ -76,7 +79,14 @@
>>  
>>              fcgiwrap-configuration
>>              fcgiwrap-configuration?
>> -            fcgiwrap-service-type))
>> +            fcgiwrap-service-type
>> +
>> +            php-fpm-configuration
>> +            php-fpm-configuration?
>> +            php-fpm-process-manager-configuration
>> +            php-fpm-process-manager-configuration?
>> +            php-fpm-service-type
>> +            nginx-php-location))
> 
> When using other Guix services, I've run in to problems when field
> accessors and record types were not exported. The biggest cost I can
> think of is the lines of code, but I'd still suggest to export
> everything by default here.
> 
Okay, I export them now.
The fcgiwrapper package does not export them though.
That should be addressed in a separate patch later.


>>  ;;; Commentary:
>>  ;;;
>> @@ -256,10 +266,12 @@ of index files."
>>            "events {}\n")))
>>  
>>  (define %nginx-accounts
>> -  (list (user-group (name "nginx") (system? #t))
>> +  (list (user-group (name "www-data") (system? #t))
>> +        (user-group (name "nginx") (system? #t))
>>          (user-account
>>           (name "nginx")
>>           (group "nginx")
>> +         (supplementary-groups '("www-data"))
>>           (system? #t)
>>           (comment "nginx server user")
>>           (home-directory "/var/empty")
> 
> Pulling in your comment about the accounts...
> 
>> About the accounts:
>> Nginx needs write access to the unix socket of php-fpm. I didn't want
>> to set nginx as default user for php-fpm in case we add other
>> webservers, so I added the nginx to the newly created www-data group.
> 
> This sounds like it would work, however, the defaults for the
> socket-user and socket-group of the php-fpm-configuration are currently
> "nginx", which seems to disagree with what you say above?
> 
That's a mistake. Seems like I actually forgot to change that.

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

>> @@ -385,3 +397,160 @@ of index files."
>>                     (service-extension account-service-type
>>                                            fcgiwrap-accounts)))
>>                  (default-value (fcgiwrap-configuration))))
>> +
>> +(define-record-type* <php-fpm-configuration> php-fpm-configuration
>> +  make-php-fpm-configuration
>> +  php-fpm-configuration?
>> +  (php             php-fpm-configuration-php ;<package>
>> +                   (default php))
>> +  (socket          php-fpm-configuration-socket
>> +                   (default (string-append "/var/run/php"
>> +                                         (version-major
>> (package-version php))
>> +                                         "-fpm.sock")))
>> +  (user            php-fpm-configuration-user
>> +                   (default "php-fpm"))
>> +  (group           php-fpm-configuration-group
>> +                   (default "php-fpm"))
>> +  (socket-user     php-fpm-configuration-socket-user
>> +                   (default "nginx"))
>> +  (socket-group    php-fpm-configuration-socket-group
>> +                   (default "nginx"))
> 
> As above, I'm not sure the use of nginx here matches the
> description you gave...?
> 
Mistake on my side. I changed it to php-fpm now.

>> +  (process-manager php-fpm-configuration-process-manager
>> +                   (default (php-fpm-process-manager-configuration)))
>> +  (display-errors  php-fpm-configuration-display-errors
>> +                   (default #f))
>> +  (workers-logfile php-fpm-configuration-workers-logfile
>> +                   (default (string-append "/var/log/php"
>> +                                         (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.

>> +  (file          php-fpm-configuration-file ;#f | file-like
>> +                 (default #f)))
>> +
>> +(define-record-type* <php-fpm-process-manager-configuration>
>> php-fpm-process-manager-configuration
>> +  make-php-fpm-process-manager-configuration
>> +  php-fpm-process-manager-configuration?
>> +  (type                 php-fpm-process-manager-configuration-type
>> +                        (default "dynamic"))
>> +  (max-children
>> php-fpm-process-manager-configuration-max-children
>> +                        (default 5))
>> +  (start-servers
>> php-fpm-process-manager-configuration-start-servers
>> +                        (default 2))
>> +  (min-spare-servers
>> php-fpm-process-manager-configuration-min-spare-servers
>> +                        (default 1))
>> +  (max-spare-servers
>> php-fpm-process-manager-configuration-max-spare-servers
>> +                        (default 3))
>> +  (process-idle-timeout
>> php-fpm-process-manager-configuration-process-idle-timeout
>> +                        (default 10)))
>> +
>> +(define php-fpm-accounts
>> +  (match-lambda
>> +    (($ <php-fpm-configuration> php socket user group socket-user
>> socket-group _ _ _ _)
>> +     (filter identity
>> +             (list
>> +              (user-group (name "www-data") (system? #t))
>> +              (and (equal? group "php-fpm")
>> +                   (user-group
>> +                    (name "php-fpm")
>> +                    (system? #t)))
>> +              (and (equal? user "php-fpm")
>> +                   (user-account
>> +                    (name "php-fpm")
>> +                    (group group)
>> +                    (supplementary-groups '("www-data"))
>> +                    (system? #t)
>> +                    (comment "web services group")
>> +                    (home-directory "/var/empty")
>> +                    (shell (file-append shadow
>> "/sbin/nologin"))))))))) +
> 
> As you can specify the user and group in the <php-fpm-configuration>, I
> think it might be better to just use the user and group names from
> there, and always setup a user and group with those names, I'm guessing
> that this will be what the average user would expect to happen.
> 
I also thought that, but I took the config from the fastcgi service as
example. The consequence of that is that you have to define your users
yourself and get an error if you didn't.

I'm changing it for this patch.

>> +(define (default-php-fpm-config socket user group socket-user
>> socket-group
>> +          pm display-errors workers-logfile)
>> +  (match
>> +      pm
>> +    (($ <php-fpm-process-manager-configuration> pm.type
>> +                                                pm.max-children
>> +                                                pm.start-servers
>> +                                                pm.min-spare-servers
>> +                                                pm.max-spare-servers
>> +
>> pm.process-idle-timeout)
>> +     (apply mixed-text-file "php-fpm.conf"
>> +            "[global]\n"
>> +            "[www]\n"
>> +            "user =" user "\n"
>> +            "group =" group "\n"
>> +            "listen =" socket "\n"
>> +            "listen.owner =" socket-user "\n"
>> +            "listen.group =" socket-group "\n"
>> +
>> +            "pm =" pm.type "\n"
>> +            "pm.max_children =" (number->string pm.max-children) "\n"
>> +            "pm.start_servers =" (number->string pm.start-servers)
>> "\n"
>> +            "pm.min_spare_servers =" (number->string
>> pm.min-spare-servers) "\n"
>> +            "pm.max_spare_servers =" (number->string
>> pm.max-spare-servers) "\n"
>> +            "pm.process_idle_timeout =" (number->string
>> pm.process-idle-timeout) "s\n" +
>> +            "php_flag[display_errors] = " (if display-errors "on"
>> "off") "\n" +
>> +            (if workers-logfile
>> +                (list "catch_workers_output = yes\n"
>> +                      "php_admin_value[error_log] =" workers-logfile
>> "\n"
>> +                      "php_admin_flag[log_errors] = on\n")
>> +                (list "catch_workers_output = no\n"))))))
>> +
>> +(define php-fpm-shepherd-service
>> +  (match-lambda
>> +    (($ <php-fpm-configuration> php socket user group socket-user
>> socket-group
>> +                                pm display-errors workers-logfile
>> file)
>> +     (list (shepherd-service
>> +            (provision '(php-fpm))
>> +            (documentation "Run the php-fpm daemon.")
>> +            (requirement '(networking))
>> +            (start #~(make-forkexec-constructor
>> +                      '(#$(file-append php "/sbin/php-fpm")
>> +                        "--nodaemonize" "-p" "/var" "--fpm-config"
>> +                        #$(or file
>> +                              (default-php-fpm-config socket user
>> group
>> +                                socket-user socket-group pm
>> display-errors
>> +                                workers-logfile)))))
>> +            (stop #~(make-kill-destructor)))))))
> 
> As php-fpm supports using a pid file, I'd recommend configuring both
> php-fpm and shepherd to use this by default. It means that the shepherd
> service will wait until php-fpm creates the PID file before starting
> services that say they require it, which can prevent some issues.
> 
I didn't know about this. It sounds like a good feature. I added it to
the config and to the make-forkexec-constructor key now. Thanks for
noting it!

I also renamed the logfile variables to log-file to match the pid-file
naming.

>> +(define php-fpm-activation
>> +  (match-lambda
>> +    (($ <php-fpm-configuration> _ _ user _ _ _ _ _ workers-logfile _)
>> +     #~(begin
>> +         (use-modules (guix build utils))
>> +         (let ((user (getpwnam #$user))
>> +               (touch (lambda (file-name)
>> +                        (call-with-output-file file-name (const
>> #t)))))
>> +           ;; prepare writable logfile
>> +           (when #$workers-logfile
>> +             (when (not (file-exists? #$workers-logfile))
>> +               (touch #$workers-logfile))
>> +             (chown #$workers-logfile (passwd:uid user) (passwd:gid
>> user))
>> +             (chmod #$workers-logfile #o660)))))))
>> +
>> +
>> +(define php-fpm-service-type
>> +  (service-type (name 'php-fpm)
>> +                (extensions
>> +                 (list (service-extension shepherd-root-service-type
>> +                                          php-fpm-shepherd-service)
>> +                       (service-extension activation-service-type
>> +                                          php-fpm-activation)
>> +                       (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.



>> +(define* (nginx-php-location
>> +          #:key
>> +          (nginx-package nginx)
>> +          (socket (string-append "/var/run/php"
>> +                                 (version-major (package-version
>> php))
>> +                                 "-fpm.sock")))
>> +  "Return a nginx-location-configuration that makes nginx run .php
>> files."
>> +  (nginx-location-configuration
>> +   (uri "~ \\.php$")
>> +   (body (list
>> +          "fastcgi_split_path_info ^(.+\\.php)(/.+)$;"
>> +          (string-append "fastcgi_pass unix:" socket ";")
>> +          "fastcgi_index index.php;"
>> +          (list "include " nginx-package
>> "/share/nginx/conf/fastcgi.conf;")))))
> 
> This helper seems good, although I think putting the "include"
> elsewhere would be better, and I recognise that this isn't possible
> with the nginx service yet.
> 
Alright, so I'll keep it like this for now.

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

Thank you for reviewing this very big patch.
Happy hacking!

0001-guix-utils-add-version-major.patch
Description: Text Data

0002-gnu-services-Add-php-fpm.patch
Description: Text Data