Merge branch 'add-retry-in-empty-ldap-results' into 'master'

Retry if got empty ldap results for certain response code

See merge request gitlab-org/gitlab!63134

config/gitlab.yml.example

@@ -828,6 +828,11 @@ production: &base
         #
         sync_ssh_keys: false
 
+        # Retry ldap search connection if got empty results with specified response code(s)
+        #
+        #   Ex. [80]
+        # retry_empty_result_with_codes: []
+
         # LDAP attributes that GitLab will use to create an account for the LDAP user.
         # The specified attribute can either be the attribute name as a string (e.g. 'mail'),
         # or an array of attribute names to try in order (e.g. ['mail', 'email']).

doc/administration/auth/ldap/index.md

@@ -174,6 +174,7 @@ production:
 | `base` | Base where we can search for users. | **{check-circle}** Yes | `'ou=people,dc=gitlab,dc=example'` or `'DC=mydomain,DC=com'` |
 | `user_filter`      | Filter LDAP users. Format: [RFC 4515](https://tools.ietf.org/search/rfc4515) Note: GitLab does not support `omniauth-ldap`'s custom filter syntax. | **{dotted-circle}** No | For examples, read [Examples of user filters](#examples-of-user-filters). |
 | `lowercase_usernames` | If enabled, GitLab converts the name to lower case. | **{dotted-circle}** No | boolean |
+| `retry_empty_result_with_codes` | An array of LDAP query response code that will attempt to retrying the operation if the result/content is empty. | **{dotted-circle}** No | `[80]` |
 
 #### Examples of user filters
 

lib/gitlab/auth/ldap/adapter.rb

@@ -53,11 +53,7 @@ module Gitlab
 
             if results.nil?
               response = ldap.get_operation_result
-
-              unless response.code == 0
-                Gitlab::AppLogger.warn("LDAP search error: #{response.message}")
-              end
-
+              check_empty_response_code(response)
               []
             else
               results
@@ -136,6 +132,16 @@ module Gitlab
         def renew_connection_adapter
           @ldap = Net::LDAP.new(config.adapter_options)
         end
+
+        def check_empty_response_code(response)
+          if config.retry_empty_result_with_codes.include?(response.code)
+            raise Net::LDAP::Error, "Got empty results with response code: #{response.code}, message: #{response.message}"
+          end
+
+          unless response.code == 0
+            Gitlab::AppLogger.warn("LDAP search error: #{response.message}")
+          end
+        end
       end
     end
   end

lib/gitlab/auth/ldap/config.rb

@@ -163,6 +163,10 @@ module Gitlab
           options['timeout'].to_i
         end
 
+        def retry_empty_result_with_codes
+          options.fetch('retry_empty_result_with_codes', [])
+        end
+
         def external_groups
           options['external_groups'] || []
         end

spec/lib/gitlab/auth/ldap/adapter_spec.rb

@@ -95,6 +95,40 @@ RSpec.describe Gitlab::Auth::Ldap::Adapter do
   describe '#ldap_search' do
     subject { adapter.ldap_search(base: :dn, filter: :filter) }
 
+    shared_examples 'connection retry' do
+      before do
+        allow(adapter).to receive(:renew_connection_adapter).and_return(ldap)
+        allow(Gitlab::AppLogger).to receive(:warn)
+      end
+
+      context 'retries the operation' do
+        before do
+          stub_const("#{described_class}::MAX_SEARCH_RETRIES", 3)
+        end
+
+        it 'as many times as MAX_SEARCH_RETRIES' do
+          expect(ldap).to receive(:search).exactly(3).times
+          expect { subject }.to raise_error(Gitlab::Auth::Ldap::LdapConnectionError)
+        end
+
+        context 'when no more retries' do
+          before do
+            stub_const("#{described_class}::MAX_SEARCH_RETRIES", 1)
+          end
+
+          it 'raises the exception' do
+            expect { subject }.to raise_error(Gitlab::Auth::Ldap::LdapConnectionError)
+          end
+
+          it 'logs the error' do
+            expect { subject }.to raise_error(Gitlab::Auth::Ldap::LdapConnectionError)
+            expect(Gitlab::AppLogger).to have_received(:warn).with(
+              "LDAP search raised exception Net::LDAP::Error: #{err_message}")
+          end
+        end
+      end
+    end
+
     context "when the search is successful" do
       context "and the result is non-empty" do
         before do
@@ -110,6 +144,22 @@ RSpec.describe Gitlab::Auth::Ldap::Adapter do
         end
 
         it { is_expected.to eq [] }
+
+        context 'when returned with expected code' do
+          let(:response_code) { 80 }
+          let(:response_message) { 'Other' }
+          let(:err_message) { "Got empty results with response code: #{response_code}, message: #{response_message}" }
+
+          before do
+            stub_ldap_config(retry_empty_result_with_codes: [response_code])
+            allow(ldap).to receive_messages(
+              search: nil,
+              get_operation_result: double(code: response_code, message: response_message)
+            )
+          end
+
+          it_behaves_like 'connection retry'
+        end
       end
     end
 
@@ -132,30 +182,13 @@ RSpec.describe Gitlab::Auth::Ldap::Adapter do
       end
 
       context 'retries the operation' do
-        before do
-          stub_const("#{described_class}::MAX_SEARCH_RETRIES", 3)
-        end
-
-        it 'as many times as MAX_SEARCH_RETRIES' do
-          expect(ldap).to receive(:search).exactly(3).times
-          expect { subject }.to raise_error(Gitlab::Auth::Ldap::LdapConnectionError)
-        end
+        let(:err_message) { 'some error' }
 
-        context 'when no more retries' do
         before do
-            stub_const("#{described_class}::MAX_SEARCH_RETRIES", 1)
+          allow(ldap).to receive(:search) { raise Net::LDAP::Error, err_message }
         end
 
-          it 'raises the exception' do
-            expect { subject }.to raise_error(Gitlab::Auth::Ldap::LdapConnectionError)
-          end
-
-          it 'logs the error' do
-            expect { subject }.to raise_error(Gitlab::Auth::Ldap::LdapConnectionError)
-            expect(Gitlab::AppLogger).to have_received(:warn).with(
-              "LDAP search raised exception Net::LDAP::Error: some error")
-          end
-        end
+        it_behaves_like 'connection retry'
       end
     end
   end