From e128ba6538eb806589ebc01c16ee5e298bb0ee86 Mon Sep 17 00:00:00 2001
From: Diana Clarke <diana.joan.clarke@gmail.com>
Date: Tue, 7 Mar 2017 14:07:16 -0500
Subject: [PATCH] Correct permissions on the nova logfiles

When you execute nova-manage commands, oslo logs to the following
location (file name is dynamically created based on command name).

    /var/log/nova/nova-manage.log

Because puppet-nova is executing these commands as root,
nova-manage.log is owned by root, preventing the 'nova-manage
db archive_deleted_rows' entry in nova's crontab from executing.

    Permission denied: '/var/log/nova/nova-manage.log'

This log file is also an outlier, as all other log files in
/var/log/nova/ are owned by nova:nova.

Similar issues are possible for other nova logs, if for example
a nova services is initially started manually as root, so the
ownership of all nova logs is corrected before configuring nova.

Co-Authored-By: Oliver Walsh <owalsh@redhat.com>
Co-Authored-By: Diana Clarke <diana.joan.clarke@gmail.com>
Co-Authored-By: Maciej Kucia <maciej@kucia.net>
Closes-Bug: #1671681
Change-Id: I0ca0110cbf9139c79074cf603dcab9135f96e765
---
 lib/puppet/provider/nova.rb                   |  5 +++
 manifests/cell_v2/discover_hosts.pp           |  2 +
 manifests/cell_v2/map_cell0.pp                |  2 +
 manifests/cell_v2/map_cell_and_hosts.pp       |  2 +
 manifests/cell_v2/map_instances.pp            |  2 +
 manifests/cron/archive_deleted_rows.pp        |  8 ++--
 manifests/db/online_data_migrations.pp        |  1 +
 manifests/db/sync.pp                          |  1 +
 manifests/db/sync_api.pp                      |  1 +
 manifests/logging.pp                          | 41 +++++++++++++++++++
 manifests/params.pp                           |  6 ++-
 .../nova-manage-user-16e7145d0c10bf57.yaml    | 17 ++++++++
 .../nova_cell_v2_discover_hosts_spec.rb       |  2 +
 spec/classes/nova_cell_v2_map_cell0_spec.rb   |  2 +
 .../nova_cell_v2_map_cell_and_hosts_spec.rb   |  2 +
 spec/classes/nova_cell_v2_map_instances.rb    |  2 +
 .../nova_cron_archive_deleted_rows_spec.rb    |  2 +
 .../nova_db_online_data_migrations_spec.rb    |  3 ++
 spec/classes/nova_db_sync_api_spec.rb         |  3 ++
 spec/classes/nova_db_sync_spec.rb             |  3 ++
 spec/classes/nova_logging_spec.rb             | 25 ++++++++++-
 21 files changed, 126 insertions(+), 6 deletions(-)
 create mode 100644 releasenotes/notes/nova-manage-user-16e7145d0c10bf57.yaml

