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

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<RibbonApacheHttpRequest, RibbonApacheHttpResponse> {
+		extends
+		AbstractLoadBalancerAwareClient<RibbonApacheHttpRequest, RibbonApacheHttpResponse> {
 	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;
 	}
 }

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(),

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<RequestConfig> requestConfigCaptor = ArgumentCaptor
+				.forClass(RequestConfig.class);
+		verify(request).toRequest(requestConfigCaptor.capture());
+		return requestConfigCaptor.getValue();
+	}
+
+}