You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@james.apache.org by "chibenwa (via GitHub)" <gi...@apache.org> on 2023/02/22 02:24:24 UTC
[GitHub] [james-project] chibenwa commented on a diff in pull request #1453: JAMES-2601 Implement a LDAP healthCheck
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