Make ldap pool period and idle time configurable #2091

Martin-Kemp · 2022-09-15T10:18:48Z

Description

Configure ldap connection pool pruning period and idle time through config.yml. For example, currently you can enable pooling and set the min and max size. ideally pool.idle_time and pool.pruning_period should also be configurable. Like this:

pool.enabled: true
pool.min_size: 0
pool.max_size: 5
pool.idle_time: 2
pool.pruning_period: 1

Category (Enhancement)
Why these changes are required?
To comply with strict ldap providers.
What is the old behavior before changes and new behavior after changes?
Defaults are the same, this just makes things configurable.

Issues Resolved

#2090

Is this a backport? If so, please add backport PR # and/or commits #
No

Testing

Manual testing locally.

Check List

New functionality includes testing
New functionality has been documented
Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Martin Kemp <[email protected]>

codecov-commenter · 2022-09-15T17:57:33Z

Codecov Report

Merging #2091 (519712c) into main (7f992eb) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##               main    #2091      +/-   ##
============================================
+ Coverage     60.99%   61.00%   +0.01%     
- Complexity     3226     3227       +1     
============================================
  Files           256      256              
  Lines         18075    18075              
  Branches       3225     3225              
============================================
+ Hits          11024    11026       +2     
+ Misses         5472     5471       -1     
+ Partials       1579     1578       -1

