Commit fa0a09f9 authored by Omar Mochtar's avatar Omar Mochtar Committed by Heinrich Lee Yu

Retry if got empty ldap results for certain response code

parent 8ee19b92
...@@ -828,6 +828,11 @@ production: &base ...@@ -828,6 +828,11 @@ production: &base
# #
sync_ssh_keys: false 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. # 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'), # 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']). # or an array of attribute names to try in order (e.g. ['mail', 'email']).
......
...@@ -174,6 +174,7 @@ production: ...@@ -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'` | | `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). | | `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 | | `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 #### Examples of user filters
......
...@@ -53,11 +53,7 @@ module Gitlab ...@@ -53,11 +53,7 @@ module Gitlab
if results.nil? if results.nil?
response = ldap.get_operation_result response = ldap.get_operation_result
check_empty_response_code(response)
unless response.code == 0
Gitlab::AppLogger.warn("LDAP search error: #{response.message}")
end
[] []
else else
results results
...@@ -136,6 +132,16 @@ module Gitlab ...@@ -136,6 +132,16 @@ module Gitlab
def renew_connection_adapter def renew_connection_adapter
@ldap = Net::LDAP.new(config.adapter_options) @ldap = Net::LDAP.new(config.adapter_options)
end 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 end
end end
......
...@@ -163,6 +163,10 @@ module Gitlab ...@@ -163,6 +163,10 @@ module Gitlab
options['timeout'].to_i options['timeout'].to_i
end end
def retry_empty_result_with_codes
options.fetch('retry_empty_result_with_codes', [])
end
def external_groups def external_groups
options['external_groups'] || [] options['external_groups'] || []
end end
......
...@@ -95,6 +95,40 @@ RSpec.describe Gitlab::Auth::Ldap::Adapter do ...@@ -95,6 +95,40 @@ RSpec.describe Gitlab::Auth::Ldap::Adapter do
describe '#ldap_search' do describe '#ldap_search' do
subject { adapter.ldap_search(base: :dn, filter: :filter) } 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 "when the search is successful" do
context "and the result is non-empty" do context "and the result is non-empty" do
before do before do
...@@ -110,6 +144,22 @@ RSpec.describe Gitlab::Auth::Ldap::Adapter do ...@@ -110,6 +144,22 @@ RSpec.describe Gitlab::Auth::Ldap::Adapter do
end end
it { is_expected.to eq [] } 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
end end
...@@ -132,30 +182,13 @@ RSpec.describe Gitlab::Auth::Ldap::Adapter do ...@@ -132,30 +182,13 @@ RSpec.describe Gitlab::Auth::Ldap::Adapter do
end end
context 'retries the operation' do context 'retries the operation' do
before do let(:err_message) { 'some error' }
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 before do
stub_const("#{described_class}::MAX_SEARCH_RETRIES", 1) allow(ldap).to receive(:search) { raise Net::LDAP::Error, err_message }
end end
it 'raises the exception' do it_behaves_like 'connection retry'
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
end end
end end
end end
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment