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

Set log directory ownership / permissions explicitly #959

Merged
merged 1 commit into from
Nov 2, 2016
Merged

Set log directory ownership / permissions explicitly #959

merged 1 commit into from
Nov 2, 2016

Conversation

wyardley
Copy link
Collaborator

@wyardley wyardley commented Nov 2, 2016

Does this look sane as a fix for #664? This is one we should definitely try and get right, as it could cause problems if the ownership isn't correct on various platforms.

  1. Should this also have an interface in ::nginx as well as ::nginx::config? I think with new parameters, it's better not to do that until we work out the exact structural changes.
  2. @bastelfreak: can you confirm permissions on ArchLinux?

I added log_mode in nginx::config, as I think it's important to have a sane default that's restrictive (0700), but people often do also need to grant group / other permissions on the nginx logdir that are custom. We could make the default 0750 or 0751 if that makes more sense.

@dhoppe
Copy link
Member

dhoppe commented Nov 2, 2016

@wyardley Since you use a different set of user/ group you should use 0750 as mode. Otherwise logging tools like Logstash could have problems accessing the logs.

@wyardley
Copy link
Collaborator Author

wyardley commented Nov 2, 2016

@dhoppe: you're right, updated config and spec tests to have it default to 0750.

@bastelfreak bastelfreak merged commit 3f8ae88 into voxpupuli:master Nov 2, 2016
Rubueno pushed a commit to Rubueno/puppet-nginx that referenced this pull request Oct 19, 2020
Set log directory ownership / permissions explicitly
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants