You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@james.apache.org by "HoussemNasri (via GitHub)" <gi...@apache.org> on 2023/02/21 17:39:01 UTC
[GitHub] [james-project] HoussemNasri opened a new pull request, #1453: JAMES-2601 Implement a LDAP healthCheck
HoussemNasri opened a new pull request, #1453:
URL: https://github.com/apache/james-project/pull/1453
As the title says, added a new health checker to check LDAP server's health and tested it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] chibenwa commented on a diff in pull request #1453: JAMES-2601 Implement a LDAP healthCheck
Posted by "chibenwa (via GitHub)" <gi...@apache.org>.
chibenwa commented on code in PR #1453:
URL: https://github.com/apache/james-project/pull/1453#discussion_r1113755280
##########
server/data/data-ldap/src/main/java/org/apache/james/user/ldap/LdapHealthCheck.java:
##########
@@ -0,0 +1,38 @@
+package org.apache.james.user.ldap;
Review Comment:
You are missing the compulsary ASF V2 license header
##########
server/data/data-ldap/src/main/java/org/apache/james/user/ldap/LdapHealthCheck.java:
##########
@@ -0,0 +1,38 @@
+package org.apache.james.user.ldap;
+
+import javax.inject.Inject;
+
+import org.apache.james.core.Username;
+import org.apache.james.core.healthcheck.ComponentName;
+import org.apache.james.core.healthcheck.HealthCheck;
+import org.apache.james.core.healthcheck.Result;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import reactor.core.publisher.Mono;
+
+public class LdapHealthCheck implements HealthCheck {
+ public static final ComponentName COMPONENT_NAME = new ComponentName("LDAP User Server");
+ public static final Username LDAP_TEST_USER = Username.of("ldap-test");
+ private static final Logger LOGGER = LoggerFactory.getLogger(LdapHealthCheck.class);
+
+ private final ReadOnlyUsersLDAPRepository ldapUserRepository;
+
+ @Inject
+ public LdapHealthCheck(ReadOnlyUsersLDAPRepository ldapUserRepository) {
+ this.ldapUserRepository = ldapUserRepository;
+ }
+
+ @Override
+ public ComponentName componentName() {
+ return COMPONENT_NAME;
+ }
+
+ @Override
+ public Mono<Result> check() {
+ return Mono.fromCallable(() -> ldapUserRepository.getUserByName(LDAP_TEST_USER))
+ .map(user -> Result.healthy(COMPONENT_NAME))
+ .onErrorResume(e -> Mono.just(Result.unhealthy(COMPONENT_NAME, "Error checking LDAP server!", e)))
+ .doOnError(e -> LOGGER.error("Error in LDAP server", e));
Review Comment:
LDAP connections are blocking, we should schedule them on the `bundedElastic` scheduler....
##########
server/data/data-ldap/src/test/java/org/apache/james/user/ldap/LdapHealthCheckTest.java:
##########
@@ -0,0 +1,41 @@
+package org.apache.james.user.ldap;
+
+import org.apache.james.core.healthcheck.Result;
+import org.apache.james.domainlist.api.mock.SimpleDomainList;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+public class LdapHealthCheckTest {
+
+ LdapHealthCheck ldapHealthCheck;
+ ReadOnlyUsersLDAPRepository ldapUserRepository;
+ LdapGenericContainer ldapContainer;
+
+ @BeforeEach
+ public void setUp() {
+ ldapUserRepository = new ReadOnlyUsersLDAPRepository(new SimpleDomainList());
+ ldapHealthCheck = new LdapHealthCheck(ldapUserRepository);
+ ldapContainer = DockerLdapSingleton.ldapContainer;
+ }
+
+ @Test
+ void checkShouldReturnUnhealthyIfLdapIsDown() {
+ ldapContainer.pause();
+
+ Result checkResult = ldapHealthCheck.check().block();
+ assertNotNull(checkResult);
+ assertTrue(checkResult.isUnHealthy());
+
+ ldapContainer.unpause();
+ }
+
+ @Test
+ void checkShouldReturnHealthyIfLdapIsRunning() {
+ Result checkResult = ldapHealthCheck.check().block();
+ assertNotNull(checkResult);
+ assertTrue(checkResult.isHealthy());
Review Comment:
```suggestion
assertThat(checkResult.isHealthy()).isTrue();
```
##########
server/container/guice/data-ldap/src/main/java/org/apache/james/data/LdapUsersRepositoryModule.java:
##########
@@ -43,6 +45,7 @@ public void configure() {
bind(UsersRepository.class).to(ReadOnlyUsersLDAPRepository.class);
bind(DelegationStore.class).to(NaiveDelegationStore.class);
bind(Authorizator.class).to(UserRepositoryAuthorizator.class);
+ bind(HealthCheck.class).to(LdapHealthCheck.class);
Review Comment:
No this needs to be done through the multi-binder:
```suggestion
Multibinder.newSetBinder(binder(), HealthCheck.class).addBinding().to(LdapHealthCheck.class);
```
##########
server/data/data-ldap/src/main/java/org/apache/james/user/ldap/LdapHealthCheck.java:
##########
@@ -0,0 +1,38 @@
+package org.apache.james.user.ldap;
+
+import javax.inject.Inject;
+
+import org.apache.james.core.Username;
+import org.apache.james.core.healthcheck.ComponentName;
+import org.apache.james.core.healthcheck.HealthCheck;
+import org.apache.james.core.healthcheck.Result;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import reactor.core.publisher.Mono;
+
+public class LdapHealthCheck implements HealthCheck {
+ public static final ComponentName COMPONENT_NAME = new ComponentName("LDAP User Server");
+ public static final Username LDAP_TEST_USER = Username.of("ldap-test");
+ private static final Logger LOGGER = LoggerFactory.getLogger(LdapHealthCheck.class);
+
+ private final ReadOnlyUsersLDAPRepository ldapUserRepository;
+
+ @Inject
+ public LdapHealthCheck(ReadOnlyUsersLDAPRepository ldapUserRepository) {
+ this.ldapUserRepository = ldapUserRepository;
+ }
+
+ @Override
+ public ComponentName componentName() {
+ return COMPONENT_NAME;
+ }
+
+ @Override
+ public Mono<Result> check() {
+ return Mono.fromCallable(() -> ldapUserRepository.getUserByName(LDAP_TEST_USER))
Review Comment:
Please handle the case "user not found" and return healthy in this case: we should not require the ldat-test user to be preprovisionned.
##########
server/data/data-ldap/src/test/java/org/apache/james/user/ldap/LdapHealthCheckTest.java:
##########
@@ -0,0 +1,41 @@
+package org.apache.james.user.ldap;
Review Comment:
Missing license
##########
server/data/data-ldap/src/test/java/org/apache/james/user/ldap/LdapHealthCheckTest.java:
##########
@@ -0,0 +1,41 @@
+package org.apache.james.user.ldap;
+
+import org.apache.james.core.healthcheck.Result;
+import org.apache.james.domainlist.api.mock.SimpleDomainList;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+public class LdapHealthCheckTest {
+
+ LdapHealthCheck ldapHealthCheck;
+ ReadOnlyUsersLDAPRepository ldapUserRepository;
+ LdapGenericContainer ldapContainer;
+
+ @BeforeEach
+ public void setUp() {
+ ldapUserRepository = new ReadOnlyUsersLDAPRepository(new SimpleDomainList());
+ ldapHealthCheck = new LdapHealthCheck(ldapUserRepository);
+ ldapContainer = DockerLdapSingleton.ldapContainer;
+ }
+
+ @Test
+ void checkShouldReturnUnhealthyIfLdapIsDown() {
+ ldapContainer.pause();
+
+ Result checkResult = ldapHealthCheck.check().block();
+ assertNotNull(checkResult);
+ assertTrue(checkResult.isUnHealthy());
Review Comment:
Use assertj library:
```suggestion
assertThat(checkResult.isUnHealthy()).isTrue();
```
This is a common project practice and it tends to be more readable than Junit assertions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] chibenwa merged pull request #1453: JAMES-2601 Implement a LDAP healthCheck
Posted by "chibenwa (via GitHub)" <gi...@apache.org>.
chibenwa merged PR #1453:
URL: https://github.com/apache/james-project/pull/1453
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] HoussemNasri commented on a diff in pull request #1453: JAMES-2601 Implement a LDAP healthCheck
Posted by "HoussemNasri (via GitHub)" <gi...@apache.org>.
HoussemNasri commented on code in PR #1453:
URL: https://github.com/apache/james-project/pull/1453#discussion_r1114164465
##########
server/data/data-ldap/src/main/java/org/apache/james/user/ldap/LdapHealthCheck.java:
##########
@@ -0,0 +1,38 @@
+package org.apache.james.user.ldap;
+
+import javax.inject.Inject;
+
+import org.apache.james.core.Username;
+import org.apache.james.core.healthcheck.ComponentName;
+import org.apache.james.core.healthcheck.HealthCheck;
+import org.apache.james.core.healthcheck.Result;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import reactor.core.publisher.Mono;
+
+public class LdapHealthCheck implements HealthCheck {
+ public static final ComponentName COMPONENT_NAME = new ComponentName("LDAP User Server");
+ public static final Username LDAP_TEST_USER = Username.of("ldap-test");
+ private static final Logger LOGGER = LoggerFactory.getLogger(LdapHealthCheck.class);
+
+ private final ReadOnlyUsersLDAPRepository ldapUserRepository;
+
+ @Inject
+ public LdapHealthCheck(ReadOnlyUsersLDAPRepository ldapUserRepository) {
+ this.ldapUserRepository = ldapUserRepository;
+ }
+
+ @Override
+ public ComponentName componentName() {
+ return COMPONENT_NAME;
+ }
+
+ @Override
+ public Mono<Result> check() {
+ return Mono.fromCallable(() -> ldapUserRepository.getUserByName(LDAP_TEST_USER))
Review Comment:
> Please handle the case "user not found" and return healthy in this case
I think it's already handled. Since `getUserByName` returns null if the user is not found, so the final result should be healthy as long the repository method doesn't throw an exception. This is the first time I use the reactor library, so there might be a flaw in my logic.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] HoussemNasri commented on pull request #1453: JAMES-2601 Implement a LDAP healthCheck
Posted by "HoussemNasri (via GitHub)" <gi...@apache.org>.
HoussemNasri commented on PR #1453:
URL: https://github.com/apache/james-project/pull/1453#issuecomment-1443640704
Yes, please go ahead
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] Arsnael commented on pull request #1453: JAMES-2601 Implement a LDAP healthCheck
Posted by "Arsnael (via GitHub)" <gi...@apache.org>.
Arsnael commented on PR #1453:
URL: https://github.com/apache/james-project/pull/1453#issuecomment-1441107895
```
21:31:35,552 [ERROR] Failed to execute goal org.apache.maven.plugins:maven-checkstyle-plugin:3.1.2:check (check-style) on project james-server-guice-data-ldap: You have 1 Checkstyle violation. -> [Help 1]
org.apache.maven.lifecycle.LifecycleExecutionException: Failed to execute goal org.apache.maven.plugins:maven-checkstyle-plugin:3.1.2:check (check-style) on project james-server-guice-data-ldap: You have 1 Checkstyle violation.
```
Please help fixing the checkstyle violation, thanks !
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] chibenwa commented on pull request #1453: JAMES-2601 Implement a LDAP healthCheck
Posted by "chibenwa (via GitHub)" <gi...@apache.org>.
chibenwa commented on PR #1453:
URL: https://github.com/apache/james-project/pull/1453#issuecomment-1442745590
@HoussemNasri do you agree with the fixes? Can we merge this?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] HoussemNasri commented on a diff in pull request #1453: JAMES-2601 Implement a LDAP healthCheck
Posted by "HoussemNasri (via GitHub)" <gi...@apache.org>.
HoussemNasri commented on code in PR #1453:
URL: https://github.com/apache/james-project/pull/1453#discussion_r1114182909
##########
server/container/guice/data-ldap/src/main/java/org/apache/james/data/LdapUsersRepositoryModule.java:
##########
@@ -43,6 +45,7 @@ public void configure() {
bind(UsersRepository.class).to(ReadOnlyUsersLDAPRepository.class);
bind(DelegationStore.class).to(NaiveDelegationStore.class);
bind(Authorizator.class).to(UserRepositoryAuthorizator.class);
+ bind(HealthCheck.class).to(LdapHealthCheck.class);
Review Comment:
Ah right! because we need to inject the list of health check implementations somewhere else in the code.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org