From 08ee5866a6c6440f9202eddbb8f7ae4ec24a1fca Mon Sep 17 00:00:00 2001
From: Denis Egorenko <degorenko@mirantis.com>
Date: Wed, 23 Sep 2015 18:50:36 +0300
Subject: [PATCH] Use OpenstackClient for nova providers auth

This patch changes the nova providers to use puppet-openstacklib's
authentication methods, which use python-openstackclient as an interface,
instead of the nova command line client.

The benefits of this is a code reduction. This patch reduces the amount
of code in the nova parent provider and nova providers by reusing code from
Puppet::Provider::Openstack instead of implementing authentication,
retries, and response parsing in the provider.

This patch doesn't affect next providers:

 * nova_network and nova_floating:
   openstack client has small functionality for managing nova floatings
   and doesn't provide possibility to manage nova-networks,
   so keeping old format of auth for this providers.
   Also Nova-Network is deprecated.
 * nova_cell:
   openstack client doesn't provide possibility to manage cells;
 * nova security groups - will be done in separate patch;

Additional reasoning for this change is in the blueprint.

Also added new tests for providers.

blueprint use-openstackclient-in-module-resources

Change-Id: Ifa09aeb71ba0bcc425eece314803a0d1609bed9f
---
 lib/puppet/provider/nova.rb                   |  95 +++-------
 lib/puppet/provider/nova_aggregate/nova.rb    | 169 ------------------
 .../provider/nova_aggregate/openstack.rb      | 107 +++++++++++
 lib/puppet/type/nova_aggregate.rb             |   8 +-
 ...client_for_providers-5c83fdf2128197cb.yaml |   3 +
 spec/acceptance/nova_wsgi_apache_spec.rb      |  16 ++
 spec/spec_helper.rb                           |   2 +
 .../provider/nova_aggregate/openstack_spec.rb | 100 +++++++++++
 spec/unit/provider/nova_spec.rb               | 145 ---------------
 spec/unit/type/nova_aggregate_spec.rb         |  33 ++++
 10 files changed, 296 insertions(+), 382 deletions(-)
 delete mode 100644 lib/puppet/provider/nova_aggregate/nova.rb
 create mode 100644 lib/puppet/provider/nova_aggregate/openstack.rb
 create mode 100644 releasenotes/notes/implement_openstack_client_for_providers-5c83fdf2128197cb.yaml
 create mode 100644 spec/unit/provider/nova_aggregate/openstack_spec.rb
 create mode 100644 spec/unit/type/nova_aggregate_spec.rb

diff --git a/lib/puppet/provider/nova.rb b/lib/puppet/provider/nova.rb
index 9eaa63742..3d6d72337 100644
--- a/lib/puppet/provider/nova.rb
+++ b/lib/puppet/provider/nova.rb
@@ -1,13 +1,37 @@
 # Run test ie with: rspec spec/unit/provider/nova_spec.rb
 
 require 'puppet/util/inifile'
+require 'puppet/provider/openstack'
+require 'puppet/provider/openstack/auth'
+require 'puppet/provider/openstack/credentials'
 
-class Puppet::Provider::Nova < Puppet::Provider
+class Puppet::Provider::Nova < Puppet::Provider::Openstack
+
+  extend Puppet::Provider::Openstack::Auth
+
+  def self.request(service, action, properties=nil)
+    begin
+      super
+    rescue Puppet::Error::OpenstackAuthInputError => error
+      nova_request(service, action, error, properties)
+    end
+  end
+
+  def self.nova_request(service, action, error, properties=nil)
+    properties ||= []
+    @credentials.username = nova_credentials['admin_user']
+    @credentials.password = nova_credentials['admin_password']
+    @credentials.project_name = nova_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.conf_filename
     '/etc/nova/nova.conf'
   end
 
+  # deprecated: method for old nova cli auth
   def self.withenv(hash, &block)
     saved = ENV.to_hash
     hash.each do |name, val|
