From aef034a6f6c5ed630dabdcf19247f88fb6ab64ac 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 This CR also adds resources to manage the creation of proxy_cache_path and fastcgi_cache_path directories Fixes #1372 --- manifests/config.pp | 28 +++++++++--- manifests/init.pp | 5 +-- manifests/params.pp | 8 ---- spec/classes/nginx_spec.rb | 79 +++++++-------------------------- templates/conf.d/nginx.conf.erb | 2 +- 5 files changed, 42 insertions(+), 80 deletions(-) diff --git a/manifests/config.pp b/manifests/config.pp index 4de9c5b34..84cd151c6 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, @@ -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, 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..39dfe999e 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( @@ -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', @@ -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 @@ -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 diff --git a/templates/conf.d/nginx.conf.erb b/templates/conf.d/nginx.conf.erb index 85f364980..c19557ed3 100644 --- a/templates/conf.d/nginx.conf.erb +++ b/templates/conf.d/nginx.conf.erb @@ -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 -%>