Fix duplicated keystone endpoints

Duplicated endpoints were created when updating 2 or more URLs.

Refactor keystone_endpoint to use prefetch and flush paradigms.

Closes-bug: #1234480
Change-Id: I91340995297c6afaf9c8841bc7ae71a8e25b1eb7
(cherry picked from commit ee7956e)

lib/puppet/provider/keystone_endpoint/keystone.rb

@@ -17,22 +17,34 @@
 
   optional_commands :keystone => "keystone"
 
-  def self.prefetch(resource)
-    # rebuild the cahce for every puppet run
-    @endpoint_hash = nil
+  def initialize(resource = nil)
+    super(resource)
+    @property_flush = {}
   end
 
-  def self.endpoint_hash
-    @endpoint_hash ||= build_endpoint_hash
-  end
-
-  def endpoint_hash
-    self.class.endpoint_hash
+  def self.prefetch(resources)
+    endpoints = instances
+    resources.keys.each do |name|
+      if provider = endpoints.find{ |endpoint| endpoint.name == name }
+        resources[name].provider = provider
+      end
+    end
   end
 
   def self.instances
-    endpoint_hash.collect do |k, v|
-      new(:name => k)
+    list_keystone_objects('endpoint', [5,6]).collect do |endpoint|
+      service_name = get_keystone_object('service', endpoint[5], 'name')
+      new(
+        :name         => "#{endpoint[1]}/#{service_name}",
+        :ensure       => :present,
+        :id           => endpoint[0],
+        :region       => endpoint[1],
+        :public_url   => endpoint[2],
+        :internal_url => endpoint[3],
+        :admin_url    => endpoint[4],
+        :service_id   => endpoint[5],
+        :service_name => service_name
+      )
     end
   end
 
@@ -47,82 +59,67 @@ def create
         optional_opts.push(opt).push(resource[param])
       end
     end
+
     (region, service_name) = resource[:name].split('/')
     resource[:region] = region
     optional_opts.push('--region').push(resource[:region])
-    service_id = self.class.list_keystone_objects('service', 4).detect do |user|
-      user[1] == service_name
+
+    service_id = self.class.list_keystone_objects('service', 4).detect do |s|
+      s[1] == service_name
     end.first
 
-    auth_keystone(
-      'endpoint-create',
-      '--service-id', service_id,
-      optional_opts
-    )
+    auth_keystone('endpoint-create', '--service-id', service_id, optional_opts)
   end
 
   def exists?
-    endpoint_hash[resource[:name]]
+    @property_hash[:ensure] == :present
   end
 
   def destroy
-    auth_keystone('endpoint-delete', endpoint_hash[resource[:name]][:id])
+    auth_keystone('endpoint-delete', @property_hash[:id])
+  end
+
+  def flush
+    if ! @property_flush.empty?
+      destroy
+      create
+      @property_flush.clear
+    end
+    @property_hash = resource.to_hash
   end
 
   def id
-    endpoint_hash[resource[:name]][:id]
+    @property_hash[:id]
   end
 
   def region
-    endpoint_hash[resource[:name]][:region]
+    @property_hash[:region]
   end
 
   def public_url
-    endpoint_hash[resource[:name]][:public_url]
+    @property_hash[:public_url]
   end
 
   def internal_url
-    endpoint_hash[resource[:name]][:internal_url]
+    @property_hash[:internal_url]
   end
 
   def admin_url
-    endpoint_hash[resource[:name]][:admin_url]
+    @property_hash[:admin_url]
   end
 
   def public_url=(value)
-    destroy
-    endpoint_hash[resource[:name]][:public_url] = value
-    create
+    @property_hash[:public_url] = value
+    @property_flush[:public_url] = value
   end
 
   def internal_url=(value)
-    destroy
-    endpoint_hash[resource[:name]][:internal_url] = value
-    create
+    @property_hash[:internal_url] = value
+    @property_flush[:internal_url] = value
   end
 
   def admin_url=(value)
-    destroy
-    endpoint_hash[resource[:name]][:admin_url]
-    create
+    @property_hash[:admin_url] = value
+    @property_flush[:admin_url] = value
   end
-
-  private
-
-    def self.build_endpoint_hash
-      hash = {}
-      list_keystone_objects('endpoint', [5,6]).each do |endpoint|
-        service_name = get_keystone_object('service', endpoint[5], 'name')
-        hash["#{endpoint[1]}/#{service_name}"] = {
-          :id           => endpoint[0],
-          :region       => endpoint[1],
-          :public_url   => endpoint[2],
-          :internal_url => endpoint[3],
-          :admin_url    => endpoint[4],
-          :service_id   => endpoint[5]
-        }
-      end
-      hash
-    end
-
 end