@@ -66,6 +90,7 @@ class Puppet::Provider::Nova < Puppet::Provider
     @auth_endpoint ||= get_auth_endpoint
   end
 
+  # deprecated: method for old nova cli auth
   def self.auth_nova(*args)
     q = nova_credentials
     authenv = {
@@ -94,6 +119,7 @@ class Puppet::Provider::Nova < Puppet::Provider
     end
   end
 
+  # deprecated: method for old nova cli auth
   def auth_nova(*args)
     self.class.auth_nova(args)
   end
@@ -113,6 +139,7 @@ class Puppet::Provider::Nova < Puppet::Provider
     end
   end
 
+  # deprecated: string to list for nova cli
   def self.str2list(s)
     #parse string
     if s.include? ","
@@ -146,6 +173,7 @@ class Puppet::Provider::Nova < Puppet::Provider
     end
   end
 
+  # deprecated: nova cli to list
   def self.cliout2list(output)
     #don't proceed with empty output
     if output.empty?
@@ -176,69 +204,4 @@ class Puppet::Provider::Nova < Puppet::Provider
     return hash_list
   end
 
-  def self.nova_hosts
-    return @nova_hosts if @nova_hosts
-    cmd_output = auth_nova("host-list")
-    @nova_hosts = cliout2list(cmd_output)
-    @nova_hosts
-  end
-
-  def self.nova_get_host_by_name_and_type(host_name, service_type)
-    #find the host by name and service type
-    nova_hosts.each do |entry|
-      # (mdorman) Support api!cell_name@host_name -style output of nova host-list under nova cells
-      if entry["host_name"] =~ /^([a-zA-Z0-9\-_]+![a-zA-Z0-9\-_]+@)?#{Regexp.quote(host_name)}$/
-        if entry["service"] == service_type
-            return host_name
-        end
-      end
-    end
-    #name/service combo not found
-    return nil
-  end
-
-  def self.nova_aggregate_resources_ids(force_refresh=false)
-    # return the cached list unless requested
-    if not force_refresh
-      return @nova_aggregate_resources_ids if @nova_aggregate_resources_ids
-    end
-    #produce a list of hashes with Id=>Name pairs
-    lines = []
-    #run command
-    cmd_output = auth_nova("aggregate-list")
-    #parse output
-    @nova_aggregate_resources_ids = cliout2list(cmd_output)
-    #only interessted in Id and Name
-    @nova_aggregate_resources_ids.map{ |e| e.delete("Availability Zone")}
-    @nova_aggregate_resources_ids.map{ |e|
-      if e['Id'] =~ /^[0-9]+$/
-        e['Id'] = e['Id'].to_i
-      end }
-    @nova_aggregate_resources_ids
-  end
-
-  def self.nova_aggregate_resources_get_name_by_id(name, force_refresh=false)
-    #find the id by the given name
-    nova_aggregate_resources_ids(force_refresh).each do |entry|
-      if entry["Name"] == name
-        return entry["Id"]
-      end
-    end
-    #name not found
-    return nil
-  end
-
-  def self.nova_aggregate_resources_attr(id)
-    #run command to get details for given Id
-    cmd_output = auth_nova("aggregate-details", id)
-    list = cliout2list(cmd_output)[0]
-    if ! list["Hosts"].is_a?(Array)
-      if list["Hosts"] == ""
-        list["Hosts"] = []
-      else
-        list["Hosts"] = [ list["Hosts"] ]
-      end
-    end
-    return list
-  end
 end
diff --git a/lib/puppet/provider/nova_aggregate/nova.rb b/lib/puppet/provider/nova_aggregate/nova.rb
deleted file mode 100644
index ec326eb51..000000000
--- a/lib/puppet/provider/nova_aggregate/nova.rb
+++ /dev/null
@@ -1,169 +0,0 @@
-require File.join(File.dirname(__FILE__), '..','..','..',
-                  'puppet/provider/nova')
-
-Puppet::Type.type(:nova_aggregate).provide(
-  :nova,
-  :parent => Puppet::Provider::Nova
-) do
-
-  desc "Manage nova aggregations"
-
-  commands :nova => 'nova'
-
-  mk_resource_methods
-
-  def self.instances
-    nova_aggregate_resources_ids().collect do |el|
-      attrs = nova_aggregate_resources_attr(el['Name'])
-      new(
-          :ensure => :present,
-          :name => attrs['Name'],
-          :id => attrs['Id'],
-          :availability_zone => attrs['Availability Zone'],
-          :metadata => attrs['Metadata'],
-          :hosts => attrs['Hosts'].sort
-          )
-    end
-  end
-
-  def self.prefetch(resources)
-    instances_ = instances
-    resources.keys.each do |name|
-      if provider = instances_.find{ |instance| instance.name == name }
-        resources[name].provider = provider
-      end
-    end
-  end
-
-  def exists?
-    @property_hash[:ensure] == :present
-  end
-
-  def destroy
-    #delete hosts first
-    if not @property_hash[:hosts].nil?
-      @property_hash[:hosts].each do |h|
-        auth_nova("aggregate-remove-host", name, h)
-      end
-    end
-    #now delete aggregate
-    auth_nova("aggregate-delete", name)
-    @property_hash[:ensure] = :absent
-  end
-
-  def create
-    extras = Array.new
-    #check for availability zone
-    if not @resource[:availability_zone].nil? and not @resource[:availability_zone].empty?
-      extras << "#{@resource[:availability_zone]}"
-    end
-    #run the command
-    result = auth_nova("aggregate-create", resource[:name], extras)
-
-    #get Id by Name
-    #force a refresh of the aggregate list on creation
-    id = self.class.nova_aggregate_resources_get_name_by_id(resource[:name], true)
-
-    @property_hash = {
-      :ensure => :present,
-      :name => resource[:name],
-      :id => id,
-      :availability_zone => resource[:availability_zone]
-      }
-
-    #add metadata
-    if not @resource[:metadata].nil? and not @resource[:metadata].empty?
-      @resource[:metadata].each do |key, value|
-        set_metadata_helper(resource[:name], key, value)
-      end
-      @property_hash[:metadata] = resource[:metadata]
-    end
-
-    #add hosts - This throws an error if the host is already attached to another aggregate!
-    if not @resource[:hosts].nil? and not @resource[:hosts].empty?
-      @resource[:hosts].each do |host|
-        # make sure the host exists in nova, or nova will fail the call
-        # this solves weird ordering issues with a compute node that's
-        # not 100% up being added to the host aggregate
-        if is_host_in_nova?(host)
-          auth_nova("aggregate-add-host", resource[:name], "#{host}")
-        else
-          warning("Cannot add #{host} to host aggregate, it's not available yet in nova host-list")
-        end
-      end
-      @property_hash[:hosts] = resource[:hosts]
-    end
-  end
-
-  def is_host_in_nova?(host)
-    return host==self.class.nova_get_host_by_name_and_type(host, "compute")
-  end
-
-  def hosts=(val)
-    #get current hosts
-    attrs = self.class.nova_aggregate_resources_attr(name)
-    #remove all hosts which are not in new value list
-    attrs['Hosts'].each do |h|
-      if not val.include? h
-        auth_nova("aggregate-remove-host", name, "#{h}")
-      end
-    end
-
-    #add hosts from the value list
-    val.each do |h|
-      if not attrs['Hosts'].include? h
-        if is_host_in_nova?(h)
-          auth_nova("aggregate-add-host", name, "#{h}")
-        else
-          warning("Cannot add #{h} to host aggregate, it's not available yet in nova host-list")
-        end
-      end
-    end
-  end
-
-  def set_metadata_helper(agg_id, key, value)
-    auth_nova("aggregate-set-metadata", agg_id, "#{key}=#{value}")
-  end
-
-  def metadata
-    #get current metadata
-    attrs = self.class.nova_aggregate_resources_attr(name)
-    #just ignore the availability_zone. that's handled directly by nova
-    attrs['Metadata'].delete('availability_zone')
-    return attrs['Metadata']
-  end
-
-  def metadata=(val)
-    #get current metadata
-    attrs = self.class.nova_aggregate_resources_attr(name)
-    #get keys which are in current metadata but not in val. Make sure it has data first!
-    if attrs['Metadata'].length > 0
-      obsolete_keys = attrs['Metadata'].keys - val.keys
-    end
-    # clear obsolete keys. If there are any!
-    if obsolete_keys
-      obsolete_keys.each do |key|
-        if not key.include? 'availability_zone'
-          auth_nova("aggregate-set-metadata", name, "#{key}")
-        end
-      end
-      #handle keys (with obsolete keys)
-      new_keys = val.keys - obsolete_keys
-    else
-      #handle keys (without obsolete keys)
-      new_keys = val.keys
-    end
-    #set new metadata if value changed
-    new_keys.each do |key|
-      if val[key] != attrs['Metadata'][key.to_s]
-        value = val[key]
-        set_metadata_helper(name, key, value)
-      end
-    end
-  end
-
-  def availability_zone=(val)
-    auth_nova("aggregate-set-metadata", name, "availability_zone=#{val}")
-  end
-
-end
diff --git a/lib/puppet/provider/nova_aggregate/openstack.rb b/lib/puppet/provider/nova_aggregate/openstack.rb
new file mode 100644
index 000000000..a5906f5db
--- /dev/null
+++ b/lib/puppet/provider/nova_aggregate/openstack.rb
@@ -0,0 +1,107 @@
+require 'puppet/provider/nova'
+
+Puppet::Type.type(:nova_aggregate).provide(
+  :openstack,
+  :parent => Puppet::Provider::Nova
+) do
+  desc <<-EOT
+    Provider to manage nova aggregations
+  EOT
+
+  @credentials = Puppet::Provider::Openstack::CredentialsV2_0.new
+
+  mk_resource_methods
+
+  def self.instances
+    request('aggregate', 'list').collect do |el|
+      attrs = request('aggregate', 'show', el[:name])
+      new(
+          :ensure => :present,
+          :name => attrs[:name],
+          :id => attrs[:id],
+          :availability_zone => attrs[:availability_zone],
+          :metadata => str2hash(attrs[:properties]),
+          :hosts => string2list(attrs[:hosts]).sort
+      )
+    end
+  end
+
+  def self.string2list(input)
+    return input[1..-2].split(",").map { |x| x.match(/'(.*?)'/)[1] }
+  end
+
+  def self.prefetch(resources)
+    instances_ = instances
+    resources.keys.each do |name|
+      if provider = instances_.find{ |instance| instance.name == name }
+        resources[name].provider = provider
+      end
+    end
+  end
+
+  def exists?
+    @property_hash[:ensure] == :present
+  end
+
+  def destroy
+    @property_hash[:hosts].each do |h|
+      properties = [@property_hash[:name], h]
+      self.class.request('aggregate', 'remove host', properties)
+    end
+    self.class.request('aggregate', 'delete', @property_hash[:name])
+  end
+
+  def create
+    properties = [@resource[:name]]
+    if not @resource[:availability_zone].nil? and not @resource[:availability_zone].empty?
+      properties << "--zone" << @resource[:availability_zone]
+    end
+    if not @resource[:metadata].nil? and not @resource[:metadata].empty?
+      @resource[:metadata].each do |key, value|
+        properties << "--property" << "#{key}=#{value}"
+      end
+    end
+    @property_hash = self.class.request('aggregate', 'create', properties)
+
+    if not @resource[:hosts].nil? and not @resource[:hosts].empty?
+      @resource[:hosts].each do |host|
+        properties = [@property_hash[:name], host]
+        self.class.request('aggregate', 'add host', properties)
+      end
+    end
+  end
+
+  def availability_zone=(value)
+    self.class.request('aggregate', 'set', [ @resource[:name], '--zone', @resource[:availability_zone] ])
+  end
+
+  def metadata=(value)
+    # clear obsolete keys
+    # wip untill #1559866
+#   if @property_hash[:metadata].keys.length > 0
+#     properties = [@resource[:name] ]
+#     (@property_hash[:metadata].keys - @resource[:metadata].keys).each do |key|
+#       properties << "--property" << "#{key}"
+#     end
+#     self.class.request('aggregate', 'unset', properties)
+#   end
+    properties = [@resource[:name] ]
+    @resource[:metadata].each do |key, value|
+      properties << "--property" << "#{key}=#{value}"
+    end
+    self.class.request('aggregate', 'set', properties)
+  end
+
+  def hosts=(value)
+    # remove hosts, which are not present in update
+    (@property_hash[:hosts] - @resource[:hosts]).each do |host|
+      properties = [@property_hash[:id], host]
+      self.class.request('aggregate', 'remove host', properties)
+    end
+    # add new hosts
+    (@resource[:hosts] - @property_hash[:hosts]).each do |host|
+      properties = [@property_hash[:id], host]
+      self.class.request('aggregate', 'add host', properties)
+    end
+  end
+end
diff --git a/lib/puppet/type/nova_aggregate.rb b/lib/puppet/type/nova_aggregate.rb
index 28120a4a7..4255a6355 100644
--- a/lib/puppet/type/nova_aggregate.rb
+++ b/lib/puppet/type/nova_aggregate.rb
@@ -45,7 +45,7 @@ Puppet::Type.newtype(:nova_aggregate) do
   ensurable
 
   autorequire(:nova_config) do
-    ['auth_host', 'auth_port', 'auth_protocol', 'admin_tenant_name', 'admin_user', 'admin_password']
+    ['auth_uri', 'admin_tenant_name', 'admin_user', 'admin_password']
   end
 
   newparam(:name, :namevar => true) do
@@ -105,7 +105,11 @@ Puppet::Type.newtype(:nova_aggregate) do
     desc 'Single host or comma seperated list of hosts'
     #convert DSL/string form to internal form
     munge do |value|
-      return value.split(",").map{|el| el.strip()}.sort
+      if value.is_a?(Array)
+        return value
+      else
+        return value.split(",").map{|el| el.strip()}.sort
+      end
     end
   end
 
diff --git a/releasenotes/notes/implement_openstack_client_for_providers-5c83fdf2128197cb.yaml b/releasenotes/notes/implement_openstack_client_for_providers-5c83fdf2128197cb.yaml
new file mode 100644
index 000000000..41227fd48
--- /dev/null
+++ b/releasenotes/notes/implement_openstack_client_for_providers-5c83fdf2128197cb.yaml
@@ -0,0 +1,3 @@
+---
+features:
+  - Using OpenStack client for Nova providers
diff --git a/spec/acceptance/nova_wsgi_apache_spec.rb b/spec/acceptance/nova_wsgi_apache_spec.rb
index accce9cf0..6b6002f4b 100644
--- a/spec/acceptance/nova_wsgi_apache_spec.rb
+++ b/spec/acceptance/nova_wsgi_apache_spec.rb
@@ -66,6 +66,14 @@ describe 'basic nova' do
       }
       class { '::nova::scheduler': }
       class { '::nova::vncproxy': }
