From 42add12c9e5dcd6a47679fa4c29bfc5797ffad2e Mon Sep 17 00:00:00 2001 From: Takashi Kajinami Date: Thu, 22 Jun 2023 19:45:21 +0900 Subject: [PATCH] Remove support for creating endpoints without service type Creating endpoint without service type was deprecated multiple cycles ago. This removes the logic to support that old usage. Change-Id: Ifaebb3658254bb91130807153624480df78443aa --- .../provider/keystone_endpoint/openstack.rb | 49 +------ lib/puppet/type/keystone_endpoint.rb | 22 +-- manifests/resource/service_identity.pp | 33 ++--- ...ndpoint-require-type-4fa889f6788d8312.yaml | 4 + ...keystone_resource_service_identity_spec.rb | 10 +- .../keystone_endpoint/openstack_spec.rb | 134 ++++++------------ spec/unit/type/keystone_endpoint_spec.rb | 22 --- 7 files changed, 62 insertions(+), 212 deletions(-) create mode 100644 releasenotes/notes/endpoint-require-type-4fa889f6788d8312.yaml diff --git a/lib/puppet/provider/keystone_endpoint/openstack.rb b/lib/puppet/provider/keystone_endpoint/openstack.rb index d7e4dcfca..93022fb70 100644 --- a/lib/puppet/provider/keystone_endpoint/openstack.rb +++ b/lib/puppet/provider/keystone_endpoint/openstack.rb @@ -36,7 +36,6 @@ Puppet::Type.type(:keystone_endpoint).provide( name = resource[:name] region = resource[:region] type = resource[:type] - type = self.class.type_from_service(name) unless set?(:type) @property_hash[:type] = type ids = [] s_id = service_id @@ -200,54 +199,8 @@ Puppet::Type.type(:keystone_endpoint).provide( @services = value end - def self.endpoint_from_region_name(region, name) - endpoints.find_all { |e| e[:region] == region && e[:service_name] == name } - .map { |e| e[:service_type] }.uniq - end - - def self.type_from_service(name) - types = services.find_all { |s| s[:name] == name }.map { |e| e[:type] }.uniq - if types.count == 1 - types[0] - else - # We don't fail here as it can happen during a ensure => absent. - PuppetX::Keystone::CompositeNamevar::Unset - end - end - - def self.service_type(services, region, name) - nbr_of_services = services.count - err_msg = ["endpoint matching #{region}/#{name}:"] - type = nil - - case - when nbr_of_services == 1 - type = services[0] - when nbr_of_services > 1 - err_msg += [endpoint_from_region_name(region, name).join(' ')] - when nbr_of_services < 1 - # Then we try to get the type by service name. - type = type_from_service(name) - end - - if !type.nil? - type - else - fail(Puppet::Error, 'Cannot get the correct endpoint type: ' \ - "#{err_msg.join(' ')}") - end - end - def self.transform_name(region, name, type) - if type == PuppetX::Keystone::CompositeNamevar::Unset - type = service_type(endpoint_from_region_name(region, name), region, name) - end - if type == PuppetX::Keystone::CompositeNamevar::Unset - Puppet.debug("Could not find the type for endpoint #{region}/#{name}") - "#{region}/#{name}" - else - "#{region}/#{name}::#{type}" - end + "#{region}/#{name}::#{type}" end def self.make_id(endpoint) diff --git a/lib/puppet/type/keystone_endpoint.rb b/lib/puppet/type/keystone_endpoint.rb index 8566c3254..4d858aea0 100644 --- a/lib/puppet/type/keystone_endpoint.rb +++ b/lib/puppet/type/keystone_endpoint.rb @@ -24,13 +24,7 @@ Puppet::Type.newtype(:keystone_endpoint) do newparam(:type) do isnamevar - defaultto do - deprecation_msg = 'Support for a endpoint without the type ' \ - 'set is deprecated in Liberty. ' \ - 'It will be dropped in Mitaka.' - warning(deprecation_msg) - PuppetX::Keystone::CompositeNamevar::Unset - end + include PuppetX::Keystone::Type::Required end newproperty(:public_url) @@ -45,19 +39,7 @@ Puppet::Type.newtype(:keystone_endpoint) do end autorequire(:keystone_service) do - if parameter_set?(:type) - "#{name}::#{self[:type]}" - else - title = catalog.resources - .find_all { |e| e.type == :keystone_service && e[:name] == name } - .map { |e| e.title }.uniq - if title.count == 1 - title - else - warning("Couldn't find the type of the domain to require using #{name}") - name - end - end + "#{name}::#{self[:type]}" end def self.title_patterns diff --git a/manifests/resource/service_identity.pp b/manifests/resource/service_identity.pp index 58ff3e5d8..3e7ac434b 100644 --- a/manifests/resource/service_identity.pp +++ b/manifests/resource/service_identity.pp @@ -214,29 +214,18 @@ define keystone::resource::service_identity( } if $configure_endpoint { - if $service_type { - if $public_url and $admin_url and $internal_url { - ensure_resource('keystone_endpoint', "${region}/${service_name_real}::${service_type}", { - 'ensure' => $ensure, - 'public_url' => $public_url, - 'admin_url' => $admin_url, - 'internal_url' => $internal_url, - }) - } else { - fail ('When configuring an endpoint, you need to set the _url parameters.') - } + if ! $service_type { + fail('When configuring an endpoint, you need to set the service_type parameter.') + } + if $public_url and $admin_url and $internal_url { + ensure_resource('keystone_endpoint', "${region}/${service_name_real}::${service_type}", { + 'ensure' => $ensure, + 'public_url' => $public_url, + 'admin_url' => $admin_url, + 'internal_url' => $internal_url, + }) } else { - if $public_url and $admin_url and $internal_url { - ensure_resource('keystone_endpoint', "${region}/${service_name_real}", { - 'ensure' => $ensure, - 'public_url' => $public_url, - 'admin_url' => $admin_url, - 'internal_url' => $internal_url, - }) - } else { - fail ('When configuring an endpoint, you need to set the _url parameters.') - } - warning('Defining a endpoint without the type is supported in Liberty and will be dropped in Mitaka. See https://bugs.launchpad.net/puppet-keystone/+bug/1506996') + fail ('When configuring an endpoint, you need to set the _url parameters.') } } } diff --git a/releasenotes/notes/endpoint-require-type-4fa889f6788d8312.yaml b/releasenotes/notes/endpoint-require-type-4fa889f6788d8312.yaml new file mode 100644 index 000000000..79dcc1291 --- /dev/null +++ b/releasenotes/notes/endpoint-require-type-4fa889f6788d8312.yaml @@ -0,0 +1,4 @@ +--- +upgrade: + - | + The ``keystone_endpoint`` resource type now requires the ``type`` property. diff --git a/spec/defines/keystone_resource_service_identity_spec.rb b/spec/defines/keystone_resource_service_identity_spec.rb index e6c4b7a24..ede67dea8 100644 --- a/spec/defines/keystone_resource_service_identity_spec.rb +++ b/spec/defines/keystone_resource_service_identity_spec.rb @@ -116,19 +116,15 @@ describe 'keystone::resource::service_identity' do )} end - context 'when trying to create an endpoint without service_type (will be dropped in Mitaka)' do + context 'when trying to create an endpoint without service_type' do let :params do required_params.delete(:service_type) required_params.merge( :configure_service => false, ) end - it { is_expected.to contain_keystone_endpoint("RegionOne/#{title}").with( - :ensure => 'present', - :public_url => 'http://7.7.7.7:9696', - :internal_url => 'http://10.0.0.1:9696', - :admin_url => 'http://192.168.0.1:9696', - )} + + it { is_expected.to raise_error(Puppet::Error) } end context 'when trying to create a service without service_type' do diff --git a/spec/unit/provider/keystone_endpoint/openstack_spec.rb b/spec/unit/provider/keystone_endpoint/openstack_spec.rb index 302dc9932..46898bd44 100644 --- a/spec/unit/provider/keystone_endpoint/openstack_spec.rb +++ b/spec/unit/provider/keystone_endpoint/openstack_spec.rb @@ -19,7 +19,8 @@ describe Puppet::Type.type(:keystone_endpoint).provider(:openstack) do :ensure => 'present', :public_url => 'http://127.0.0.1:5000', :internal_url => 'http://127.0.0.1:5001', - :admin_url => 'http://127.0.0.1:5002' + :admin_url => 'http://127.0.0.1:5002', + :type => 'type_one', } end @@ -66,25 +67,8 @@ region="region" "service_id1","endpoint","type_one" ') end - context 'without the type' do - it 'creates an endpoint' do - provider.create - expect(provider.exists?).to be_truthy - expect(provider.id).to eq('endpoint1_id,endpoint2_id,endpoint3_id') - end - end - context 'with the type' do - let(:endpoint_attrs) do - { - :title => 'region/endpoint', - :ensure => 'present', - :public_url => 'http://127.0.0.1:5000', - :internal_url => 'http://127.0.0.1:5001', - :admin_url => 'http://127.0.0.1:5002', - :type => 'type_one' - } - end + context 'with required parameters' do it 'creates an endpoint' do provider.create expect(provider.exists?).to be_truthy @@ -160,55 +144,70 @@ region="region" ') instances = described_class.instances expect(instances).to have_array_of_instances_hash([ - {:name=>"RegionOne/keystone::identity", + { + :name=>"RegionOne/keystone::identity", :ensure=>:present, :id=>"endpoint1_id,endpoint2_id,endpoint3_id", :region=>"RegionOne", :admin_url=>"http://One-127.0.0.1:5002", :internal_url=>"https://One-127.0.0.1:5001", - :public_url=>"https://One-127.0.0.1:5000"}, - {:name=>"RegionTwo/keystone::identity", + :public_url=>"https://One-127.0.0.1:5000" + }, + { + :name=>"RegionTwo/keystone::identity", :ensure=>:present, :id=>"endpoint4_id,endpoint5_id,endpoint6_id", :region=>"RegionTwo", :admin_url=>"http://Two-127.0.0.1:5002", :internal_url=>"https://Two-127.0.0.1:5001", - :public_url=>"https://Two-127.0.0.1:5000"}, - {:name=>"RegionThree/keystone::identity", + :public_url=>"https://Two-127.0.0.1:5000" + }, + { + :name=>"RegionThree/keystone::identity", :ensure=>:present, :id=>"endpoint7_id,endpoint8_id,endpoint9_id", :region=>"RegionThree", :admin_url=>"http://Three-127.0.0.1:5002", :internal_url=>"https://Three-127.0.0.1:5001", - :public_url=>"https://Three-127.0.0.1:5000"}, - {:name=>"RegionFour/keystone::identity", + :public_url=>"https://Three-127.0.0.1:5000" + }, + { + :name=>"RegionFour/keystone::identity", :ensure=>:present, :id=>"endpoint10_id,endpoint11_id,endpoint12_id", :region=>"RegionFour", :admin_url=>"http://Four-127.0.0.1:5002", :internal_url=>"https://Four-127.0.0.1:5001", - :public_url=>"https://Four-127.0.0.1:5000"}, - {:name=>"RegionFive/keystone::identity", + :public_url=>"https://Four-127.0.0.1:5000" + }, + { + :name=>"RegionFive/keystone::identity", :ensure=>:present, :id=>"endpoint13_id,endpoint14_id,endpoint15_id", :region=>"RegionFive", :admin_url=>"http://Five-127.0.0.1:5002", :internal_url=>"https://Five-127.0.0.1:5001", - :public_url=>"https://Five-127.0.0.1:5000"}, - {:name=>"RegionSix/keystone::identity", + :public_url=>"https://Five-127.0.0.1:5000" + }, + { + :name=>"RegionSix/keystone::identity", :ensure=>:present, :id=>"endpoint16_id,endpoint17_id,endpoint18_id", :region=>"RegionSix", :admin_url=>"http://Six-127.0.0.1:5002", :internal_url=>"https://Six-127.0.0.1:5001", - :public_url=>"https://Six-127.0.0.1:5000"}, - {:name=>"RegionSeven/keystone::identity", + :public_url=>"https://Six-127.0.0.1:5000" + }, + { + :name=>"RegionSeven/keystone::identity", :ensure=>:present, :id=>"endpoint19_id,endpoint20_id,endpoint21_id", :region=>"RegionSeven", :admin_url=>"http://Seven-127.0.0.1:5002", :internal_url=>"https://Seven-127.0.0.1:5001", - :public_url=>"https://Seven-127.0.0.1:5000"}]) + :public_url=>"https://Seven-127.0.0.1:5000" + } + ]) end end end @@ -226,68 +225,22 @@ region="region" end context '#fq resource in title' do let(:resources) do - [Puppet::Type.type(:keystone_endpoint).new(:title => 'RegionOne/keystone::identity', :ensure => :present), - Puppet::Type.type(:keystone_endpoint).new(:title => 'RegionOne/keystone::identityv3', :ensure => :present)] + [ + Puppet::Type.type(:keystone_endpoint).new(:title => 'RegionOne/keystone::identity', :ensure => :present), + Puppet::Type.type(:keystone_endpoint).new(:title => 'RegionOne/keystone::identityv3', :ensure => :present) + ] end include_examples 'prefetch the resources' end context '#fq resource' do let(:resources) do - [Puppet::Type.type(:keystone_endpoint).new(:title => 'keystone', :region => 'RegionOne', :type => 'identity', :ensure => :present), - Puppet::Type.type(:keystone_endpoint).new(:title => 'RegionOne/keystone::identityv3', :ensure => :present)] + [ + Puppet::Type.type(:keystone_endpoint).new(:title => 'keystone', :region => 'RegionOne', :type => 'identity', :ensure => :present), + Puppet::Type.type(:keystone_endpoint).new(:title => 'keystone', :region => 'RegionOne', :type => 'identityv3', :ensure => :present) + ] end include_examples 'prefetch the resources' end - context '#nfq resource in title matching existing endpoint' do - let(:resources) do - [Puppet::Type.type(:keystone_endpoint).new(:title => 'RegionOne/keystone', :ensure => :present), - Puppet::Type.type(:keystone_endpoint).new(:title => 'RegionOne/keystone::identityv3', :ensure => :present)] - end - include_examples 'prefetch the resources' - end - context '#nfq resource matching existing endpoint' do - let(:resources) do - [Puppet::Type.type(:keystone_endpoint).new(:title => 'keystone', :region => 'RegionOne', :ensure => :present), - Puppet::Type.type(:keystone_endpoint).new(:title => 'RegionOne/keystone::identityv3', :ensure => :present)] - end - include_examples 'prefetch the resources' - end - end - - context 'not working' do - context 'too many type' do - before(:each) do - expect(described_class).to receive(:openstack) - .with('endpoint', 'list', '--quiet', '--format', 'csv', []) - .and_return('"ID","Region","Service Name","Service Type","Enabled","Interface","URL" -"endpoint1_id","RegionOne","keystone","identity",True,"admin","http://127.0.0.1:5002" -"endpoint2_id","RegionOne","keystone","identity",True,"internal","https://127.0.0.1:5001" -"endpoint3_id","RegionOne","keystone","identity",True,"public","https://127.0.0.1:5000" -"endpoint4_id","RegionOne","keystone","identityv3",True,"admin","http://127.0.0.1:5002" -"endpoint5_id","RegionOne","keystone","identityv3",True,"internal","https://127.0.0.1:5001" -"endpoint6_id","RegionOne","keystone","identityv3",True,"public","https://127.0.0.1:5000" -') - end - it "should fail as it's not possible to get the right type here" do - existing = Puppet::Type.type(:keystone_endpoint) - .new(:title => 'RegionOne/keystone', :ensure => :present) - resource = double - r = [] - r << existing - - catalog = Puppet::Resource::Catalog.new - r.each { |res| catalog.add_resource(res) } - m_value = double - m_first = double - expect(resource).to receive(:values).and_return(m_value) - expect(m_value).to receive(:first).and_return(m_first) - expect(m_first).to receive(:catalog).and_return(catalog) - expect(m_first).to receive(:class).and_return(described_class.resource_type) - expect { described_class.prefetch(resource) } - .to raise_error(Puppet::Error, - /endpoint matching RegionOne\/keystone: identity identityv3/) - end - end end context 'not any type but existing service' do @@ -298,16 +251,11 @@ region="region" "endpoint1_id","RegionOne","keystone","identity",True,"admin","http://127.0.0.1:5002" "endpoint2_id","RegionOne","keystone","identity",True,"internal","https://127.0.0.1:5001" "endpoint3_id","RegionOne","keystone","identity",True,"public","https://127.0.0.1:5000" -') - expect(described_class).to receive(:openstack) - .with('service', 'list', '--quiet', '--format', 'csv', []) - .and_return('"ID","Name","Type" -"service1_id","keystonev3","identity" ') end it 'should be successful' do existing = Puppet::Type.type(:keystone_endpoint) - .new(:title => 'RegionOne/keystonev3', :ensure => :present) + .new(:title => 'RegionOne/keystonev3::identity', :ensure => :present) resource = double r = [] r << existing diff --git a/spec/unit/type/keystone_endpoint_spec.rb b/spec/unit/type/keystone_endpoint_spec.rb index d13d0bb41..290e51e49 100644 --- a/spec/unit/type/keystone_endpoint_spec.rb +++ b/spec/unit/type/keystone_endpoint_spec.rb @@ -25,10 +25,6 @@ describe Puppet::Type.type(:keystone_endpoint) do Puppet::Type.type(:keystone_service).new(:title => 'service_one::type_two') end - let(:service_three) do - Puppet::Type.type(:keystone_service).new(:title => 'service_two::type_one') - end - context 'domain autorequire from title' do let(:endpoint) do Puppet::Type.type(:keystone_endpoint).new(:title => 'region_one/service_one::type_one') @@ -38,23 +34,5 @@ describe Puppet::Type.type(:keystone_endpoint) do include_examples 'autorequire the correct resources' end end - context 'domain autorequire from title without type (to be removed at Mitaka)' do - let(:endpoint) do - Puppet::Type.type(:keystone_endpoint).new(:title => 'region_one/service_one') - end - describe 'should require the correct domain' do - let(:resources) { [endpoint, service_one, service_two] } - include_examples 'autorequire the correct resources' - end - end - context 'domain autorequire from title without type on fq service name (to be removed at Mitaka)' do - let(:endpoint) do - Puppet::Type.type(:keystone_endpoint).new(:title => 'region_one/service_two') - end - describe 'should require the correct domain' do - let(:resources) { [endpoint, service_three, service_one] } - include_examples 'autorequire the correct resources' - end - end end end