Issue 2600 - Normalize ownership and priviliges for Nginx logs

modules/nginx/manifests/init.pp

 class nginx (
     $worker_processes = $nginx::params::worker_processes,
     $worker_connections = $nginx::params::worker_connections,
     $ssl_session_cache =  $nginx::params::ssl_session_cache
   ) inherits nginx::params {
 
   apt::source {'nginx':
     location => "http://nginx.org/packages/ubuntu",
     repos => "nginx",
     key => "7BD9BF62",
@@ skipping 107 matching lines @@
       require => File["/etc/nginx/sites-available/${domain}"],
       content => template('nginx/logrotate.erb')
     }
   }
 
   file {'/etc/logrotate.d/nginx':
     source => 'puppet:///modules/nginx/logrotate',
     require => Package['nginx']
   }
 
-  $log_path = '/var/log/nginx'

[mathias, 2015/07/10 10:57:46]

Just used once and not an nginx::param anyway? Then there's no need for the
explicit $log_path symbol.

[Fred, 2015/07/13 12:53:01]

Done.

-  $user_quoted = shellquote($nginx::params::user)

[mathias, 2015/07/10 10:57:46]

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.

[Fred, 2015/07/13 12:53:00]

Done.

-  $find_cmd_base = ['find', $log_path, '-mindepth', '1', '-maxdepth', '1', '-type', 'f']
-  $find_kill_exec = ['-exec', 'sh', '-c', 'ps -p $$ -o ppid= | xargs kill -TERM', ';']

[mathias, 2015/07/10 10:57:46]

This definitely needs an explaining comment! ;-)

[Fred, 2015/07/13 12:53:00]

Done.

+  $find_cmd_base = [
+    'find', '/var/log/nginx',
+    '-mindepth', '1', '-maxdepth', '1', '-type', 'f',
+  ]
 
-  $find_chown_base = [$find_cmd_base, '-not', '(', '-user', $user_quoted, '-and', '-group', 'adm', ')']
-  $find_chown_exec = ['-ls', '-exec', 'chown', "${user_quoted}.adm", '{}', ';']
+  # Kill the find process to force an exit status != 0 by finding the parent pid
+  # of the exec's sh process
+  $find_kill_exec = [
+    '-exec', 'sh', '-c',
+    'ps -p $$ -o ppid= | xargs kill -TERM',
+     ';',
+  ]
+
+  $find_chown_base = [
+    $find_cmd_base,
+    '-not', '(', '-user', $nginx::params::user, '-and', '-group', 'adm', ')',
+  ]
+  $find_chown_exec = [
+    '-ls', '-exec', 'chown',
+    "${nginx::params::user}.adm", '{}', ';',
+  ]
 
   exec {"set_logfiles_owner":
     command => shellquote($find_chown_base, $find_chown_exec),
     unless => shellquote($find_chown_base, $find_kill_exec),
-    require => Package['nginx'],

[mathias, 2015/07/10 10:57:46]

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

[Fred, 2015/07/13 12:53:01]

Done.

     subscribe => Service['nginx'], 
   }
 
   $find_chmod_base = [$find_cmd_base, '-not', '-perm', '0640']
   $find_chmod_exec = ['-ls', '-exec', 'chmod', '0640', '{}', ';']
 
   exec {"set_logfiles_permissions":
     command => shellquote($find_chmod_base, $find_chmod_exec),
     unless => shellquote($find_chmod_base, $find_kill_exec),
-    require => Package['nginx'],
     subscribe => Service['nginx'],
   }
 
   service {'nginx':
     ensure => running,
     enable => true,
     restart => '/etc/init.d/nginx reload',
     hasstatus => true,
     require => File['/etc/nginx/nginx.conf']
   }
 }