From 74bfe20e1542141e33d61a87f30b8414e25859bb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mathieu=20Gagne=CC=81?= Date: Fri, 18 Oct 2013 13:08:22 -0400 Subject: [PATCH] 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 ee7956e99a2979929fc09446929b60ca246821f6) --- .../provider/keystone_endpoint/keystone.rb | 103 ++++++++-------- .../keystone_endpoint/keystone_spec.rb | 112 ++++++++++-------- 2 files changed, 113 insertions(+), 102 deletions(-) diff --git a/lib/puppet/provider/keystone_endpoint/keystone.rb b/lib/puppet/provider/keystone_endpoint/keystone.rb index ef69879da..c7696c8a6 100644 --- a/lib/puppet/provider/keystone_endpoint/keystone.rb +++ b/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 diff --git a/spec/unit/provider/keystone_endpoint/keystone_spec.rb b/spec/unit/provider/keystone_endpoint/keystone_spec.rb index 4dddb9ac3..41ccefc23 100644 --- a/spec/unit/provider/keystone_endpoint/keystone_spec.rb +++ b/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