Use httpd -S / apache2ctl configtest as a sanity check before restarting

I don't think we should parse httpd.conf. It's not that the file format is that complex in itself, it's that there's a lot of corner cases to cover. We will have to work *very* hard to make sure we avoid an error that is basically an operator error (provisionning virtual hosts outside of hostmaster). Once hostmaster is on a server, it should rightly expect to have every domain available to itself.

Of course it should check if it's allowed to host certain domains, e.g. allowing yahoo.com to be hosted on hostmaster is probably a bad idea, but this is a matter of policy and outside the scope of this issue here.

So my opinion is that we can *check* if a hostname is already defined in the apache configuration files in a crude grep-style way, but going further than that is a waste of time.

We could use `httpd -S` to check what hostnames are taken:

VirtualHost configuration:
wildcard NameVirtualHosts and _default_ servers:
*:*                    is a NameVirtualHost
         default server marvin.anarcat.ath.cx (/etc/apache2/sites-enabled/000-default:2)
         port * namevhost marvin.anarcat.ath.cx (/etc/apache2/sites-enabled/000-default:2)
         port * namevhost anarcat.ath.cx (/etc/apache2/sites-enabled/0_anarcat.ath.cx.conf:1)
Syntax OK

The advantage is that this is the actual parsed configuration that's currently active. As a bonus, we check if we screwed up the config by declaring duplicate stuff or whatever.. ;)

a) we should make that customizable per webserver, just like apachectl is
b) we could lookup a few paths in the verification of the webserver (/usr/s?bin/(apache|httpd)2?)
c) same as apachectl again: it (the above + -S) needs to be added to sudo

So apache.conf is not all: it may Include another file or a directory which then would include Aegir. That's how the INSTALL.txt tells people to setup Aegir anyways, and I think it's proper from a packaging point of view.

Basically, the apache.conf would be a start, then you need to parse each file for Include and recursively parse those to find your final include.

For NginX it is a matter of good init script. We are using it already in Barracuda, so it always runs configtest before running reload. Furthermore, even without this configtest, NginX will ignore reload command completely if there are any fatal errors. Of course it will fail to start when admin will try to force restart, but at least we know that Aegir will not be able to take down the NginX web server. This init script is Debian/Ubuntu compatible, but I don't think we can include it in the Aegir docs/hints. Anyway, we are safe by default with vanilla NginX install because we are using safe reload init command, and not restart.

Is this still relevant? I don't recall seeing any recent problems caused by errors in Apache conf files... Still, this seems like it would be pretty easy to implement. We'd certainly want it if we undertook anything like #1217302: Add functionality to change Apache VirtualHost definitions from UI

It is just good practice, so yes, this is still relevant. we should probably rollback the task if the config check fails.

The attached patch appears to work for the base apache service.

Here it is for the rest of the http services. I haven't tested on Nginx, but I believe '-t' is the proper option to do a configtest.

Since we use $this within the function, we can't make this static as we do with apache_restart_cmd().

This doesn't work for Nginx, since the -t works only with Nginx binary and not with the init script. Note that you don't really need to use any specific command other then typical service nginx reload because it will not reload the config and will show the config error when the reload fails. That is the difference between reload and restart in Nginx.

v251a:~# /usr/sbin/nginx -t
nginx: the configuration file /etc/nginx/nginx.conf syntax is ok
nginx: configuration file /etc/nginx/nginx.conf test is successful
v251a:~#

v251a:~# service nginx -t
Usage: /etc/init.d/nginx {start|stop|restart|force-reload|status|configtest|quietupgrade|terminate}
v251a:~# service nginx configtest
nginx: the configuration file /etc/nginx/nginx.conf syntax is ok
nginx: configuration file /etc/nginx/nginx.conf test is successful
v251a:~#

Note however that configtest may not be available since it depends on the init script provided by particular distro, so calling Nginx binary with -t flag is the most reliable method. IMO.

