Skip to content

Commit

Permalink
Adds warning log if route contains bad regex
Browse files Browse the repository at this point in the history 
Fixes gh-2956
  • Loading branch information
@chenkunyun @spencergibb
chenkunyun authored and spencergibb committed Sep 5, 2018
1 parent 3ef01f7 commit 9287f44
Show file tree
Hide file tree
Showing 2 changed files with 64 additions and 32 deletions.
19 changes: 15 additions & 4 deletions ...zuul/src/main/java/org/springframework/cloud/netflix/zuul/filters/SimpleRouteLocator.java
View file
Expand Up @@ -69,7 +69,18 @@ public List<Route> getRoutes() {
for (Entry<String, ZuulRoute> entry : getRoutesMap().entrySet()) { for (Entry<String, ZuulRoute> entry : getRoutesMap().entrySet()) {
ZuulRoute route = entry.getValue(); ZuulRoute route = entry.getValue();
String path = route.getPath(); String path = route.getPath();
values.add(getRoute(route, path)); try {
values.add(getRoute(route, path));
}
catch (Exception e) {
if (log.isWarnEnabled()) {
log.warn("Invalid route, routeId: " + route.getId() + ", routeServiceId: "
+ route.getServiceId() + ", msg: " + e.getMessage());
}
if (log.isDebugEnabled()) {
log.debug("", e);
}
}
} }
return values; return values;
} }
Expand Down Expand Up @@ -139,7 +150,7 @@ protected Route getRoute(ZuulRoute route, String path) {
} }
String targetPath = path; String targetPath = path;
String prefix = this.properties.getPrefix(); String prefix = this.properties.getPrefix();
if(prefix.endsWith("/")) { if (prefix.endsWith("/")) {
prefix = prefix.substring(0, prefix.length() - 1); prefix = prefix.substring(0, prefix.length() - 1);
} }
if (path.startsWith(prefix + "/") && this.properties.isStripPrefix()) { if (path.startsWith(prefix + "/") && this.properties.isStripPrefix()) {
Expand All @@ -159,7 +170,7 @@ protected Route getRoute(ZuulRoute route, String path) {
} }
return new Route(route.getId(), targetPath, route.getLocation(), prefix, return new Route(route.getId(), targetPath, route.getLocation(), prefix,
retryable, retryable,
route.isCustomSensitiveHeaders() ? route.getSensitiveHeaders() : null, route.isCustomSensitiveHeaders() ? route.getSensitiveHeaders() : null,
route.isStripPrefix()); route.isStripPrefix());
} }


