ba2cdaf4a24f61dfd4f4b7c1112f1e4b35e4c797 removes the fastcgi params, which are already defined in the debian package.

if they are required on some platforms, they should be part of the sample configuration file, maybe #1635552: remove unused config file?

Comments

omega8cc’s picture

Status: Needs review » Fixed

Redundant config has been removed from the main config file: http://drupalcode.org/project/provision.git/commit/c74f5af

I recall it was put there after some request to support non-drupal sites/vhosts, but this can be done easily by including params like every vhost do.

The reason there are two copies is that fastcgi_ssl_params.conf includes hardcoded fastcgi_param HTTPS on;, which is required for SSL enabled sites, and using just separate copy is more sane than some funky on the fly tweaks. Also, removing it would break all sites using it, until re-verified.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

anarcat’s picture

Priority: Normal » Minor
Status: Closed (fixed) » Needs work

The configuration file is still there and still duplicates the configuration file shipped with the Debian package.

Now, maybe that file is not present in the upstream Nginx distribution so that we need to ship our own. But I am not sure about that - it seems to me it would be fair to assume this file is already loaded unless proven otherwise on a specific platform.

Here's the diff between the aegir and debian fastcgi_params configuration file:

--- /var/aegir/config/includes/fastcgi_params.conf      2013-04-19 17:05:13.637971039 -0400
+++ /etc/nginx/fastcgi_params   2012-09-29 23:36:49.000000000 -0400
@@ -1,22 +1,25 @@
-#
-# fastcgi_params.conf for Aegir
-#
-fastcgi_param  SCRIPT_FILENAME    $document_root$fastcgi_script_name;
 fastcgi_param  QUERY_STRING       $query_string;
 fastcgi_param  REQUEST_METHOD     $request_method;
 fastcgi_param  CONTENT_TYPE       $content_type;
 fastcgi_param  CONTENT_LENGTH     $content_length;
+
+fastcgi_param  SCRIPT_FILENAME         $request_filename;
 fastcgi_param  SCRIPT_NAME        $fastcgi_script_name;
 fastcgi_param  REQUEST_URI        $request_uri;
 fastcgi_param  DOCUMENT_URI       $document_uri;
 fastcgi_param  DOCUMENT_ROOT      $document_root;
 fastcgi_param  SERVER_PROTOCOL    $server_protocol;
+
 fastcgi_param  GATEWAY_INTERFACE  CGI/1.1;
 fastcgi_param  SERVER_SOFTWARE    nginx/$nginx_version;
+
 fastcgi_param  REMOTE_ADDR        $remote_addr;
 fastcgi_param  REMOTE_PORT        $remote_port;
 fastcgi_param  SERVER_ADDR        $server_addr;
 fastcgi_param  SERVER_PORT        $server_port;
 fastcgi_param  SERVER_NAME        $server_name;
+
+fastcgi_param  HTTPS                   $https;
+
+# PHP only, required if PHP was built with --enable-force-cgi-redirect
 fastcgi_param  REDIRECT_STATUS    200;
-fastcgi_index  index.php;
omega8cc’s picture

This file is critically important, so I'm against using shipped (or not) and Drupal compatible (or not) version.

Even if we assume that the shipped version will not break things on some upgrade in the future (and this is already too optimistic), there are some known problems with the defaults used.

While using $request_filename for SCRIPT_FILENAME is as good as using $document_root$fastcgi_script_name it is good only thanks to the rewrites we have in place, as otherwise it would never work for Drupal with clean URLs enabled.

But using $https variable for fastcgi_param HTTPS is something which requires Nginx version >= 1.1.11 and this would break support for current Debian and Ubuntu LTS versions. That is why we use separate copy for SSL enabled vhosts with HTTPS hardcoded.

anarcat’s picture

For HTTPS - can't we just override the variable for < 1.1?

I am not sure I understand the issue with SCRIPT_FILENAME - can we use the upstream value or not?

As to whether or not we can rely on the upstream configuration file being there, I'd like to see some actual data as to whether or not that file is available outside of debian (say in a stock Nginx install).

Thanks!

omega8cc’s picture

For HTTPS - can't we just override the variable for < 1.1?

We would need to *define* this variable in the server.tpl.php template for older Nginx versions and add yet another check for Nginx version used, but it is doable, of course.

SCRIPT_FILENAME - can we use the upstream value or not

