From 1cd349f893408803fec307f615ae3fe265d54fed Mon Sep 17 00:00:00 2001 From: Oliver Walsh Date: Wed, 9 Aug 2017 15:38:49 +0100 Subject: [PATCH] Fix handling of nova pci MultiStrOpt params The changes in Ie27dbbc510c73c685b239a9be4af2700a0eb42f0 did not appear to fix the handling of the nova pci_* params. The config being written to nova.conf remains in the ListOpt format. As support for MultiStrOpt was added to nova_config in I6be7bb4cea1906bd98c513bd2d01153e4643e3ac we just need to pass an array of strings to nova_config to set these MultiStrOpt params. Nova expects the string values to be JSON encoded so that is what the parser function now returns. This function has also been renamed from 'check_array_of_hash' (which it never did) to 'to_array_of_json_strings'. The acceptable input formats are a JSON array of objects or a puppet Array of Hashes. Perviously a "JSON with single quotes" format was used but should now be considered deprecated as it could corrupt data. This also removes any pci_alias entries from nova.conf when the param is not set. Change-Id: Ida3ecab717bc3113ba23553c559263f35c49c46a Closes-bug: #1696955 --- .../parser/functions/check_array_of_hash.rb | 27 ----------- .../functions/to_array_of_json_strings.rb | 38 ++++++++++++++++ manifests/api.pp | 22 +++++---- manifests/compute.pp | 6 +-- .../pci_params_format-bf62bf47585ad1cc.yaml | 11 +++++ spec/classes/nova_api_spec.rb | 44 +++++++++++++++++- spec/classes/nova_compute_spec.rb | 45 ++++++++++++++++--- 7 files changed, 145 insertions(+), 48 deletions(-) delete mode 100644 lib/puppet/parser/functions/check_array_of_hash.rb create mode 100644 lib/puppet/parser/functions/to_array_of_json_strings.rb create mode 100644 releasenotes/notes/pci_params_format-bf62bf47585ad1cc.yaml diff --git a/lib/puppet/parser/functions/check_array_of_hash.rb b/lib/puppet/parser/functions/check_array_of_hash.rb deleted file mode 100644 index be75db296..000000000 --- a/lib/puppet/parser/functions/check_array_of_hash.rb +++ /dev/null @@ -1,27 +0,0 @@ -require 'json' - -def array_of_hash?(list) - return false unless !list.empty? && list.class == Array - list.each do |e| - return false unless e.class == Hash - end - true -end - -module Puppet::Parser::Functions - newfunction(:check_array_of_hash, :arity =>1, :type => :rvalue, :doc => "Check - input String is a valid Array of Hash in JSON style") do |arg| - if arg[0].class == String - begin - list = JSON.load(arg[0].gsub("'","\"")) - rescue JSON::ParserError - raise Puppet::ParseError, "Syntax error: #{arg[0]} is invalid" - else - return arg[0] if array_of_hash?(list) - end - else - raise Puppet::ParseError, "Syntax error: #{arg[0]} is not a String" - end - return '' - end -end diff --git a/lib/puppet/parser/functions/to_array_of_json_strings.rb b/lib/puppet/parser/functions/to_array_of_json_strings.rb new file mode 100644 index 000000000..3a6cd26aa --- /dev/null +++ b/lib/puppet/parser/functions/to_array_of_json_strings.rb @@ -0,0 +1,38 @@ +require 'json' + +def array_of_hash?(list) + return false unless list.class == Array + list.each do |e| + return false unless e.class == Hash + end + true +end + +module Puppet::Parser::Functions + newfunction(:to_array_of_json_strings, :arity =>1, :type => :rvalue, :doc => "Convert + input array of hashes (optionally JSON encoded) to a puppet Array of JSON encoded Strings") do |arg| + list = arg[0] + if list.class == String + begin + begin + list = JSON.load(list) + rescue JSON::ParserError + # If parsing failed it could be a legacy format that uses single quotes. + # NB This will corrupt valid JSON data, e.g {"foo": "\'"} => {"foo": "\""} + list = JSON.load(list.gsub("'","\"")) + Puppet.warning("#{arg[0]} is not valid JSON. Support for this format is deprecated and may be removed in future.") + end + rescue JSON::ParserError + raise Puppet::ParseError, "Syntax error: #{arg[0]} is not valid" + end + end + unless array_of_hash?(list) + raise Puppet::ParseError, "Syntax error: #{arg[0]} is not an Array or JSON encoded String" + end + rv = [] + list.each do |e| + rv.push(e.to_json) + end + return rv + end +end diff --git a/manifests/api.pp b/manifests/api.pp index b1022341a..12e5bacff 100644 --- a/manifests/api.pp +++ b/manifests/api.pp @@ -75,10 +75,11 @@ # Defaults to undef # # [*pci_alias*] -# (optional) Pci passthrough for controller: -# Defaults to undef -# Example -# "[ {'vendor_id':'1234', 'product_id':'5678', 'name':'default'}, {...} ]" +# (optional) A list of pci alias hashes +# Defaults to $::os_service_default +# Example: +# [{"vendor_id" => "1234", "product_id" => "5678", "name" => "default"}, +# {"vendor_id" => "1234", "product_id" => "6789", "name" => "other"}] # # [*ratelimits*] # (optional) A string that is a semicolon-separated list of 5-tuples. @@ -291,7 +292,7 @@ class nova::api( $sync_db_api = true, $db_online_data_migrations = false, $neutron_metadata_proxy_shared_secret = undef, - $pci_alias = undef, + $pci_alias = $::os_service_default, $ratelimits = undef, $ratelimits_factory = 'nova.api.openstack.compute.limits:RateLimitingMiddleware.factory', @@ -535,10 +536,13 @@ as a standalone service, or httpd for being run by a httpd server") 'filter:authtoken/auth_admin_prefix': ensure => absent; } - if $pci_alias { - nova_config { - 'pci/alias': value => join(any2array(check_array_of_hash($pci_alias)), ','); - } + if !is_service_default($pci_alias) and !empty($pci_alias) { + $pci_alias_real = to_array_of_json_strings($pci_alias) + } else { + $pci_alias_real = $::os_service_default + } + nova_config { + 'pci/alias': value => $pci_alias_real; } if $validate { diff --git a/manifests/compute.pp b/manifests/compute.pp index 943602eda..d1c6356ca 100644 --- a/manifests/compute.pp +++ b/manifests/compute.pp @@ -87,8 +87,8 @@ # (optional) Pci passthrough list of hash. # Defaults to $::os_service_default # Example of format: -# "[ { 'vendor_id':'1234','product_id':'5678' }, -# { 'vendor_id':'4321','product_id':'8765','physical_network':'default' } ] " +# [ { "vendor_id" => "1234","product_id" => "5678" }, +# { "vendor_id" => "4321","product_id" => "8765", "physical_network" => "default" } ] # # [*config_drive_format*] # (optional) Config drive format. One of iso9660 (default) or vfat @@ -186,7 +186,7 @@ class nova::compute ( # the value is computed in a function and it makes things more complex. Let's just check if # a value is set or if it's empty. if !is_service_default($pci_passthrough) and !empty($pci_passthrough) { - $pci_passthrough_real = check_array_of_hash($pci_passthrough) + $pci_passthrough_real = to_array_of_json_strings($pci_passthrough) } else { $pci_passthrough_real = $::os_service_default } diff --git a/releasenotes/notes/pci_params_format-bf62bf47585ad1cc.yaml b/releasenotes/notes/pci_params_format-bf62bf47585ad1cc.yaml new file mode 100644 index 000000000..5ebafa2fe --- /dev/null +++ b/releasenotes/notes/pci_params_format-bf62bf47585ad1cc.yaml @@ -0,0 +1,11 @@ +--- +features: + - | + The nova::api::pci_alias and nova::compute::pci_passthrough params now + accept an array of hashes. A valid JSON encoded array of objects is also + acceptable. +deprecations: + - | + Invalid JSON was previously accepted for the nova::api::pci_alias and + nova::compute::pci_passthrough parameters. This format is now deprecated + and may not be accepted in future. diff --git a/spec/classes/nova_api_spec.rb b/spec/classes/nova_api_spec.rb index 50ed82f5a..7a84006e0 100644 --- a/spec/classes/nova_api_spec.rb +++ b/spec/classes/nova_api_spec.rb @@ -79,6 +79,7 @@ describe 'nova::api' do is_expected.to contain_nova_config('vendordata_dynamic_auth/project_name').with('value' => '') is_expected.to contain_nova_config('vendordata_dynamic_auth/user_domain_name').with('value' => '') is_expected.to contain_nova_config('vendordata_dynamic_auth/username').with('value' => '') + is_expected.to contain_nova_config('pci/alias').with(:value => '') end it 'unconfigures neutron_metadata proxy' do @@ -117,7 +118,6 @@ describe 'nova::api' do :enable_network_quota => false, :enable_instance_password => true, :password_length => 12, - :pci_alias => "[{\"vendor_id\":\"8086\",\"product_id\":\"0126\",\"name\":\"graphic_card\"},{\"vendor_id\":\"9096\",\"product_id\":\"1520\",\"name\":\"network_card\"}]", :allow_resize_to_same_host => true, :vendordata_dynamic_auth_auth_type => 'password', :vendordata_dynamic_auth_auth_url => 'http://127.0.0.1:5000', @@ -182,14 +182,54 @@ describe 'nova::api' do is_expected.to contain_nova_config('vendordata_dynamic_auth/user_domain_name').with('value' => 'Default') is_expected.to contain_nova_config('vendordata_dynamic_auth/username').with('value' => 'user') end + end + context 'with pci_alias array' do + before do + params.merge!({ + :pci_alias => [{ + "vendor_id" => "8086", + "product_id" => "0126", + "name" => "graphic_card" + }, + { + "vendor_id" => "9096", + "product_id" => "1520", + "name" => "network_card" + } + ] + }) + end it 'configures nova pci_alias entries' do is_expected.to contain_nova_config('pci/alias').with( - 'value' => "[{\"vendor_id\":\"8086\",\"product_id\":\"0126\",\"name\":\"graphic_card\"},{\"vendor_id\":\"9096\",\"product_id\":\"1520\",\"name\":\"network_card\"}]" + 'value' => ['{"vendor_id":"8086","product_id":"0126","name":"graphic_card"}','{"vendor_id":"9096","product_id":"1520","name":"network_card"}'] ) end end + context 'with pci_alias JSON encoded string (deprecated)' do + before do + params.merge!({ + :pci_alias => "[{\"vendor_id\":\"8086\",\"product_id\":\"0126\",\"name\":\"graphic_card\"},{\"vendor_id\":\"9096\",\"product_id\":\"1520\",\"name\":\"network_card\"}]", + }) + end + it 'configures nova pci_alias entries' do + is_expected.to contain_nova_config('pci/alias').with( + 'value' => ['{"vendor_id":"8086","product_id":"0126","name":"graphic_card"}','{"vendor_id":"9096","product_id":"1520","name":"network_card"}'] + ) + end + end + + context 'when pci_alias is empty' do + let :params do + { :pci_alias => "" } + end + + it 'clears pci_alias configuration' do + is_expected.to contain_nova_config('pci/alias').with(:value => '') + end + end + context 'while validating the service with default command' do before do params.merge!({ diff --git a/spec/classes/nova_compute_spec.rb b/spec/classes/nova_compute_spec.rb index cc0ae7e98..3af6a9f94 100644 --- a/spec/classes/nova_compute_spec.rb +++ b/spec/classes/nova_compute_spec.rb @@ -34,6 +34,7 @@ describe 'nova::compute' do it { is_expected.to contain_nova_config('barbican/barbican_api_version').with_value('') } it { is_expected.to contain_nova_config('barbican/auth_endpoint').with_value('') } it { is_expected.to contain_nova_config('DEFAULT/max_concurrent_live_migrations').with_value('') } + it { is_expected.to contain_nova_config('pci/passthrough_whitelist').with(:value => '') } it { is_expected.to_not contain_package('cryptsetup').with( :ensure => 'present' )} @@ -69,7 +70,6 @@ describe 'nova::compute' do :force_raw_images => false, :reserved_host_memory => '0', :heal_instance_info_cache_interval => '120', - :pci_passthrough => "[{\"vendor_id\":\"8086\",\"product_id\":\"0126\"},{\"vendor_id\":\"9096\",\"product_id\":\"1520\",\"physical_network\":\"physnet1\"}]", :config_drive_format => 'vfat', :resize_confirm_window => '3', :vcpu_pin_set => ['4-12','^8','15'], @@ -130,12 +130,6 @@ describe 'nova::compute' do it { is_expected.to contain_nova_config('DEFAULT/resume_guests_state_on_host_boot').with_value(true) } - it 'configures nova pci_passthrough_whitelist entries' do - is_expected.to contain_nova_config('pci/passthrough_whitelist').with( - 'value' => "[{\"vendor_id\":\"8086\",\"product_id\":\"0126\"},{\"vendor_id\":\"9096\",\"product_id\":\"1520\",\"physical_network\":\"physnet1\"}]" - ) - end - it 'configures nova config_drive_format to vfat' do is_expected.to contain_nova_config('DEFAULT/config_drive_format').with_value('vfat') is_expected.to_not contain_package('genisoimage').with( @@ -144,6 +138,43 @@ describe 'nova::compute' do end end + context 'with pci_passthrough array' do + let :params do + { + :pci_passthrough => [ + { + "vendor_id" => "8086", + "product_id" => "0126" + }, + { + "vendor_id" => "9096", + "product_id" => "1520", + "physical_network" => "physnet1" + } + ] + } + end + + it 'configures nova pci_passthrough_whitelist entries' do + is_expected.to contain_nova_config('pci/passthrough_whitelist').with( + 'value' => ['{"vendor_id":"8086","product_id":"0126"}','{"vendor_id":"9096","product_id":"1520","physical_network":"physnet1"}'] + ) + end + end + + context 'with pci_passthrough JSON encoded string (deprecated)' do + let :params do + { + :pci_passthrough => "[{\"vendor_id\":\"8086\",\"product_id\":\"0126\"},{\"vendor_id\":\"9096\",\"product_id\":\"1520\",\"physical_network\":\"physnet1\"}]" + } + end + + it 'configures nova pci_passthrough_whitelist entries' do + is_expected.to contain_nova_config('pci/passthrough_whitelist').with( + 'value' => ['{"vendor_id":"8086","product_id":"0126"}','{"vendor_id":"9096","product_id":"1520","physical_network":"physnet1"}'] + ) + end + end context 'when vcpu_pin_set and pci_passthrough are empty' do let :params do