Expand Down Expand Up @@ -223,7 +234,7 @@ else if (RequestUtils.isZuulServletRequest()) {
public int getOrder() { public int getOrder() {
return order; return order;
} }

public void setOrder(int order) { public void setOrder(int order) {
this.order = order; this.order = order;
} }
Expand Down
77 changes: 49 additions & 28 deletions ...src/test/java/org/springframework/cloud/netflix/zuul/filters/SimpleRouteLocatorTests.java
View file
Expand Up @@ -16,43 +16,58 @@


package org.springframework.cloud.netflix.zuul.filters; package org.springframework.cloud.netflix.zuul.filters;


import java.util.ArrayList;
import java.util.List; import java.util.List;
import java.util.Map; import java.util.Map;
import java.util.Map.Entry; import java.util.stream.Collectors;

import org.junit.Before;
import org.junit.Rule;
import org.junit.Test;

import org.springframework.boot.test.rule.OutputCapture;
import org.springframework.cloud.netflix.zuul.filters.ZuulProperties.ZuulRoute;

import static org.hamcrest.CoreMatchers.containsString;
import static org.hamcrest.CoreMatchers.hasItem; import static org.hamcrest.CoreMatchers.hasItem;
import static org.hamcrest.CoreMatchers.is; import static org.hamcrest.CoreMatchers.is;
import static org.hamcrest.CoreMatchers.nullValue; import static org.hamcrest.CoreMatchers.nullValue;
import static org.hamcrest.collection.IsCollectionWithSize.hasSize; import static org.hamcrest.collection.IsCollectionWithSize.hasSize;
import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertThat; import static org.junit.Assert.assertThat;
import org.junit.Test;
import org.springframework.cloud.netflix.zuul.filters.ZuulProperties.ZuulRoute;


/** /**
* @author Tom Cawley * @author Tom Cawley
*/ */
public class SimpleRouteLocatorTests { public class SimpleRouteLocatorTests {
private ZuulProperties zuul = new ZuulProperties();
@Rule
public OutputCapture output = new OutputCapture();

private ZuulProperties properties;


public SimpleRouteLocatorTests() { public SimpleRouteLocatorTests() {
} }


@Before
public void init() {
properties = new ZuulProperties();
}

@Test @Test
public void test_getRoutesDefaultRouteAcceptor() { public void test_getRoutesDefaultRouteAcceptor() {
RouteLocator locator = new SimpleRouteLocator("/", this.zuul); RouteLocator locator = new SimpleRouteLocator("/", this.properties);
this.zuul.getRoutes().clear(); this.properties.getRoutes().clear();
this.zuul.getRoutes().put("foo", new ZuulRoute("/foo/**", "foo")); this.properties.getRoutes().put("foo", new ZuulRoute("/foo/**", "foo"));


assertThat(locator.getRoutes(), hasItem(createRoute("foo", "/**", "/foo"))); assertThat(locator.getRoutes(), hasItem(createRoute("foo", "/**", "/foo")));
} }


@Test @Test
public void test_getRoutesFilterRouteAcceptor() { public void test_getRoutesFilterRouteAcceptor() {
RouteLocator locator = new FilteringRouteLocator("/", this.zuul); RouteLocator locator = new FilteringRouteLocator("/", this.properties);
this.zuul.getRoutes().clear(); this.properties.getRoutes().clear();
this.zuul.getRoutes().put("foo", new ZuulRoute("/foo/**", "foo")); this.properties.getRoutes().put("foo", new ZuulRoute("/foo/**", "foo"));
this.zuul.getRoutes().put("bar", new ZuulRoute("/bar/**", "bar")); this.properties.getRoutes().put("bar", new ZuulRoute("/bar/**", "bar"));


final List<Route> routes = locator.getRoutes(); final List<Route> routes = locator.getRoutes();
assertThat(routes, hasItem(createRoute("bar", "/**", "/bar"))); assertThat(routes, hasItem(createRoute("bar", "/**", "/bar")));
Expand All @@ -61,7 +76,6 @@ public void test_getRoutesFilterRouteAcceptor() {


@Test @Test
public void testStripPrefix() { public void testStripPrefix() {
ZuulProperties properties = new ZuulProperties();
properties.setPrefix("/test"); properties.setPrefix("/test");
properties.setStripPrefix(true); properties.setStripPrefix(true);
RouteLocator locator = new FilteringRouteLocator("/", properties); RouteLocator locator = new FilteringRouteLocator("/", properties);
Expand All @@ -71,7 +85,6 @@ public void testStripPrefix() {


@Test @Test
public void testPrefix() { public void testPrefix() {
ZuulProperties properties = new ZuulProperties();
properties.setPrefix("/test/"); properties.setPrefix("/test/");
RouteLocator locator = new FilteringRouteLocator("/", properties); RouteLocator locator = new FilteringRouteLocator("/", properties);
properties.getRoutes().put("testservicea", new ZuulRoute("/testservicea/**", "testservicea")); properties.getRoutes().put("testservicea", new ZuulRoute("/testservicea/**", "testservicea"));
Expand All @@ -80,15 +93,26 @@ public void testPrefix() {


@Test @Test
public void test_getMatchingRouteFilterRouteAcceptor() { public void test_getMatchingRouteFilterRouteAcceptor() {
RouteLocator locator = new FilteringRouteLocator("/", this.zuul); RouteLocator locator = new FilteringRouteLocator("/", this.properties);
this.zuul.getRoutes().clear(); this.properties.getRoutes().clear();
this.zuul.getRoutes().put("foo", new ZuulRoute("/foo/**", "foo")); this.properties.getRoutes().put("foo", new ZuulRoute("/foo/**", "foo"));
this.zuul.getRoutes().put("bar", new ZuulRoute("/bar/**", "bar")); this.properties.getRoutes().put("bar", new ZuulRoute("/bar/**", "bar"));


assertThat(locator.getMatchingRoute("/foo/1"), nullValue()); assertThat(locator.getMatchingRoute("/foo/1"), nullValue());
assertThat(locator.getMatchingRoute("/bar/1"), is(createRoute("bar", "/1", "/bar"))); assertThat(locator.getMatchingRoute("/bar/1"), is(createRoute("bar", "/1", "/bar")));
} }


@Test
public void testBadRegex() {
this.properties.getRoutes().clear();
this.properties.getRoutes().put("foo", new ZuulRoute("/foo{}/**", "foo"));
RouteLocator locator = new FilteringRouteLocator("/", this.properties);
locator.getRoutes();

this.output.expect(containsString("Invalid route, "));

}

private Route createRoute(String id, String path, String prefix) { private Route createRoute(String id, String path, String prefix) {
return new Route(id, path, id, prefix, false, null); return new Route(id, path, id, prefix, false, null);
} }
Expand All @@ -100,16 +124,13 @@ public FilteringRouteLocator(String servletPath, ZuulProperties properties) {


@Override @Override
public List<Route> getRoutes() { public List<Route> getRoutes() {
List<Route> values = new ArrayList<>(); return super.getRoutes().stream()

.filter(this::acceptRoute)
for (Entry<String, ZuulRoute> entry : getRoutesMap().entrySet()) { .collect(Collectors.toList());
ZuulRoute route = entry.getValue(); }
if (acceptRoute(route)) {
String path = route.getPath(); private boolean acceptRoute(Route route) {
values.add(getRoute(route, path)); return route != null && !(route.getId().equals("foo"));
}
}
return values;
} }


private boolean acceptRoute(ZuulRoute route) { private boolean acceptRoute(ZuulRoute route) {
Expand Down

0 comments on commit 9287f44

Please sign in to comment.