From 795bb1f60467fcbc56e094bb900a63dd64d8cc5f Mon Sep 17 00:00:00 2001
From: Sofer Athlan-Guyot <chem@sathlan.org>
Date: Wed, 30 Mar 2016 13:00:58 +0200
Subject: [PATCH] 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
(cherry picked from commit 64100bb284dbfb72f4af14eae9665ca042f0239a)
---
 lib/puppet/provider/keystone.rb               |  33 ++++++
 .../provider/keystone_user/openstack.rb       |  57 +++-------
 .../provider/keystone_user_role/openstack.rb  | 102 +++---------------
 ...support_multi_domain-bd04f18aa7913eaa.yaml |  21 ++++
 spec/acceptance/default_domain_spec.rb        |  20 +---
 spec/spec_helper.rb                           |   2 +
 .../provider/keystone_user/openstack_spec.rb  |  86 ++-------------
 .../keystone_user_role/openstack_spec.rb      |  36 -------
 8 files changed, 96 insertions(+), 261 deletions(-)
 create mode 100644 releasenotes/notes/support_multi_domain-bd04f18aa7913eaa.yaml

diff --git a/lib/puppet/provider/keystone.rb b/lib/puppet/provider/keystone.rb
index 8a5ab299c..3bb4251d4 100644
--- a/lib/puppet/provider/keystone.rb
+++ b/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')
diff --git a/lib/puppet/provider/keystone_user/openstack.rb b/lib/puppet/provider/keystone_user/openstack.rb
index 5198daa73..d974181cc 100644
--- a/lib/puppet/provider/keystone_user/openstack.rb
+++ b/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
diff --git a/lib/puppet/provider/keystone_user_role/openstack.rb b/lib/puppet/provider/keystone_user_role/openstack.rb
index 3a66ba512..c96cb4597 100644
--- a/lib/puppet/provider/keystone_user_role/openstack.rb
+++ b/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)
-      # Since requesting every combination of users, roles, and
-      # projects is so expensive, construct the property hash here
-      # instead of in self.instances so it can be used in the role
-      # and destroy methods
-      @property_hash[:name] = resource[:name]
-      if roles_db.empty?
-        @property_hash[:ensure] = :absent
-      else
-        @property_hash[:ensure] = :present
-        @property_hash[:roles]  = roles_db.collect do |role|
-          role[:name]
-        end
+    roles_db = self.class.request('role', 'list', properties)
+    @property_hash[:name] = resource[:name]
+    if roles_db.empty?
+      @property_hash[:ensure] = :absent
+    else
+      @property_hash[:ensure] = :present
+      @property_hash[:roles]  = roles_db.collect do |role|
+        role[:name]
       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)
-    raise(Puppet::Error, "No user #{user} with domain #{user_domain} found") if user_db.nil?
-    user_db[:id]
+    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 id.nil?
+    id
   end
 
   def get_project_id
-    project_db = self.class.fetch_project(project, project_domain)
-    if project_db.nil?
+    id = self.class.project_id_from_name_and_domain_name(project, project_domain)
+    if id.nil?
       raise(Puppet::Error, "No project #{project} with domain #{project_domain} found")
     end
-    project_db[: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
+    id
   end
 end
diff --git a/releasenotes/notes/support_multi_domain-bd04f18aa7913eaa.yaml b/releasenotes/notes/support_multi_domain-bd04f18aa7913eaa.yaml
new file mode 100644
index 000000000..dfd40b660
--- /dev/null
+++ b/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.
diff --git a/spec/acceptance/default_domain_spec.rb b/spec/acceptance/default_domain_spec.rb
index b0d19f2c3..7dead423d 100644
--- a/spec/acceptance/default_domain_spec.rb
+++ b/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 '/,
-                                /Puppet::Type::Keystone_tenant::ProviderOpenstack: Support for a resource without/])
+            .to include_regexp([/Puppet::Type::Keystone_tenant::ProviderOpenstack: Support for a resource without the domain.*using 'Default'.*default domain id is '/])
         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 '/,
-                                /Puppet::Type::Keystone_tenant::ProviderOpenstack: Support for a resource without/])
+            .to include_regexp([/Puppet::Type::Keystone_tenant::ProviderOpenstack: Support for a resource without the domain.*using 'Default'.*default domain id is '/])
         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
diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb
index 35783f650..3c86612cc 100644
--- a/spec/spec_helper.rb
+++ b/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
diff --git a/spec/unit/provider/keystone_user/openstack_spec.rb b/spec/unit/provider/keystone_user/openstack_spec.rb
index e083c3cef..6357ef859 100644
--- a/spec/unit/provider/keystone_user/openstack_spec.rb
+++ b/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
-          provider.exists?
-        end
-
-        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"
+        it 'should detect it' do
+          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
-        # resource_to_name
-        instances = described_class.instances
-        expect(instances.count).to eq(3)
-        expect(instances[0].name).to eq('user1::domain1')
-        expect(instances[0].domain).to eq('domain1')
-        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')
+          described_class.expects(:openstack)
+            .with('user', 'show', '--format', 'shell',
+                  ['user1', '--domain', 'domain1_id'])
+            .returns('')
+          expect(provider.exists?).to be_falsey
+        end
       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
         [
diff --git a/spec/unit/provider/keystone_user_role/openstack_spec.rb b/spec/unit/provider/keystone_user_role/openstack_spec.rb
index dba406a2b..9178b80a5 100644
--- a/spec/unit/provider/keystone_user_role/openstack_spec.rb
+++ b/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
         {