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.

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

Log directory ownership and permissions do not respect OS #664

Closed
Maliuta opened this issue Aug 3, 2015 · 5 comments
Closed

Log directory ownership and permissions do not respect OS #664

Maliuta opened this issue Aug 3, 2015 · 5 comments
Assignees
Labels
bug Something isn't working enhancement New feature or request

Comments

@Maliuta
Copy link

Maliuta commented Aug 3, 2015

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',

@3flex 3flex added the enhancement New feature or request label Aug 12, 2015
@wyardley wyardley self-assigned this Oct 11, 2016
@wyardley
Copy link
Collaborator

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
Copy link
Collaborator

wyardley commented Nov 2, 2016

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
Copy link
Collaborator

wyardley commented Nov 2, 2016

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
Copy link
Collaborator

wyardley commented Nov 2, 2016

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
Copy link
Collaborator

wyardley commented Nov 2, 2016

#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.

@wyardley wyardley closed this as completed Nov 2, 2016
@wyardley wyardley added bug Something isn't working and removed needs discussion labels Nov 2, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants