search

voxpupuli / puppet-nginx

Puppet Module to manage NGINX on various UNIXes
https://forge.puppet.com/puppet/nginx
MIT License
470 stars 881 forks source link

Log directory ownership and permissions do not respect OS #664

Closed Maliuta closed 7 years ago

Maliuta commented 9 years ago

The setting up of the $log_dir does not respect the variance in operating systems, and what the owner and group of that directory should be. Currently it is set to owner => $global_user, group =>$global_group,

This should vary with different operating systems - for example in Debian/Ubuntu it should be: owner => 'www-data', group => 'adm',

wyardley commented 7 years ago

One other option would be to not force creation of it? I think in most cases, the package handles creation of the log directories.

wyardley commented 7 years ago

It seems like on most (all?) platforms, $daemon_user should also be the owner of the log directories

Default: nginx ArchLinux: http Debian / Ubuntu: www-data FreeBSD: www

There isn't a direct equivalent that I can see for the group. Would it make sense to use daemon_user as the owner, and maybe make a new parameter for the log group as well?

wyardley commented 7 years ago

I've got a branch that will set this explicitly (and, potentially, also force the mode). However, the existing code doesn't force the owner or mode, and that may be a good thing.

The default permissions, based on spec tests, are mode 0644), so I think we'd be seeing a lot of problems if the ownership and permissions were not already coming from the package, rather than from our module. I'm not totally convinced that the module should handle this if it doesn't have to.

Also, it seems like different package sources even within the same platform expect different ownership of the log directory and logfiles, for example, on CentOS the official nginx package seems to create the log dir and log files as root:root, but the epel (and possibly the Phusion / passenger) package do nginx:nginx.

It does seem like a good idea to force 0751 or 0700 on the directory permissions, though doing so might require adding another configurable knob for end-users who want to set more restrictive permissions.

Which platform are you actually seeing this problem come up on, and which version of the nginx package (i.e., which package_source and manage_repos settings are you using)? I'm a bit concerned that a fix might cause more problems than it solves.

That said, my first pass at this is at: https://github.com/wyardley/puppet-nginx/tree/issues_664_log_ownership I'm going to try to get a bit more discussion of how it should work before formally putting this in as a PR

wyardley commented 7 years ago

Oh right, the File resource has it set to global_owner / global_group in https://github.com/voxpupuli/puppet-nginx/blob/master/manifests/config.pp#L223 I can verify that it is forcing permissions as described above, so I guess we do have to figure out how to do this in a "smart" way.

wyardley commented 7 years ago

959 is now merged. I would love it if you (or anyone interested) can test this "in the wild" before the next release, otherwise, we may have some angry people on our hands if the behavior isn't as expected on certain platforms.