From fdf24468c5fb2716c70decb19245ef7a00ce0752 Mon Sep 17 00:00:00 2001 From: marnee01 Date: Mon, 24 Feb 2025 15:45:25 -0600 Subject: [PATCH] Config Client: Fix logic to try all URLs 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: Marnee DeRider <20825300+marnee01@users.noreply.github.com> --- .../client/ConfigServerConfigDataLoader.java | 17 ++++- .../ConfigServicePropertySourceLocator.java | 17 ++++- .../ConfigServerConfigDataLoaderTests.java | 63 +++++++++++++++++- ...nfigServicePropertySourceLocatorTests.java | 64 ++++++++++++++++++- 4 files changed, 157 insertions(+), 4 deletions(-) diff --git a/spring-cloud-config-client/src/main/java/org/springframework/cloud/config/client/ConfigServerConfigDataLoader.java b/spring-cloud-config-client/src/main/java/org/springframework/cloud/config/client/ConfigServerConfigDataLoader.java index fc0f8db02e..a8dc23903a 100644 --- a/spring-cloud-config-client/src/main/java/org/springframework/cloud/config/client/ConfigServerConfigDataLoader.java +++ b/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; } diff --git a/spring-cloud-config-client/src/main/java/org/springframework/cloud/config/client/ConfigServicePropertySourceLocator.java b/spring-cloud-config-client/src/main/java/org/springframework/cloud/config/client/ConfigServicePropertySourceLocator.java index e10b6cb6a0..9bc5c8b8f4 100644 --- a/spring-cloud-config-client/src/main/java/org/springframework/cloud/config/client/ConfigServicePropertySourceLocator.java +++ b/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; } diff --git a/spring-cloud-config-client/src/test/java/org/springframework/cloud/config/client/ConfigServerConfigDataLoaderTests.java b/spring-cloud-config-client/src/test/java/org/springframework/cloud/config/client/ConfigServerConfigDataLoaderTests.java index 356ce00428..46177c3cb4 100644 --- a/spring-cloud-config-client/src/test/java/org/springframework/cloud/config/client/ConfigServerConfigDataLoaderTests.java +++ b/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 expectedCause) throws Exception { + assertNextUriIsNotTried(true, multipleUriStrategy, firstUriResponse, expectedCause); + } + + private void assertNextUriIsNotTried(boolean failFast, + ConfigClientProperties.MultipleUriStrategy multipleUriStrategy, HttpStatus firstUriResponse, + Class 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); diff --git a/spring-cloud-config-client/src/test/java/org/springframework/cloud/config/client/ConfigServicePropertySourceLocatorTests.java b/spring-cloud-config-client/src/test/java/org/springframework/cloud/config/client/ConfigServicePropertySourceLocatorTests.java index 077b5d9862..28401a04bf 100644 --- a/spring-cloud-config-client/src/test/java/org/springframework/cloud/config/client/ConfigServicePropertySourceLocatorTests.java +++ b/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,6 +515,12 @@ public void shouldPreserveOrder() { private void assertNextUriIsNotTried(ConfigClientProperties.MultipleUriStrategy multipleUriStrategy, HttpStatus firstUriResponse, Class expectedCause) { + assertNextUriIsNotTried(true, multipleUriStrategy, firstUriResponse, expectedCause); + } + + private void assertNextUriIsNotTried(boolean failFast, + ConfigClientProperties.MultipleUriStrategy multipleUriStrategy, HttpStatus firstUriResponse, + Class expectedCause) { AbstractThrowableAssert throwableAssert = Assertions.assertThatThrownBy(() -> { // Set up with two URIs. ConfigClientProperties clientProperties = new ConfigClientProperties(this.environment); @@ -466,7 +528,7 @@ private void assertNextUriIsNotTried(ConfigClientProperties.MultipleUriStrategy 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);