From 05ba82855e8f6cf20a6eda35a6ad83cce302af57 Mon Sep 17 00:00:00 2001 From: Spencer Gibb Date: Thu, 27 Feb 2020 23:48:08 -0500 Subject: [PATCH] Makes failing on route definition errors opt-out. Since this is a behavior change, the new behavior needs to be opt in. See gh-1376 --- .../gateway/config/GatewayProperties.java | 23 +++- .../CompositeRouteDefinitionLocator.java | 7 +- .../route/RouteDefinitionRouteLocator.java | 35 +++--- .../RouteDefinitionRouteLocatorTests.java | 109 +++++------------- .../RouteConstructionIntegrationTests.java | 6 +- 5 files changed, 75 insertions(+), 105 deletions(-) diff --git a/spring-cloud-gateway-core/src/main/java/org/springframework/cloud/gateway/config/GatewayProperties.java b/spring-cloud-gateway-core/src/main/java/org/springframework/cloud/gateway/config/GatewayProperties.java index 6db4bac2..c67fc577 100644 --- a/spring-cloud-gateway-core/src/main/java/org/springframework/cloud/gateway/config/GatewayProperties.java +++ b/spring-cloud-gateway-core/src/main/java/org/springframework/cloud/gateway/config/GatewayProperties.java @@ -29,6 +29,7 @@ import org.apache.commons.logging.LogFactory; import org.springframework.boot.context.properties.ConfigurationProperties; import org.springframework.cloud.gateway.filter.FilterDefinition; import org.springframework.cloud.gateway.route.RouteDefinition; +import org.springframework.core.style.ToStringCreator; import org.springframework.http.MediaType; import org.springframework.validation.annotation.Validated; @@ -56,6 +57,12 @@ public class GatewayProperties { private List streamingMediaTypes = Arrays .asList(MediaType.TEXT_EVENT_STREAM, MediaType.APPLICATION_STREAM_JSON); + /** + * Option to fail on route definition errors, defaults to true. Otherwise, a warning + * is logged. + */ + private boolean failOnRouteDefinitionError = true; + public List getRoutes() { return routes; } @@ -83,10 +90,22 @@ public class GatewayProperties { this.streamingMediaTypes = streamingMediaTypes; } + public boolean isFailOnRouteDefinitionError() { + return failOnRouteDefinitionError; + } + + public void setFailOnRouteDefinitionError(boolean failOnRouteDefinitionError) { + this.failOnRouteDefinitionError = failOnRouteDefinitionError; + } + @Override public String toString() { - return "GatewayProperties{" + "routes=" + routes + ", defaultFilters=" - + defaultFilters + ", streamingMediaTypes=" + streamingMediaTypes + '}'; + return new ToStringCreator(this).append("routes", routes) + .append("defaultFilters", defaultFilters) + .append("streamingMediaTypes", streamingMediaTypes) + .append("failOnRouteDefinitionError", failOnRouteDefinitionError) + .toString(); + } } diff --git a/spring-cloud-gateway-core/src/main/java/org/springframework/cloud/gateway/route/CompositeRouteDefinitionLocator.java b/spring-cloud-gateway-core/src/main/java/org/springframework/cloud/gateway/route/CompositeRouteDefinitionLocator.java index 11df375c..4cafd929 100644 --- a/spring-cloud-gateway-core/src/main/java/org/springframework/cloud/gateway/route/CompositeRouteDefinitionLocator.java +++ b/spring-cloud-gateway-core/src/main/java/org/springframework/cloud/gateway/route/CompositeRouteDefinitionLocator.java @@ -55,7 +55,8 @@ public class CompositeRouteDefinitionLocator implements RouteDefinitionLocator { return randomId().map(id -> { routeDefinition.setId(id); if (log.isDebugEnabled()) { - log.debug("Id set on route definition: " + routeDefinition); + log.debug( + "Id set on route definition: " + routeDefinition); } return routeDefinition; }); @@ -65,6 +66,8 @@ public class CompositeRouteDefinitionLocator implements RouteDefinitionLocator { } protected Mono randomId() { - return Mono.fromSupplier(idGenerator::toString).publishOn(Schedulers.boundedElastic()); + return Mono.fromSupplier(idGenerator::toString) + .publishOn(Schedulers.boundedElastic()); } + } diff --git a/spring-cloud-gateway-core/src/main/java/org/springframework/cloud/gateway/route/RouteDefinitionRouteLocator.java b/spring-cloud-gateway-core/src/main/java/org/springframework/cloud/gateway/route/RouteDefinitionRouteLocator.java index 9870b257..f7ab0183 100644 --- a/spring-cloud-gateway-core/src/main/java/org/springframework/cloud/gateway/route/RouteDefinitionRouteLocator.java +++ b/spring-cloud-gateway-core/src/main/java/org/springframework/cloud/gateway/route/RouteDefinitionRouteLocator.java @@ -143,23 +143,26 @@ public class RouteDefinitionRouteLocator @Override public Flux getRoutes() { - return this.routeDefinitionLocator.getRouteDefinitions().map(this::convertToRoute) - .onErrorContinue((error, obj) -> { - if (logger.isWarnEnabled()) { - logger.warn("RouteDefinition id " + ((RouteDefinition) obj).getId() + " will be ignored. Definition has invalid configs, " + error.getMessage()); - } - }) - .map(route -> { - if (logger.isDebugEnabled()) { - logger.debug("RouteDefinition matched: " + route.getId()); - } - return route; - }); + Flux routes = this.routeDefinitionLocator.getRouteDefinitions() + .map(this::convertToRoute); - /* - * TODO: trace logging if (logger.isTraceEnabled()) { - * logger.trace("RouteDefinition did not match: " + routeDefinition.getId()); } - */ + if (!gatewayProperties.isFailOnRouteDefinitionError()) { + // instead of letting error bubble up, continue + routes = routes.onErrorContinue((error, obj) -> { + if (logger.isWarnEnabled()) { + logger.warn("RouteDefinition id " + ((RouteDefinition) obj).getId() + + " will be ignored. Definition has invalid configs, " + + error.getMessage()); + } + }); + } + + return routes.map(route -> { + if (logger.isDebugEnabled()) { + logger.debug("RouteDefinition matched: " + route.getId()); + } + return route; + }); } private Route convertToRoute(RouteDefinition routeDefinition) { diff --git a/spring-cloud-gateway-core/src/test/java/org/springframework/cloud/gateway/route/RouteDefinitionRouteLocatorTests.java b/spring-cloud-gateway-core/src/test/java/org/springframework/cloud/gateway/route/RouteDefinitionRouteLocatorTests.java index c2f6919d..504128cb 100644 --- a/spring-cloud-gateway-core/src/test/java/org/springframework/cloud/gateway/route/RouteDefinitionRouteLocatorTests.java +++ b/spring-cloud-gateway-core/src/test/java/org/springframework/cloud/gateway/route/RouteDefinitionRouteLocatorTests.java @@ -68,24 +68,23 @@ public class RouteDefinitionRouteLocatorTests { } })); - PropertiesRouteDefinitionLocator routeDefinitionLocator = new PropertiesRouteDefinitionLocator(gatewayProperties); + PropertiesRouteDefinitionLocator routeDefinitionLocator = new PropertiesRouteDefinitionLocator( + gatewayProperties); @SuppressWarnings("deprecation") RouteDefinitionRouteLocator routeDefinitionRouteLocator = new RouteDefinitionRouteLocator( new CompositeRouteDefinitionLocator(Flux.just(routeDefinitionLocator)), predicates, gatewayFilterFactories, gatewayProperties, new ConfigurationService()); - StepVerifier.create(routeDefinitionRouteLocator.getRoutes()) - .assertNext(route -> { - List filters = route.getFilters(); - assertThat(filters).hasSize(3); - assertThat(getFilterClassName(filters.get(0))).contains("RemoveResponseHeader"); - assertThat(getFilterClassName(filters.get(1))).contains("AddResponseHeader"); - assertThat(getFilterClassName(filters.get(2))) - .contains("RouteDefinitionRouteLocatorTests$TestOrderedGateway"); - }) - .expectComplete() - .verify(); + StepVerifier.create(routeDefinitionRouteLocator.getRoutes()).assertNext(route -> { + List filters = route.getFilters(); + assertThat(filters).hasSize(3); + assertThat(getFilterClassName(filters.get(0))) + .contains("RemoveResponseHeader"); + assertThat(getFilterClassName(filters.get(1))).contains("AddResponseHeader"); + assertThat(getFilterClassName(filters.get(2))) + .contains("RouteDefinitionRouteLocatorTests$TestOrderedGateway"); + }).expectComplete().verify(); } @Test @@ -98,99 +97,43 @@ public class RouteDefinitionRouteLocatorTests { new TestOrderedGatewayFilterFactory()); GatewayProperties gatewayProperties = new GatewayProperties(); gatewayProperties.setRoutes(containsInvalidRoutes()); + gatewayProperties.setFailOnRouteDefinitionError(false); - PropertiesRouteDefinitionLocator routeDefinitionLocator = new PropertiesRouteDefinitionLocator(gatewayProperties); + PropertiesRouteDefinitionLocator routeDefinitionLocator = new PropertiesRouteDefinitionLocator( + gatewayProperties); @SuppressWarnings("deprecation") RouteDefinitionRouteLocator routeDefinitionRouteLocator = new RouteDefinitionRouteLocator( new CompositeRouteDefinitionLocator(Flux.just(routeDefinitionLocator)), predicates, gatewayFilterFactories, gatewayProperties, new ConfigurationService()); - StepVerifier.create(routeDefinitionRouteLocator.getRoutes()) - .assertNext(route -> { - List filters = route.getFilters(); - assertThat(filters).hasSize(3); - assertThat(getFilterClassName(filters.get(0))).contains("RemoveResponseHeader"); - assertThat(getFilterClassName(filters.get(1))).contains("AddResponseHeader"); - assertThat(getFilterClassName(filters.get(2))) - .contains("RouteDefinitionRouteLocatorTests$TestOrderedGateway"); - }) - .expectComplete() - .verify(); + StepVerifier.create(routeDefinitionRouteLocator.getRoutes()).assertNext(route -> { + List filters = route.getFilters(); + assertThat(filters).hasSize(3); + assertThat(getFilterClassName(filters.get(0))) + .contains("RemoveResponseHeader"); + assertThat(getFilterClassName(filters.get(1))).contains("AddResponseHeader"); + assertThat(getFilterClassName(filters.get(2))) + .contains("RouteDefinitionRouteLocatorTests$TestOrderedGateway"); + }).expectComplete().verify(); } private List containsInvalidRoutes() { RouteDefinition foo = new RouteDefinition(); foo.setId("foo"); foo.setUri(URI.create("https://foo.example.com")); - foo.setPredicates( - Arrays.asList(new PredicateDefinition("Host=*.example.com"))); - foo.setFilters(Arrays.asList( - new FilterDefinition("RemoveResponseHeader=Server"), + foo.setPredicates(Arrays.asList(new PredicateDefinition("Host=*.example.com"))); + foo.setFilters(Arrays.asList(new FilterDefinition("RemoveResponseHeader=Server"), new FilterDefinition("TestOrdered="), new FilterDefinition("AddResponseHeader=X-Response-Foo, Bar"))); RouteDefinition bad = new RouteDefinition(); bad.setId("exceptionRaised"); bad.setUri(URI.create("https://foo.example.com")); - bad.setPredicates( - Arrays.asList(new PredicateDefinition("Host=*.example.com"))); + bad.setPredicates(Arrays.asList(new PredicateDefinition("Host=*.example.com"))); bad.setFilters(Arrays.asList(new FilterDefinition("Generate exception"))); return Arrays.asList(foo, bad); } - @Test - public void contextLoadsWithErrorRecovery() { - List predicates = Arrays - .asList(new HostRoutePredicateFactory()); - List gatewayFilterFactories = Arrays.asList( - new RemoveResponseHeaderGatewayFilterFactory(), - new AddResponseHeaderGatewayFilterFactory(), - new TestOrderedGatewayFilterFactory()); - GatewayProperties gatewayProperties = new GatewayProperties(); - gatewayProperties.setRoutes(containsInvalidRoutes()); - - RouteDefinitionRouteLocator routeDefinitionRouteLocator = new RouteDefinitionRouteLocator( - new PropertiesRouteDefinitionLocator(gatewayProperties), predicates, - gatewayFilterFactories, gatewayProperties, - new DefaultConversionService()); - - List routes = routeDefinitionRouteLocator.getRoutes().collectList() - .block(); - List filters = routes.get(0).getFilters(); - assertThat(filters).hasSize(3); - assertThat(getFilterClassName(filters.get(0))).contains("RemoveResponseHeader"); - assertThat(getFilterClassName(filters.get(1))).contains("AddResponseHeader"); - assertThat(getFilterClassName(filters.get(2))) - .contains("RouteDefinitionRouteLocatorTests$TestOrderedGateway"); - } - - private List containsInvalidRoutes() { - return Arrays.asList( - new RouteDefinition() { - { - setId("foo"); - setUri(URI.create("https://foo.example.com")); - setPredicates( - Arrays.asList(new PredicateDefinition("Host=*.example.com"))); - setFilters(Arrays.asList( - new FilterDefinition("RemoveResponseHeader=Server"), - new FilterDefinition("TestOrdered="), - new FilterDefinition("AddResponseHeader=X-Response-Foo, Bar"))); - } - }, - - new RouteDefinition() { - { - setId("exceptionRaised"); - setUri(URI.create("https://foo.example.com")); - setPredicates( - Arrays.asList(new PredicateDefinition("Host=*.example.com"))); - setFilters(Arrays.asList(new FilterDefinition("Generate exception"))); - } - } - ); - } - private String getFilterClassName(GatewayFilter target) { if (target instanceof OrderedGatewayFilter) { return getFilterClassName(((OrderedGatewayFilter) target).getDelegate()); diff --git a/spring-cloud-gateway-core/src/test/java/org/springframework/cloud/gateway/test/RouteConstructionIntegrationTests.java b/spring-cloud-gateway-core/src/test/java/org/springframework/cloud/gateway/test/RouteConstructionIntegrationTests.java index b6b65192..ffa9fe08 100644 --- a/spring-cloud-gateway-core/src/test/java/org/springframework/cloud/gateway/test/RouteConstructionIntegrationTests.java +++ b/spring-cloud-gateway-core/src/test/java/org/springframework/cloud/gateway/test/RouteConstructionIntegrationTests.java @@ -35,8 +35,7 @@ public class RouteConstructionIntegrationTests { @Test public void routesWithVerificationShouldFail() { exception.expect(Throwable.class); - new SpringApplicationBuilder(TestConfig.class) - .profiles("verification-route") + new SpringApplicationBuilder(TestConfig.class).profiles("verification-route") .run(); } @@ -74,6 +73,9 @@ public class RouteConstructionIntegrationTests { public void setArg1(String arg1) { this.arg1 = arg1; } + } + } + }