Optimize LDAP and add a search timeout

Related to #4282 (moved)

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.

Title changed from Optimize LDAP and add a search timeout to WIP: Optimize LDAP and add a search timeout

Added 1 commit:

• a2ff5763 - Optimize LDAP and add a search timeout

Added 1 commit:

• 6f9f3670 - Optimize LDAP and add a search timeout

Added 1 commit:

• 56d8ee17 - Optimize LDAP and add a search timeout

Title changed from WIP: Optimize LDAP and add a search timeout to Optimize LDAP and add a search timeout

Added 1 commit:

• 025ab11f - Optimize LDAP and add a search timeout

Added 1 commit:

• eafd8c61 - Optimize LDAP and add a search timeout

Added 1 commit:

• d8de2867 - Optimize LDAP and add a search timeout

Added 1 commit:

• b3adaf27 - Optimize LDAP and add a search timeout

Added 1 commit:

• cc238e31 - Optimize LDAP and add a search timeout

There's an explicit reader method below so this is redundant.

@rspeicher I changed the default to 10.seconds as you suggested.

I also changed this to use Ruby's Timeout class instead of Net::LDAP's timeout. When I simulated a connection hang with toxiproxy I found Net::LDAP's timeout doesn't work, but Ruby's does. I raised an issue with the ruby-net-ldap project.

Can we pull this in for the next patch, please?

Milestone changed to 8.3

mentioned in issue #5547 (closed)

@dblessing LGTM.

/cc @marin gitlab.yml changes!

@marin @rspeicher We shouldn't need to change anything on Omnibus because the change was inside of the YAML part of LDAP config.

@dblessing Be sure to assign for final review and merge.

We shouldn't need to change anything on Omnibus because the change was inside of the YAML part of LDAP config.

Not sure that's right. If we add a setting to gitlab.yml it usually needs to be reflected in Omnibus, right?

Reassigned to @DouweM

Not sure that's right. If we add a setting to gitlab.yml it usually needs to be reflected in Omnibus, right?

Normally, yes. But here there's no new key because it's just a new piece inside the embedded YAML.

# gitlab_rails['ldap_servers'] = YAML.load <<-'EOS' # remember to close this block with 'EOS' below
#   main: # 'main' is the GitLab 'provider ID' of this LDAP server
#     label: 'LDAP'
#     host: '_your_ldap_server'
#     port: 389
#     uid: 'sAMAccountName'
#     method: 'plain' # "tls" or "ssl" or "plain"
#     bind_dn: '_the_full_dn_of_the_user_you_will_bind_with'
#     password: '_the_password_of_the_bind_user'
#     timeout: 30
...

Added 228 commits:

• cc238e31...87000b25 - 227 commits from branch gitlab-org:master
• f006a928 - Optimize LDAP and add a search timeout

Added 1 commit:

• 13be89d9 - Optimize LDAP and add a search timeout

Duplicated changelog entry? :)

Reassigned to @dbalexandre

Reassigned to @dblessing

/cc @marin gitlab.yml changes!

Thanks for the ping @razer6 !

@marin @rspeicher We shouldn't need to change anything on Omnibus because the change was inside of the YAML part of LDAP config.

@dblessing Correct! One of the advantages of the current way of specifying LDAP settings.

Added 100 commits:

• 13be89d9...a9800ce4 - 99 commits from branch gitlab-org:master
• 67aa0b8c - Optimize LDAP and add a search timeout

@DouweM Thanks for the feedback. I removed dup CHANGELOG and changed to . instead of ::.

Reassigned to @DouweM

Enabled an automatic merge when the build for dblessing/gitlab-ce@67aa0b8c4cbf762211ad178efb537f1649d91776 succeeds

Status changed to merged

mentioned in commit 49f51ff1

mentioned in commit 76ec8a11

Mentioned in commit maxraab/gitlab-ce@76ec8a11