From f6bfd6e474fbbc0f627d9bd6bc6c1c80c7f393c9 Mon Sep 17 00:00:00 2001 From: Ignacio Lozano Date: Tue, 27 Jul 2021 11:34:04 +0200 Subject: [PATCH] Enrich error response for invalid filter/predicate requests in actuator In the previous implementation, server responds 400 Bad Request when no existing predicate or filter matches with the specified in the request (See `GatewayControllerEndpointTests.testPostRouteWithNotExistingFilter` & `GatewayControllerEndpointTests.testPostRouteWithNotExistingPredicate`). This change replaces the filter by an exception that can take the advantage of the default `ExceptionHandler` including a body with more details specified by the server. In order to get the message that helps the client to diagnose the problem, the property `server.error.include-message=always` is required to be configured. Example: `POST /actuator/gateway/routes/invalid-route-test` request Response ``` HTTP 400 Bad Request Payload { "timestamp": "2021-07-27T08:55:35.596+00:00", "path": "/actuator/gateway/routes/invalid-route-test", "status": 400, "error": "Bad Request", "message": "Invalid FilterDefinition: [InvalidFilter]", "requestId": "e5551c86-6, L:/0:0:0:0:0:0:0:1:8080 - R:/0:0:0:0:0:0:0:1:50437" } ``` Fixes gh-2107 Fixes gh-2311 --- .../AbstractGatewayControllerEndpoint.java | 54 ++++++++++++++----- .../GatewayControllerEndpointTests.java | 6 ++- 2 files changed, 46 insertions(+), 14 deletions(-) diff --git a/spring-cloud-gateway-server/src/main/java/org/springframework/cloud/gateway/actuate/AbstractGatewayControllerEndpoint.java b/spring-cloud-gateway-server/src/main/java/org/springframework/cloud/gateway/actuate/AbstractGatewayControllerEndpoint.java index ccdf5ec8..fe96739a 100644 --- a/spring-cloud-gateway-server/src/main/java/org/springframework/cloud/gateway/actuate/AbstractGatewayControllerEndpoint.java +++ b/spring-cloud-gateway-server/src/main/java/org/springframework/cloud/gateway/actuate/AbstractGatewayControllerEndpoint.java @@ -19,6 +19,8 @@ package org.springframework.cloud.gateway.actuate; import java.net.URI; import java.util.HashMap; import java.util.List; +import java.util.Set; +import java.util.stream.Collectors; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; @@ -26,8 +28,10 @@ import reactor.core.publisher.Flux; import reactor.core.publisher.Mono; import org.springframework.cloud.gateway.event.RefreshRoutesEvent; +import org.springframework.cloud.gateway.filter.FilterDefinition; import org.springframework.cloud.gateway.filter.GlobalFilter; import org.springframework.cloud.gateway.filter.factory.GatewayFilterFactory; +import org.springframework.cloud.gateway.handler.predicate.PredicateDefinition; import org.springframework.cloud.gateway.handler.predicate.RoutePredicateFactory; import org.springframework.cloud.gateway.route.RouteDefinition; import org.springframework.cloud.gateway.route.RouteDefinitionLocator; @@ -37,12 +41,14 @@ import org.springframework.cloud.gateway.support.NotFoundException; import org.springframework.context.ApplicationEventPublisher; import org.springframework.context.ApplicationEventPublisherAware; import org.springframework.core.Ordered; +import org.springframework.http.HttpStatus; import org.springframework.http.ResponseEntity; import org.springframework.web.bind.annotation.DeleteMapping; import org.springframework.web.bind.annotation.GetMapping; import org.springframework.web.bind.annotation.PathVariable; import org.springframework.web.bind.annotation.PostMapping; import org.springframework.web.bind.annotation.RequestBody; +import org.springframework.web.server.ResponseStatusException; /** * @author Spencer Gibb @@ -129,8 +135,9 @@ public class AbstractGatewayControllerEndpoint implements ApplicationEventPublis @SuppressWarnings("unchecked") public Mono> save(@PathVariable String id, @RequestBody RouteDefinition route) { - return Mono.just(route).filter(this::validateRouteDefinition) - .flatMap(routeDefinition -> this.routeDefinitionWriter.save(Mono.just(routeDefinition).map(r -> { + return Mono.just(route).doOnNext(this::validateRouteDefinition) + .flatMap(routeDefinition -> this.routeDefinitionWriter + .save(Mono.just(routeDefinition).map(r -> { r.setId(id); log.debug("Saving route: " + route); return r; @@ -138,17 +145,40 @@ public class AbstractGatewayControllerEndpoint implements ApplicationEventPublis .switchIfEmpty(Mono.defer(() -> Mono.just(ResponseEntity.badRequest().build()))); } - private boolean validateRouteDefinition(RouteDefinition routeDefinition) { - boolean hasValidFilterDefinitions = routeDefinition.getFilters().stream() - .allMatch(filterDefinition -> GatewayFilters.stream().anyMatch( - gatewayFilterFactory -> filterDefinition.getName().equals(gatewayFilterFactory.name()))); + private void validateRouteDefinition(RouteDefinition routeDefinition) { + Set unavailableFilterDefinitions = routeDefinition.getFilters().stream() + .filter(rd -> !isAvailable(rd)).map(FilterDefinition::getName) + .collect(Collectors.toSet()); - boolean hasValidPredicateDefinitions = routeDefinition.getPredicates().stream() - .allMatch(predicateDefinition -> routePredicates.stream() - .anyMatch(routePredicate -> predicateDefinition.getName().equals(routePredicate.name()))); - log.debug("FilterDefinitions valid: " + hasValidFilterDefinitions); - log.debug("PredicateDefinitions valid: " + hasValidPredicateDefinitions); - return hasValidFilterDefinitions && hasValidPredicateDefinitions; + Set unavailablePredicatesDefinitions = routeDefinition.getPredicates() + .stream().filter(rd -> !isAvailable(rd)).map(PredicateDefinition::getName) + .collect(Collectors.toSet()); + if (!unavailableFilterDefinitions.isEmpty()) { + handleUnavailableDefinition(FilterDefinition.class.getSimpleName(), + unavailableFilterDefinitions); + } + else if (!unavailablePredicatesDefinitions.isEmpty()) { + handleUnavailableDefinition(PredicateDefinition.class.getSimpleName(), + unavailablePredicatesDefinitions); + } + } + + private void handleUnavailableDefinition(String simpleName, + Set unavailableDefinitions) { + final String errorMessage = String.format("Invalid %s: %s", simpleName, + unavailableDefinitions); + log.debug(errorMessage); + throw new ResponseStatusException(HttpStatus.BAD_REQUEST, errorMessage); + } + + private boolean isAvailable(FilterDefinition filterDefinition) { + return GatewayFilters.stream().anyMatch(gatewayFilterFactory -> filterDefinition + .getName().equals(gatewayFilterFactory.name())); + } + + private boolean isAvailable(PredicateDefinition predicateDefinition) { + return routePredicates.stream().anyMatch(routePredicate -> predicateDefinition + .getName().equals(routePredicate.name())); } @DeleteMapping("/routes/{id}") diff --git a/spring-cloud-gateway-server/src/test/java/org/springframework/cloud/gateway/actuate/GatewayControllerEndpointTests.java b/spring-cloud-gateway-server/src/test/java/org/springframework/cloud/gateway/actuate/GatewayControllerEndpointTests.java index da1e5617..fd06a155 100644 --- a/spring-cloud-gateway-server/src/test/java/org/springframework/cloud/gateway/actuate/GatewayControllerEndpointTests.java +++ b/spring-cloud-gateway-server/src/test/java/org/springframework/cloud/gateway/actuate/GatewayControllerEndpointTests.java @@ -151,7 +151,8 @@ public class GatewayControllerEndpointTests { testClient.post().uri("http://localhost:" + port + "/actuator/gateway/routes/test-route") .accept(MediaType.APPLICATION_JSON).body(BodyInserters.fromValue(testRouteDefinition)).exchange() - .expectStatus().isBadRequest(); + .expectStatus().isBadRequest().expectBody().jsonPath("$.message") + .isEqualTo("Invalid FilterDefinition: [NotExistingFilter]"); } @Test @@ -165,7 +166,8 @@ public class GatewayControllerEndpointTests { testClient.post().uri("http://localhost:" + port + "/actuator/gateway/routes/test-route") .accept(MediaType.APPLICATION_JSON).body(BodyInserters.fromValue(testRouteDefinition)).exchange() - .expectStatus().isBadRequest(); + .expectStatus().isBadRequest().expectBody().jsonPath("$.message") + .isEqualTo("Invalid PredicateDefinition: [NotExistingPredicate]"); } @SpringBootConfiguration