Config Client: Fix logic to try all URLs

spring-cloud-config-client/src/main/java/org/springframework/cloud/config/client/ConfigServerConfigDataLoader.java

@@ -369,7 +369,22 @@ protected Environment getRemoteEnvironment(ConfigDataLoaderContext context, Conf
 				}
 			}
 
-			if (response == null || response.getStatusCode() != HttpStatus.OK) {
+			if (response == null) {
+				if (i < noOfUrls - 1 && properties.getMultipleUriStrategy() == MultipleUriStrategy.ALWAYS) {
+					logger.info("Failed to fetch configs from server at  : " + uri
+							+ ". The response was null. Will try the next url if available.");
+					continue;
+				}
+
+				return null;
+			}
+			else if (response.getStatusCode() != HttpStatus.OK) {
+				if (i < noOfUrls - 1 && properties.getMultipleUriStrategy() == MultipleUriStrategy.ALWAYS) {
+					logger.info("Failed to fetch configs from server at  : " + uri
+							+ ". Will try the next url if available. StatusCode : " + response.getStatusCode());
+					continue;
+				}
+
 				return null;
 			}
 

spring-cloud-config-client/src/main/java/org/springframework/cloud/config/client/ConfigServicePropertySourceLocator.java

@@ -307,7 +307,22 @@ private Environment getRemoteEnvironment(ConfigClientRequestTemplateFactory requ
 				}
 			}
 
