From 75a34df7e2bdcaac4cac052e74642ed1b365665f Mon Sep 17 00:00:00 2001 From: MattyA Date: Mon, 17 Feb 2020 11:01:59 +0000 Subject: [PATCH] Moves routeId generation schedule to more specific location. This allows only the routeId generation to be on an elastic scheduler while everything else remains on the main thread. That way if there are errors loading routes, this will halt execution. Fixes gh-1574 Fixes gh-1575 --- .../CompositeRouteDefinitionLocator.java | 23 +++--- .../RouteDefinitionRouteLocatorTests.java | 81 ++++++++++++++++--- .../RouteConstructionIntegrationTests.java | 79 ++++++++++++++++++ .../application-verification-route.yml | 11 +++ 4 files changed, 172 insertions(+), 22 deletions(-) create mode 100644 spring-cloud-gateway-core/src/test/java/org/springframework/cloud/gateway/test/RouteConstructionIntegrationTests.java create mode 100644 spring-cloud-gateway-core/src/test/resources/application-verification-route.yml 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 5195b407..11df375c 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 @@ -50,18 +50,21 @@ public class CompositeRouteDefinitionLocator implements RouteDefinitionLocator { @Override public Flux getRouteDefinitions() { return this.delegates.flatMap(RouteDefinitionLocator::getRouteDefinitions) - .flatMap(routeDefinition -> Mono.justOrEmpty(routeDefinition.getId()) - .defaultIfEmpty(idGenerator.generateId().toString()) - .publishOn(Schedulers.elastic()).map(id -> { - if (routeDefinition.getId() == null) { - routeDefinition.setId(id); - if (log.isDebugEnabled()) { - log.debug("Id set on route definition: " - + routeDefinition); - } + .flatMap(routeDefinition -> { + if (routeDefinition.getId() == null) { + return randomId().map(id -> { + routeDefinition.setId(id); + if (log.isDebugEnabled()) { + log.debug("Id set on route definition: " + routeDefinition); } return routeDefinition; - })); + }); + } + return Mono.just(routeDefinition); + }); } + protected Mono randomId() { + return Mono.fromSupplier(idGenerator::toString).publishOn(Schedulers.boundedElastic()); + } } 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 f21a0a83..c2f6919d 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 @@ -21,6 +21,8 @@ import java.util.Arrays; import java.util.List; import org.junit.Test; +import reactor.core.publisher.Flux; +import reactor.test.StepVerifier; import org.springframework.cloud.gateway.config.GatewayProperties; import org.springframework.cloud.gateway.config.PropertiesRouteDefinitionLocator; @@ -34,7 +36,7 @@ import org.springframework.cloud.gateway.filter.factory.RemoveResponseHeaderGate import org.springframework.cloud.gateway.handler.predicate.HostRoutePredicateFactory; import org.springframework.cloud.gateway.handler.predicate.PredicateDefinition; import org.springframework.cloud.gateway.handler.predicate.RoutePredicateFactory; -import org.springframework.core.convert.support.DefaultConversionService; +import org.springframework.cloud.gateway.support.ConfigurationService; import org.springframework.util.StringUtils; import static org.assertj.core.api.Assertions.assertThat; @@ -66,19 +68,74 @@ public class RouteDefinitionRouteLocatorTests { } })); + PropertiesRouteDefinitionLocator routeDefinitionLocator = new PropertiesRouteDefinitionLocator(gatewayProperties); + @SuppressWarnings("deprecation") RouteDefinitionRouteLocator routeDefinitionRouteLocator = new RouteDefinitionRouteLocator( - new PropertiesRouteDefinitionLocator(gatewayProperties), predicates, - gatewayFilterFactories, gatewayProperties, - new DefaultConversionService()); + new CompositeRouteDefinitionLocator(Flux.just(routeDefinitionLocator)), + predicates, gatewayFilterFactories, gatewayProperties, + new ConfigurationService()); - 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"); + 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 + 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()); + + 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(); + } + + 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"), + 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.setFilters(Arrays.asList(new FilterDefinition("Generate exception"))); + return Arrays.asList(foo, bad); } @Test 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 new file mode 100644 index 00000000..b6b65192 --- /dev/null +++ b/spring-cloud-gateway-core/src/test/java/org/springframework/cloud/gateway/test/RouteConstructionIntegrationTests.java @@ -0,0 +1,79 @@ +/* + * Copyright 2013-2020 the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.springframework.cloud.gateway.test; + +import org.junit.Rule; +import org.junit.Test; +import org.junit.rules.ExpectedException; + +import org.springframework.boot.SpringBootConfiguration; +import org.springframework.boot.autoconfigure.EnableAutoConfiguration; +import org.springframework.boot.builder.SpringApplicationBuilder; +import org.springframework.cloud.gateway.filter.GatewayFilter; +import org.springframework.cloud.gateway.filter.factory.AbstractGatewayFilterFactory; +import org.springframework.context.annotation.Bean; + +public class RouteConstructionIntegrationTests { + + @Rule + public ExpectedException exception = ExpectedException.none(); + + @Test + public void routesWithVerificationShouldFail() { + exception.expect(Throwable.class); + new SpringApplicationBuilder(TestConfig.class) + .profiles("verification-route") + .run(); + } + + @EnableAutoConfiguration + @SpringBootConfiguration + public static class TestConfig { + + @Bean + public TestFilterGatewayFilterFactory testFilterGatewayFilterFactory() { + return new TestFilterGatewayFilterFactory(); + } + + } + + public static class TestFilterGatewayFilterFactory + extends AbstractGatewayFilterFactory { + + public TestFilterGatewayFilterFactory() { + super(Config.class); + } + + @Override + public GatewayFilter apply(Config config) { + throw new AssertionError("Stop right now!"); + } + + public static class Config { + + private String arg1; + + public String getArg1() { + return arg1; + } + + public void setArg1(String arg1) { + this.arg1 = arg1; + } + } + } +} diff --git a/spring-cloud-gateway-core/src/test/resources/application-verification-route.yml b/spring-cloud-gateway-core/src/test/resources/application-verification-route.yml new file mode 100644 index 00000000..c0f92d49 --- /dev/null +++ b/spring-cloud-gateway-core/src/test/resources/application-verification-route.yml @@ -0,0 +1,11 @@ +spring: + cloud: + gateway: + routes: + - uri: https://example.com + predicates: + - Path=/verification/** + filters: + - name: TestFilter + args: + arg1: world \ No newline at end of file