spec/unit/provider/keystone_endpoint/keystone_spec.rb

@@ -2,59 +2,73 @@
 require 'spec_helper'
 require 'puppet/provider/keystone_endpoint/keystone'
 
-provider_class = Puppet::Type.type(:keystone_endpoint).provider(:keystone)
-
-describe provider_class do
-
-  describe 'when updating the endpoint URLS' do
-    let :resource do
-      Puppet::Type::Keystone_endpoint.new(
-        {
-          :name         => 'region/foo',
-          :public_url   => 'public_url',
-          :internal_url => 'internal_url',
-          :admin_url    => 'admin_url'
-        }
-      )
+
+describe Puppet::Type.type(:keystone_endpoint).provider(:keystone) do
+
+  let :resource do
+    Puppet::Type::Keystone_endpoint.new(
+      :provider     => :keystone,
+      :name         => 'region/foo',
+      :ensure       => :present,
+      :public_url   => 'public_url',
+      :internal_url => 'internal_url',
+      :admin_url    => 'admin_url'
+    )
+  end
+
+  let :provider do
+    described_class.new(resource)
+  end
+
+  before :each do
+    # keystone endpoint-list
+    described_class.stubs(:list_keystone_objects).with('endpoint', [5,6]).returns([
+      ['endpoint-id', 'region', 'public_url', 'internal_url', 'admin_url', 4]
+    ])
+    # keystone service-list
+    described_class.stubs(:list_keystone_objects).with('service', 4).returns([
+      [4, 'foo', 'type', 'description']
+    ])
+    described_class.stubs(:get_keystone_object).with('service', 4, 'name').returns('foo')
+    described_class.prefetch('region/foo' => resource)
+  end
+
+  after :each do
+    described_class.prefetch({})
+  end
+
+  describe "self.instances" do
+    it "should have an instances method" do
+      provider.class.should respond_to(:instances)
     end
-    let :provider do
-      provider_class.new(resource)
+
+    it "should list instances" do
+      endpoints = described_class.instances
+      endpoints.size.should == 1
+      endpoints.map {|provider| provider.name} == ['region/foo']
     end
-    before :each do
-      provider_class.expects(:build_endpoint_hash).returns(
-        'region/foo' => {:id   => 'id'}
+  end
+
+  describe '#create' do
+    it 'should call endpoint-create' do
+      provider.expects(:auth_keystone).with(
+        'endpoint-create', '--service-id', 4, includes(
+          '--publicurl', 'public_url', '--internalurl', 'internal_url',
+          '--region', 'region')
       )
+      provider.create
     end
-    after :each do
-      # reset global state
-      provider_class.prefetch(nil)
-    end
-    it 'should delete endpoint for every url that gets synced' do
-      provider.expects(:create).times(3)
-      provider.expects(:auth_keystone).with('endpoint-delete', 'id').times(3)
-      provider.public_url=('public_url')
-      provider.internal_url=('internal_url')
-      provider.admin_url=('admin_url')
-    end
-    it 'should recreate endpoints for every url that gets synced' do
-      provider_class.expects(:list_keystone_objects).with('service', 4).times(3).returns(
-        [['id', 'foo']]
-      )
-      provider.expects(:destroy).times(3)
-      provider.expects(:auth_keystone).with do |a,b,c,d|
-        (
-          a == 'endpoint-create' &&
-          b == '--service-id'       &&
-          c == 'id'              &&
-          d[d.index('--publicurl')   + 1 ] == 'public_url'   &&
-          d[d.index('--adminurl')    + 1 ] ==  'admin_url'   &&
-          d[d.index('--internalurl') + 1 ] == 'internal_url'
-        )
-      end.times(3)
-
-      provider.public_url=('public_url')
-      provider.internal_url=('internal_url')
-      provider.admin_url=('admin_url')
+  end
+
+  describe '#flush' do
+    it 'should delete and create the endpoint once when any url gets updated' do
+      provider.expects(:destroy).times(1)
+      provider.expects(:create).times(1)
+
+      provider.public_url=('new-public_url')
+      provider.internal_url=('new-internal_url')
+      provider.admin_url=('new-admin_url')
+      provider.flush
     end
   end
 end