Merge branch 'single_group_sync' into 'master'

Allow syncing a group against all providers at once

Allows `Sync::Group` to sync members for a single group across all providers at once. Closes #399

No

The current implementation of LDAP group sync manages members based on a single LDAP provider. This is optimal because we can open a single connection to a given LDAP server (provider) and run all queries needed to sync all GitLab groups with links to that provider. If we want to sync a single group at a time we need to flip that model - for a single group, run the sync for all providers.

This presents a few challenges since we have a state machine on the group. In the former/current model we need to set the state inside the loop. However, if we do that in the latter model we will likely run into a race condition. At least, it will execute unnecessary queries against the database. Other than adding the functionality needed to sync a single group, the MR moves the state machine handling to the respective class method.

See merge request !636

CHANGELOG-EE

@@ -6,6 +6,7 @@ v 8.11.0 (unreleased)
   - Performance improvement of push rules
   - Temporary fix for #825 - LDAP sync converts access requests to members. !655
   - Optimize commit and diff changes access check to reduce git operations
+  - Allow syncing a group against all providers at once
   - Change LdapGroupSyncWorker to use new LDAP group sync classes
   - Removed unused GitLab GEO database index
   - Enable monitoring for ES classes

lib/ee/gitlab/ldap/sync/group.rb

@@ -5,8 +5,58 @@ module EE
         class Group
           attr_reader :provider, :group, :proxy
 
-          def self.execute(group, proxy)
-            self.new(group, proxy).update_permissions
+          class << self
+            # Sync members across all providers for the given group.
+            def execute_all_providers(group)
+              return unless ldap_sync_ready?(group)
+
+              group.start_ldap_sync
+              Rails.logger.debug { "Started syncing all providers for '#{group.name}' group" }
+
+              # Shuffle providers to prevent a scenario where sync fails after a time
+              # and only the first provider or two get synced. This shuffles the order
+              # so subsequent syncs should eventually get to all providers. Obviously
+              # we should avoid failure, but this is an additional safeguard.
+              ::Gitlab::LDAP::Config.providers.shuffle.each do |provider|
+                Sync::Proxy.open(provider) do |proxy|
+                  new(group, proxy).update_permissions
+                end
+              end
+
+              group.finish_ldap_sync
+              Rails.logger.debug { "Finished syncing all providers for '#{group.name}' group" }
+            end
+
+            # Sync members across a single provider for the given group.
+            def execute(group, proxy)
+              return unless ldap_sync_ready?(group)
+
+              group.start_ldap_sync
+              Rails.logger.debug { "Started syncing '#{proxy.provider}' provider for '#{group.name}' group" }
+
+              sync_group = new(group, proxy)
+              sync_group.update_permissions
+
+              group.finish_ldap_sync
+              Rails.logger.debug { "Finished syncing '#{proxy.provider}' provider for '#{group.name}' group" }
+            end
+
+            def ldap_sync_ready?(group)
+              fail_stuck_group(group)
+
+              return true unless group.ldap_sync_started?
+
+              Rails.logger.warn "Group '#{group.name}' is not ready for LDAP sync. Skipping"
+              false
+            end
+
+            def fail_stuck_group(group)
+              return unless group.ldap_sync_started?
+
+              if group.ldap_sync_last_sync_at < 1.hour.ago
+                group.mark_ldap_sync_as_failed('The sync took too long to complete.')
+              end
+            end
           end
 
           def initialize(group, proxy)
@@ -16,15 +66,10 @@ module EE
           end
 
           def update_permissions
-            fail_stuck_group(group)
-
-            if group.ldap_sync_started?
-              logger.debug { "Group '#{group.name}' is not ready for LDAP sync. Skipping" }
+            unless group.ldap_sync_started?
+              logger.warn "Group '#{group.name}' LDAP sync status must be 'started' before updating permissions"
               return
             end
-            group.start_ldap_sync
-
-            logger.debug { "Syncing '#{group.name}' group" }
 
             access_levels = AccessLevels.new
             # Only iterate over group links for the current provider
@@ -39,10 +84,6 @@ module EE
 
             update_existing_group_membership(group, access_levels)
             add_new_members(group, access_levels)
-
-            group.finish_ldap_sync
-
-            logger.debug { "Finished syncing '#{group.name}' group" }
           end
 
           private
@@ -145,14 +186,6 @@ module EE
               .with_identity_provider(provider).preload(:user)
           end
 
-          def fail_stuck_group(group)
-            return false unless group.ldap_sync_started?
-
-            if group.ldap_sync_last_sync_at < 1.hour.ago
-              group.mark_ldap_sync_as_failed('The sync took too long to complete.')
-            end
-          end
-
           def logger
             Rails.logger
           end

