openstack/puppet-glance
Code Issues Proposed changes

Use OpenstackClient for glance_image auth

Browse Source 
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 f377c0229c.
- 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
This commit is contained in:
Colleen Murphy 2015-04-10 14:24:32 -07:00 committed by Colleen Murphy
parent 93f79bb56d
commit 39b58df7a8
9 changed files with 291 additions and 282 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

153
lib/puppet/provider/glance.rb
View File

@ -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

111
lib/puppet/provider/glance_image/glance.rb
View File

@ -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

124
lib/puppet/provider/glance_image/openstack.rb Normal file
View File

@ -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

10
lib/puppet/type/glance_image.rb
View File

@ -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

2
manifests/init.pp
View File

@ -29,4 +29,6 @@ class glance(
tag => ['openstack'],
}
}
ensure_resource('package', 'python-openstackclient', {'ensure' => $package_ensure})
}

2
spec/classes/glance_spec.rb
View File

@ -31,6 +31,8 @@ describe 'glance' do
'mode' => '0770'
)}
it { is_expected.to contain_package('python-openstackclient') }
end
end

2
spec/spec_helper.rb
View File

@ -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'

119
spec/unit/provider/glance_image_spec.rb Normal file
View File

@ -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

50
spec/unit/provider/glance_spec.rb
View File

@ -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