From f2795e2a87b59f7d53772f2996d2611f92f5f5c1 Mon Sep 17 00:00:00 2001 From: Oliver Bertuch Date: Wed, 25 Nov 2020 15:21:14 +0100 Subject: [PATCH 01/23] Support sub-directories for MPCONFIG SecretDirConfigSource. #5006 This adds support for mounting your secrets in subdirectories to create scopes. The relative path is used as the property name. A secret file "SECRET_DIR/foo/bar/secret" will be available as MPCONFIG value "foo.bar.secret". You can still put a file "foo.bar.secret" in "SECRET_DIR" and have the same value available. If both are present, the topmost file will be picked. Mix'n'match isn't supported. Retrieving "foo.bar.secret" from "SECRET_DIR/foo/bar.secret" will not work reliably when added at runtime. --- .../config/source/SecretsDirConfigSource.java | 89 +++++++++++-------- .../source/SecretsDirConfigSourceTest.java | 83 +++++++++++++++-- 2 files changed, 126 insertions(+), 46 deletions(-) diff --git a/nucleus/payara-modules/nucleus-microprofile/config-service/src/main/java/fish/payara/nucleus/microprofile/config/source/SecretsDirConfigSource.java b/nucleus/payara-modules/nucleus-microprofile/config-service/src/main/java/fish/payara/nucleus/microprofile/config/source/SecretsDirConfigSource.java index aee95511123..a8b1fa1e037 100644 --- a/nucleus/payara-modules/nucleus-microprofile/config-service/src/main/java/fish/payara/nucleus/microprofile/config/source/SecretsDirConfigSource.java +++ b/nucleus/payara-modules/nucleus-microprofile/config-service/src/main/java/fish/payara/nucleus/microprofile/config/source/SecretsDirConfigSource.java @@ -43,12 +43,10 @@ import java.io.File; import java.io.IOException; -import java.nio.file.Files; -import java.nio.file.Path; -import java.nio.file.Paths; +import java.nio.file.*; +import java.nio.file.attribute.BasicFileAttributes; import java.nio.file.attribute.FileTime; -import java.util.Map; -import java.util.Set; +import java.util.*; import java.util.concurrent.ConcurrentHashMap; import java.util.logging.Level; import java.util.logging.Logger; @@ -65,9 +63,10 @@ public class SecretsDirConfigSource extends PayaraConfigSource implements Config private Path secretsDir; private ConcurrentHashMap properties; private ConcurrentHashMap storedModifiedTimes; + private ConcurrentHashMap storedPaths; public SecretsDirConfigSource() { - findFile(); + findDir(); loadProperties(); } @@ -97,28 +96,32 @@ public String getValue(String property) { String result = properties.get(property); if (result != null) { try { - // check the last modified time + // check existence (secret mount might have gone away) and the last modified time FileTime ft = storedModifiedTimes.get(property); - Path path = Paths.get(secretsDir.toString(), property); + Path path = storedPaths.get(property); if (Files.exists(path) && Files.getLastModifiedTime(path).compareTo(ft) > 0) { - // file has been modified since last check - result = readFile(property); + // file has been modified since last check, re-read content + result = readFile(path); storedModifiedTimes.put(property, Files.getLastModifiedTime(path)); properties.put(property, result); } } catch (IOException ex) { - Logger.getLogger(SecretsDirConfigSource.class.getName()).log(Level.SEVERE, null, ex); + Logger.getLogger(SecretsDirConfigSource.class.getName()).log(Level.SEVERE, "Unable to read file in the directory", ex); } } else { // check whether there is a file there now as there wasn't before - Path path = Paths.get(secretsDir.toString(), property); - if (Files.exists(path) && Files.isRegularFile(path) && Files.isReadable(path)) { + // --> the list of possible paths is used "first match, first serve". + List paths = Arrays.asList(Paths.get(secretsDir.toString(), property), + Paths.get(secretsDir.toString(), property.replace('.', File.separatorChar))); + for (Path path : paths) { try { - result = readFile(property); + result = readFile(path); storedModifiedTimes.put(property, Files.getLastModifiedTime(path)); + storedPaths.put(property, path); properties.put(property, result); + break; } catch (IOException ex) { - Logger.getLogger(SecretsDirConfigSource.class.getName()).log(Level.SEVERE, null, ex); + Logger.getLogger(SecretsDirConfigSource.class.getName()).log(Level.SEVERE, "Unable to read file in the directory", ex); } } } @@ -130,7 +133,7 @@ public String getName() { return "Secrets Directory"; } - private void findFile() { + private void findDir() { secretsDir = Paths.get(configService.getMPConfig().getSecretDir()); if (!Files.exists(secretsDir) || !Files.isDirectory(secretsDir) || !Files.isReadable(secretsDir)) { @@ -143,21 +146,14 @@ private void findFile() { } } - private String readFile(String name) { + private String readFile(Path file) throws IOException { String result = null; - if (Files.exists(secretsDir) && Files.isDirectory(secretsDir) && Files.isReadable(secretsDir)) { - try { - Path file = Paths.get(secretsDir.toString(), name); - if (Files.exists(file) && Files.isReadable(file)) { - StringBuilder collector = new StringBuilder(); - for (String line : Files.readAllLines(file)) { - collector.append(line); - } - result = collector.toString(); - } - } catch (IOException ex) { - Logger.getLogger(SecretsDirConfigSource.class.getName()).log(Level.SEVERE, null, ex); + if (Files.exists(file) && Files.isRegularFile(file) && Files.isReadable(file)) { + StringBuilder collector = new StringBuilder(); + for (String line : Files.readAllLines(file)) { + collector.append(line); } + result = collector.toString(); } return result; } @@ -165,17 +161,34 @@ private String readFile(String name) { private void loadProperties() { properties = new ConcurrentHashMap<>(); storedModifiedTimes = new ConcurrentHashMap<>(); + storedPaths = new ConcurrentHashMap<>(); if (Files.exists(secretsDir) && Files.isDirectory(secretsDir) && Files.isReadable(secretsDir)) { - File files[] = secretsDir.toFile().listFiles(); - for (File file : files) { - try { - if (file.isFile() && file.canRead()) { - properties.put(file.getName(), readFile(file.getName())); - storedModifiedTimes.put(file.getName(), Files.getLastModifiedTime(file.toPath())); + try { + Files.walkFileTree(secretsDir, new SimpleFileVisitor() { + @Override + public FileVisitResult preVisitDirectory(Path dir, BasicFileAttributes attrs) throws IOException { + return dir.toFile().isHidden() ? FileVisitResult.SKIP_SUBTREE : FileVisitResult.CONTINUE; } - } catch (IOException ex) { - Logger.getLogger(SecretsDirConfigSource.class.getName()).log(Level.SEVERE, "Unable to read file in the directory", ex); - } + + @Override + public FileVisitResult visitFile(Path path, BasicFileAttributes mainAtts) throws IOException { + File file = path.toFile(); + // do not read hidden files, as K8s Secret filenames are symlinks to hidden files with data. + if (file.isFile() && ! file.isHidden() && file.canRead()) { + // 1. get relative path based on the secrets dir ("/foobar"), + // 2. replace all path seps with a ".", + // so "/foobar/test/foo/bar" becomes "test/foo/bar" becomes "test.foo.bar" property name + String property = secretsDir.relativize(path).toString().replace(File.separatorChar, '.'); + + properties.put(property, readFile(path)); + storedModifiedTimes.put(property, mainAtts.lastModifiedTime()); + storedPaths.put(property, path); + } + return FileVisitResult.CONTINUE; + } + }); + } catch (IOException ex) { + Logger.getLogger(SecretsDirConfigSource.class.getName()).log(Level.SEVERE, "Unable to read file in the directory", ex); } } } diff --git a/nucleus/payara-modules/nucleus-microprofile/config-service/src/test/java/fish/payara/nucleus/microprofile/config/source/SecretsDirConfigSourceTest.java b/nucleus/payara-modules/nucleus-microprofile/config-service/src/test/java/fish/payara/nucleus/microprofile/config/source/SecretsDirConfigSourceTest.java index b92979d7041..af07a2c145a 100644 --- a/nucleus/payara-modules/nucleus-microprofile/config-service/src/test/java/fish/payara/nucleus/microprofile/config/source/SecretsDirConfigSourceTest.java +++ b/nucleus/payara-modules/nucleus-microprofile/config-service/src/test/java/fish/payara/nucleus/microprofile/config/source/SecretsDirConfigSourceTest.java @@ -45,6 +45,7 @@ import java.nio.file.Path; import java.nio.file.Paths; import java.nio.file.attribute.FileTime; +import java.util.Comparator; import java.util.HashMap; import java.util.HashSet; import java.util.Map; @@ -69,22 +70,44 @@ public class SecretsDirConfigSourceTest { @Before public void setUp() throws IOException { testDirectory = Files.createTempDirectory("microprofile-config-test"); - // create a couple of test files + + // create a couple of test simple files Path file1 = Paths.get(testDirectory.toString(), "property1"); Path file2 = Paths.get(testDirectory.toString(), "property2"); + Path fileHidden = Paths.get(testDirectory.toString(), ".hidden-property"); file1 = Files.createFile(file1); Files.write(file1, "value1".getBytes()); file2 = Files.createFile(file2); Files.write(file2, "value2".getBytes()); + fileHidden = Files.createFile(fileHidden); + + // create a subdirectory structure with test files + Path mounted = Paths.get(testDirectory.toString(), "foo", "bar"); + Files.createDirectories(mounted); + + Path fileMounted = Paths.get(mounted.toString(), "property3"); + fileMounted = Files.createFile(fileMounted); + Files.write(fileMounted, "value3".getBytes()); + + // create "foo/bar/..data/property4" and symlink from "foo/bar/property4" as done on K8s + Path mountedK8s = Paths.get(mounted.toString(), "..data"); + Files.createDirectories(mountedK8s); + Path fileK8sMounted = Paths.get(mountedK8s.toString(), "property4"); + fileK8sMounted = Files.createFile(fileK8sMounted); + Files.write(fileK8sMounted, "value4".getBytes()); + Path fileK8sSymlink = Paths.get(mounted.toString(), "property4"); + fileK8sSymlink = Files.createSymbolicLink(fileK8sSymlink, fileK8sMounted); + + // create & load source = new SecretsDirConfigSource(testDirectory); } @After public void tearDown() throws IOException { - for (File file : testDirectory.toFile().listFiles()) { - file.delete(); - } - Files.delete(testDirectory); + Files.walk(testDirectory) + .sorted(Comparator.reverseOrder()) + .map(Path::toFile) + .forEach(File::delete); } /** @@ -95,6 +118,8 @@ public void testGetProperties() { Map expected = new HashMap<>(); expected.put("property1", "value1"); expected.put("property2", "value2"); + expected.put("foo.bar.property3", "value3"); + expected.put("foo.bar.property4", "value4"); assertEquals(expected, source.getProperties()); } @@ -103,7 +128,7 @@ public void testGetProperties() { */ @Test public void testGetPropertyNames() { - assertEquals(new HashSet<>(asList("property1", "property2")), source.getPropertyNames()); + assertEquals(new HashSet<>(asList("property1", "property2", "foo.bar.property3", "foo.bar.property4")), source.getPropertyNames()); } /** @@ -113,6 +138,8 @@ public void testGetPropertyNames() { public void testGetValue() { assertEquals("value1", source.getValue("property1")); assertEquals("value2", source.getValue("property2")); + assertEquals("value3", source.getValue("foo.bar.property3")); + assertEquals("value4", source.getValue("foo.bar.property4")); } /** @@ -142,7 +169,27 @@ public void testChangeProperty() throws IOException { Files.write(file1, "value1".getBytes()); } } - + + /** + * Test the changed Property in subdirectories + * @throws java.io.IOException + */ + @Test + public void testChangePropertyInSubdir() throws IOException { + assertEquals("value4", source.getValue("foo.bar.property4")); + // change the file + Path file = Paths.get(testDirectory.toString(), "foo", "bar", "..data", "property4"); + Files.write(file, "value-changed".getBytes()); + try { + FileTime nowplus1sec = FileTime.fromMillis(System.currentTimeMillis() + TimeUnit.SECONDS.toMillis(1)); + Files.setLastModifiedTime(file, nowplus1sec); + assertEquals("value-changed", source.getValue("foo.bar.property4")); + } finally { + // clean up + Files.write(file, "value4".getBytes()); + } + } + /** * Tests getting a new property as the file has now appeared */ @@ -159,7 +206,27 @@ public void testNewFile() throws IOException { Files.delete(file1); } } - + + /** + * Tests getting a new property as the file has now appeared in a subdirectory + */ + @Test + public void testNewFileInSubdir() throws IOException { + assertNull(source.getValue("foo.bar.property-new")); + // change the file + Path file = Paths.get(testDirectory.toString(), "foo", "bar", "..data", "property-new"); + Files.write(file, "newValue".getBytes()); + Path fileSymlink = Paths.get(testDirectory.toString(), "foo", "bar", "property-new"); + Files.createSymbolicLink(fileSymlink, file); + try { + assertEquals("newValue", source.getValue("foo.bar.property-new")); + } finally { + // clean up + Files.delete(file); + Files.delete(fileSymlink); + } + } + @Test public void testBadDirectoryNoBlowUp() { assertNull(new SecretsDirConfigSource(Paths.get(testDirectory.toString(), "FOOBLE")).getValue("BILLY")); From 74288cbe744d64f82088a717663c822dcf8462f6 Mon Sep 17 00:00:00 2001 From: Oliver Bertuch Date: Thu, 26 Nov 2020 12:56:40 +0100 Subject: [PATCH 02/23] Revert glob import from java.nio and java.util introduced in f2795e2a. --- .../config/source/SecretsDirConfigSource.java | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/nucleus/payara-modules/nucleus-microprofile/config-service/src/main/java/fish/payara/nucleus/microprofile/config/source/SecretsDirConfigSource.java b/nucleus/payara-modules/nucleus-microprofile/config-service/src/main/java/fish/payara/nucleus/microprofile/config/source/SecretsDirConfigSource.java index a8b1fa1e037..bb9f0796856 100644 --- a/nucleus/payara-modules/nucleus-microprofile/config-service/src/main/java/fish/payara/nucleus/microprofile/config/source/SecretsDirConfigSource.java +++ b/nucleus/payara-modules/nucleus-microprofile/config-service/src/main/java/fish/payara/nucleus/microprofile/config/source/SecretsDirConfigSource.java @@ -43,10 +43,17 @@ import java.io.File; import java.io.IOException; -import java.nio.file.*; +import java.nio.file.FileVisitResult; +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.Paths; +import java.nio.file.SimpleFileVisitor; import java.nio.file.attribute.BasicFileAttributes; import java.nio.file.attribute.FileTime; -import java.util.*; +import java.util.Arrays; +import java.util.List; +import java.util.Map; +import java.util.Set; import java.util.concurrent.ConcurrentHashMap; import java.util.logging.Level; import java.util.logging.Logger; From 2a6f8c385456c0482079a5948fe1ac964c95071f Mon Sep 17 00:00:00 2001 From: Oliver Bertuch Date: Mon, 30 Nov 2020 19:35:06 +0100 Subject: [PATCH 03/23] Add first draft of complete rewrite of the SecretsDirConfigSource moving to DirConfigSource. #5006 --- .../config/source/DirConfigSource.java | 352 ++++++++++++++++++ .../spi/ConfigProviderResolverImpl.java | 9 +- 2 files changed, 359 insertions(+), 2 deletions(-) create mode 100644 nucleus/payara-modules/nucleus-microprofile/config-service/src/main/java/fish/payara/nucleus/microprofile/config/source/DirConfigSource.java diff --git a/nucleus/payara-modules/nucleus-microprofile/config-service/src/main/java/fish/payara/nucleus/microprofile/config/source/DirConfigSource.java b/nucleus/payara-modules/nucleus-microprofile/config-service/src/main/java/fish/payara/nucleus/microprofile/config/source/DirConfigSource.java new file mode 100644 index 00000000000..03808f0dcc7 --- /dev/null +++ b/nucleus/payara-modules/nucleus-microprofile/config-service/src/main/java/fish/payara/nucleus/microprofile/config/source/DirConfigSource.java @@ -0,0 +1,352 @@ +/* + * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS HEADER. + * + * Copyright (c) [2017-2020] Payara Foundation and/or its affiliates. All rights reserved. + * + * The contents of this file are subject to the terms of either the GNU + * General Public License Version 2 only ("GPL") or the Common Development + * and Distribution License("CDDL") (collectively, the "License"). You + * may not use this file except in compliance with the License. You can + * obtain a copy of the License at + * https://github.com/payara/Payara/blob/master/LICENSE.txt + * See the License for the specific + * language governing permissions and limitations under the License. + * + * When distributing the software, include this License Header Notice in each + * file and include the License file at glassfish/legal/LICENSE.txt. + * + * GPL Classpath Exception: + * The Payara Foundation designates this particular file as subject to the "Classpath" + * exception as provided by the Payara Foundation in the GPL Version 2 section of the License + * file that accompanied this code. + * + * Modifications: + * If applicable, add the following below the License Header, with the fields + * enclosed by brackets [] replaced by your own identifying information: + * "Portions Copyright [year] [name of copyright owner]" + * + * Contributor(s): + * If you wish your version of this file to be governed by only the CDDL or + * only the GPL Version 2, indicate your decision by adding "[Contributor] + * elects to include this software in this distribution under the [CDDL or GPL + * Version 2] license." If you don't indicate a single choice of license, a + * recipient has the option to distribute your version of this file under + * either the CDDL, the GPL Version 2 or to extend the choice of license to + * its licensees as provided above. However, if you add GPL Version 2 code + * and therefore, elected the GPL Version 2 license, then the option applies + * only if the new code is made subject to such option by the copyright + * holder. + */ +package fish.payara.nucleus.microprofile.config.source; + +import fish.payara.nucleus.executorservice.PayaraExecutorService; +import org.eclipse.microprofile.config.spi.ConfigSource; + +import java.io.File; +import java.io.IOException; +import java.nio.file.*; +import java.nio.file.attribute.BasicFileAttributes; +import java.nio.file.attribute.FileTime; +import java.util.*; +import java.util.concurrent.ConcurrentHashMap; +import java.util.logging.Level; +import java.util.logging.Logger; +import java.util.stream.Collectors; + +import static java.util.Collections.unmodifiableMap; +import static java.nio.file.StandardWatchEventKinds.ENTRY_CREATE; +import static java.nio.file.StandardWatchEventKinds.ENTRY_DELETE; +import static java.nio.file.StandardWatchEventKinds.ENTRY_MODIFY; + +/** + * Config Source that reads properties from files from a directory + * where filename is the property name and file contents is the property value. + * @since 5.2020.7 + */ +public class DirConfigSource extends PayaraConfigSource implements ConfigSource { + + class DirProperty { + String property; + FileTime lastModifiedTime; + Path path; + int pathDepth; + + DirProperty(String property, FileTime lastModifiedTime, Path path, int pathDepth) { + this.property = property; + this.lastModifiedTime = lastModifiedTime; + this.path = path; + this.pathDepth = pathDepth; + } + } + + class DirPropertyWatcher implements Runnable { + + private Logger logger = Logger.getLogger(DirConfigSource.class.getName()); + WatchService watcher = FileSystems.getDefault().newWatchService(); + ConcurrentHashMap keys = new ConcurrentHashMap<>(); + + DirPropertyWatcher(Path topmostDir) throws IOException { + if (Files.exists(topmostDir) && Files.isDirectory(topmostDir) && Files.isReadable(topmostDir)) { + registerAll(topmostDir); + } else { + throw new IOException("Given directory '"+topmostDir+"' is no directory or cannot be read."); + } + } + + void registerAll(Path dir) throws IOException { + // register file watchers recursively (they don't attach to subdirs...) + Files.walkFileTree(dir, new SimpleFileVisitor() { + @Override + public FileVisitResult preVisitDirectory(Path dir, BasicFileAttributes attrs) throws IOException + { + register(dir); + return FileVisitResult.CONTINUE; + } + }); + } + + void register(Path dir) throws IOException { + WatchKey key = dir.register(watcher, ENTRY_CREATE, ENTRY_DELETE, ENTRY_MODIFY); + keys.putIfAbsent(key, dir); + } + + @Override + public void run() { + while (true) { + // wait infinitely until we receive an event (or the executor is shutting down) + WatchKey key; + try { + key = watcher.take(); + } catch (InterruptedException ex) { + return; + } + + Path workDir = keys.get(key); + for (WatchEvent event : key.pollEvents()) { + WatchEvent.Kind kind = event.kind(); + + @SuppressWarnings("unchecked") + WatchEvent ev = (WatchEvent) event; + Path fileName = ev.context(); + Path path = workDir.resolve(fileName); + + try { + // new directory to be watched and traversed + if (kind == ENTRY_CREATE && Files.isDirectory(path) && !Files.isHidden(path) && Files.isReadable(path)) { + registerAll(path); + initializePropertiesFromPath(path); + } + // new or updated file found + if (Files.isRegularFile(path) && (kind == ENTRY_CREATE || kind == ENTRY_MODIFY)) { + updatePropertyFromPath(path, Files.readAttributes(path, BasicFileAttributes.class)); + } + if (Files.isRegularFile(path) && (kind == ENTRY_DELETE)) { + properties.remove(parsePropertyNameFromPath(path)); + } + } catch (IOException e) { + logger.log(Level.SEVERE, "Could not process event '"+kind+"' on '"+path+"'", e); + } + } + + // Reset key (obligatory) and remove from set if directory no longer accessible + boolean valid = key.reset(); + if (!valid) { + keys.remove(key); + + // all directories became inaccessible, even topmostDir! + if (keys.isEmpty()) break; + } + } + } + } + + class DirConfigFileVisitor extends SimpleFileVisitor { + /** + * Ignore hidden directories + */ + @Override + public FileVisitResult preVisitDirectory(java.nio.file.Path dir, BasicFileAttributes attrs) throws IOException { + return dir.toFile().isHidden() ? FileVisitResult.SKIP_SUBTREE : FileVisitResult.CONTINUE; + } + + @Override + public FileVisitResult visitFile(Path path, BasicFileAttributes mainAtts) throws IOException { + updatePropertyFromPath(path, mainAtts); + return FileVisitResult.CONTINUE; + } + } + + private Path directory; + private ConcurrentHashMap properties = new ConcurrentHashMap<>(); + private static Logger logger = Logger.getLogger(DirConfigSource.class.getName()); + + public DirConfigSource(PayaraExecutorService executorService) { + // get the directory from the app server config + findDir(); + + try { + // create the watcher for the directory + executorService.submit(new DirPropertyWatcher(this.directory)); + // initial loading + initializePropertiesFromPath(this.directory); + } catch (IOException e) { + logger.log(Level.SEVERE, "Error during setup of MicroProfile Config Directory Source", e); + } + } + + // Used for testing only with explicit dependency injection + DirConfigSource(Path directory, PayaraExecutorService executorService) { + super(true); + this.directory = directory; + try { + initializePropertiesFromPath(this.directory); + } catch (IOException e) { + logger.log(Level.SEVERE, "Error during setup of MicroProfile Config Directory Source", e); + } + } + + @Override + public Map getProperties() { + return unmodifiableMap(properties + .entrySet() + .stream() + .collect( + Collectors.toMap(Map.Entry::getKey, e -> e.getValue().property) + )); + } + + @Override + public Set getPropertyNames() { + return properties.keySet(); + } + + @Override + public int getOrdinal() { + return Integer.parseInt(configService.getMPConfig().getSecretDirOrdinality()); + } + + @Override + public String getValue(String property) { + DirProperty result = properties.get(property); + return result == null ? null : result.property; + } + + @Override + public String getName() { + return "Directory"; + } + + private void findDir() { + List candidates = Arrays.asList( + Paths.get(configService.getMPConfig().getSecretDir()), + // let's try it relative to server environment root + Paths.get(System.getProperty("com.sun.aas.instanceRoot"), configService.getMPConfig().getSecretDir()) + ); + for (Path candidate : candidates) { + if (Files.exists(candidate) || Files.isDirectory(candidate) || Files.isReadable(candidate)) { + this.directory = candidate; + return; + } + } + } + + void initializePropertiesFromPath(Path topmostDir) throws IOException { + if (Files.exists(topmostDir) && Files.isDirectory(topmostDir) && Files.isReadable(topmostDir)) { + // initialize properties on first run + Files.walkFileTree(topmostDir, new DirConfigFileVisitor()); + } else { + throw new IOException("Given directory '"+topmostDir+"' is no directory or cannot be read."); + } + } + + void updatePropertyFromPath(Path path, BasicFileAttributes mainAtts) throws IOException { + // do not read hidden files, as K8s Secret filenames are symlinks to hidden files with data. + // also ignore files > 512KB, as they are most likely no text config files... + if (Files.isRegularFile(path) && ! Files.isHidden(path) && Files.isReadable(path) && mainAtts.size() < 512*1024) { + // retrieve the property name from the file path + String property = parsePropertyNameFromPath(path); + + // Conflict handling: + // When this property is already present, check how to solve the conflict. + // This property file will be skipped if the file we already have is deeper in the file tree... + if (checkLongestMatchForPath(property, path)) { + return; + } + + properties.put(property, readPropertyFromPath(path, mainAtts)); + } + } + + void removePropertyFromPath(Path path) { + String property = parsePropertyNameFromPath(path); + + // not present? go away silently. + if (! properties.containsKey(property)) return; + + // only delete from the map if the file that has been deleted is the same as the one stored in the map + // -> deleting a file less specific but matching a property should not remove from the map + // -> deleting a file more specific than in map shouldn't occur (it had to slip through longest match check then). + if (path.equals(properties.get(property).path)) { + properties.remove(property); + } + + } + + String parsePropertyNameFromPath(Path path) { + // 1. get relative path based on the config dir ("/config"), + String property = directory.relativize(path.getParent()).toString(); + // 2. ignore all file suffixes after last dot + property += path.getFileName().toString().substring(0, path.getFileName().toString().lastIndexOf('.')-1); + // 3. replace all path seps with a ".", + property = property.replace(File.separatorChar, '.'); + // so "/config/foo/bar/test/one.txt" becomes "foo/bar/test/one.txt" becomes "foo.bar.test.one" property name + return property; + } + + /** + * Check if the path given is a more specific path to a value for the given property + * @param property + * @param path + * @return true if more specific, false if not + */ + boolean checkLongestMatchForPath(String property, Path path) { + // Make path relative to config directory + // NOTE: we will never have a path containing "..", as our tree walkers are always inside this "root". + Path relativePath = directory.relativize(path); + + // No property -> path is new and more specific + if (! properties.containsKey(property)) + return true; + DirProperty old = properties.get(property); + + // Check if this element has a higher path depth (longest match) + // Example: "foo.bar/test/one.txt" (depth 2) wins over "foo.bar.test.one.txt" (depth 0) + boolean depth = old.pathDepth > relativePath.getNameCount(); + + // In case that both pathes have the same depth, we need to check on the position of dots. + // Example: /config/foo.bar/test/one.txt is less specific than /config/foo/bar.test/one.txt + if (old.pathDepth == relativePath.getNameCount()) { + String oldPath = old.path.toString(); + String newPath = path.toAbsolutePath().toString(); + int offset = 0; + while (offset > -1) { + if (newPath.indexOf(".", offset) > oldPath.indexOf(".", offset)) return true; + offset = oldPath.indexOf(".", offset + 1); + } + } + return depth; + } + + DirProperty readPropertyFromPath(Path path, BasicFileAttributes mainAtts) throws IOException { + if (Files.exists(path) && Files.isRegularFile(path) && Files.isReadable(path)) { + return new DirProperty( + new String(Files.readAllBytes(path)), + mainAtts.lastModifiedTime(), + path.toAbsolutePath(), + directory.relativize(path).getNameCount() + ); + } + return null; + } + +} diff --git a/nucleus/payara-modules/nucleus-microprofile/config-service/src/main/java/fish/payara/nucleus/microprofile/config/spi/ConfigProviderResolverImpl.java b/nucleus/payara-modules/nucleus-microprofile/config-service/src/main/java/fish/payara/nucleus/microprofile/config/spi/ConfigProviderResolverImpl.java index 2ac8ed3da73..b92344e596c 100644 --- a/nucleus/payara-modules/nucleus-microprofile/config-service/src/main/java/fish/payara/nucleus/microprofile/config/spi/ConfigProviderResolverImpl.java +++ b/nucleus/payara-modules/nucleus-microprofile/config-service/src/main/java/fish/payara/nucleus/microprofile/config/spi/ConfigProviderResolverImpl.java @@ -63,6 +63,7 @@ import javax.inject.Inject; import javax.inject.Named; +import fish.payara.nucleus.executorservice.PayaraExecutorService; import org.eclipse.microprofile.config.Config; import org.eclipse.microprofile.config.spi.ConfigBuilder; import org.eclipse.microprofile.config.spi.ConfigProviderResolver; @@ -104,7 +105,7 @@ import fish.payara.nucleus.microprofile.config.source.PayaraExpressionConfigSource; import fish.payara.nucleus.microprofile.config.source.PayaraServerProperties; import fish.payara.nucleus.microprofile.config.source.PropertiesConfigSource; -import fish.payara.nucleus.microprofile.config.source.SecretsDirConfigSource; +import fish.payara.nucleus.microprofile.config.source.DirConfigSource; import fish.payara.nucleus.microprofile.config.source.ServerConfigSource; import fish.payara.nucleus.microprofile.config.source.SystemPropertyConfigSource; import fish.payara.nucleus.microprofile.config.source.extension.ExtensionConfigSourceService; @@ -134,6 +135,10 @@ public class ConfigProviderResolverImpl extends ConfigProviderResolver { @Inject private ServerContext context; + + // Some sources might want to execute background tasks in a controlled fashion + @Inject + private PayaraExecutorService executorService; // Gives access to deployed applications @Inject @@ -317,7 +322,7 @@ private List getDefaultSources(String appName, String moduleName) sources.add(new SystemPropertyConfigSource()); sources.add(new JNDIConfigSource()); sources.add(new PayaraServerProperties()); - sources.add(new SecretsDirConfigSource()); + sources.add(new DirConfigSource(executorService)); sources.add(new PasswordAliasConfigSource()); sources.add(new JDBCConfigSource()); if (appName != null) { From dceb4c70a8b5242849c9ef044d79103f22b21f49 Mon Sep 17 00:00:00 2001 From: Oliver Bertuch Date: Mon, 30 Nov 2020 19:36:06 +0100 Subject: [PATCH 04/23] Remove SecretsDirConfigSource, replaced with DirConfigSource. #5006 --- .../config/source/SecretsDirConfigSource.java | 203 ------------------ 1 file changed, 203 deletions(-) delete mode 100644 nucleus/payara-modules/nucleus-microprofile/config-service/src/main/java/fish/payara/nucleus/microprofile/config/source/SecretsDirConfigSource.java diff --git a/nucleus/payara-modules/nucleus-microprofile/config-service/src/main/java/fish/payara/nucleus/microprofile/config/source/SecretsDirConfigSource.java b/nucleus/payara-modules/nucleus-microprofile/config-service/src/main/java/fish/payara/nucleus/microprofile/config/source/SecretsDirConfigSource.java deleted file mode 100644 index bb9f0796856..00000000000 --- a/nucleus/payara-modules/nucleus-microprofile/config-service/src/main/java/fish/payara/nucleus/microprofile/config/source/SecretsDirConfigSource.java +++ /dev/null @@ -1,203 +0,0 @@ -/* - * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS HEADER. - * - * Copyright (c) [2017-2020] Payara Foundation and/or its affiliates. All rights reserved. - * - * The contents of this file are subject to the terms of either the GNU - * General Public License Version 2 only ("GPL") or the Common Development - * and Distribution License("CDDL") (collectively, the "License"). You - * may not use this file except in compliance with the License. You can - * obtain a copy of the License at - * https://github.com/payara/Payara/blob/master/LICENSE.txt - * See the License for the specific - * language governing permissions and limitations under the License. - * - * When distributing the software, include this License Header Notice in each - * file and include the License file at glassfish/legal/LICENSE.txt. - * - * GPL Classpath Exception: - * The Payara Foundation designates this particular file as subject to the "Classpath" - * exception as provided by the Payara Foundation in the GPL Version 2 section of the License - * file that accompanied this code. - * - * Modifications: - * If applicable, add the following below the License Header, with the fields - * enclosed by brackets [] replaced by your own identifying information: - * "Portions Copyright [year] [name of copyright owner]" - * - * Contributor(s): - * If you wish your version of this file to be governed by only the CDDL or - * only the GPL Version 2, indicate your decision by adding "[Contributor] - * elects to include this software in this distribution under the [CDDL or GPL - * Version 2] license." If you don't indicate a single choice of license, a - * recipient has the option to distribute your version of this file under - * either the CDDL, the GPL Version 2 or to extend the choice of license to - * its licensees as provided above. However, if you add GPL Version 2 code - * and therefore, elected the GPL Version 2 license, then the option applies - * only if the new code is made subject to such option by the copyright - * holder. - */ -package fish.payara.nucleus.microprofile.config.source; - -import static java.util.Collections.unmodifiableMap; - -import java.io.File; -import java.io.IOException; -import java.nio.file.FileVisitResult; -import java.nio.file.Files; -import java.nio.file.Path; -import java.nio.file.Paths; -import java.nio.file.SimpleFileVisitor; -import java.nio.file.attribute.BasicFileAttributes; -import java.nio.file.attribute.FileTime; -import java.util.Arrays; -import java.util.List; -import java.util.Map; -import java.util.Set; -import java.util.concurrent.ConcurrentHashMap; -import java.util.logging.Level; -import java.util.logging.Logger; -import org.eclipse.microprofile.config.spi.ConfigSource; - -/** - * Config Source that reads properties from files from a directory - * where filename is the property name and file contents is the property value. - * @since 4.1.181 - * @author steve - */ -public class SecretsDirConfigSource extends PayaraConfigSource implements ConfigSource { - - private Path secretsDir; - private ConcurrentHashMap properties; - private ConcurrentHashMap storedModifiedTimes; - private ConcurrentHashMap storedPaths; - - public SecretsDirConfigSource() { - findDir(); - loadProperties(); - } - - SecretsDirConfigSource(Path directory) { - super(true); - secretsDir = directory; - loadProperties(); - } - - @Override - public Map getProperties() { - return unmodifiableMap(properties); - } - - @Override - public Set getPropertyNames() { - return properties.keySet(); - } - - @Override - public int getOrdinal() { - return Integer.parseInt(configService.getMPConfig().getSecretDirOrdinality()); - } - - @Override - public String getValue(String property) { - String result = properties.get(property); - if (result != null) { - try { - // check existence (secret mount might have gone away) and the last modified time - FileTime ft = storedModifiedTimes.get(property); - Path path = storedPaths.get(property); - if (Files.exists(path) && Files.getLastModifiedTime(path).compareTo(ft) > 0) { - // file has been modified since last check, re-read content - result = readFile(path); - storedModifiedTimes.put(property, Files.getLastModifiedTime(path)); - properties.put(property, result); - } - } catch (IOException ex) { - Logger.getLogger(SecretsDirConfigSource.class.getName()).log(Level.SEVERE, "Unable to read file in the directory", ex); - } - } else { - // check whether there is a file there now as there wasn't before - // --> the list of possible paths is used "first match, first serve". - List paths = Arrays.asList(Paths.get(secretsDir.toString(), property), - Paths.get(secretsDir.toString(), property.replace('.', File.separatorChar))); - for (Path path : paths) { - try { - result = readFile(path); - storedModifiedTimes.put(property, Files.getLastModifiedTime(path)); - storedPaths.put(property, path); - properties.put(property, result); - break; - } catch (IOException ex) { - Logger.getLogger(SecretsDirConfigSource.class.getName()).log(Level.SEVERE, "Unable to read file in the directory", ex); - } - } - } - return result; - } - - @Override - public String getName() { - return "Secrets Directory"; - } - - private void findDir() { - secretsDir = Paths.get(configService.getMPConfig().getSecretDir()); - - if (!Files.exists(secretsDir) || !Files.isDirectory(secretsDir) || !Files.isReadable(secretsDir)) { - // let's try it relative to server environment root - String instancePath = System.getProperty("com.sun.aas.instanceRoot"); - Path test = Paths.get(instancePath, secretsDir.toString()); - if (Files.exists(test) && Files.isDirectory(test) && Files.isReadable(test)) { - secretsDir = test; - } - } - } - - private String readFile(Path file) throws IOException { - String result = null; - if (Files.exists(file) && Files.isRegularFile(file) && Files.isReadable(file)) { - StringBuilder collector = new StringBuilder(); - for (String line : Files.readAllLines(file)) { - collector.append(line); - } - result = collector.toString(); - } - return result; - } - - private void loadProperties() { - properties = new ConcurrentHashMap<>(); - storedModifiedTimes = new ConcurrentHashMap<>(); - storedPaths = new ConcurrentHashMap<>(); - if (Files.exists(secretsDir) && Files.isDirectory(secretsDir) && Files.isReadable(secretsDir)) { - try { - Files.walkFileTree(secretsDir, new SimpleFileVisitor() { - @Override - public FileVisitResult preVisitDirectory(Path dir, BasicFileAttributes attrs) throws IOException { - return dir.toFile().isHidden() ? FileVisitResult.SKIP_SUBTREE : FileVisitResult.CONTINUE; - } - - @Override - public FileVisitResult visitFile(Path path, BasicFileAttributes mainAtts) throws IOException { - File file = path.toFile(); - // do not read hidden files, as K8s Secret filenames are symlinks to hidden files with data. - if (file.isFile() && ! file.isHidden() && file.canRead()) { - // 1. get relative path based on the secrets dir ("/foobar"), - // 2. replace all path seps with a ".", - // so "/foobar/test/foo/bar" becomes "test/foo/bar" becomes "test.foo.bar" property name - String property = secretsDir.relativize(path).toString().replace(File.separatorChar, '.'); - - properties.put(property, readFile(path)); - storedModifiedTimes.put(property, mainAtts.lastModifiedTime()); - storedPaths.put(property, path); - } - return FileVisitResult.CONTINUE; - } - }); - } catch (IOException ex) { - Logger.getLogger(SecretsDirConfigSource.class.getName()).log(Level.SEVERE, "Unable to read file in the directory", ex); - } - } - } - -} From 0b3233a9421f57328d191b59bdc64e40be99e160 Mon Sep 17 00:00:00 2001 From: Oliver Bertuch Date: Mon, 30 Nov 2020 21:44:39 +0100 Subject: [PATCH 05/23] DirConfigSource: fix imports, add equals() and hashCode() to DirProperty. #5006 --- .../config/source/DirConfigSource.java | 32 +++++++++++++++++-- 1 file changed, 30 insertions(+), 2 deletions(-) diff --git a/nucleus/payara-modules/nucleus-microprofile/config-service/src/main/java/fish/payara/nucleus/microprofile/config/source/DirConfigSource.java b/nucleus/payara-modules/nucleus-microprofile/config-service/src/main/java/fish/payara/nucleus/microprofile/config/source/DirConfigSource.java index 03808f0dcc7..dfbb245fdc0 100644 --- a/nucleus/payara-modules/nucleus-microprofile/config-service/src/main/java/fish/payara/nucleus/microprofile/config/source/DirConfigSource.java +++ b/nucleus/payara-modules/nucleus-microprofile/config-service/src/main/java/fish/payara/nucleus/microprofile/config/source/DirConfigSource.java @@ -44,10 +44,22 @@ import java.io.File; import java.io.IOException; -import java.nio.file.*; +import java.nio.file.FileSystems; +import java.nio.file.FileVisitResult; +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.Paths; +import java.nio.file.SimpleFileVisitor; +import java.nio.file.WatchEvent; +import java.nio.file.WatchKey; +import java.nio.file.WatchService; import java.nio.file.attribute.BasicFileAttributes; import java.nio.file.attribute.FileTime; -import java.util.*; +import java.util.Arrays; +import java.util.List; +import java.util.Map; +import java.util.Objects; +import java.util.Set; import java.util.concurrent.ConcurrentHashMap; import java.util.logging.Level; import java.util.logging.Logger; @@ -77,6 +89,22 @@ class DirProperty { this.path = path; this.pathDepth = pathDepth; } + + @Override + public boolean equals(Object o) { + if (this == o) return true; + if (o == null || getClass() != o.getClass()) return false; + DirProperty that = (DirProperty) o; + return pathDepth == that.pathDepth && + property.equals(that.property) && + lastModifiedTime.equals(that.lastModifiedTime) && + path.equals(that.path); + } + + @Override + public int hashCode() { + return Objects.hash(property, lastModifiedTime, path, pathDepth); + } } class DirPropertyWatcher implements Runnable { From a76cdb4428ef048916ea11b8f269044f8b89f813 Mon Sep 17 00:00:00 2001 From: Oliver Bertuch Date: Mon, 30 Nov 2020 21:48:02 +0100 Subject: [PATCH 06/23] DirConfigSource: move initial tree walker to anonymous declaration. --- .../config/source/DirConfigSource.java | 31 +++++++++---------- 1 file changed, 14 insertions(+), 17 deletions(-) diff --git a/nucleus/payara-modules/nucleus-microprofile/config-service/src/main/java/fish/payara/nucleus/microprofile/config/source/DirConfigSource.java b/nucleus/payara-modules/nucleus-microprofile/config-service/src/main/java/fish/payara/nucleus/microprofile/config/source/DirConfigSource.java index dfbb245fdc0..0c656f1a5e5 100644 --- a/nucleus/payara-modules/nucleus-microprofile/config-service/src/main/java/fish/payara/nucleus/microprofile/config/source/DirConfigSource.java +++ b/nucleus/payara-modules/nucleus-microprofile/config-service/src/main/java/fish/payara/nucleus/microprofile/config/source/DirConfigSource.java @@ -188,22 +188,6 @@ public void run() { } } - class DirConfigFileVisitor extends SimpleFileVisitor { - /** - * Ignore hidden directories - */ - @Override - public FileVisitResult preVisitDirectory(java.nio.file.Path dir, BasicFileAttributes attrs) throws IOException { - return dir.toFile().isHidden() ? FileVisitResult.SKIP_SUBTREE : FileVisitResult.CONTINUE; - } - - @Override - public FileVisitResult visitFile(Path path, BasicFileAttributes mainAtts) throws IOException { - updatePropertyFromPath(path, mainAtts); - return FileVisitResult.CONTINUE; - } - } - private Path directory; private ConcurrentHashMap properties = new ConcurrentHashMap<>(); private static Logger logger = Logger.getLogger(DirConfigSource.class.getName()); @@ -281,7 +265,20 @@ private void findDir() { void initializePropertiesFromPath(Path topmostDir) throws IOException { if (Files.exists(topmostDir) && Files.isDirectory(topmostDir) && Files.isReadable(topmostDir)) { // initialize properties on first run - Files.walkFileTree(topmostDir, new DirConfigFileVisitor()); + Files.walkFileTree(topmostDir, new SimpleFileVisitor() { + // Ignore hidden directories + @Override + public FileVisitResult preVisitDirectory(java.nio.file.Path dir, BasicFileAttributes attrs) throws IOException { + return dir.toFile().isHidden() ? FileVisitResult.SKIP_SUBTREE : FileVisitResult.CONTINUE; + } + + // Read and ingest all files and dirs present + @Override + public FileVisitResult visitFile(Path path, BasicFileAttributes mainAtts) throws IOException { + updatePropertyFromPath(path, mainAtts); + return FileVisitResult.CONTINUE; + } + }); } else { throw new IOException("Given directory '"+topmostDir+"' is no directory or cannot be read."); } From ccde673deae9a9cab5e102ef4409c34de6a4f25c Mon Sep 17 00:00:00 2001 From: Oliver Bertuch Date: Mon, 7 Dec 2020 12:31:56 +0100 Subject: [PATCH 07/23] Refactor after review by @jbee. Mostly scope and naming changes. #5006 --- .../config/source/DirConfigSource.java | 59 +++++++++---------- 1 file changed, 28 insertions(+), 31 deletions(-) diff --git a/nucleus/payara-modules/nucleus-microprofile/config-service/src/main/java/fish/payara/nucleus/microprofile/config/source/DirConfigSource.java b/nucleus/payara-modules/nucleus-microprofile/config-service/src/main/java/fish/payara/nucleus/microprofile/config/source/DirConfigSource.java index 0c656f1a5e5..4c21837f567 100644 --- a/nucleus/payara-modules/nucleus-microprofile/config-service/src/main/java/fish/payara/nucleus/microprofile/config/source/DirConfigSource.java +++ b/nucleus/payara-modules/nucleus-microprofile/config-service/src/main/java/fish/payara/nucleus/microprofile/config/source/DirConfigSource.java @@ -44,6 +44,7 @@ import java.io.File; import java.io.IOException; +import java.nio.charset.StandardCharsets; import java.nio.file.FileSystems; import java.nio.file.FileVisitResult; import java.nio.file.Files; @@ -63,9 +64,9 @@ import java.util.concurrent.ConcurrentHashMap; import java.util.logging.Level; import java.util.logging.Logger; -import java.util.stream.Collectors; import static java.util.Collections.unmodifiableMap; +import static java.util.stream.Collectors.toMap; import static java.nio.file.StandardWatchEventKinds.ENTRY_CREATE; import static java.nio.file.StandardWatchEventKinds.ENTRY_DELETE; import static java.nio.file.StandardWatchEventKinds.ENTRY_MODIFY; @@ -77,11 +78,11 @@ */ public class DirConfigSource extends PayaraConfigSource implements ConfigSource { - class DirProperty { - String property; - FileTime lastModifiedTime; - Path path; - int pathDepth; + static final class DirProperty { + final String property; + final FileTime lastModifiedTime; + final Path path; + final int pathDepth; DirProperty(String property, FileTime lastModifiedTime, Path path, int pathDepth) { this.property = property; @@ -107,11 +108,11 @@ public int hashCode() { } } - class DirPropertyWatcher implements Runnable { + final class DirPropertyWatcher implements Runnable { - private Logger logger = Logger.getLogger(DirConfigSource.class.getName()); - WatchService watcher = FileSystems.getDefault().newWatchService(); - ConcurrentHashMap keys = new ConcurrentHashMap<>(); + private final Logger logger = Logger.getLogger(DirConfigSource.class.getName()); + private final WatchService watcher = FileSystems.getDefault().newWatchService(); + private final ConcurrentHashMap watchedFileKeys = new ConcurrentHashMap<>(); DirPropertyWatcher(Path topmostDir) throws IOException { if (Files.exists(topmostDir) && Files.isDirectory(topmostDir) && Files.isReadable(topmostDir)) { @@ -135,7 +136,7 @@ public FileVisitResult preVisitDirectory(Path dir, BasicFileAttributes attrs) th void register(Path dir) throws IOException { WatchKey key = dir.register(watcher, ENTRY_CREATE, ENTRY_DELETE, ENTRY_MODIFY); - keys.putIfAbsent(key, dir); + watchedFileKeys.putIfAbsent(key, dir); } @Override @@ -149,7 +150,7 @@ public void run() { return; } - Path workDir = keys.get(key); + Path workDir = watchedFileKeys.get(key); for (WatchEvent event : key.pollEvents()) { WatchEvent.Kind kind = event.kind(); @@ -169,7 +170,7 @@ public void run() { updatePropertyFromPath(path, Files.readAttributes(path, BasicFileAttributes.class)); } if (Files.isRegularFile(path) && (kind == ENTRY_DELETE)) { - properties.remove(parsePropertyNameFromPath(path)); + removePropertyFromPath(path); } } catch (IOException e) { logger.log(Level.SEVERE, "Could not process event '"+kind+"' on '"+path+"'", e); @@ -179,24 +180,23 @@ public void run() { // Reset key (obligatory) and remove from set if directory no longer accessible boolean valid = key.reset(); if (!valid) { - keys.remove(key); + watchedFileKeys.remove(key); // all directories became inaccessible, even topmostDir! - if (keys.isEmpty()) break; + if (watchedFileKeys.isEmpty()) break; } } } } + private static final Logger logger = Logger.getLogger(DirConfigSource.class.getName()); private Path directory; private ConcurrentHashMap properties = new ConcurrentHashMap<>(); - private static Logger logger = Logger.getLogger(DirConfigSource.class.getName()); public DirConfigSource(PayaraExecutorService executorService) { - // get the directory from the app server config - findDir(); - try { + // get the directory from the app server config + this.directory = findDir(); // create the watcher for the directory executorService.submit(new DirPropertyWatcher(this.directory)); // initial loading @@ -219,12 +219,8 @@ public DirConfigSource(PayaraExecutorService executorService) { @Override public Map getProperties() { - return unmodifiableMap(properties - .entrySet() - .stream() - .collect( - Collectors.toMap(Map.Entry::getKey, e -> e.getValue().property) - )); + return unmodifiableMap(properties.entrySet().stream() + .collect(toMap(Map.Entry::getKey, e -> e.getValue().property))); } @Override @@ -248,18 +244,19 @@ public String getName() { return "Directory"; } - private void findDir() { + private Path findDir() throws IOException { + String path = configService.getMPConfig().getSecretDir(); List candidates = Arrays.asList( - Paths.get(configService.getMPConfig().getSecretDir()), + Paths.get(path), // let's try it relative to server environment root - Paths.get(System.getProperty("com.sun.aas.instanceRoot"), configService.getMPConfig().getSecretDir()) + Paths.get(System.getProperty("com.sun.aas.instanceRoot"), path) ); for (Path candidate : candidates) { if (Files.exists(candidate) || Files.isDirectory(candidate) || Files.isReadable(candidate)) { - this.directory = candidate; - return; + return candidate; } } + throw new IOException("Given MPCONFIG directory '"+path+"' is no directory or cannot be read."); } void initializePropertiesFromPath(Path topmostDir) throws IOException { @@ -365,7 +362,7 @@ boolean checkLongestMatchForPath(String property, Path path) { DirProperty readPropertyFromPath(Path path, BasicFileAttributes mainAtts) throws IOException { if (Files.exists(path) && Files.isRegularFile(path) && Files.isReadable(path)) { return new DirProperty( - new String(Files.readAllBytes(path)), + new String(Files.readAllBytes(path), StandardCharsets.UTF_8), mainAtts.lastModifiedTime(), path.toAbsolutePath(), directory.relativize(path).getNameCount() From 763f400bae4ff473a181d8367b8c709974d24d38 Mon Sep 17 00:00:00 2001 From: Oliver Bertuch Date: Mon, 7 Dec 2020 13:03:30 +0100 Subject: [PATCH 08/23] Make DirConfigSource retrieve the executor from the global ConfigProviderResolverImpl instead of explicit dep inject. #5006 --- .../microprofile/config/source/DirConfigSource.java | 8 ++++---- .../config/spi/ConfigProviderResolverImpl.java | 6 +++++- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/nucleus/payara-modules/nucleus-microprofile/config-service/src/main/java/fish/payara/nucleus/microprofile/config/source/DirConfigSource.java b/nucleus/payara-modules/nucleus-microprofile/config-service/src/main/java/fish/payara/nucleus/microprofile/config/source/DirConfigSource.java index 4c21837f567..8226a6933b6 100644 --- a/nucleus/payara-modules/nucleus-microprofile/config-service/src/main/java/fish/payara/nucleus/microprofile/config/source/DirConfigSource.java +++ b/nucleus/payara-modules/nucleus-microprofile/config-service/src/main/java/fish/payara/nucleus/microprofile/config/source/DirConfigSource.java @@ -191,14 +191,14 @@ public void run() { private static final Logger logger = Logger.getLogger(DirConfigSource.class.getName()); private Path directory; - private ConcurrentHashMap properties = new ConcurrentHashMap<>(); + private final ConcurrentHashMap properties = new ConcurrentHashMap<>(); - public DirConfigSource(PayaraExecutorService executorService) { + public DirConfigSource() { try { // get the directory from the app server config this.directory = findDir(); // create the watcher for the directory - executorService.submit(new DirPropertyWatcher(this.directory)); + configService.getExecutor().submit(new DirPropertyWatcher(this.directory)); // initial loading initializePropertiesFromPath(this.directory); } catch (IOException e) { @@ -207,7 +207,7 @@ public DirConfigSource(PayaraExecutorService executorService) { } // Used for testing only with explicit dependency injection - DirConfigSource(Path directory, PayaraExecutorService executorService) { + DirConfigSource(Path directory) { super(true); this.directory = directory; try { diff --git a/nucleus/payara-modules/nucleus-microprofile/config-service/src/main/java/fish/payara/nucleus/microprofile/config/spi/ConfigProviderResolverImpl.java b/nucleus/payara-modules/nucleus-microprofile/config-service/src/main/java/fish/payara/nucleus/microprofile/config/spi/ConfigProviderResolverImpl.java index b92344e596c..7e9312f92ce 100644 --- a/nucleus/payara-modules/nucleus-microprofile/config-service/src/main/java/fish/payara/nucleus/microprofile/config/spi/ConfigProviderResolverImpl.java +++ b/nucleus/payara-modules/nucleus-microprofile/config-service/src/main/java/fish/payara/nucleus/microprofile/config/spi/ConfigProviderResolverImpl.java @@ -297,6 +297,10 @@ public ConfigBuilder getBuilder() { return new PayaraConfigBuilder(this); } + public PayaraExecutorService getExecutor() { + return this.executorService; + } + Config getNamedConfig(String applicationName) { Config result = null; ApplicationInfo info = applicationRegistry.get(applicationName); @@ -322,7 +326,7 @@ private List getDefaultSources(String appName, String moduleName) sources.add(new SystemPropertyConfigSource()); sources.add(new JNDIConfigSource()); sources.add(new PayaraServerProperties()); - sources.add(new DirConfigSource(executorService)); + sources.add(new DirConfigSource()); sources.add(new PasswordAliasConfigSource()); sources.add(new JDBCConfigSource()); if (appName != null) { From 7601c8d90169d94c7818506adf311f1a741a79b3 Mon Sep 17 00:00:00 2001 From: Oliver Bertuch Date: Mon, 7 Dec 2020 15:38:18 +0100 Subject: [PATCH 09/23] Add initial test DirConfigSourceTest and remove SecretDirConfigSourceTest --- .../config/source/DirConfigSourceTest.java | 130 ++++++++++ .../config/source/NewTestSuite.java | 2 +- .../source/SecretsDirConfigSourceTest.java | 234 ------------------ 3 files changed, 131 insertions(+), 235 deletions(-) create mode 100644 nucleus/payara-modules/nucleus-microprofile/config-service/src/test/java/fish/payara/nucleus/microprofile/config/source/DirConfigSourceTest.java delete mode 100644 nucleus/payara-modules/nucleus-microprofile/config-service/src/test/java/fish/payara/nucleus/microprofile/config/source/SecretsDirConfigSourceTest.java diff --git a/nucleus/payara-modules/nucleus-microprofile/config-service/src/test/java/fish/payara/nucleus/microprofile/config/source/DirConfigSourceTest.java b/nucleus/payara-modules/nucleus-microprofile/config-service/src/test/java/fish/payara/nucleus/microprofile/config/source/DirConfigSourceTest.java new file mode 100644 index 00000000000..c9cef9831e8 --- /dev/null +++ b/nucleus/payara-modules/nucleus-microprofile/config-service/src/test/java/fish/payara/nucleus/microprofile/config/source/DirConfigSourceTest.java @@ -0,0 +1,130 @@ +/* + * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS HEADER. + * + * Copyright (c) 2017-2020 Payara Foundation and/or its affiliates. All rights reserved. + * + * The contents of this file are subject to the terms of either the GNU + * General Public License Version 2 only ("GPL") or the Common Development + * and Distribution License("CDDL") (collectively, the "License"). You + * may not use this file except in compliance with the License. You can + * obtain a copy of the License at + * https://github.com/payara/Payara/blob/master/LICENSE.txt + * See the License for the specific + * language governing permissions and limitations under the License. + * + * When distributing the software, include this License Header Notice in each + * file and include the License file at glassfish/legal/LICENSE.txt. + * + * GPL Classpath Exception: + * The Payara Foundation designates this particular file as subject to the "Classpath" + * exception as provided by the Payara Foundation in the GPL Version 2 section of the License + * file that accompanied this code. + * + * Modifications: + * If applicable, add the following below the License Header, with the fields + * enclosed by brackets [] replaced by your own identifying information: + * "Portions Copyright [year] [name of copyright owner]" + * + * Contributor(s): + * If you wish your version of this file to be governed by only the CDDL or + * only the GPL Version 2, indicate your decision by adding "[Contributor] + * elects to include this software in this distribution under the [CDDL or GPL + * Version 2] license." If you don't indicate a single choice of license, a + * recipient has the option to distribute your version of this file under + * either the CDDL, the GPL Version 2 or to extend the choice of license to + * its licensees as provided above. However, if you add GPL Version 2 code + * and therefore, elected the GPL Version 2 license, then the option applies + * only if the new code is made subject to such option by the copyright + * holder. + */ +package fish.payara.nucleus.microprofile.config.source; + +import org.junit.*; + +import java.io.File; +import java.io.IOException; +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.Paths; +import java.nio.file.attribute.FileTime; +import java.util.Comparator; +import java.util.HashMap; +import java.util.HashSet; +import java.util.Map; +import java.util.concurrent.TimeUnit; +import java.util.stream.Collectors; + +import static java.util.Arrays.asList; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNull; + +public class DirConfigSourceTest { + + private static Path testDirectory; + private static DirConfigSource source; + + @BeforeClass + public static void setUp() throws IOException { + testDirectory = Files.createTempDirectory("microprofile-config-test"); + + // create a couple of test simple files + Path file1 = Paths.get(testDirectory.toString(), "property1"); + Path file2 = Paths.get(testDirectory.toString(), "property2"); + Path fileHidden = Paths.get(testDirectory.toString(), ".hidden-property"); + file1 = Files.createFile(file1); + Files.write(file1, "value1".getBytes()); + file2 = Files.createFile(file2); + Files.write(file2, "value2".getBytes()); + fileHidden = Files.createFile(fileHidden); + + // create a subdirectory structure with test files + Path mounted = Paths.get(testDirectory.toString(), "foo", "bar"); + Files.createDirectories(mounted); + + Path fileMounted = Paths.get(mounted.toString(), "property3"); + fileMounted = Files.createFile(fileMounted); + Files.write(fileMounted, "value3".getBytes()); + + // create "foo/bar/..data/property4" and symlink from "foo/bar/property4" as done on K8s + Path mountedK8s = Paths.get(mounted.toString(), "..data"); + Files.createDirectories(mountedK8s); + Path fileK8sMounted = Paths.get(mountedK8s.toString(), "property4"); + fileK8sMounted = Files.createFile(fileK8sMounted); + Files.write(fileK8sMounted, "value4".getBytes()); + Path fileK8sSymlink = Paths.get(mounted.toString(), "property4"); + fileK8sSymlink = Files.createSymbolicLink(fileK8sSymlink, fileK8sMounted); + + // create & load + source = new DirConfigSource(testDirectory); + } + + @AfterClass + public static void tearDown() throws IOException { + Files.walk(testDirectory) + .sorted(Comparator.reverseOrder()) + .map(Path::toFile) + .forEach(File::delete); + } + + @Test + public void testParsePropertyNameFromPath() { + // given + Map examples = new HashMap<>(); + examples.put(Paths.get(testDirectory.toString(), "/foo/bar/test/ex"), "foo.bar.test.ex"); + examples.put(Paths.get(testDirectory.toString(), "/foo.bar.test/ex"), "foo.bar.test.ex"); + examples.put(Paths.get(testDirectory.toString(), "/foo/bar.test/ex"), "foo.bar.test.ex"); + examples.put(Paths.get(testDirectory.toString(), "/foo.bar/test/ex"), "foo.bar.test.ex"); + examples.put(Paths.get(testDirectory.toString(), "/foo/bar/test/ex.txt"), "foo.bar.test.ex"); + examples.put(Paths.get(testDirectory.toString(), "/foo/bar/test/ex.tar.gz"), "foo.bar.test.ex.tar"); + examples.put(Paths.get(testDirectory.toString(), "/foo.bar/test.ex"), "foo.bar.test"); + examples.put(Paths.get(testDirectory.toString(), "/foo/bar.test.ex"), "foo.bar.test"); + examples.put(Paths.get(testDirectory.toString(), "/foo.bar.test.ex"), "foo.bar.test"); + examples.put(Paths.get(testDirectory.toString(), "/foo/bar/test.ex"), "foo.bar.test"); + + // when & then + for (Map.Entry ex : examples.entrySet()) { + System.out.println(ex.getKey()+" = "+ex.getValue()); + assertEquals(ex.getValue(), source.parsePropertyNameFromPath(ex.getKey())); + } + } +} diff --git a/nucleus/payara-modules/nucleus-microprofile/config-service/src/test/java/fish/payara/nucleus/microprofile/config/source/NewTestSuite.java b/nucleus/payara-modules/nucleus-microprofile/config-service/src/test/java/fish/payara/nucleus/microprofile/config/source/NewTestSuite.java index 82983cc8e55..84af88c0341 100644 --- a/nucleus/payara-modules/nucleus-microprofile/config-service/src/test/java/fish/payara/nucleus/microprofile/config/source/NewTestSuite.java +++ b/nucleus/payara-modules/nucleus-microprofile/config-service/src/test/java/fish/payara/nucleus/microprofile/config/source/NewTestSuite.java @@ -47,7 +47,7 @@ * @author steve */ @RunWith(Suite.class) -@Suite.SuiteClasses({ SecretsDirConfigSourceTest.class }) +@Suite.SuiteClasses({ DirConfigSourceTest.class }) public class NewTestSuite { diff --git a/nucleus/payara-modules/nucleus-microprofile/config-service/src/test/java/fish/payara/nucleus/microprofile/config/source/SecretsDirConfigSourceTest.java b/nucleus/payara-modules/nucleus-microprofile/config-service/src/test/java/fish/payara/nucleus/microprofile/config/source/SecretsDirConfigSourceTest.java deleted file mode 100644 index af07a2c145a..00000000000 --- a/nucleus/payara-modules/nucleus-microprofile/config-service/src/test/java/fish/payara/nucleus/microprofile/config/source/SecretsDirConfigSourceTest.java +++ /dev/null @@ -1,234 +0,0 @@ -/* - * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS HEADER. - * - * Copyright (c) 2017-2020 Payara Foundation and/or its affiliates. All rights reserved. - * - * The contents of this file are subject to the terms of either the GNU - * General Public License Version 2 only ("GPL") or the Common Development - * and Distribution License("CDDL") (collectively, the "License"). You - * may not use this file except in compliance with the License. You can - * obtain a copy of the License at - * https://github.com/payara/Payara/blob/master/LICENSE.txt - * See the License for the specific - * language governing permissions and limitations under the License. - * - * When distributing the software, include this License Header Notice in each - * file and include the License file at glassfish/legal/LICENSE.txt. - * - * GPL Classpath Exception: - * The Payara Foundation designates this particular file as subject to the "Classpath" - * exception as provided by the Payara Foundation in the GPL Version 2 section of the License - * file that accompanied this code. - * - * Modifications: - * If applicable, add the following below the License Header, with the fields - * enclosed by brackets [] replaced by your own identifying information: - * "Portions Copyright [year] [name of copyright owner]" - * - * Contributor(s): - * If you wish your version of this file to be governed by only the CDDL or - * only the GPL Version 2, indicate your decision by adding "[Contributor] - * elects to include this software in this distribution under the [CDDL or GPL - * Version 2] license." If you don't indicate a single choice of license, a - * recipient has the option to distribute your version of this file under - * either the CDDL, the GPL Version 2 or to extend the choice of license to - * its licensees as provided above. However, if you add GPL Version 2 code - * and therefore, elected the GPL Version 2 license, then the option applies - * only if the new code is made subject to such option by the copyright - * holder. - */ -package fish.payara.nucleus.microprofile.config.source; - -import java.io.File; -import java.io.IOException; -import java.nio.file.Files; -import java.nio.file.Path; -import java.nio.file.Paths; -import java.nio.file.attribute.FileTime; -import java.util.Comparator; -import java.util.HashMap; -import java.util.HashSet; -import java.util.Map; -import java.util.concurrent.TimeUnit; - -import org.junit.After; -import org.junit.Before; -import org.junit.Test; - -import static java.util.Arrays.asList; -import static org.junit.Assert.*; - -/** - * - * @author steve - */ -public class SecretsDirConfigSourceTest { - - private Path testDirectory; - private SecretsDirConfigSource source; - - @Before - public void setUp() throws IOException { - testDirectory = Files.createTempDirectory("microprofile-config-test"); - - // create a couple of test simple files - Path file1 = Paths.get(testDirectory.toString(), "property1"); - Path file2 = Paths.get(testDirectory.toString(), "property2"); - Path fileHidden = Paths.get(testDirectory.toString(), ".hidden-property"); - file1 = Files.createFile(file1); - Files.write(file1, "value1".getBytes()); - file2 = Files.createFile(file2); - Files.write(file2, "value2".getBytes()); - fileHidden = Files.createFile(fileHidden); - - // create a subdirectory structure with test files - Path mounted = Paths.get(testDirectory.toString(), "foo", "bar"); - Files.createDirectories(mounted); - - Path fileMounted = Paths.get(mounted.toString(), "property3"); - fileMounted = Files.createFile(fileMounted); - Files.write(fileMounted, "value3".getBytes()); - - // create "foo/bar/..data/property4" and symlink from "foo/bar/property4" as done on K8s - Path mountedK8s = Paths.get(mounted.toString(), "..data"); - Files.createDirectories(mountedK8s); - Path fileK8sMounted = Paths.get(mountedK8s.toString(), "property4"); - fileK8sMounted = Files.createFile(fileK8sMounted); - Files.write(fileK8sMounted, "value4".getBytes()); - Path fileK8sSymlink = Paths.get(mounted.toString(), "property4"); - fileK8sSymlink = Files.createSymbolicLink(fileK8sSymlink, fileK8sMounted); - - // create & load - source = new SecretsDirConfigSource(testDirectory); - } - - @After - public void tearDown() throws IOException { - Files.walk(testDirectory) - .sorted(Comparator.reverseOrder()) - .map(Path::toFile) - .forEach(File::delete); - } - - /** - * Test of getProperties method, of class SecretsDirConfigSource. - */ - @Test - public void testGetProperties() { - Map expected = new HashMap<>(); - expected.put("property1", "value1"); - expected.put("property2", "value2"); - expected.put("foo.bar.property3", "value3"); - expected.put("foo.bar.property4", "value4"); - assertEquals(expected, source.getProperties()); - } - - /** - * Test of getPropertyNames method, of class SecretsDirConfigSource. - */ - @Test - public void testGetPropertyNames() { - assertEquals(new HashSet<>(asList("property1", "property2", "foo.bar.property3", "foo.bar.property4")), source.getPropertyNames()); - } - - /** - * Test of getValue method, of class SecretsDirConfigSource. - */ - @Test - public void testGetValue() { - assertEquals("value1", source.getValue("property1")); - assertEquals("value2", source.getValue("property2")); - assertEquals("value3", source.getValue("foo.bar.property3")); - assertEquals("value4", source.getValue("foo.bar.property4")); - } - - /** - * Test of getName method, of class SecretsDirConfigSource. - */ - @Test - public void testGetName() { - assertEquals("Secrets Directory", source.getName()); - } - - /** - * Test the changed Property - * @throws java.io.IOException - */ - @Test - public void testChangeProperty() throws IOException { - assertEquals("value1", source.getValue("property1")); - // change the file - Path file1 = Paths.get(testDirectory.toString(), "property1"); - Files.write(file1, "value-changed".getBytes()); - try { - FileTime nowplus1sec = FileTime.fromMillis(System.currentTimeMillis() + TimeUnit.SECONDS.toMillis(1)); - Files.setLastModifiedTime(file1, nowplus1sec); - assertEquals("value-changed", source.getValue("property1")); - } finally { - // clean up - Files.write(file1, "value1".getBytes()); - } - } - - /** - * Test the changed Property in subdirectories - * @throws java.io.IOException - */ - @Test - public void testChangePropertyInSubdir() throws IOException { - assertEquals("value4", source.getValue("foo.bar.property4")); - // change the file - Path file = Paths.get(testDirectory.toString(), "foo", "bar", "..data", "property4"); - Files.write(file, "value-changed".getBytes()); - try { - FileTime nowplus1sec = FileTime.fromMillis(System.currentTimeMillis() + TimeUnit.SECONDS.toMillis(1)); - Files.setLastModifiedTime(file, nowplus1sec); - assertEquals("value-changed", source.getValue("foo.bar.property4")); - } finally { - // clean up - Files.write(file, "value4".getBytes()); - } - } - - /** - * Tests getting a new property as the file has now appeared - */ - @Test - public void testNewFile() throws IOException { - assertNull(source.getValue("property-new")); - // change the file - Path file1 = Paths.get(testDirectory.toString(), "property-new"); - Files.write(file1, "newValue".getBytes()); - try { - assertEquals("newValue", source.getValue("property-new")); - } finally { - // clean up - Files.delete(file1); - } - } - - /** - * Tests getting a new property as the file has now appeared in a subdirectory - */ - @Test - public void testNewFileInSubdir() throws IOException { - assertNull(source.getValue("foo.bar.property-new")); - // change the file - Path file = Paths.get(testDirectory.toString(), "foo", "bar", "..data", "property-new"); - Files.write(file, "newValue".getBytes()); - Path fileSymlink = Paths.get(testDirectory.toString(), "foo", "bar", "property-new"); - Files.createSymbolicLink(fileSymlink, file); - try { - assertEquals("newValue", source.getValue("foo.bar.property-new")); - } finally { - // clean up - Files.delete(file); - Files.delete(fileSymlink); - } - } - - @Test - public void testBadDirectoryNoBlowUp() { - assertNull(new SecretsDirConfigSource(Paths.get(testDirectory.toString(), "FOOBLE")).getValue("BILLY")); - } -} From 4234a68b0294fb300026bc330fdb84baceea5520 Mon Sep 17 00:00:00 2001 From: Oliver Bertuch Date: Mon, 7 Dec 2020 15:39:03 +0100 Subject: [PATCH 10/23] Fix DirConfigSource.parsePropertyNameFromPath() to fix failing tests. --- .../config/source/DirConfigSource.java | 21 ++++++++++++------- .../config/source/DirConfigSourceTest.java | 3 +++ 2 files changed, 17 insertions(+), 7 deletions(-) diff --git a/nucleus/payara-modules/nucleus-microprofile/config-service/src/main/java/fish/payara/nucleus/microprofile/config/source/DirConfigSource.java b/nucleus/payara-modules/nucleus-microprofile/config-service/src/main/java/fish/payara/nucleus/microprofile/config/source/DirConfigSource.java index 8226a6933b6..f6d283fbedd 100644 --- a/nucleus/payara-modules/nucleus-microprofile/config-service/src/main/java/fish/payara/nucleus/microprofile/config/source/DirConfigSource.java +++ b/nucleus/payara-modules/nucleus-microprofile/config-service/src/main/java/fish/payara/nucleus/microprofile/config/source/DirConfigSource.java @@ -210,11 +210,6 @@ public DirConfigSource() { DirConfigSource(Path directory) { super(true); this.directory = directory; - try { - initializePropertiesFromPath(this.directory); - } catch (IOException e) { - logger.log(Level.SEVERE, "Error during setup of MicroProfile Config Directory Source", e); - } } @Override @@ -316,15 +311,27 @@ void removePropertyFromPath(Path path) { String parsePropertyNameFromPath(Path path) { // 1. get relative path based on the config dir ("/config"), - String property = directory.relativize(path.getParent()).toString(); + String property = ""; + if (! path.getParent().equals(directory)) + property += directory.relativize(path.getParent()).toString() + File.separatorChar; // 2. ignore all file suffixes after last dot - property += path.getFileName().toString().substring(0, path.getFileName().toString().lastIndexOf('.')-1); + property += removeFileExtension(path.getFileName().toString()); // 3. replace all path seps with a ".", property = property.replace(File.separatorChar, '.'); // so "/config/foo/bar/test/one.txt" becomes "foo/bar/test/one.txt" becomes "foo.bar.test.one" property name return property; } + String removeFileExtension(String filename) { + if (filename == null || ! filename.contains(".")) + return filename; + int lastIndex = filename.lastIndexOf('.'); + // dot does not belong to file, but parent dir + if (filename.lastIndexOf(File.separatorChar) > lastIndex) + return filename; + return filename.substring(0, lastIndex); + } + /** * Check if the path given is a more specific path to a value for the given property * @param property diff --git a/nucleus/payara-modules/nucleus-microprofile/config-service/src/test/java/fish/payara/nucleus/microprofile/config/source/DirConfigSourceTest.java b/nucleus/payara-modules/nucleus-microprofile/config-service/src/test/java/fish/payara/nucleus/microprofile/config/source/DirConfigSourceTest.java index c9cef9831e8..bfb6366ca92 100644 --- a/nucleus/payara-modules/nucleus-microprofile/config-service/src/test/java/fish/payara/nucleus/microprofile/config/source/DirConfigSourceTest.java +++ b/nucleus/payara-modules/nucleus-microprofile/config-service/src/test/java/fish/payara/nucleus/microprofile/config/source/DirConfigSourceTest.java @@ -114,6 +114,9 @@ public void testParsePropertyNameFromPath() { examples.put(Paths.get(testDirectory.toString(), "/foo.bar.test/ex"), "foo.bar.test.ex"); examples.put(Paths.get(testDirectory.toString(), "/foo/bar.test/ex"), "foo.bar.test.ex"); examples.put(Paths.get(testDirectory.toString(), "/foo.bar/test/ex"), "foo.bar.test.ex"); + + // we ignore the last file extension. always. this might lead to unexpected behaviour for a user. + // best advice: do not use dots in filename, only in directory names. examples.put(Paths.get(testDirectory.toString(), "/foo/bar/test/ex.txt"), "foo.bar.test.ex"); examples.put(Paths.get(testDirectory.toString(), "/foo/bar/test/ex.tar.gz"), "foo.bar.test.ex.tar"); examples.put(Paths.get(testDirectory.toString(), "/foo.bar/test.ex"), "foo.bar.test"); From eb622fca8dd8cb6b5e35fa87862407e470fa3feb Mon Sep 17 00:00:00 2001 From: Oliver Bertuch Date: Mon, 7 Dec 2020 17:30:51 +0100 Subject: [PATCH 11/23] Refactor DirConfigSource.DirProperty and test longest match. #5006 --- .../config/source/DirConfigSource.java | 26 ++++-- .../config/source/DirConfigSourceTest.java | 92 ++++++++++++++++--- 2 files changed, 96 insertions(+), 22 deletions(-) diff --git a/nucleus/payara-modules/nucleus-microprofile/config-service/src/main/java/fish/payara/nucleus/microprofile/config/source/DirConfigSource.java b/nucleus/payara-modules/nucleus-microprofile/config-service/src/main/java/fish/payara/nucleus/microprofile/config/source/DirConfigSource.java index f6d283fbedd..65139e67009 100644 --- a/nucleus/payara-modules/nucleus-microprofile/config-service/src/main/java/fish/payara/nucleus/microprofile/config/source/DirConfigSource.java +++ b/nucleus/payara-modules/nucleus-microprofile/config-service/src/main/java/fish/payara/nucleus/microprofile/config/source/DirConfigSource.java @@ -79,16 +79,16 @@ public class DirConfigSource extends PayaraConfigSource implements ConfigSource { static final class DirProperty { - final String property; + final String propertyValue; final FileTime lastModifiedTime; final Path path; final int pathDepth; - DirProperty(String property, FileTime lastModifiedTime, Path path, int pathDepth) { - this.property = property; + DirProperty(String propertyValue, FileTime lastModifiedTime, Path path, Path rootPath) { + this.propertyValue = propertyValue; this.lastModifiedTime = lastModifiedTime; this.path = path; - this.pathDepth = pathDepth; + this.pathDepth = rootPath.relativize(path).getNameCount(); } @Override @@ -97,14 +97,14 @@ public boolean equals(Object o) { if (o == null || getClass() != o.getClass()) return false; DirProperty that = (DirProperty) o; return pathDepth == that.pathDepth && - property.equals(that.property) && + propertyValue.equals(that.propertyValue) && lastModifiedTime.equals(that.lastModifiedTime) && path.equals(that.path); } @Override public int hashCode() { - return Objects.hash(property, lastModifiedTime, path, pathDepth); + return Objects.hash(propertyValue, lastModifiedTime, path, pathDepth); } } @@ -215,7 +215,13 @@ public DirConfigSource() { @Override public Map getProperties() { return unmodifiableMap(properties.entrySet().stream() - .collect(toMap(Map.Entry::getKey, e -> e.getValue().property))); + .collect(toMap(Map.Entry::getKey, e -> e.getValue().propertyValue))); + } + + // Test use only + void setProperties(Map properties) { + this.properties.clear(); + this.properties.putAll(properties); } @Override @@ -231,7 +237,7 @@ public int getOrdinal() { @Override public String getValue(String property) { DirProperty result = properties.get(property); - return result == null ? null : result.property; + return result == null ? null : result.propertyValue; } @Override @@ -350,7 +356,7 @@ boolean checkLongestMatchForPath(String property, Path path) { // Check if this element has a higher path depth (longest match) // Example: "foo.bar/test/one.txt" (depth 2) wins over "foo.bar.test.one.txt" (depth 0) - boolean depth = old.pathDepth > relativePath.getNameCount(); + boolean depth = old.pathDepth < relativePath.getNameCount(); // In case that both pathes have the same depth, we need to check on the position of dots. // Example: /config/foo.bar/test/one.txt is less specific than /config/foo/bar.test/one.txt @@ -372,7 +378,7 @@ DirProperty readPropertyFromPath(Path path, BasicFileAttributes mainAtts) throws new String(Files.readAllBytes(path), StandardCharsets.UTF_8), mainAtts.lastModifiedTime(), path.toAbsolutePath(), - directory.relativize(path).getNameCount() + this.directory ); } return null; diff --git a/nucleus/payara-modules/nucleus-microprofile/config-service/src/test/java/fish/payara/nucleus/microprofile/config/source/DirConfigSourceTest.java b/nucleus/payara-modules/nucleus-microprofile/config-service/src/test/java/fish/payara/nucleus/microprofile/config/source/DirConfigSourceTest.java index bfb6366ca92..1981b075b15 100644 --- a/nucleus/payara-modules/nucleus-microprofile/config-service/src/test/java/fish/payara/nucleus/microprofile/config/source/DirConfigSourceTest.java +++ b/nucleus/payara-modules/nucleus-microprofile/config-service/src/test/java/fish/payara/nucleus/microprofile/config/source/DirConfigSourceTest.java @@ -47,6 +47,7 @@ import java.nio.file.Path; import java.nio.file.Paths; import java.nio.file.attribute.FileTime; +import java.time.Instant; import java.util.Comparator; import java.util.HashMap; import java.util.HashSet; @@ -55,8 +56,7 @@ import java.util.stream.Collectors; import static java.util.Arrays.asList; -import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertNull; +import static org.junit.Assert.*; public class DirConfigSourceTest { @@ -110,19 +110,19 @@ public static void tearDown() throws IOException { public void testParsePropertyNameFromPath() { // given Map examples = new HashMap<>(); - examples.put(Paths.get(testDirectory.toString(), "/foo/bar/test/ex"), "foo.bar.test.ex"); - examples.put(Paths.get(testDirectory.toString(), "/foo.bar.test/ex"), "foo.bar.test.ex"); - examples.put(Paths.get(testDirectory.toString(), "/foo/bar.test/ex"), "foo.bar.test.ex"); - examples.put(Paths.get(testDirectory.toString(), "/foo.bar/test/ex"), "foo.bar.test.ex"); + examples.put(Paths.get(testDirectory.toString(), "foo/bar/test/ex"), "foo.bar.test.ex"); + examples.put(Paths.get(testDirectory.toString(), "foo.bar.test/ex"), "foo.bar.test.ex"); + examples.put(Paths.get(testDirectory.toString(), "foo/bar.test/ex"), "foo.bar.test.ex"); + examples.put(Paths.get(testDirectory.toString(), "foo.bar/test/ex"), "foo.bar.test.ex"); // we ignore the last file extension. always. this might lead to unexpected behaviour for a user. // best advice: do not use dots in filename, only in directory names. - examples.put(Paths.get(testDirectory.toString(), "/foo/bar/test/ex.txt"), "foo.bar.test.ex"); - examples.put(Paths.get(testDirectory.toString(), "/foo/bar/test/ex.tar.gz"), "foo.bar.test.ex.tar"); - examples.put(Paths.get(testDirectory.toString(), "/foo.bar/test.ex"), "foo.bar.test"); - examples.put(Paths.get(testDirectory.toString(), "/foo/bar.test.ex"), "foo.bar.test"); - examples.put(Paths.get(testDirectory.toString(), "/foo.bar.test.ex"), "foo.bar.test"); - examples.put(Paths.get(testDirectory.toString(), "/foo/bar/test.ex"), "foo.bar.test"); + examples.put(Paths.get(testDirectory.toString(), "foo/bar/test/ex.txt"), "foo.bar.test.ex"); + examples.put(Paths.get(testDirectory.toString(), "foo/bar/test/ex.tar.gz"), "foo.bar.test.ex.tar"); + examples.put(Paths.get(testDirectory.toString(), "foo.bar/test.ex"), "foo.bar.test"); + examples.put(Paths.get(testDirectory.toString(), "foo/bar.test.ex"), "foo.bar.test"); + examples.put(Paths.get(testDirectory.toString(), "foo.bar.test.ex"), "foo.bar.test"); + examples.put(Paths.get(testDirectory.toString(), "foo/bar/test.ex"), "foo.bar.test"); // when & then for (Map.Entry ex : examples.entrySet()) { @@ -130,4 +130,72 @@ public void testParsePropertyNameFromPath() { assertEquals(ex.getValue(), source.parsePropertyNameFromPath(ex.getKey())); } } + + @Test + public void testCheckLongestMatchForPath_PathDepthLessSpecific() { + // given + Map props = new HashMap<>(); + // a property with a most specific path + String property = "foo.bar.test.ex"; + props.put(property, + new DirConfigSource.DirProperty( + "test", FileTime.from(Instant.now()), + Paths.get(testDirectory.toString(), "foo/bar/test/ex"), + testDirectory)); + source.setProperties(props); + + // when & then + assertFalse(source.checkLongestMatchForPath(property, Paths.get(testDirectory.toString(), "foo/bar.test/ex"))); + } + + @Test + public void testCheckLongestMatchForPath_PathDepthMoreSpecific() { + // given + Map props = new HashMap<>(); + // a property with a most specific path + String property = "foo.bar.test.ex"; + props.put(property, + new DirConfigSource.DirProperty( + "test", FileTime.from(Instant.now()), + Paths.get(testDirectory.toString(), "foo.bar/test/ex"), + testDirectory)); + source.setProperties(props); + + // when & then + assertTrue(source.checkLongestMatchForPath(property, Paths.get(testDirectory.toString(), "foo/bar/test/ex"))); + } + + @Test + public void testCheckLongestMatchForPath_PathDepthEqualMoreSpecific() { + // given + Map props = new HashMap<>(); + // a property with a most specific path + String property = "foo.bar.test.ex.one"; + props.put(property, + new DirConfigSource.DirProperty( + "test", FileTime.from(Instant.now()), + Paths.get(testDirectory.toString(), "foo.bar/test.ex/one"), + testDirectory)); + source.setProperties(props); + + // when & then + assertTrue(source.checkLongestMatchForPath(property, Paths.get(testDirectory.toString(), "foo.bar/test/ex.one.txt"))); + } + + @Test + public void testCheckLongestMatchForPath_PropNotPresent() { + // given + Map props = new HashMap<>(); + // a property with a most specific path + String property = "foo.bar.test.ex.one"; + props.put(property, + new DirConfigSource.DirProperty( + "test", FileTime.from(Instant.now()), + Paths.get(testDirectory.toString(), "foo.bar/test.ex/one"), + testDirectory)); + source.setProperties(props); + + // when & then + assertTrue(source.checkLongestMatchForPath("foo.bar.test.ex.two", Paths.get(testDirectory.toString(), "foo.bar/test/ex.two.txt"))); + } } From 8e86e89a8aa4a06018bc247d3824b7bb0eb21eb2 Mon Sep 17 00:00:00 2001 From: Oliver Bertuch Date: Mon, 7 Dec 2020 18:14:41 +0100 Subject: [PATCH 12/23] Rename DirConfigSource.checkLongestMatchForPath() to isLongerMatchForPath() to be more readable and fix check if property needs update. Adding more tests. --- .../config/source/DirConfigSource.java | 6 +- .../config/source/DirConfigSourceTest.java | 55 +++++++++++++++++-- 2 files changed, 54 insertions(+), 7 deletions(-) diff --git a/nucleus/payara-modules/nucleus-microprofile/config-service/src/main/java/fish/payara/nucleus/microprofile/config/source/DirConfigSource.java b/nucleus/payara-modules/nucleus-microprofile/config-service/src/main/java/fish/payara/nucleus/microprofile/config/source/DirConfigSource.java index 65139e67009..b84bef70bf8 100644 --- a/nucleus/payara-modules/nucleus-microprofile/config-service/src/main/java/fish/payara/nucleus/microprofile/config/source/DirConfigSource.java +++ b/nucleus/payara-modules/nucleus-microprofile/config-service/src/main/java/fish/payara/nucleus/microprofile/config/source/DirConfigSource.java @@ -292,7 +292,7 @@ void updatePropertyFromPath(Path path, BasicFileAttributes mainAtts) throws IOEx // Conflict handling: // When this property is already present, check how to solve the conflict. // This property file will be skipped if the file we already have is deeper in the file tree... - if (checkLongestMatchForPath(property, path)) { + if (! isLongerMatchForPath(property, path)) { return; } @@ -344,7 +344,7 @@ String removeFileExtension(String filename) { * @param path * @return true if more specific, false if not */ - boolean checkLongestMatchForPath(String property, Path path) { + boolean isLongerMatchForPath(String property, Path path) { // Make path relative to config directory // NOTE: we will never have a path containing "..", as our tree walkers are always inside this "root". Path relativePath = directory.relativize(path); @@ -381,7 +381,7 @@ DirProperty readPropertyFromPath(Path path, BasicFileAttributes mainAtts) throws this.directory ); } - return null; + throw new IOException("Cannot read property from '"+path.toString()+"'."); } } diff --git a/nucleus/payara-modules/nucleus-microprofile/config-service/src/test/java/fish/payara/nucleus/microprofile/config/source/DirConfigSourceTest.java b/nucleus/payara-modules/nucleus-microprofile/config-service/src/test/java/fish/payara/nucleus/microprofile/config/source/DirConfigSourceTest.java index 1981b075b15..605f77c214f 100644 --- a/nucleus/payara-modules/nucleus-microprofile/config-service/src/test/java/fish/payara/nucleus/microprofile/config/source/DirConfigSourceTest.java +++ b/nucleus/payara-modules/nucleus-microprofile/config-service/src/test/java/fish/payara/nucleus/microprofile/config/source/DirConfigSourceTest.java @@ -43,6 +43,7 @@ import java.io.File; import java.io.IOException; +import java.nio.charset.StandardCharsets; import java.nio.file.Files; import java.nio.file.Path; import java.nio.file.Paths; @@ -67,6 +68,7 @@ public class DirConfigSourceTest { public static void setUp() throws IOException { testDirectory = Files.createTempDirectory("microprofile-config-test"); + /* // create a couple of test simple files Path file1 = Paths.get(testDirectory.toString(), "property1"); Path file2 = Paths.get(testDirectory.toString(), "property2"); @@ -93,6 +95,7 @@ public static void setUp() throws IOException { Files.write(fileK8sMounted, "value4".getBytes()); Path fileK8sSymlink = Paths.get(mounted.toString(), "property4"); fileK8sSymlink = Files.createSymbolicLink(fileK8sSymlink, fileK8sMounted); + */ // create & load source = new DirConfigSource(testDirectory); @@ -145,7 +148,7 @@ public void testCheckLongestMatchForPath_PathDepthLessSpecific() { source.setProperties(props); // when & then - assertFalse(source.checkLongestMatchForPath(property, Paths.get(testDirectory.toString(), "foo/bar.test/ex"))); + assertFalse(source.isLongerMatchForPath(property, Paths.get(testDirectory.toString(), "foo/bar.test/ex"))); } @Test @@ -162,7 +165,7 @@ public void testCheckLongestMatchForPath_PathDepthMoreSpecific() { source.setProperties(props); // when & then - assertTrue(source.checkLongestMatchForPath(property, Paths.get(testDirectory.toString(), "foo/bar/test/ex"))); + assertTrue(source.isLongerMatchForPath(property, Paths.get(testDirectory.toString(), "foo/bar/test/ex"))); } @Test @@ -179,7 +182,7 @@ public void testCheckLongestMatchForPath_PathDepthEqualMoreSpecific() { source.setProperties(props); // when & then - assertTrue(source.checkLongestMatchForPath(property, Paths.get(testDirectory.toString(), "foo.bar/test/ex.one.txt"))); + assertTrue(source.isLongerMatchForPath(property, Paths.get(testDirectory.toString(), "foo.bar/test/ex.one.txt"))); } @Test @@ -196,6 +199,50 @@ public void testCheckLongestMatchForPath_PropNotPresent() { source.setProperties(props); // when & then - assertTrue(source.checkLongestMatchForPath("foo.bar.test.ex.two", Paths.get(testDirectory.toString(), "foo.bar/test/ex.two.txt"))); + assertTrue(source.isLongerMatchForPath("foo.bar.test.ex.two", Paths.get(testDirectory.toString(), "foo.bar/test/ex.two.txt"))); } + + @Test + public void testRemovePropertyFromPath() { + // given + Map props = new HashMap<>(); + // a property with a most specific path + String property = "foo.bar.test"; + props.put(property, + new DirConfigSource.DirProperty( + "test", FileTime.from(Instant.now()), + Paths.get(testDirectory.toString(), "foo/bar/test"), + testDirectory)); + source.setProperties(props); + assertEquals("test", source.getValue(property)); + + // when + source.removePropertyFromPath(Paths.get(testDirectory.toString(), "foo/bar/test")); + // then + assertTrue(source.getValue(property) == null); + + } + + @Test + public void testInitializeProperties() throws IOException { + // given + // only the most specific should be picked up (=test3) + writeFile(testDirectory, "foo.bar.test", "test"); + writeFile(testDirectory, "foo.bar/test", "test2"); + writeFile(testDirectory, "foo/bar/test", "test3"); + + //when + source.initializePropertiesFromPath(testDirectory); + + //then + assertEquals("test3", source.getValue("foo.bar.test")); + } + + public static Path writeFile(Path parentDir, String filename, String content) throws IOException { + Path file = Paths.get(parentDir.toString(), filename); + Files.createDirectories(file.getParent()); + Files.write(file, content.getBytes(StandardCharsets.UTF_8)); + return file; + } + } From a277be5c6ffa8de5cb695dfbf981c2bcb8770a6d Mon Sep 17 00:00:00 2001 From: Oliver Bertuch Date: Mon, 7 Dec 2020 19:33:27 +0100 Subject: [PATCH 13/23] Add more test for initial read of property files in DirConfigSource --- .../config/source/DirConfigSourceTest.java | 55 ++++++++----------- 1 file changed, 23 insertions(+), 32 deletions(-) diff --git a/nucleus/payara-modules/nucleus-microprofile/config-service/src/test/java/fish/payara/nucleus/microprofile/config/source/DirConfigSourceTest.java b/nucleus/payara-modules/nucleus-microprofile/config-service/src/test/java/fish/payara/nucleus/microprofile/config/source/DirConfigSourceTest.java index 605f77c214f..11a7df41037 100644 --- a/nucleus/payara-modules/nucleus-microprofile/config-service/src/test/java/fish/payara/nucleus/microprofile/config/source/DirConfigSourceTest.java +++ b/nucleus/payara-modules/nucleus-microprofile/config-service/src/test/java/fish/payara/nucleus/microprofile/config/source/DirConfigSourceTest.java @@ -47,6 +47,7 @@ import java.nio.file.Files; import java.nio.file.Path; import java.nio.file.Paths; +import java.nio.file.attribute.FileAttribute; import java.nio.file.attribute.FileTime; import java.time.Instant; import java.util.Comparator; @@ -66,37 +67,7 @@ public class DirConfigSourceTest { @BeforeClass public static void setUp() throws IOException { - testDirectory = Files.createTempDirectory("microprofile-config-test"); - - /* - // create a couple of test simple files - Path file1 = Paths.get(testDirectory.toString(), "property1"); - Path file2 = Paths.get(testDirectory.toString(), "property2"); - Path fileHidden = Paths.get(testDirectory.toString(), ".hidden-property"); - file1 = Files.createFile(file1); - Files.write(file1, "value1".getBytes()); - file2 = Files.createFile(file2); - Files.write(file2, "value2".getBytes()); - fileHidden = Files.createFile(fileHidden); - - // create a subdirectory structure with test files - Path mounted = Paths.get(testDirectory.toString(), "foo", "bar"); - Files.createDirectories(mounted); - - Path fileMounted = Paths.get(mounted.toString(), "property3"); - fileMounted = Files.createFile(fileMounted); - Files.write(fileMounted, "value3".getBytes()); - - // create "foo/bar/..data/property4" and symlink from "foo/bar/property4" as done on K8s - Path mountedK8s = Paths.get(mounted.toString(), "..data"); - Files.createDirectories(mountedK8s); - Path fileK8sMounted = Paths.get(mountedK8s.toString(), "property4"); - fileK8sMounted = Files.createFile(fileK8sMounted); - Files.write(fileK8sMounted, "value4".getBytes()); - Path fileK8sSymlink = Paths.get(mounted.toString(), "property4"); - fileK8sSymlink = Files.createSymbolicLink(fileK8sSymlink, fileK8sMounted); - */ - + testDirectory = Files.createTempDirectory("microprofile-config-test-"); // create & load source = new DirConfigSource(testDirectory); } @@ -224,7 +195,7 @@ public void testRemovePropertyFromPath() { } @Test - public void testInitializeProperties() throws IOException { + public void testInitializeProperties_SimpleFiles() throws IOException { // given // only the most specific should be picked up (=test3) writeFile(testDirectory, "foo.bar.test", "test"); @@ -238,6 +209,26 @@ public void testInitializeProperties() throws IOException { assertEquals("test3", source.getValue("foo.bar.test")); } + @Test + public void testInitializeProperties_IgnoreHidden() throws IOException { + // given + // none of these should be picked up (hidden file or dir) + writeFile(testDirectory, ".hidden.bar.test", "test"); + writeFile(testDirectory, ".hidden/bar.test", "test"); + //when + source.initializePropertiesFromPath(testDirectory); + //then + assertEquals(null, source.getValue("hidden.bar.test")); + } + + @Test(expected = IOException.class) + public void testInitializeProperties_FailDirectory() throws IOException { + // given + Path failDir = Paths.get("/tmp/fail-112312"); + //when & then + source.initializePropertiesFromPath(failDir); + } + public static Path writeFile(Path parentDir, String filename, String content) throws IOException { Path file = Paths.get(parentDir.toString(), filename); Files.createDirectories(file.getParent()); From 9bda0ae1dcbf6f0a653f7d453383885da631bf57 Mon Sep 17 00:00:00 2001 From: Oliver Bertuch Date: Mon, 25 Jan 2021 19:16:05 +0100 Subject: [PATCH 14/23] refactor(mpconfig): incorporate review comments by @jbee and @pdudits #5006 This is kinda major refactoring, including code tested in sample project to ensure this runs on K8s. It's still lacking tests! This is a commit to save the status. --- .../config/source/DirConfigSource.java | 298 +++++++++++------- 1 file changed, 178 insertions(+), 120 deletions(-) diff --git a/nucleus/payara-modules/nucleus-microprofile/config-service/src/main/java/fish/payara/nucleus/microprofile/config/source/DirConfigSource.java b/nucleus/payara-modules/nucleus-microprofile/config-service/src/main/java/fish/payara/nucleus/microprofile/config/source/DirConfigSource.java index b84bef70bf8..016a40291a0 100644 --- a/nucleus/payara-modules/nucleus-microprofile/config-service/src/main/java/fish/payara/nucleus/microprofile/config/source/DirConfigSource.java +++ b/nucleus/payara-modules/nucleus-microprofile/config-service/src/main/java/fish/payara/nucleus/microprofile/config/source/DirConfigSource.java @@ -39,7 +39,6 @@ */ package fish.payara.nucleus.microprofile.config.source; -import fish.payara.nucleus.executorservice.PayaraExecutorService; import org.eclipse.microprofile.config.spi.ConfigSource; import java.io.File; @@ -62,10 +61,13 @@ import java.util.Objects; import java.util.Set; import java.util.concurrent.ConcurrentHashMap; -import java.util.logging.Level; +import java.util.concurrent.TimeUnit; import java.util.logging.Logger; import static java.util.Collections.unmodifiableMap; +import static java.util.concurrent.TimeUnit.SECONDS; +import static java.util.logging.Level.SEVERE; +import static java.util.logging.Level.WARNING; import static java.util.stream.Collectors.toMap; import static java.nio.file.StandardWatchEventKinds.ENTRY_CREATE; import static java.nio.file.StandardWatchEventKinds.ENTRY_DELETE; @@ -84,11 +86,15 @@ static final class DirProperty { final Path path; final int pathDepth; - DirProperty(String propertyValue, FileTime lastModifiedTime, Path path, Path rootPath) { + DirProperty(String propertyValue, FileTime lastModifiedTime, Path path, int pathDepth) { this.propertyValue = propertyValue; this.lastModifiedTime = lastModifiedTime; this.path = path; - this.pathDepth = rootPath.relativize(path).getNameCount(); + this.pathDepth = pathDepth; + } + + DirProperty(String propertyValue, FileTime lastModifiedTime, Path path, Path rootPath) { + this(propertyValue, lastModifiedTime, path, rootPath.relativize(path).getNameCount()); } @Override @@ -121,71 +127,93 @@ final class DirPropertyWatcher implements Runnable { throw new IOException("Given directory '"+topmostDir+"' is no directory or cannot be read."); } } - - void registerAll(Path dir) throws IOException { - // register file watchers recursively (they don't attach to subdirs...) + + /** + * Register file watchers recursively (as they don't attach themselfs to sub directories...) + * and initialize values from files present and suitable. + * @param dir Topmost directory to start recursive traversal from + * @throws IOException + */ + final void registerAll(Path dir) throws IOException { Files.walkFileTree(dir, new SimpleFileVisitor() { @Override public FileVisitResult preVisitDirectory(Path dir, BasicFileAttributes attrs) throws IOException { - register(dir); + // only register subdirectories if the directory itself is suitable. + if ( isAptDir(dir) ) { + register(dir); + return FileVisitResult.CONTINUE; + } + return FileVisitResult.SKIP_SUBTREE; + } + + // Read and ingest all files and dirs present + @Override + public FileVisitResult visitFile(Path path, BasicFileAttributes mainAtts) throws IOException { + // file will be checked before upserting. + upsertPropertyFromPath(path, mainAtts); return FileVisitResult.CONTINUE; } }); } - void register(Path dir) throws IOException { + final void register(Path dir) throws IOException { WatchKey key = dir.register(watcher, ENTRY_CREATE, ENTRY_DELETE, ENTRY_MODIFY); watchedFileKeys.putIfAbsent(key, dir); + logger.finer("MPCONFIG DirConfigSource: registered \""+dir+"\" as key \""+key+"\"."); } @Override - public void run() { - while (true) { - // wait infinitely until we receive an event (or the executor is shutting down) - WatchKey key; - try { - key = watcher.take(); - } catch (InterruptedException ex) { - return; - } - - Path workDir = watchedFileKeys.get(key); - for (WatchEvent event : key.pollEvents()) { - WatchEvent.Kind kind = event.kind(); - - @SuppressWarnings("unchecked") - WatchEvent ev = (WatchEvent) event; - Path fileName = ev.context(); - Path path = workDir.resolve(fileName); + public final void run() { + // wait infinitely until we receive an event (or the executor is shutting down) + WatchKey key; + try { + key = watcher.take(); + } catch (InterruptedException ex) { + logger.info("MPCONFIG DirConfigSource: shutting down watcher thread."); + return; + } - try { - // new directory to be watched and traversed - if (kind == ENTRY_CREATE && Files.isDirectory(path) && !Files.isHidden(path) && Files.isReadable(path)) { - registerAll(path); - initializePropertiesFromPath(path); - } - // new or updated file found - if (Files.isRegularFile(path) && (kind == ENTRY_CREATE || kind == ENTRY_MODIFY)) { - updatePropertyFromPath(path, Files.readAttributes(path, BasicFileAttributes.class)); - } - if (Files.isRegularFile(path) && (kind == ENTRY_DELETE)) { - removePropertyFromPath(path); - } - } catch (IOException e) { - logger.log(Level.SEVERE, "Could not process event '"+kind+"' on '"+path+"'", e); - } - } - - // Reset key (obligatory) and remove from set if directory no longer accessible - boolean valid = key.reset(); - if (!valid) { - watchedFileKeys.remove(key); + Path workDir = watchedFileKeys.get(key); + for (WatchEvent event : key.pollEvents()) { + WatchEvent.Kind kind = event.kind(); + + @SuppressWarnings("unchecked") + WatchEvent ev = (WatchEvent) event; + Path fileName = ev.context(); + Path path = workDir.resolve(fileName); + + logger.finer("MPCONFIG DirConfigSource: detected change: "+fileName.toString()+" : "+kind.toString()); + + try { + BasicFileAttributes atts = Files.readAttributes(path, BasicFileAttributes.class); - // all directories became inaccessible, even topmostDir! - if (watchedFileKeys.isEmpty()) break; + // new directory to be watched and traversed + if (kind == ENTRY_CREATE && isAptDir(path)) { + logger.finer("MPCONFIG DirConfigSource: registering new paths."); + registerAll(path); + } + // new or updated file found (new = create + modify on content save) + // or new symlink found (symlinks are create only!) (also, aptness of file is checked inside update routine) + if ( kind == ENTRY_MODIFY || (kind == ENTRY_CREATE && Files.isSymbolicLink(path)) ) { + logger.finer("MPCONFIG DirConfigSource: processing new or updated file \""+path.toString()+"\"."); + upsertPropertyFromPath(path, atts); + } + if (Files.notExists(path) && ! watchedFileKeys.containsValue(path) && kind == ENTRY_DELETE) { + logger.finer("MPCONFIG DirConfigSource: removing deleted file \""+path.toString()+"\"."); + removePropertyFromPath(path); + } + } catch (IOException e) { + logger.log(WARNING, "MPCONFIG DirConfigSource: could not process event '"+kind+"' on '"+path+"'", e); } } + + // Reset key (obligatory) and remove from set if directory no longer accessible + boolean valid = key.reset(); + if (!valid) { + logger.finer("MPCONFIG DirConfigSource: removing watcher for key \""+key+"\"."); + watchedFileKeys.remove(key); + } } } @@ -198,11 +226,9 @@ public DirConfigSource() { // get the directory from the app server config this.directory = findDir(); // create the watcher for the directory - configService.getExecutor().submit(new DirPropertyWatcher(this.directory)); - // initial loading - initializePropertiesFromPath(this.directory); + configService.getExecutor().scheduleWithFixedDelay(createWatcher(this.directory), 0, 1, SECONDS); } catch (IOException e) { - logger.log(Level.SEVERE, "Error during setup of MicroProfile Config Directory Source", e); + logger.log(SEVERE, "MPCONFIG DirConfigSource: error during setup.", e); } } @@ -211,6 +237,11 @@ public DirConfigSource() { super(true); this.directory = directory; } + + // Used for testing only + DirPropertyWatcher createWatcher(Path topmostDirectory) throws IOException { + return new DirPropertyWatcher(topmostDirectory); + } @Override public Map getProperties() { @@ -218,7 +249,7 @@ public Map getProperties() { .collect(toMap(Map.Entry::getKey, e -> e.getValue().propertyValue))); } - // Test use only + // Used for testing only void setProperties(Map properties) { this.properties.clear(); this.properties.putAll(properties); @@ -260,82 +291,41 @@ private Path findDir() throws IOException { throw new IOException("Given MPCONFIG directory '"+path+"' is no directory or cannot be read."); } - void initializePropertiesFromPath(Path topmostDir) throws IOException { - if (Files.exists(topmostDir) && Files.isDirectory(topmostDir) && Files.isReadable(topmostDir)) { - // initialize properties on first run - Files.walkFileTree(topmostDir, new SimpleFileVisitor() { - // Ignore hidden directories - @Override - public FileVisitResult preVisitDirectory(java.nio.file.Path dir, BasicFileAttributes attrs) throws IOException { - return dir.toFile().isHidden() ? FileVisitResult.SKIP_SUBTREE : FileVisitResult.CONTINUE; - } - - // Read and ingest all files and dirs present - @Override - public FileVisitResult visitFile(Path path, BasicFileAttributes mainAtts) throws IOException { - updatePropertyFromPath(path, mainAtts); - return FileVisitResult.CONTINUE; - } - }); - } else { - throw new IOException("Given directory '"+topmostDir+"' is no directory or cannot be read."); - } - } - - void updatePropertyFromPath(Path path, BasicFileAttributes mainAtts) throws IOException { - // do not read hidden files, as K8s Secret filenames are symlinks to hidden files with data. - // also ignore files > 512KB, as they are most likely no text config files... - if (Files.isRegularFile(path) && ! Files.isHidden(path) && Files.isReadable(path) && mainAtts.size() < 512*1024) { + /** + * Upserting a property from a file. Checking for suitability of the file, too. + * @param path Path to the file + * @param mainAtts The attributes of the file (like mod time etc) + * @return true if file was suitable, false if not. + * @throws IOException In case anything goes bonkers. + */ + final boolean upsertPropertyFromPath(Path path, BasicFileAttributes mainAtts) throws IOException { + // check for a suitable file first, return aptness. + if ( isAptFile(path, mainAtts) ) { // retrieve the property name from the file path - String property = parsePropertyNameFromPath(path); + String property = parsePropertyNameFromPath(path, this.directory); // Conflict handling: - // When this property is already present, check how to solve the conflict. + // When this property is not already present, check how to solve the conflict. // This property file will be skipped if the file we already have is deeper in the file tree... - if (! isLongerMatchForPath(property, path)) { - return; + if (isLongerMatchForPath(property, path)) { + properties.put(property, readPropertyFromPath(path, mainAtts, this.directory)); + return true; } - - properties.put(property, readPropertyFromPath(path, mainAtts)); } + return false; } - void removePropertyFromPath(Path path) { - String property = parsePropertyNameFromPath(path); - + final void removePropertyFromPath(Path path) { + String property = parsePropertyNameFromPath(path, this.directory); // not present? go away silently. if (! properties.containsKey(property)) return; - + // only delete from the map if the file that has been deleted is the same as the one stored in the map // -> deleting a file less specific but matching a property should not remove from the map // -> deleting a file more specific than in map shouldn't occur (it had to slip through longest match check then). if (path.equals(properties.get(property).path)) { properties.remove(property); } - - } - - String parsePropertyNameFromPath(Path path) { - // 1. get relative path based on the config dir ("/config"), - String property = ""; - if (! path.getParent().equals(directory)) - property += directory.relativize(path.getParent()).toString() + File.separatorChar; - // 2. ignore all file suffixes after last dot - property += removeFileExtension(path.getFileName().toString()); - // 3. replace all path seps with a ".", - property = property.replace(File.separatorChar, '.'); - // so "/config/foo/bar/test/one.txt" becomes "foo/bar/test/one.txt" becomes "foo.bar.test.one" property name - return property; - } - - String removeFileExtension(String filename) { - if (filename == null || ! filename.contains(".")) - return filename; - int lastIndex = filename.lastIndexOf('.'); - // dot does not belong to file, but parent dir - if (filename.lastIndexOf(File.separatorChar) > lastIndex) - return filename; - return filename.substring(0, lastIndex); } /** @@ -344,7 +334,7 @@ String removeFileExtension(String filename) { * @param path * @return true if more specific, false if not */ - boolean isLongerMatchForPath(String property, Path path) { + final boolean isLongerMatchForPath(String property, Path path) { // Make path relative to config directory // NOTE: we will never have a path containing "..", as our tree walkers are always inside this "root". Path relativePath = directory.relativize(path); @@ -372,16 +362,84 @@ boolean isLongerMatchForPath(String property, Path path) { return depth; } - DirProperty readPropertyFromPath(Path path, BasicFileAttributes mainAtts) throws IOException { + /** + * Check path to be a directory, readable by us, but ignore if starting with a "." + * (as done for K8s secrets mounts). + * @param path + * @return true if suitable, false if not. + * @throws IOException when path cannot be resolved, maybe because of a broken symlink. + */ + public final static boolean isAptDir(Path path) throws IOException { + return path != null && Files.exists(path) && + Files.isDirectory(path) && Files.isReadable(path) && + !path.getFileName().toString().startsWith("."); + } + + /** + * Check if the file exists (follows symlinks), is a regular file (following symlinks), is readable (following symlinks), + * the filename does not start with a "." (not following the symlink!) and the file is less than 512 KB. + * @param path + * @return true if suitable file, false if not. + * @throws IOException when path cannot be accessed or resolved etc. + */ + public final static boolean isAptFile(Path path, BasicFileAttributes atts) throws IOException { + return path != null && Files.exists(path) && + Files.isRegularFile(path) && Files.isReadable(path) && + !path.getFileName().toString().startsWith(".") && + atts.size() < 512*1024; + } + + /** + * Parsing the relative path into a configuration property name. + * Using dots to mark scopes based on directories plus keeping dots used in the files name. + * @param path The path to the property file + * @return The property name + */ + public final static String parsePropertyNameFromPath(Path path, Path rootDir) { + // 1. get relative path based on the config dir ("/config"), + String property = ""; + if (! path.getParent().equals(rootDir)) + property += rootDir.relativize(path.getParent()).toString() + File.separatorChar; + // 2. ignore all file suffixes after last dot + property += removeFileExtension(path.getFileName().toString()); + // 3. replace all path seps with a ".", + property = property.replace(File.separatorChar, '.'); + // so "/config/foo/bar/test/one.txt" becomes "foo/bar/test/one.txt" becomes "foo.bar.test.one" property name + return property; + } + + /** + * Litte helper to remove the file extension (not present in Java std functionality) + * @param filename A filename containing a dot, marking the start of the file extension + * @return Filename without a suffix (if present) + */ + public final static String removeFileExtension(String filename) { + if (filename == null || ! filename.contains(".")) + return filename; + int lastIndex = filename.lastIndexOf('.'); + // dot does not belong to file, but parent dir or + // extension is longer than 3 chars (all clear text formats would have 3 chars max) + if (filename.lastIndexOf(File.separatorChar) > lastIndex || lastIndex < filename.length() - 4) + return filename; + return filename.substring(0, lastIndex); + } + + /** + * Actually read the data from the file, assuming UTF-8 formatted content, creating properties from it. + * @param path The file to read + * @param mainAtts The files basic attributes like mod time, ... + * @return The configuration property this path represents + * @throws IOException When reading fails. + */ + static final DirProperty readPropertyFromPath(Path path, BasicFileAttributes mainAtts, Path rootPath) throws IOException { if (Files.exists(path) && Files.isRegularFile(path) && Files.isReadable(path)) { return new DirProperty( new String(Files.readAllBytes(path), StandardCharsets.UTF_8), mainAtts.lastModifiedTime(), path.toAbsolutePath(), - this.directory + rootPath ); } throw new IOException("Cannot read property from '"+path.toString()+"'."); } - } From a50ad85f504ea43b75174fc4f9e56ba4511ee1ad Mon Sep 17 00:00:00 2001 From: Oliver Bertuch Date: Tue, 26 Jan 2021 00:27:55 +0100 Subject: [PATCH 15/23] fix(mpconfig): make dirconfigsource match same pathes on update and fix IOE when deleting. #5006 --- .../microprofile/config/source/DirConfigSource.java | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/nucleus/payara-modules/nucleus-microprofile/config-service/src/main/java/fish/payara/nucleus/microprofile/config/source/DirConfigSource.java b/nucleus/payara-modules/nucleus-microprofile/config-service/src/main/java/fish/payara/nucleus/microprofile/config/source/DirConfigSource.java index 016a40291a0..520764268e1 100644 --- a/nucleus/payara-modules/nucleus-microprofile/config-service/src/main/java/fish/payara/nucleus/microprofile/config/source/DirConfigSource.java +++ b/nucleus/payara-modules/nucleus-microprofile/config-service/src/main/java/fish/payara/nucleus/microprofile/config/source/DirConfigSource.java @@ -61,7 +61,6 @@ import java.util.Objects; import java.util.Set; import java.util.concurrent.ConcurrentHashMap; -import java.util.concurrent.TimeUnit; import java.util.logging.Logger; import static java.util.Collections.unmodifiableMap; @@ -186,8 +185,6 @@ public final void run() { logger.finer("MPCONFIG DirConfigSource: detected change: "+fileName.toString()+" : "+kind.toString()); try { - BasicFileAttributes atts = Files.readAttributes(path, BasicFileAttributes.class); - // new directory to be watched and traversed if (kind == ENTRY_CREATE && isAptDir(path)) { logger.finer("MPCONFIG DirConfigSource: registering new paths."); @@ -197,6 +194,7 @@ public final void run() { // or new symlink found (symlinks are create only!) (also, aptness of file is checked inside update routine) if ( kind == ENTRY_MODIFY || (kind == ENTRY_CREATE && Files.isSymbolicLink(path)) ) { logger.finer("MPCONFIG DirConfigSource: processing new or updated file \""+path.toString()+"\"."); + BasicFileAttributes atts = Files.readAttributes(path, BasicFileAttributes.class); upsertPropertyFromPath(path, atts); } if (Files.notExists(path) && ! watchedFileKeys.containsValue(path) && kind == ENTRY_DELETE) { @@ -307,7 +305,7 @@ final boolean upsertPropertyFromPath(Path path, BasicFileAttributes mainAtts) th // Conflict handling: // When this property is not already present, check how to solve the conflict. // This property file will be skipped if the file we already have is deeper in the file tree... - if (isLongerMatchForPath(property, path)) { + if (isLongestMatchForPath(property, path)) { properties.put(property, readPropertyFromPath(path, mainAtts, this.directory)); return true; } @@ -330,11 +328,12 @@ final void removePropertyFromPath(Path path) { /** * Check if the path given is a more specific path to a value for the given property + * Same path counts as more specific, too, to allow updates to the same file. * @param property * @param path * @return true if more specific, false if not */ - final boolean isLongerMatchForPath(String property, Path path) { + final boolean isLongestMatchForPath(String property, Path path) { // Make path relative to config directory // NOTE: we will never have a path containing "..", as our tree walkers are always inside this "root". Path relativePath = directory.relativize(path); @@ -344,6 +343,10 @@ final boolean isLongerMatchForPath(String property, Path path) { return true; DirProperty old = properties.get(property); + // Old and new path are the same -> "more" specific (update case) + if ( old.path.equals(path) ) + return true; + // Check if this element has a higher path depth (longest match) // Example: "foo.bar/test/one.txt" (depth 2) wins over "foo.bar.test.one.txt" (depth 0) boolean depth = old.pathDepth < relativePath.getNameCount(); From e8af024ff0e301a1590b160f288aa4ca23167d88 Mon Sep 17 00:00:00 2001 From: Oliver Bertuch Date: Tue, 26 Jan 2021 00:28:26 +0100 Subject: [PATCH 16/23] refactor(mpconfig): Add unit tests for DirConfigSource. #5006 --- .../config/source/DirConfigSourceTest.java | 295 ++++++++++++++---- 1 file changed, 237 insertions(+), 58 deletions(-) diff --git a/nucleus/payara-modules/nucleus-microprofile/config-service/src/test/java/fish/payara/nucleus/microprofile/config/source/DirConfigSourceTest.java b/nucleus/payara-modules/nucleus-microprofile/config-service/src/test/java/fish/payara/nucleus/microprofile/config/source/DirConfigSourceTest.java index 11a7df41037..a88fbffc6d1 100644 --- a/nucleus/payara-modules/nucleus-microprofile/config-service/src/test/java/fish/payara/nucleus/microprofile/config/source/DirConfigSourceTest.java +++ b/nucleus/payara-modules/nucleus-microprofile/config-service/src/test/java/fish/payara/nucleus/microprofile/config/source/DirConfigSourceTest.java @@ -39,7 +39,9 @@ */ package fish.payara.nucleus.microprofile.config.source; -import org.junit.*; +import org.junit.AfterClass; +import org.junit.BeforeClass; +import org.junit.Test; import java.io.File; import java.io.IOException; @@ -47,23 +49,28 @@ import java.nio.file.Files; import java.nio.file.Path; import java.nio.file.Paths; -import java.nio.file.attribute.FileAttribute; +import java.nio.file.attribute.BasicFileAttributes; import java.nio.file.attribute.FileTime; import java.time.Instant; import java.util.Comparator; import java.util.HashMap; -import java.util.HashSet; import java.util.Map; +import java.util.Random; +import java.util.concurrent.Executors; +import java.util.concurrent.ScheduledExecutorService; import java.util.concurrent.TimeUnit; -import java.util.stream.Collectors; -import static java.util.Arrays.asList; -import static org.junit.Assert.*; +import static java.lang.Boolean.FALSE; +import static java.lang.Boolean.TRUE; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; public class DirConfigSourceTest { private static Path testDirectory; private static DirConfigSource source; + private static ScheduledExecutorService exec = Executors.newScheduledThreadPool(3); @BeforeClass public static void setUp() throws IOException { @@ -73,38 +80,110 @@ public static void setUp() throws IOException { } @AfterClass - public static void tearDown() throws IOException { + public static void tearDown() throws IOException, InterruptedException { + exec.shutdown(); + exec.awaitTermination(1, TimeUnit.SECONDS); Files.walk(testDirectory) .sorted(Comparator.reverseOrder()) .map(Path::toFile) .forEach(File::delete); } + @Test + public void testIsAptDir() throws IOException { + // given + Map examples = new HashMap<>(); + examples.put(subpath( "aptdir"), TRUE); + examples.put(subpath( ".unaptdir"), FALSE); + + // when & then + assertEquals(FALSE, DirConfigSource.isAptDir(null)); + assertEquals(FALSE, DirConfigSource.isAptDir(subpath( "aptnotexisting"))); + for (Map.Entry ex : examples.entrySet()) { + Files.createDirectories(ex.getKey()); + assertEquals(ex.getValue(), DirConfigSource.isAptDir(ex.getKey())); + } + } + + @Test + public void testIsAptFile() throws IOException { + + Map examples = new HashMap<>(); + examples.put(subpath( "aptdir/aptfile"), TRUE); + examples.put(subpath( "aptdir/.unaptfile"), FALSE); + + assertEquals(FALSE, DirConfigSource.isAptFile(null, null)); + assertEquals(FALSE, DirConfigSource.isAptFile(subpath( "aptdir/aptnotexisting"), null)); + for (Map.Entry ex : examples.entrySet()) { + BasicFileAttributes atts = writeFile(ex.getKey(), "test"); + assertEquals(ex.getValue(), DirConfigSource.isAptFile(ex.getKey(), atts)); + } + + Path file100k = subpath("aptdir/100k-file"); + BasicFileAttributes atts100k = writeRandFile(file100k, 100); + assertEquals(TRUE, DirConfigSource.isAptFile(file100k, atts100k)); + + Path file600k = subpath("aptdir/600k-file"); + BasicFileAttributes atts600k = writeRandFile(file600k, 600); + assertEquals(TRUE, DirConfigSource.isAptFile(file600k, atts600k)); + } + @Test public void testParsePropertyNameFromPath() { // given Map examples = new HashMap<>(); - examples.put(Paths.get(testDirectory.toString(), "foo/bar/test/ex"), "foo.bar.test.ex"); - examples.put(Paths.get(testDirectory.toString(), "foo.bar.test/ex"), "foo.bar.test.ex"); - examples.put(Paths.get(testDirectory.toString(), "foo/bar.test/ex"), "foo.bar.test.ex"); - examples.put(Paths.get(testDirectory.toString(), "foo.bar/test/ex"), "foo.bar.test.ex"); + examples.put(subpath( "foo/bar/test/ex"), "foo.bar.test.ex"); + examples.put(subpath( "foo.bar.test/ex"), "foo.bar.test.ex"); + examples.put(subpath( "foo/bar.test/ex"), "foo.bar.test.ex"); + examples.put(subpath( "foo.bar/test/ex"), "foo.bar.test.ex"); - // we ignore the last file extension. always. this might lead to unexpected behaviour for a user. + // we ignore the last file extension if not more than 3 chars. + // this might lead to unexpected behaviour for a user. // best advice: do not use dots in filename, only in directory names. - examples.put(Paths.get(testDirectory.toString(), "foo/bar/test/ex.txt"), "foo.bar.test.ex"); - examples.put(Paths.get(testDirectory.toString(), "foo/bar/test/ex.tar.gz"), "foo.bar.test.ex.tar"); - examples.put(Paths.get(testDirectory.toString(), "foo.bar/test.ex"), "foo.bar.test"); - examples.put(Paths.get(testDirectory.toString(), "foo/bar.test.ex"), "foo.bar.test"); - examples.put(Paths.get(testDirectory.toString(), "foo.bar.test.ex"), "foo.bar.test"); - examples.put(Paths.get(testDirectory.toString(), "foo/bar/test.ex"), "foo.bar.test"); + examples.put(subpath( "foo/bar/test/ex.txt"), "foo.bar.test.ex"); + examples.put(subpath( "foo/bar/test/ex.tar.gz"), "foo.bar.test.ex.tar"); + examples.put(subpath( "foo/bar/test/ex.helo"), "foo.bar.test.ex.helo"); + examples.put(subpath( "foo.bar/test.ex"), "foo.bar.test"); + examples.put(subpath( "foo/bar.test.ex"), "foo.bar.test"); + examples.put(subpath( "foo.bar.test.ex"), "foo.bar.test"); + examples.put(subpath( "foo/bar/test.ex"), "foo.bar.test"); // when & then for (Map.Entry ex : examples.entrySet()) { - System.out.println(ex.getKey()+" = "+ex.getValue()); - assertEquals(ex.getValue(), source.parsePropertyNameFromPath(ex.getKey())); + //System.out.println(ex.getKey()+" = "+ex.getValue()); + assertEquals(ex.getValue(), DirConfigSource.parsePropertyNameFromPath(ex.getKey(), testDirectory)); } } + @Test + public void testReadPropertyFromPath() throws IOException { + // given + Path sut = subpath("aptdir/sut-read-property"); + BasicFileAttributes attsSUT = writeFile(sut, "foobar"); + DirConfigSource.DirProperty example = new DirConfigSource.DirProperty( + "foobar", attsSUT.lastModifiedTime(), sut, testDirectory + ); + // when & then + assertEquals(example, DirConfigSource.readPropertyFromPath(sut, attsSUT, testDirectory)); + } + + @Test + public void testCheckLongestMatchForPath_SamePath() { + // given + Map props = new HashMap<>(); + // a property with a most specific path + String property = "foo.bar.test.ex"; + props.put(property, + new DirConfigSource.DirProperty( + "test", FileTime.from(Instant.now()), + subpath( "foo/bar/test/ex"), + testDirectory)); + source.setProperties(props); + + // when & then + assertTrue(source.isLongestMatchForPath(property, subpath("foo/bar/test/ex"))); + } + @Test public void testCheckLongestMatchForPath_PathDepthLessSpecific() { // given @@ -114,12 +193,12 @@ public void testCheckLongestMatchForPath_PathDepthLessSpecific() { props.put(property, new DirConfigSource.DirProperty( "test", FileTime.from(Instant.now()), - Paths.get(testDirectory.toString(), "foo/bar/test/ex"), + subpath( "foo/bar/test/ex"), testDirectory)); source.setProperties(props); // when & then - assertFalse(source.isLongerMatchForPath(property, Paths.get(testDirectory.toString(), "foo/bar.test/ex"))); + assertFalse(source.isLongestMatchForPath(property, subpath( "foo/bar.test/ex"))); } @Test @@ -131,12 +210,12 @@ public void testCheckLongestMatchForPath_PathDepthMoreSpecific() { props.put(property, new DirConfigSource.DirProperty( "test", FileTime.from(Instant.now()), - Paths.get(testDirectory.toString(), "foo.bar/test/ex"), + subpath( "foo.bar/test/ex"), testDirectory)); source.setProperties(props); // when & then - assertTrue(source.isLongerMatchForPath(property, Paths.get(testDirectory.toString(), "foo/bar/test/ex"))); + assertTrue(source.isLongestMatchForPath(property, subpath( "foo/bar/test/ex"))); } @Test @@ -148,12 +227,12 @@ public void testCheckLongestMatchForPath_PathDepthEqualMoreSpecific() { props.put(property, new DirConfigSource.DirProperty( "test", FileTime.from(Instant.now()), - Paths.get(testDirectory.toString(), "foo.bar/test.ex/one"), + subpath( "foo.bar/test.ex/one"), testDirectory)); source.setProperties(props); // when & then - assertTrue(source.isLongerMatchForPath(property, Paths.get(testDirectory.toString(), "foo.bar/test/ex.one.txt"))); + assertTrue(source.isLongestMatchForPath(property, subpath( "foo.bar/test/ex.one.txt"))); } @Test @@ -165,12 +244,12 @@ public void testCheckLongestMatchForPath_PropNotPresent() { props.put(property, new DirConfigSource.DirProperty( "test", FileTime.from(Instant.now()), - Paths.get(testDirectory.toString(), "foo.bar/test.ex/one"), + subpath( "foo.bar/test.ex/one"), testDirectory)); source.setProperties(props); // when & then - assertTrue(source.isLongerMatchForPath("foo.bar.test.ex.two", Paths.get(testDirectory.toString(), "foo.bar/test/ex.two.txt"))); + assertTrue(source.isLongestMatchForPath("foo.bar.test.ex.two", subpath( "foo.bar/test/ex.two.txt"))); } @Test @@ -182,58 +261,158 @@ public void testRemovePropertyFromPath() { props.put(property, new DirConfigSource.DirProperty( "test", FileTime.from(Instant.now()), - Paths.get(testDirectory.toString(), "foo/bar/test"), + subpath( "foo/bar/test"), testDirectory)); source.setProperties(props); assertEquals("test", source.getValue(property)); // when - source.removePropertyFromPath(Paths.get(testDirectory.toString(), "foo/bar/test")); + source.removePropertyFromPath(subpath( "foo/bar/test")); // then assertTrue(source.getValue(property) == null); } - + + @Test + public void testUpsertPropertyFromPath_InsertCase() throws IOException { + // given + Path sut = subpath("aptdir/sut-upsert-property-insert"); + BasicFileAttributes attsSUT = writeFile(sut, "foobar"); + DirConfigSource.DirProperty example = new DirConfigSource.DirProperty( + "foobar", attsSUT.lastModifiedTime(), sut, testDirectory + ); + + // when & then + assertEquals(true, source.upsertPropertyFromPath(sut, attsSUT)); + assertEquals(example.propertyValue, source.getValue("aptdir.sut-upsert-property-insert")); + } + @Test - public void testInitializeProperties_SimpleFiles() throws IOException { + public void testUpsertPropertyFromPath_UpdateCase() throws IOException { // given - // only the most specific should be picked up (=test3) - writeFile(testDirectory, "foo.bar.test", "test"); - writeFile(testDirectory, "foo.bar/test", "test2"); - writeFile(testDirectory, "foo/bar/test", "test3"); + Path sut = subpath("aptdir/sut-upsert-property-update"); + BasicFileAttributes attsSUT = writeFile(sut, "foobar"); + DirConfigSource.DirProperty example = new DirConfigSource.DirProperty( + "foobar", attsSUT.lastModifiedTime(), sut, testDirectory + ); - //when - source.initializePropertiesFromPath(testDirectory); + assertEquals(true, source.upsertPropertyFromPath(sut, attsSUT)); + assertEquals(example.propertyValue, source.getValue("aptdir.sut-upsert-property-update")); - //then - assertEquals("test3", source.getValue("foo.bar.test")); + // when & then + BasicFileAttributes attsUpdate = writeFile(sut, "foobar2"); + assertEquals(true, source.upsertPropertyFromPath(sut, attsUpdate)); + assertEquals("foobar2", source.getValue("aptdir.sut-upsert-property-update")); } @Test - public void testInitializeProperties_IgnoreHidden() throws IOException { + public void testPropertyWatcher_RegisterAndInit() throws Exception { // given - // none of these should be picked up (hidden file or dir) - writeFile(testDirectory, ".hidden.bar.test", "test"); - writeFile(testDirectory, ".hidden/bar.test", "test"); - //when - source.initializePropertiesFromPath(testDirectory); - //then - assertEquals(null, source.getValue("hidden.bar.test")); + Map examples = new HashMap<>(); + examples.put(subpath( "init-watcher/foo/bar/test/ex"), "init-watcher.foo.bar.test.ex"); + examples.put(subpath( "init-watcher/foo.bar.test/hello"), "init-watcher.foo.bar.test.hello"); + examples.put(subpath( "init-watcher/.foo/ex"), "init-watcher.foo.ex"); + for (Map.Entry ex : examples.entrySet()) { + writeFile(ex.getKey(), "foobar"); + } + Files.createSymbolicLink(subpath( "init-watcher/foo.hello"), subpath("init-watcher/.foo/ex")); + + // when + DirConfigSource.DirPropertyWatcher watcher = source.createWatcher(subpath("init-watcher")); + + // then + assertEquals("foobar", source.getValue("init-watcher.foo.bar.test.ex")); + assertEquals("foobar", source.getValue("init-watcher.foo.bar.test.hello")); + assertEquals(null, source.getValue("init-watcher.foo.ex")); + assertEquals("foobar", source.getValue("init-watcher.foo.hello")); } - @Test(expected = IOException.class) - public void testInitializeProperties_FailDirectory() throws IOException { + @Test + public void testPropertyWatcher_RunFilesNewUpdate() throws Exception { // given - Path failDir = Paths.get("/tmp/fail-112312"); - //when & then - source.initializePropertiesFromPath(failDir); + writeFile(subpath("watcher-files/foobar"), "test"); + writeFile(subpath("watcher-files/.hidden/foobar"), "hidden"); + Files.createSymbolicLink(subpath("watcher-files/revealed"), subpath("watcher-files/.hidden/foobar")); + + DirConfigSource.DirPropertyWatcher watcher = source.createWatcher(subpath("watcher-files")); + exec.scheduleWithFixedDelay(watcher, 0, 10, TimeUnit.MILLISECONDS); + + assertEquals("test", source.getValue("watcher-files.foobar")); + assertEquals("hidden", source.getValue("watcher-files.revealed")); + + // when + writeFile(subpath("watcher-files/foobar"), "test2"); + writeFile(subpath("watcher-files/example"), "test2"); + writeFile(subpath("watcher-files/reveal/.hidden"), "test2"); + writeFile(subpath("watcher-files/.hidden/foobar"), "showme"); + Files.delete(subpath("watcher-files/revealed")); + Files.createSymbolicLink(subpath("watcher-files/revealed"), subpath("watcher-files/.hidden/foobar")); + Thread.sleep(100); + + // then + assertEquals("test2", source.getValue("watcher-files.foobar")); + assertEquals("test2", source.getValue("watcher-files.example")); + assertEquals("showme", source.getValue("watcher-files.revealed")); + assertEquals(null, source.getValue("watcher-files.reveal.hidden")); + } + + @Test + public void testPropertyWatcher_RunNewDir() throws Exception { + // given + writeFile(subpath("watcher-newdir/test"), "test"); + + DirConfigSource.DirPropertyWatcher watcher = source.createWatcher(subpath("watcher-newdir")); + exec.scheduleWithFixedDelay(watcher, 0, 10, TimeUnit.MILLISECONDS); + + assertEquals("test", source.getValue("watcher-newdir.test")); + + // when + writeFile(subpath("watcher-newdir/foobar/test"), "test"); + writeFile(subpath("watcher-newdir/.hidden/foobar"), "test"); + Thread.sleep(100); + + // then + assertEquals("test", source.getValue("watcher-newdir.foobar.test")); + assertEquals(null, source.getValue("watcher-newdir.hidden.foobar")); + } + + @Test + public void testPropertyWatcher_RunRemove() throws Exception { + // given + writeFile(subpath("watcher-remove/test"), "test"); + + DirConfigSource.DirPropertyWatcher watcher = source.createWatcher(subpath("watcher-remove")); + exec.scheduleWithFixedDelay(watcher, 0, 10, TimeUnit.MILLISECONDS); + + assertEquals("test", source.getValue("watcher-remove.test")); + + // when + Files.delete(subpath("watcher-remove/test")); + Thread.sleep(100); + + // then + assertEquals(null, source.getValue("watcher-remove.test")); + } + + public static BasicFileAttributes writeFile(Path filepath, String content) throws IOException { + Files.createDirectories(filepath.getParent()); + Files.write(filepath, content.getBytes(StandardCharsets.UTF_8)); + return Files.readAttributes(filepath, BasicFileAttributes.class); + } + + public static BasicFileAttributes writeRandFile(Path filepath, int bytes) throws IOException { + Files.createDirectories(filepath.getParent()); + + Random rnd = new Random(); + byte[] content = new byte[bytes]; + rnd.nextBytes(content); + + Files.write(filepath, content); + return Files.readAttributes(filepath, BasicFileAttributes.class); } - public static Path writeFile(Path parentDir, String filename, String content) throws IOException { - Path file = Paths.get(parentDir.toString(), filename); - Files.createDirectories(file.getParent()); - Files.write(file, content.getBytes(StandardCharsets.UTF_8)); - return file; + private static Path subpath(String subpath) { + return Paths.get(testDirectory.toString(), subpath); } } From 91a07ca4a5a26d0976a0a6250d5b3519ffdbe26f Mon Sep 17 00:00:00 2001 From: Oliver Bertuch Date: Tue, 26 Jan 2021 00:59:26 +0100 Subject: [PATCH 17/23] test(mpconfig): fix wrong file size test for DirSourceConfig --- .../microprofile/config/source/DirConfigSourceTest.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/nucleus/payara-modules/nucleus-microprofile/config-service/src/test/java/fish/payara/nucleus/microprofile/config/source/DirConfigSourceTest.java b/nucleus/payara-modules/nucleus-microprofile/config-service/src/test/java/fish/payara/nucleus/microprofile/config/source/DirConfigSourceTest.java index a88fbffc6d1..4b88af7cdff 100644 --- a/nucleus/payara-modules/nucleus-microprofile/config-service/src/test/java/fish/payara/nucleus/microprofile/config/source/DirConfigSourceTest.java +++ b/nucleus/payara-modules/nucleus-microprofile/config-service/src/test/java/fish/payara/nucleus/microprofile/config/source/DirConfigSourceTest.java @@ -120,12 +120,12 @@ public void testIsAptFile() throws IOException { } Path file100k = subpath("aptdir/100k-file"); - BasicFileAttributes atts100k = writeRandFile(file100k, 100); + BasicFileAttributes atts100k = writeRandFile(file100k, 100*1024); assertEquals(TRUE, DirConfigSource.isAptFile(file100k, atts100k)); Path file600k = subpath("aptdir/600k-file"); - BasicFileAttributes atts600k = writeRandFile(file600k, 600); - assertEquals(TRUE, DirConfigSource.isAptFile(file600k, atts600k)); + BasicFileAttributes atts600k = writeRandFile(file600k, 600*1024); + assertEquals(FALSE, DirConfigSource.isAptFile(file600k, atts600k)); } @Test From dc78c29891f616ce5ef9a9b68579a982e75c715a Mon Sep 17 00:00:00 2001 From: Oliver Bertuch Date: Wed, 27 Jan 2021 11:59:23 +0100 Subject: [PATCH 18/23] fix(mpconfig): Check for absolute pathes in DirConfigSource.findDir() This avoids pathes on Windows being invalid, because concatenating two absolute pathes is not possible on Windows. Part of a solution for #5006 --- .../config/source/DirConfigSource.java | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/nucleus/payara-modules/nucleus-microprofile/config-service/src/main/java/fish/payara/nucleus/microprofile/config/source/DirConfigSource.java b/nucleus/payara-modules/nucleus-microprofile/config-service/src/main/java/fish/payara/nucleus/microprofile/config/source/DirConfigSource.java index 520764268e1..a2fadfbd0ca 100644 --- a/nucleus/payara-modules/nucleus-microprofile/config-service/src/main/java/fish/payara/nucleus/microprofile/config/source/DirConfigSource.java +++ b/nucleus/payara-modules/nucleus-microprofile/config-service/src/main/java/fish/payara/nucleus/microprofile/config/source/DirConfigSource.java @@ -55,7 +55,7 @@ import java.nio.file.WatchService; import java.nio.file.attribute.BasicFileAttributes; import java.nio.file.attribute.FileTime; -import java.util.Arrays; +import java.util.ArrayList; import java.util.List; import java.util.Map; import java.util.Objects; @@ -276,17 +276,20 @@ public String getName() { private Path findDir() throws IOException { String path = configService.getMPConfig().getSecretDir(); - List candidates = Arrays.asList( - Paths.get(path), - // let's try it relative to server environment root - Paths.get(System.getProperty("com.sun.aas.instanceRoot"), path) - ); + List candidates = new ArrayList<>(); + + // adding all pathes where to look for the directory... + candidates.add(Paths.get(path)); + // let's try it relative to server environment root + if ( ! Paths.get(path).isAbsolute()) + candidates.add(Paths.get(System.getProperty("com.sun.aas.instanceRoot"), path).normalize()); + for (Path candidate : candidates) { - if (Files.exists(candidate) || Files.isDirectory(candidate) || Files.isReadable(candidate)) { + if (isAptDir(candidate)) { return candidate; } } - throw new IOException("Given MPCONFIG directory '"+path+"' is no directory or cannot be read."); + throw new IOException("Given MPCONFIG directory '"+path+"' is no directory, cannot be read or has a leading dot."); } /** From f7c2046cca0eb6de842d038f6fb8fb13b029723d Mon Sep 17 00:00:00 2001 From: Oliver Bertuch Date: Wed, 27 Jan 2021 12:45:55 +0100 Subject: [PATCH 19/23] test(mpconfig): Have DirConfigSource.findDir() covered by tests. Introducing some mocks to enable unit testing of findDir(). Needed to extend the base class constructors to be able to inject the mock. Relates to #5006 P.S. Crossing fingers this runs on Windoze. --- .../config/source/DirConfigSource.java | 8 +++-- .../config/source/PayaraConfigSource.java | 8 +++++ .../config/source/DirConfigSourceTest.java | 34 ++++++++++++++++++- 3 files changed, 46 insertions(+), 4 deletions(-) diff --git a/nucleus/payara-modules/nucleus-microprofile/config-service/src/main/java/fish/payara/nucleus/microprofile/config/source/DirConfigSource.java b/nucleus/payara-modules/nucleus-microprofile/config-service/src/main/java/fish/payara/nucleus/microprofile/config/source/DirConfigSource.java index a2fadfbd0ca..32f31b8044c 100644 --- a/nucleus/payara-modules/nucleus-microprofile/config-service/src/main/java/fish/payara/nucleus/microprofile/config/source/DirConfigSource.java +++ b/nucleus/payara-modules/nucleus-microprofile/config-service/src/main/java/fish/payara/nucleus/microprofile/config/source/DirConfigSource.java @@ -39,6 +39,7 @@ */ package fish.payara.nucleus.microprofile.config.source; +import fish.payara.nucleus.microprofile.config.spi.ConfigProviderResolverImpl; import org.eclipse.microprofile.config.spi.ConfigSource; import java.io.File; @@ -231,8 +232,9 @@ public DirConfigSource() { } // Used for testing only with explicit dependency injection - DirConfigSource(Path directory) { - super(true); + // Used for testing only with explicit dependency injection + DirConfigSource(Path directory, ConfigProviderResolverImpl configService) { + super(configService); this.directory = directory; } @@ -274,7 +276,7 @@ public String getName() { return "Directory"; } - private Path findDir() throws IOException { + Path findDir() throws IOException { String path = configService.getMPConfig().getSecretDir(); List candidates = new ArrayList<>(); diff --git a/nucleus/payara-modules/nucleus-microprofile/config-service/src/main/java/fish/payara/nucleus/microprofile/config/source/PayaraConfigSource.java b/nucleus/payara-modules/nucleus-microprofile/config-service/src/main/java/fish/payara/nucleus/microprofile/config/source/PayaraConfigSource.java index c1f207aec90..c5794d5fd80 100644 --- a/nucleus/payara-modules/nucleus-microprofile/config-service/src/main/java/fish/payara/nucleus/microprofile/config/source/PayaraConfigSource.java +++ b/nucleus/payara-modules/nucleus-microprofile/config-service/src/main/java/fish/payara/nucleus/microprofile/config/source/PayaraConfigSource.java @@ -67,4 +67,12 @@ public PayaraConfigSource() { configService = null; } + /** + * Should only be used for test purposes + * @param configService Usually a mocked implementation + */ + PayaraConfigSource(ConfigProviderResolverImpl configService) { + this.domainConfiguration = null; + this.configService = configService; + } } diff --git a/nucleus/payara-modules/nucleus-microprofile/config-service/src/test/java/fish/payara/nucleus/microprofile/config/source/DirConfigSourceTest.java b/nucleus/payara-modules/nucleus-microprofile/config-service/src/test/java/fish/payara/nucleus/microprofile/config/source/DirConfigSourceTest.java index 4b88af7cdff..9ee41675d71 100644 --- a/nucleus/payara-modules/nucleus-microprofile/config-service/src/test/java/fish/payara/nucleus/microprofile/config/source/DirConfigSourceTest.java +++ b/nucleus/payara-modules/nucleus-microprofile/config-service/src/test/java/fish/payara/nucleus/microprofile/config/source/DirConfigSourceTest.java @@ -39,6 +39,8 @@ */ package fish.payara.nucleus.microprofile.config.source; +import fish.payara.nucleus.microprofile.config.spi.ConfigProviderResolverImpl; +import fish.payara.nucleus.microprofile.config.spi.MicroprofileConfigConfiguration; import org.junit.AfterClass; import org.junit.BeforeClass; import org.junit.Test; @@ -65,18 +67,21 @@ import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; +import static org.mockito.Mockito.*; public class DirConfigSourceTest { private static Path testDirectory; private static DirConfigSource source; private static ScheduledExecutorService exec = Executors.newScheduledThreadPool(3); + private static ConfigProviderResolverImpl configService; @BeforeClass public static void setUp() throws IOException { testDirectory = Files.createTempDirectory("microprofile-config-test-"); + configService = mock(ConfigProviderResolverImpl.class); // create & load - source = new DirConfigSource(testDirectory); + source = new DirConfigSource(testDirectory, configService); } @AfterClass @@ -89,6 +94,33 @@ public static void tearDown() throws IOException, InterruptedException { .forEach(File::delete); } + @Test + public void testFindDir_AbsolutePath() throws IOException { + // given + MicroprofileConfigConfiguration config = mock(MicroprofileConfigConfiguration.class); + when(configService.getMPConfig()).thenReturn(config); + when(config.getSecretDir()).thenReturn(testDirectory.toString()); + // when + Path sut = source.findDir(); + // then + assertEquals(testDirectory, sut); + } + + @Test + public void testFindDir_RelativePath() throws IOException { + // given + System.setProperty("com.sun.aas.instanceRoot", testDirectory.toString()); + MicroprofileConfigConfiguration config = mock(MicroprofileConfigConfiguration.class); + when(configService.getMPConfig()).thenReturn(config); + when(config.getSecretDir()).thenReturn("."); + + // when + Path sut = source.findDir(); + + // then + assertEquals(testDirectory, sut); + } + @Test public void testIsAptDir() throws IOException { // given From 784995fc5fffe5423fdf9a1783c1ed937f9cf8e0 Mon Sep 17 00:00:00 2001 From: Oliver Bertuch Date: Wed, 27 Jan 2021 18:26:56 +0100 Subject: [PATCH 20/23] refactor(mpconfig): Make DirConfigSource.isLongestMatch() more testable and use less memory This commits make the logic part of isLongestMatch() a static function, much easier to unit test. The lookup part checking if the path is already present somewhere is handled in a non-static function, so staying compatible with using this config source for multiple locations. It also refactored the inner DirProperty class to avoid saving the path depth and compute it instead, reducing the waste of resources. Relates to #5006 --- .../config/source/DirConfigSource.java | 71 +++--- .../config/source/DirConfigSourceTest.java | 234 +++++++++++------- 2 files changed, 178 insertions(+), 127 deletions(-) diff --git a/nucleus/payara-modules/nucleus-microprofile/config-service/src/main/java/fish/payara/nucleus/microprofile/config/source/DirConfigSource.java b/nucleus/payara-modules/nucleus-microprofile/config-service/src/main/java/fish/payara/nucleus/microprofile/config/source/DirConfigSource.java index 32f31b8044c..a16c21c6fc9 100644 --- a/nucleus/payara-modules/nucleus-microprofile/config-service/src/main/java/fish/payara/nucleus/microprofile/config/source/DirConfigSource.java +++ b/nucleus/payara-modules/nucleus-microprofile/config-service/src/main/java/fish/payara/nucleus/microprofile/config/source/DirConfigSource.java @@ -84,33 +84,26 @@ static final class DirProperty { final String propertyValue; final FileTime lastModifiedTime; final Path path; - final int pathDepth; - DirProperty(String propertyValue, FileTime lastModifiedTime, Path path, int pathDepth) { + DirProperty(String propertyValue, FileTime lastModifiedTime, Path path) { this.propertyValue = propertyValue; this.lastModifiedTime = lastModifiedTime; this.path = path; - this.pathDepth = pathDepth; } - - DirProperty(String propertyValue, FileTime lastModifiedTime, Path path, Path rootPath) { - this(propertyValue, lastModifiedTime, path, rootPath.relativize(path).getNameCount()); - } - + @Override public boolean equals(Object o) { if (this == o) return true; if (o == null || getClass() != o.getClass()) return false; DirProperty that = (DirProperty) o; - return pathDepth == that.pathDepth && - propertyValue.equals(that.propertyValue) && + return propertyValue.equals(that.propertyValue) && lastModifiedTime.equals(that.lastModifiedTime) && path.equals(that.path); } @Override public int hashCode() { - return Objects.hash(propertyValue, lastModifiedTime, path, pathDepth); + return Objects.hash(propertyValue, lastModifiedTime, path); } } @@ -332,42 +325,59 @@ final void removePropertyFromPath(Path path) { } /** - * Check if the path given is a more specific path to a value for the given property - * Same path counts as more specific, too, to allow updates to the same file. + * Check if a new path to a given property is a more specific match for it. + * If both old and new paths are the same, it's still treated as more specific, allowing for updates. * @param property - * @param path - * @return true if more specific, false if not + * @param newPath + * @return true if new path more specific or equal to old path of property, false if not. */ - final boolean isLongestMatchForPath(String property, Path path) { - // Make path relative to config directory - // NOTE: we will never have a path containing "..", as our tree walkers are always inside this "root". - Path relativePath = directory.relativize(path); - + final boolean isLongestMatchForPath(String property, Path newPath) { + if (newPath == null || property == null || property.isEmpty()) + return false; // No property -> path is new and more specific if (! properties.containsKey(property)) return true; DirProperty old = properties.get(property); - + return isLongestMatchForPath(this.directory, old.path, newPath); + } + + /** + * Check if the new path given is a more specific path to a property file for a given old path + * Same path is more specific, too, to allow updates to the same file. + * @param rootDir + * @param oldPath + * @param newPath + * @return true if more specific, false if not + */ + static final boolean isLongestMatchForPath(Path rootDir, Path oldPath, Path newPath) { // Old and new path are the same -> "more" specific (update case) - if ( old.path.equals(path) ) + if (oldPath.equals(newPath)) return true; + // Make pathes relative to config directory and count the "levels" (path depth) + // NOTE: we will never have a path containing "..", as our tree walkers are always inside this "root". + int oldPathDepth = rootDir.relativize(oldPath).getNameCount(); + int newPathDepth = rootDir.relativize(newPath).getNameCount(); + // Check if this element has a higher path depth (longest match) // Example: "foo.bar/test/one.txt" (depth 2) wins over "foo.bar.test.one.txt" (depth 0) - boolean depth = old.pathDepth < relativePath.getNameCount(); + if (oldPathDepth < newPathDepth) + return true; // In case that both pathes have the same depth, we need to check on the position of dots. // Example: /config/foo.bar/test/one.txt is less specific than /config/foo/bar.test/one.txt - if (old.pathDepth == relativePath.getNameCount()) { - String oldPath = old.path.toString(); - String newPath = path.toAbsolutePath().toString(); + if (oldPathDepth == newPathDepth) { + String oldPathS = oldPath.toString(); + String newPathS = newPath.toAbsolutePath().toString(); int offset = 0; while (offset > -1) { - if (newPath.indexOf(".", offset) > oldPath.indexOf(".", offset)) return true; - offset = oldPath.indexOf(".", offset + 1); + if (newPathS.indexOf(".", offset) > oldPathS.indexOf(".", offset)) return true; + offset = oldPathS.indexOf(".", offset + 1); } } - return depth; + + // All other cases: no, it's not more specific. + return false; } /** @@ -444,8 +454,7 @@ static final DirProperty readPropertyFromPath(Path path, BasicFileAttributes mai return new DirProperty( new String(Files.readAllBytes(path), StandardCharsets.UTF_8), mainAtts.lastModifiedTime(), - path.toAbsolutePath(), - rootPath + path.toAbsolutePath() ); } throw new IOException("Cannot read property from '"+path.toString()+"'."); diff --git a/nucleus/payara-modules/nucleus-microprofile/config-service/src/test/java/fish/payara/nucleus/microprofile/config/source/DirConfigSourceTest.java b/nucleus/payara-modules/nucleus-microprofile/config-service/src/test/java/fish/payara/nucleus/microprofile/config/source/DirConfigSourceTest.java index 9ee41675d71..ff341e401a6 100644 --- a/nucleus/payara-modules/nucleus-microprofile/config-service/src/test/java/fish/payara/nucleus/microprofile/config/source/DirConfigSourceTest.java +++ b/nucleus/payara-modules/nucleus-microprofile/config-service/src/test/java/fish/payara/nucleus/microprofile/config/source/DirConfigSourceTest.java @@ -41,6 +41,7 @@ import fish.payara.nucleus.microprofile.config.spi.ConfigProviderResolverImpl; import fish.payara.nucleus.microprofile.config.spi.MicroprofileConfigConfiguration; +import org.junit.After; import org.junit.AfterClass; import org.junit.BeforeClass; import org.junit.Test; @@ -54,8 +55,11 @@ import java.nio.file.attribute.BasicFileAttributes; import java.nio.file.attribute.FileTime; import java.time.Instant; +import java.util.Arrays; +import java.util.Collections; import java.util.Comparator; import java.util.HashMap; +import java.util.List; import java.util.Map; import java.util.Random; import java.util.concurrent.Executors; @@ -94,6 +98,12 @@ public static void tearDown() throws IOException, InterruptedException { .forEach(File::delete); } + @After + public void cleanProps() { + // reset properties map after every test to avoid side effects + source.setProperties(Collections.emptyMap()); + } + @Test public void testFindDir_AbsolutePath() throws IOException { // given @@ -141,21 +151,21 @@ public void testIsAptDir() throws IOException { public void testIsAptFile() throws IOException { Map examples = new HashMap<>(); - examples.put(subpath( "aptdir/aptfile"), TRUE); - examples.put(subpath( "aptdir/.unaptfile"), FALSE); + examples.put(subpath( "aptdir", "aptfile"), TRUE); + examples.put(subpath( "aptdir", ".unaptfile"), FALSE); assertEquals(FALSE, DirConfigSource.isAptFile(null, null)); - assertEquals(FALSE, DirConfigSource.isAptFile(subpath( "aptdir/aptnotexisting"), null)); + assertEquals(FALSE, DirConfigSource.isAptFile(subpath( "aptdir", "aptnotexisting"), null)); for (Map.Entry ex : examples.entrySet()) { BasicFileAttributes atts = writeFile(ex.getKey(), "test"); assertEquals(ex.getValue(), DirConfigSource.isAptFile(ex.getKey(), atts)); } - Path file100k = subpath("aptdir/100k-file"); + Path file100k = subpath("aptdir", "100k-file"); BasicFileAttributes atts100k = writeRandFile(file100k, 100*1024); assertEquals(TRUE, DirConfigSource.isAptFile(file100k, atts100k)); - Path file600k = subpath("aptdir/600k-file"); + Path file600k = subpath("aptdir", "600k-file"); BasicFileAttributes atts600k = writeRandFile(file600k, 600*1024); assertEquals(FALSE, DirConfigSource.isAptFile(file600k, atts600k)); } @@ -164,21 +174,21 @@ public void testIsAptFile() throws IOException { public void testParsePropertyNameFromPath() { // given Map examples = new HashMap<>(); - examples.put(subpath( "foo/bar/test/ex"), "foo.bar.test.ex"); - examples.put(subpath( "foo.bar.test/ex"), "foo.bar.test.ex"); - examples.put(subpath( "foo/bar.test/ex"), "foo.bar.test.ex"); - examples.put(subpath( "foo.bar/test/ex"), "foo.bar.test.ex"); + examples.put(subpath( "foo", "bar", "test", "ex"), "foo.bar.test.ex"); + examples.put(subpath( "foo.bar.test", "ex"), "foo.bar.test.ex"); + examples.put(subpath( "foo", "bar.test", "ex"), "foo.bar.test.ex"); + examples.put(subpath( "foo.bar", "test", "ex"), "foo.bar.test.ex"); // we ignore the last file extension if not more than 3 chars. // this might lead to unexpected behaviour for a user. // best advice: do not use dots in filename, only in directory names. - examples.put(subpath( "foo/bar/test/ex.txt"), "foo.bar.test.ex"); - examples.put(subpath( "foo/bar/test/ex.tar.gz"), "foo.bar.test.ex.tar"); - examples.put(subpath( "foo/bar/test/ex.helo"), "foo.bar.test.ex.helo"); - examples.put(subpath( "foo.bar/test.ex"), "foo.bar.test"); - examples.put(subpath( "foo/bar.test.ex"), "foo.bar.test"); + examples.put(subpath( "foo", "bar", "test", "ex.txt"), "foo.bar.test.ex"); + examples.put(subpath( "foo", "bar", "test", "ex.tar.gz"), "foo.bar.test.ex.tar"); + examples.put(subpath( "foo", "bar", "test", "ex.helo"), "foo.bar.test.ex.helo"); + examples.put(subpath( "foo.bar", "test.ex"), "foo.bar.test"); + examples.put(subpath( "foo", "bar.test.ex"), "foo.bar.test"); examples.put(subpath( "foo.bar.test.ex"), "foo.bar.test"); - examples.put(subpath( "foo/bar/test.ex"), "foo.bar.test"); + examples.put(subpath( "foo", "bar", "test.ex"), "foo.bar.test"); // when & then for (Map.Entry ex : examples.entrySet()) { @@ -190,10 +200,10 @@ public void testParsePropertyNameFromPath() { @Test public void testReadPropertyFromPath() throws IOException { // given - Path sut = subpath("aptdir/sut-read-property"); + Path sut = subpath("aptdir", "sut-read-property"); BasicFileAttributes attsSUT = writeFile(sut, "foobar"); DirConfigSource.DirProperty example = new DirConfigSource.DirProperty( - "foobar", attsSUT.lastModifiedTime(), sut, testDirectory + "foobar", attsSUT.lastModifiedTime(), sut ); // when & then assertEquals(example, DirConfigSource.readPropertyFromPath(sut, attsSUT, testDirectory)); @@ -202,69 +212,104 @@ public void testReadPropertyFromPath() throws IOException { @Test public void testCheckLongestMatchForPath_SamePath() { // given - Map props = new HashMap<>(); - // a property with a most specific path - String property = "foo.bar.test.ex"; - props.put(property, - new DirConfigSource.DirProperty( - "test", FileTime.from(Instant.now()), - subpath( "foo/bar/test/ex"), - testDirectory)); - source.setProperties(props); + List paths = Arrays.asList( + subpath("foo.bar.ex.test.hello"), + subpath("foo.bar.ex.test", "hello.txt"), + subpath("foo.bar.ex", "test", "hello.txt"), + subpath("foo.bar", "ex", "test", "hello.txt") + ); // when & then - assertTrue(source.isLongestMatchForPath(property, subpath("foo/bar/test/ex"))); + for (Path p : paths) + assertTrue(DirConfigSource.isLongestMatchForPath(testDirectory, p, p)); } @Test - public void testCheckLongestMatchForPath_PathDepthLessSpecific() { + public void testCheckLongestMatchForPath_PathDepthGrowing() { // given - Map props = new HashMap<>(); - // a property with a most specific path - String property = "foo.bar.test.ex"; - props.put(property, - new DirConfigSource.DirProperty( - "test", FileTime.from(Instant.now()), - subpath( "foo/bar/test/ex"), - testDirectory)); - source.setProperties(props); - - // when & then - assertFalse(source.isLongestMatchForPath(property, subpath( "foo/bar.test/ex"))); + String propFile = "foo.bar.ex.test.hello"; + + for (int i = 0; i < propFile.chars().filter(ch -> ch == '.').count(); i++) { + // when + String newPath = propFile.replaceFirst("\\.", File.separator); + + // then + assertTrue(DirConfigSource.isLongestMatchForPath(testDirectory, subpath(propFile), subpath(newPath))); + assertFalse(DirConfigSource.isLongestMatchForPath(testDirectory, subpath(newPath), subpath(propFile))); + + propFile = newPath; + } } @Test - public void testCheckLongestMatchForPath_PathDepthMoreSpecific() { + public void testCheckLongestMatchForPath_PathDepthShrinking() { // given - Map props = new HashMap<>(); - // a property with a most specific path - String property = "foo.bar.test.ex"; - props.put(property, - new DirConfigSource.DirProperty( - "test", FileTime.from(Instant.now()), - subpath( "foo.bar/test/ex"), - testDirectory)); - source.setProperties(props); - - // when & then - assertTrue(source.isLongestMatchForPath(property, subpath( "foo/bar/test/ex"))); + String propFile = Paths.get("foo", "bar", "ex", "test", "hello").toString(); + + for (int i = 0; i < propFile.chars().filter(ch -> ch == '.').count(); i++) { + // when + String newPath = propFile.replaceFirst(File.separator, "."); + + // then + assertFalse(DirConfigSource.isLongestMatchForPath(testDirectory, subpath(propFile), subpath(newPath))); + assertTrue(DirConfigSource.isLongestMatchForPath(testDirectory, subpath(newPath), subpath(propFile))); + + propFile = newPath; + } } @Test - public void testCheckLongestMatchForPath_PathDepthEqualMoreSpecific() { + public void testCheckLongestMatchForPath_PathDepthEqualMoreSpecificDotsLeftGrowing() { // given - Map props = new HashMap<>(); - // a property with a most specific path - String property = "foo.bar.test.ex.one"; - props.put(property, - new DirConfigSource.DirProperty( - "test", FileTime.from(Instant.now()), - subpath( "foo.bar/test.ex/one"), - testDirectory)); - source.setProperties(props); + String prop = "foo.bar.example.test.hello.world.stranger.ohmy.how.long.is.this"; + + // get all dot positions + int dotcount = (int)prop.chars().filter(ch -> ch == '.').count(); + int[] pos = new int[dotcount]; + int offset = 0; + for (int i = 0; i < pos.length; i++) { + pos[i] = prop.indexOf(".", offset); + offset = pos[i]+1; + } + + // move the slashes on the string and check for being more specific + for (int i = 0; i < dotcount-6; i++) { + // when + StringBuffer oldPath = new StringBuffer(prop) + .replace(pos[i], pos[i]+1, File.separator) + .replace(pos[i+2], pos[i+2]+1, File.separator) + .replace(pos[i+5], pos[i+5]+1, File.separator); + //System.out.println(oldPath.toString()); + StringBuffer newPath = new StringBuffer(prop) + .replace(pos[i+1], pos[i+1]+1, File.separator) + .replace(pos[i+3], pos[i+3]+1, File.separator) + .replace(pos[i+6], pos[i+6]+1, File.separator); + //System.out.println(newPath.toString()); + + // then + // --> we assert that independent on the number of dots in dir names (the total count of dots present + // does not change above), the path with a dot more counting from the left is always more specific. + // keep in mind that the number of subdirectory levels is not changing! + assertTrue(DirConfigSource.isLongestMatchForPath(testDirectory, subpath(oldPath.toString()), subpath(newPath.toString()))); + } + } + + @Test + public void testCheckLongestMatchForPath_PathDepthEqualMoreSpecificDotMoving() { + // given + List propPaths = Arrays.asList( + new String[]{"foo.bar", "example", "test", "hello"}, + new String[]{"foo", "bar.example", "test", "hello"}, + new String[]{"foo", "bar", "example.test", "hello"}, + new String[]{"foo", "bar", "example", "test.hello"} + ); - // when & then - assertTrue(source.isLongestMatchForPath(property, subpath( "foo.bar/test/ex.one.txt"))); + // move the slashes on the string and check for being more specific + for (int i = 1; i < propPaths.size()-1; i++) { + // when & then + // --> we assert that a larger number of subdirectories before a dot is always a more specific path + assertTrue(DirConfigSource.isLongestMatchForPath(testDirectory, subpath(propPaths.get(i-1)), subpath(propPaths.get(i))));; + } } @Test @@ -276,12 +321,12 @@ public void testCheckLongestMatchForPath_PropNotPresent() { props.put(property, new DirConfigSource.DirProperty( "test", FileTime.from(Instant.now()), - subpath( "foo.bar/test.ex/one"), - testDirectory)); + // different from call below, as we want to test a missing property! + subpath("foo.bar", "test.ex", "one"))); source.setProperties(props); // when & then - assertTrue(source.isLongestMatchForPath("foo.bar.test.ex.two", subpath( "foo.bar/test/ex.two.txt"))); + assertTrue(source.isLongestMatchForPath("foo.bar.test.ex.two", subpath( "foo.bar", "test", "ex.two.txt"))); } @Test @@ -293,13 +338,12 @@ public void testRemovePropertyFromPath() { props.put(property, new DirConfigSource.DirProperty( "test", FileTime.from(Instant.now()), - subpath( "foo/bar/test"), - testDirectory)); + subpath( "foo", "bar", "test"))); source.setProperties(props); assertEquals("test", source.getValue(property)); // when - source.removePropertyFromPath(subpath( "foo/bar/test")); + source.removePropertyFromPath(subpath( "foo", "bar", "test")); // then assertTrue(source.getValue(property) == null); @@ -308,11 +352,10 @@ public void testRemovePropertyFromPath() { @Test public void testUpsertPropertyFromPath_InsertCase() throws IOException { // given - Path sut = subpath("aptdir/sut-upsert-property-insert"); + Path sut = subpath("aptdir", "sut-upsert-property-insert"); BasicFileAttributes attsSUT = writeFile(sut, "foobar"); DirConfigSource.DirProperty example = new DirConfigSource.DirProperty( - "foobar", attsSUT.lastModifiedTime(), sut, testDirectory - ); + "foobar", attsSUT.lastModifiedTime(), sut); // when & then assertEquals(true, source.upsertPropertyFromPath(sut, attsSUT)); @@ -322,11 +365,10 @@ public void testUpsertPropertyFromPath_InsertCase() throws IOException { @Test public void testUpsertPropertyFromPath_UpdateCase() throws IOException { // given - Path sut = subpath("aptdir/sut-upsert-property-update"); + Path sut = subpath("aptdir", "sut-upsert-property-update"); BasicFileAttributes attsSUT = writeFile(sut, "foobar"); DirConfigSource.DirProperty example = new DirConfigSource.DirProperty( - "foobar", attsSUT.lastModifiedTime(), sut, testDirectory - ); + "foobar", attsSUT.lastModifiedTime(), sut); assertEquals(true, source.upsertPropertyFromPath(sut, attsSUT)); assertEquals(example.propertyValue, source.getValue("aptdir.sut-upsert-property-update")); @@ -341,13 +383,13 @@ public void testUpsertPropertyFromPath_UpdateCase() throws IOException { public void testPropertyWatcher_RegisterAndInit() throws Exception { // given Map examples = new HashMap<>(); - examples.put(subpath( "init-watcher/foo/bar/test/ex"), "init-watcher.foo.bar.test.ex"); - examples.put(subpath( "init-watcher/foo.bar.test/hello"), "init-watcher.foo.bar.test.hello"); - examples.put(subpath( "init-watcher/.foo/ex"), "init-watcher.foo.ex"); + examples.put(subpath( "init-watcher", "foo", "bar", "test", "ex"), "init-watcher.foo.bar.test.ex"); + examples.put(subpath( "init-watcher", "foo.bar.test", "hello"), "init-watcher.foo.bar.test.hello"); + examples.put(subpath( "init-watcher", ".foo", "ex"), "init-watcher.foo.ex"); for (Map.Entry ex : examples.entrySet()) { writeFile(ex.getKey(), "foobar"); } - Files.createSymbolicLink(subpath( "init-watcher/foo.hello"), subpath("init-watcher/.foo/ex")); + Files.createSymbolicLink(subpath( "init-watcher", "foo.hello"), subpath("init-watcher", ".foo", "ex")); // when DirConfigSource.DirPropertyWatcher watcher = source.createWatcher(subpath("init-watcher")); @@ -362,9 +404,9 @@ public void testPropertyWatcher_RegisterAndInit() throws Exception { @Test public void testPropertyWatcher_RunFilesNewUpdate() throws Exception { // given - writeFile(subpath("watcher-files/foobar"), "test"); - writeFile(subpath("watcher-files/.hidden/foobar"), "hidden"); - Files.createSymbolicLink(subpath("watcher-files/revealed"), subpath("watcher-files/.hidden/foobar")); + writeFile(subpath("watcher-files", "foobar"), "test"); + writeFile(subpath("watcher-files", ".hidden", "foobar"), "hidden"); + Files.createSymbolicLink(subpath("watcher-files", "revealed"), subpath("watcher-files", ".hidden", "foobar")); DirConfigSource.DirPropertyWatcher watcher = source.createWatcher(subpath("watcher-files")); exec.scheduleWithFixedDelay(watcher, 0, 10, TimeUnit.MILLISECONDS); @@ -373,12 +415,12 @@ public void testPropertyWatcher_RunFilesNewUpdate() throws Exception { assertEquals("hidden", source.getValue("watcher-files.revealed")); // when - writeFile(subpath("watcher-files/foobar"), "test2"); - writeFile(subpath("watcher-files/example"), "test2"); - writeFile(subpath("watcher-files/reveal/.hidden"), "test2"); - writeFile(subpath("watcher-files/.hidden/foobar"), "showme"); - Files.delete(subpath("watcher-files/revealed")); - Files.createSymbolicLink(subpath("watcher-files/revealed"), subpath("watcher-files/.hidden/foobar")); + writeFile(subpath("watcher-files", "foobar"), "test2"); + writeFile(subpath("watcher-files", "example"), "test2"); + writeFile(subpath("watcher-files", "reveal", ".hidden"), "test2"); + writeFile(subpath("watcher-files", ".hidden", "foobar"), "showme"); + Files.delete(subpath("watcher-files", "revealed")); + Files.createSymbolicLink(subpath("watcher-files", "revealed"), subpath("watcher-files", ".hidden", "foobar")); Thread.sleep(100); // then @@ -391,7 +433,7 @@ public void testPropertyWatcher_RunFilesNewUpdate() throws Exception { @Test public void testPropertyWatcher_RunNewDir() throws Exception { // given - writeFile(subpath("watcher-newdir/test"), "test"); + writeFile(subpath("watcher-newdir", "test"), "test"); DirConfigSource.DirPropertyWatcher watcher = source.createWatcher(subpath("watcher-newdir")); exec.scheduleWithFixedDelay(watcher, 0, 10, TimeUnit.MILLISECONDS); @@ -399,8 +441,8 @@ public void testPropertyWatcher_RunNewDir() throws Exception { assertEquals("test", source.getValue("watcher-newdir.test")); // when - writeFile(subpath("watcher-newdir/foobar/test"), "test"); - writeFile(subpath("watcher-newdir/.hidden/foobar"), "test"); + writeFile(subpath("watcher-newdir", "foobar/test"), "test"); + writeFile(subpath("watcher-newdir", ".hidden/foobar"), "test"); Thread.sleep(100); // then @@ -411,7 +453,7 @@ public void testPropertyWatcher_RunNewDir() throws Exception { @Test public void testPropertyWatcher_RunRemove() throws Exception { // given - writeFile(subpath("watcher-remove/test"), "test"); + writeFile(subpath("watcher-remove", "test"), "test"); DirConfigSource.DirPropertyWatcher watcher = source.createWatcher(subpath("watcher-remove")); exec.scheduleWithFixedDelay(watcher, 0, 10, TimeUnit.MILLISECONDS); @@ -419,7 +461,7 @@ public void testPropertyWatcher_RunRemove() throws Exception { assertEquals("test", source.getValue("watcher-remove.test")); // when - Files.delete(subpath("watcher-remove/test")); + Files.delete(subpath("watcher-remove", "test")); Thread.sleep(100); // then @@ -443,7 +485,7 @@ public static BasicFileAttributes writeRandFile(Path filepath, int bytes) throws return Files.readAttributes(filepath, BasicFileAttributes.class); } - private static Path subpath(String subpath) { + private static Path subpath(String... subpath) { return Paths.get(testDirectory.toString(), subpath); } From 401555dc167bc9ab319e2b35839073360f0e061a Mon Sep 17 00:00:00 2001 From: Oliver Bertuch Date: Wed, 27 Jan 2021 19:25:50 +0100 Subject: [PATCH 21/23] feat(mpconfig): Make DirConfigSource ignore files with certains extensions Ignore *.properties, *.yaml, *.yml, *.xml, *.json files, as those are likely to contain more complex structures used for other config sources. Remember: the file name compiles to the property name (up to three letter endings silently cut off) and might contain whatever. If you put a file with any of these extensions in the directory on purpose, it should not be used with this config source. - Relates to #5006 - Based on request from @pdudits in #5007 --- .../config/source/DirConfigSource.java | 8 +++----- .../config/source/DirConfigSourceTest.java | 20 ++++++++++++------- 2 files changed, 16 insertions(+), 12 deletions(-) diff --git a/nucleus/payara-modules/nucleus-microprofile/config-service/src/main/java/fish/payara/nucleus/microprofile/config/source/DirConfigSource.java b/nucleus/payara-modules/nucleus-microprofile/config-service/src/main/java/fish/payara/nucleus/microprofile/config/source/DirConfigSource.java index a16c21c6fc9..d4426d30aea 100644 --- a/nucleus/payara-modules/nucleus-microprofile/config-service/src/main/java/fish/payara/nucleus/microprofile/config/source/DirConfigSource.java +++ b/nucleus/payara-modules/nucleus-microprofile/config-service/src/main/java/fish/payara/nucleus/microprofile/config/source/DirConfigSource.java @@ -56,11 +56,7 @@ import java.nio.file.WatchService; import java.nio.file.attribute.BasicFileAttributes; import java.nio.file.attribute.FileTime; -import java.util.ArrayList; -import java.util.List; -import java.util.Map; -import java.util.Objects; -import java.util.Set; +import java.util.*; import java.util.concurrent.ConcurrentHashMap; import java.util.logging.Logger; @@ -210,6 +206,7 @@ public final void run() { } private static final Logger logger = Logger.getLogger(DirConfigSource.class.getName()); + static final String[] ignoredExtensions = {".xml", ".yaml", ".yml", ".json", ".properties"}; private Path directory; private final ConcurrentHashMap properties = new ConcurrentHashMap<>(); @@ -404,6 +401,7 @@ public final static boolean isAptFile(Path path, BasicFileAttributes atts) throw return path != null && Files.exists(path) && Files.isRegularFile(path) && Files.isReadable(path) && !path.getFileName().toString().startsWith(".") && + Arrays.stream(ignoredExtensions).noneMatch(ext -> path.getFileName().toString().endsWith(ext)) && atts.size() < 512*1024; } diff --git a/nucleus/payara-modules/nucleus-microprofile/config-service/src/test/java/fish/payara/nucleus/microprofile/config/source/DirConfigSourceTest.java b/nucleus/payara-modules/nucleus-microprofile/config-service/src/test/java/fish/payara/nucleus/microprofile/config/source/DirConfigSourceTest.java index ff341e401a6..2724c470180 100644 --- a/nucleus/payara-modules/nucleus-microprofile/config-service/src/test/java/fish/payara/nucleus/microprofile/config/source/DirConfigSourceTest.java +++ b/nucleus/payara-modules/nucleus-microprofile/config-service/src/test/java/fish/payara/nucleus/microprofile/config/source/DirConfigSourceTest.java @@ -71,7 +71,8 @@ import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; -import static org.mockito.Mockito.*; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; public class DirConfigSourceTest { @@ -149,13 +150,12 @@ public void testIsAptDir() throws IOException { @Test public void testIsAptFile() throws IOException { - Map examples = new HashMap<>(); examples.put(subpath( "aptdir", "aptfile"), TRUE); examples.put(subpath( "aptdir", ".unaptfile"), FALSE); - - assertEquals(FALSE, DirConfigSource.isAptFile(null, null)); - assertEquals(FALSE, DirConfigSource.isAptFile(subpath( "aptdir", "aptnotexisting"), null)); + + assertFalse(DirConfigSource.isAptFile(null, null)); + assertFalse(DirConfigSource.isAptFile(subpath( "aptdir", "aptnotexisting"), null)); for (Map.Entry ex : examples.entrySet()) { BasicFileAttributes atts = writeFile(ex.getKey(), "test"); assertEquals(ex.getValue(), DirConfigSource.isAptFile(ex.getKey(), atts)); @@ -163,11 +163,17 @@ public void testIsAptFile() throws IOException { Path file100k = subpath("aptdir", "100k-file"); BasicFileAttributes atts100k = writeRandFile(file100k, 100*1024); - assertEquals(TRUE, DirConfigSource.isAptFile(file100k, atts100k)); + assertTrue(DirConfigSource.isAptFile(file100k, atts100k)); Path file600k = subpath("aptdir", "600k-file"); BasicFileAttributes atts600k = writeRandFile(file600k, 600*1024); - assertEquals(FALSE, DirConfigSource.isAptFile(file600k, atts600k)); + assertFalse(DirConfigSource.isAptFile(file600k, atts600k)); + + for (String ext : DirConfigSource.ignoredExtensions) { + Path file = subpath("aptdir", "ignorefile"+ext); + BasicFileAttributes attsFile = writeFile(file, "test"); + assertFalse(DirConfigSource.isAptFile(file, attsFile)); + } } @Test From ce30d87d4c05366db7451beee69e581b449fafcc Mon Sep 17 00:00:00 2001 From: Oliver Bertuch Date: Wed, 27 Jan 2021 19:41:16 +0100 Subject: [PATCH 22/23] revert(mpconfig): by request from @pdudits, this isn't necessary for now. This reverts commit 401555dc167bc9ab319e2b35839073360f0e061a. --- .../config/source/DirConfigSource.java | 8 +++++--- .../config/source/DirConfigSourceTest.java | 20 +++++++------------ 2 files changed, 12 insertions(+), 16 deletions(-) diff --git a/nucleus/payara-modules/nucleus-microprofile/config-service/src/main/java/fish/payara/nucleus/microprofile/config/source/DirConfigSource.java b/nucleus/payara-modules/nucleus-microprofile/config-service/src/main/java/fish/payara/nucleus/microprofile/config/source/DirConfigSource.java index d4426d30aea..a16c21c6fc9 100644 --- a/nucleus/payara-modules/nucleus-microprofile/config-service/src/main/java/fish/payara/nucleus/microprofile/config/source/DirConfigSource.java +++ b/nucleus/payara-modules/nucleus-microprofile/config-service/src/main/java/fish/payara/nucleus/microprofile/config/source/DirConfigSource.java @@ -56,7 +56,11 @@ import java.nio.file.WatchService; import java.nio.file.attribute.BasicFileAttributes; import java.nio.file.attribute.FileTime; -import java.util.*; +import java.util.ArrayList; +import java.util.List; +import java.util.Map; +import java.util.Objects; +import java.util.Set; import java.util.concurrent.ConcurrentHashMap; import java.util.logging.Logger; @@ -206,7 +210,6 @@ public final void run() { } private static final Logger logger = Logger.getLogger(DirConfigSource.class.getName()); - static final String[] ignoredExtensions = {".xml", ".yaml", ".yml", ".json", ".properties"}; private Path directory; private final ConcurrentHashMap properties = new ConcurrentHashMap<>(); @@ -401,7 +404,6 @@ public final static boolean isAptFile(Path path, BasicFileAttributes atts) throw return path != null && Files.exists(path) && Files.isRegularFile(path) && Files.isReadable(path) && !path.getFileName().toString().startsWith(".") && - Arrays.stream(ignoredExtensions).noneMatch(ext -> path.getFileName().toString().endsWith(ext)) && atts.size() < 512*1024; } diff --git a/nucleus/payara-modules/nucleus-microprofile/config-service/src/test/java/fish/payara/nucleus/microprofile/config/source/DirConfigSourceTest.java b/nucleus/payara-modules/nucleus-microprofile/config-service/src/test/java/fish/payara/nucleus/microprofile/config/source/DirConfigSourceTest.java index 2724c470180..ff341e401a6 100644 --- a/nucleus/payara-modules/nucleus-microprofile/config-service/src/test/java/fish/payara/nucleus/microprofile/config/source/DirConfigSourceTest.java +++ b/nucleus/payara-modules/nucleus-microprofile/config-service/src/test/java/fish/payara/nucleus/microprofile/config/source/DirConfigSourceTest.java @@ -71,8 +71,7 @@ import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; -import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.when; +import static org.mockito.Mockito.*; public class DirConfigSourceTest { @@ -150,12 +149,13 @@ public void testIsAptDir() throws IOException { @Test public void testIsAptFile() throws IOException { + Map examples = new HashMap<>(); examples.put(subpath( "aptdir", "aptfile"), TRUE); examples.put(subpath( "aptdir", ".unaptfile"), FALSE); - - assertFalse(DirConfigSource.isAptFile(null, null)); - assertFalse(DirConfigSource.isAptFile(subpath( "aptdir", "aptnotexisting"), null)); + + assertEquals(FALSE, DirConfigSource.isAptFile(null, null)); + assertEquals(FALSE, DirConfigSource.isAptFile(subpath( "aptdir", "aptnotexisting"), null)); for (Map.Entry ex : examples.entrySet()) { BasicFileAttributes atts = writeFile(ex.getKey(), "test"); assertEquals(ex.getValue(), DirConfigSource.isAptFile(ex.getKey(), atts)); @@ -163,17 +163,11 @@ public void testIsAptFile() throws IOException { Path file100k = subpath("aptdir", "100k-file"); BasicFileAttributes atts100k = writeRandFile(file100k, 100*1024); - assertTrue(DirConfigSource.isAptFile(file100k, atts100k)); + assertEquals(TRUE, DirConfigSource.isAptFile(file100k, atts100k)); Path file600k = subpath("aptdir", "600k-file"); BasicFileAttributes atts600k = writeRandFile(file600k, 600*1024); - assertFalse(DirConfigSource.isAptFile(file600k, atts600k)); - - for (String ext : DirConfigSource.ignoredExtensions) { - Path file = subpath("aptdir", "ignorefile"+ext); - BasicFileAttributes attsFile = writeFile(file, "test"); - assertFalse(DirConfigSource.isAptFile(file, attsFile)); - } + assertEquals(FALSE, DirConfigSource.isAptFile(file600k, atts600k)); } @Test From 45d7a33ca55b6237f175f3dcac5c605dbe675a13 Mon Sep 17 00:00:00 2001 From: Oliver Bertuch Date: Wed, 27 Jan 2021 19:46:11 +0100 Subject: [PATCH 23/23] fix(mpconfig): Do not cutoff file extensions for property names in DirConfigSource As requested by @pdudits, do not cut off file extensions silently. People on Windows can deal with text files without a file extension, better stay consistent. Relates to #5006 --- .../config/source/DirConfigSource.java | 20 ++----------------- .../config/source/DirConfigSourceTest.java | 14 ++----------- 2 files changed, 4 insertions(+), 30 deletions(-) diff --git a/nucleus/payara-modules/nucleus-microprofile/config-service/src/main/java/fish/payara/nucleus/microprofile/config/source/DirConfigSource.java b/nucleus/payara-modules/nucleus-microprofile/config-service/src/main/java/fish/payara/nucleus/microprofile/config/source/DirConfigSource.java index a16c21c6fc9..e8bdc93f99e 100644 --- a/nucleus/payara-modules/nucleus-microprofile/config-service/src/main/java/fish/payara/nucleus/microprofile/config/source/DirConfigSource.java +++ b/nucleus/payara-modules/nucleus-microprofile/config-service/src/main/java/fish/payara/nucleus/microprofile/config/source/DirConfigSource.java @@ -418,30 +418,14 @@ public final static String parsePropertyNameFromPath(Path path, Path rootDir) { String property = ""; if (! path.getParent().equals(rootDir)) property += rootDir.relativize(path.getParent()).toString() + File.separatorChar; - // 2. ignore all file suffixes after last dot - property += removeFileExtension(path.getFileName().toString()); + // 2. add the file name (might be used for mangling in the future) + property += path.getFileName(); // 3. replace all path seps with a ".", property = property.replace(File.separatorChar, '.'); // so "/config/foo/bar/test/one.txt" becomes "foo/bar/test/one.txt" becomes "foo.bar.test.one" property name return property; } - /** - * Litte helper to remove the file extension (not present in Java std functionality) - * @param filename A filename containing a dot, marking the start of the file extension - * @return Filename without a suffix (if present) - */ - public final static String removeFileExtension(String filename) { - if (filename == null || ! filename.contains(".")) - return filename; - int lastIndex = filename.lastIndexOf('.'); - // dot does not belong to file, but parent dir or - // extension is longer than 3 chars (all clear text formats would have 3 chars max) - if (filename.lastIndexOf(File.separatorChar) > lastIndex || lastIndex < filename.length() - 4) - return filename; - return filename.substring(0, lastIndex); - } - /** * Actually read the data from the file, assuming UTF-8 formatted content, creating properties from it. * @param path The file to read diff --git a/nucleus/payara-modules/nucleus-microprofile/config-service/src/test/java/fish/payara/nucleus/microprofile/config/source/DirConfigSourceTest.java b/nucleus/payara-modules/nucleus-microprofile/config-service/src/test/java/fish/payara/nucleus/microprofile/config/source/DirConfigSourceTest.java index ff341e401a6..5300557f64c 100644 --- a/nucleus/payara-modules/nucleus-microprofile/config-service/src/test/java/fish/payara/nucleus/microprofile/config/source/DirConfigSourceTest.java +++ b/nucleus/payara-modules/nucleus-microprofile/config-service/src/test/java/fish/payara/nucleus/microprofile/config/source/DirConfigSourceTest.java @@ -71,7 +71,8 @@ import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; -import static org.mockito.Mockito.*; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; public class DirConfigSourceTest { @@ -179,17 +180,6 @@ public void testParsePropertyNameFromPath() { examples.put(subpath( "foo", "bar.test", "ex"), "foo.bar.test.ex"); examples.put(subpath( "foo.bar", "test", "ex"), "foo.bar.test.ex"); - // we ignore the last file extension if not more than 3 chars. - // this might lead to unexpected behaviour for a user. - // best advice: do not use dots in filename, only in directory names. - examples.put(subpath( "foo", "bar", "test", "ex.txt"), "foo.bar.test.ex"); - examples.put(subpath( "foo", "bar", "test", "ex.tar.gz"), "foo.bar.test.ex.tar"); - examples.put(subpath( "foo", "bar", "test", "ex.helo"), "foo.bar.test.ex.helo"); - examples.put(subpath( "foo.bar", "test.ex"), "foo.bar.test"); - examples.put(subpath( "foo", "bar.test.ex"), "foo.bar.test"); - examples.put(subpath( "foo.bar.test.ex"), "foo.bar.test"); - examples.put(subpath( "foo", "bar", "test.ex"), "foo.bar.test"); - // when & then for (Map.Entry ex : examples.entrySet()) { //System.out.println(ex.getKey()+" = "+ex.getValue());