Impacted Files	Coverage Δ
.../dlic/auth/ldap2/LDAPConnectionFactoryFactory.java	`56.48% <100.00%> (ø)`
...a/org/opensearch/security/tools/SecurityAdmin.java	`36.00% <0.00%> (+0.24%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

cwperks · 2022-09-16T14:01:45Z

src/main/java/com/amazon/dlic/auth/ldap/util/ConfigConstants.java

@@ -82,6 +82,9 @@ public final class ConfigConstants {

    public static final String LDAP_POOL_TYPE = "pool.type";

+    public static final String LDAP_POOL_PRUNING_PERIOD = "pool.pruning_period";


Thank you for creating a ConfigConstant out of these values. It looks like there's a lack in documentation for these settings: https://github.com/opensearch-project/documentation-website/search?q=pruning.period and https://github.com/opensearch-project/documentation-website/search?q=pruning.idleTime.

I will create an issue on the documentation website to review the LDAP documentation and add or update where needed.

While lower_snake_case appears to be the convention for settings keys, this may have the potential to break existing configs referencing these settings.

What do you think about supporting the existing settings keys as well?

this.settings.getAsLong(ConfigConstants.LDAP_POOL_PRUNING_PERIOD, this.settings.getAsLong("pruning.period", 5l)) this.settings.getAsLong(ConfigConstants.LDAP_POOL_IDLE_TIME, this.settings.getAsLong("pruning.idleTime", 10l))

@cwillum - Looks like the documentation site for LDAP Configuration needs a review. https://opensearch.org/docs/latest/security-plugin/configuration/ldap/

@cwperks, thanks for having a look!

What do you think about supporting the existing settings keys as well?

Yes, good idea. I'll update it on Monday.

@cwillum - Looks like the documentation site for LDAP Configuration needs a review. https://opensearch.org/docs/latest/security-plugin/configuration/ldap/

The whole ldap2 backend type isn't documented, I'm not even sure if it's actually supposed to be used.

I added an issue for review and update of LDAP documentation: #1248.

peternied

We need to support backward capability, registering the previous setting as deprecated Setting.Property.Deprecated on the current settings values, and then registering the new ones you've created. This would allow the proper behavior and allow for fallback for anyone that used the code as documentation.

I know this was mentioned on the thread with @cwperks, just making sure to document how to move forward with this PR.

Signed-off-by: Martin Kemp <[email protected]>

Martin-Kemp · 2022-09-19T10:07:38Z

registering the previous setting as deprecated Setting.Property.Deprecated on the current settings values, and then registering the new ones you've created

@peternied, I'm not sure how to do this. Could you point me in the right direction? How do I set the property to Deprecated?

I've pushed another commit to support the existing keys as suggested by @cwperks

peternied

@Martin-Kemp No worries, I'll make a follow up PR to add the deprecation notice for the legacy settings, what you've done here is great!

(cherry picked from commit 1fc02ad)

(cherry picked from commit 1fc02ad) Co-authored-by: Martin-Kemp <[email protected]>

…2091)

…2091) Signed-off-by: Stephen Crawford <[email protected]>

…2091) (opensearch-project#2097) (cherry picked from commit 1fc02ad) Co-authored-by: Martin-Kemp <[email protected]>

Martin-Kemp requested a review from a team September 15, 2022 10:18

Make ldap pool period and idle time configurable

519712c

Signed-off-by: Martin Kemp <[email protected]>

Martin-Kemp force-pushed the main branch from 7b56841 to 519712c Compare September 15, 2022 10:21

cwperks reviewed Sep 16, 2022

View reviewed changes

cwillum mentioned this pull request Sep 16, 2022

[DOC-META] Security Plugin Documentation opensearch-project/documentation-website#816

Open

19 tasks

peternied requested changes Sep 16, 2022

View reviewed changes

Support existing setting keys

5549b2c

Signed-off-by: Martin Kemp <[email protected]>

peternied approved these changes Sep 20, 2022

View reviewed changes

cwperks approved these changes Sep 21, 2022

View reviewed changes

cwperks added the backport 2.x backport to 2.x branch label Sep 21, 2022

peternied merged commit 1fc02ad into opensearch-project:main Sep 21, 2022

opensearch-trigger-bot bot pushed a commit that referenced this pull request Sep 21, 2022

Make ldap pool period and idle time configurable (#2091)

9aea210

(cherry picked from commit 1fc02ad)

opensearch-trigger-bot bot mentioned this pull request Sep 21, 2022

[Backport 2.x] Make ldap pool period and idle time configurable #2097

Merged

peternied pushed a commit that referenced this pull request Sep 21, 2022

Make ldap pool period and idle time configurable (#2091) (#2097)

edd9f49

(cherry picked from commit 1fc02ad) Co-authored-by: Martin-Kemp <[email protected]>

peternied mentioned this pull request Sep 22, 2022

Log deprecation message on legacy ldap pool settings #2099

Merged

3 tasks

vinayak15 pushed a commit to vinayak15/security that referenced this pull request Sep 29, 2022

Make ldap pool period and idle time configurable (opensearch-project#…

1758d8a

…2091)

cwillum mentioned this pull request Oct 26, 2022

Add connection pooling settings to LDAP documentation opensearch-project/documentation-website#1698

Merged

1 task

peternied mentioned this pull request Nov 3, 2022

2.4 Release Notes for Security Plugin #2231

Merged

3 tasks

stephen-crawford pushed a commit to stephen-crawford/security that referenced this pull request Nov 10, 2022

Make ldap pool period and idle time configurable (opensearch-project#…

28558b4

…2091) Signed-off-by: Stephen Crawford <[email protected]>

wuychn pushed a commit to ochprince/security that referenced this pull request Mar 16, 2023

Make ldap pool period and idle time configurable (opensearch-project#…

2a60b5b

…2091) (opensearch-project#2097) (cherry picked from commit 1fc02ad) Co-authored-by: Martin-Kemp <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make ldap pool period and idle time configurable #2091

Make ldap pool period and idle time configurable #2091

Martin-Kemp commented Sep 15, 2022

codecov-commenter commented Sep 15, 2022 •

edited

Loading

cwperks Sep 16, 2022

cwperks Sep 16, 2022

Martin-Kemp Sep 16, 2022 •

edited

Loading

Martin-Kemp Sep 16, 2022

cwillum Sep 16, 2022

peternied left a comment

Martin-Kemp commented Sep 19, 2022

peternied left a comment

		@@ -82,6 +82,9 @@ public final class ConfigConstants {

		public static final String LDAP_POOL_TYPE = "pool.type";

		public static final String LDAP_POOL_PRUNING_PERIOD = "pool.pruning_period";

Make ldap pool period and idle time configurable #2091

Make ldap pool period and idle time configurable #2091

Conversation

Martin-Kemp commented Sep 15, 2022

Description

Issues Resolved

Testing

Check List

codecov-commenter commented Sep 15, 2022 • edited Loading

Codecov Report

cwperks Sep 16, 2022

Choose a reason for hiding this comment

cwperks Sep 16, 2022

Choose a reason for hiding this comment

Martin-Kemp Sep 16, 2022 • edited Loading

Choose a reason for hiding this comment

Martin-Kemp Sep 16, 2022

Choose a reason for hiding this comment

cwillum Sep 16, 2022

Choose a reason for hiding this comment

peternied left a comment

Choose a reason for hiding this comment

Martin-Kemp commented Sep 19, 2022

peternied left a comment

Choose a reason for hiding this comment

codecov-commenter commented Sep 15, 2022 •

edited

Loading

Martin-Kemp Sep 16, 2022 •

edited

Loading