Message ID | e02dd0f19603c3e0090137ace5a407dd448e0d88.1685887116.git.guix@twilken.net |
---|---|
State | New |
Headers | show |
Series | [bug#63877,v2] gnu: services: web: Allow specifying extra php-fpm environment variables. | expand |
On 2023-06-04 14:59, Timo Wilken wrote: > @@ -1096,6 +1100,9 @@ (define php-fpm-shepherd-service > #$@(if php-ini-file > `("-c" ,php-ini-file) > '())) > + #:environment-variables > + (append #$environment-variables > + (default-environment-variables)) Ungexp-ing lists can be rather tricky since your snippet will expand to: --8<---------------cut here---------------start------------->8--- ... #:environment-variables (append ("FOO=bar" ...) (default-environment-variables)) ... --8<---------------cut here---------------end--------------->8--- Which is interpreted as a procedure call. (and results in a hanged shepherd) You need to quote the list here: --8<---------------cut here---------------start------------->8--- #:environment-variables (append '#$environment-variables (default-environment-variables)) --8<---------------cut here---------------end--------------->8--- Bonus points if you can write a small system test for this. (see gnu/tests/web.scm for inspiration) For our purposes, a pair of HTTP servers where one of them uses a self-signed certificate will suffice.
Hi Timo, Did you have a chance to look into implementing Bruno’s suggestions? https://issues.guix.gnu.org/63877 Ludo’. Bruno Victal <mirai@makinata.eu> skribis: > On 2023-06-04 14:59, Timo Wilken wrote: >> @@ -1096,6 +1100,9 @@ (define php-fpm-shepherd-service >> #$@(if php-ini-file >> `("-c" ,php-ini-file) >> '())) >> + #:environment-variables >> + (append #$environment-variables >> + (default-environment-variables)) > > Ungexp-ing lists can be rather tricky since your snippet will expand to: > > ... > #:environment-variables (append ("FOO=bar" ...) > (default-environment-variables)) > ... > > > Which is interpreted as a procedure call. (and results in a hanged shepherd) > > You need to quote the list here: > > #:environment-variables (append '#$environment-variables > (default-environment-variables)) > > Bonus points if you can write a small system test for this. (see > gnu/tests/web.scm for inspiration) > For our purposes, a pair of HTTP servers where one of them uses a > self-signed certificate will suffice.
Hi Bruno, (hi Ludo'), thank you for your detailed feedback and sorry for not responding earlier! On Mon Jun 5, 2023 at 5:44 AM CEST, Bruno Victal wrote: > Ungexp-ing lists can be rather tricky [...] > > You need to quote the list [...] I was thinking of something closer to the example I added to doc/guix.texi in my patch. The gexp would not be a list directly, but instead be some code that would produce a list when evaluated, e.g.: --8<---------------cut here---------------start------------->8--- #~(list (string-append "SSL_CERT_DIR=" #$nss-certs "/etc/ssl/certs")))) --8<---------------cut here---------------end--------------->8--- That would let you refer to store paths in variable values, instead of being limited to literal strings. As far as I know, the following throws an error, and `file-append' instead of `string-append' wouldn't work because of the `"SSL_CERT_DIR="' prefix, right? --8<---------------cut here---------------start------------->8--- #~(#$(string-append "SSL_CERT_DIR=" nss-certs "/etc/ssl/certs")))) --8<---------------cut here---------------end--------------->8--- If you have any ideas on a better way to do this, let me know! > Bonus points if you can write a small system test for this. (see > gnu/tests/web.scm for inspiration) > For our purposes, a pair of HTTP servers where one of them uses a > self-signed certificate will suffice. Thanks for the pointer! I'll try to get something basic working along the lines of the php-fpm tests already there, and send a PATCH v3 soon. I was thinking of only verifying that an arbitrary sentinel variable is set, and not bother to test SSL_*-related behaviour, but I can try to get the latter working if you think that would be better.
Hi Timo, On 2023-10-15 21:54, Timo Wilken wrote: > Hi Bruno, (hi Ludo'), thank you for your detailed feedback and sorry for not > responding earlier! > > On Mon Jun 5, 2023 at 5:44 AM CEST, Bruno Victal wrote: >> Ungexp-ing lists can be rather tricky [...] >> >> You need to quote the list [...] > > I was thinking of something closer to the example I added to doc/guix.texi in > my patch. The gexp would not be a list directly, but instead be some code that > would produce a list when evaluated, e.g.: > > --8<---------------cut here---------------start------------->8--- > #~(list (string-append "SSL_CERT_DIR=" #$nss-certs "/etc/ssl/certs")))) > --8<---------------cut here---------------end--------------->8--- > > That would let you refer to store paths in variable values, instead of being > limited to literal strings. Right, I can see that it is indeed useful to accept a G-Exp instead. > As far as I know, the following throws an error, and `file-append' instead of > `string-append' wouldn't work because of the `"SSL_CERT_DIR="' prefix, right? > > --8<---------------cut here---------------start------------->8--- > #~(#$(string-append "SSL_CERT_DIR=" nss-certs "/etc/ssl/certs")))) > --8<---------------cut here---------------end--------------->8--- This ungexp doesn't work because it's “too wide”, in fact the bug in [1] was caused by a very similar snippet. Furthermore this would still run into the ungexp pitfall of being interpreted as a procedure call since you now have: --8<---------------cut here---------------start------------->8--- … #:environment-variables (append ("SSL_CERT_DIR=<garbage-here>…" …) (default-environment-variables)) … --8<---------------cut here---------------end--------------->8--- You could try using a list gexps/strings like this: --8<---------------cut here---------------start------------->8--- (list #~(string-append "SSL_CERT_DIR=" #$nss-certs "/etc/ssl/certs") "FOO=bar" (string-append "BAR=" 999)) --8<---------------cut here---------------end--------------->8--- Although your G-Exp idea might be better as it obviates the need to do things like '#$ (by using #~(list …) or #~'("foo" …)). [1]: <https://issues.guix.gnu.org/65383>
Hi Bruno, sorry for taking a while to get back to this. Writing a test for curl's behaviour with the SSL_CERT_DIR variable proved too fiddly for me, so I gave up and wrote a simpler test that just checks for a sentinel variable in the phpinfo output instead. I also found out that php-fpm clears environment variables when it starts, except for those listed in its configuration. However, libcurl isn't affected by this as far as I can tell -- it needs the SSL_CERT_DIR variable to be set in the process environment, not only in the php-fpm config file! I decided to set environment variables in the process environment and list them in the generated configuration file, so they're passed through to any PHP programs run through PHP-FPM. This should minimise surprise, I hope. (That's also be useful for setting e.g. PATH -- Nextcloud has started complaining that that variable is unset, and it needs the variable to be listed in the php-fpm configuration.) The reworked patch also removes some of the gexp-related hairyness -- the `environment-variables' property just takes a list of (variable-name . value) pairs now, no gexp'ing required, though file-like objects like what `file-append' returns are accepted. Please let me know what you think, and thank you for your considerable patience with this patch series! :)
diff --git a/doc/guix.texi b/doc/guix.texi index 7f8d8d66e9..441867afee 100644 --- a/doc/guix.texi +++ b/doc/guix.texi @@ -30994,6 +30994,18 @@ Web Services An optional override of the default php settings. It may be any ``file-like'' object (@pxref{G-Expressions, file-like objects}). You can use the @code{mixed-text-file} function or an absolute filepath for it. +@item @code{environment-variables} (default @code{#~(list)}) +A gexp (@pxref{G-Expressions}) which produces a list of strings +representing environment variable assignments. +These environment variables are set for the php-fpm process. +This can be used to, for example, point php-fpm at the CA certificates +in the @code{nss-certs} package from @code{(gnu packages certs)}: +@lisp +(php-fpm-configuration + ;; @dots{} + (environment-variables + #~(list (string-append "SSL_CERT_DIR=" #$nss-certs "/etc/ssl/certs")))) +@end lisp For local development it is useful to set a higher timeout and memory limit for spawned php processes. This be accomplished with the diff --git a/gnu/services/web.scm b/gnu/services/web.scm index 45897d7d6f..1c496d5946 100644 --- a/gnu/services/web.scm +++ b/gnu/services/web.scm @@ -16,6 +16,7 @@ ;;; Copyright © 2020, 2021 Alexandru-Sergiu Marton <brown121407@posteo.ro> ;;; Copyright © 2022 Simen Endsjø <simendsjo@gmail.com> ;;; Copyright © 2023 Bruno Victal <mirai@makinata.eu> +;;; Copyright © 2023 Timo Wilken <guix@twilken.net> ;;; ;;; This file is part of GNU Guix. ;;; @@ -974,7 +975,9 @@ (define-record-type* <php-fpm-configuration> php-fpm-configuration (file php-fpm-configuration-file ;#f | file-like (default #f)) (php-ini-file php-fpm-configuration-php-ini-file ;#f | file-like - (default #f))) + (default #f)) + (environment-variables php-fpm-configuration-environment-variables ;gexp producing list-of-strings + (default #~(list)))) (define-record-type* <php-fpm-dynamic-process-manager-configuration> php-fpm-dynamic-process-manager-configuration @@ -1081,7 +1084,8 @@ (define php-fpm-shepherd-service (match-lambda (($ <php-fpm-configuration> php socket user group socket-user socket-group pid-file log-file pm display-errors - timezone workers-log-file file php-ini-file) + timezone workers-log-file file php-ini-file + environment-variables) (list (shepherd-service (provision '(php-fpm)) (documentation "Run the php-fpm daemon.") @@ -1096,6 +1100,9 @@ (define php-fpm-shepherd-service #$@(if php-ini-file `("-c" ,php-ini-file) '())) + #:environment-variables + (append #$environment-variables + (default-environment-variables)) #:pid-file #$pid-file)) (stop #~(make-kill-destructor)))))))