Fix to support distro dependent FPM socks directory

[iliajie]

No description provided.

[verne-wv]

from very quick testing this morning, /var/run/php will work fine on my RedHat system ... so while its nice to have a RedHat variation of /var/run/php-fpm it may not be needed.

The original issue was ... NOT being inside the /var/run parent directory.

Verne

[iliajie]

from very quick testing this morning, /var/run/php will work fine on my RedHat system

I doubt as standard RHEL installs don't have /var/run/php directory.

What is the output of the following commands on your system:

grep -Rs "/var/run/php" /etc
ls -lL /var/run | grep php

[verne-wv]

I did not make the new subdirectory by hand ... the VirtualMin code made it on the fly when it made the first new SOCK file as part of my testing just now.

[root@vhost92-test ~]# grep -Rs "/var/run/php" /etc
/etc/httpd/conf.d/000-vhost92-ssl.conf--007:SetHandler proxy:unix:/var/run/php-fpm/vhost92.sock|fcgi://localhost/
/etc/httpd/conf/httpd.conf--023:SetHandler proxy:unix:/var/run/php-fpm/vhost92.sock|fcgi://localhost/
/etc/httpd/conf/httpd.conf--021:SetHandler proxy:unix:/var/run/php-fpm/vhost92.sock|fcgi://localhost/
/etc/httpd/conf/httpd.conf--019:SetHandler proxy:unix:/var/run/php-fpm/vhost92.sock|fcgi://localhost/
/etc/httpd/conf/httpd.conf--018:SetHandler proxy:unix:/var/run/php-fpm/vhost92.sock|fcgi://localhost/
/etc/httpd/conf/httpd.conf:        SetHandler proxy:unix:/var/run/php-fpm/166075271665066.sock|fcgi://localhost
/etc/httpd/conf/httpd.conf:        SetHandler proxy:unix:/var/run/php/166075265463380.sock|fcgi://localhost
/etc/httpd/conf/httpd.conf:        SetHandler proxy:unix:/var/run/php-fpm/1661396029113775.sock|fcgi://localhost
/etc/httpd/conf/httpd.conf--010--with-linux-socket:SetHandler proxy:unix:/var/run/php-fpm/vhost92.sock|fcgi://localhost/
/etc/httpd/conf/httpd.conf--020:SetHandler proxy:unix:/var/run/php-fpm/vhost92.sock|fcgi://localhost/
/etc/httpd/conf/httpd.conf-example:SetHandler proxy:unix:/var/run/php-fpm/vhost92.sock|fcgi://localhost/
/etc/httpd/conf/xx.txt:SetHandler proxy:unix:/var/run/php-fpm/vhost92.sock|fcgi://localhost/
/etc/httpd/conf/httpd.conf--022:SetHandler proxy:unix:/var/run/php-fpm/vhost92.sock|fcgi://localhost/
/etc/php-fpm.d/155655479617843.conf--linux-socket:listen = /var/run/php-fpm/vhost92.sock
/etc/php-fpm.d/166075265463380.conf:listen = /var/run/php/166075265463380.sock
/etc/php-fpm.d/166075271665066.conf:listen = /var/run/php-fpm/166075271665066.sock
/etc/php-fpm.d/1661396029113775.conf:listen = /var/run/php-fpm/1661396029113775.sock
/etc/selinux/targeted/contexts/files/file_contexts:/var/run/php-fpm(/.*)?       system_u:object_r:httpd_var_run_t:s0
Binary file /etc/selinux/targeted/contexts/files/file_contexts.bin matches
/etc/selinux/targeted/active/file_contexts:/var/run/php-fpm(/.*)?       system_u:object_r:httpd_var_run_t:s0

and

[root@vhost92-test ~]# ls -lL /var/run | grep php
drwxr-xr-x.  2 root           root             60 Aug 25 08:30 php
drwxr-xr-x.  2 root           root            100 Aug 25 08:30 php-fpm

