Remove user/role prefetch to support multi-domain.

In keystone when the multi-domain configuration is enable, listing all
the user is no longer supported.  You have to specify the domain.  The
rational is that some domain will have LDAP backend (possibly AD) with
tons of users.  Listing them all would not be reliable.

The prefetch feature in puppet needs to know all users and create an
associated object.  This is not a good idea when the number of user is
too high.  Thus the removal of this is necessary.  The rational for
using prefetch is that checking all items in one go "cost" less than
fetching individual information.  As the number of user defined in the
catalog is likely to be less than the number of user in the keystone db,
this seems dubious that this would be case here, hence the removal.

As a consequence the keystone_user_role needs prefetch removal as well.
It actually greatly simplify the code.  A cache is made for user and
project id to minimize the number of requests to the minimum.

Closes-Bug: 1554555
Closes-Bug: 1485508

Depends-On: I5b334e3ffd26df4ba8584d77a5e41b56e73536c8
Change-Id: I8e117a9ddbd2ed5b3df739a0b27a66ad07a33e29

lib/puppet/provider/keystone.rb

@@ -103,6 +103,39 @@ class Puppet::Provider::Keystone < Puppet::Provider::Openstack
     resource_to_name(*name_to_resource(name), false)
   end
 
+#  def self.roles_assignement_for_userid(user_id)
+#    unless @role_assignement_table
+#      @role_assignement_table = request('role assignment', 'list')
+#    end
+#    roles_id = []
+#    @role_assignement_table.each do |row|
+#      roles_id << row[:role] if row[:user] == user_id
+#    end
+#    roles_id
+#  end
+
+  def self.user_id_from_name_and_domain_name(name, domain_name)
+    @users_name ||= {}
+    id_str = "#{name}_#{domain_name}"
+    unless @users_name.keys.include?(id_str)
+      user = fetch_user(name, domain_name)
+      err("Could not find user with name [#{name}] and domain [#{domain_name}]") unless user
+      @users_name[id_str] = user[:id]
+    end
+    @users_name[id_str]
+  end
+
+  def self.project_id_from_name_and_domain_name(name, domain_name)
+    @projects_name ||= {}
+    id_str = "#{name}_#{domain_name}"
+    unless @projects_name.keys.include?(id_str)
+      project = fetch_project(name, domain_name)
+      err("Could not find project with name [#{name}] and domain [#{domain_name}]") unless project
+      @projects_name[id_str] = project[:id]
+    end
+    @projects_name[id_str]
+  end
+
   def self.domain_name_from_id(id)
     unless @domain_hash
       list = request('domain', 'list')

lib/puppet/provider/keystone_user/openstack.rb

