Use nginx defaults for fastcgi_params / uwsgi_params

[wyardley]

Per various discussions (#862), use nginx shipped defaults for fastcgi_params / uwsgi_params.

Obsoletes #855, etc.

I think long-term, we still may need a way to make this more customizable, but I think we have signoff in principle on at least pulling in the current vendor defaults from 1.10 / 1.11.

[wyardley]

@jostmart: how does this look to you, and would you be Ok with this superceding #855?

[wyardley]

It does seem like this line doesn't exist in current upstream, though there is SCRIPT_NAME.

[wyardley]

As noted in comment on the relevant text, this doesn't seem to be in current upstream:
fastcgi_param SCRIPT_FILENAME $request_filename;
so I guess that would make it breaking (though hopefully in a minor way).

[3flex]

Upstream has two files, fastcgi.conf and fastcgi_params. As written the PR syncs with fastcgi_params which doesn't set SCRIPT_FILENAME. fastcgi.conf is more modern and does set it (that's the only difference between them).

I suggest syncing to fastcgi.conf, and renaming this template to fastcgi.conf.erb... that's likely to have the least impact on users while also being explicit about which upstream fastcgi config file it's trying to replicate.

Some background here if you like: https://blog.martinfjordvald.com/2013/04/nginx-config-history-fastcgi_params-versus-fastcgi-conf/

[wyardley]

Thanks, that's very helpful context, so I'm glad I bugged you about this one. And here I had hoped this was going to be simple and uncontroversial.

I agree with you about this, do we just leave an existing /etc/nginx/fastcgi_params alone if it exists, though? I guess if the module doesn't include it, it's not an issue, and maybe safer than assuming we managed it in the past? Should we rename the parameter itself from fastcgi_param?

[3flex]

Yeah I think it should be left alone.

I'd like the parameter to be renamed, but I don't think it's worth it...

[wyardley]

@3flex: pushed a new version with those changes and updated tests.
Also added a fix for #1063, and a test for that case.

ps - Do you think it's worth making them optional (as discussed in #682), so that if they're undef, you can suppress the include entirely in the config? I roughed in the changes, but didn't push or add tests yet.

[3flex]

With this change in, I think it makes sense to default to fastcgi.conf shipped in this module only and leave fastcgi_params alone (if it exists). And allowing users to remove the config would also be useful.

[wyardley]

And allowing users to remove the config would also be useful.

Ok, this version makes it optional, and adds a few more tests that I think make sense.

[3flex]

👍 I think this will clear up all the outstanding FastCGI issues, great work!

Edit: Probably should have checked the test results and spotted that merge conflict before clicking this 🤷‍♂️ but besides that, looks great!

[wyardley]

Edit: Probably should have checked the test results and spotted that merge conflict before clicking this 🤷‍♂️ but besides that, looks great!

@3flex I think the conflict came from another recent change that got merged. I don't know how I missed the rubocop failure. Fixed all that and another minor issue with a test.

[oranenj]

Seems fine to me.

[vchepkov]

Curious, why this block was removed?
Seems like a good idea to keep

[wyardley]

@vchepkov The idea is to synchronize it with the upstream config from nginx.
But it's still possible to configure additional fastcgi params with the module, or refer to a different file.

[vchepkov]

It seems to me this change makes configuration to deviate from upstream package:

# rpm -qf /etc/nginx/fastcgi_params
nginx-1.12.1-1.el7.ngx.x86_64

# rpm -qi nginx
Name        : nginx
Epoch       : 1
Version     : 1.12.1
Release     : 1.el7.ngx
Architecture: x86_64
Install Date: Mon 18 Sep 2017 07:13:02 AM EDT
Group       : System Environment/Daemons
Size        : 2640392
License     : 2-clause BSD-like license
Signature   : RSA/SHA1, Tue 11 Jul 2017 10:20:52 AM EDT, Key ID abf5bd827bd9bf62
Source RPM  : nginx-1.12.1-1.el7.ngx.src.rpm
Build Date  : Tue 11 Jul 2017 09:50:21 AM EDT
Build Host  : centos7-amd64-builder-builder.gnt.nginx.com
Relocations : (not relocatable)
Vendor      : Nginx, Inc.
URL         : http://nginx.org/
Summary     : High performance web server
Description :
nginx [engine x] is an HTTP and reverse proxy server, as well as
a mail proxy server.

# diff -u /etc/nginx/fastcgi_params /etc/nginx/fastcgi.conf 
--- /etc/nginx/fastcgi_params	2017-07-11 09:50:19.000000000 -0400
+++ /etc/nginx/fastcgi.conf	2017-09-16 12:09:44.121363628 -0400
@@ -1,4 +1,6 @@
+# This file managed by puppet on host host.example.com
 
+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;

This change also rolling back 3404c4c

which improves security of the server and is recommended by upstream. I don't think a typical user expects to have their configuration less secure by upgrading puppet module. It would be nice if there was at least an option to easily re-enable previous configuration.

[wyardley]

I think fastcgi.conf is in sync with upstream still: https://github.com/nginx/nginx/blob/f8a9d528df92c7634088e575e5c3d63a1d4ab8ea/conf/fastcgi.conf
I don't think the module is making it less secure than the default distribution; if you think other items should be added, bring that up with nginx.

As best I could tell at the time we made these changes, fastcgi.conf is newer and slightly preferred, so we made the decision to use it instead of fastcgi_params These links exist above but just for reference:
https://www.nginx.com/resources/wiki/start/topics/examples/fastcgiexample/
https://blog.martinfjordvald.com/2013/04/nginx-config-history-fastcgi_params-versus-fastcgi-conf/