also I scrolled back in my putty window to verify that before I started my test, /var/run/php did not exist.

Verne

[jcameron]

I don't think we want to make this change - it's just increasing complexity when the current path, while non-standard, works fine as far as I can tell.

[iliajie]

I don't think we want to make this change

Well .. 🙂 .. I asked you before making this change, and you welcomed it .. This is why I did it as at first I was sceptical about making this change too, and even closed that initial PR ..

it's just increasing complexity when the current path, while non-standard, works fine as far as I can tell.

Rising complexity by a few percents to already complex code makes no difference .. what makes a difference is that this change will be bug-free .. even though I tested it dozens of times with both Apache and Nginx, and in various configurations, I want you to check it thoroughly as well .. We have no right to make a mistake in such widely-used and crucial area.

[verne-wv]

the current path DOES NOT WORK 100 percent of the time, if you have SELinux turned on :-)

and I would hate for VirtualMin to take the position ... we refuse to support SELinux in any manner :-(

Also note that it appears a lot of other packages use the /var/run/ directory ...

Verne

[iliajie]

We must use conventional distro-based paths. This is what this patch is for.

SELinux is configurable. If it doesn't work, just create a context for this particular blockage. It takes less than a minute. We cannot help it in this code.

[verne-wv]

you are correct on both points ... in fact you offer two solutions for the same issue ....

1. each admin has to make a local custom SELinux rule to permit this non-standard path usage

or

2. adjust/update VirtualMin to use a distro-based path.

I like number 2 best :-)

Verne

[iliajie]

@jcameron Jamie, all further conversations regarding this PR should be done here, rather than at #349 thread! Let's keep it all in one place.

I'm OK with fixing the SElinux problem, but not a fan of adding complexity to just be compatible with other distros. For example, what happens if a domain is moved from one distro to another via backup / restore?

That was a way to fix SELinux, right?
About restoring a domain - I didn't test this case scenario but I can assume that if the distro is changed the socket file won't match and the mode detection with a current code may fail!
Unless we fix the code to use regex to match it (socket) disregard of the socket path? As in other parts of the code?
By the way, this must not hold a new 7.2 release.

Alright, I think I fixed it to work correctly with restores too. As far as I understand the code detecting FPM mode shouldn't be tide to exact port number or socket path. This patch should address it. It also removes no longer necessary get_php_fpm_socket_oldfile sub. The code looks cleaner and simpler to me now.

In the future when we add PHP per-directory support with FPM mode, this code should also work just fine as we will use instead of $vconf (searching for directives inside of VirtualHost), we will use $dir (to search inside of a directory), as there can only be one record for it.

Please review and test it.

[iliajie]

the current path DOES NOT WORK 100 percent of the time, if you have SELinux turned on :-)

Try using the latest patches it will use /run directly. Also, considering this PR enables now using non strict matching for sockets and ports (which isn't needed in my opinion but I want Jamie comment on it anyway!), we can also use configurable fpm_socket_dir directory (for whoever and whatever purposes).

However, again, the main goal of this PR is to use distro dependent directory, exactly what FPM services use natively, i.e. /run/php-fpm on RHEL systems and /run/php on Debian/Ubuntu!

[verne-wv]

I really think, while its nice I suppose to have something specific to RedHat/Centos, that /var/run/php will work fine on RedHat systems ... its not required to have something different for RedHat compared to other distros

Verne

[iliajie]

It already all worked before this PR.

[verne-wv]

maybe my older comments were in the other ticket ....

on a RedHat system with SELinux in enforcing mode, the original path of /var/php-fpm is blocked by SELinux.

so --- php-fpm that tries to use a socket file in /var/php-fpm will never ever work :-) in fact php-fpm fails to restart after VirtualMin creates that socket file and that failure to restart would take down all virtual hosts, not just ones trying to use a socket. Specifically, VirtualMin can create the file but SELinux blocks it from being opened as a socket.