@@ -52,9 +52,6 @@ Puppet::Type.type(:keystone_user).provide(
   end
 
   def destroy
-    if self.class.do_not_manage
-      fail("Not managing Keystone_user[#{@resource[:name]}] due to earlier Keystone API failures.")
-    end
     self.class.request('user', 'delete', id)
     @property_hash.clear
   end
@@ -79,25 +76,31 @@ Puppet::Type.type(:keystone_user).provide(
   mk_resource_methods
 
   def exists?
-    @property_hash[:ensure] == :present
+    return true if @property_hash[:ensure] == :present
+    domain_name = self.class.domain_id_from_name(resource[:domain])
+    self.class.request_without_retry do
+      @property_hash =
+        self.class.fetch_user(resource[:name], domain_name)
+      @property_hash ||= {}
+    end
+    # This can happen in bad LDAP mapping
+    @property_hash[:enabled] = 'true' if @property_hash[:enable].nil?
+
+    return false if @property_hash.nil? || @property_hash[:id].nil?
+    true
   end
 
   # Types properties
   def enabled
-    bool_to_sym(@property_hash[:enabled])
+    is_enabled = @property_hash[:enabled].downcase.chomp == 'true' ? true : false
+    bool_to_sym(is_enabled)
   end
 
   def enabled=(value)
-    if self.class.do_not_manage
-      fail("Not managing Keystone_user[#{@resource[:name]}] due to earlier Keystone API failures.")
-    end
     @property_flush[:enabled] = value
   end
 
   def email=(value)
-    if self.class.do_not_manage
-      fail("Not managing Keystone_user[#{@resource[:name]}] due to earlier Keystone API failures.")
-    end
     @property_flush[:email] = value
   end
 
@@ -169,36 +172,4 @@ Puppet::Type.type(:keystone_user).provide(
     @property_hash[:domain_id]
   end
 
-  def self.instances
-    if default_domain_changed
-      warning(default_domain_deprecation_message)
-    end
-    self.do_not_manage = true
-    users = request('user', 'list', ['--long'])
-    list = users.collect do |user|
-      domain_name = domain_name_from_id(user[:domain])
-      new(
-        :name        => resource_to_name(domain_name, user[:name]),
-        :ensure      => :present,
-        :enabled     => user[:enabled].downcase.chomp == 'true' ? true : false,
-        :password    => user[:password],
-        :email       => user[:email],
-        :description => user[:description],
-        :domain      => domain_name,
-        :domain_id   => user[:domain],
-        :id          => user[:id]
-      )
-    end
-    self.do_not_manage = false
-    list
-  end
-
-  def self.prefetch(resources)
-    prefetch_composite(resources) do |sorted_namevars|
-      domain = sorted_namevars[0]
-      name   = sorted_namevars[1]
-      resource_to_name(domain, name)
-    end
-  end
-
 end

lib/puppet/provider/keystone_user_role/openstack.rb

@@ -45,20 +45,14 @@ Puppet::Type.type(:keystone_user_role).provide(
   end
 
   def exists?
-    if self.class.user_role_hash.nil? || self.class.user_role_hash.empty?
+    roles_db = self.class.request('role', 'list', properties)
-      roles_db = self.class.request('role', 'list', properties)
+    @property_hash[:name] = resource[:name]
-      # Since requesting every combination of users, roles, and
+    if roles_db.empty?
-      # projects is so expensive, construct the property hash here
+      @property_hash[:ensure] = :absent
-      # instead of in self.instances so it can be used in the role
+    else
-      # and destroy methods
+      @property_hash[:ensure] = :present
-      @property_hash[:name] = resource[:name]
+      @property_hash[:roles]  = roles_db.collect do |role|
-      if roles_db.empty?
+        role[:name]
-        @property_hash[:ensure] = :absent
-      else
-        @property_hash[:ensure] = :present
-        @property_hash[:roles]  = roles_db.collect do |role|
-          role[:name]
-        end
       end
     end
     return @property_hash[:ensure] == :present
@@ -86,20 +80,6 @@ Puppet::Type.type(:keystone_user_role).provide(
     end
   end
 
-  def self.instances
-    if default_domain_changed
-      warning(default_domain_deprecation_message)
-    end
-    instances = build_user_role_hash
-    instances.collect do |title, roles|
-      new({
-        :name   => title,
-        :ensure => :present,
-        :roles  => roles
-      }.merge(@user_role_parameters[title]))
-    end
-  end
-
   private
 
   def properties
@@ -117,70 +97,16 @@ Puppet::Type.type(:keystone_user_role).provide(
   end
 
   def get_user_id
-    user_db = self.class.fetch_user(user, user_domain)
+    id = self.class.user_id_from_name_and_domain_name(user, user_domain)
-    raise(Puppet::Error, "No user #{user} with domain #{user_domain} found") if user_db.nil?
+    raise(Puppet::Error, "No user #{user} with domain #{user_domain} found") if id.nil?
-    user_db[:id]
+    id
   end
 
   def get_project_id
-    project_db = self.class.fetch_project(project, project_domain)
+    id = self.class.project_id_from_name_and_domain_name(project, project_domain)
-    if project_db.nil?
+    if id.nil?
       raise(Puppet::Error, "No project #{project} with domain #{project_domain} found")
     end
-    project_db[:id]
+    id
-  end
-
-  def self.user_role_hash
-    @user_role_hash
-  end
-
-  def self.set_user_role_hash(user_role_hash)
-    @user_role_hash = user_role_hash
-  end
-
-  def self.build_user_role_hash
-    self.do_not_manage = true
-    # The new hash will have the property that if the
-    # given key does not exist, create it with an empty
-    # array as the value for the hash key
-    hash = @user_role_hash || Hash.new{|h,k| h[k] = []}
-    @user_role_parameters = {}
-    return hash unless hash.empty?
-    # Need a mapping of project id to names.
-    project_hash = {}
-    Puppet::Type.type(:keystone_tenant).provider(:openstack).instances.each do |project|
-      project_hash[project.id] = project.name
-    end
-    # Need a mapping of user id to names.
-    user_hash = {}
-    Puppet::Type.type(:keystone_user).provider(:openstack).instances.each do |user|
-      user_hash[user.id] = user.name
-    end
-    # need a mapping of role id to name
-    role_hash = {}
-    request('role', 'list').each {|role| role_hash[role[:id]] = role[:name]}
-    # now, get all role assignments
-    request('role assignment', 'list').each do |assignment|
-      if assignment[:user]
-        user_str = user_hash[assignment[:user]]
-        if assignment[:project] && !assignment[:project].empty?
-          project_str = project_hash[assignment[:project]]
-          name = "#{user_str}@#{project_str}"
-          @user_role_parameters[name] = Hash[
-            [:user_domain, :user, :project_domain, :project]
-              .zip(name_to_resource(user_str) + name_to_resource(project_str))]
-        else
-          domainname = domain_name_from_id(assignment[:domain])
-          name = "#{user_hash[assignment[:user]]}@::#{domainname}"
-          @user_role_parameters[name] = Hash[
-            [:user_domain, :user, :domain]
-              .zip(name_to_resource(user_str) + [domainname])]
-        end
-        hash[name] << role_hash[assignment[:role]]
-      end
-    end
-    set_user_role_hash(hash)
-    self.do_not_manage = false
-    hash
   end
 end

releasenotes/notes/support_multi_domain-bd04f18aa7913eaa.yaml

@@ -0,0 +1,21 @@
+---
+prelude: >
+    Support for multi-domain has been added.  You can configure LDAP
+    identity drivers along with the sql, and have multi-domain
+    working.
+features:
+  - Support for multi-domain;
+  - Remove prefetch in keystone_user/keystone_user_role
+upgrade:
+  - The prefetch and associated instances class function removal
+    could impact users that somehow use the command `puppet resource
+    keystone_user` or `puppet resource keystone_user_role` in
+    production.  Those commands won't work anymore.  Directly use
+    the associated `openstack` commands to get the same effect.
+fixes:
+  - Fixes `bug 1554555
+    <https://bugs.launchpad.net/puppet-keystone/+bug/1554555>`__ so
+    openstack cli provider needs to pass domain in v3 calls
+  - Fixes `bug 1485508
+    <https://bugs.launchpad.net/puppet-keystone/+bug/1485508>`__ so
+    when domain_specific_drivers_enabled=True keystone_user provider fails.

spec/acceptance/default_domain_spec.rb

@@ -60,15 +60,13 @@ describe 'basic keystone server with changed domain id' do
       it 'should work with no errors and catch deprecation warning' do
         apply_manifest(pp, :catch_failures => true) do |result|
           expect(result.stderr)
-            .to include_regexp([/Puppet::Type::Keystone_user::ProviderOpenstack: Support for a resource without the domain.*using 'Default'.*default domain id is '/,
+            .to include_regexp([/Puppet::Type::Keystone_tenant::ProviderOpenstack: Support for a resource without the domain.*using 'Default'.*default domain id is '/])
-                                /Puppet::Type::Keystone_tenant::ProviderOpenstack: Support for a resource without/])
         end
       end
       it 'should be idempotent' do
         apply_manifest(pp, :catch_changes => true) do |result|
           expect(result.stderr)
-            .to include_regexp([/Puppet::Type::Keystone_user::ProviderOpenstack: Support for a resource without the domain.*using 'Default'.*default domain id is '/,
+            .to include_regexp([/Puppet::Type::Keystone_tenant::ProviderOpenstack: Support for a resource without the domain.*using 'Default'.*default domain id is '/])
-                                /Puppet::Type::Keystone_tenant::ProviderOpenstack: Support for a resource without/])
         end
       end
     end
@@ -80,20 +78,6 @@ describe 'basic keystone server with changed domain id' do
                                 /keystone_tenant { 'project_in_my_default_domain::other_domain':/])
         end
       end
-      it 'for user' do
-        shell('puppet resource keystone_user') do |result|
-          expect(result.stdout)
-            .to include_regexp([/keystone_user { 'user_in_my_default_domain':/,
-                                /keystone_user { 'user_in_my_default_domain::other_domain':/])
-        end
-      end
-      it 'for role' do
-        shell('puppet resource keystone_user_role') do |result|
-          expect(result.stdout)
-            .to include_regexp([/keystone_user_role { 'user_in_my_default_domain@project_in_my_default_domain':/,
-                                /keystone_user_role { 'user_in_my_default_domain::other_domain@::other_domain':/])
-        end
-      end
     end
   end
 end

spec/spec_helper.rb

@@ -38,6 +38,8 @@ def setup_provider_tests
       @keystone_file  = nil
       Puppet::Provider::Keystone.class_variable_set('@@default_domain_id', nil)
       @domain_hash = nil
+      @users_name  = nil
+      @projects_name = nil
     end
   end
 end

spec/unit/provider/keystone_user/openstack_spec.rb

@@ -62,70 +62,29 @@ username="user1"
         described_class.expects(:openstack)
           .with('user', 'delete', 'my-user-id')
         provider.destroy
-        expect(provider.exists?).to be_falsey
       end
     end
 
     describe '#exists' do
       context 'when user does not exist' do
-        subject(:response) do
+        it 'should detect it' do
-          provider.exists?
+          described_class.expects(:openstack)
-        end
+            .with('domain', 'list', '--quiet', '--format', 'csv', [])
-
+            .returns('"ID","Name","Enabled","Description"
-        it { is_expected.to be_falsey }
-      end
-    end
-
-    describe '#instances' do
-      it 'finds every user' do
-        described_class.expects(:openstack)
-          .with('user', 'list', '--quiet', '--format', 'csv', ['--long'])
-          .returns('"ID","Name","Project Id","Domain","Description","Email","Enabled"
-"user1_id","user1","project1_id","domain1_id","user1 description","user1@example.com",True
-"user2_id","user2","project2_id","domain2_id","user2 description","user2@example.com",True
-"user3_id","user3","project3_id","domain3_id","user3 description","user3@example.com",True
-')
-        described_class.expects(:openstack)
-          .with('domain', 'list', '--quiet', '--format', 'csv', [])
-          .returns('"ID","Name","Enabled","Description"
 "default","Default",True,"default"
 "domain1_id","domain1",True,"domain1"
 "domain2_id","domain2",True,"domain2"
 "domain3_id","domain3",True,"domain3"
 ')
-        # for self.instances to create the name string in
+          described_class.expects(:openstack)
-        # resource_to_name
+            .with('user', 'show', '--format', 'shell',
-        instances = described_class.instances
+                  ['user1', '--domain', 'domain1_id'])
-        expect(instances.count).to eq(3)
+            .returns('')
-        expect(instances[0].name).to eq('user1::domain1')
+          expect(provider.exists?).to be_falsey
-        expect(instances[0].domain).to eq('domain1')
+        end
-        expect(instances[1].name).to eq('user2::domain2')
-        expect(instances[1].domain).to eq('domain2')
-        expect(instances[2].name).to eq('user3::domain3')
-        expect(instances[2].domain).to eq('domain3')
       end
     end
 
-    describe '#prefetch' do
-      let(:resources) do
-        [Puppet::Type.type(:keystone_user).new(:title => 'exists', :ensure => :present),
-          Puppet::Type.type(:keystone_user).new(:title => 'non_exists', :ensure => :present)]
-      end
-      before(:each) do
-        described_class.expects(:domain_name_from_id).with('default')
-          .returns('Default')
-        described_class.expects(:domain_name_from_id).with('domain2_id')
-          .returns('bar')
-        described_class.expects(:openstack)
-          .with('user', 'list', '--quiet', '--format', 'csv', ['--long'])
-          .returns('"ID","Name","Project Id","Domain","Description","Email","Enabled"
-"user1_id","exists","project1_id","default","user1 description","user1@example.com",True
-"user2_id","user2","project2_id","domain2_id","user2 description","user2@example.com",True
-')
-      end
-      include_examples 'prefetch the resources'
-    end
-
     describe '#flush' do
       context '.enable' do
         describe '-> false' do
@@ -367,31 +326,6 @@ username="user1"
       end
     end
 
-    describe '#prefetch' do
-      let(:resources) do
-        [
-          Puppet::Type.type(:keystone_user)
-            .new(:title => 'exists::domain1', :ensure => :present),
-          Puppet::Type.type(:keystone_user)
-            .new(:title => 'non_exists::domain1', :ensure => :present)
-        ]
-      end
-      before(:each) do
-        # Used to make the final display name
-        described_class.expects(:domain_name_from_id)
-          .with('domain1_id').returns('domain1')
-        described_class.expects(:domain_name_from_id)
-          .with('domain2_id').returns('bar')
-        described_class.expects(:openstack)
-          .with('user', 'list', '--quiet', '--format', 'csv', ['--long'])
-          .returns('"ID","Name","Project Id","Domain","Description","Email","Enabled"
-"user1_id","exists","project1_id","domain1_id","user1 description","user1@example.com",True
-"user2_id","user2","project2_id","domain2_id","user2 description","user2@example.com",True
-')
-      end
-      include_examples 'prefetch the resources'
-    end
-
     context 'different name, identical resource' do
       let(:resources) do
         [

spec/unit/provider/keystone_user_role/openstack_spec.rb

@@ -160,42 +160,6 @@ id="user1_id"
       it { is_expected.to be_truthy }
     end
 
-    describe '#instances' do
-      it 'finds every user role' do
-        project_class = Puppet::Type.type(:keystone_tenant).provider(:openstack)
-        user_class = Puppet::Type.type(:keystone_user).provider(:openstack)
-
-        usermock = user_class.new(:id => 'user1_id', :name => 'user1')
-        user_class.expects(:instances).with(any_parameters).returns([usermock])
-
-        projectmock = project_class.new(:id => 'project1_id', :name => 'project1')
-        # 2 for tenant and user and 2 for user_role
-        project_class.expects(:instances).with(any_parameters).returns([projectmock])
-
-        described_class.expects(:openstack)
-          .with('role', 'list', '--quiet', '--format', 'csv', [])
-          .returns('"ID","Name"
-"role1-id","role1"
-"role2-id","role2"
-')
-        described_class.expects(:openstack)
-          .with('role assignment', 'list', '--quiet', '--format', 'csv', [])
-          .returns('
-"Role","User","Group","Project","Domain"
-"role1-id","user1_id","","project1_id","Default"
-"role2-id","user1_id","","project1_id","Default"
-')
-        instances = described_class.instances
-        expect(instances.count).to eq(1)
-        expect(instances[0].name).to eq('user1@project1')
-        expect(instances[0].roles).to eq(['role1', 'role2'])
-        expect(instances[0].user).to eq('user1')
-        expect(instances[0].user_domain).to eq('Default')
-        expect(instances[0].project).to eq('project1')
-        expect(instances[0].project_domain).to eq('Default')
-      end
-    end
-
     describe '#roles=' do
       let(:resource_attrs) do
         {