From f67bdba5ed418f9bc9c5f3c21107e7aab8d24954 Mon Sep 17 00:00:00 2001 From: Dave Syer Date: Mon, 8 May 2017 16:45:11 +0100 Subject: [PATCH] Derive base dir of new pattern matching repositories from the parent Before this change when a repository is created to support a pattern match in the URL, the basedir for cloning the scm repository is a new temp dir. So if user configures the basedir to be something other than temp then he loses the benefit once patterns are used in the URL matchers. The basedir still has to be unique, so we now use the same parent directory as the parent repo, and add the temp file name as a directory name. I.e. if basedir=/foo/bar the pattern matched basedir will be something like /foo/config-repo-23874691. You need the directory to have a parent that is writable by the config server JVM. Fixes gh-451 --- .../MultipleJGitEnvironmentRepository.java | 25 ++++--- ...mentProfilePlaceholderRepositoryTests.java | 31 ++++++--- ...EnvironmentRepositoryIntegrationTests.java | 65 ++++++++++++++----- ...ultipleJGitEnvironmentRepositoryTests.java | 53 ++++++++++----- 4 files changed, 124 insertions(+), 50 deletions(-) diff --git a/spring-cloud-config-server/src/main/java/org/springframework/cloud/config/server/environment/MultipleJGitEnvironmentRepository.java b/spring-cloud-config-server/src/main/java/org/springframework/cloud/config/server/environment/MultipleJGitEnvironmentRepository.java index 1d278f3f..a63b8a29 100644 --- a/spring-cloud-config-server/src/main/java/org/springframework/cloud/config/server/environment/MultipleJGitEnvironmentRepository.java +++ b/spring-cloud-config-server/src/main/java/org/springframework/cloud/config/server/environment/MultipleJGitEnvironmentRepository.java @@ -76,6 +76,11 @@ public class MultipleJGitEnvironmentRepository extends JGitEnvironmentRepository } repo.afterPropertiesSet(); } + if (!getBasedir().getParentFile().canWrite()) { + throw new IllegalStateException( + "Cannot write parent of basedir (please configure a writable location): " + + getBasedir()); + } } public void setRepos(Map repos) { @@ -103,7 +108,8 @@ public class MultipleJGitEnvironmentRepository extends JGitEnvironmentRepository if (logger.isDebugEnabled()) { this.logger.debug("Cannot retrieve resource locations from " + candidate.getUri() + ", cause: (" - + e.getClass().getSimpleName() + ") " + e.getMessage()); + + e.getClass().getSimpleName() + ") " + + e.getMessage()); } continue; } @@ -125,7 +131,7 @@ public class MultipleJGitEnvironmentRepository extends JGitEnvironmentRepository for (JGitEnvironmentRepository candidate : getRepositories(repository, application, profile, label)) { try { - if (label==null) { + if (label == null) { label = candidate.getDefaultLabel(); } Environment source = candidate.findOne(application, profile, @@ -136,9 +142,10 @@ public class MultipleJGitEnvironmentRepository extends JGitEnvironmentRepository } catch (Exception e) { if (logger.isDebugEnabled()) { - this.logger.debug("Cannot load configuration from " - + candidate.getUri() + ", cause: (" - + e.getClass().getSimpleName() + ") " + e.getMessage()); + this.logger.debug( + "Cannot load configuration from " + candidate.getUri() + + ", cause: (" + e.getClass().getSimpleName() + + ") " + e.getMessage()); } continue; } @@ -147,7 +154,7 @@ public class MultipleJGitEnvironmentRepository extends JGitEnvironmentRepository } JGitEnvironmentRepository candidate = getRepository(this, application, profile, label); - if (label==null) { + if (label == null) { label = candidate.getDefaultLabel(); } if (candidate == this) { @@ -175,7 +182,8 @@ public class MultipleJGitEnvironmentRepository extends JGitEnvironmentRepository } String key = repository.getUri(); - // cover the case where label is in the uri, but no label was sent with the request + // cover the case where label is in the uri, but no label was sent with the + // request if (key.contains("{label}") && label == null) { label = repository.getDefaultLabel(); } @@ -200,7 +208,8 @@ public class MultipleJGitEnvironmentRepository extends JGitEnvironmentRepository File basedir = repository.getBasedir(); BeanUtils.copyProperties(source, repository); repository.setUri(uri); - repository.setBasedir(basedir); + repository.setBasedir( + new File(source.getBasedir().getParentFile(), basedir.getName())); return repository; } diff --git a/spring-cloud-config-server/src/test/java/org/springframework/cloud/config/server/environment/MultipleJGitEnvironmentProfilePlaceholderRepositoryTests.java b/spring-cloud-config-server/src/test/java/org/springframework/cloud/config/server/environment/MultipleJGitEnvironmentProfilePlaceholderRepositoryTests.java index 732ed9f3..1a5d3106 100644 --- a/spring-cloud-config-server/src/test/java/org/springframework/cloud/config/server/environment/MultipleJGitEnvironmentProfilePlaceholderRepositoryTests.java +++ b/spring-cloud-config-server/src/test/java/org/springframework/cloud/config/server/environment/MultipleJGitEnvironmentProfilePlaceholderRepositoryTests.java @@ -15,22 +15,27 @@ */ package org.springframework.cloud.config.server.environment; -import static org.junit.Assert.assertArrayEquals; -import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertNotNull; -import static org.junit.Assert.assertTrue; - import java.io.File; import java.util.HashMap; import java.util.Map; import org.junit.Before; import org.junit.Test; + import org.springframework.cloud.config.environment.Environment; import org.springframework.cloud.config.server.environment.MultipleJGitEnvironmentRepository.PatternMatchingJGitEnvironmentRepository; import org.springframework.cloud.config.server.environment.SearchPathLocator.Locations; import org.springframework.cloud.config.server.test.ConfigServerTestUtils; import org.springframework.core.env.StandardEnvironment; +import org.springframework.test.util.ReflectionTestUtils; +import org.springframework.util.StringUtils; + +import static org.hamcrest.CoreMatchers.containsString; +import static org.junit.Assert.assertArrayEquals; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertThat; +import static org.junit.Assert.assertTrue; /** * @author Dave Syer @@ -46,6 +51,7 @@ public class MultipleJGitEnvironmentProfilePlaceholderRepositoryTests { public void init() throws Exception { String defaultUri = ConfigServerTestUtils.prepareLocalRepo("config-repo"); this.repository.setUri(defaultUri); + this.repository.setBasedir(new File("target/repos/parent_repo")); this.repository.setRepos(createRepositories()); } @@ -67,6 +73,7 @@ public class MultipleJGitEnvironmentProfilePlaceholderRepositoryTests { repo.setName(name); repo.setPattern(new String[] { pattern }); repo.setUri(uri); + repo.setBasedir(new File("target/repos/pattern_repos", name)); return repo; } @@ -84,11 +91,12 @@ public class MultipleJGitEnvironmentProfilePlaceholderRepositoryTests { Environment environment = this.repository.findOne("application", "test1-config-repo", "master"); assertEquals(1, environment.getPropertySources().size()); - assertEquals( - getUri("*").replace("{profile}", "test1-config-repo") - + "/application.yml", + String uri = getUri("*").replace("{profile}", "test1-config-repo"); + assertEquals(uri + "/application.yml", environment.getPropertySources().get(0).getName()); assertVersion(environment); + assertThat(StringUtils.cleanPath(getRepository(uri).getBasedir().toString()), + containsString("target/repos")); } @Test @@ -141,6 +149,13 @@ public class MultipleJGitEnvironmentProfilePlaceholderRepositoryTests { "test2-config-repo", "missing-config-repo" }); } + @SuppressWarnings("unchecked") + private JGitEnvironmentRepository getRepository(String uri) { + Map repos = (Map) ReflectionTestUtils + .getField(repository, "placeholders"); + return repos.get(uri); + } + private void assertVersion(Environment environment) { String version = environment.getVersion(); assertNotNull("version was null", version); diff --git a/spring-cloud-config-server/src/test/java/org/springframework/cloud/config/server/environment/MultipleJGitEnvironmentRepositoryIntegrationTests.java b/spring-cloud-config-server/src/test/java/org/springframework/cloud/config/server/environment/MultipleJGitEnvironmentRepositoryIntegrationTests.java index 3e20e33f..da7d953d 100644 --- a/spring-cloud-config-server/src/test/java/org/springframework/cloud/config/server/environment/MultipleJGitEnvironmentRepositoryIntegrationTests.java +++ b/spring-cloud-config-server/src/test/java/org/springframework/cloud/config/server/environment/MultipleJGitEnvironmentRepositoryIntegrationTests.java @@ -16,8 +16,6 @@ package org.springframework.cloud.config.server.environment; -import static org.junit.Assert.assertEquals; - import java.io.File; import java.io.IOException; import java.util.LinkedHashMap; @@ -26,7 +24,11 @@ import java.util.Map; import org.eclipse.jgit.util.FileUtils; import org.junit.After; import org.junit.Before; +import org.junit.Rule; import org.junit.Test; +import org.junit.internal.matchers.ThrowableMessageMatcher; +import org.junit.rules.ExpectedException; + import org.springframework.boot.autoconfigure.PropertyPlaceholderAutoConfiguration; import org.springframework.boot.builder.SpringApplicationBuilder; import org.springframework.boot.context.properties.EnableConfigurationProperties; @@ -38,6 +40,9 @@ import org.springframework.context.ConfigurableApplicationContext; import org.springframework.context.annotation.Configuration; import org.springframework.context.annotation.Import; +import static org.hamcrest.CoreMatchers.containsString; +import static org.junit.Assert.assertEquals; + /** * @author Andy Chan (iceycake) * @author Dave Syer @@ -49,6 +54,9 @@ public class MultipleJGitEnvironmentRepositoryIntegrationTests { private File basedir = new File("target/config"); + @Rule + public ExpectedException expected = ExpectedException.none(); + @Before public void init() throws Exception { if (this.basedir.exists()) { @@ -69,7 +77,8 @@ public class MultipleJGitEnvironmentRepositoryIntegrationTests { String defaultRepoUri = ConfigServerTestUtils.prepareLocalRepo("config-repo"); this.context = new SpringApplicationBuilder(TestConfiguration.class).web(false) .properties("spring.cloud.config.server.git.uri:" + defaultRepoUri).run(); - EnvironmentRepository repository = this.context.getBean(EnvironmentRepository.class); + EnvironmentRepository repository = this.context + .getBean(EnvironmentRepository.class); repository.findOne("bar", "staging", "master"); Environment environment = repository.findOne("bar", "staging", "master"); assertEquals(2, environment.getPropertySources().size()); @@ -86,7 +95,8 @@ public class MultipleJGitEnvironmentRepositoryIntegrationTests { this.context = new SpringApplicationBuilder(TestConfiguration.class).web(false) .properties("spring.cloud.config.server.git.uri:" + defaultRepoUri) .properties(repoMapping).run(); - EnvironmentRepository repository = this.context.getBean(EnvironmentRepository.class); + EnvironmentRepository repository = this.context + .getBean(EnvironmentRepository.class); repository.findOne("test1-svc", "staging", "master"); Environment environment = repository.findOne("test1-svc", "staging", "master"); assertEquals(2, environment.getPropertySources().size()); @@ -98,12 +108,14 @@ public class MultipleJGitEnvironmentRepositoryIntegrationTests { String test1RepoUri = ConfigServerTestUtils.prepareLocalRepo("test1-config-repo"); Map repoMapping = new LinkedHashMap(); - repoMapping.put("spring.cloud.config.server.git.repos[test1].pattern", "*/staging"); + repoMapping.put("spring.cloud.config.server.git.repos[test1].pattern", + "*/staging"); repoMapping.put("spring.cloud.config.server.git.repos[test1].uri", test1RepoUri); this.context = new SpringApplicationBuilder(TestConfiguration.class).web(false) .properties("spring.cloud.config.server.git.uri:" + defaultRepoUri) .properties(repoMapping).run(); - EnvironmentRepository repository = this.context.getBean(EnvironmentRepository.class); + EnvironmentRepository repository = this.context + .getBean(EnvironmentRepository.class); repository.findOne("test1-svc", "staging", "master"); Environment environment = repository.findOne("test1-svc", "staging", "master"); assertEquals(2, environment.getPropertySources().size()); @@ -115,14 +127,17 @@ public class MultipleJGitEnvironmentRepositoryIntegrationTests { String test1RepoUri = ConfigServerTestUtils.prepareLocalRepo("test1-config-repo"); Map repoMapping = new LinkedHashMap(); - repoMapping.put("spring.cloud.config.server.git.repos[test1].pattern", "*/staging"); + repoMapping.put("spring.cloud.config.server.git.repos[test1].pattern", + "*/staging"); repoMapping.put("spring.cloud.config.server.git.repos[test1].uri", test1RepoUri); this.context = new SpringApplicationBuilder(TestConfiguration.class).web(false) .properties("spring.cloud.config.server.git.uri:" + defaultRepoUri) .properties(repoMapping).run(); - EnvironmentRepository repository = this.context.getBean(EnvironmentRepository.class); + EnvironmentRepository repository = this.context + .getBean(EnvironmentRepository.class); repository.findOne("test1-svc", "staging", "master"); - Environment environment = repository.findOne("test1-svc", "staging,cloud", "master"); + Environment environment = repository.findOne("test1-svc", "staging,cloud", + "master"); assertEquals(2, environment.getPropertySources().size()); } @@ -132,16 +147,21 @@ public class MultipleJGitEnvironmentRepositoryIntegrationTests { String test1RepoUri = ConfigServerTestUtils.prepareLocalRepo("test1-config-repo"); Map repoMapping = new LinkedHashMap(); - repoMapping.put("spring.cloud.config.server.git.repos[test1].pattern[0]", "*/staging,*"); - repoMapping.put("spring.cloud.config.server.git.repos[test1].pattern[1]", "*/*,staging"); - repoMapping.put("spring.cloud.config.server.git.repos[test1].pattern[2]", "*/staging"); + repoMapping.put("spring.cloud.config.server.git.repos[test1].pattern[0]", + "*/staging,*"); + repoMapping.put("spring.cloud.config.server.git.repos[test1].pattern[1]", + "*/*,staging"); + repoMapping.put("spring.cloud.config.server.git.repos[test1].pattern[2]", + "*/staging"); repoMapping.put("spring.cloud.config.server.git.repos[test1].uri", test1RepoUri); this.context = new SpringApplicationBuilder(TestConfiguration.class).web(false) .properties("spring.cloud.config.server.git.uri:" + defaultRepoUri) .properties(repoMapping).run(); - EnvironmentRepository repository = this.context.getBean(EnvironmentRepository.class); + EnvironmentRepository repository = this.context + .getBean(EnvironmentRepository.class); repository.findOne("test1-svc", "staging", "master"); - Environment environment = repository.findOne("test1-svc", "cloud,staging", "master"); + Environment environment = repository.findOne("test1-svc", "cloud,staging", + "master"); assertEquals(2, environment.getPropertySources().size()); environment = repository.findOne("test1-svc", "staging,cloud", "master"); assertEquals(2, environment.getPropertySources().size()); @@ -157,15 +177,28 @@ public class MultipleJGitEnvironmentRepositoryIntegrationTests { this.context = new SpringApplicationBuilder(TestConfiguration.class).web(false) .properties("spring.cloud.config.server.git.uri:" + defaultRepoUri) .properties(repoMapping).run(); - EnvironmentRepository repository = this.context.getBean(EnvironmentRepository.class); + EnvironmentRepository repository = this.context + .getBean(EnvironmentRepository.class); repository.findOne("test1-svc", "staging", "master"); Environment environment = repository.findOne("test1-svc", "staging", "master"); assertEquals(2, environment.getPropertySources().size()); } + @Test + public void nonWritableBasedir() throws IOException { + String defaultRepoUri = ConfigServerTestUtils.prepareLocalRepo("config-repo"); + expected.expectCause(ThrowableMessageMatcher + .hasMessage(containsString("Cannot write parent"))); + this.context = new SpringApplicationBuilder(TestConfiguration.class).web(false) + .properties("spring.cloud.config.server.git.uri:" + defaultRepoUri, + "spring.cloud.config.server.git.basedir:/tmp") + .run(); + } + @Configuration @EnableConfigurationProperties(ConfigServerProperties.class) - @Import({ PropertyPlaceholderAutoConfiguration.class, EnvironmentRepositoryConfiguration.class }) + @Import({ PropertyPlaceholderAutoConfiguration.class, + EnvironmentRepositoryConfiguration.class }) protected static class TestConfiguration { } diff --git a/spring-cloud-config-server/src/test/java/org/springframework/cloud/config/server/environment/MultipleJGitEnvironmentRepositoryTests.java b/spring-cloud-config-server/src/test/java/org/springframework/cloud/config/server/environment/MultipleJGitEnvironmentRepositoryTests.java index 5a38a50c..9fd7d902 100644 --- a/spring-cloud-config-server/src/test/java/org/springframework/cloud/config/server/environment/MultipleJGitEnvironmentRepositoryTests.java +++ b/spring-cloud-config-server/src/test/java/org/springframework/cloud/config/server/environment/MultipleJGitEnvironmentRepositoryTests.java @@ -15,22 +15,25 @@ */ package org.springframework.cloud.config.server.environment; -import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertNotNull; -import static org.junit.Assert.assertNull; -import static org.junit.Assert.assertTrue; - +import java.io.File; import java.io.IOException; import java.util.HashMap; import java.util.Map; import org.junit.Before; import org.junit.Test; + import org.springframework.cloud.config.environment.Environment; import org.springframework.cloud.config.server.environment.MultipleJGitEnvironmentRepository.PatternMatchingJGitEnvironmentRepository; import org.springframework.cloud.config.server.test.ConfigServerTestUtils; import org.springframework.core.env.StandardEnvironment; +import static org.hamcrest.CoreMatchers.containsString; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertThat; +import static org.junit.Assert.assertTrue; + /** * @author Andy Chan (iceycake) * @author Dave Syer @@ -40,7 +43,8 @@ import org.springframework.core.env.StandardEnvironment; public class MultipleJGitEnvironmentRepositoryTests { private StandardEnvironment environment = new StandardEnvironment(); - private MultipleJGitEnvironmentRepository repository = new MultipleJGitEnvironmentRepository(this.environment); + private MultipleJGitEnvironmentRepository repository = new MultipleJGitEnvironmentRepository( + this.environment); @Before public void init() throws Exception { @@ -49,7 +53,8 @@ public class MultipleJGitEnvironmentRepositoryTests { this.repository.setRepos(createRepositories()); } - private Map createRepositories() throws Exception { + private Map createRepositories() + throws Exception { String test1Uri = ConfigServerTestUtils.prepareLocalRepo("test1-config-repo"); Map repos = new HashMap<>(); @@ -57,12 +62,14 @@ public class MultipleJGitEnvironmentRepositoryTests { return repos; } - private PatternMatchingJGitEnvironmentRepository createRepository(String name, String pattern, String uri) { + private PatternMatchingJGitEnvironmentRepository createRepository(String name, + String pattern, String uri) { PatternMatchingJGitEnvironmentRepository repo = new PatternMatchingJGitEnvironmentRepository(); repo.setEnvironment(this.environment); repo.setName(name); - repo.setPattern(new String[] {pattern}); + repo.setPattern(new String[] { pattern }); repo.setUri(uri); + repo.setBasedir(new File(this.repository.getBasedir().getParentFile(), name)); return repo; } @@ -78,14 +85,15 @@ public class MultipleJGitEnvironmentRepositoryTests { private void assertVersion(Environment environment) { String version = environment.getVersion(); assertNotNull("version was null", version); - assertTrue("version length was wrong", version.length() >= 40 && version.length() <= 64); + assertTrue("version length was wrong", + version.length() >= 40 && version.length() <= 64); } @Test public void defaultRepoNested() throws IOException { String uri = ConfigServerTestUtils.prepareLocalRepo("another-config-repo"); this.repository.setUri(uri); - this.repository.setSearchPaths(new String[] {"sub"}); + this.repository.setSearchPaths(new String[] { "sub" }); this.repository.findOne("bar", "staging", "master"); Environment environment = this.repository.findOne("bar", "staging", "master"); assertEquals(2, environment.getPropertySources().size()); @@ -107,13 +115,13 @@ public class MultipleJGitEnvironmentRepositoryTests { public void defaultRepoTag() { Environment environment = this.repository.findOne("bar", "staging", "foo"); assertEquals(2, environment.getPropertySources().size()); - assertEquals(this.repository.getUri() + "/bar.properties", environment - .getPropertySources().get(0).getName()); + assertEquals(this.repository.getUri() + "/bar.properties", + environment.getPropertySources().get(0).getName()); assertVersion(environment); } @Test - public void defaultRepoBasedir() { + public void defaultRepoTwice() { this.repository.findOne("bar", "staging", "master"); Environment environment = this.repository.findOne("bar", "staging", "master"); assertEquals(2, environment.getPropertySources().size()); @@ -122,9 +130,18 @@ public class MultipleJGitEnvironmentRepositoryTests { assertVersion(environment); } + @Test + public void defaultRepoBasedir() { + repository.setBasedir(new File("target/testBase")); + assertThat(repository.getBasedir().toString(), containsString("target/testBase")); + assertThat(repository.getRepos().get("test1").getBasedir().toString(), + containsString("tmp/test1")); + } + @Test public void mappingRepo() { - Environment environment = this.repository.findOne("test1-svc", "staging", "master"); + Environment environment = this.repository.findOne("test1-svc", "staging", + "master"); assertEquals(2, environment.getPropertySources().size()); assertEquals(getUri("*test1*") + "/test1-svc.properties", environment.getPropertySources().get(0).getName()); @@ -152,15 +169,15 @@ public class MultipleJGitEnvironmentRepositoryTests { assertVersion(environment); } - private String getUri(String pattern) { String uri = null; - Map repoMappings = this.repository.getRepos(); + Map repoMappings = this.repository + .getRepos(); for (PatternMatchingJGitEnvironmentRepository repo : repoMappings.values()) { String[] mappingPattern = repo.getPattern(); - if (mappingPattern != null && mappingPattern.length!=0) { + if (mappingPattern != null && mappingPattern.length != 0) { uri = repo.getUri(); break; }