SpringCloud/spring-cloud-config
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Config Client: Fix logic to try all URLs (#2798)

Browse Source 
There was a miss when implementing the MultipleUriStrategy.ALWAYS.
It was not trying the next URL if response was null, or if
the HTTP Status Code was not OK, but no exception was thrown.

(This author does not know under what scenario null would be
returned as a response, but the code was already checking for it,
so I updated the logic to also try the next URL for null response.)

Closes gh-2735

Signed-off-by: marnee01 <MarneeDeRider40@gmail.com>
This commit is contained in:
marnee01
2025-03-21 14:12:04 -05:00
committed by GitHub
parent 87258a03c7
commit d959f4a071
4 changed files with 157 additions and 4 deletions
Download Patch File Download Diff File Expand all files Collapse all files

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

@@ -359,7 +359,22 @@ public class ConfigServerConfigDataLoader implements ConfigDataLoader<ConfigServ
}
}
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;
}

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

@@ -304,7 +304,22 @@ public class ConfigServicePropertySourceLocator implements PropertySourceLocator
}
}
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;
}

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

@@ -133,6 +133,7 @@ public class ConfigServerConfigDataLoaderTests {
.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 class ConfigServerConfigDataLoaderTests {
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 @@ public class ConfigServerConfigDataLoaderTests {
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);

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

@@ -20,6 +20,7 @@ import java.io.ByteArrayInputStream;
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 class ConfigServicePropertySourceLocatorTests {
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,6 +515,12 @@ public class ConfigServicePropertySourceLocatorTests {
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);
@@ -466,7 +528,7 @@ public class ConfigServicePropertySourceLocatorTests {
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);