Merge "Use OpenstackClient for glance_image auth"

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
+  def bool_to_sym(bool)
-    @glance_hash        = nil
+    bool == true ? :true : :false
-    @glance_file        = nil
-    @glance_credentials = nil
-    @auth_endpoint      = nil
   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

lib/puppet/provider/glance_image/glance.rb

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

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

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/)
+    newvalues(/(y|Y)es/, /(n|N)o/, /(t|T)rue/, /(f|F)alse/, true, false)
-    defaultto('Yes')
+    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

manifests/init.pp

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

spec/classes/glance_spec.rb

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

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'
 

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

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