Skip to content

Commit

Permalink
(1372) Drop run_dir and make client_body_temp_path/proxy_temp_path op…
Browse files Browse the repository at this point in the history 
…tional

The [general description of `/run/`](https://refspecs.linuxfoundation.org/FHS_3.0/fhs/ch03s15.html) is fairly vague and not in the original spec however my high level reading is that it relates more to small files which are used for service orchestration and not data (temporary or otherwise) which is specific to the programs operations for the later /var/lib which states the following seems more appropriate:

> Furthermore, this hierarchy holds state information pertaining to an application ....  State information is data that programs modify while they run, and that pertains to one specific host.

https://tldp.org/LDP/Linux-Filesystem-Hierarchy/html/Linux-Filesystem-Hierarchy.html#var

And as we see from the [debian packages](https://salsa.debian.org/nginx-team/nginx/-/blob/master/debian/rules#L61-63) this is the route they have gone (and likely don't consider this a bug).  although it is worth noting they only override this only for the full package, the light package keeps the default which ends up being `--prefix` and thus `/usr/share/nginx/client_temp/`.

Of course there is a performance argument to make that says theses files should be in a ramdisk, however its also possible that this dir could become very large and putting it in a ramdisk could cause swapping or other system issues as such i think this decision should be left to the operator with sane defaults provided by the package maintain

As run_dir is only ever used for setting `proxy_temp_path` and `client_body_temp_path` i suggest that we completely drop run_dir and make `client_body_temp_path` and `proxy_temp_path` optional, thus ensuring the configuration uses the distro configured defaults.  if we don't want to use the package defaults then we should ensure we ensure things work correctly annd in this case i think using the systemd dropin would make the most senses

This CR also adds resources to manage the creation of proxy_cache_path
and fastcgi_cache_path directories

Fixes #1372
  • Loading branch information
@b4ldr
b4ldr committed Oct 27, 2021
1 parent c3d2ae6 commit aef034a
Show file tree
Hide file tree
Showing 5 changed files with 42 additions and 80 deletions.
28 changes: 22 additions & 6 deletions manifests/config.pp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@
$pid = $nginx::pid
$proxy_temp_path = $nginx::proxy_temp_path
$root_group = $nginx::root_group
$run_dir = $nginx::run_dir
$sites_available_owner = $nginx::sites_available_owner
$sites_available_group = $nginx::sites_available_group
$sites_available_mode = $nginx::sites_available_mode
Expand Down Expand Up @@ -189,11 +188,6 @@
}
}

file { $run_dir:
ensure => directory,
mode => '0644',
}

if $nginx::manage_snippets_dir {
file { $nginx::snippets_dir:
ensure => directory,
Expand Down Expand Up @@ -223,6 +217,28 @@
}
}

if $fastcgi_cache_path {
file { $fastcgi_cache_path:
ensure => directory,
owner => $daemon_user,
mode => '0700',
}
}

if $proxy_cache_path =~ Hash {
file { $proxy_cache_path.keys():
ensure => directory,
owner => $daemon_user,
mode => '0700',
}
} elsif $proxy_cache_path =~ String {
file { $proxy_cache_path:
ensure => directory,
owner => $daemon_user,
mode => '0700',
}
}

