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

Make ldap pool period and idle time configurable #2091

Merged
merged 2 commits into from
Sep 21, 2022
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Copy link
Member

Choose a reason for hiding this comment

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

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))

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@Martin-Kemp Martin-Kemp Sep 16, 2022

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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.

Copy link

Choose a reason for hiding this comment

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

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

public static final String LDAP_POOL_IDLE_TIME = "pool.idle_time";

private ConfigConstants() {

}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -127,8 +127,8 @@ public ConnectionPool createConnectionPool() {
}

result.setValidator(getConnectionValidator());
result.setPruneStrategy(new IdlePruneStrategy(Duration.ofMinutes(this.settings.getAsLong("pruning.period", 5l)),
Duration.ofMinutes(this.settings.getAsLong("pruning.idleTime", 10l))));
result.setPruneStrategy(new IdlePruneStrategy(Duration.ofMinutes(this.settings.getAsLong(ConfigConstants.LDAP_POOL_PRUNING_PERIOD, 5l)),
Duration.ofMinutes(this.settings.getAsLong(ConfigConstants.LDAP_POOL_IDLE_TIME, 10l))));

result.initialize();

Expand Down