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

make directory mode configurable for client_body_temp_path and proxy_temp_path #1475

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

make directory mode configurable for client_body_temp_path and proxy_temp_path #1475

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
36 changes: 27 additions & 9 deletions REFERENCE.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,10 @@ The following parameters are available in the `nginx` class:
* [`reset_timedout_connection`](#-nginx--reset_timedout_connection)
* [`nginx_snippets`](#-nginx--nginx_snippets)
* [`nginx_snippets_defaults`](#-nginx--nginx_snippets_defaults)
* [`proxy_temp_mode`](#-nginx--proxy_temp_mode)
* [`proxy_temp_path`](#-nginx--proxy_temp_path)
* [`client_body_temp_path`](#-nginx--client_body_temp_path)
* [`client_body_temp_mode`](#-nginx--client_body_temp_mode)
* [`confd_only`](#-nginx--confd_only)
* [`confd_purge`](#-nginx--confd_purge)
* [`conf_dir`](#-nginx--conf_dir)
Expand All @@ -104,7 +107,6 @@ The following parameters are available in the `nginx` class:
* [`nginx_error_log`](#-nginx--nginx_error_log)
* [`nginx_error_log_severity`](#-nginx--nginx_error_log_severity)
* [`pid`](#-nginx--pid)
* [`proxy_temp_path`](#-nginx--proxy_temp_path)
* [`root_group`](#-nginx--root_group)
* [`sites_available_owner`](#-nginx--sites_available_owner)
* [`sites_available_group`](#-nginx--sites_available_group)
Expand Down Expand Up @@ -326,11 +328,35 @@ Can be used to define default values for the parameter `nginx_snippets`.

Default value: `{}`

##### <a name="-nginx--proxy_temp_mode"></a>`proxy_temp_mode`

Data type: `Optional[Stdlib::Filemode]`

Permissions for the $proxy_temp_path file resource.

Default value: `undef`

##### <a name="-nginx--proxy_temp_path"></a>`proxy_temp_path`

Data type: `Optional[Stdlib::Absolutepath]`

Directory for storing temporary files with data received from proxied servers.

Default value: `undef`

##### <a name="-nginx--client_body_temp_path"></a>`client_body_temp_path`

Data type: `Optional[Stdlib::Absolutepath]`

Directory for storing temporary files holding client request bodies.

Default value: `undef`

##### <a name="-nginx--client_body_temp_mode"></a>`client_body_temp_mode`

Data type: `Optional[Stdlib::Filemode]`

Permissions for the $client_body_temp_path file resource.

Default value: `undef`

Expand Down Expand Up @@ -502,14 +528,6 @@ Data type: `Any`

Default value: `$nginx::params::pid`

##### <a name="-nginx--proxy_temp_path"></a>`proxy_temp_path`

Data type: `Optional[Stdlib::Absolutepath]`



Default value: `undef`

##### <a name="-nginx--root_group"></a>`root_group`

Data type: `Any`
Expand Down
4 changes: 2 additions & 2 deletions manifests/config.pp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,15 +206,15 @@
file { $client_body_temp_path:
ensure => directory,
owner => $daemon_user,
mode => '0700',
mode => $nginx::client_body_temp_mode,
}
}

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

Expand Down
14 changes: 14 additions & 0 deletions manifests/init.pp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,9 +44,22 @@
# @param nginx_snippets_defaults
# Can be used to define default values for the parameter `nginx_snippets`.
#
# @param proxy_temp_mode
# Permissions for the $proxy_temp_path file resource.
#
# @param proxy_temp_path
# Directory for storing temporary files with data received from proxied servers.
#
# @param client_body_temp_path
# Directory for storing temporary files holding client request bodies.
#
# @param client_body_temp_mode
# Permissions for the $client_body_temp_path file resource.
#
class nginx (
### START Nginx Configuration ###
Optional[Stdlib::Absolutepath] $client_body_temp_path = undef,
Optional[Stdlib::Filemode] $client_body_temp_mode = undef,
Copy link
Member

Choose a reason for hiding this comment

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

I think that's a breaking change. In #1443 mode was set to 0700 to make the module idempotent. Probably this change will break this?

Copy link
Author

Choose a reason for hiding this comment

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

i could change the module parameter default value to 0700. the reason why i chose undef as default value, is, because it is the only way to ensure puppet does not manage the filemode unless explicitly told to do so. changing the default value to NotUndef would no longer allow this.

# this is the same as not configuring it at all
nginx::client_body_temp_mode: ~

i do not know in which version of nginx this was implemented, but current versions manage the filemode on its own.

this leaves us with 3 options:

leave the default value to undef

this would leave the mode management to either nginx (in newer versions)
or the system umask (in older versions)

change the default value to 0700

this would not change the behaviour of the puppet
module but would require the module user to
set this value to whatever nginx uses (in newer
versions) otherwise the puppet runs will report changes every time

implement explicit hands off logic

$real_client_body_temp_mode = $nginx::manage_client_body_temp_mode ? {
  true    => $nginx::client_body_temp_mode,
  default => undef,
}

Copy link
Member

Choose a reason for hiding this comment

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

Setting this to undef will probably cause some issues as well, if the directory and the file don't exists, puppet will create them with 0644 permissions.

If the mode is omitted (or explicitly set to undef), Puppet does not enforce permissions on existing files and creates new files with permissions of 0644.
The documentation doesn't state what will happen on next Puppet run though

The weird thing is that before #1443 PR even though the parameter was omitted, Puppet still changed the mode 0755
Or maybe I'm missing something?

Copy link
Author

Choose a reason for hiding this comment

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

puppet creates directories with 0755 permissions when no mode parameter is specified

root@01b032aec128:/src# cat poc.pp 
file { '/opt/poc':
  ensure => directory,
  owner  => '0',
  group  => '0',
}

root@01b032aec128:/src# puppet apply ./poc.pp 
Notice: Compiled catalog for 01b032aec128.example.com in environment production in 0.03 seconds
Notice: /Stage[main]/Main/File[/opt/poc]/ensure: created
Notice: Applied catalog in 0.15 seconds

root@01b032aec128:/src# stat /opt/poc
  File: /opt/poc
  Size: 4096      	Blocks: 8          IO Block: 4096   directory
Device: 2ch/44d	Inode: 3020743     Links: 2
Access: (0755/drwxr-xr-x)  Uid: (    0/    root)   Gid: (    0/    root)
Access: 2021-11-27 07:05:40.301078681 +0000
Modify: 2021-11-27 07:05:40.301078681 +0000
Change: 2021-11-27 07:05:40.301078681 +0000
 Birth: -

root@01b032aec128:/src# puppet --version
7.12.1

Copy link
Member

Choose a reason for hiding this comment

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

+1 for setting the default value to the current default value of 0700 and not break backward compatibility. If some systems need different values, they are already affected, and their users are welcome to send e.g. hiera data for these edge cases.

Boolean $confd_only = false,
Boolean $confd_purge = false,
$conf_dir = $nginx::params::conf_dir,
Expand All @@ -69,6 +82,7 @@
Nginx::ErrorLogSeverity $nginx_error_log_severity = 'error',
$pid = $nginx::params::pid,
Optional[Stdlib::Absolutepath] $proxy_temp_path = undef,
Optional[Stdlib::Filemode] $proxy_temp_mode = undef,
$root_group = $nginx::params::root_group,
$sites_available_owner = $nginx::params::sites_available_owner,
$sites_available_group = $nginx::params::sites_available_group,
Expand Down
46 changes: 46 additions & 0 deletions spec/classes/nginx_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1427,6 +1427,52 @@
it { is_expected.to contain_file('/var/log/nginx').with(mode: '0771') }
end

context 'when proxy_temp_path is non-default' do
let(:params) { { proxy_temp_path: '/tmp/nginx_proxy' } }

it do
is_expected.to contain_file('/tmp/nginx_proxy').
without('mode')
end
end

context 'when proxy_temp_mode is non-default' do
let(:params) do
{
proxy_temp_path: '/tmp/nginx_proxy',
proxy_temp_mode: '0771',
}
end

it do
is_expected.to contain_file('/tmp/nginx_proxy').
with_mode('0771')
end
end

context 'when client_body_temp_path is non-default' do
let(:params) { { client_body_temp_path: '/tmp/nginx_client' } }

it do
is_expected.to contain_file('/tmp/nginx_client').
without('mode')
end
end

context 'when client_body_temp_mode is non-default' do
let(:params) do
{
client_body_temp_path: '/tmp/nginx_client',
client_body_temp_mode: '0771',
}
end

it do
is_expected.to contain_file('/tmp/nginx_client').
with_mode('0771')
end
end

context 'when gzip is non-default (on) test gzip defaults' do
let(:params) { { gzip: 'on' } }

Expand Down