But -- anything in or underneath, /var/run will work, so the suggested path /var/run/php for other distros will work on a RedHat system as well.

[iliajie]

Does /run/php-fpm works for you and SELinux out of the box on RHEL systems?

[verne-wv]

no it does not, which is why I opened #349 in December to replace /run/php-fpm with something that does work :-)

[iliajie]

no it does not, which is why I opened

This doesn't seem to be correct. I have made a clean Virtualmin Pro and keeping SELinux enabled. Updated Virtualmin code with latest checked in code including this patch and PHP FPM started without any complains, and without creating any custom rules. What didn't start was Apache, because of blocked access to /var/log/virtualmin/* log files. Telling SELinux to generate a custom local policy module to allow access made Apache start.

But again, using system default /run/php-fpm makes it work with not issue. I would be surprised to have issues with that as it's being used as system default FPM directory.

But -- anything in or underneath, /var/run will work

Isn't /var/run is just a symlink to an actual /run?

so the suggested path /var/run/php for other distros will work on a RedHat system as well.

No, this is not correct, sorry. Here is an example of what happens if you use /var/run/php with SELinux enabled on Fedora:

I am keeping it for RHEL systems with /run/php-fpm, as again this is what works out of the box.

[verne-wv]

your screen shot of the failure shows the path /var/php-fpm/ ... did I miss something? I thought we are looking at /var/run and perhaps /run ???

[verne-wv]

and I see that I had an error in my comment yesterday of

no it does not, which is why I opened #349 in December to replace /run/php-fpm with something that does work :-)

what I wrote about in December was ... change from using /var/php-fpm/ to /var/run/php-fpm/

and your screenshot shows that today, /var/php-fpm/ does not work :-)

[iliajie]

your screen shot of the failure shows the path /var/php-fpm/ ... did I miss something?

No, you're right. I replaced rhel path in my new code and it defaulted to /var/php-fpm. Now I tried with /var/run/php and it seems to work too as you were suggesting. Although, I prefer to have system based path on RHEL, which is /run/php-fpm.

I would like to know what Jamie thinks about it, and also about, matching socket name and port number loosely.

[iliajie]

This patch will still work even if the expected directory doesn't exist though, right?

Jamie, I simply don't remember. Also, I'm not sure if this patch won't break anything now, as it's been over a year ago since it was first submitted. I will have to re-run in-depth test over again.

On my Fedora and Rocky 9 and Ubuntu 20 systems /var/run is a symbolic link to /run. We can trust that all current Linux distros do /run.

Joe, I think I made it work either way, with the fall back set to /var/php-fpm (the path we use now across all distros).

I remember now, that my initial goal was to store PHP-FPM pool file in a directory that a distro expects. However, Virtualmin needs to know how to read it from any possible location for variety of reasons.

I will have to review, and then re-test the code.

[iliajie]

But, I guess 18.04 isn't supported anymore

Ubuntu 16 and 18 also have /run simlinked from /var/run .. I don't think it's that new of a stuff ..

[verne-work]

My initial point was, and still is, to stop using /var/php-fpm everywhere !!

[iliajie]

My initial point was, and still is, to stop using /var/php-fpm everywhere

This PR addressed exactly that. By the way, is this because of SELinux only or there are other reasons?

[verne-work]

only because of SELinux --- I hope that's OK 😄

[iliajie]

only because of SELinux --- I hope that's OK 😄

In fact AppArmor also doesn't work with /var/php-fpm.

[jcameron]

Jamie, I simply don't remember. Also, I'm not sure if this patch won't break anything now, as it's been over a year ago since it was first submitted. I will have to re-run in-depth test over again.

I can only see this breaking things if it doesn't handle the case where an existing domain is using a different path..

[verne-work]

I don't understand (maybe I don't need to) Jamie's comment/issue about existing domains ... I thought every time php-fpm started, it would read the individual xxxxx.conf file to find the socket file definition, and create that socket file on the fly. So as long as the setting in httpd.conf uses the same name/path , whatever it may be (the old one or the new one), all is good.

And since these socket names are per-domain, is it an issue if some are different from others? Of course it would be cleaner and simpler for all of them to be uniform and use the same path 😄 Perhaps he is concerned that somehow one item (the xxxx.conf file or the httpd.conf line) could end up being adjusted while the other item does not get adjusted.

[jcameron]

So the Apache / FPM config for existing domains won't be broken, but it's also important that calls to the get_php_fpm_socket_file function will still return the same path on existing Virtualmin domains even after this patch.

[iliajie]

but it's also important that calls to the get_php_fpm_socket_file function will still return the same path on existing Virtualmin domains even after this patch.

Yes, I implied that in the first place. Although, I will have to review it ..

[jcameron]

Looking at the code, I think it's fine as long as the .socket file never disappears.

I'll merge this commit ...

[iliajie]

Looking at the code, I think it's fine as long as the .socket file never disappears.

What do you mean by that ..? 🙂

[jcameron]

I mean if the socket file in the Apache config was under the old directory and the file disappeared, this function would now return the old path which might not match the Apache and FPM pool configs.

[iliajie]

I mean if the socket file in the Apache config was under the old directory and the file disappeared, this function would now return the old path which might not match the Apache and FPM pool configs.

Yes, this is the issue. I could reproduce it! 🐞 With the old code the socket was set to /var/php-fpm in both Apache and FPM. After the new code is applied, and when PHP-FPM version changed (without changing mode), then the new FPM pool file is generated with the new path used for the socket, i.e. /run/php yet Apache socket isn't updated and still points to /var/php-fpm. This obviously break things.

We should sync the sockets between Apache and PHP-FPM!

[jcameron]

Ok that's an issue .... can you post the fully sequence of actions that triggered this issue?

[iliajie]

can you post the fully sequence of actions that triggered this issue?

I think there is nothing to add to what I have just mentioned in my previous comment. There are clear steps to reproduce it.

[jcameron]

Did you have to delete the .socket file to trigger this issue?

[iliajie]

Did you have to delete the .socket file to trigger this issue?

No.

Quick steps to reproduce it:

1. Old PHP-FPM setup with the socket set to /var/php-fpm directory in both Apache and FPM
2. Then apply the new code (from recently merged PR)
3. Change PHP version (not mode, the mode is set to FPM)
4. And you get socket directories out of sync, i.e. FPM /run/php and Apache /var/php-fpm.

This happens because the new code sets the socket path to /run/php but Apache config isn't getting updated (because mode isn't changed).

[jcameron]

Ok I see is ... digging into the cause now.

[jcameron]

Ok, this patch should fix it : b62216b

[iliajie]

Ok, this patch should fix it : b62216b

I don't see how this patch can solve described above problem.

Also, why do we even need to pre-create a socket file, isn't it something that's done by the process itself?

[jcameron]

Did you test it? It works for me ...

[iliajie]

Yes, I did test it. I can assure you it doesn't work. We already have to fix it, remember that alert we recently added? It detects the problem if you return to PHP Options page but not on the initial change ..

[jcameron]

You're right, I missed a case. Check out 86709a8

[iliajie]

Thanks, Jamie! This patch seems to work as expected now!

Although, I think it would be better to update Apache socket instead of preserving the old socket name in FPM config, as otherwise the socket name will never be updated to the new path unless the mode is changed or webserver feature is disabled. However, we can also live with the old path, I think, unless it can be changed easily ..

[jcameron]

I think it's safest to stick to the current path, as it would be unexpected for it to move just due to a PHP version change..

[iliajie]

I think it's safest to stick to the current path, as it would be unexpected for it to move just due to a PHP version change..

Got it! I thought so too!