Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add connection pooling settings to LDAP documentation #1698

Merged
merged 7 commits into from
Oct 28, 2022

Conversation

cwillum
Copy link
Contributor

@cwillum cwillum commented Oct 26, 2022

Signed-off-by: cwillum [email protected]

Description

Upon request to add documentation for new pool pruning period and pool pruning idle time settings to LDAP documentation, it became apparent that all connection pooling settings keys needed to be added to LDAP documentation.

Issues Resolved

Fixes #1583.

Added connection pooling settings to LDAP documentation:
pool.enabled: true
pool.min_size: 0
pool.max_size: 5
pool.idle_time: 2
pool.pruning_period: 1

Also created a new heading for advanced settings (which includes connection pooling settings) so that essential configuration for connection, authentication, and authorization are clearly distinct from these optional settings.

See PR #2091 in security for original request to add the settings to the code.

Checklist

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

@cwillum cwillum added enhancement New feature or request backport 2.0 PR: Backport label for v2.0.x security backport 2.1 PR: Backport label for 2.1 backport 2.2 PR: Backport label for 2.2 backport 2.3 PR: Backport label for 2.3 labels Oct 26, 2022
@cwillum cwillum self-assigned this Oct 26, 2022
@cwillum cwillum requested a review from a team as a code owner October 26, 2022 03:03
@cwillum cwillum marked this pull request as draft October 26, 2022 03:03
@cwillum cwillum marked this pull request as ready for review October 26, 2022 16:37
@cwillum cwillum added 3 - Tech review PR: Tech review in progress 4 - Doc review PR: Doc review in progress labels Oct 26, 2022
@cwillum
Copy link
Contributor Author

cwillum commented Oct 26, 2022

@opensearch-project/security Could someone on the team have a look at this for technical accuracy? Thanks.
One other question. The backport 2.x label was added to #2091. I'm guessing it wouldn't be accurate to add a backport 1.3 in this PR for documentation. Correct?

_security-plugin/configuration/ldap.md Outdated Show resolved Hide resolved
_security-plugin/configuration/ldap.md Outdated Show resolved Hide resolved
Comment on lines +653 to +654
`pool.pruning_period` | The interval in minutes at which the pruning implementation is executed. For example: when 5, the implementation is executed every five minutes. By default, the period is 5.
`pool.idle_time` | The length of time elapsed, in minutes, after a connnection is considered idle. Once elapsed, the connection becomes a candidate for pruning from the pool. By default, idle time is 10.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These settings are correct for v2.4+ otherwise they have different values pruning.period and pruning.idleTime in the older versions.

In 2.4+ legacy settings are still read and used and they are marked as deprecated

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@peternied I'm not sure I understand what the differences will be between versions -2.4 and versions 2.4+. Will all five connection pooling settings be available to versions before 2.4? If yes, I think you're saying the values will be different for the pruning.period and pruning.idleTime settings. If that's correct, what will they be?

Ultimately, I'm wondering what details in the documentation added here will lead users astray when they add these settings to config.yml and they're running versions before 2.4? If there are differences, it sounds like I'll need to create a separate PR for earlier versions.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Versions Less than 2.4.0 (e.g. 1.3, 2.1, 2.2, 2.3)

pruning.period and pruning.idleTime work, but were never documented except for the code

Versions Equal or greater than 2.4.0

pool.idle_time and pool.pruning_period work.
pruning.period and pruning.idleTime work and spit out a deprecation warning in the OpenSearch log

I think you could document all the settings for the version to release in 2.4.0. Then make another pull request that removes them targeting the older versions. Someone familiar with this repo might have cleaner instructions or a workflow to follow

@cwillum cwillum added the v2.4.0 'Issues and PRs related to version v2.4.0' label Oct 26, 2022
Copy link
Member

@cwperks cwperks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good to me. Looking into the different between LDAPAuthenticationBackend2 and LDAPAuthenticationBackend and will follow-up with a more certain answer.

@@ -426,12 +426,15 @@ If you don't use or have a role subtree, you can disable the role search complet
rolesearch_enabled: false
```

## Advanced settings
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍


### (Advanced) Control LDAP user attributes
The advanced settings presented below are optional for an essential LDAP configuration. They can, however, improve efficiency and performance for the LDAP implementation.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These settings also improve security as well. Perhaps, efficiency, performance and security?


### Connection pooling settings

The directory server can maintain a pool of connections at the ready, assigning them when needed and returning them to the pool after a connection is closed. This arrangement can lower demands on the resources used to create connections, improve OpenSearch performance, and reduce load on the server. You can use the settings below to control the way the pooling is carried out.
Copy link
Member

@cwperks cwperks Oct 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Opensearch is the one maintaining the pool of connections, not the directory server. The security plugin achieves pooling using this library: https://www.ldaptive.org/docs/guide/connections/pooling.html

I'm also only seeing connection pooling in LDAPAuthenticationBackend2 here: https://github.com/opensearch-project/security/blob/main/src/main/java/com/amazon/dlic/auth/ldap2/LDAPAuthenticationBackend2.java#L71, but not in LDAPAuthenticationBackend

Diving in deeper now to determine when LDAPAuthenticationBackend2 is used instead of LDAPAuthenticationBackend and will follow-up with another comment when I determine when each is used.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the tests for LDAPAuthenticationBackend2 it looks like the type in the config.yml is the fully qualified classname com.amazon.dlic.auth.ldap2.LDAPAuthenticationBackend2 and I don't see this anywhere in the documentation website. @nibix would you be able to provide more context about the 2 different LDAP authentication backends?

See https://github.com/opensearch-project/security/blob/main/src/test/resources/ldap/config_ldap2.yml#L35

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like you can also use the short-hand ldap2 judging by this: https://github.com/opensearch-project/security/blob/main/src/main/java/org/opensearch/security/securityconf/DynamicConfigModelV7.java#L349-L356 though I don't see any mention of this on the documentation website.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I remember, the major difference of the ldap2 module is indeed connection pooling. Historically, it was introduced as an experimental module in Search Guard which was supposed to replace the ldap module at some point in time. Thus, it is also undocumented so far.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed this to, "OpenSearch can maintain a pool of connections..."

@cwillum cwillum removed 3 - Tech review PR: Tech review in progress backport 2.0 PR: Backport label for v2.0.x 4 - Doc review PR: Doc review in progress backport 2.1 PR: Backport label for 2.1 backport 2.2 PR: Backport label for 2.2 labels Oct 28, 2022
@cwillum cwillum removed the backport 2.3 PR: Backport label for 2.3 label Oct 28, 2022
@cwillum
Copy link
Contributor Author

cwillum commented Oct 28, 2022

Will leave the v2.4 tag for this PR so that all five connection pooling settings are documented for this version of OS. I can create a new PR for v2.3 with only the three connection pooling settings pool.enabled, pool.min_size, and pool.max_size then backport, if that's the appropriate thing to do.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request security v2.4.0 'Issues and PRs related to version v2.4.0'
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add documentation for pool pruning period and idle time settings for LDAP
5 participants