diff --git a/lib/puppet/provider/nova.rb b/lib/puppet/provider/nova.rb
index 6754c51d7..bb5f3216a 100644
--- a/lib/puppet/provider/nova.rb
+++ b/lib/puppet/provider/nova.rb
@@ -46,6 +46,7 @@ class Puppet::Provider::Nova < Puppet::Provider::Openstack
     # NOTE(mnaser): We pass the arguments as an array to avoid problems with
     #               symbols in the arguments breaking things.
     Puppet::Util::Execution.execute(args, {
+      :uid                => nova_user,
       :failonfail         => true,
       :combine            => false,
       :custom_environment => {}
@@ -56,6 +57,10 @@ class Puppet::Provider::Nova < Puppet::Provider::Openstack
     self.class.nova_manage_request(args)
   end
 
+  def self.nova_user
+    'nova'
+  end
+
   def self.conf_filename
     '/etc/nova/nova.conf'
   end
diff --git a/manifests/cell_v2/discover_hosts.pp b/manifests/cell_v2/discover_hosts.pp
index 82b00f53c..c392e2b03 100644
--- a/manifests/cell_v2/discover_hosts.pp
+++ b/manifests/cell_v2/discover_hosts.pp
@@ -13,10 +13,12 @@ class nova::cell_v2::discover_hosts (
 ) {
 
   include ::nova::deps
+  include ::nova::params
 
   exec { 'nova-cell_v2-discover_hosts':
     path        => ['/bin', '/usr/bin'],
     command     => "nova-manage ${extra_params} cell_v2 discover_hosts",
+    user        => $::nova::params::nova_user,
     refreshonly => true,
     subscribe   => Anchor['nova::service::end']
   }
diff --git a/manifests/cell_v2/map_cell0.pp b/manifests/cell_v2/map_cell0.pp
index 1ee83ab3f..40ace350e 100644
--- a/manifests/cell_v2/map_cell0.pp
+++ b/manifests/cell_v2/map_cell0.pp
@@ -16,10 +16,12 @@ class nova::cell_v2::map_cell0 (
 ) {
 
   include ::nova::deps
+  include ::nova::params
 
   exec { 'nova-cell_v2-map_cell0':
     path        => ['/bin', '/usr/bin'],
     command     => "nova-manage ${extra_params} cell_v2 map_cell0",
+    user        => $::nova::params::nova_user,
     refreshonly => true,
     logoutput   => on_failure,
     subscribe   => Anchor['nova::cell_v2::begin'],
diff --git a/manifests/cell_v2/map_cell_and_hosts.pp b/manifests/cell_v2/map_cell_and_hosts.pp
index a8a632f43..5d4cfabf1 100644
--- a/manifests/cell_v2/map_cell_and_hosts.pp
+++ b/manifests/cell_v2/map_cell_and_hosts.pp
@@ -13,10 +13,12 @@ class nova::cell_v2::map_cell_and_hosts (
 ) {
 
   include ::nova::deps
+  include ::nova::params
 
   exec { 'nova-cell_v2-map_cell_and_hosts':
     path        => ['/bin', '/usr/bin'],
     command     => "nova-manage ${extra_params} cell_v2 map_cell_and_hosts",
+    user        => $::nova::params::nova_user,
     refreshonly => true,
   }
 }
diff --git a/manifests/cell_v2/map_instances.pp b/manifests/cell_v2/map_instances.pp
index 435753e83..7d900447d 100644
--- a/manifests/cell_v2/map_instances.pp
+++ b/manifests/cell_v2/map_instances.pp
@@ -26,6 +26,7 @@ class nova::cell_v2::map_instances (
 ) {
 
   include ::nova::deps
+  include ::nova::params
 
   if (!$cell_uuid and !$cell_name) {
     fail('Either cell_uuid or cell_name must be provided')
@@ -42,6 +43,7 @@ class nova::cell_v2::map_instances (
   exec { 'nova-cell_v2-map_instances':
     path        => ['/bin', '/usr/bin'],
     command     => "nova-manage ${extra_params} cell_v2 map_instances --cell_uuid=${cell_uuid_real}",
+    user        => $::nova::params::nova_user,
     refreshonly => true,
   }
 }
diff --git a/manifests/cron/archive_deleted_rows.pp b/manifests/cron/archive_deleted_rows.pp
index 8cd3e45d0..3f2ce2fee 100644
--- a/manifests/cron/archive_deleted_rows.pp
+++ b/manifests/cron/archive_deleted_rows.pp
@@ -43,7 +43,8 @@
 #
 #  [*user*]
 #    (optional) User with access to nova files.
-#    Defaults to 'nova'.
+#    nova::params::nova_user will be used if this is undef.
+#    Defaults to undef.
 #
 #  [*destination*]
 #    (optional) Path to file to which rows should be archived
@@ -60,12 +61,13 @@ class nova::cron::archive_deleted_rows (
   $month       = '*',
   $weekday     = '*',
   $max_rows    = '100',
-  $user        = 'nova',
+  $user        = undef,
   $destination = '/var/log/nova/nova-rowsflush.log',
   $until_complete = false,
 ) {
 
   include ::nova::deps
+  include ::nova::params
 
   if $until_complete {
     $until_complete_real = '--until_complete'
@@ -74,7 +76,7 @@ class nova::cron::archive_deleted_rows (
   cron { 'nova-manage db archive_deleted_rows':
     command     => "nova-manage db archive_deleted_rows --max_rows ${max_rows} ${until_complete_real} >>${destination} 2>&1",
     environment => 'PATH=/bin:/usr/bin:/usr/sbin SHELL=/bin/sh',
-    user        => $user,
+    user        => pick($user, $::nova::params::nova_user),
     minute      => $minute,
     hour        => $hour,
     monthday    => $monthday,
diff --git a/manifests/db/online_data_migrations.pp b/manifests/db/online_data_migrations.pp
index f1bfafc54..a4db8612d 100644
--- a/manifests/db/online_data_migrations.pp
+++ b/manifests/db/online_data_migrations.pp
@@ -23,6 +23,7 @@ class nova::db::online_data_migrations(
 
   exec { 'nova-db-online-data-migrations':
     command     => "/usr/bin/nova-manage ${extra_params} db online_data_migrations",
+    user        => $::nova::params::nova_user,
     refreshonly => true,
     try_sleep   => 5,
     tries       => 10,
diff --git a/manifests/db/sync.pp b/manifests/db/sync.pp
index 668520f96..94a336fb2 100644
--- a/manifests/db/sync.pp
+++ b/manifests/db/sync.pp
@@ -23,6 +23,7 @@ class nova::db::sync(
 
   exec { 'nova-db-sync':
     command     => "/usr/bin/nova-manage ${extra_params} db sync",
+    user        => $::nova::params::nova_user,
     refreshonly => true,
     try_sleep   => 5,
     tries       => 10,
diff --git a/manifests/db/sync_api.pp b/manifests/db/sync_api.pp
index 5f09582be..b26bfa3ed 100644
--- a/manifests/db/sync_api.pp
+++ b/manifests/db/sync_api.pp
@@ -32,6 +32,7 @@ class nova::db::sync_api(
 
   exec { 'nova-db-sync-api':
     command     => "/usr/bin/nova-manage ${extra_params} api_db sync",
+    user        => $::nova::params::nova_user,
     refreshonly => true,
     try_sleep   => 5,
     tries       => 10,
diff --git a/manifests/logging.pp b/manifests/logging.pp
index 7d53d7a48..63c88e636 100644
--- a/manifests/logging.pp
+++ b/manifests/logging.pp
@@ -110,6 +110,7 @@ class nova::logging(
 ) {
 
   include ::nova::deps
+  include ::nova::params
 
   # NOTE(spredzy): In order to keep backward compatibility we rely on the pick function
   # to use nova::<myparam> first then nova::logging::<myparam>.
@@ -123,6 +124,46 @@ class nova::logging(
   }
   $debug_real = pick($::nova::debug,$debug)
 
+  if $log_dir_real != $::os_service_default {
+    # TODO: can probably remove this in Rocky once we've had it for 1 upgrade cycle
+    # Ensure ownership/permissions for any existing logfiles are correct before configuring nova
+    # This matches the rpm/deb logic:
+    #    Ubuntu: /var/log/nova is nova:adm
+    #    CentOS: /var/log/nova is nova:root
+    #    Both: /var/log/nova/*.log is nova:nova
+    $log_dir_owner = $::nova::params::nova_user
+    $log_dir_group = $::nova::params::nova_log_group
+    $log_file_owner = $::nova::params::nova_user
+    $log_file_group = $::nova::params::nova_group
+
+    file { $log_dir_real:
+      ensure  => directory,
+      owner   => $log_dir_owner,
+      group   => $log_dir_group,
+      require => Anchor['nova::install::end'],
+      before  => Anchor['nova::config::begin']
+    }
+
+    # Can't tell File[$log_dir_real] to use a different user/group when recursing so resort to chown
+    exec { 'chown nova logfiles':
+      command => "chown ${log_file_owner}:${log_file_group} ${log_dir_real}/*.log",
+      onlyif  => "test \"\$(stat -c '%U:%G' ${log_dir_real}/*.log | grep -v '${log_file_owner}:${log_file_group}')\" != ''",
+      path    => ['/usr/bin', '/bin'],
+      require => File[$log_dir_real],
+      before  => Anchor['nova::config::begin']
+    }
+
+    # END TODO, the following resource is likely to be necessary in Rocky and later
+
+    # This should force an update the selinux role if the logfile exists.
+    # It will be incorrect if the file was created by the dbsync exec resources.
+    file { "${log_dir_real}/nova-manage.log":
+      owner   => $log_file_owner,
+      group   => $log_file_group,
+      require => Anchor['nova::service::end']
+    }
+  }
+
   oslo::log { 'nova_config':
     debug                         => $debug_real,
     use_stderr                    => $use_stderr_real,
diff --git a/manifests/params.pp b/manifests/params.pp
index c55b6f2d6..a7cf621cd 100644
--- a/manifests/params.pp
+++ b/manifests/params.pp
@@ -45,7 +45,7 @@ class nova::params {
       # redhat specific config defaults
       $root_helper                   = 'sudo nova-rootwrap'
       $lock_path                     = '/var/lib/nova/tmp'
-      $nova_log_group                = 'nova'
+      $nova_log_group                = 'root'
       $nova_wsgi_script_path         = '/var/www/cgi-bin/nova'
       $nova_api_wsgi_script_source   = '/usr/bin/nova-api-wsgi'
       $placement_wsgi_script_source  = '/usr/bin/nova-placement-api'
@@ -70,6 +70,8 @@ class nova::params {
           $messagebus_service_name  = undef
         }
       }
+      $nova_user                    = 'nova'
+      $nova_group                   = 'nova'
     }
     'Debian': {
       # package names
@@ -132,6 +134,8 @@ class nova::params {
         }
       }
       $libvirt_service_name            = 'libvirtd'
+      $nova_user                       = 'nova'
+      $nova_group                      = 'nova'
     }
     default: {
       fail("Unsupported osfamily: ${::osfamily} operatingsystem: ${::operatingsystem}, \
diff --git a/releasenotes/notes/nova-manage-user-16e7145d0c10bf57.yaml b/releasenotes/notes/nova-manage-user-16e7145d0c10bf57.yaml
new file mode 100644
index 000000000..8888cf3d7
--- /dev/null
+++ b/releasenotes/notes/nova-manage-user-16e7145d0c10bf57.yaml
@@ -0,0 +1,17 @@
+---
+fixes:
+  - |
+    Correct permissions on the nova logfiles.
+    If the ``nova-manage`` commands (such as dbsync) were initially run as
+    root then subsequent runs as the nova user would fail as the logfile is
+    owned by root (see `bug 1671681`_).
+    The ownership of all nova logfiles is now checked before configuring
+    nova, as a similar issue could prevent a service starting, and the
+    nova-manage command is now run as the correct user.
+
+    Adds nova::params::nova_user and nova::params::nova_group.
+    nova::cron::archive_deleted_rows::user now defaults to
+    nova::params::nova_user instead of hardcoding 'nova'
+
+    .. _bug 1671681: https://bugs.launchpad.net/puppet-nova/+bug/1671681
+
diff --git a/spec/classes/nova_cell_v2_discover_hosts_spec.rb b/spec/classes/nova_cell_v2_discover_hosts_spec.rb
index 62d092c30..8e7421518 100644
--- a/spec/classes/nova_cell_v2_discover_hosts_spec.rb
+++ b/spec/classes/nova_cell_v2_discover_hosts_spec.rb
@@ -9,6 +9,7 @@ describe 'nova::cell_v2::discover_hosts' do
         is_expected.to contain_exec('nova-cell_v2-discover_hosts').with(
           :path        => ['/bin', '/usr/bin'],
           :command     => 'nova-manage  cell_v2 discover_hosts',
+          :user        => 'nova',
           :refreshonly => true,
           :subscribe   => 'Anchor[nova::service::end]'
         )
@@ -26,6 +27,7 @@ describe 'nova::cell_v2::discover_hosts' do
         is_expected.to contain_exec('nova-cell_v2-discover_hosts').with(
           :path        => ['/bin', '/usr/bin'],
           :command     => 'nova-manage --config-file /etc/nova/nova.conf cell_v2 discover_hosts',
+          :user        => 'nova',
           :refreshonly => true,
           :subscribe   => 'Anchor[nova::service::end]'
         )
diff --git a/spec/classes/nova_cell_v2_map_cell0_spec.rb b/spec/classes/nova_cell_v2_map_cell0_spec.rb
index 3dfeb0ad3..45272bf86 100644
--- a/spec/classes/nova_cell_v2_map_cell0_spec.rb
+++ b/spec/classes/nova_cell_v2_map_cell0_spec.rb
@@ -9,6 +9,7 @@ describe 'nova::cell_v2::map_cell0' do
         is_expected.to contain_exec('nova-cell_v2-map_cell0').with(
           :path        => ['/bin', '/usr/bin'],
           :command     => 'nova-manage  cell_v2 map_cell0',
+          :user        => 'nova',
           :refreshonly => 'true',
           :logoutput   => 'on_failure',
           :subscribe   => 'Anchor[nova::cell_v2::begin]',
@@ -28,6 +29,7 @@ describe 'nova::cell_v2::map_cell0' do
         is_expected.to contain_exec('nova-cell_v2-map_cell0').with(
           :path        => ['/bin', '/usr/bin'],
           :command     => 'nova-manage --config-file /etc/nova/nova.conf cell_v2 map_cell0',
+          :user        => 'nova',
           :refreshonly => 'true',
           :logoutput   => 'on_failure',
           :subscribe   => 'Anchor[nova::cell_v2::begin]',
diff --git a/spec/classes/nova_cell_v2_map_cell_and_hosts_spec.rb b/spec/classes/nova_cell_v2_map_cell_and_hosts_spec.rb
index 47aa8aaa2..45ad56563 100644
--- a/spec/classes/nova_cell_v2_map_cell_and_hosts_spec.rb
+++ b/spec/classes/nova_cell_v2_map_cell_and_hosts_spec.rb
@@ -9,6 +9,7 @@ describe 'nova::cell_v2::map_cell_and_hosts' do
         is_expected.to contain_exec('nova-cell_v2-map_cell_and_hosts').with(
           :path        => ['/bin', '/usr/bin'],
           :command     => 'nova-manage  cell_v2 map_cell_and_hosts',
+          :user        => 'nova',
           :refreshonly => true,
         )
       }
@@ -25,6 +26,7 @@ describe 'nova::cell_v2::map_cell_and_hosts' do
         is_expected.to contain_exec('nova-cell_v2-map_cell_and_hosts').with(
           :path        => ['/bin', '/usr/bin'],
           :command     => 'nova-manage --config-file /etc/nova/nova.conf cell_v2 map_cell_and_hosts',
+          :user        => 'nova',
           :refreshonly => true,
         )
       }
diff --git a/spec/classes/nova_cell_v2_map_instances.rb b/spec/classes/nova_cell_v2_map_instances.rb
index 4c0c148ab..50b21dc06 100644
--- a/spec/classes/nova_cell_v2_map_instances.rb
+++ b/spec/classes/nova_cell_v2_map_instances.rb
@@ -10,6 +10,7 @@ describe 'nova::cell_v2::map_instances' do
         is_expected.to contain_exec('nova-cell_v2-map_instances').with(
           :path        => ['/bin', '/usr/bin'],
           :command     => 'nova-manage  cell_v2 map_instances --cell_uuid=uuid',
+          :user        => 'nova',
           :refreshonly => true,
         )
       }
@@ -27,6 +28,7 @@ describe 'nova::cell_v2::map_instances' do
         is_expected.to contain_exec('nova-cell_v2-map_instances').with(
           :path        => ['/bin', '/usr/bin'],
           :command     => 'nova-manage --config-file /etc/nova/nova.conf cell_v2 map_instances --cell_uuid=uuid',
+          :user        => 'nova',
           :refreshonly => true,
         )
       }
diff --git a/spec/classes/nova_cron_archive_deleted_rows_spec.rb b/spec/classes/nova_cron_archive_deleted_rows_spec.rb
index 0bc1217ea..576bf1ad3 100644
--- a/spec/classes/nova_cron_archive_deleted_rows_spec.rb
+++ b/spec/classes/nova_cron_archive_deleted_rows_spec.rb
@@ -22,6 +22,7 @@ describe 'nova::cron::archive_deleted_rows' do
     it 'configures a cron without until_complete' do
       is_expected.to contain_cron('nova-manage db archive_deleted_rows').with(
         :command     => "nova-manage db archive_deleted_rows --max_rows #{params[:max_rows]}  >>#{params[:destination]} 2>&1",
+        :user        => 'nova',
         :environment => 'PATH=/bin:/usr/bin:/usr/sbin SHELL=/bin/sh',
         :user        => params[:user],
         :minute      => params[:minute],
@@ -44,6 +45,7 @@ describe 'nova::cron::archive_deleted_rows' do
     it 'configures a cron with until_complete' do
       is_expected.to contain_cron('nova-manage db archive_deleted_rows').with(
         :command     => "nova-manage db archive_deleted_rows --max_rows #{params[:max_rows]} --until_complete >>#{params[:destination]} 2>&1",
+        :user        => 'nova',
         :environment => 'PATH=/bin:/usr/bin:/usr/sbin SHELL=/bin/sh',
         :user        => params[:user],
         :minute      => params[:minute],
diff --git a/spec/classes/nova_db_online_data_migrations_spec.rb b/spec/classes/nova_db_online_data_migrations_spec.rb
index 89c7f5fb0..b6fcbeaf1 100644
--- a/spec/classes/nova_db_online_data_migrations_spec.rb
+++ b/spec/classes/nova_db_online_data_migrations_spec.rb
@@ -7,6 +7,7 @@ describe 'nova::db::online_data_migrations' do
     it 'runs nova-db-sync' do
       is_expected.to contain_exec('nova-db-online-data-migrations').with(
         :command     => '/usr/bin/nova-manage  db online_data_migrations',
+        :user        => 'nova',
         :refreshonly => 'true',
         :try_sleep   => 5,
         :tries       => 10,
@@ -30,6 +31,7 @@ describe 'nova::db::online_data_migrations' do
       it {
         is_expected.to contain_exec('nova-db-online-data-migrations').with(
           :command     => '/usr/bin/nova-manage --config-file /etc/nova/nova.conf db online_data_migrations',
+          :user        => 'nova',
           :refreshonly => 'true',
           :try_sleep   => 5,
           :tries       => 10,
@@ -54,6 +56,7 @@ describe 'nova::db::online_data_migrations' do
       it {
         is_expected.to contain_exec('nova-db-online-data-migrations').with(
           :command     => '/usr/bin/nova-manage  db online_data_migrations',
+          :user        => 'nova',
           :refreshonly => 'true',
           :try_sleep   => 5,
           :tries       => 10,
diff --git a/spec/classes/nova_db_sync_api_spec.rb b/spec/classes/nova_db_sync_api_spec.rb
index 3dfd80137..fc2a412b1 100644
--- a/spec/classes/nova_db_sync_api_spec.rb
+++ b/spec/classes/nova_db_sync_api_spec.rb
@@ -7,6 +7,7 @@ describe 'nova::db::sync_api' do
       it {
         is_expected.to contain_exec('nova-db-sync-api').with(
           :command     => '/usr/bin/nova-manage  api_db sync',
+          :user        => 'nova',
           :refreshonly => 'true',
           :timeout     => 300,
           :logoutput   => 'on_failure',
@@ -31,6 +32,7 @@ describe 'nova::db::sync_api' do
       it {
         is_expected.to contain_exec('nova-db-sync-api').with(
           :command     => '/usr/bin/nova-manage --config-file /etc/nova/nova.conf api_db sync',
+          :user        => 'nova',
           :refreshonly => 'true',
           :timeout     => 300,
           :logoutput   => 'on_failure',
@@ -54,6 +56,7 @@ describe 'nova::db::sync_api' do
       it {
         is_expected.to contain_exec('nova-db-sync-api').with(
           :command     => '/usr/bin/nova-manage  api_db sync',
+          :user        => 'nova',
           :refreshonly => 'true',
           :timeout     => 750,
           :logoutput   => 'on_failure',
diff --git a/spec/classes/nova_db_sync_spec.rb b/spec/classes/nova_db_sync_spec.rb
index 72b3cb26f..e6fb133cc 100644
--- a/spec/classes/nova_db_sync_spec.rb
+++ b/spec/classes/nova_db_sync_spec.rb
@@ -7,6 +7,7 @@ describe 'nova::db::sync' do
     it 'runs nova-db-sync' do
       is_expected.to contain_exec('nova-db-sync').with(
         :command     => '/usr/bin/nova-manage  db sync',
+        :user        => 'nova',
         :refreshonly => 'true',
         :try_sleep   => 5,
         :tries       => 10,
@@ -30,6 +31,7 @@ describe 'nova::db::sync' do
       it {
         is_expected.to contain_exec('nova-db-sync').with(
           :command     => '/usr/bin/nova-manage --config-file /etc/nova/nova.conf db sync',
+          :user        => 'nova',
           :refreshonly => 'true',
           :try_sleep   => 5,
           :tries       => 10,
@@ -54,6 +56,7 @@ describe 'nova::db::sync' do
       it {
         is_expected.to contain_exec('nova-db-sync').with(
           :command     => '/usr/bin/nova-manage  db sync',
+          :user        => 'nova',
           :refreshonly => 'true',
           :try_sleep   => 5,
           :tries       => 10,
diff --git a/spec/classes/nova_logging_spec.rb b/spec/classes/nova_logging_spec.rb
index 62f51d9c3..dba768bf7 100644
--- a/spec/classes/nova_logging_spec.rb
+++ b/spec/classes/nova_logging_spec.rb
@@ -27,7 +27,7 @@ describe 'nova::logging' do
      :use_json => true,
      :use_stderr => false,
      :log_facility => 'LOG_FOO',
-     :log_dir => '/var/log',
+     :log_dir => '/var/log/foo',
      :debug => true,
     }
   end
@@ -63,6 +63,13 @@ describe 'nova::logging' do
         :log_dir    => '/var/log/nova',
         :debug      => '<SERVICE DEFAULT>',
       )
+      is_expected.to contain_file('/var/log/nova').with(
+        :owner        => 'nova',
+      )
+      is_expected.to contain_file('/var/log/nova/nova-manage.log').with(
+        :owner        => 'nova',
+      )
+      is_expected.to contain_exec('chown nova logfiles')
     end
   end
 
@@ -73,9 +80,16 @@ describe 'nova::logging' do
         :use_json            => true,
         :use_stderr          => false,
         :syslog_log_facility => 'LOG_FOO',
-        :log_dir             => '/var/log',
+        :log_dir             => '/var/log/foo',
         :debug               => true,
       )
+      is_expected.to contain_file('/var/log/foo').with(
+        :owner        => 'nova',
+      )
+      is_expected.to contain_file('/var/log/foo/nova-manage.log').with(
+        :owner        => 'nova',
+      )
+      is_expected.to contain_exec('chown nova logfiles')
     end
   end
 
@@ -98,6 +112,13 @@ describe 'nova::logging' do
        :instance_uuid_format           => '[instance: %(uuid)s] ',
        :log_date_format                => '%Y-%m-%d %H:%M:%S',
       )
+      is_expected.to contain_file('/var/log/foo').with(
+        :owner        => 'nova',
+      )
+      is_expected.to contain_file('/var/log/foo/nova-manage.log').with(
+        :owner        => 'nova',
+      )
+      is_expected.to contain_exec('chown nova logfiles')
     end
   end