spec/lib/ee/gitlab/ldap/sync/group_spec.rb

@@ -13,16 +13,15 @@ describe EE::Gitlab::LDAP::Sync::Group, lib: true do
     stub_ldap_group_find_by_cn('ldap_group1', ldap_group1, adapter)
   end
 
-  describe '#update_permissions' do
-    let(:group) do
-      create(:group_with_ldap_group_link,
-             cn: 'ldap_group1',
-             group_access: ::Gitlab::Access::DEVELOPER)
-    end
+  shared_examples :group_state_machine do
+    it 'uses the ldap sync state machine' do
+      expect(group).to receive(:start_ldap_sync)
+      expect(group).to receive(:finish_ldap_sync)
+      expect(EE::Gitlab::LDAP::Sync::Group)
+        .to receive(:new).at_most(:twice).and_call_original
 
-    context 'with all functionality against one LDAP group type' do
-      context 'with basic add/update actions' do
-        let(:ldap_group1) { ldap_group_entry(user_dn(user.username)) }
+      execute
+    end
 
     it 'fails a stuck group older than 1 hour' do
       group.start_ldap_sync
@@ -30,27 +29,90 @@ describe EE::Gitlab::LDAP::Sync::Group, lib: true do
 
       expect(group).to receive(:mark_ldap_sync_as_failed)
 
-          sync_group.update_permissions
+      execute
     end
 
     context 'when the group ldap sync is already started' do
       it 'logs a debug message' do
         group.start_ldap_sync
 
-            expect(Rails.logger).to receive(:debug) do |&block|
-              expect(block.call).to match /^Group '\w*' is not ready for LDAP sync. Skipping/
-            end.at_least(1).times
+        expect(Rails.logger)
+          .to receive(:warn)
+                .with(/^Group '\w*' is not ready for LDAP sync. Skipping/)
+                .at_least(:once)
 
-            sync_group.update_permissions
+        execute
       end
 
-          it 'does not add new members' do
+      it 'does not update permissions' do
         group.start_ldap_sync
 
-            sync_group.update_permissions
+        expect_any_instance_of(EE::Gitlab::LDAP::Sync::Group)
+          .not_to receive(:update_permissions)
+
+        execute
+      end
+    end
+  end
+
+  describe '.execute_all_providers' do
+    def execute
+      described_class.execute_all_providers(group)
+    end
+
+    before do
+      allow(Gitlab::LDAP::Config)
+        .to receive(:providers).and_return(['main', 'secondary'])
+      allow(EE::Gitlab::LDAP::Sync::Proxy)
+        .to receive(:open).and_yield(double('proxy').as_null_object)
+    end
+
+    let(:group) do
+      create(:group_with_ldap_group_link,
+             cn: 'ldap_group1',
+             group_access: ::Gitlab::Access::DEVELOPER)
+    end
+    let(:ldap_group1) { ldap_group_entry(user_dn(user.username)) }
+
+    include_examples :group_state_machine
+  end
 
-            expect(group.members).not_to include(user)
+  describe '.execute' do
+    def execute
+      described_class.execute(group, proxy(adapter))
     end
+
+    let(:group) do
+      create(:group_with_ldap_group_link,
+             cn: 'ldap_group1',
+             group_access: ::Gitlab::Access::DEVELOPER)
+    end
+    let(:ldap_group1) { ldap_group_entry(user_dn(user.username)) }
+
+    include_examples :group_state_machine
+  end
+
+  describe '#update_permissions' do
+    before { group.start_ldap_sync }
+    after { group.finish_ldap_sync }
+
+    let(:group) do
+      create(:group_with_ldap_group_link,
+             cn: 'ldap_group1',
+             group_access: ::Gitlab::Access::DEVELOPER)
+    end
+
+    context 'with all functionality against one LDAP group type' do
+      context 'with basic add/update actions' do
+        let(:ldap_group1) { ldap_group_entry(user_dn(user.username)) }
+
+        it 'does not update permissions unless ldap sync status is started' do
+          group.finish_ldap_sync
+
+          expect(Rails.logger)
+            .to receive(:warn).with(/status must be 'started' before updating permissions/)
+
+          sync_group.update_permissions
         end
 
         it 'adds new members' do
@@ -80,13 +142,6 @@ describe EE::Gitlab::LDAP::Sync::Group, lib: true do
           expect(group.members.find_by(user_id: user.id).access_level)
             .to eq(::Gitlab::Access::DEVELOPER)
         end
-
-        it 'uses the ldap sync state machine' do
-          expect(group).to receive(:start_ldap_sync)
-          expect(group).to receive(:finish_ldap_sync)
-
-          sync_group.update_permissions
-        end
       end
 
       context 'when existing user is no longer in LDAP group' do