-			if (response == null || response.getStatusCode() != HttpStatus.OK) {
+			if (response == null) {
+				if (i < noOfUrls - 1 && defaultProperties.getMultipleUriStrategy() == MultipleUriStrategy.ALWAYS) {
+					logger.info("Failed to fetch configs from server at  : " + uri
+							+ ". The response was null. Will try the next url if available.");
+					continue;
+				}
+
+				return null;
+			}
+			else if (response.getStatusCode() != HttpStatus.OK) {
+				if (i < noOfUrls - 1 && defaultProperties.getMultipleUriStrategy() == MultipleUriStrategy.ALWAYS) {
+					logger.info("Failed to fetch configs from server at  : " + uri
+							+ ". Will try the next url if available. StatusCode : " + response.getStatusCode());
+					continue;
+				}
+
 				return null;
 			}
 

spring-cloud-config-client/src/test/java/org/springframework/cloud/config/client/ConfigServerConfigDataLoaderTests.java

@@ -133,6 +133,7 @@ public void init() {
 			.thenReturn(mock(ConfigClientRequestTemplateFactory.class));
 		when(bootstrapContext.get(RestTemplate.class)).thenReturn(restTemplate);
 		when(resource.getProperties()).thenReturn(properties);
+		when(resource.isOptional()).thenReturn(true);
 
 		when(resource.getProfiles()).thenReturn(PROFILES);
 	}
@@ -336,6 +337,60 @@ public void shouldUseNextUriFor_500_And_ALWAYS_Strategy() throws Exception {
 		assertNextUriIsTried(ConfigClientProperties.MultipleUriStrategy.ALWAYS, HttpStatus.INTERNAL_SERVER_ERROR);
 	}
 
+	@Test
+	public void shouldUseNextUriFor_NoExceptionNotOK_And_CONNECTION_TIMEOUT_ONLY_Strategy_FailFastIsFalse()
+			throws Exception {
+		// At the time of this writing, TEMPORARY_REDIRECT will not cause an exception to
+		// be thrown back to
+		// getRemoteEnvironment, but since status is not OK, the method returns null and
+		// locate method will
+		// simply return null since fail-fast=false. (Second URL is never tried, due to
+		// the strategy.
+
+		// Set up with two URIs.
+		String badURI = "http://baduri";
+		String goodURI = "http://localhost:8888";
+		String[] uris = new String[] { badURI, goodURI };
+		properties.setUri(uris);
+		properties.setFailFast(false);
+		// Strategy is CONNECTION_TIMEOUT_ONLY, so it should not try the next URI for
+		// TEMPORARY_REDIRECT
+		properties.setMultipleUriStrategy(ConfigClientProperties.MultipleUriStrategy.CONNECTION_TIMEOUT_ONLY);
+		this.loader = new ConfigServerConfigDataLoader(destination -> logger);
+		ClientHttpRequestFactory requestFactory = mock(ClientHttpRequestFactory.class);
+		RestTemplate restTemplate = new RestTemplate(requestFactory);
+		mockRequestResponse(requestFactory, badURI, HttpStatus.TEMPORARY_REDIRECT);
+		mockRequestResponse(requestFactory, goodURI, HttpStatus.OK);
+		when(bootstrapContext.get(RestTemplate.class)).thenReturn(restTemplate);
+		assertThat(this.loader.load(context, resource)).isNull();
+	}
+
+	@Test
+	public void shouldUseNextUriFor_NoExceptionNotOK_And_CONNECTION_TIMEOUT_ONLY_Strategy_WithFailFastIsTrue()
+			throws Exception {
+		// At the time of this writing, TEMPORARY_REDIRECT will not cause an exception to
+		// be thrown back to
+		// getRemoteEnvironment, but since status is not OK, the method returns null and
+		// locate method will
+		// throw an IllegalStateException with no cause, since fail-fast=true. Second URL
+		// is never tried, due to the strategy.
+		assertNextUriIsNotTried(true, ConfigClientProperties.MultipleUriStrategy.CONNECTION_TIMEOUT_ONLY,
+				HttpStatus.TEMPORARY_REDIRECT, null // IllegalStateException has no cause,
+													// because getRemoteEnvironment did
+													// not throw an exception
+		);
+	}
+
+	@Test
+	public void shouldUseNextUriFor_NoExceptionNotOK_And_ALWAYS_Strategy() throws Exception {
+		// At the time of this writing, TEMPORARY_REDIRECT will not cause an exception to
+		// be thrown back to
+		// getRemoteEnvironment, but since status is not OK, the method will treat it the
+		// same as an exception and
+		// thus try the next URL.
+		assertNextUriIsTried(ConfigClientProperties.MultipleUriStrategy.ALWAYS, HttpStatus.TEMPORARY_REDIRECT);
+	}
+
 	@Test
 	public void shouldUseNextUriFor_TimeOut_And_ALWAYS_Strategy() throws Exception {
 		assertNextUriIsTriedOnTimeout(ConfigClientProperties.MultipleUriStrategy.ALWAYS);
@@ -631,12 +686,18 @@ private ConfigClientRequestTemplateFactory factory(ConfigClientProperties proper
 
 	private void assertNextUriIsNotTried(ConfigClientProperties.MultipleUriStrategy multipleUriStrategy,
 			HttpStatus firstUriResponse, Class<? extends Exception> expectedCause) throws Exception {
+		assertNextUriIsNotTried(true, multipleUriStrategy, firstUriResponse, expectedCause);
+	}
+
+	private void assertNextUriIsNotTried(boolean failFast,
+			ConfigClientProperties.MultipleUriStrategy multipleUriStrategy, HttpStatus firstUriResponse,
+			Class<? extends Exception> expectedCause) throws Exception {
 		// Set up with two URIs.
 		String badURI = "http://baduri";
 		String goodURI = "http://localhost:8888";
 		String[] uris = new String[] { badURI, goodURI };
 		properties.setUri(uris);
-		properties.setFailFast(true);
+		properties.setFailFast(failFast);
 		// Strategy is CONNECTION_TIMEOUT_ONLY, so it should not try the next URI for
 		// INTERNAL_SERVER_ERROR
 		properties.setMultipleUriStrategy(multipleUriStrategy);

spring-cloud-config-client/src/test/java/org/springframework/cloud/config/client/ConfigServicePropertySourceLocatorTests.java

@@ -20,6 +20,7 @@
 import java.io.IOException;
 import java.net.URI;
 import java.util.ArrayList;
+import java.util.Collections;
 import java.util.HashMap;
 import java.util.Iterator;
 import java.util.LinkedHashMap;
@@ -332,6 +333,61 @@ public void shouldUseNextUriFor_500_And_ALWAYS_Strategy() throws Exception {
 		assertNextUriIsTried(ConfigClientProperties.MultipleUriStrategy.ALWAYS, HttpStatus.INTERNAL_SERVER_ERROR);
 	}
 
+	@Test
+	public void shouldUseNextUriFor_NoExceptionNotOK_And_CONNECTION_TIMEOUT_ONLY_Strategy_FailFastIsFalse()
+			throws Exception {
+		// At the time of this writing, TEMPORARY_REDIRECT will not cause an exception to
+		// be thrown back to
+		// getRemoteEnvironment, but since status is not OK, the method returns null and
+		// locate method will
+		// simply return null since fail-fast=false. (Second URL is never tried, due to
+		// the strategy.
+
+		// Set up with two URIs.
+		ConfigClientProperties clientProperties = new ConfigClientProperties(this.environment);
+		String badURI = "http://baduri";
+		String goodURI = "http://localhost:8888";
+		String[] uris = new String[] { badURI, goodURI };
+		clientProperties.setUri(uris);
+		clientProperties.setFailFast(false);
+		// Strategy is CONNECTION_TIMEOUT_ONLY, so it should not try the next URI for
+		// TEMPORARY_REDIRECT
+		clientProperties.setMultipleUriStrategy(ConfigClientProperties.MultipleUriStrategy.CONNECTION_TIMEOUT_ONLY);
+		this.locator = new ConfigServicePropertySourceLocator(clientProperties);
+		ClientHttpRequestFactory requestFactory = Mockito.mock(ClientHttpRequestFactory.class);
+		RestTemplate restTemplate1 = new RestTemplate(requestFactory);
+		mockRequestResponse(requestFactory, badURI, HttpStatus.TEMPORARY_REDIRECT);
+		mockRequestResponse(requestFactory, goodURI, HttpStatus.OK);
+		this.locator.setRestTemplate(restTemplate1);
+		assertThat(this.locator.locateCollection(this.environment)).isEqualTo(Collections.emptyList());
+	}
+
+	@Test
+	public void shouldUseNextUriFor_NoExceptionNotOK_And_CONNECTION_TIMEOUT_ONLY_Strategy_WithFailFastIsTrue()
+			throws Exception {
+		// At the time of this writing, TEMPORARY_REDIRECT will not cause an exception to
+		// be thrown back to
+		// getRemoteEnvironment, but since status is not OK, the method returns null and
+		// locate method will
+		// throw an IllegalStateException with no cause, since fail-fast=true. Second URL
+		// is never tried, due to the strategy.
+		assertNextUriIsNotTried(true, ConfigClientProperties.MultipleUriStrategy.CONNECTION_TIMEOUT_ONLY,
+				HttpStatus.TEMPORARY_REDIRECT, null // IllegalStateException has no cause,
+													// because getRemoteEnvironment did
+													// not throw an exception
+		);
+	}
+
+	@Test
+	public void shouldUseNextUriFor_NoExceptionNotOK_And_ALWAYS_Strategy() throws Exception {
+		// At the time of this writing, TEMPORARY_REDIRECT will not cause an exception to
+		// be thrown back to
+		// getRemoteEnvironment, but since status is not OK, the method will treat it the
+		// same as an exception and
+		// thus try the next URL.
+		assertNextUriIsTried(ConfigClientProperties.MultipleUriStrategy.ALWAYS, HttpStatus.TEMPORARY_REDIRECT);
+	}
+
 	@Test
 	public void shouldUseNextUriFor_TimeOut_And_ALWAYS_Strategy() throws Exception {
 		assertNextUriIsTriedOnTimeout(ConfigClientProperties.MultipleUriStrategy.ALWAYS);
@@ -459,14 +515,20 @@ public void shouldPreserveOrder() {
 
 	private void assertNextUriIsNotTried(ConfigClientProperties.MultipleUriStrategy multipleUriStrategy,
 			HttpStatus firstUriResponse, Class<? extends Exception> expectedCause) {
+		assertNextUriIsNotTried(true, multipleUriStrategy, firstUriResponse, expectedCause);
+	}
+
+	private void assertNextUriIsNotTried(boolean failFast,
+			ConfigClientProperties.MultipleUriStrategy multipleUriStrategy, HttpStatus firstUriResponse,
+			Class<? extends Exception> expectedCause) {
 		AbstractThrowableAssert throwableAssert = Assertions.assertThatThrownBy(() -> {
 			// Set up with two URIs.
 			ConfigClientProperties clientProperties = new ConfigClientProperties(this.environment);
 			String badURI = "http://baduri";
 			String goodURI = "http://localhost:8888";
 			String[] uris = new String[] { badURI, goodURI };
 			clientProperties.setUri(uris);
-			clientProperties.setFailFast(true);
+			clientProperties.setFailFast(failFast);
 			// Strategy is CONNECTION_TIMEOUT_ONLY, so it should not try the next URI for
 			// INTERNAL_SERVER_ERROR
 			clientProperties.setMultipleUriStrategy(multipleUriStrategy);