From 691a0e33ebab422f2c218124ebe2072e31b25d97 Mon Sep 17 00:00:00 2001 From: John Bond Date: Wed, 27 Oct 2021 12:45:38 +0200 Subject: [PATCH] (1372) Drop run_dir and make client_body_temp_path/proxy_temp_path optional 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 --- manifests/config.pp | 6 ---- manifests/init.pp | 5 ++- manifests/params.pp | 8 ----- spec/classes/nginx_spec.rb | 62 -------------------------------------- 4 files changed, 2 insertions(+), 79 deletions(-) diff --git a/manifests/config.pp b/manifests/config.pp index 4de9c5b34..f3c666514 100644 --- a/manifests/config.pp +++ b/manifests/config.pp @@ -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 @@ -189,11 +188,6 @@ } } - file { $run_dir: - ensure => directory, - mode => '0644', - } - if $nginx::manage_snippets_dir { file { $nginx::snippets_dir: ensure => directory, diff --git a/manifests/init.pp b/manifests/init.pp index 830598ab4..efcb3cb5b 100644 --- a/manifests/init.pp +++ b/manifests/init.pp @@ -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, @@ -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, diff --git a/manifests/params.pp b/manifests/params.pp index 1bfec883f..16074392a 100644 --- a/manifests/params.pp +++ b/manifests/params.pp @@ -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, @@ -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 { @@ -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, } @@ -180,7 +177,6 @@ 'log_dir' => '/var/www/logs', 'log_user' => 'www', 'log_group' => 'wheel', - 'run_dir' => '/var/www', } } 'AIX': { @@ -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: { @@ -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'] @@ -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' diff --git a/spec/classes/nginx_spec.rb b/spec/classes/nginx_spec.rb index 7efe29992..5a872680e 100644 --- a/spec/classes/nginx_spec.rb +++ b/spec/classes/nginx_spec.rb @@ -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', @@ -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( @@ -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( @@ -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