From 76ec8a112e1b1b04dccc4de68b06ea05fdd06192 Mon Sep 17 00:00:00 2001
From: Douwe Maan <douwe@gitlab.com>
Date: Mon, 11 Jan 2016 17:32:33 +0000
Subject: [PATCH] Merge branch 'optimize_ldap' into 'master'
Optimize LDAP and add a search timeout
Related to #4282
This merge request arranges some things in `access.rb` to facilitate some optimizations in EE (to come later). It also adds a 10 second timeout to all LDAP searches so the entire worker is not blocked if some query doesn't return in a reasonable amount of time. This timeout is configurable per LDAP server.
See merge request !2267
---
CHANGELOG | 1 +
config/gitlab.yml.example | 5 +++++
config/initializers/1_settings.rb | 1 +
doc/integration/ldap.md | 5 +++++
lib/gitlab/ldap/access.rb | 8 ++++++--
lib/gitlab/ldap/adapter.rb | 24 +++++++++++++++---------
lib/gitlab/ldap/config.rb | 4 ++++
7 files changed, 37 insertions(+), 11 deletions(-)
diff --git a/CHANGELOG b/CHANGELOG
index 34cba00bfaa..e754a2ee1b7 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -3,6 +3,7 @@ Please view this file on the master branch, on stable branches it's out of date.
v 8.3.3 (unreleased)
- Preserve CE behavior with JIRA integration by only calling API if URL is set
- Fix duplicated branch creation/deletion events when using Web UI (Stan Hu)
+ - Add configurable LDAP server query timeout
- Get "Merge when build succeeds" to work when commits were pushed to MR target branch while builds were running
- Suppress e-mails on failed builds if allow_failure is set (Stan Hu)
- Fix project transfer e-mail sending incorrect paths in e-mail notification (Stan Hu)
diff --git a/config/gitlab.yml.example b/config/gitlab.yml.example
index 54dddb41ac4..a402626eeb3 100644
--- a/config/gitlab.yml.example
+++ b/config/gitlab.yml.example
@@ -204,6 +204,11 @@ production: &base
bind_dn: '_the_full_dn_of_the_user_you_will_bind_with'
password: '_the_password_of_the_bind_user'
+ # Set a timeout, in seconds, for LDAP queries. This helps avoid blocking
+ # a request if the LDAP server becomes unresponsive.
+ # A value of 0 means there is no timeout.
+ timeout: 10
+
# This setting specifies if LDAP server is Active Directory LDAP server.
# For non AD servers it skips the AD specific queries.
# If your LDAP server is not AD, set this to false.
diff --git a/config/initializers/1_settings.rb b/config/initializers/1_settings.rb
index 46aa79c308f..3cbbc3b52ae 100644
--- a/config/initializers/1_settings.rb
+++ b/config/initializers/1_settings.rb
@@ -108,6 +108,7 @@ if Settings.ldap['enabled'] || Rails.env.test?
Settings.ldap['servers'].each do |key, server|
server['label'] ||= 'LDAP'
+ server['timeout'] ||= 10.seconds
server['block_auto_created_users'] = false if server['block_auto_created_users'].nil?
server['allow_username_or_email_login'] = false if server['allow_username_or_email_login'].nil?
server['active_directory'] = true if server['active_directory'].nil?
diff --git a/doc/integration/ldap.md b/doc/integration/ldap.md
index 845f588f913..f256477196b 100644
--- a/doc/integration/ldap.md
+++ b/doc/integration/ldap.md
@@ -48,6 +48,11 @@ main: # 'main' is the GitLab 'provider ID' of this LDAP server
bind_dn: '_the_full_dn_of_the_user_you_will_bind_with'
password: '_the_password_of_the_bind_user'
+ # Set a timeout, in seconds, for LDAP queries. This helps avoid blocking
+ # a request if the LDAP server becomes unresponsive.
+ # A value of 0 means there is no timeout.
+ timeout: 10
+
# This setting specifies if LDAP server is Active Directory LDAP server.
# For non AD servers it skips the AD specific queries.
# If your LDAP server is not AD, set this to false.
diff --git a/lib/gitlab/ldap/access.rb b/lib/gitlab/ldap/access.rb
index c438a3d167b..b2bdbc10d7f 100644
--- a/lib/gitlab/ldap/access.rb
+++ b/lib/gitlab/ldap/access.rb
@@ -5,7 +5,7 @@
module Gitlab
module LDAP
class Access
- attr_reader :adapter, :provider, :user
+ attr_reader :provider, :user
def self.open(user, &block)
Gitlab::LDAP::Adapter.open(user.ldap_identity.provider) do |adapter|
@@ -32,7 +32,7 @@ module Gitlab
end
def allowed?
- if Gitlab::LDAP::Person.find_by_dn(user.ldap_identity.extern_uid, adapter)
+ if ldap_user
return true unless ldap_config.active_directory
# Block user in GitLab if he/she was blocked in AD
@@ -59,6 +59,10 @@ module Gitlab
def ldap_config
Gitlab::LDAP::Config.new(provider)
end
+
+ def ldap_user
+ @ldap_user ||= Gitlab::LDAP::Person.find_by_dn(user.ldap_identity.extern_uid, adapter)
+ end
end
end
end
diff --git a/lib/gitlab/ldap/adapter.rb b/lib/gitlab/ldap/adapter.rb
index 577a890a7d9..df65179bfea 100644
--- a/lib/gitlab/ldap/adapter.rb
+++ b/lib/gitlab/ldap/adapter.rb
@@ -70,19 +70,25 @@ module Gitlab
end
def ldap_search(*args)
- results = ldap.search(*args)
+ # Net::LDAP's `time` argument doesn't work. Use Ruby `Timeout` instead.
+ Timeout.timeout(config.timeout) do
+ results = ldap.search(*args)
- if results.nil?
- response = ldap.get_operation_result
+ if results.nil?
+ response = ldap.get_operation_result
- unless response.code.zero?
- Rails.logger.warn("LDAP search error: #{response.message}")
- end
+ unless response.code.zero?
+ Rails.logger.warn("LDAP search error: #{response.message}")
+ end
- []
- else
- results
+ []
+ else
+ results
+ end
end
+ rescue Timeout::Error
+ Rails.logger.warn("LDAP search timed out after #{config.timeout} seconds")
+ []
end
end
end
diff --git a/lib/gitlab/ldap/config.rb b/lib/gitlab/ldap/config.rb
index 101a3285f4b..aff7ccb157f 100644
--- a/lib/gitlab/ldap/config.rb
+++ b/lib/gitlab/ldap/config.rb
@@ -88,6 +88,10 @@ module Gitlab
options['attributes']
end
+ def timeout
+ options['timeout'].to_i
+ end
+
protected
def base_config
Gitlab.config.ldap
--
2.30.9