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

DHIS2-18568: allow Route response timeout to be customised #19872

Merged
merged 31 commits into from
Feb 17, 2025
Merged
Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
5e57ae5
feat(DHIS2-18568): allow response timeout to be customised
cjmamo Feb 5, 2025
88ff1b5
fix: persist response timeout
cjmamo Feb 6, 2025
f83be8e
fix: validate response timeout
cjmamo Feb 6, 2025
9bcb8cf
Merge branch 'master' into DHIS2-18568
cjmamo Feb 6, 2025
bf79d6c
fix: solve SonarQube issues
cjmamo Feb 6, 2025
e9cfe03
style: format
cjmamo Feb 6, 2025
039eba5
Merge branch 'master' into DHIS2-18568
cjmamo Feb 6, 2025
b40ee46
fix: update to newer patch version of Spring to solve reported securi…
cjmamo Feb 6, 2025
1d64c79
fix: validate Route response time update
cjmamo Feb 6, 2025
5c0206e
fix: handle max body size
cjmamo Feb 6, 2025
412ebd1
Merge branch 'master' into DHIS2-18568
cjmamo Feb 6, 2025
7ec47f8
refactor: eliminate code smell
cjmamo Feb 6, 2025
677b880
refactor: add DB constraint
cjmamo Feb 6, 2025
b058fff
Merge branch 'master' into DHIS2-18568
cjmamo Feb 7, 2025
266326b
refactor: rename responseTimeout to responseTimeoutSeconds
cjmamo Feb 10, 2025
990032f
Merge branch 'master' into DHIS2-18568
cjmamo Feb 10, 2025
85e607b
fix(security): reduce default to 5s after conversation with @amcgee
cjmamo Feb 10, 2025
db77f10
perf: stream request and response bodies
cjmamo Feb 11, 2025
972a3ae
Merge branch 'master' into DHIS2-18568
cjmamo Feb 11, 2025
b198d58
refactor: follow-up PR comments
cjmamo Feb 11, 2025
2005c82
fix: filter response headers
cjmamo Feb 11, 2025
173a24f
style: reformat
cjmamo Feb 11, 2025
77888e1
refactor: drop error handing for DataBufferLimitException since it ca…
cjmamo Feb 11, 2025
2a4b840
refactor: drop redundant close on output stream
cjmamo Feb 11, 2025
062fbbb
touch
cjmamo Feb 11, 2025
6990a99
style: reformat
cjmamo Feb 11, 2025
f747384
fix: clean up resources and bump request buffer size
cjmamo Feb 14, 2025
0b46ab5
Merge branch 'master' into DHIS2-18568
cjmamo Feb 14, 2025
c5fda78
chore: update dependencies to fix reported security issues
cjmamo Feb 14, 2025
d38e917
fix: limit total transfer time to 5 mins
cjmamo Feb 17, 2025
a255d2c
Merge branch 'master' into DHIS2-18568
cjmamo Feb 17, 2025
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
4 changes: 4 additions & 0 deletions dhis-2/dhis-api/src/main/java/org/hisp/dhis/route/Route.java
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@
@EqualsAndHashCode(callSuper = true)
@Accessors(chain = true)
public class Route extends BaseIdentifiableObject implements MetadataObject {
public static final String DEFAULT_RESPONSE_TIMEOUT_SECONDS = "10";
public static final String PATH_WILDCARD_SUFFIX = "/**";

@JsonProperty private String description;
Expand All @@ -67,6 +68,9 @@
/** Optional. Required authorities for invoking the route. */
@JsonProperty private List<String> authorities = new ArrayList<>();

@JsonProperty(defaultValue = DEFAULT_RESPONSE_TIMEOUT_SECONDS)
private int responseTimeoutSeconds = Integer.parseInt(DEFAULT_RESPONSE_TIMEOUT_SECONDS);

/**
* If the route url ends with /** return true. Otherwise return false.
*
Expand Down
29 changes: 21 additions & 8 deletions dhis-2/dhis-services/dhis-service-core/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -69,10 +69,6 @@
<artifactId>dhis-support-sql</artifactId>
</dependency>
<!-- Other -->
<dependency>
<groupId>org.apache.httpcomponents.core5</groupId>
<artifactId>httpcore5</artifactId>
</dependency>
<dependency>
<groupId>org.hisp.dhis.parser</groupId>
<artifactId>dhis-antlr-expression-parser</artifactId>
Expand All @@ -97,6 +93,26 @@
<groupId>org.springframework.security</groupId>
<artifactId>spring-security-oauth2-core</artifactId>
</dependency>
<dependency>
<groupId>org.springframework</groupId>
<artifactId>spring-webflux</artifactId>
<version>${spring.version}</version>
</dependency>
<dependency>
<groupId>io.projectreactor</groupId>
<artifactId>reactor-core</artifactId>
<version>3.6.9</version>
</dependency>
<dependency>
<groupId>io.projectreactor.netty</groupId>
<artifactId>reactor-netty-http</artifactId>
<version>1.2.2</version>
</dependency>
<dependency>
<groupId>io.netty</groupId>
<artifactId>netty-handler</artifactId>
<version>4.1.116.Final</version>
</dependency>
<dependency>
<groupId>com.nimbusds</groupId>
<artifactId>nimbus-jose-jwt</artifactId>
Expand Down Expand Up @@ -197,10 +213,6 @@
<groupId>com.google.code.gson</groupId>
<artifactId>gson</artifactId>
</dependency>
<dependency>
<groupId>org.apache.httpcomponents.client5</groupId>
<artifactId>httpclient5</artifactId>
</dependency>

<!-- Apache JClouds -->

Expand Down Expand Up @@ -290,6 +302,7 @@
<groupId>org.springframework</groupId>
<artifactId>spring-webmvc</artifactId>
</dependency>

<dependency>
<groupId>org.springframework.security</groupId>
<artifactId>spring-security-crypto</artifactId>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,9 +29,11 @@

import static org.hisp.dhis.config.HibernateEncryptionConfig.AES_128_STRING_ENCRYPTOR;

import io.netty.handler.timeout.ReadTimeoutException;
import jakarta.servlet.http.HttpServletRequest;
import java.io.IOException;
import java.nio.charset.StandardCharsets;
import java.time.Duration;
import java.util.Arrays;
import java.util.Collection;
import java.util.Collections;
Expand All @@ -45,31 +47,28 @@
import java.util.function.Function;
import javax.annotation.Nonnull;
import javax.annotation.PostConstruct;
import lombok.Getter;
import lombok.RequiredArgsConstructor;
import lombok.Setter;
import lombok.extern.slf4j.Slf4j;
import org.apache.hc.client5.http.config.ConnectionConfig;
import org.apache.hc.client5.http.config.RequestConfig;
import org.apache.hc.client5.http.impl.io.PoolingHttpClientConnectionManager;
import org.apache.hc.core5.http.io.SocketConfig;
import org.apache.hc.core5.util.Timeout;
import org.hibernate.Hibernate;
import org.hisp.dhis.feedback.BadRequestException;
import org.hisp.dhis.jsontree.Json;
import org.hisp.dhis.jsontree.JsonObject;
import org.hisp.dhis.user.UserDetails;
import org.jasypt.encryption.pbe.PBEStringCleanablePasswordEncryptor;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.beans.factory.annotation.Qualifier;
import org.springframework.http.HttpEntity;
import org.springframework.core.io.buffer.DataBufferLimitException;
import org.springframework.http.HttpHeaders;
import org.springframework.http.HttpMethod;
import org.springframework.http.HttpStatus;
import org.springframework.http.ResponseEntity;
import org.springframework.http.client.HttpComponentsClientHttpRequestFactory;
import org.springframework.http.client.reactive.ClientHttpConnector;
import org.springframework.stereotype.Service;
import org.springframework.util.StreamUtils;
import org.springframework.util.StringUtils;
import org.springframework.web.client.RestTemplate;
import org.springframework.web.reactive.function.client.WebClient;
import org.springframework.web.util.UriComponentsBuilder;
import reactor.core.publisher.Mono;
import reactor.netty.http.client.HttpClientRequest;

/**
* @author Morten Olav Hansen
Expand All @@ -85,7 +84,7 @@ public class RouteService {
@Qualifier(AES_128_STRING_ENCRYPTOR)
private final PBEStringCleanablePasswordEncryptor encryptor;

@Autowired @Getter @Setter private RestTemplate restTemplate;
private final ClientHttpConnector clientHttpConnector;

private static final Set<String> ALLOWED_REQUEST_HEADERS =
Set.of(
Expand Down Expand Up @@ -117,32 +116,11 @@ public class RouteService {
"cache-control",
"last-modified",
"etag");
private WebClient webClient;

@PostConstruct
public void postConstruct() {
// Connect timeout
ConnectionConfig connectionConfig =
ConnectionConfig.custom().setConnectTimeout(Timeout.ofMilliseconds(5_000)).build();

// Socket timeout
SocketConfig socketConfig =
SocketConfig.custom().setSoTimeout(Timeout.ofMilliseconds(10_000)).build();

// Connection request timeout
RequestConfig requestConfig =
RequestConfig.custom().setConnectionRequestTimeout(Timeout.ofMilliseconds(1_000)).build();

PoolingHttpClientConnectionManager connectionManager = new PoolingHttpClientConnectionManager();
connectionManager.setDefaultSocketConfig(socketConfig);
connectionManager.setDefaultConnectionConfig(connectionConfig);

org.apache.hc.client5.http.classic.HttpClient httpClient =
org.apache.hc.client5.http.impl.classic.HttpClientBuilder.create()
.setConnectionManager(connectionManager)
.setDefaultRequestConfig(requestConfig)
.build();

restTemplate.setRequestFactory(new HttpComponentsClientHttpRequestFactory(httpClient));
webClient = WebClient.builder().clientConnector(clientHttpConnector).build();
}

/**
Expand Down Expand Up @@ -224,24 +202,58 @@ public ResponseEntity<String> execute(

String body = StreamUtils.copyToString(request.getInputStream(), StandardCharsets.UTF_8);

HttpEntity<String> entity = new HttpEntity<>(body, headers);

HttpMethod httpMethod =
Objects.requireNonNullElse(HttpMethod.valueOf(request.getMethod()), HttpMethod.GET);
String targetUri = uriComponentsBuilder.toUriString();

WebClient.RequestHeadersSpec<?> requestHeadersSpec =
webClient
.method(httpMethod)
.uri(targetUri)
.httpRequest(
clientHttpRequest -> {
Object nativeRequest = clientHttpRequest.getNativeRequest();
if (nativeRequest instanceof HttpClientRequest httpClientRequest) {
httpClientRequest.responseTimeout(
Duration.ofSeconds(route.getResponseTimeoutSeconds()));
}
})
.bodyValue(body);
for (Map.Entry<String, List<String>> header : headers.entrySet()) {
requestHeadersSpec =
requestHeadersSpec.header(header.getKey(), header.getValue().toArray(new String[0]));
}

log.info(
"Sending '{}' '{}' with route '{}' ('{}')",
httpMethod,
targetUri,
route.getName(),
route.getUid());

ResponseEntity<String> response =
restTemplate.exchange(targetUri, httpMethod, entity, String.class);
WebClient.ResponseSpec responseSpec =
requestHeadersSpec
.retrieve()
.onStatus(httpStatusCode -> true, clientResponse -> Mono.empty());

ResponseEntity<String> response =
responseSpec
.toEntity(String.class)
.onErrorReturn(
throwable -> throwable.getCause() instanceof ReadTimeoutException,
new ResponseEntity<>(HttpStatus.GATEWAY_TIMEOUT))
.onErrorResume(
throwable -> throwable.getCause() instanceof DataBufferLimitException,
throwable -> {
JsonObject message =
Json.object(
jsonObjectBuilder ->
jsonObjectBuilder.addString(
"message", throwable.getCause().getMessage()));
return Mono.just(new ResponseEntity<>(message.toJson(), HttpStatus.BAD_GATEWAY));
})
.block();
HttpHeaders responseHeaders = filterResponseHeaders(response.getHeaders());

String responseBody = response.getBody();

log.info(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,5 +39,7 @@
<!-- Dynamic attribute values -->
<property name="attributeValues" column="attributevalues" type="jsbAttributeValues"/>

<property name="responseTimeoutSeconds" type="integer"/>

</class>
</hibernate-mapping>
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
ALTER TABLE route ADD COLUMN IF NOT EXISTS responsetimeoutseconds INTEGER NOT NULL DEFAULT 10 CHECK (responsetimeoutseconds > 0 AND responsetimeoutseconds <= 60);
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@

import org.springframework.context.annotation.Bean;
import org.springframework.context.annotation.Configuration;
import org.springframework.http.client.reactive.ClientHttpConnector;
import org.springframework.http.client.reactive.ReactorClientHttpConnector;
import org.springframework.web.client.RestTemplate;
import org.springframework.web.util.UriComponentsBuilder;

Expand All @@ -42,6 +44,11 @@ public RestTemplate restTemplate() {
return new RestTemplate();
}

@Bean
public ClientHttpConnector clientHttpConnector() {
return new ReactorClientHttpConnector();
}

@Bean
public UriComponentsBuilder uriComponentsBuilder() {
return UriComponentsBuilder.newInstance();
Expand Down
11 changes: 11 additions & 0 deletions dhis-2/dhis-test-web-api/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -361,6 +361,17 @@
</exclusion>
</exclusions>
</dependency>
<dependency>
<groupId>org.testcontainers</groupId>
<artifactId>testcontainers</artifactId>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.mock-server</groupId>
<artifactId>mockserver-client-java</artifactId>
<version>5.15.0</version>
<scope>test</scope>
</dependency>
</dependencies>

<build>
Expand Down
Loading
Loading