-
-
Notifications
You must be signed in to change notification settings - Fork 882
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.
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
base: master
Are you sure you want to change the base?
Conversation
nginx::config is a classthat may have no external impact to Forge modules. nginx is a classthat may have no external impact to Forge modules. This module is declared in 9 of 578 indexed public
|
@@ -41,6 +41,7 @@ | |||
class nginx ( | |||
### START Nginx Configuration ### | |||
Variant[Stdlib::Absolutepath, Boolean] $client_body_temp_path = $nginx::params::client_body_temp_path, | |||
Optional[Stdlib::Filemode] $client_body_temp_mode = undef, |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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,
}
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add puppet-strings @param
documentation to new parameters.
bf5e198
to
ce2c70c
Compare
ce2c70c
to
7a1e4ad
Compare
is there anything else that needs to be done for this PR to be merged? the failing //cc @ekohl |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please update your puppet-strings version and the regenerate REFERENCE.md
. It's outputting the old format which generates a HUGE diff.
nginx manages the directory permissions on its own, so the default value is undef to avoid conflicts.
7a1e4ad
to
d1e5365
Compare
i was still using ruby 2.5; puppet-strings 3.x requires ruby2.7+, which is why my local dev env never got a newer gem version. i have rebuilt the reference using ruby-2.7/puppet-strings-3.0.1 |
My concern has been addressed. I didn't look at the specific changes.
Pull Request (PR) description
nginx manages the directory permissions on its own,
so the default value is
undef
to avoid conflicts.This Pull Request (PR) fixes the following issues
Related to #1443