diff --git a/lib/puppet/provider/nova.rb b/lib/puppet/provider/nova.rb index 09b875170..e52a7b87c 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 53fbf18f9..207c45f44 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 4bdaef566..44bbc5fda 100644 --- a/manifests/logging.pp +++ b/manifests/logging.pp @@ -105,6 +105,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:: first then nova::logging::. @@ -114,6 +115,46 @@ class nova::logging( $log_dir_real = pick($::nova::log_dir,$log_dir) $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 93a156e95..c1e76b3d3 100644 --- a/manifests/params.pp +++ b/manifests/params.pp @@ -49,7 +49,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/lib/python2.7/site-packages/nova/wsgi/nova-api.py' $placement_wsgi_script_source = '/usr/bin/nova-placement-api' @@ -74,6 +74,8 @@ class nova::params { $messagebus_service_name = undef } } + $nova_user = 'nova' + $nova_group = 'nova' } 'Debian': { # package names @@ -136,6 +138,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 9ac7c6256..ad09d66da 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 e489e646b..6b778b9f8 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 f070b8233..0312b8f0d 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 66ced5b56..50a6ac03d 100644 --- a/spec/classes/nova_logging_spec.rb +++ b/spec/classes/nova_logging_spec.rb @@ -26,7 +26,7 @@ describe 'nova::logging' do :use_syslog => true, :use_stderr => false, :log_facility => 'LOG_FOO', - :log_dir => '/var/log', + :log_dir => '/var/log/foo', :debug => true, } end @@ -59,6 +59,13 @@ describe 'nova::logging' do is_expected.to contain_nova_config('DEFAULT/use_stderr').with(:value => '') is_expected.to contain_nova_config('DEFAULT/log_dir').with(:value => '/var/log/nova') is_expected.to contain_nova_config('DEFAULT/debug').with(:value => '') + 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 @@ -67,8 +74,12 @@ describe 'nova::logging' do is_expected.to contain_nova_config('DEFAULT/use_syslog').with(:value => 'true') is_expected.to contain_nova_config('DEFAULT/use_stderr').with(:value => 'false') is_expected.to contain_nova_config('DEFAULT/syslog_log_facility').with(:value => 'LOG_FOO') - is_expected.to contain_nova_config('DEFAULT/log_dir').with(:value => '/var/log') + is_expected.to contain_nova_config('DEFAULT/log_dir').with(:value => '/var/log/foo') is_expected.to contain_nova_config('DEFAULT/debug').with(:value => 'true') + is_expected.to contain_file('/var/log/foo/nova-manage.log').with( + :owner => 'nova', + ) + is_expected.to contain_exec('chown nova logfiles') end end @@ -105,6 +116,16 @@ describe 'nova::logging' do is_expected.to contain_nova_config('DEFAULT/log_date_format').with_value( '%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