From 08fe5a89e9b6e13c72b0ec6e4c57ad11c26e1c07 Mon Sep 17 00:00:00 2001 From: Spencer Gibb Date: Mon, 12 Dec 2016 15:00:06 -0700 Subject: [PATCH] Move SendErrorFilter to type error. This will clear up empty 200 responses. fixes gh-1123 --- .../zuul/filters/post/SendErrorFilter.java | 45 +++-- .../zuul/filters/post/SendResponseFilter.java | 8 +- .../filters/route/RibbonRoutingFilter.java | 18 +- .../route/SimpleHostRoutingFilter.java | 6 +- .../support/AbstractRibbonCommandFactory.java | 2 +- .../zuul/util/ZuulRuntimeException.java | 35 ++++ .../post/SendErrorFilterIntegrationTests.java | 164 ++++++++++++++++++ .../filters/post/SendErrorFilterTests.java | 5 +- .../HttpClientRibbonCommandFallbackTests.java | 9 +- .../support/RibbonCommandFallbackTests.java | 5 +- 10 files changed, 260 insertions(+), 37 deletions(-) create mode 100644 spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/zuul/util/ZuulRuntimeException.java create mode 100644 spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/zuul/filters/post/SendErrorFilterIntegrationTests.java diff --git a/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/zuul/filters/post/SendErrorFilter.java b/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/zuul/filters/post/SendErrorFilter.java index 547bdd3d..eef14020 100644 --- a/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/zuul/filters/post/SendErrorFilter.java +++ b/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/zuul/filters/post/SendErrorFilter.java @@ -18,18 +18,23 @@ package org.springframework.cloud.netflix.zuul.filters.post; import javax.servlet.RequestDispatcher; import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; +import com.netflix.zuul.exception.ZuulException; import lombok.extern.apachecommons.CommonsLog; import org.springframework.beans.factory.annotation.Value; +import org.springframework.cloud.netflix.zuul.util.ZuulRuntimeException; import org.springframework.util.ReflectionUtils; import com.netflix.zuul.ZuulFilter; import com.netflix.zuul.context.RequestContext; +import org.springframework.util.StringUtils; /** * @author Spencer Gibb */ +//TODO: move to error package in Edgware @CommonsLog public class SendErrorFilter extends ZuulFilter { @@ -40,7 +45,7 @@ public class SendErrorFilter extends ZuulFilter { @Override public String filterType() { - return "post"; + return "error"; } @Override @@ -52,7 +57,7 @@ public class SendErrorFilter extends ZuulFilter { public boolean shouldFilter() { RequestContext ctx = RequestContext.getCurrentContext(); // only forward to errorPath if it hasn't been forwarded to already - return ctx.containsKey("error.status_code") + return ctx.getThrowable() != null && !ctx.getBoolean(SEND_ERROR_FILTER_RAN, false); } @@ -60,20 +65,16 @@ public class SendErrorFilter extends ZuulFilter { public Object run() { try { RequestContext ctx = RequestContext.getCurrentContext(); + ZuulException exception = findZuulException(ctx.getThrowable()); HttpServletRequest request = ctx.getRequest(); - int statusCode = (Integer) ctx.get("error.status_code"); - request.setAttribute("javax.servlet.error.status_code", statusCode); + request.setAttribute("javax.servlet.error.status_code", exception.nStatusCode); - if (ctx.containsKey("error.exception")) { - Object e = ctx.get("error.exception"); - log.warn("Error during filtering", Throwable.class.cast(e)); - request.setAttribute("javax.servlet.error.exception", e); - } + log.warn("Error during filtering", exception); + request.setAttribute("javax.servlet.error.exception", exception); - if (ctx.containsKey("error.message")) { - String message = (String) ctx.get("error.message"); - request.setAttribute("javax.servlet.error.message", message); + if (StringUtils.hasText(exception.errorCause)) { + request.setAttribute("javax.servlet.error.message", exception.errorCause); } RequestDispatcher dispatcher = request.getRequestDispatcher( @@ -91,6 +92,26 @@ public class SendErrorFilter extends ZuulFilter { return null; } + ZuulException findZuulException(Throwable throwable) { + if (throwable.getCause() instanceof ZuulRuntimeException) { + // this was a failure initiated by one of the local filters + return (ZuulException) throwable.getCause().getCause(); + } + + if (throwable.getCause() instanceof ZuulException) { + // wrapped zuul exception + return (ZuulException) throwable.getCause(); + } + + if (throwable instanceof ZuulException) { + // exception thrown by zuul lifecycle + return (ZuulException) throwable; + } + + // fallback, should never get here + return new ZuulException(throwable, HttpServletResponse.SC_INTERNAL_SERVER_ERROR, null); + } + public void setErrorPath(String errorPath) { this.errorPath = errorPath; } diff --git a/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/zuul/filters/post/SendResponseFilter.java b/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/zuul/filters/post/SendResponseFilter.java index 7c846571..1bc1d54e 100644 --- a/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/zuul/filters/post/SendResponseFilter.java +++ b/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/zuul/filters/post/SendResponseFilter.java @@ -77,9 +77,11 @@ public class SendResponseFilter extends ZuulFilter { @Override public boolean shouldFilter() { - return !RequestContext.getCurrentContext().getZuulResponseHeaders().isEmpty() - || RequestContext.getCurrentContext().getResponseDataStream() != null - || RequestContext.getCurrentContext().getResponseBody() != null; + RequestContext context = RequestContext.getCurrentContext(); + return context.getThrowable() == null + && (!context.getZuulResponseHeaders().isEmpty() + || context.getResponseDataStream() != null + || context.getResponseBody() != null); } @Override diff --git a/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/zuul/filters/route/RibbonRoutingFilter.java b/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/zuul/filters/route/RibbonRoutingFilter.java index fabbf543..ed5189df 100644 --- a/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/zuul/filters/route/RibbonRoutingFilter.java +++ b/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/zuul/filters/route/RibbonRoutingFilter.java @@ -16,29 +16,32 @@ package org.springframework.cloud.netflix.zuul.filters.route; -import lombok.extern.apachecommons.CommonsLog; - import java.io.IOException; import java.io.InputStream; import java.util.List; import java.util.Map; + import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; + import org.springframework.cloud.netflix.ribbon.support.RibbonRequestCustomizer; import org.springframework.cloud.netflix.zuul.filters.ProxyRequestHelper; +import org.springframework.cloud.netflix.zuul.util.ZuulRuntimeException; import org.springframework.http.HttpStatus; import org.springframework.http.client.ClientHttpResponse; import org.springframework.util.MultiValueMap; + import com.netflix.client.ClientException; import com.netflix.hystrix.exception.HystrixRuntimeException; import com.netflix.zuul.ZuulFilter; import com.netflix.zuul.context.RequestContext; import com.netflix.zuul.exception.ZuulException; +import lombok.extern.apachecommons.CommonsLog; + @CommonsLog public class RibbonRoutingFilter extends ZuulFilter { - private static final String ERROR_STATUS_CODE = "error.status_code"; protected ProxyRequestHelper helper; protected RibbonCommandFactory ribbonCommandFactory; protected List requestCustomizers; @@ -90,16 +93,11 @@ public class RibbonRoutingFilter extends ZuulFilter { return response; } catch (ZuulException ex) { - context.set(ERROR_STATUS_CODE, ex.nStatusCode); - context.set("error.message", ex.errorCause); - context.set("error.exception", ex); + throw new ZuulRuntimeException(ex); } catch (Exception ex) { - context.set("error.status_code", - HttpServletResponse.SC_INTERNAL_SERVER_ERROR); - context.set("error.exception", ex); + throw new ZuulRuntimeException(ex); } - return null; } protected RibbonCommandContext buildCommandContext(RequestContext context) { diff --git a/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/zuul/filters/route/SimpleHostRoutingFilter.java b/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/zuul/filters/route/SimpleHostRoutingFilter.java index a76a5dd5..9e5a6ec6 100644 --- a/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/zuul/filters/route/SimpleHostRoutingFilter.java +++ b/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/zuul/filters/route/SimpleHostRoutingFilter.java @@ -34,7 +34,6 @@ import javax.net.ssl.SSLContext; import javax.net.ssl.TrustManager; import javax.net.ssl.X509TrustManager; import javax.servlet.http.HttpServletRequest; -import javax.servlet.http.HttpServletResponse; import org.apache.http.Header; import org.apache.http.HttpHost; @@ -69,6 +68,7 @@ import org.apache.http.protocol.HttpContext; import org.springframework.cloud.netflix.zuul.filters.ProxyRequestHelper; import org.springframework.cloud.netflix.zuul.filters.ZuulProperties; import org.springframework.cloud.netflix.zuul.filters.ZuulProperties.Host; +import org.springframework.cloud.netflix.zuul.util.ZuulRuntimeException; import org.springframework.util.LinkedMultiValueMap; import org.springframework.util.MultiValueMap; import org.springframework.util.StringUtils; @@ -91,7 +91,6 @@ public class SimpleHostRoutingFilter extends ZuulFilter { private static final DynamicIntProperty CONNECTION_TIMEOUT = DynamicPropertyFactory .getInstance() .getIntProperty(ZuulConstants.ZUUL_HOST_CONNECT_TIMEOUT_MILLIS, 2000); - private static final String ERROR_STATUS_CODE = "error.status_code"; private final Timer connectionManagerTimer = new Timer( "SimpleHostRoutingFilter.connectionManagerTimer", true); @@ -182,8 +181,7 @@ public class SimpleHostRoutingFilter extends ZuulFilter { setResponse(response); } catch (Exception ex) { - context.set(ERROR_STATUS_CODE, HttpServletResponse.SC_INTERNAL_SERVER_ERROR); - context.set("error.exception", ex); + throw new ZuulRuntimeException(ex); } return null; } diff --git a/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/zuul/filters/route/support/AbstractRibbonCommandFactory.java b/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/zuul/filters/route/support/AbstractRibbonCommandFactory.java index 537ad91c..11c97a21 100644 --- a/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/zuul/filters/route/support/AbstractRibbonCommandFactory.java +++ b/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/zuul/filters/route/support/AbstractRibbonCommandFactory.java @@ -32,7 +32,7 @@ public abstract class AbstractRibbonCommandFactory implements RibbonCommandFacto private Map fallbackProviderCache; public AbstractRibbonCommandFactory(Set fallbackProviders){ - this.fallbackProviderCache = new HashMap(); + this.fallbackProviderCache = new HashMap<>(); for(ZuulFallbackProvider provider : fallbackProviders) { fallbackProviderCache.put(provider.getRoute(), provider); } diff --git a/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/zuul/util/ZuulRuntimeException.java b/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/zuul/util/ZuulRuntimeException.java new file mode 100644 index 00000000..d10bd4ac --- /dev/null +++ b/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/zuul/util/ZuulRuntimeException.java @@ -0,0 +1,35 @@ +/* + * Copyright 2013-2016 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 + * + * http://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.netflix.zuul.util; + +import com.netflix.zuul.exception.ZuulException; +import org.springframework.http.HttpStatus; + +/** + * @author Spencer Gibb + */ +public class ZuulRuntimeException extends RuntimeException { + + public ZuulRuntimeException(ZuulException cause) { + super(cause); + } + + public ZuulRuntimeException(Exception ex) { + this(new ZuulException(ex, HttpStatus.INTERNAL_SERVER_ERROR.value(), null)); + } +} diff --git a/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/zuul/filters/post/SendErrorFilterIntegrationTests.java b/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/zuul/filters/post/SendErrorFilterIntegrationTests.java new file mode 100644 index 00000000..777dd777 --- /dev/null +++ b/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/zuul/filters/post/SendErrorFilterIntegrationTests.java @@ -0,0 +1,164 @@ +/* + * Copyright 2013-2016 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 + * + * http://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.netflix.zuul.filters.post; + +import javax.servlet.http.HttpServletRequest; + +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.springframework.boot.SpringBootConfiguration; +import org.springframework.boot.autoconfigure.EnableAutoConfiguration; +import org.springframework.boot.context.embedded.LocalServerPort; +import org.springframework.boot.test.context.SpringBootTest; +import org.springframework.boot.test.web.client.TestRestTemplate; +import org.springframework.cloud.netflix.ribbon.RibbonClient; +import org.springframework.cloud.netflix.ribbon.StaticServerList; +import org.springframework.cloud.netflix.zuul.EnableZuulProxy; +import org.springframework.context.annotation.Bean; +import org.springframework.http.HttpStatus; +import org.springframework.http.ResponseEntity; +import org.springframework.test.annotation.DirtiesContext; +import org.springframework.test.context.junit4.SpringRunner; +import org.springframework.web.bind.annotation.RequestMapping; +import org.springframework.web.bind.annotation.RestController; + +import com.netflix.loadbalancer.Server; +import com.netflix.loadbalancer.ServerList; +import com.netflix.zuul.ZuulFilter; +import com.netflix.zuul.context.RequestContext; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.springframework.boot.test.context.SpringBootTest.WebEnvironment.RANDOM_PORT; + +/** + * @author Spencer Gibb + */ +@RunWith(SpringRunner.class) +@SpringBootTest(classes = SendErrorFilterIntegrationTests.Config.class, + properties = "zuul.routes.filtertest:/filtertest/**", + webEnvironment = RANDOM_PORT) +@DirtiesContext +public class SendErrorFilterIntegrationTests { + + @LocalServerPort + private int port; + + @Before + public void setTestRequestcontext() { + RequestContext context = new RequestContext(); + RequestContext.testSetCurrentContext(context); + } + + @Test + public void testPreFails() { + String url = "http://localhost:" + port + "/filtertest/get?failpre=true"; + ResponseEntity response = new TestRestTemplate().getForEntity(url, String.class); + assertThat(response.getStatusCode()).isEqualTo(HttpStatus.INTERNAL_SERVER_ERROR); + } + + @Test + public void testRouteFails() { + String url = "http://localhost:" + port + "/filtertest/get?failroute=true"; + ResponseEntity response = new TestRestTemplate().getForEntity(url, String.class); + assertThat(response.getStatusCode()).isEqualTo(HttpStatus.INTERNAL_SERVER_ERROR); + } + + @Test + public void testPostFails() { + String url = "http://localhost:" + port + "/filtertest/get?failpost=true"; + ResponseEntity response = new TestRestTemplate().getForEntity(url, String.class); + assertThat(response.getStatusCode()).isEqualTo(HttpStatus.INTERNAL_SERVER_ERROR); + } + + @SpringBootConfiguration + @EnableAutoConfiguration + @EnableZuulProxy + @RestController + @RibbonClient(name = "filtertest", configuration = RibbonConfig.class) + protected static class Config { + + @RequestMapping("/get") + public String get() { + return "Hello"; + } + + @Bean + public ZuulFilter testPreFilter() { + return new FailureFilter() { + @Override + public String filterType() { + return "pre"; + } + }; + } + + @Bean + public ZuulFilter testRouteFilter() { + return new FailureFilter() { + @Override + public String filterType() { + return "route"; + } + }; + } + + @Bean + public ZuulFilter testPostFilter() { + return new FailureFilter() { + @Override + public String filterType() { + return "post"; + } + }; + } + } + + public static class RibbonConfig { + @LocalServerPort + private int port; + + @Bean + public ServerList ribbonServerList() { + return new StaticServerList<>(new Server("localhost", this.port)); + } + + } + + private abstract static class FailureFilter extends ZuulFilter { + @Override + public int filterOrder() { + return Integer.MIN_VALUE; + } + + @Override + public boolean shouldFilter() { + HttpServletRequest request = RequestContext.getCurrentContext().getRequest(); + return request.getParameter("fail"+filterType()) != null; + } + + @Override + public Object run() { + HttpServletRequest request = RequestContext.getCurrentContext().getRequest(); + if (request.getParameter("fail"+filterType()) != null) { + throw new RuntimeException("failing on purpose in " + filterType()); + } + return null; + } + } +} diff --git a/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/zuul/filters/post/SendErrorFilterTests.java b/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/zuul/filters/post/SendErrorFilterTests.java index ce270fb4..643a0152 100644 --- a/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/zuul/filters/post/SendErrorFilterTests.java +++ b/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/zuul/filters/post/SendErrorFilterTests.java @@ -18,6 +18,8 @@ package org.springframework.cloud.netflix.zuul.filters.post; import javax.servlet.http.HttpServletRequest; +import com.netflix.zuul.exception.ZuulException; +import com.netflix.zuul.monitoring.MonitoringHelper; import org.junit.After; import org.junit.Before; import org.junit.Test; @@ -38,6 +40,7 @@ public class SendErrorFilterTests { @Before public void setTestRequestcontext() { + MonitoringHelper.initMocks(); RequestContext context = new RequestContext(); RequestContext.testSetCurrentContext(context); } @@ -58,7 +61,7 @@ public class SendErrorFilterTests { RequestContext context = new RequestContext(); context.setRequest(request); context.setResponse(new MockHttpServletResponse()); - context.set("error.status_code", HttpStatus.NOT_FOUND.value()); + context.setThrowable(new ZuulException(new RuntimeException(), HttpStatus.NOT_FOUND.value(), null)); RequestContext.testSetCurrentContext(context); SendErrorFilter filter = new SendErrorFilter(); filter.setErrorPath("/error"); diff --git a/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/zuul/filters/route/apache/HttpClientRibbonCommandFallbackTests.java b/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/zuul/filters/route/apache/HttpClientRibbonCommandFallbackTests.java index ea30e4e1..43a94807 100644 --- a/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/zuul/filters/route/apache/HttpClientRibbonCommandFallbackTests.java +++ b/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/zuul/filters/route/apache/HttpClientRibbonCommandFallbackTests.java @@ -23,15 +23,18 @@ import org.junit.runner.RunWith; import org.springframework.boot.test.context.SpringBootTest; import org.springframework.cloud.netflix.zuul.filters.route.support.RibbonCommandFallbackTests; import org.springframework.test.annotation.DirtiesContext; -import org.springframework.test.context.junit4.SpringJUnit4ClassRunner; +import org.springframework.test.context.junit4.SpringRunner; import com.netflix.zuul.context.RequestContext; +import static org.springframework.boot.test.context.SpringBootTest.WebEnvironment.RANDOM_PORT; + /** * @author Ryan Baxter */ -@RunWith(SpringJUnit4ClassRunner.class) -@SpringBootTest(classes = HttpClientRibbonCommandIntegrationTests.TestConfig.class, webEnvironment = SpringBootTest.WebEnvironment.RANDOM_PORT, value = { +@RunWith(SpringRunner.class) +@SpringBootTest(classes = HttpClientRibbonCommandIntegrationTests.TestConfig.class, webEnvironment = RANDOM_PORT, + properties = { "zuul.routes.simple: /simple/**", "zuul.routes.another: /another/twolevel/**", "ribbon.ReadTimeout: 1"}) @DirtiesContext diff --git a/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/zuul/filters/route/support/RibbonCommandFallbackTests.java b/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/zuul/filters/route/support/RibbonCommandFallbackTests.java index 303241e1..205204f4 100644 --- a/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/zuul/filters/route/support/RibbonCommandFallbackTests.java +++ b/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/zuul/filters/route/support/RibbonCommandFallbackTests.java @@ -19,7 +19,7 @@ package org.springframework.cloud.netflix.zuul.filters.route.support; import org.junit.Test; -import org.springframework.beans.factory.annotation.Value; +import org.springframework.boot.context.embedded.LocalServerPort; import org.springframework.boot.test.web.client.TestRestTemplate; import org.springframework.http.HttpEntity; import org.springframework.http.HttpMethod; @@ -33,7 +33,7 @@ import static org.junit.Assert.assertEquals; */ public abstract class RibbonCommandFallbackTests { - @Value("${local.server.port}") + @LocalServerPort protected int port; @Test @@ -52,7 +52,6 @@ public abstract class RibbonCommandFallbackTests { ResponseEntity result = new TestRestTemplate().exchange( "http://localhost:" + this.port + uri, HttpMethod.GET, new HttpEntity<>((Void) null), String.class); - System.out.println("no fallback body: " + result.getBody()); assertEquals(HttpStatus.INTERNAL_SERVER_ERROR, result.getStatusCode()); } }