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.

Sign up for GitHub

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

Make ldap pool period and idle time configurable #2091

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
519712c
Make ldap pool period and idle time configurable
Sep 15, 2022
5549b2c
Support existing setting keys
Sep 19, 2022
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
3 changes: 3 additions & 0 deletions src/main/java/com/amazon/dlic/auth/ldap/util/ConfigConstants.java
View file
Open in desktop
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 •
edited
Loading

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
7 changes: 5 additions & 2 deletions src/main/java/com/amazon/dlic/auth/ldap2/LDAPConnectionFactoryFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,8 +127,11 @@ 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, this.settings.getAsLong("pruning.period", 5l))),
Duration.ofMinutes(this.settings.getAsLong(ConfigConstants.LDAP_POOL_IDLE_TIME, this.settings.getAsLong("pruning.idleTime", 10l))))
);

result.initialize();

Expand Down