From 39b58df7a8bde6fa3645e77965ce1380def32c99 Mon Sep 17 00:00:00 2001 From: Colleen Murphy Date: Fri, 10 Apr 2015 14:24:32 -0700 Subject: [PATCH] Use OpenstackClient for glance_image auth This patch changes the glance_image provider to use puppet-openstacklib's authentication methods, which use python-openstackclient as an interface, instead of the glance command line client. The benefits of this is: - Code reduction. This patch reduces the amount of code in the glance parent provider and glance_image provider by reusing code from Puppet::Provider::Openstack instead of implementing authentication, retries, and response parsing in the provider. - Presumed reduction in sudden API changes that require quick fixes in this module, such as f377c0229c006b02f43a14be4979553e983cb98e. - Ability to set project-based access control for images Additional reasoning for this change is in the blueprint. Important note: this change does not work on Ubuntu under Juno due to a major bug in the version of python-openstackclient shipped in UCA [1]. This change targets the Kilo releases. Note about performance: the self.instances and instances methods make API requests for each image found in the list of instances. This is not a change from how this was implemented before. The --long formatted list, either from the glance client or openstackclient, does not provide all information needed to query an instance and populate setters. Future patches could possibly improve on this. Other details of this change: - Removes auth_glance method, replaced by the request and glance_request methods - Removes auth_glance_stdin method which was not being used - Removes parse_table which is now unnecessary because openstackclient formats its responses as CSV and Puppet::Provider::Openstack#request returns the results a hash - Removes remove_warnings methods which are handled by Puppet::Provider::Openstack#request - Removes list_glance_images and get_glance_image_attr which are sufficiently replaced by using the 'list' and 'show' commands with Puppet::Provider::Openstack's request method. - Removed self.prefetch since we can't populate auth parameters during a prefetch. Instead we prepopulate the list via the instances method. - Added a flush method to do updates more efficiently - Changed is_public property to accept true/false in addition to yes/no and to munge to a symbol instead of a capitalized string, for consistency with keystone_tenant's enabled property - Move the reset method into the spec tests since it is only necessary for testing - Added tests for glance_image, which did not exist before - Removed connection retry test since this is taken care of in openstacklib [1] https://bugs.launchpad.net/python-openstackclient/+bug/1269821 blueprint use-openstackclient-in-module-resources Change-Id: Iceab5e1c6138e7aba0f883ed4acb8f7ecbcd2830 --- lib/puppet/provider/glance.rb | 153 +++--------------- lib/puppet/provider/glance_image/glance.rb | 111 ------------- lib/puppet/provider/glance_image/openstack.rb | 124 ++++++++++++++ lib/puppet/type/glance_image.rb | 10 +- manifests/init.pp | 2 + spec/classes/glance_spec.rb | 2 + spec/spec_helper.rb | 2 + spec/unit/provider/glance_image_spec.rb | 119 ++++++++++++++ spec/unit/provider/glance_spec.rb | 50 ++---- 9 files changed, 291 insertions(+), 282 deletions(-) delete mode 100644 lib/puppet/provider/glance_image/glance.rb create mode 100644 lib/puppet/provider/glance_image/openstack.rb create mode 100644 spec/unit/provider/glance_image_spec.rb diff --git a/lib/puppet/provider/glance.rb b/lib/puppet/provider/glance.rb index 2501d63c..a34944f8 100644 --- a/lib/puppet/provider/glance.rb +++ b/lib/puppet/provider/glance.rb @@ -2,7 +2,29 @@ # this probably could have all gone in the provider file. # But maybe this is good long-term. require 'puppet/util/inifile' -class Puppet::Provider::Glance < Puppet::Provider +require 'puppet/provider/openstack' +require 'puppet/provider/openstack/auth' +require 'puppet/provider/openstack/credentials' +class Puppet::Provider::Glance < Puppet::Provider::Openstack + + extend Puppet::Provider::Openstack::Auth + + def self.request(service, action, properties=nil) + begin + super + rescue Puppet::Error::OpenstackAuthInputError => error + glance_request(service, action, error, properties) + end + end + + def self.glance_request(service, action, error, properties=nil) + @credentials.username = glance_credentials['admin_user'] + @credentials.password = glance_credentials['admin_password'] + @credentials.project_name = glance_credentials['admin_tenant_name'] + @credentials.auth_url = auth_endpoint + raise error unless @credentials.set? + Puppet::Provider::Openstack.request(service, action, properties, @credentials) + end def self.glance_credentials @glance_credentials ||= get_glance_credentials @@ -51,10 +73,6 @@ class Puppet::Provider::Glance < Puppet::Provider end end - def glance_credentials - self.class.glance_credentials - end - def self.auth_endpoint @auth_endpoint ||= get_auth_endpoint end @@ -79,129 +97,8 @@ class Puppet::Provider::Glance < Puppet::Provider @glance_hash ||= build_glance_hash end - def self.reset - @glance_hash = nil - @glance_file = nil - @glance_credentials = nil - @auth_endpoint = nil + def bool_to_sym(bool) + bool == true ? :true : :false end - def glance_hash - self.class.glance_hash - end - - def self.auth_glance(*args) - begin - g = glance_credentials - remove_warnings(glance('--os-tenant-name', g['admin_tenant_name'], '--os-username', g['admin_user'], '--os-password', g['admin_password'], '--os-region-name', g['os_region_name'], '--os-auth-url', auth_endpoint, args)) - rescue Exception => e - if (e.message =~ /\[Errno 111\] Connection refused/) or (e.message =~ /\(HTTP 400\)/) or (e.message =~ /HTTP Unable to establish connection/) - sleep 10 - remove_warnings(glance('--os-tenant-name', g['admin_tenant_name'], '--os-username', g['admin_user'], '--os-password', g['admin_password'], '--os-region-name', g['os_region_name'], '--os-auth-url', auth_endpoint, args)) - else - raise(e) - end - end - end - - def auth_glance(*args) - self.class.auth_glance(args) - end - - def self.auth_glance_stdin(*args) - begin - g = glance_credentials - command = "glance --os-tenant-name #{g['admin_tenant_name']} --os-username #{g['admin_user']} --os-password #{g['admin_password']} --os-region-name #{g['os_region_name']} --os-auth-url #{auth_endpoint} #{args.join(' ')}" - - # This is a horrible, horrible hack - # Redirect stderr to stdout in order to report errors - # Ignore good output - err = `#{command} 3>&1 1>/dev/null 2>&3` - if $? != 0 - raise(Puppet::Error, err) - end - end - end - - def auth_glance_stdin(*args) - self.class.auth_glance_stdin(args) - end - - private - def self.list_glance_images - ids = [] - (auth_glance('image-list').split("\n")[3..-2] || []).collect do |line| - ids << line.split('|')[1].strip() - end - return ids - end - - def self.get_glance_image_attr(id, attr) - (auth_glance('image-show', id).split("\n") || []).collect do |line| - if line =~ /^#{attr}:/ - return line.split(': ')[1..-1] - end - end - end - - def self.get_glance_image_attrs(id) - attrs = {} - (auth_glance('image-show', id).split("\n")[3..-2] || []).collect do |line| - attrs[line.split('|')[1].strip()] = line.split('|')[2].strip() - end - return attrs - end - - def parse_table(table) - # parse the table into an array of maps with a simplistic state machine - found_header = false - parsed_header = false - keys = nil - results = [] - table.split("\n").collect do |line| - # look for the header - if not found_header - if line =~ /^\+[-|+]+\+$/ - found_header = true - nil - end - # look for the key names in the table header - elsif not parsed_header - if line =~ /^(\|\s*[:alpha:]\s*)|$/ - keys = line.split('|').map(&:strip) - parsed_header = true - end - # parse the values in the rest of the table - elsif line =~ /^|.*|$/ - values = line.split('|').map(&:strip) - result = Hash[keys.zip values] - results << result - end - end - results - end - - # Remove warning from the output. This is a temporary hack until - # things will be refactored to use the REST API - def self.remove_warnings(results) - found_header = false - in_warning = false - results.split("\n").collect do |line| - unless found_header - if line =~ /^\+[-\+]+\+$/ # Matches upper and lower box borders - in_warning = false - found_header = true - line - elsif line =~ /^WARNING/ or line =~ /UserWarning/ or in_warning - # warnings can be multi line, we have to skip all of them - in_warning = true - nil - else - line - end - else - line - end - end.compact.join("\n") - end end diff --git a/lib/puppet/provider/glance_image/glance.rb b/lib/puppet/provider/glance_image/glance.rb deleted file mode 100644 index fae22e03..00000000 --- a/lib/puppet/provider/glance_image/glance.rb +++ /dev/null @@ -1,111 +0,0 @@ -# Load the Glance provider library to help -require File.join(File.dirname(__FILE__), '..','..','..', 'puppet/provider/glance') - -Puppet::Type.type(:glance_image).provide( - :glance, - :parent => Puppet::Provider::Glance -) do - desc <<-EOT - Glance provider to manage glance_image type. - - Assumes that the glance-api service is on the same host and is working. - EOT - - commands :glance => 'glance' - - mk_resource_methods - - def self.instances - list_glance_images.collect do |image| - attrs = get_glance_image_attrs(image) - new( - :ensure => :present, - :name => attrs['name'], - :is_public => attrs['is_public'], - :container_format => attrs['container_format'], - :id => attrs['id'], - :disk_format => attrs['disk_format'] - ) - end - end - - def self.prefetch(resources) - images = instances - resources.keys.each do |name| - if provider = images.find{ |pkg| pkg.name == name } - resources[name].provider = provider - end - end - end - - def exists? - @property_hash[:ensure] == :present - end - - def create - if resource[:source] - # copy_from cannot handle file:// - if resource[:source] =~ /^\// # local file - location = "--file=#{resource[:source]}" - else - location = "--copy-from=#{resource[:source]}" - end - # location cannot handle file:// - # location does not import, so no sense in doing anything more than this - elsif resource[:location] - location = "--location=#{resource[:location]}" - else - raise(Puppet::Error, "Must specify either source or location") - end - results = auth_glance('image-create', "--name=#{resource[:name]}", "--is-public=#{resource[:is_public]}", "--container-format=#{resource[:container_format]}", "--disk-format=#{resource[:disk_format]}", location) - - id = nil - - # Check the old behavior of the python-glanceclient - if results =~ /Added new image with ID: (\S+)/ - id = $1 - else # the new behavior doesn't print the status, so parse the table - results_array = parse_table(results) - results_array.each do |result| - if result["Property"] == "id" - id = result["Value"] - end - end - end - - if id - @property_hash = { - :ensure => :present, - :name => resource[:name], - :is_public => resource[:is_public], - :container_format => resource[:container_format], - :disk_format => resource[:disk_format], - :id => id - } - else - fail("did not get expected message from image creation, got #{results}") - end - end - - def destroy - auth_glance('image-delete', id) - @property_hash[:ensure] = :absent - end - - def is_public=(value) - auth_glance('image-update', id, "--is-public=#{value}") - end - - def disk_format=(value) - auth_glance('image-update', id, "--disk-format=#{value}") - end - - def container_format=(value) - auth_glance('image-update', id, "--container-format=#{value}") - end - - def id=(id) - fail('id is read only') - end - -end diff --git a/lib/puppet/provider/glance_image/openstack.rb b/lib/puppet/provider/glance_image/openstack.rb new file mode 100644 index 00000000..72628150 --- /dev/null +++ b/lib/puppet/provider/glance_image/openstack.rb @@ -0,0 +1,124 @@ +require File.join(File.dirname(__FILE__), '..','..','..', 'puppet/provider/glance') + +Puppet::Type.type(:glance_image).provide( + :openstack, + :parent => Puppet::Provider::Glance +) do + desc <<-EOT + Provider to manage glance_image type. + EOT + + @credentials = Puppet::Provider::Openstack::CredentialsV2_0.new + + def initialize(value={}) + super(value) + @property_flush = {} + end + + def create + if resource[:source] + # copy_from cannot handle file:// + if resource[:source] =~ /^\// # local file + location = "--file=#{resource[:source]}" + else + location = "--copy-from=#{resource[:source]}" + end + # location cannot handle file:// + # location does not import, so no sense in doing anything more than this + elsif resource[:location] + location = "--location=#{resource[:location]}" + else + raise(Puppet::Error, "Must specify either source or location") + end + properties = [resource[:name]] + if resource[:is_public] == :true + properties << "--public" + else + # This is the default, but it's nice to be verbose + properties << "--private" + end + properties << "--container-format=#{resource[:container_format]}" + properties << "--disk-format=#{resource[:disk_format]}" + properties << location + @property_hash = self.class.request('image', 'create', properties) + @property_hash[:ensure] = :present + end + + def exists? + @property_hash[:ensure] == :present + end + + def destroy + self.class.request('image', 'delete', resource[:name]) + @property_hash.clear + end + + def is_public=(value) + @property_flush[:is_public] = value + end + + def is_public + bool_to_sym(@property_hash[:is_public]) + end + + def disk_format=(value) + @property_flush[:disk_format] = value + end + + def disk_format + @property_hash[:disk_format] + end + + def container_format=(value) + @property_flush[:container_format] = value + end + + def container_format + @property_hash[:container_format] + end + + def id=(id) + fail('id is read only') + end + + def id + @property_hash[:id] + end + + def self.instances + list = request('image', 'list', '--long') + list.collect do |image| + attrs = request('image', 'show', image[:id]) + new( + :ensure => :present, + :name => attrs[:name], + :is_public => attrs[:is_public].downcase.chomp == 'true'? true : false, + :container_format => attrs[:container_format], + :id => attrs[:id], + :disk_format => attrs[:disk_format] + ) + end + end + + def self.prefetch(resources) + images = instances + resources.keys.each do |name| + if provider = images.find{ |image| image.name == name } + resources[name].provider = provider + end + end + end + + def flush + properties = [resource[:name]] + if @property_flush + (properties << '--public') if @property_flush[:is_public] == :true + (properties << '--private') if @property_flush[:is_public] == :false + (properties << "--container-format=#{@property_flush[:container_format]}") if @property_flush[:container_format] + (properties << "--disk-format=#{@property_flush[:disk_format]}") if @property_flush[:disk_format] + self.class.request('image', 'set', properties) + @property_flush.clear + end + end + +end diff --git a/lib/puppet/type/glance_image.rb b/lib/puppet/type/glance_image.rb index 10e577ef..76783f5f 100644 --- a/lib/puppet/type/glance_image.rb +++ b/lib/puppet/type/glance_image.rb @@ -43,13 +43,15 @@ Puppet::Type.newtype(:glance_image) do newproperty(:is_public) do desc "Whether the image is public or not. Default true" - newvalues(/(y|Y)es/, /(n|N)o/) - defaultto('Yes') + newvalues(/(y|Y)es/, /(n|N)o/, /(t|T)rue/, /(f|F)alse/, true, false) + defaultto(true) munge do |v| if v =~ /^(y|Y)es$/ - 'True' + :true elsif v =~ /^(n|N)o$/ - 'False' + :false + else + v.to_s.downcase.to_sym end end end diff --git a/manifests/init.pp b/manifests/init.pp index 3b104225..d7d4184d 100644 --- a/manifests/init.pp +++ b/manifests/init.pp @@ -29,4 +29,6 @@ class glance( tag => ['openstack'], } } + + ensure_resource('package', 'python-openstackclient', {'ensure' => $package_ensure}) } diff --git a/spec/classes/glance_spec.rb b/spec/classes/glance_spec.rb index 075cec5e..fc027d44 100644 --- a/spec/classes/glance_spec.rb +++ b/spec/classes/glance_spec.rb @@ -31,6 +31,8 @@ describe 'glance' do 'mode' => '0770' )} + it { is_expected.to contain_package('python-openstackclient') } + end end diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 53d4dd02..78594f8a 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -1,3 +1,5 @@ +# Load libraries from openstacklib here to simulate how they live together in a real puppet run (for provider unit tests) +$LOAD_PATH.push(File.join(File.dirname(__FILE__), 'fixtures', 'modules', 'openstacklib', 'lib')) require 'puppetlabs_spec_helper/module_spec_helper' require 'shared_examples' diff --git a/spec/unit/provider/glance_image_spec.rb b/spec/unit/provider/glance_image_spec.rb new file mode 100644 index 00000000..275f9ca9 --- /dev/null +++ b/spec/unit/provider/glance_image_spec.rb @@ -0,0 +1,119 @@ +require 'puppet' +require 'spec_helper' +require 'puppet/provider/glance_image/openstack' + +provider_class = Puppet::Type.type(:glance_image).provider(:openstack) + +describe provider_class do + + shared_examples 'authenticated with environment variables' do + ENV['OS_USERNAME'] = 'test' + ENV['OS_PASSWORD'] = 'abc123' + ENV['OS_PROJECT_NAME'] = 'test' + ENV['OS_AUTH_URL'] = 'http://127.0.0.1:35357/v2.0' + end + + describe 'when managing an image' do + + let(:tenant_attrs) do + { + :ensure => 'present', + :name => 'image1', + :is_public => 'yes', + :container_format => 'bare', + :disk_format => 'qcow2', + :source => 'http://example.com/image1.img', + } + end + + let(:resource) do + Puppet::Type::Glance_image.new(tenant_attrs) + end + + let(:provider) do + provider_class.new(resource) + end + + it_behaves_like 'authenticated with environment variables' do + describe '#create' do + it 'creates an image' do + provider.class.stubs(:openstack) + .with('image', 'list', '--quiet', '--format', 'csv', '--long') + .returns('"ID","Name","Disk Format","Container Format","Size","Status" +"534 5b502-efe4-4852-a45d-edaba3a3acc6","image1","raw","bare",1270,"active" +') + provider.class.stubs(:openstack) + .with('image', 'create', '--format', 'shell', ['image1', '--public', '--container-format=bare', '--disk-format=qcow2', '--copy-from=http://example.com/image1.img' ]) + .returns('checksum="09b9c392dc1f6e914cea287cb6be34b0" +container_format="bare" +created_at="2015-04-08T18:28:01" +deleted="False" +deleted_at="None" +disk_format="qcow2" +id="5345b502-efe4-4852-a45d-edaba3a3acc6" +is_public="True" +min_disk="0" +min_ram="0" +name="image1" +owner="None" +properties="{}" +protected="False" +size="1270" +status="active" +updated_at="2015-04-10T18:18:18" +virtual_size="None" +') + provider.create + expect(provider.exists?).to be_truthy + end + end + end + + describe '#destroy' do + it 'destroys an image' do + provider.class.stubs(:openstack) + .with('image', 'list', '--quiet', '--format', 'csv') + .returns('"ID","Name","Disk Format","Container Format","Size","Status"') + provider.class.stubs(:openstack) + .with('image', 'delete', 'image1') + provider.destroy + expect(provider.exists?).to be_falsey + end + + end + + describe '.instances' do + it 'finds every image' do + provider.class.stubs(:openstack) + .with('image', 'list', '--quiet', '--format', 'csv', '--long') + .returns('"ID","Name","Disk Format","Container Format","Size","Status" +"5345b502-efe4-4852-a45d-edaba3a3acc6","image1","raw","bare",1270,"active" +') + provider.class.stubs(:openstack) + .with('image', 'show', '--format', 'shell', '5345b502-efe4-4852-a45d-edaba3a3acc6') + .returns('checksum="09b9c392dc1f6e914cea287cb6be34b0" +container_format="bare" +created_at="2015-04-08T18:28:01" +deleted="False" +deleted_at="None" +disk_format="qcow2" +id="5345b502-efe4-4852-a45d-edaba3a3acc6" +is_public="True" +min_disk="0" +min_ram="0" +name="image1" +owner="None" +properties="{}" +protected="False" +size="1270" +status="active" +updated_at="2015-04-10T18:18:18" +virtual_size="None" +') + instances = provider_class.instances + expect(instances.count).to eq(1) + end + end + + end +end diff --git a/spec/unit/provider/glance_spec.rb b/spec/unit/provider/glance_spec.rb index 344ec4a2..33ea965e 100644 --- a/spec/unit/provider/glance_spec.rb +++ b/spec/unit/provider/glance_spec.rb @@ -6,6 +6,15 @@ require 'tempfile' klass = Puppet::Provider::Glance +class Puppet::Provider::Glance + def self.reset + @admin_endpoint = nil + @tenant_hash = nil + @admin_token = nil + @keystone_file = nil + end +end + describe Puppet::Provider::Glance do after :each do @@ -14,7 +23,7 @@ describe Puppet::Provider::Glance do describe 'when retrieving the auth credentials' do - it 'should fail if the glance config file does not have the expected contents' do + it 'should fail if no auth params are passed and the glance config file does not have the expected contents' do mock = {} Puppet::Util::IniConfig::File.expects(:new).returns(mock) mock.expects(:read).with('/etc/glance/glance-api.conf') @@ -23,43 +32,6 @@ describe Puppet::Provider::Glance do end.to raise_error(Puppet::Error, /does not contain all required sections/) end - describe 'when testing glance connection retries' do - - ['[Errno 111] Connection refused', '(HTTP 400)', 'HTTP Unable to establish connection'].reverse.each do |valid_message| - it "should retry when glance is not ready with error #{valid_message}" do - mock = {'keystone_authtoken' => - { - 'auth_host' => '127.0.0.1', - 'auth_port' => '35357', - 'auth_protocol' => 'http', - 'admin_tenant_name' => 'foo', - 'admin_user' => 'user', - 'admin_password' => 'pass' - }, - 'glance_store' => - { - 'os_region_name' => 'SomeRegion', - } - } - Puppet::Util::IniConfig::File.expects(:new).returns(mock) - mock.expects(:read).with('/etc/glance/glance-api.conf') - klass.expects(:sleep).with(10).returns(nil) - klass.expects(:glance).twice.with( - '--os-tenant-name', - 'foo', - '--os-username', - 'user', - '--os-password', - 'pass', - '--os-region-name', - 'SomeRegion', - '--os-auth-url', - 'http://127.0.0.1:35357/v2.0/', - ['test_retries'] - ).raises(Exception, valid_message).then.returns('') - klass.auth_glance('test_retries') - end - end - end end + end