Yes, as long as we have the current logic/rewrites for handling clean URLs in the config include.

As to whether or not we can rely on the upstream configuration file being there, I'd like to see some actual data as to whether or not that file is available outside of debian (say in a stock Nginx install).

Good question. There are bad news. Stock Nginx (just checked latest 1.4.0) comes with fastcgi_params file below:


fastcgi_param  QUERY_STRING       $query_string;
fastcgi_param  REQUEST_METHOD     $request_method;
fastcgi_param  CONTENT_TYPE       $content_type;
fastcgi_param  CONTENT_LENGTH     $content_length;

fastcgi_param  SCRIPT_NAME        $fastcgi_script_name;
fastcgi_param  REQUEST_URI        $request_uri;
fastcgi_param  DOCUMENT_URI       $document_uri;
fastcgi_param  DOCUMENT_ROOT      $document_root;
fastcgi_param  SERVER_PROTOCOL    $server_protocol;
fastcgi_param  HTTPS              $https if_not_empty;

fastcgi_param  GATEWAY_INTERFACE  CGI/1.1;
fastcgi_param  SERVER_SOFTWARE    nginx/$nginx_version;

fastcgi_param  REMOTE_ADDR        $remote_addr;
fastcgi_param  REMOTE_PORT        $remote_port;
fastcgi_param  SERVER_ADDR        $server_addr;
fastcgi_param  SERVER_PORT        $server_port;
fastcgi_param  SERVER_NAME        $server_name;

# PHP only, required if PHP was built with --enable-force-cgi-redirect
fastcgi_param  REDIRECT_STATUS    200;

Note that it doesn't include the line:

fastcgi_param  SCRIPT_FILENAME    $document_root$fastcgi_script_name;

And it includes the line:

fastcgi_param  HTTPS              $https if_not_empty;

Where "if_not_empty" requires Nginx version >= 1.1.11

But! The old Nginx versions (from sources) come without HTTPS line at all, so we would have to set them in the vhost (!), which requires that the vhost must be up-to-date (the site re-verified), which only further complicates things, because we would have to check Nginx version on every site verify (not just server verify).

This is exactly why I'm against using shipped (or not) and Drupal compatible (or not) fastcgi_params version - because the defaults are unreliable and are OS distro (or Nginx source) specific and we are only adding unpredictable complexity here, instead of simply ship the same two files we ship so far, which doesn't require any extra version check, because we don't use version specific variables for HTTPS.

anarcat’s picture

Okay, so if i understand this right, the only problem is with the missing SCRIPT_FILENAME line (we can add that line to our config) and varying HTTPS line (which we can override).

What I want to avoid is just to copy configuration that is already available upstream.

Also, maybe we want to make the nginx dependencies stricter as we go along. For example, Debian squeeze has 1.2.1 in its backports, and that's the version that will ship with wheezy (release on 2013-05-05!). Ubuntu is more problematic: Lucid LTS is still supported until 2015 and has nginx 0.7 (!!), while Precise LTS has 1.1.19 (which seems to address the issues you are mentionning here, so that's okay).

Couldn't we ditch support for Lucid 10.04 LTS? (I am ignoring Hardy 8.04 here, which will not be supported anymore in may.)

omega8cc’s picture

The problem is that we can't assume what is available, because the "upstream" includes different stuff if installed from packages and from sources, and adding more layers of complexity with cross checks in Provision is just an exact opposite to KISS methodology. I think I have provided enough proof that we can't reliably rely on the "upstream" for fastcgi_params and it is too critically important to play with checks and workarounds etc.

anarcat’s picture

I was just trying to make a rational inventory of what was out there. It seems consistent enough to me for a *subset* of what we are currently shipping, so my feeling is that we shouldn't ship that subset.

The variation, again, are just *two* lines, for HTTPS and SCRIPT_FILENAME. It seems overkill to ship a full config file just for those two lines.

omega8cc’s picture

Status: Needs work » Fixed
anarcat’s picture

yaaay! thank you!!

omega8cc’s picture

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

  • Commit fca8e64 on dev-drupal-8, 6.x-2.x, dev-ssl-ip-allocation-refactor, 7.x-3.x, dev-subdir-multiserver, 6.x-2.x-backports, dev-helmo-3.x by omega8cc:
    Issue #1635586 by anarcat - remove duplicate nginx fastcgi params.