Issue 29321355: Issue 2600 - Normalize ownership and priviliges for Nginx logs (Closed)
DescriptionIssue 2600 - Normalize ownership and priviliges for Nginx logs
Patch Set 1 #
Total comments: 6
Patch Set 2 : Approach using two execs and find #
Total comments: 2
Patch Set 3 : A more elegant solution #
Total comments: 8
Patch Set 4 : Patch-set adressing latest remarks #Patch Set 5 : Apply 80 character line length formatting #MessagesTotal messages: 11
https://codereview.adblockplus.org/29321355/diff/29321356/modules/nginx/manif... File modules/nginx/manifests/init.pp (right): https://codereview.adblockplus.org/29321355/diff/29321356/modules/nginx/manif... modules/nginx/manifests/init.pp:123: command => '/bin/chown www-data.adm /var/log/nginx/* && /bin/chmod 0640 /var/log/nginx/*', Except for intermediate fixes, please do not use shell commands - especially not with logic operators (&&). By the way, there's no need to make the chmod dependent on a successful chown here. Why not just creating two exec's? Also, the asterisk wildcard can very well lead to more issues; it may expand to either unrecognized sub-directories or, altogether, in a too long command line. What about `find /var/log/nginx -mindepth 1 ... -exec ...`? And please take the user from $nginx::params::user (the group can remain hard-coded). Have a look at the shellquote() function at https://docs.puppetlabs.com/references/latest/function.html#shellquote - it's the best way to integrate parameters (like the user name) with your command. https://codereview.adblockplus.org/29321355/diff/29321356/modules/nginx/manif... modules/nginx/manifests/init.pp:125: # require => [File['/var/log/nginx'], User['www-data'], Group['adm']] This comment is both wrong (neither User['www-data'] nor Group['adm'] is provided by Package['nginx']) and unnecessary. The require parameter below should say enough. https://codereview.adblockplus.org/29321355/diff/29321356/modules/nginx/manif... modules/nginx/manifests/init.pp:126: require => Package['nginx'] Please always use trailing commas, even when defining the last item. (I know this has been forgotten before, even in this file.)
https://codereview.adblockplus.org/29321355/diff/29321356/modules/nginx/manif... File modules/nginx/manifests/init.pp (right): https://codereview.adblockplus.org/29321355/diff/29321356/modules/nginx/manif... modules/nginx/manifests/init.pp:123: command => '/bin/chown www-data.adm /var/log/nginx/* && /bin/chmod 0640 /var/log/nginx/*', On 2015/07/03 13:51:04, mathias wrote: > Except for intermediate fixes, please do not use shell commands - especially not > with logic operators (&&). By the way, there's no need to make the chmod > dependent on a successful chown here. Why not just creating two exec's? > > Also, the asterisk wildcard can very well lead to more issues; it may expand to > either unrecognized sub-directories or, altogether, in a too long command line. > What about `find /var/log/nginx -mindepth 1 ... -exec ...`? > > And please take the user from $nginx::params::user (the group can remain > hard-coded). Have a look at the shellquote() function at > https://docs.puppetlabs.com/references/latest/function.html#shellquote - it's > the best way to integrate parameters (like the user name) with your command. Acknowledged. https://codereview.adblockplus.org/29321355/diff/29321356/modules/nginx/manif... modules/nginx/manifests/init.pp:125: # require => [File['/var/log/nginx'], User['www-data'], Group['adm']] On 2015/07/03 13:51:04, mathias wrote: > This comment is both wrong (neither User['www-data'] nor Group['adm'] is > provided by Package['nginx']) and unnecessary. The require parameter below > should say enough. Acknowledged. https://codereview.adblockplus.org/29321355/diff/29321356/modules/nginx/manif... modules/nginx/manifests/init.pp:126: require => Package['nginx'] On 2015/07/03 13:51:04, mathias wrote: > Please always use trailing commas, even when defining the last item. (I know > this has been forgotten before, even in this file.) Acknowledged.
While that probably works quite well,... isn't there a more elegant solution, based on lists instead of strings or similar? https://codereview.adblockplus.org/29321355/diff/29321423/modules/nginx/manif... File modules/nginx/manifests/init.pp (right): https://codereview.adblockplus.org/29321355/diff/29321423/modules/nginx/manif... modules/nginx/manifests/init.pp:131: path => '/bin:/usr/bin', Please use "/usr/bin:/bin". https://codereview.adblockplus.org/29321355/diff/29321423/modules/nginx/manif... modules/nginx/manifests/init.pp:141: path => '/bin:/usr/bin', It may actually be easier to use a class-wide `Exec { path => "..." }` instead of applying the aforementioned change twice.
Way better. What about the 80 character limit? https://codereview.adblockplus.org/29321355/diff/29321562/modules/nginx/manif... File modules/nginx/manifests/init.pp (right): https://codereview.adblockplus.org/29321355/diff/29321562/modules/nginx/manif... modules/nginx/manifests/init.pp:128: $log_path = '/var/log/nginx' Just used once and not an nginx::param anyway? Then there's no need for the explicit $log_path symbol. https://codereview.adblockplus.org/29321355/diff/29321562/modules/nginx/manif... modules/nginx/manifests/init.pp:129: $user_quoted = shellquote($nginx::params::user) As far as I can see, there is no need to explicitly quote the user here. Every list this serves as an item for is passed to shellquote() later on anyway. https://codereview.adblockplus.org/29321355/diff/29321562/modules/nginx/manif... modules/nginx/manifests/init.pp:131: $find_kill_exec = ['-exec', 'sh', '-c', 'ps -p $$ -o ppid= | xargs kill -TERM', ';'] This definitely needs an explaining comment! ;-) https://codereview.adblockplus.org/29321355/diff/29321562/modules/nginx/manif... modules/nginx/manifests/init.pp:139: require => Package['nginx'], With the subscription to Service['nginx'] the requirement for the Package is implicit. While this does not automatically render an explicit requirement wrong (one may express some particular depenency or similar), it is often considered redundant. IMHO we can drop it here (and in set_logfiles_permissions as well).
https://codereview.adblockplus.org/29321355/diff/29321562/modules/nginx/manif... File modules/nginx/manifests/init.pp (right): https://codereview.adblockplus.org/29321355/diff/29321562/modules/nginx/manif... modules/nginx/manifests/init.pp:128: $log_path = '/var/log/nginx' On 2015/07/10 10:57:46, mathias wrote: > Just used once and not an nginx::param anyway? Then there's no need for the > explicit $log_path symbol. Done. https://codereview.adblockplus.org/29321355/diff/29321562/modules/nginx/manif... modules/nginx/manifests/init.pp:129: $user_quoted = shellquote($nginx::params::user) On 2015/07/10 10:57:46, mathias wrote: > As far as I can see, there is no need to explicitly quote the user here. Every > list this serves as an item for is passed to shellquote() later on anyway. Done. https://codereview.adblockplus.org/29321355/diff/29321562/modules/nginx/manif... modules/nginx/manifests/init.pp:131: $find_kill_exec = ['-exec', 'sh', '-c', 'ps -p $$ -o ppid= | xargs kill -TERM', ';'] On 2015/07/10 10:57:46, mathias wrote: > This definitely needs an explaining comment! ;-) Done. https://codereview.adblockplus.org/29321355/diff/29321562/modules/nginx/manif... modules/nginx/manifests/init.pp:139: require => Package['nginx'], On 2015/07/10 10:57:46, mathias wrote: > With the subscription to Service['nginx'] the requirement for the Package is > implicit. While this does not automatically render an explicit requirement wrong > (one may express some particular depenency or similar), it is often considered > redundant. IMHO we can drop it here (and in set_logfiles_permissions as well). Done.
LGTM. 
|