Fixed extra ')', and now trying to find the nginx binary.

I don't like manually parsing the PATH at all: if nginx is in the PATH, iterating over the PATH is useless, as we should just be able to call "nginx" directly (without an absolute path).

Furthermore, if "nginx reload" works, why do we mess around with the init.d? If we would skip using the init.d, we would avoid the code duplication created by the patch in #20, which i dislike a little. Same for apache, actually: let's just call apache2ctl directly please.

Nginx does require init script. There is no such thing as "nginx reload" -- it is "service nginx reload" which is "/etc/init.d/nginx reload" effectively. That said, I don't see any need for extra check for Nginx, since "service nginx reload" runs that check -- but of course you will not notice that it failed to reload when there is no config check in provision.

Well, I don't know about the different versions of nginx, but here there *is* such a thing as nginx reload: it's

anarcat@marcos:provision$ nginx -h
nginx version: nginx/1.2.1
Usage: nginx [-?hvVtq] [-s signal] [-c filename] [-p prefix] [-g directives]

Options:
[...]
  -s signal     : send signal to a master process: stop, quit, reopen, reload

So I assume that this:

nginx -s reload

will do the right thing.

Also, the idea behind this feature request is to rollback in provision if we introduce a syntax error, instead of creating a configuration file that could crash the webserver on reboot or keep it from reloading properly until there's a manual intervention.

Right, we can call binary for reload with "nginx -s reload" and it still checks the config syntax, so we don't need to rely on the init script -- but note that then we kind of "enforce policy" if we ignore init script. But in this case it is not a problem to me.

So, we can check the syntax with "nginx -t" and if there is an error, revert the task and use "nginx -s reload" for config update.

Note that we have introduced that paths discovery (if I recall this correctly) to support OS X installs. Or maybe we just followed the way the path discovery was done for Apache, not sure, but before we will drop this, maybe we should ask folks using OS X if calling just nginx directly works -- and then maybe modify PATH in the aegir cron entry, if needed.

However, we will break things on upgrades, until the server admin will modify /etc/sudoers entry.

I think it's fine to break the upgrade path for 2.x, it's the right time.

If we'd like to not impose policy, we'd need to allow users to set specific "test" and "reload" commands, something which we are not doing right now either: we are just autodetecting, not allowing customization, if I understand properly.

As for path discovery: I just don't get it, I think. :) Really, why do we need to iterate manually through $PATH? Isn't that what "exec" does?

It has been added to make it work when Nginx is installed from sources in a path which is not listed in PATH in the aegir cron entry, like /usr/local/bin/nginx or /usr/local/sbin/nginx and to support config reloads with and without init script on various systems. Here is relevant issue: #1259098: nginx HTTP service should support different restart commands plus #1303088: wrong nginx restart script detection code

right i see. let me clarify what i mean.

+++ b/http/Provision/Service/http/nginx.phpundefined
@@ -137,6 +137,38 @@ class Provision_Service_http_nginx extends Provision_Service_http_public {
+    foreach (explode(':', $_SERVER['PATH']) as $path) {
+      $options[] = "$path/nginx";
+    }

this is useless.

+++ b/http/Provision/Service/http/nginx.phpundefined
@@ -137,6 +137,38 @@ class Provision_Service_http_nginx extends Provision_Service_http_public {
+    $options[] = '/usr/sbin/nginx';
+    $options[] = '/usr/local/sbin/nginx';
+    $options[] = '/usr/local/bin/nginx';

this may not be, but I would expect people to modify their PATH to the same effect... but i'm ready to concede this may be expecting too much.... maybe we should set the PATH in the crontab instead?

The last few comments strike me as out-of-scope for this issue. However we do it, it seems to me that the sanity check should be consistent with the way the restart command itself is determined. I'm tempted to commit this, and start a follow-up issue to continue the discussion about changing how we set the nginx commands.

i feel this should be pushed to 3.x.