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

Fixes #1372
  • Loading branch information
@b4ldr
b4ldr committed Oct 27, 2021
1 parent c3d2ae6 commit 691a0e3
Show file tree
Hide file tree
Showing 4 changed files with 2 additions and 79 deletions.
6 changes: 0 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
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
62 changes: 0 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 @@ -1378,14 +1324,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

0 comments on commit 691a0e3

Please sign in to comment.