diff --git a/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/ribbon/apache/RetryableRibbonLoadBalancingHttpClient.java b/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/ribbon/apache/RetryableRibbonLoadBalancingHttpClient.java index 949343b7..3bff78b8 100644 --- a/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/ribbon/apache/RetryableRibbonLoadBalancingHttpClient.java +++ b/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/ribbon/apache/RetryableRibbonLoadBalancingHttpClient.java @@ -40,7 +40,6 @@ import org.springframework.retry.backoff.BackOffPolicy; import org.springframework.retry.backoff.NoBackOffPolicy; import org.springframework.retry.policy.NeverRetryPolicy; import org.springframework.retry.support.RetryTemplate; -import org.springframework.web.util.UriComponentsBuilder; import com.netflix.client.RequestSpecificRetryHandler; import com.netflix.client.RetryHandler; import com.netflix.client.config.CommonClientConfigKey; @@ -85,75 +84,55 @@ public class RetryableRibbonLoadBalancingHttpClient extends RibbonLoadBalancingH } @Override - public RibbonApacheHttpResponse execute(final RibbonApacheHttpRequest request, - final IClientConfig configOverride) throws Exception { + public RibbonApacheHttpResponse execute(final RibbonApacheHttpRequest request, final IClientConfig configOverride) throws Exception { final RequestConfig.Builder builder = RequestConfig.custom(); IClientConfig config = configOverride != null ? configOverride : this.config; - builder.setConnectTimeout( - config.get(CommonClientConfigKey.ConnectTimeout, this.connectTimeout)); - builder.setSocketTimeout( - config.get(CommonClientConfigKey.ReadTimeout, this.readTimeout)); - builder.setRedirectsEnabled( - config.get(CommonClientConfigKey.FollowRedirects, this.followRedirects)); + builder.setConnectTimeout(config.get( + CommonClientConfigKey.ConnectTimeout, this.connectTimeout)); + builder.setSocketTimeout(config.get( + CommonClientConfigKey.ReadTimeout, this.readTimeout)); + builder.setRedirectsEnabled(config.get( + CommonClientConfigKey.FollowRedirects, this.followRedirects)); final RequestConfig requestConfig = builder.build(); - final LoadBalancedRetryPolicy retryPolicy = loadBalancedRetryPolicyFactory - .create(this.getClientName(), this); + final LoadBalancedRetryPolicy retryPolicy = loadBalancedRetryPolicyFactory.create(this.getClientName(), this); RetryCallback retryCallback = new RetryCallback() { @Override - public RibbonApacheHttpResponse doWithRetry(RetryContext context) - throws Exception { - // on retries the policy will choose the server and set it in the context - // extract the server and update the request being made + public RibbonApacheHttpResponse doWithRetry(RetryContext context) throws Exception { + //on retries the policy will choose the server and set it in the context + //extract the server and update the request being made RibbonApacheHttpRequest newRequest = request; - if (context instanceof LoadBalancedRetryContext) { - ServiceInstance service = ((LoadBalancedRetryContext) context) - .getServiceInstance(); - if (service != null) { - // Reconstruct the request URI using the host and port set in the - // retry context - newRequest = newRequest.withNewUri(new URI( - service.getUri().getScheme(), - newRequest.getURI().getUserInfo(), service.getHost(), - service.getPort(), newRequest.getURI().getPath(), - newRequest.getURI().getQuery(), + if(context instanceof LoadBalancedRetryContext) { + ServiceInstance service = ((LoadBalancedRetryContext)context).getServiceInstance(); + if(service != null) { + //Reconstruct the request URI using the host and port set in the retry context + newRequest = newRequest.withNewUri(new URI(service.getUri().getScheme(), + newRequest.getURI().getUserInfo(), service.getHost(), service.getPort(), + newRequest.getURI().getPath(), newRequest.getURI().getQuery(), newRequest.getURI().getFragment())); } } - if (isSecure(configOverride)) { - final URI secureUri = UriComponentsBuilder - .fromUri(newRequest.getUri()).scheme("https").build().toUri(); - newRequest = newRequest.withNewUri(secureUri); - } + newRequest = getSecureRequest(request, configOverride); HttpUriRequest httpUriRequest = newRequest.toRequest(requestConfig); - final HttpResponse httpResponse = RetryableRibbonLoadBalancingHttpClient.this.delegate - .execute(httpUriRequest); - if (retryPolicy.retryableStatusCode( - httpResponse.getStatusLine().getStatusCode())) { - if (CloseableHttpResponse.class.isInstance(httpResponse)) { - ((CloseableHttpResponse) httpResponse).close(); + final HttpResponse httpResponse = RetryableRibbonLoadBalancingHttpClient.this.delegate.execute(httpUriRequest); + if(retryPolicy.retryableStatusCode(httpResponse.getStatusLine().getStatusCode())) { + if(CloseableHttpResponse.class.isInstance(httpResponse)) { + ((CloseableHttpResponse)httpResponse).close(); } - throw new RetryableStatusCodeException( - RetryableRibbonLoadBalancingHttpClient.this.clientName, + throw new RetryableStatusCodeException(RetryableRibbonLoadBalancingHttpClient.this.clientName, httpResponse.getStatusLine().getStatusCode()); } - return new RibbonApacheHttpResponse(httpResponse, - httpUriRequest.getURI()); + return new RibbonApacheHttpResponse(httpResponse, httpUriRequest.getURI()); } }; return this.executeWithRetry(request, retryPolicy, retryCallback); } - private RibbonApacheHttpResponse executeWithRetry(RibbonApacheHttpRequest request, - LoadBalancedRetryPolicy retryPolicy, - RetryCallback callback) - throws Exception { + private RibbonApacheHttpResponse executeWithRetry(RibbonApacheHttpRequest request, LoadBalancedRetryPolicy retryPolicy, RetryCallback callback) throws Exception { RetryTemplate retryTemplate = new RetryTemplate(); - boolean retryable = request.getContext() == null ? true - : BooleanUtils.toBooleanDefaultIfNull(request.getContext().getRetryable(), - true); - retryTemplate.setRetryPolicy(retryPolicy == null || !retryable - ? new NeverRetryPolicy() + boolean retryable = request.getContext() == null ? true : + BooleanUtils.toBooleanDefaultIfNull(request.getContext().getRetryable(), true); + retryTemplate.setRetryPolicy(retryPolicy == null || !retryable ? new NeverRetryPolicy() : new RetryPolicy(request, retryPolicy, this, this.getClientName())); BackOffPolicy backOffPolicy = loadBalancedBackOffPolicyFactory.createBackOffPolicy(this.getClientName()); retryTemplate.setBackOffPolicy(backOffPolicy == null ? new NoBackOffPolicy() : backOffPolicy); @@ -167,14 +146,12 @@ public class RetryableRibbonLoadBalancingHttpClient extends RibbonLoadBalancingH } @Override - public RequestSpecificRetryHandler getRequestSpecificRetryHandler( - RibbonApacheHttpRequest request, IClientConfig requestConfig) { + public RequestSpecificRetryHandler getRequestSpecificRetryHandler(RibbonApacheHttpRequest request, IClientConfig requestConfig) { return new RequestSpecificRetryHandler(false, false, RetryHandler.DEFAULT, null); } static class RetryPolicy extends FeignRetryPolicy { - public RetryPolicy(HttpRequest request, LoadBalancedRetryPolicy policy, - ServiceInstanceChooser serviceInstanceChooser, String serviceName) { + public RetryPolicy(HttpRequest request, LoadBalancedRetryPolicy policy, ServiceInstanceChooser serviceInstanceChooser, String serviceName) { super(request, policy, serviceInstanceChooser, serviceName); } } diff --git a/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/ribbon/apache/RibbonLoadBalancingHttpClient.java b/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/ribbon/apache/RibbonLoadBalancingHttpClient.java index 17eaa9af..1650120e 100644 --- a/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/ribbon/apache/RibbonLoadBalancingHttpClient.java +++ b/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/ribbon/apache/RibbonLoadBalancingHttpClient.java @@ -77,11 +77,7 @@ public class RibbonLoadBalancingHttpClient extends config.get(CommonClientConfigKey.FollowRedirects, this.followRedirects)); final RequestConfig requestConfig = builder.build(); - if (isSecure(configOverride)) { - final URI secureUri = UriComponentsBuilder.fromUri(request.getUri()) - .scheme("https").build().toUri(); - request = request.withNewUri(secureUri); - } + request = getSecureRequest(request, configOverride); final HttpUriRequest httpUriRequest = request.toRequest(requestConfig); final HttpResponse httpResponse = this.delegate.execute(httpUriRequest); return new RibbonApacheHttpResponse(httpResponse, httpUriRequest.getURI()); @@ -100,4 +96,13 @@ public class RibbonLoadBalancingHttpClient extends return new RequestSpecificRetryHandler(false, false, RetryHandler.DEFAULT, requestConfig); } + + protected RibbonApacheHttpRequest getSecureRequest(RibbonApacheHttpRequest request, IClientConfig configOverride) { + if (isSecure(configOverride)) { + final URI secureUri = UriComponentsBuilder.fromUri(request.getUri()) + .scheme("https").build(true).toUri(); + return request.withNewUri(secureUri); + } + return request; + } } diff --git a/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/ribbon/apache/RibbonLoadBalancingHttpClientTests.java b/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/ribbon/apache/RibbonLoadBalancingHttpClientTests.java index 3a63a090..90c4039a 100644 --- a/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/ribbon/apache/RibbonLoadBalancingHttpClientTests.java +++ b/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/ribbon/apache/RibbonLoadBalancingHttpClientTests.java @@ -17,8 +17,10 @@ package org.springframework.cloud.netflix.ribbon.apache; +import java.io.ByteArrayInputStream; import java.io.IOException; import java.net.URI; +import java.util.ArrayList; import org.apache.http.HttpResponse; import org.apache.http.StatusLine; import org.apache.http.client.HttpClient; @@ -32,6 +34,8 @@ import org.junit.Before; import org.junit.Test; import org.mockito.ArgumentCaptor; import org.springframework.cloud.client.loadbalancer.LoadBalancedBackOffPolicyFactory; + +import org.mockito.ArgumentMatcher; import org.springframework.cloud.client.loadbalancer.LoadBalancedRetryPolicyFactory; import org.springframework.cloud.commons.httpclient.HttpClientConfiguration; import org.springframework.cloud.netflix.ribbon.RibbonAutoConfiguration; @@ -40,6 +44,8 @@ import org.springframework.cloud.netflix.ribbon.RibbonLoadBalancedRetryPolicyFac import org.springframework.cloud.netflix.ribbon.RibbonLoadBalancerContext; import org.springframework.cloud.netflix.ribbon.ServerIntrospector; import org.springframework.cloud.netflix.ribbon.SpringClientFactory; +import org.springframework.cloud.netflix.ribbon.support.RibbonCommandContext; +import org.springframework.cloud.netflix.ribbon.support.RibbonRequestCustomizer; import org.springframework.context.annotation.AnnotationConfigApplicationContext; import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Configuration; @@ -49,6 +55,7 @@ import org.springframework.retry.backoff.BackOffContext; import org.springframework.retry.backoff.BackOffInterruptedException; import org.springframework.retry.backoff.BackOffPolicy; import org.springframework.test.util.ReflectionTestUtils; +import org.springframework.util.LinkedMultiValueMap; import com.netflix.client.DefaultLoadBalancerRetryHandler; import com.netflix.client.RetryHandler; @@ -65,6 +72,7 @@ import static org.junit.Assert.assertThat; import static org.junit.Assert.fail; import static org.mockito.BDDMockito.given; import static org.mockito.Matchers.any; +import static org.mockito.Matchers.argThat; import static org.mockito.Matchers.eq; import static org.mockito.Mockito.doReturn; import static org.mockito.Mockito.doThrow; @@ -209,6 +217,16 @@ public class RibbonLoadBalancingHttpClientTests { String serviceName, String host, int port, CloseableHttpClient delegate, ILoadBalancer lb, String statusCodes, LoadBalancedBackOffPolicyFactory loadBalancedBackOffPolicyFactory) throws Exception { + return setupClientForRetry(retriesNextServer, retriesSameServer, retryable, retryOnAllOps, serviceName, host, port, + delegate, lb, statusCodes, loadBalancedBackOffPolicyFactory, false); + } + + private RetryableRibbonLoadBalancingHttpClient setupClientForRetry(int retriesNextServer, int retriesSameServer, + boolean retryable, boolean retryOnAllOps, + String serviceName, String host, int port, + CloseableHttpClient delegate, ILoadBalancer lb, String statusCodes, + LoadBalancedBackOffPolicyFactory loadBalancedBackOffPolicyFactory, + boolean isSecure) throws Exception { ServerIntrospector introspector = mock(ServerIntrospector.class); RetryHandler retryHandler = new DefaultLoadBalancerRetryHandler(retriesSameServer, retriesNextServer, retryable); doReturn(new Server(host, port)).when(lb).chooseServer(eq(serviceName)); @@ -217,6 +235,7 @@ public class RibbonLoadBalancingHttpClientTests { clientConfig.set(CommonClientConfigKey.MaxAutoRetriesNextServer, retriesNextServer); clientConfig.set(CommonClientConfigKey.MaxAutoRetries, retriesSameServer); clientConfig.set(RibbonLoadBalancedRetryPolicy.RETRYABLE_STATUS_CODES, statusCodes); + clientConfig.set(CommonClientConfigKey.IsSecure, isSecure); clientConfig.setClientName(serviceName); RibbonLoadBalancerContext context = new RibbonLoadBalancerContext(lb, clientConfig, retryHandler); SpringClientFactory clientFactory = mock(SpringClientFactory.class); @@ -332,6 +351,83 @@ public class RibbonLoadBalancingHttpClientTests { assertEquals(2, myBackOffPolicyFactory.getCount()); } + @Test + public void testDoubleEncoding() throws Exception { + String serviceName = "foo"; + String host = serviceName; + int port = 80; + HttpMethod method = HttpMethod.GET; + final URI uri = new URI("https://" + host + ":" + port + "/a%2Bb"); + DefaultClientConfigImpl clientConfig = new DefaultClientConfigImpl(); + clientConfig.setClientName(serviceName); + ServerIntrospector introspector = mock(ServerIntrospector.class); + RibbonCommandContext context = new RibbonCommandContext(serviceName, method.toString(), uri.toString(), false, + new LinkedMultiValueMap(), new LinkedMultiValueMap(), + new ByteArrayInputStream(new String("bar").getBytes()), + new ArrayList()); + RibbonApacheHttpRequest request = new RibbonApacheHttpRequest(context); + CloseableHttpClient delegate = mock(CloseableHttpClient.class); + final CloseableHttpResponse response = mock(CloseableHttpResponse.class); + StatusLine statusLine = mock(StatusLine.class); + doReturn(200).when(statusLine).getStatusCode(); + doReturn(statusLine).when(response).getStatusLine(); + doReturn(response). + when(delegate).execute(any(HttpUriRequest.class)); + RibbonLoadBalancingHttpClient client = new RibbonLoadBalancingHttpClient(delegate, clientConfig, introspector); + RibbonApacheHttpResponse returnedResponse = client.execute(request, null); + verify(response, times(0)).close(); + verify(delegate, times(1)).execute(argThat(new ArgumentMatcher() { + @Override + public boolean matches(HttpUriRequest argument) { + if(argument instanceof HttpUriRequest) { + HttpUriRequest arg = (HttpUriRequest)argument; + return arg.getURI().equals(uri); + } + return false; + } + })); + } + + @Test + public void testDoubleEncodingWithRetry() throws Exception { + int retriesNextServer = 0; + int retriesSameServer = 0; + boolean retryable = true; + boolean retryOnAllOps = true; + String serviceName = "foo"; + String host = serviceName; + int port = 80; + HttpMethod method = HttpMethod.GET; + final URI uri = new URI("https://" + host + ":" + port + "/a%2Bb"); + RibbonCommandContext context = new RibbonCommandContext(serviceName, method.toString(), uri.toString(), true, + new LinkedMultiValueMap(), new LinkedMultiValueMap(), + new ByteArrayInputStream(new String("bar").getBytes()), + new ArrayList()); + RibbonApacheHttpRequest request = new RibbonApacheHttpRequest(context); + CloseableHttpClient delegate = mock(CloseableHttpClient.class); + final CloseableHttpResponse response = mock(CloseableHttpResponse.class); + StatusLine statusLine = mock(StatusLine.class); + doReturn(200).when(statusLine).getStatusCode(); + doReturn(statusLine).when(response).getStatusLine(); + doReturn(response). + when(delegate).execute(any(HttpUriRequest.class)); + ILoadBalancer lb = mock(ILoadBalancer.class); + RetryableRibbonLoadBalancingHttpClient client = setupClientForRetry(retriesNextServer, retriesSameServer, retryable, retryOnAllOps, + serviceName, host, port, delegate, lb, "", loadBalancedBackOffPolicyFactory,true); + client.execute(request, null); + verify(response, times(0)).close(); + verify(delegate, times(1)).execute(argThat(new ArgumentMatcher() { + @Override + public boolean matches(HttpUriRequest argument) { + if(argument instanceof HttpUriRequest) { + HttpUriRequest arg = (HttpUriRequest)argument; + return arg.getURI().equals(uri); + } + return false; + } + })); + } + @Test public void testNoRetryOnPost() throws Exception { int retriesNextServer = 1; diff --git a/spring-cloud-netflix-zuul/src/main/java/org/springframework/cloud/netflix/zuul/util/RequestContentDataExtractor.java b/spring-cloud-netflix-zuul/src/main/java/org/springframework/cloud/netflix/zuul/util/RequestContentDataExtractor.java index b06e04e0..5ff6d2a6 100644 --- a/spring-cloud-netflix-zuul/src/main/java/org/springframework/cloud/netflix/zuul/util/RequestContentDataExtractor.java +++ b/spring-cloud-netflix-zuul/src/main/java/org/springframework/cloud/netflix/zuul/util/RequestContentDataExtractor.java @@ -41,13 +41,13 @@ public class RequestContentDataExtractor { } private static MultiValueMap extractFromRequest(HttpServletRequest request) throws IOException { - MultiValueMap builder = new LinkedMultiValueMap<>(); - Set queryParams = findQueryParams(request); + MultiValueMap builder = new LinkedMultiValueMap<>(); + Set queryParams = findQueryParams(request); for (Entry entry : request.getParameterMap().entrySet()) { String key = entry.getKey(); - if (!queryParams.contains(key)) { + if (!queryParams.contains(key) && entry.getValue() != null) { for (String value : entry.getValue()) { builder.add(key, value); } @@ -59,8 +59,8 @@ public class RequestContentDataExtractor { private static MultiValueMap extractFromMultipartRequest(MultipartHttpServletRequest request) throws IOException { - MultiValueMap builder = new LinkedMultiValueMap<>(); - Set queryParams = findQueryParams(request); + MultiValueMap builder = new LinkedMultiValueMap<>(); + Set queryParams = findQueryParams(request); for (Entry entry : request.getParameterMap().entrySet()) { String key = entry.getKey(); @@ -97,7 +97,7 @@ public class RequestContentDataExtractor { private static Set findQueryParams(HttpServletRequest request) { Set result = new HashSet<>(); - String query = request.getQueryString(); + String query = request.getQueryString(); if (query != null) { for (String value : StringUtils.tokenizeToStringArray(query, "&")) { diff --git a/spring-cloud-netflix-zuul/src/test/java/org/springframework/cloud/netflix/zuul/filters/route/SimpleHostRoutingFilterTests.java b/spring-cloud-netflix-zuul/src/test/java/org/springframework/cloud/netflix/zuul/filters/route/SimpleHostRoutingFilterTests.java index 1dacf8cc..3f67ea8c 100644 --- a/spring-cloud-netflix-zuul/src/test/java/org/springframework/cloud/netflix/zuul/filters/route/SimpleHostRoutingFilterTests.java +++ b/spring-cloud-netflix-zuul/src/test/java/org/springframework/cloud/netflix/zuul/filters/route/SimpleHostRoutingFilterTests.java @@ -32,6 +32,7 @@ import java.util.zip.GZIPOutputStream; import javax.servlet.http.HttpServletResponse; import com.netflix.zuul.context.RequestContext; +import com.netflix.zuul.monitoring.CounterFactory; import org.apache.http.HttpEntityEnclosingRequest; import org.apache.http.HttpHost; import org.apache.http.HttpRequest; @@ -44,6 +45,7 @@ import org.apache.http.impl.client.CloseableHttpClient; import org.apache.http.impl.conn.PoolingHttpClientConnectionManager; import org.assertj.core.api.Assertions; import org.junit.After; +import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; import org.springframework.boot.SpringApplication; @@ -59,6 +61,8 @@ import org.springframework.cloud.commons.httpclient.DefaultApacheHttpClientFacto import org.springframework.cloud.context.environment.EnvironmentChangeEvent; import org.springframework.cloud.netflix.zuul.filters.ProxyRequestHelper; import org.springframework.cloud.netflix.zuul.filters.ZuulProperties; +import org.springframework.cloud.netflix.zuul.metrics.EmptyCounterFactory; +import org.springframework.cloud.netflix.zuul.util.ZuulRuntimeException; import org.springframework.context.annotation.AnnotationConfigApplicationContext; import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Configuration; @@ -98,11 +102,17 @@ public class SimpleHostRoutingFilterTests { @LocalServerPort private int port; + @Before + public void setup() { + CounterFactory.initialize(new EmptyCounterFactory()); + } + @After public void clear() { if (this.context != null) { this.context.close(); } + CounterFactory.initialize(null); } @Test @@ -281,7 +291,7 @@ public class SimpleHostRoutingFilterTests { assertEquals(100, getFilter().filterOrder()); } - @Test(expected = IllegalStateException.class) + @Test(expected = ZuulRuntimeException.class) public void run() throws Exception { setupContext(); MockHttpServletRequest request = new MockHttpServletRequest("POST", "/");