From 85314467fe6a85de1ce4ad1fbd7e8a763204ab60 Mon Sep 17 00:00:00 2001 From: Ryan Baxter Date: Tue, 31 Jan 2023 19:51:13 -0500 Subject: [PATCH] Fix AWS S3 ResourceController support. (#2219) There are several problems here: * Fixes and improves documentation * Fixes a bug where we were using the application as the bucket folder name when it should have been the label * Uses the default label property if set and the query parameter useDefaultLabel is supplied * Fixes the resource location check so it takes into account the encoded / in the path name of an S3 object There is one use case that requires a fix in Spring Cloud AWS, and that is when there is no label supplied and there is no default label set. There appears to be a. bug in the way the createRelative method creates the S3 object name in this case. I have submitted a PR for this https://github.com/awspring/spring-cloud-aws/issues/648 Fixes #2216 Co-authored-by: Ryan Baxter <524254+ryanjbaxter@users.noreply.github.com> --- .../main/asciidoc/spring-cloud-config.adoc | 7 +-- .../AwsS3EnvironmentRepository.java | 11 +++- .../server/environment/SearchPathLocator.java | 23 ++++++++ .../config/server/support/PathUtils.java | 6 ++- .../AwsS3EnvironmentRepositoryTests.java | 29 ++++++++++ .../GenericResourceRepositoryTests.java | 54 +++++++++++++++++++ 6 files changed, 124 insertions(+), 6 deletions(-) diff --git a/docs/src/main/asciidoc/spring-cloud-config.adoc b/docs/src/main/asciidoc/spring-cloud-config.adoc index 378b11f3..db5ac73b 100644 --- a/docs/src/main/asciidoc/spring-cloud-config.adoc +++ b/docs/src/main/asciidoc/spring-cloud-config.adoc @@ -1626,11 +1626,12 @@ server { [[spring-cloud-config-serving-plain-text-aws-s3]] === AWS S3 -To enable serving plain text for AWS s3, the Config Server application needs to include a dependency on Spring Cloud AWS. +To enable serving plain text for AWS s3, the Config Server application needs to include a dependency on `io.awspring.cloud:spring-cloud-aws-context`. For details on how to set up that dependency, see the -https://cloud.spring.io/spring-cloud-static/spring-cloud-aws/2.1.3.RELEASE/single/spring-cloud-aws.html#_spring_cloud_aws_maven_dependency_management[Spring Cloud AWS Reference Guide]. +https://docs.awspring.io/spring-cloud-aws/docs/2.4.3/reference/html/index.html#spring-cloud-aws-maven-dependency-management[Spring Cloud AWS Reference Guide]. +In addition, when using Spring Cloud AWS with Spring Boot it is useful to include the https://docs.awspring.io/spring-cloud-aws/docs/2.4.3/reference/html/index.html#spring-boot-auto-configuration[auto-configuration dependency]. Then you need to configure Spring Cloud AWS, as described in the -https://cloud.spring.io/spring-cloud-static/spring-cloud-aws/2.1.3.RELEASE/single/spring-cloud-aws.html#_configuring_credentials[Spring Cloud AWS Reference Guide]. +https://docs.awspring.io/spring-cloud-aws/docs/2.4.3/reference/html/index.html#configuring-credentials[Spring Cloud AWS Reference Guide]. === Decrypting Plain Text diff --git a/spring-cloud-config-server/src/main/java/org/springframework/cloud/config/server/environment/AwsS3EnvironmentRepository.java b/spring-cloud-config-server/src/main/java/org/springframework/cloud/config/server/environment/AwsS3EnvironmentRepository.java index d23bce78..db2fae1c 100644 --- a/spring-cloud-config-server/src/main/java/org/springframework/cloud/config/server/environment/AwsS3EnvironmentRepository.java +++ b/spring-cloud-config-server/src/main/java/org/springframework/cloud/config/server/environment/AwsS3EnvironmentRepository.java @@ -165,9 +165,16 @@ public class AwsS3EnvironmentRepository implements EnvironmentRepository, Ordere @Override public Locations getLocations(String application, String profiles, String label) { - String baseLocation = AWS_S3_RESOURCE_SCHEME + bucketName + PATH_SEPARATOR + application; + StringBuilder baseLocation = new StringBuilder(AWS_S3_RESOURCE_SCHEME + bucketName + PATH_SEPARATOR); + if (!StringUtils.hasText(label) && StringUtils.hasText(serverProperties.getDefaultLabel())) { + label = serverProperties.getDefaultLabel(); + } + // both the passed in label and the default label property could be null + if (StringUtils.hasText(label)) { + baseLocation.append(label); + } - return new Locations(application, profiles, label, null, new String[] { baseLocation }); + return new Locations(application, profiles, label, null, new String[] { baseLocation.toString() }); } } diff --git a/spring-cloud-config-server/src/main/java/org/springframework/cloud/config/server/environment/SearchPathLocator.java b/spring-cloud-config-server/src/main/java/org/springframework/cloud/config/server/environment/SearchPathLocator.java index fe100201..b24eb19d 100644 --- a/spring-cloud-config-server/src/main/java/org/springframework/cloud/config/server/environment/SearchPathLocator.java +++ b/spring-cloud-config-server/src/main/java/org/springframework/cloud/config/server/environment/SearchPathLocator.java @@ -17,6 +17,7 @@ package org.springframework.cloud.config.server.environment; import java.util.Arrays; +import java.util.Objects; /** * Strategy for locating a search path for resource (e.g. in the file system or @@ -78,6 +79,28 @@ public interface SearchPathLocator { + ", locations=" + Arrays.toString(this.locations) + ", version=" + this.version + "]"; } + @Override + public boolean equals(Object o) { + if (this == o) { + return true; + } + if (o == null || getClass() != o.getClass()) { + return false; + } + Locations locations1 = (Locations) o; + return getApplication().equals(locations1.getApplication()) && getProfile().equals(locations1.getProfile()) + && Objects.equals(getLabel(), locations1.getLabel()) + && Arrays.equals(getLocations(), locations1.getLocations()) + && Objects.equals(getVersion(), locations1.getVersion()); + } + + @Override + public int hashCode() { + int result = Objects.hash(getApplication(), getProfile(), getLabel(), getVersion()); + result = 31 * result + Arrays.hashCode(getLocations()); + return result; + } + } } diff --git a/spring-cloud-config-server/src/main/java/org/springframework/cloud/config/server/support/PathUtils.java b/spring-cloud-config-server/src/main/java/org/springframework/cloud/config/server/support/PathUtils.java index ed065f22..ae65f1a7 100644 --- a/spring-cloud-config-server/src/main/java/org/springframework/cloud/config/server/support/PathUtils.java +++ b/spring-cloud-config-server/src/main/java/org/springframework/cloud/config/server/support/PathUtils.java @@ -19,6 +19,7 @@ package org.springframework.cloud.config.server.support; import java.io.IOException; import java.io.UnsupportedEncodingException; import java.net.URLDecoder; +import java.net.URLEncoder; import java.util.List; import org.apache.commons.logging.Log; @@ -266,7 +267,10 @@ public abstract class PathUtils { return true; } locationPath = (locationPath.endsWith("/") || locationPath.isEmpty() ? locationPath : locationPath + "/"); - return (resourcePath.startsWith(locationPath) && !isInvalidEncodedPath(resourcePath)); + String encodedLocationPath = locationPath.endsWith("/") + ? locationPath.substring(0, locationPath.length() - 1) + URLEncoder.encode("/", "UTF-8") : locationPath; + return ((resourcePath.startsWith(locationPath) || resourcePath.startsWith(encodedLocationPath)) + && !isInvalidEncodedPath(resourcePath)); } } diff --git a/spring-cloud-config-server/src/test/java/org/springframework/cloud/config/server/environment/AwsS3EnvironmentRepositoryTests.java b/spring-cloud-config-server/src/test/java/org/springframework/cloud/config/server/environment/AwsS3EnvironmentRepositoryTests.java index 505a30a8..787ab5d4 100644 --- a/spring-cloud-config-server/src/test/java/org/springframework/cloud/config/server/environment/AwsS3EnvironmentRepositoryTests.java +++ b/spring-cloud-config-server/src/test/java/org/springframework/cloud/config/server/environment/AwsS3EnvironmentRepositoryTests.java @@ -204,6 +204,35 @@ public class AwsS3EnvironmentRepositoryTests { assertThat(repository).isNotNull(); } + @Test + public void getLocationsTest() { + AwsS3EnvironmentRepositoryFactory factory = new AwsS3EnvironmentRepositoryFactory(new ConfigServerProperties()); + AwsS3EnvironmentProperties properties = new AwsS3EnvironmentProperties(); + properties.setRegion("us-east-1"); + properties.setBucket("test"); + AwsS3EnvironmentRepository repository = factory.build(properties); + + assertThat(repository.getLocations("app", "default", "main")).isEqualTo( + new SearchPathLocator.Locations("app", "default", "main", null, new String[] { "s3://test/main" })); + + assertThat(repository.getLocations("app", "default", null)).isEqualTo( + new SearchPathLocator.Locations("app", "default", null, null, new String[] { "s3://test/" })); + + assertThat(repository.getLocations("app", "default", "")) + .isEqualTo(new SearchPathLocator.Locations("app", "default", "", null, new String[] { "s3://test/" })); + + ConfigServerProperties configServerProperties = new ConfigServerProperties(); + configServerProperties.setDefaultLabel("defaultlabel"); + factory = new AwsS3EnvironmentRepositoryFactory(configServerProperties); + repository = factory.build(properties); + + assertThat(repository.getLocations("app", "default", null)).isEqualTo(new SearchPathLocator.Locations("app", + "default", "defaultlabel", null, new String[] { "s3://test/defaultlabel" })); + + assertThat(repository.getLocations("app", "default", "")).isEqualTo(new SearchPathLocator.Locations("app", + "default", "defaultlabel", null, new String[] { "s3://test/defaultlabel" })); + } + private void setupS3(String fileName, String propertyContent) throws UnsupportedEncodingException { setupS3(fileName, null, propertyContent); } diff --git a/spring-cloud-config-server/src/test/java/org/springframework/cloud/config/server/resource/GenericResourceRepositoryTests.java b/spring-cloud-config-server/src/test/java/org/springframework/cloud/config/server/resource/GenericResourceRepositoryTests.java index c916bfa9..c9363c5f 100644 --- a/spring-cloud-config-server/src/test/java/org/springframework/cloud/config/server/resource/GenericResourceRepositoryTests.java +++ b/spring-cloud-config-server/src/test/java/org/springframework/cloud/config/server/resource/GenericResourceRepositoryTests.java @@ -16,6 +16,10 @@ package org.springframework.cloud.config.server.resource; +import java.io.IOException; +import java.net.URL; + +import com.amazonaws.services.s3.AmazonS3; import org.junit.After; import org.junit.Before; import org.junit.Rule; @@ -25,13 +29,21 @@ import org.junit.rules.ExpectedException; import org.springframework.boot.WebApplicationType; import org.springframework.boot.builder.SpringApplicationBuilder; import org.springframework.boot.test.system.OutputCaptureRule; +import org.springframework.cloud.config.server.config.ConfigServerProperties; +import org.springframework.cloud.config.server.environment.AwsS3EnvironmentRepository; import org.springframework.cloud.config.server.environment.NativeEnvironmentProperties; import org.springframework.cloud.config.server.environment.NativeEnvironmentRepository; import org.springframework.cloud.config.server.environment.NativeEnvironmentRepositoryTests; import org.springframework.context.ConfigurableApplicationContext; +import org.springframework.core.io.Resource; +import org.springframework.core.io.ResourceLoader; import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.fail; import static org.hamcrest.Matchers.containsString; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; /** * @author Dave Syer @@ -131,4 +143,46 @@ public class GenericResourceRepositoryTests { this.repository.findOne("blah", "local", label, "foo.properties"); } + @Test + /** + * This test is mocking what happens when using Spring Cloud AWS ProtocolResolver to + * server text files from S3 buckets. Rather than put Spring Cloud AWS on the + * classpath we just mock the behavior. Specifically we are testing to make sure the + * encoding of the / in the path is taken into account in the + * GenericResourceRepository. + */ + public void tests3() throws IOException { + AmazonS3 s3 = mock(AmazonS3.class); + AwsS3EnvironmentRepository awsS3EnvironmentRepository = new AwsS3EnvironmentRepository(s3, "test", + new ConfigServerProperties()); + GenericResourceRepository genericResourceRepository = new GenericResourceRepository(awsS3EnvironmentRepository); + genericResourceRepository.setResourceLoader(new ResourceLoader() { + @Override + public Resource getResource(String location) { + Resource relativeResource = mock(Resource.class); + when(relativeResource.exists()).thenReturn(true); + when(relativeResource.isReadable()).thenReturn(true); + Resource resource = null; + try { + when(relativeResource.getURL()).thenReturn(new URL("https://us-east-1/test/main%2Fdata.json")); + resource = mock(Resource.class); + when(resource.getURL()).thenReturn(new URL("https://us-east-1/test")); + when(resource.createRelative(eq("data.json"))).thenReturn(relativeResource); + } + catch (IOException e) { + fail("Exception thrown", e); + } + return resource; + } + + @Override + public ClassLoader getClassLoader() { + return null; + } + }); + + Resource resource = genericResourceRepository.findOne("app", "default", "main", "data.json"); + assertThat(resource.getURL()).isEqualTo(new URL("https://us-east-1/test/main%2Fdata.json")); + } + }