unless $confd_only {
file { "${conf_dir}/sites-available":
ensure => directory,
Expand Down
5 changes: 2 additions & 3 deletions manifests/init.pp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@
#
class nginx (
### START Nginx Configuration ###
Variant[Stdlib::Absolutepath, Boolean] $client_body_temp_path = $nginx::params::client_body_temp_path,
Optional[Stdlib::Absolutepath] $client_body_temp_path = undef,
Boolean $confd_only = false,
Boolean $confd_purge = false,
$conf_dir = $nginx::params::conf_dir,
Expand All @@ -61,9 +61,8 @@
Variant[String, Array[String]] $nginx_error_log = "${log_dir}/${nginx::params::nginx_error_log_file}",
Nginx::ErrorLogSeverity $nginx_error_log_severity = 'error',
$pid = $nginx::params::pid,
Variant[Stdlib::Absolutepath, Boolean] $proxy_temp_path = $nginx::params::proxy_temp_path,
Optional[Stdlib::Absolutepath] $proxy_temp_path = undef,
$root_group = $nginx::params::root_group,
$run_dir = $nginx::params::run_dir,
$sites_available_owner = $nginx::params::sites_available_owner,
$sites_available_group = $nginx::params::sites_available_group,
$sites_available_mode = $nginx::params::sites_available_mode,
Expand Down
8 changes: 0 additions & 8 deletions manifests/params.pp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@
'log_user' => 'nginx',
'log_group' => 'root',
'log_mode' => '0750',
'run_dir' => '/var/nginx',
'package_name' => 'nginx',
'passenger_package_name' => 'passenger',
'manage_repo' => false,
Expand Down Expand Up @@ -113,7 +112,6 @@
'log_user' => 'root',
'log_group' => 'adm',
'log_mode' => '0755',
'run_dir' => '/run/nginx',
}
# The following was designed/tested on Ubuntu 18 and Debian 9/10 but probably works on newer versions as well
} else {
Expand All @@ -123,7 +121,6 @@
'log_user' => 'root',
'log_group' => 'adm',
'log_mode' => '0755',
'run_dir' => '/run/nginx',
'passenger_package_name' => 'libnginx-mod-http-passenger',
'include_modules_enabled' => true,
}
Expand Down Expand Up @@ -180,7 +177,6 @@
'log_dir' => '/var/www/logs',
'log_user' => 'www',
'log_group' => 'wheel',
'run_dir' => '/var/www',
}
}
'AIX': {
Expand All @@ -190,7 +186,6 @@
'conf_dir' => '/opt/freeware/etc/nginx/',
'log_dir' => '/opt/freeware/var/log/nginx/',
'log_group' => 'system',
'run_dir' => '/opt/freeware/share/nginx/html',
}
}
default: {
Expand All @@ -211,12 +206,10 @@
$log_user = $_module_parameters['log_user']
$log_group = $_module_parameters['log_group']
$log_mode = $_module_parameters['log_mode']
$run_dir = $_module_parameters['run_dir']
$temp_dir = '/tmp'
$pid = $_module_parameters['pid']
$include_modules_enabled = $_module_parameters['include_modules_enabled']

$client_body_temp_path = "${run_dir}/client_body_temp"
$daemon_user = $_module_parameters['daemon_user']
$global_owner = 'root'
$global_group = $_module_parameters['root_group']
Expand All @@ -228,7 +221,6 @@
$root_group = $_module_parameters['root_group']
$package_name = $_module_parameters['package_name']
$passenger_package_name = $_module_parameters['passenger_package_name']
$proxy_temp_path = "${run_dir}/proxy_temp"
$sites_available_owner = 'root'
$sites_available_group = $_module_parameters['root_group']
$sites_available_mode = '0644'
Expand Down
79 changes: 17 additions & 62 deletions spec/classes/nginx_spec.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -301,56 +301,6 @@
mode: '0644'
)
end
it do
case facts[:osfamily]
when 'Debian'
is_expected.to contain_file('/run/nginx').with(
ensure: 'directory',
owner: 'root',
group: 'root',
mode: '0644'
)
else
is_expected.to contain_file('/var/nginx').with(
ensure: 'directory',
owner: 'root',
group: 'root',
mode: '0644'
)
end
end
it do
case facts[:osfamily]
when 'Debian'
is_expected.to contain_file('/run/nginx/client_body_temp').with(
ensure: 'directory',
group: 'root',
mode: '0700'
)
else
is_expected.to contain_file('/var/nginx/client_body_temp').with(
ensure: 'directory',
group: 'root',
mode: '0700'
)
end
end
it do
case facts[:osfamily]
when 'Debian'
is_expected.to contain_file('/run/nginx/proxy_temp').with(
ensure: 'directory',
group: 'root',
mode: '0700'
)
else
is_expected.to contain_file('/var/nginx/proxy_temp').with(
ensure: 'directory',
group: 'root',
mode: '0700'
)
end
end
it do
is_expected.to contain_file('/etc/nginx/nginx.conf').with(
ensure: 'file',
Expand Down Expand Up @@ -383,8 +333,6 @@
end
case facts[:osfamily]
when 'RedHat'
it { is_expected.to contain_file('/var/nginx/client_body_temp').with(owner: 'nginx') }
it { is_expected.to contain_file('/var/nginx/proxy_temp').with(owner: 'nginx') }
it { is_expected.to contain_file('/etc/nginx/nginx.conf').with_content %r{^user nginx;} }
it do
is_expected.to contain_file('/var/log/nginx').with(
Expand All @@ -395,8 +343,6 @@
)
end
when 'Debian'
it { is_expected.to contain_file('/run/nginx/client_body_temp').with(owner: 'www-data') }
it { is_expected.to contain_file('/run/nginx/proxy_temp').with(owner: 'www-data') }
it { is_expected.to contain_file('/etc/nginx/nginx.conf').with_content %r{^user www-data;} }
it do
is_expected.to contain_file('/var/log/nginx').with(
Expand Down Expand Up @@ -728,6 +674,12 @@
value: '/path/to/proxy.cache',
match: %r{\s+proxy_cache_path\s+/path/to/proxy.cache levels=1 keys_zone=d2:100m max_size=500m inactive=20m;}
},
{
title: 'should set proxy_cache_path from hash',
attr: 'proxy_cache_path',
value: { '/path/to/proxy.cache' => 'd2:100m' },
match: %r{\s+proxy_cache_path\s+/path/to/proxy.cache levels=1 keys_zone=d2:100m max_size=500m inactive=20m;}
},
{
title: 'should set fastcgi_cache_path',
attr: 'fastcgi_cache_path',
Expand Down Expand Up @@ -1124,6 +1076,17 @@
expect(lines & Array(param[:match])).to eq(Array(param[:match]))
end

# if we have a _path attribute make sure we create the path
if param[:attr].end_with?('_path')
if param[:value].is_a?(Hash)
param[:value].keys.each do |path|
is_expected.to contain_file(path).with_ensure('directory')
end
else
is_expected.to contain_file(param[:value]).with_ensure('directory')
end
end

Array(param[:notmatch]).each do |item|
is_expected.to contain_file('/etc/nginx/nginx.conf').without_content(item)
end
Expand Down Expand Up @@ -1378,14 +1341,6 @@
context 'when daemon_user = www-data' do
let(:params) { { daemon_user: 'www-data' } }

case facts[:osfamily]
when 'Debian'
it { is_expected.to contain_file('/run/nginx/client_body_temp').with(owner: 'www-data') }
it { is_expected.to contain_file('/run/nginx/proxy_temp').with(owner: 'www-data') }
else
it { is_expected.to contain_file('/var/nginx/client_body_temp').with(owner: 'www-data') }
it { is_expected.to contain_file('/var/nginx/proxy_temp').with(owner: 'www-data') }
end
it { is_expected.to contain_file('/etc/nginx/nginx.conf').with_content %r{^user www-data;} }
end

Expand Down
2 changes: 1 addition & 1 deletion templates/conf.d/nginx.conf.erb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -216,7 +216,7 @@ http {
<% end -%>
<% if @proxy_cache_path.is_a?(Hash) -%>
<% @proxy_cache_path.sort_by{|k,v| k}.each do |key,value| -%>
proxy_cache_path <%= key %> keys_zone=<%= value %> levels=<%= @proxy_cache_levels %> max_size=<%= @proxy_cache_max_size %> inactive=<%= @proxy_cache_inactive -%>
proxy_cache_path <%= key %> levels=<%= @proxy_cache_levels %> keys_zone=<%= value %> max_size=<%= @proxy_cache_max_size %> inactive=<%= @proxy_cache_inactive -%>
<%- if @proxy_use_temp_path %> use_temp_path=<%= @proxy_use_temp_path %><% end -%>
<%- if @proxy_cache_loader_files %> loader_files=<%= @proxy_cache_loader_files %><% end -%>
<%- if @proxy_cache_loader_sleep %> loader_sleep=<%= @proxy_cache_loader_sleep %><% end -%>
Expand Down

0 comments on commit aef034a

Please sign in to comment.