From 273ef1b228fbcb73edf021fabbf77ff5054e7dd7 Mon Sep 17 00:00:00 2001 From: Sofer Athlan-Guyot Date: Thu, 31 Dec 2015 16:36:05 +0100 Subject: [PATCH] Neutron parses the wrong json revert to shell/csv. The neutron network provider is parsing the wrong json format leading to an error when you're unlucky. Affected providers: - neutron_port - neutron_subnet - neutron_router - neutron_network The neutron provider parses the json of the cliff-tablib's output while the official one is the cliff' output[1]. As discussed in the ML[2] this patch implements a simple revert of the json change. Revert "Use json output instead plain-text" This reverts commit 79f0e636c07b80b2c6699d934b42641bfef4352b[3] and part of commit 791a0f146b28544ec3d70c0ab2a950c4d5ca9f98[4] [1]: https://bugs.launchpad.net/python-neutronclient/+bug/1529914 [2]: http://lists.openstack.org/pipermail/openstack-dev/2015-December/083172.html [3]: https://review.openstack.org/#/c/238156/ [4]: https://review.openstack.org/#/c/261418/ Change-Id: I93263256eb9d83544910ca4055bb3e9a463645e3 Closes-bug: 1530163 --- lib/puppet/provider/neutron.rb | 90 ++++------ .../provider/neutron_router/neutron_spec.rb | 18 +- spec/unit/provider/neutron_spec.rb | 155 +++--------------- 3 files changed, 57 insertions(+), 206 deletions(-) diff --git a/lib/puppet/provider/neutron.rb b/lib/puppet/provider/neutron.rb index 4dcfeee0f..2fc77e73b 100644 --- a/lib/puppet/provider/neutron.rb +++ b/lib/puppet/provider/neutron.rb @@ -1,5 +1,4 @@ require 'csv' -require 'json' require 'puppet/util/inifile' class Puppet::Provider::Neutron < Puppet::Provider @@ -132,93 +131,64 @@ correctly configured.") @neutron_credentials = nil end - def self.find_and_parse_json(text) - # separate json from any possible garbage around it and parse - rv = [] - if text.is_a? String - text = text.split("\n") - elsif !text.is_a? Array - return rv - end - found = false - (0..text.size-1).reverse_each do |line_no| - if text[line_no] =~ /\]\s*$/ - end_of_json_line_no = line_no - (0..end_of_json_line_no).reverse_each do |start_of_json_line_no| - if text[start_of_json_line_no] =~ /^\s*\[/ - begin - js_txt = text[start_of_json_line_no..end_of_json_line_no].join('') - rv = JSON.parse(js_txt) - found = true - rescue - # do nothing, next iteration please, because found==false - end - end - break if found - end - end - break if found - end - rv - end - def self.list_neutron_resources(type) ids = [] - list = auth_neutron("#{type}-list", '--format=json', - '--column=id') + list = auth_neutron("#{type}-list", '--format=csv', + '--column=id', '--quote=none') if list.nil? raise(Puppet::ExecutionFailure, "Can't retrieve #{type}-list because Neutron or Keystone API is not available.") end - self.find_and_parse_json(list).each do |line| - ids << line['id'] + (list.split("\n")[1..-1] || []).compact.collect do |line| + ids << line.strip end - return ids end def self.get_neutron_resource_attrs(type, id) attrs = {} - net = auth_neutron("#{type}-show", '--format=json', id) + net = auth_neutron("#{type}-show", '--format=shell', id) if net.nil? raise(Puppet::ExecutionFailure, "Can't retrieve #{type}-show because Neutron or Keystone API is not available.") end - self.find_and_parse_json(net).each do |line| - k = line['Field'] - v = line['Value'] - if ['True', 'False'].include? v.to_s.capitalize - v = "#{v}".capitalize - elsif v.is_a? String and v =~ /\n/ - v = v.split(/\n/) - elsif v.is_a? Numeric - v = "#{v}" + last_key = nil + (net.split("\n") || []).compact.collect do |line| + if line.include? '=' + k, v = line.split('=', 2) + attrs[k] = v.gsub(/\A"|"\Z/, '') + last_key = k else - v = "#{v}" + # Handle the case of a list of values + v = line.gsub(/\A"|"\Z/, '') + attrs[last_key] = [attrs[last_key], v].flatten end - attrs[k] = v end - return attrs end def self.list_router_ports(router_name_or_id) results = [] cmd_output = auth_neutron("router-port-list", - '--format=json', + '--format=csv', router_name_or_id) - - self.find_and_parse_json(cmd_output).each do |port| - if port['fixed_ips'] - if !port['fixed_ips'].empty? - fixed_ips = JSON.parse(port['fixed_ips']) - port['subnet_id'] = fixed_ips['subnet_id'] - end - port.delete('fixed_ips') - end - results << port + if ! cmd_output + return results end + headers = nil + CSV.parse(cmd_output) do |row| + if headers == nil + headers = row + else + result = Hash[*headers.zip(row).flatten] + match_data = /.*"subnet_id": "(.*)", .*/.match(result['fixed_ips']) + if match_data + result['subnet_id'] = match_data[1] + end + results << result + end + end return results end diff --git a/spec/unit/provider/neutron_router/neutron_spec.rb b/spec/unit/provider/neutron_router/neutron_spec.rb index 93ca71215..4c7e9eb82 100644 --- a/spec/unit/provider/neutron_router/neutron_spec.rb +++ b/spec/unit/provider/neutron_router/neutron_spec.rb @@ -93,25 +93,15 @@ tenant_id="60f9544eb94c42a6b7e8e98c2be981b1"' end it 'should detect a gateway net id' do - output = <<-EOT - bla-bla-bla - - [{"Field": "external_gateway_info", - "Value": "{\\"network_id\\": \\"1b-b1\\", \\"enable_snat\\": true, \\"external_fixed_ips\\": [{\\"subnet_id\\": \\"1b-b1\\", \\"ip_address\\": \\"1.1.1.1\\"}]}" - }] - EOT - klass.stubs(:auth_neutron).returns(output) + klass.stubs(:auth_neutron).returns( + 'external_gateway_info="{\"network_id\": \"1b-b1\", \"enable_snat\": true, \"external_fixed_ips\": [{\"subnet_id\": \"1b-b1\", \"ip_address\": \"1.1.1.1\"}]}"' + ) result = klass.get_neutron_resource_attrs 'foo', nil expect(provider.parse_gateway_network_id(result['external_gateway_info'])).to eql('1b-b1') end it 'should return empty value, if there is no net id found' do - output = <<-EOT - bla-bla-bla - - [{"Field": "external_gateway_info", "Value": "{}"}] - EOT - klass.stubs(:auth_neutron).returns(output) + klass.stubs(:auth_neutron).returns('external_gateway_info="{}"') result = klass.get_neutron_resource_attrs 'foo', nil expect(provider.parse_gateway_network_id(result['external_gateway_info'])).to eql('') end diff --git a/spec/unit/provider/neutron_spec.rb b/spec/unit/provider/neutron_spec.rb index 139b2f306..d27abddeb 100644 --- a/spec/unit/provider/neutron_spec.rb +++ b/spec/unit/provider/neutron_spec.rb @@ -131,9 +131,9 @@ describe Puppet::Provider::Neutron do it 'should exclude the column header' do output = <<-EOT - bla-bla-bla - - [{"id": "net1"},{"id": "net2"}] + id + net1 + net2 EOT klass.expects(:auth_neutron).returns(output) result = klass.list_neutron_resources('foo') @@ -141,19 +141,6 @@ describe Puppet::Provider::Neutron do end it 'should return empty list when there are no neutron resources' do - output = <<-EOT - bla-bla-bla - - [] - - bla-bla - EOT - klass.stubs(:auth_neutron).returns(output) - result = klass.list_neutron_resources('foo') - expect(result).to eql([]) - end - - it 'should return empty respons when there are no neutron resources' do output = <<-EOT EOT klass.stubs(:auth_neutron).returns(output) @@ -173,21 +160,16 @@ describe Puppet::Provider::Neutron do describe 'when retrieving attributes for neutron resources' do it 'should parse single-valued attributes into a key-value pair' do - output = <<-EOT - bla-bla-bla - - [{"Field": "admin_state_up", "Value": true}] - EOT - klass.expects(:auth_neutron).returns(output) + klass.expects(:auth_neutron).returns('admin_state_up="True"') result = klass.get_neutron_resource_attrs('foo', 'id') - expect(result).to eql({"admin_state_up" => "True"}) + expect(result).to eql({"admin_state_up" => 'True'}) end it 'should parse multi-valued attributes into a key-list pair' do output = <<-EOT - bla-bla-bla - - [{"Field": "subnets", "Value": "subnet1\\nsubnet2\\nsubnet3"}] +subnets="subnet1 +subnet2 +subnet3" EOT klass.expects(:auth_neutron).returns(output) result = klass.get_neutron_resource_attrs('foo', 'id') @@ -204,63 +186,35 @@ describe Puppet::Provider::Neutron do it 'should handle an empty port list' do klass.expects(:auth_neutron).with('router-port-list', - '--format=json', + '--format=csv', router) result = klass.list_router_ports(router) expect(result).to eql([]) end it 'should handle several ports' do - output = ''' - [ - { - "id": "1345e576-a21f-4c2e-b24a-b245639852ab", - "name": "", - "mac_address": "fa:16:3e:e3:e6:38", - "fixed_ips": "{\"subnet_id\": \"839a1d2d-2c6e-44fb-9a2b-9b011dce8c2f\", \"ip_address\": \"10.0.0.1\"}" - }, - { - "id": "de0dc526-02b2-467c-9832-2c3dc69ac2b4", - "name": "", - "mac_address": "fa:16:3e:f6:b5:72", - "fixed_ips": "{\"subnet_id\": \"e4db0abd-276a-4f69-92ea-8b9e4eacfd43\", \"ip_address\": \"172.24.4.226\"}" - } - ] - ''' + output = <<-EOT +"id","name","mac_address","fixed_ips" +"1345e576-a21f-4c2e-b24a-b245639852ab","","fa:16:3e:e3:e6:38","{""subnet_id"": ""839a1d2d-2c6e-44fb-9a2b-9b011dce8c2f"", ""ip_address"": ""10.0.0.1""}" +"de0dc526-02b2-467c-9832-2c3dc69ac2b4","","fa:16:3e:f6:b5:72","{""subnet_id"": ""e4db0abd-276a-4f69-92ea-8b9e4eacfd43"", ""ip_address"": ""172.24.4.226""}" + EOT expected = - [{ "name"=>"", + [{ "fixed_ips"=> + "{\"subnet_id\": \"839a1d2d-2c6e-44fb-9a2b-9b011dce8c2f\", \ +\"ip_address\": \"10.0.0.1\"}", + "name"=>"", "subnet_id"=>"839a1d2d-2c6e-44fb-9a2b-9b011dce8c2f", "id"=>"1345e576-a21f-4c2e-b24a-b245639852ab", "mac_address"=>"fa:16:3e:e3:e6:38"}, - { "name"=>"", + {"fixed_ips"=> + "{\"subnet_id\": \"e4db0abd-276a-4f69-92ea-8b9e4eacfd43\", \ +\"ip_address\": \"172.24.4.226\"}", + "name"=>"", "subnet_id"=>"e4db0abd-276a-4f69-92ea-8b9e4eacfd43", "id"=>"de0dc526-02b2-467c-9832-2c3dc69ac2b4", "mac_address"=>"fa:16:3e:f6:b5:72"}] klass.expects(:auth_neutron). - with('router-port-list', '--format=json', router). - returns(output) - result = klass.list_router_ports(router) - expect(result).to eql(expected) - end - - - it 'should handle empty fixed_ips field' do - output = ''' - [ - { - "id": "1345e576-a21f-4c2e-b24a-b245639852ab", - "name": "", - "mac_address": "fa:16:3e:e3:e6:38", - "fixed_ips": "" - } - ] - ''' - expected = - [{ "name"=>"", - "id"=>"1345e576-a21f-4c2e-b24a-b245639852ab", - "mac_address"=>"fa:16:3e:e3:e6:38"}] - klass.expects(:auth_neutron). - with('router-port-list', '--format=json', router). + with('router-port-list', '--format=csv', router). returns(output) result = klass.list_router_ports(router) expect(result).to eql(expected) @@ -289,67 +243,4 @@ tenant_id="3056a91768d948d399f1d79051a7f221" end - describe 'should parse valid json output, covered by garbage' do - - it 'should parse valid output into a list of hashes' do - data = ''' - /usr/lib/python2.7/dist-packages/urllib3/util/ssl_.py:90: InsecurePlatformWarning: A true SSLContext object is not available. This prevents urllib3 from configuring SSL appropriately and may cause certain SSL connections to fail. For more information, see https://urllib3.readthedocs.org/en/latest/security.html#insecureplatformwarning. - InsecurePlatformWarning - /usr/lib/python2.7/dist-packages/urllib3/connection.py:251: SecurityWarning: Certificate has no `subjectAltName`, falling back to check for a `commonName` for now. This feature is being removed by major browsers and deprecated by RFC 2818. (See https://github.com/shazow/urllib3/issues/497 for details.) - SecurityWarning - [{"Field": "allocation_pools", "Value": "{\"start\": \"192.168.111.2\", \"end\": \"192.168.111.254\"}"}, {"Field": "cidr", "Value": "192.168.111.0/24"}, - {"Field": "dns_nameservers", "Value": "8.8.4.4\n8.8.8.8"}, {"Field": "enable_dhcp", "Value": true}, {"Field": "gateway_ip", "Value": "192.168.111.1"}, {"Field": "host_routes", "Value": ""}, {"Field": "id", "Value": "b87fbfd1-0e52-4ab6-8987-286ef0912d1f"}, {"Field": "ip_version", "Value": 4}, {"Field": "ipv6_address_mode", "Value": ""}, {"Field": "ipv6_ra_mode", "Value": ""}, - {"Field": "XXX", "Value": - [1, - 2,3]}, - {"Field": "name", "Value": "net04__subnet"}, {"Field": "network_id", "Value": "d70b399b-668b-4861-b092-4876ec65df60"}, {"Field": "subnetpool_id", "Value": ""}, {"Field": "tenant_id", "Value": "2764315d0ec24a07bf3773057aa51142"}] - xxx yyy zz - eof - ''' - expected = [ - {"Field"=>"allocation_pools", "Value"=>"{\"start\": \"192.168.111.2\", \"end\": \"192.168.111.254\"}"}, - {"Field"=>"cidr", "Value"=>"192.168.111.0/24"}, - {"Field"=>"dns_nameservers", "Value"=>"8.8.4.4\n8.8.8.8"}, - {"Field"=>"enable_dhcp", "Value"=>true}, - {"Field"=>"gateway_ip", "Value"=>"192.168.111.1"}, - {"Field"=>"host_routes", "Value"=>""}, - {"Field"=>"id", "Value"=>"b87fbfd1-0e52-4ab6-8987-286ef0912d1f"}, - {"Field"=>"ip_version", "Value"=>4}, - {"Field"=>"ipv6_address_mode", "Value"=>""}, - {"Field"=>"ipv6_ra_mode", "Value"=>""}, - {"Field"=>"XXX", "Value"=>[1, 2, 3]}, - {"Field"=>"name", "Value"=>"net04__subnet"}, - {"Field"=>"network_id", "Value"=>"d70b399b-668b-4861-b092-4876ec65df60"}, - {"Field"=>"subnetpool_id", "Value"=>""}, - {"Field"=>"tenant_id", "Value"=>"2764315d0ec24a07bf3773057aa51142"}] - expect(klass.find_and_parse_json(data)).to eq(expected) - end - end - - describe 'should parse valid json output, and convert booleans to idempotent strings' do - it 'boolean values should converted to capitalized strings' do - output = ''' - [{"Field": "allocation_pools", "Value": "{\"start\": \"192.168.111.2\", \"end\": \"192.168.111.254\"}"}, {"Field": "cidr", "Value": "192.168.111.0/24"}, - {"Field": "dns_nameservers", "Value": "8.8.4.4\n8.8.8.8"}, {"Field": "enable_dhcp", "Value": true}, {"Field": "gateway_ip", "Value": "192.168.111.1"}, {"Field": "host_routes", "Value": ""}, {"Field": "id", "Value": "b87fbfd1-0e52-4ab6-8987-286ef0912d1f"}, {"Field": "ip_version", "Value": 4}, {"Field": "ipv6_address_mode", "Value": ""}, {"Field": "ipv6_ra_mode", "Value": ""}, - {"Field": "YYY", "Value": false}] - ''' - klass.stubs(:auth_neutron).returns(output) - result = klass.get_neutron_resource_attrs 'foo', nil - expect(result['enable_dhcp']).to eql('True') - expect(result['YYY']).to eql('False') - end - - it 'stringifyed boolean values should converted to capitalized strings' do - output = ''' - [{"Field": "allocation_pools", "Value": "{\"start\": \"192.168.111.2\", \"end\": \"192.168.111.254\"}"}, {"Field": "cidr", "Value": "192.168.111.0/24"}, - {"Field": "dns_nameservers", "Value": "8.8.4.4\n8.8.8.8"}, {"Field": "enable_dhcp", "Value": "True"}, {"Field": "gateway_ip", "Value": "192.168.111.1"}, {"Field": "host_routes", "Value": ""}, {"Field": "id", "Value": "b87fbfd1-0e52-4ab6-8987-286ef0912d1f"}, {"Field": "ip_version", "Value": 4}, {"Field": "ipv6_address_mode", "Value": ""}, {"Field": "ipv6_ra_mode", "Value": ""}, - {"Field": "YYY", "Value": "false"}] - ''' - klass.stubs(:auth_neutron).returns(output) - result = klass.get_neutron_resource_attrs 'foo', nil - expect(result['enable_dhcp']).to eql('True') - expect(result['YYY']).to eql('False') - end - end - end