From 15f940054d68041f858a8960c4b94d219ac46131 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=A9bastien=20Nussbaumer?= Date: Tue, 8 Dec 2015 16:39:08 +0100 Subject: [PATCH] Enable configuration of the way HttpClient handles redirects RibbonLoadBalancingHttpClient being IClientConfigAware, it is now initialized within initWithNiwsConfig. The values of the config keys FollowRedirects, ConnectTimeout and ReadTimeout are now respected. Fixes gh-691 --- .../apache/RibbonLoadBalancingHttpClient.java | 41 +++-- .../HttpClientRibbonCommandFactory.java | 9 +- .../RibbonLoadBalancingHttpClientTests.java | 144 ++++++++++++++++++ 3 files changed, 179 insertions(+), 15 deletions(-) create mode 100644 spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/ribbon/apache/RibbonLoadBalancingHttpClientTests.java 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 ad9b4f98..8563e83a 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 @@ -29,29 +29,27 @@ import com.netflix.client.AbstractLoadBalancerAwareClient; import com.netflix.client.RequestSpecificRetryHandler; import com.netflix.client.RetryHandler; import com.netflix.client.config.CommonClientConfigKey; +import com.netflix.client.config.DefaultClientConfigImpl; import com.netflix.client.config.IClientConfig; import com.netflix.loadbalancer.ILoadBalancer; -import lombok.Setter; - /** * @author Christian Lohmann */ public class RibbonLoadBalancingHttpClient - extends AbstractLoadBalancerAwareClient { + extends + AbstractLoadBalancerAwareClient { private final HttpClient delegate = HttpClientBuilder.create().build(); - @Setter private int connectTimeout; - @Setter private int readTimeout; - @Setter private boolean secure; - @Setter - private IClientConfig clientConfig; + private boolean followRedirects; + + private boolean okToRetryOnAllOperations; public RibbonLoadBalancingHttpClient() { super(null); @@ -63,10 +61,29 @@ public class RibbonLoadBalancingHttpClient this.setRetryHandler(RetryHandler.DEFAULT); } + @Override + public void initWithNiwsConfig(IClientConfig clientConfig) { + super.initWithNiwsConfig(clientConfig); + this.connectTimeout = clientConfig.getPropertyAsInteger( + CommonClientConfigKey.ConnectTimeout, + DefaultClientConfigImpl.DEFAULT_CONNECT_TIMEOUT); + this.readTimeout = clientConfig.getPropertyAsInteger( + CommonClientConfigKey.ReadTimeout, + DefaultClientConfigImpl.DEFAULT_READ_TIMEOUT); + this.secure = clientConfig.getPropertyAsBoolean(CommonClientConfigKey.IsSecure, + false); + this.followRedirects = clientConfig.getPropertyAsBoolean( + CommonClientConfigKey.FollowRedirects, + DefaultClientConfigImpl.DEFAULT_FOLLOW_REDIRECTS); + this.okToRetryOnAllOperations = clientConfig.getPropertyAsBoolean( + CommonClientConfigKey.OkToRetryOnAllOperations, + DefaultClientConfigImpl.DEFAULT_OK_TO_RETRY_ON_ALL_OPERATIONS); + } + @Override public RequestSpecificRetryHandler getRequestSpecificRetryHandler( final RibbonApacheHttpRequest request, final IClientConfig requestConfig) { - if (this.clientConfig.get(CommonClientConfigKey.OkToRetryOnAllOperations, false)) { + if (this.okToRetryOnAllOperations) { return new RequestSpecificRetryHandler(true, true, this.getRetryHandler(), requestConfig); } @@ -90,10 +107,13 @@ public class RibbonLoadBalancingHttpClient CommonClientConfigKey.ConnectTimeout, this.connectTimeout)); builder.setConnectionRequestTimeout(configOverride.get( CommonClientConfigKey.ReadTimeout, this.readTimeout)); + builder.setRedirectsEnabled(configOverride.get( + CommonClientConfigKey.FollowRedirects, this.followRedirects)); } else { builder.setConnectTimeout(this.connectTimeout); builder.setConnectionRequestTimeout(this.readTimeout); + builder.setRedirectsEnabled(this.followRedirects); } final RequestConfig requestConfig = builder.build(); @@ -110,6 +130,7 @@ public class RibbonLoadBalancingHttpClient } private boolean isSecure(final IClientConfig config) { - return (config != null) ? config.get(CommonClientConfigKey.IsSecure) : secure; + return (config != null) ? config.get(CommonClientConfigKey.IsSecure) + : this.secure; } } diff --git a/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/zuul/filters/route/apache/HttpClientRibbonCommandFactory.java b/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/zuul/filters/route/apache/HttpClientRibbonCommandFactory.java index 59db82c9..dcef3e82 100644 --- a/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/zuul/filters/route/apache/HttpClientRibbonCommandFactory.java +++ b/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/zuul/filters/route/apache/HttpClientRibbonCommandFactory.java @@ -16,13 +16,13 @@ package org.springframework.cloud.netflix.zuul.filters.route.apache; +import lombok.RequiredArgsConstructor; + import org.springframework.cloud.netflix.ribbon.SpringClientFactory; import org.springframework.cloud.netflix.ribbon.apache.RibbonLoadBalancingHttpClient; import org.springframework.cloud.netflix.zuul.filters.route.RibbonCommandContext; import org.springframework.cloud.netflix.zuul.filters.route.RibbonCommandFactory; -import lombok.RequiredArgsConstructor; - /** * @author Christian Lohmann */ @@ -35,10 +35,9 @@ public class HttpClientRibbonCommandFactory implements @Override public HttpClientRibbonCommand create(final RibbonCommandContext context) { final String serviceId = context.getServiceId(); - final RibbonLoadBalancingHttpClient client = clientFactory.getClient(serviceId, - RibbonLoadBalancingHttpClient.class); + final RibbonLoadBalancingHttpClient client = this.clientFactory.getClient( + serviceId, RibbonLoadBalancingHttpClient.class); client.setLoadBalancer(this.clientFactory.getLoadBalancer(serviceId)); - client.setClientConfig(this.clientFactory.getClientConfig(serviceId)); final HttpClientRibbonCommand httpClientRibbonCommand = new HttpClientRibbonCommand( serviceId, client, context.getVerb(), context.getUri(), 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 new file mode 100644 index 00000000..b3c4d247 --- /dev/null +++ b/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/ribbon/apache/RibbonLoadBalancingHttpClientTests.java @@ -0,0 +1,144 @@ +/* + * Copyright 2013-2015 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.ribbon.apache; + +import org.apache.http.HttpResponse; +import org.apache.http.client.HttpClient; +import org.apache.http.client.config.RequestConfig; +import org.apache.http.client.methods.HttpUriRequest; +import org.junit.Test; +import org.mockito.ArgumentCaptor; +import org.springframework.cloud.netflix.ribbon.SpringClientFactory; +import org.springframework.context.annotation.AnnotationConfigApplicationContext; +import org.springframework.context.annotation.Bean; +import org.springframework.context.annotation.Configuration; +import org.springframework.test.util.ReflectionTestUtils; + +import com.netflix.client.config.CommonClientConfigKey; +import com.netflix.client.config.DefaultClientConfigImpl; +import com.netflix.client.config.IClientConfig; + +import static org.hamcrest.Matchers.is; +import static org.junit.Assert.assertThat; +import static org.mockito.BDDMockito.given; +import static org.mockito.Matchers.any; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; + +/** + * @author Sébastien Nussbaumer + */ +public class RibbonLoadBalancingHttpClientTests { + + @Test + public void testRequestConfigUseDefaultsNoOverride() throws Exception { + RequestConfig result = getBuiltRequestConfig(UseDefaults.class, null); + + assertThat(result.isRedirectsEnabled(), is(false)); + } + + @Test + public void testRequestConfigDoNotFollowRedirectsNoOverride() throws Exception { + RequestConfig result = getBuiltRequestConfig(DoNotFollowRedirects.class, null); + + assertThat(result.isRedirectsEnabled(), is(false)); + } + + @Test + public void testRequestConfigFollowRedirectsNoOverride() throws Exception { + RequestConfig result = getBuiltRequestConfig(FollowRedirects.class, null); + + assertThat(result.isRedirectsEnabled(), is(true)); + } + + @Test + public void testRequestConfigDoNotFollowRedirectsOverrideWithFollowRedirects() + throws Exception { + + DefaultClientConfigImpl override = new DefaultClientConfigImpl(); + override.set(CommonClientConfigKey.FollowRedirects, true); + override.set(CommonClientConfigKey.IsSecure, false); + + RequestConfig result = getBuiltRequestConfig(DoNotFollowRedirects.class, override); + + assertThat(result.isRedirectsEnabled(), is(true)); + } + + @Test + public void testRequestConfigFollowRedirectsOverrideWithDoNotFollowRedirects() + throws Exception { + + DefaultClientConfigImpl override = new DefaultClientConfigImpl(); + override.set(CommonClientConfigKey.FollowRedirects, false); + override.set(CommonClientConfigKey.IsSecure, false); + + RequestConfig result = getBuiltRequestConfig(FollowRedirects.class, override); + + assertThat(result.isRedirectsEnabled(), is(false)); + } + + @Configuration + protected static class UseDefaults { + + } + + @Configuration + protected static class FollowRedirects { + @Bean + public IClientConfig clientConfig() { + DefaultClientConfigImpl config = new DefaultClientConfigImpl(); + config.set(CommonClientConfigKey.FollowRedirects, true); + return config; + } + } + + @Configuration + protected static class DoNotFollowRedirects { + @Bean + public IClientConfig clientConfig() { + DefaultClientConfigImpl config = new DefaultClientConfigImpl(); + config.set(CommonClientConfigKey.FollowRedirects, false); + return config; + } + } + + private RequestConfig getBuiltRequestConfig(Class defaultConfigurationClass, + IClientConfig configOverride) throws Exception { + + SpringClientFactory factory = new SpringClientFactory(); + factory.setApplicationContext(new AnnotationConfigApplicationContext( + defaultConfigurationClass)); + HttpClient delegate = mock(HttpClient.class); + RibbonLoadBalancingHttpClient client = factory.getClient("service", + RibbonLoadBalancingHttpClient.class); + + ReflectionTestUtils.setField(client, "delegate", delegate); + given(delegate.execute(any(HttpUriRequest.class))).willReturn( + mock(HttpResponse.class)); + RibbonApacheHttpRequest request = mock(RibbonApacheHttpRequest.class); + given(request.toRequest(any(RequestConfig.class))).willReturn( + mock(HttpUriRequest.class)); + + client.execute(request, configOverride); + + ArgumentCaptor requestConfigCaptor = ArgumentCaptor + .forClass(RequestConfig.class); + verify(request).toRequest(requestConfigCaptor.capture()); + return requestConfigCaptor.getValue(); + } + +}