Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use nginx defaults for fastcgi_params / uwsgi_params #1076

Merged
merged 1 commit into from
Apr 22, 2017
Merged

Use nginx defaults for fastcgi_params / uwsgi_params #1076

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 8 additions & 5 deletions manifests/resource/location.pp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,12 +173,12 @@
Optional[String] $fastcgi = undef,
Optional[String] $fastcgi_index = undef,
Optional[Hash] $fastcgi_param = undef,
String $fastcgi_params = "${::nginx::conf_dir}/fastcgi_params",
Optional[String] $fastcgi_params = "${::nginx::conf_dir}/fastcgi.conf",
Optional[String] $fastcgi_script = undef,
Optional[String] $fastcgi_split_path = undef,
Optional[String] $uwsgi = undef,
Optional[Hash] $uwsgi_param = undef,
String $uwsgi_params = "${nginx::config::conf_dir}/uwsgi_params",
Optional[String] $uwsgi_params = "${nginx::config::conf_dir}/uwsgi_params",
Optional[String] $uwsgi_read_timeout = undef,
Boolean $ssl = false,
Boolean $ssl_only = false,
Expand Down Expand Up @@ -248,15 +248,18 @@

$config_file = "${server_dir}/${server_sanitized}.conf"

if $ensure == present and $fastcgi != undef and !defined(File[$fastcgi_params]) {
# Only try to manage these files if they're the default one (as you presumably
# usually don't want the default template if you're using a custom file.

if $ensure == present and $fastcgi != undef and !defined(File[$fastcgi_params]) and $fastcgi_params == "${::nginx::conf_dir}/fastcgi.conf" {
file { $fastcgi_params:
ensure => present,
mode => '0644',
content => template('nginx/server/fastcgi_params.erb'),
content => template('nginx/server/fastcgi.conf.erb'),
}
}

if $ensure == present and $uwsgi != undef and !defined(File[$uwsgi_params]) {
if $ensure == present and $uwsgi != undef and !defined(File[$uwsgi_params]) and $uwsgi_params == "${::nginx::conf_dir}/uwsgi_params" {
file { $uwsgi_params:
ensure => present,
mode => '0644',
Expand Down
13 changes: 8 additions & 5 deletions manifests/resource/server.pp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,10 +194,10 @@
Optional[String] $fastcgi = undef,
Optional[String] $fastcgi_index = undef,
$fastcgi_param = undef,
String $fastcgi_params = "${::nginx::conf_dir}/fastcgi_params",
Optional[String] $fastcgi_params = "${::nginx::conf_dir}/fastcgi.conf",
Optional[String] $fastcgi_script = undef,
Optional[String] $uwsgi = undef,
String $uwsgi_params = "${nginx::config::conf_dir}/uwsgi_params",
Optional[String] $uwsgi_params = "${nginx::config::conf_dir}/uwsgi_params",
Optional[String] $uwsgi_read_timeout = undef,
Array $index_files = [
'index.html',
Expand Down Expand Up @@ -380,15 +380,18 @@
$root = $www_root
}

if $fastcgi != undef and !defined(File[$fastcgi_params]) {
# Only try to manage these files if they're the default one (as you presumably
# usually don't want the default template if you're using a custom file.

if $fastcgi != undef and !defined(File[$fastcgi_params]) and $fastcgi_params == "${::nginx::conf_dir}/fastcgi.conf" {
file { $fastcgi_params:
ensure => present,
mode => '0644',
content => template('nginx/server/fastcgi_params.erb'),
content => template('nginx/server/fastcgi.conf.erb'),
}
}

if $uwsgi != undef and !defined(File[$uwsgi_params]) {
if $uwsgi != undef and !defined(File[$uwsgi_params]) and $uwsgi_params == "${::nginx::conf_dir}/uwsgi_params" {
file { $uwsgi_params:
ensure => present,
mode => '0644',
Expand Down
50 changes: 48 additions & 2 deletions spec/defines/resource_location_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@

it { is_expected.to contain_class('nginx::config') }
it { is_expected.to contain_concat__fragment('server1-500-33c6aa94600c830ad2d316bb4db36724').with_content(%r{location rspec-test}) }
it { is_expected.not_to contain_file('/etc/nginx/fastcgi_params') }
it { is_expected.not_to contain_file('/etc/nginx/fastcgi.conf') }
it { is_expected.not_to contain_concat__fragment('server1-800-rspec-test-ssl') }
it { is_expected.not_to contain_file('/etc/nginx/rspec-test_htpasswd') }
end
Expand Down Expand Up @@ -844,7 +844,41 @@
context 'when fastcgi => "localhost:9000"' do
let(:params) { { fastcgi: 'localhost:9000', server: 'server1' } }

it { is_expected.to contain_file('/etc/nginx/fastcgi_params').with_mode('0644') }
it { is_expected.to contain_file('/etc/nginx/fastcgi.conf').with_mode('0644') }
end

context 'when fastcgi_params is non-default' do
let(:params) do
{
location: 'location',
fastcgi: 'localhost:9000',
fastcgi_params: '/etc/nginx/mycustomparams',
server: 'server1'
}
end

it { is_expected.not_to contain_file('/etc/nginx/mycustomparams') }
it do
is_expected.to contain_concat__fragment('server1-500-' + Digest::MD5.hexdigest(params[:location].to_s)).
with_content(%r{include\s+/etc/nginx/mycustomparams;})
end
end

context 'when fastcgi_params is undef' do
let(:params) do
{
location: 'location',
fastcgi: 'localhost:9000',
fastcgi_params: nil,
server: 'server1'
}
end

it { is_expected.not_to contain_file('/etc/nginx/fastcgi.conf') }
it do
is_expected.to contain_concat__fragment('server1-500-' + Digest::MD5.hexdigest(params[:location].to_s)).
without_content(%r{include\s+/etc/nginx/fastcgi.conf;})
end
end

context 'when uwsgi => "unix:/home/project/uwsgi.socket"' do
Expand All @@ -853,6 +887,18 @@
it { is_expected.to contain_file('/etc/nginx/uwsgi_params') }
end

context 'when uwsgi_params is non-default' do
let(:params) do
{
uwsgi: 'uwsgi_upstream',
uwsgi_params: '/etc/nginx/bogusparams',
server: 'server1'
}
end

it { is_expected.not_to contain_file('/etc/nginx/uwsgi_params') }
end

context 'when ssl_only => true' do
let(:params) { { ssl_only: true, server: 'server1', www_root: '/' } }
it { is_expected.not_to contain_concat__fragment('server1-500-' + Digest::MD5.hexdigest('rspec-test')) }
Expand Down
34 changes: 32 additions & 2 deletions spec/defines/resource_server_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@
it { is_expected.to contain_concat__fragment("#{title}-header").with_content(%r{error_log\s+/var/log/nginx/www\.rspec\.example\.com\.error\.log}) }
it { is_expected.to contain_concat__fragment("#{title}-footer") }
it { is_expected.to contain_nginx__resource__location("#{title}-default") }
it { is_expected.not_to contain_file('/etc/nginx/fastcgi_params') }
it { is_expected.not_to contain_file('/etc/nginx/fastcgi.conf') }
it do
is_expected.to contain_file("#{title}.conf symlink").with('ensure' => 'link',
'path' => "/etc/nginx/sites-enabled/#{title}.conf",
Expand Down Expand Up @@ -1002,7 +1002,28 @@
default_params.merge(fastcgi: 'localhost:9000')
end

it { is_expected.to contain_file('/etc/nginx/fastcgi_params').with_mode('0644') }
it { is_expected.to contain_nginx__resource__location("#{title}-default").with_fastcgi_params('/etc/nginx/fastcgi.conf') }
it { is_expected.to contain_file('/etc/nginx/fastcgi.conf').with_mode('0644') }
end

context 'when fastcgi_params is non-default' do
let :params do
default_params.merge(fastcgi: 'localhost:9000',
fastcgi_params: '/etc/nginx/mycustomparams')
end

it { is_expected.to contain_nginx__resource__location("#{title}-default").with_fastcgi_params('/etc/nginx/mycustomparams') }
it { is_expected.not_to contain_file('/etc/nginx/mycustomparams') }
end

context 'when fastcgi_params is not defined' do
let :params do
default_params.merge(fastcgi: 'localhost:9000',
fastcgi_params: nil)
end

it { is_expected.to contain_nginx__resource__location("#{title}-default").with_fastcgi_params('nil') }
it { is_expected.not_to contain_file('/etc/nginx/fastcgi.conf') }
end

context 'when fastcgi_index => "index.php"' do
Expand All @@ -1029,6 +1050,15 @@
it { is_expected.to contain_file('/etc/nginx/uwsgi_params').with_mode('0644') }
end

context 'when uwsgi_params is non-default' do
let :params do
default_params.merge(uwsgi: 'uwsgi_upstream',
uwsgi_params: '/etc/nginx/bogusparams')
end

it { is_expected.not_to contain_file('/etc/nginx/bogusparams') }
end

context 'when listen_port == ssl_port but ssl = false' do
let :params do
default_params.merge(listen_port: 80,
Expand Down
9 changes: 3 additions & 6 deletions templates/server/fastcgi_params.erb → templates/server/fastcgi.conf.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,16 +1,18 @@
# This file managed by puppet on host <%= @fqdn %>

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 REQUEST_SCHEME $scheme;
fastcgi_param HTTPS $https if_not_empty;

fastcgi_param GATEWAY_INTERFACE CGI/1.1;
fastcgi_param SERVER_SOFTWARE nginx/$nginx_version;
Expand All @@ -21,10 +23,5 @@ 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;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

# Mitigate httpoxy, see https://httpoxy.org/#fix-now
fastcgi_param HTTP_PROXY "";
2 changes: 2 additions & 0 deletions templates/server/locations/fastcgi.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,9 @@
<% if defined? @www_root -%>
root <%= @www_root %>;
<% end -%>
<% if defined? @fastcgi_params -%>
include <%= @fastcgi_params %>;
<% end -%>

fastcgi_pass <%= @fastcgi %>;
<% if @fastcgi_index -%>
Expand Down
29 changes: 16 additions & 13 deletions templates/server/uwsgi_params.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,18 @@
# This file managed by puppet on host <%= @fqdn %>

uwsgi_param QUERY_STRING $query_string;
uwsgi_param REQUEST_METHOD $request_method;
uwsgi_param CONTENT_TYPE $content_type;
uwsgi_param CONTENT_LENGTH $content_length;
uwsgi_param REQUEST_URI $request_uri;
uwsgi_param PATH_INFO $document_uri;
uwsgi_param DOCUMENT_ROOT $document_root;
uwsgi_param SERVER_PROTOCOL $server_protocol;
uwsgi_param REMOTE_ADDR $remote_addr;
uwsgi_param REMOTE_PORT $remote_port;
uwsgi_param SERVER_ADDR $server_addr;
uwsgi_param SERVER_PORT $server_port;
uwsgi_param SERVER_NAME $server_name;
uwsgi_param QUERY_STRING $query_string;
uwsgi_param REQUEST_METHOD $request_method;
uwsgi_param CONTENT_TYPE $content_type;
uwsgi_param CONTENT_LENGTH $content_length;

uwsgi_param REQUEST_URI $request_uri;
uwsgi_param PATH_INFO $document_uri;
uwsgi_param DOCUMENT_ROOT $document_root;
uwsgi_param SERVER_PROTOCOL $server_protocol;
uwsgi_param REQUEST_SCHEME $scheme;
uwsgi_param HTTPS $https if_not_empty;

uwsgi_param REMOTE_ADDR $remote_addr;
uwsgi_param REMOTE_PORT $remote_port;
uwsgi_param SERVER_PORT $server_port;
uwsgi_param SERVER_NAME $server_name;