+
+      nova_aggregate { 'test_aggregate':
+        ensure            => present,
+        availability_zone => 'zone1',
+        metadata          => 'test=property',
+        require           => Class['nova::api'],
+      }
+
       # TODO: networking with neutron
       EOS
 
@@ -91,5 +99,13 @@ describe 'basic nova' do
       it { is_expected.to have_entry('1 0 * * * nova-manage db archive_deleted_rows --max_rows 100 >>/var/log/nova/nova-rowsflush.log 2>&1').with_user('nova') }
     end
 
+    describe 'nova aggregate' do
+      it 'should create new aggregate' do
+        shell('openstack --os-username nova --os-password a_big_secret --os-tenant-name services --os-auth-url http://127.0.0.1:5000/v2.0 aggregate list') do |r|
+          expect(r.stdout).to match(/test_aggregate/)
+          expect(r.stderr).to be_empty
+        end
+      end
+    end
   end
 end
diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb
index be3811cf2..3345d63ed 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/nova_aggregate/openstack_spec.rb b/spec/unit/provider/nova_aggregate/openstack_spec.rb
new file mode 100644
index 000000000..c09d2fee8
--- /dev/null
+++ b/spec/unit/provider/nova_aggregate/openstack_spec.rb
@@ -0,0 +1,100 @@
+require 'puppet'
+require 'spec_helper'
+require 'puppet/provider/nova_aggregate/openstack'
+
+provider_class = Puppet::Type.type(:nova_aggregate).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/v3'
+  end
+
+  describe 'managing aggregates' do
+
+    let(:aggregate_attrs) do
+      {
+         :name              => 'just',
+         :availability_zone => 'simple',
+         :hosts             => ['example'],
+         :ensure            => 'present',
+         :metadata          => 'nice=cookie',
+      }
+    end
+
+    let(:resource) do
+      Puppet::Type::Nova_aggregate.new(aggregate_attrs)
+    end
+
+    let(:provider) do
+      provider_class.new(resource)
+    end
+
+    it_behaves_like 'authenticated with environment variables' do
+      describe '#instances' do
+        it 'finds existing aggregates' do
+          provider_class.expects(:openstack)
+            .with('aggregate', 'list', '--quiet', '--format', 'csv', [])
+            .returns('"ID","Name","Availability Zone"
+just,"simple","just"
+')
+          provider_class.expects(:openstack)
+            .with('aggregate', 'show', '--format', 'shell', 'simple')
+            .returns('"id="just"
+name="simple"
+availability_zone=just"
+properties="key=\'2value\'"
+hosts="[]"
+')
+          instances = provider_class.instances
+          expect(instances.count).to eq(1)
+          expect(instances[0].name).to eq('simple')
+        end
+      end
+
+      describe '#create' do
+        it 'creates aggregate' do
+          provider.class.stubs(:openstack)
+                        .with('aggregate', 'list', '--quiet', '--format', 'csv', '--long')
+                        .returns('"ID","Name","Availability Zone","Properties"
+')
+          provider.class.stubs(:openstack)
+                  .with('aggregate', 'create', '--format', 'shell', ['just', '--zone', 'simple', '--property', 'nice=cookie' ])
+                  .returns('name="just"
+id="just"
+availability_zone="simple"
+properties="{u\'nice\': u\'cookie\'}"
+hosts="[]"
+')
+          provider.class.stubs(:openstack)
+                  .with('aggregate', 'add host', ['just', 'example'])
+                  .returns('name="just"
+id="just"
+availability_zone="simple"
+properties="{u\'nice\': u\'cookie\'}"
+hosts="[u\'example\']"
+')
+          provider.exists?
+          provider.create
+          expect(provider.exists?).to be_falsey
+        end
+      end
+
+      describe '#destroy' do
+        it 'removes aggregate with hosts' do
+          provider_class.expects(:openstack)
+            .with('aggregate', 'remove host', ['just', 'example'])
+          provider_class.expects(:openstack)
+            .with('aggregate', 'delete', 'just')
+          provider.instance_variable_set(:@property_hash, aggregate_attrs)
+          provider.destroy
+          expect(provider.exists?).to be_falsey
+        end
+      end
+
+    end
+  end
+end
diff --git a/spec/unit/provider/nova_spec.rb b/spec/unit/provider/nova_spec.rb
index 46b0db4fb..0730c977d 100644
--- a/spec/unit/provider/nova_spec.rb
+++ b/spec/unit/provider/nova_spec.rb
@@ -283,149 +283,4 @@ EOT
                      {"Id"=>"api!cell@8", "Name"=>"api!cell@my2", "Availability Zone"=>""}])
     end
   end
-
-  describe 'when handling cli output' do
-    it 'should return the availble Id' do
-      output = <<-EOT
-+----+-------+-------------------+
-| Id | Name  | Availability Zone |
-+----+-------+-------------------+
-| 1  | haha  | haha2             |
-| 2  | haha2 | -                 |
-+----+-------+-------------------+
-      EOT
-      klass.expects(:auth_nova).returns(output)
-      res = klass.nova_aggregate_resources_get_name_by_id("haha2")
-      expect(res).to eql(2)
-    end
-
-    it 'should return nil because given name is not available' do
-      output = <<-EOT
-+----+-------+-------------------+
-| Id | Name  | Availability Zone |
-+----+-------+-------------------+
-| 1  | haha  | haha2             |
-| 2  | haha2 | -                 |
-+----+-------+-------------------+
-      EOT
-      # used the cache copy, don't call nova again
-      klass.expects(:auth_nova).never()
-      res = klass.nova_aggregate_resources_get_name_by_id("notavailable")
-      expect(res).to eql(nil)
-    end
-  end
-
-  describe 'when handling cli output with cells enabled' do
-    it 'should return the availble Id' do
-      output = <<-EOT
-+-------------+----------------+-------------------+
-| Id          | Name           | Availability Zone |
-+-------------+----------------+-------------------+
-| api!cell@1  | api!cell@haha  | haha2             |
-| api!cell@2  | api!cell@haha2 | -                 |
-+-------------+----------------+-------------------+
-      EOT
-      klass.expects(:auth_nova).returns(output)
-      res = klass.nova_aggregate_resources_get_name_by_id("api!cell@haha2", true)
-      expect(res).to eq("api!cell@2")
-    end
-
-    it 'should return nil because given name is not available' do
-      output = <<-EOT
-+----+-------+-------------------+
-| Id | Name  | Availability Zone |
-+----+-------+-------------------+
-| api!cell@1  | api!cell@haha  | haha2             |
-| api!cell@2  | api!cell@haha2 | -                 |
-+----+-------+-------------------+
-      EOT
-      # used the cache copy, don't call nova again
-      klass.expects(:auth_nova).never()
-      res = klass.nova_aggregate_resources_get_name_by_id("notavailable")
-      expect(res).to eql(nil)
-    end
-  end
-
-  describe 'when getting details for given Id' do
-    it 'should return a Hash with the details' do
-      output = <<-EOT
-+----+-------+-------------------+-------+--------------------------------------------------+
-| Id | Name  | Availability Zone | Hosts | Metadata                                         |
-+----+-------+-------------------+-------+--------------------------------------------------+
-| 16 | agg94 |  my_-zone1        |       | 'a=b', 'availability_zone= my_-zone1', 'x_q-r=y' |
-+----+-------+-------------------+-------+--------------------------------------------------+
-        EOT
-      klass.expects(:auth_nova).returns(output)
-      res = klass.nova_aggregate_resources_attr(16)
-      expect(res).to eq({
-        "Id"=>"16",
-        "Name"=>"agg94",
-        "Availability Zone"=>"my_-zone1",
-        "Hosts"=>[],
-        "Metadata"=>{
-          "a"=>"b",
-          "availability_zone"=>" my_-zone1",
-          "x_q-r"=>"y"
-        }
-      })
-    end
-  end
-
-  describe 'when getting details for given Id with cells enabled' do
-    it 'should return a Hash with the details' do
-      output = <<-EOT
-+-------------+----------------+-------------------+-------+--------------------------------------------------+
-| Id          | Name           | Availability Zone | Hosts | Metadata                                         |
-+-------------+----------------+-------------------+-------+--------------------------------------------------+
-| api!cell@16 | api!cell@agg94 |  my_-zone1        |       | 'a=b', 'availability_zone= my_-zone1', 'x_q-r=y' |
-+-------------+----------------+-------------------+-------+--------------------------------------------------+
-        EOT
-      klass.expects(:auth_nova).returns(output)
-      res = klass.nova_aggregate_resources_attr(16)
-      expect(res).to eq({
-        "Id"=>"api!cell@16",
-        "Name"=>"api!cell@agg94",
-        "Availability Zone"=>"my_-zone1",
-        "Hosts"=>[],
-        "Metadata"=>{
-          "a"=>"b",
-          "availability_zone"=>" my_-zone1",
-          "x_q-r"=>"y"
-        }
-      })
-    end
-  end
-
-  describe 'when searching for a host/type combo' do
-    it 'should find the hostname if there is a match' do
-      output = <<-EOT
-+-------------------+-------------+----------+
-| host_name         | service     | zone     |
-+-------------------+-------------+----------+
-| node-control-001  | consoleauth | internal |
-| node-control-001  | cert        | internal |
-| node-compute-002  | compute     | nova     |
-+-------------------+-------------+----------+
-      EOT
-      klass.expects(:auth_nova).returns(output)
-      res = klass.nova_get_host_by_name_and_type("node-compute-002","compute")
-      expect(res).to eq("node-compute-002")
-    end
-
-    it 'should return nil because there is no host/type combo match' do
-      output = <<-EOT
-+-------------------+-------------+----------+
-| host_name         | service     | zone     |
-+-------------------+-------------+----------+
-| node-control-001  | consoleauth | internal |
-| node-control-001  | cert        | internal |
-| node-compute-002  | compute     | nova     |
-+-------------------+-------------+----------+
-      EOT
-      # used the cache copy, don't call nova again
-      klass.expects(:auth_nova).never()
-      res = klass.nova_get_host_by_name_and_type("node-compute-002","internal")
-      expect(res).to eql(nil)
-    end
-  end
 end
diff --git a/spec/unit/type/nova_aggregate_spec.rb b/spec/unit/type/nova_aggregate_spec.rb
new file mode 100644
index 000000000..2492f4ec8
--- /dev/null
+++ b/spec/unit/type/nova_aggregate_spec.rb
@@ -0,0 +1,33 @@
+require 'puppet'
+require 'puppet/type/nova_aggregate'
+
+describe Puppet::Type.type(:nova_aggregate) do
+
+  before :each do
+    Puppet::Type.rmtype(:nova_aggregate)
+  end
+
+  it 'should raise error for setting id property' do
+    incorrect_input = {
+      :name => 'test_type',
+      :id   => 'some_id'
+    }
+    expect { Puppet::Type.type(:nova_aggregate).new(incorrect_input) }.to raise_error(Puppet::ResourceError, /This is a read only property/)
+  end
+
+  it 'should raise error if wrong format of metadata' do
+    incorrect_input = {
+      :name     => 'new_aggr',
+      :metadata => 'some_id,sd'
+    }
+    expect { Puppet::Type.type(:nova_aggregate).new(incorrect_input) }.to raise_error(Puppet::ResourceError, /Key\/value pairs must be separated by an =/)
+  end
+
+  it 'should raise error if wrong type for availability zone' do
+    incorrect_input = {
+      :name => 'new_aggr',
+      :availability_zone => {'zone'=>'23'},
+    }
+    expect { Puppet::Type.type(:nova_aggregate).new(incorrect_input) }.to raise_error(Puppet::ResourceError, /availability zone must be a String/)
+  end
+end