Watch symlinks for changes for SSL hot reloading

Prior to this commit, the SSL file hot reloading feature would
create file watchers for all configured locations and would resolve
symbolic links if they're found as registrations.

This arrangement would work for typical Let's Encrypt setups, but
would not get notified of consecutive changes for k8s setups.
Kubernetes uses a mix of symlinks and atomic file moves to update
secrets.

This commit not only tracks changes to symlinks targets, but also
to the original symlink.

Closes gh-44807

Signed-off-by: Dmytro Nosan <[email protected]>
[[email protected]: apply code conventions]
Signed-off-by: Brian Clozel <[email protected]>

Author: nosan

spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/ssl/FileWatcher.java

@@ -35,7 +35,6 @@
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.CopyOnWriteArrayList;
 import java.util.concurrent.TimeUnit;
-import java.util.stream.Collectors;
 
 import org.apache.commons.logging.Log;
 import org.apache.commons.logging.LogFactory;
@@ -86,26 +85,18 @@ void watch(Set<Path> paths, Runnable action) {
 					this.thread = new WatcherThread();
 					this.thread.start();
 				}
-				Set<Path> actualPaths = new HashSet<>();
+				Set<Path> registrationPaths = new HashSet<>();
 				for (Path path : paths) {
-					actualPaths.add(resolveSymlinkIfNecessary(path));
+					registrationPaths.addAll(getRegistrationPaths(path));
 				}
-				this.thread.register(new Registration(actualPaths, action));
+				this.thread.register(new Registration(registrationPaths, action));
 			}
 			catch (IOException ex) {
 				throw new UncheckedIOException("Failed to register paths for watching: " + paths, ex);
 			}
 		}
 	}
 
-	private static Path resolveSymlinkIfNecessary(Path path) throws IOException {
-		if (Files.isSymbolicLink(path)) {
-			Path target = path.resolveSibling(Files.readSymbolicLink(path));
-			return resolveSymlinkIfNecessary(target);
-		}
-		return path;
-	}
-
 	@Override
 	public void close() throws IOException {
 		synchronized (this.lock) {
@@ -123,6 +114,44 @@ public void close() throws IOException {
 		}
 	}
 
+	/**
+	 * Retrieves all {@link Path Paths} that should be registered for the specified
+	 * {@link Path}. If the path is a symlink, changes to the symlink should be monitored,
+	 * not just the file it points to. For example, for the given {@code keystore.jks}
+	 * path in the following directory structure:<pre>
+	 * .
+	 * ├── ..a72e81ff-f0e1-41d8-a19b-068d3d1d4e2f
+	 * │   ├── keystore.jks
+	 * ├── ..data -> ..a72e81ff-f0e1-41d8-a19b-068d3d1d4e2f
+	 * ├── keystore.jks -> ..data/keystore.jks
+	 * </pre> the resulting paths would include:
+	 * <ul>
+	 * <li><b>keystore.jks</b></li>
+	 * <li><b>..data/keystore.jks</b></li>
+	 * <li><b>..data</b></li>
+	 * <li><b>..a72e81ff-f0e1-41d8-a19b-068d3d1d4e2f/keystore.jks</b></li>
+	 * </ul>
+	 * @param path the path
+	 * @return all possible {@link Path} instances to be registered
+	 * @throws IOException if an I/O error occurs
+	 */
+	private static Set<Path> getRegistrationPaths(Path path) throws IOException {
+		path = path.toAbsolutePath();
+		Set<Path> result = new HashSet<>();
+		result.add(path);
+		Path parent = path.getParent();
+		if (parent != null && Files.isSymbolicLink(parent)) {
+			result.add(parent);
+			Path target = parent.resolveSibling(Files.readSymbolicLink(parent));
+			result.addAll(getRegistrationPaths(target.resolve(path.getFileName())));
+		}
+		else if (Files.isSymbolicLink(path)) {
+			Path target = path.resolveSibling(Files.readSymbolicLink(path));
+			result.addAll(getRegistrationPaths(target));
+		}
+		return result;
+	}
+
 	/**
 	 * The watcher thread used to check for changes.
 	 */
@@ -145,11 +174,15 @@ private void onThreadException(Thread thread, Throwable throwable) {
 		}
 
 		void register(Registration registration) throws IOException {
+			Set<Path> directories = new HashSet<>();
 			for (Path path : registration.paths()) {
 				if (!Files.isRegularFile(path) && !Files.isDirectory(path)) {
 					throw new IOException("'%s' is neither a file nor a directory".formatted(path));
 				}
 				Path directory = Files.isDirectory(path) ? path : path.getParent();
+				directories.add(directory);
+			}
+			for (Path directory : directories) {
 				WatchKey watchKey = register(directory);
 				this.registrations.computeIfAbsent(watchKey, (key) -> new CopyOnWriteArrayList<>()).add(registration);
 			}
@@ -224,10 +257,6 @@ public void close() throws IOException {
 	 */
 	private record Registration(Set<Path> paths, Runnable action) {
 
-		Registration {
-			paths = paths.stream().map(Path::toAbsolutePath).collect(Collectors.toSet());
-		}
-
 		boolean manages(Path file) {
 			Path absolutePath = file.toAbsolutePath();
 			return this.paths.contains(absolutePath) || isInDirectories(absolutePath);

spring-boot-project/spring-boot-autoconfigure/src/test/java/org/springframework/boot/autoconfigure/ssl/FileWatcherTests.java

@@ -18,8 +18,10 @@
 
 import java.io.IOException;
 import java.io.UncheckedIOException;
+import java.nio.file.AccessDeniedException;
 import java.nio.file.Files;
 import java.nio.file.Path;
+import java.nio.file.StandardCopyOption;
 import java.time.Duration;
 import java.util.Set;
 import java.util.UUID;
@@ -254,6 +256,62 @@ void shouldTriggerOnConfigMapUpdates(@TempDir Path tempDir) throws Exception {
 		}
 	}
 
+	/**
+	 * Updates many times K8s ConfigMap/Secret with atomic move. <pre>
+	 * .
+	 * ├── ..a72e81ff-f0e1-41d8-a19b-068d3d1d4e2f
+	 * │   ├── keystore.jks
+	 * ├── ..data -> ..a72e81ff-f0e1-41d8-a19b-068d3d1d4e2f
+	 * ├── keystore.jks -> ..data/keystore.jks
+	 * </pre>
+	 *
+	 * After a first a ConfigMap/Secret update, this will look like: <pre>
+	 * .
+	 * ├── ..bba2a61f-ce04-4c35-93aa-e455110d4487
+	 * │   ├── keystore.jks
+	 * ├── ..data -> ..bba2a61f-ce04-4c35-93aa-e455110d4487
+	 * ├── keystore.jks -> ..data/keystore.jks
+	 * </pre> After a second a ConfigMap/Secret update, this will look like: <pre>
+	 * .
+	 * ├── ..134887f0-df8f-4433-b70c-7784d2a33bd1
+	 * │   ├── keystore.jks
+	 * ├── ..data -> ..134887f0-df8f-4433-b70c-7784d2a33bd1
+	 * ├── keystore.jks -> ..data/keystore.jks
+	 *</pre>
+	 * <p>
+	 * When Kubernetes updates either the ConfigMap or Secret, it performs the following
+	 * steps:
+	 * <ul>
+	 * <li>Creates a new unique directory.</li>
+	 * <li>Writes the ConfigMap/Secret content to the newly created directory.</li>
+	 * <li>Creates a symlink {@code ..data_tmp} pointing to the newly created
+	 * directory.</li>
+	 * <li>Performs an atomic rename of {@code ..data_tmp} to {@code ..data}.</li>
+	 * <li>Deletes the old ConfigMap/Secret directory.</li>
+	 * </ul>
+	 */
+	@Test
+	void shouldTriggerOnConfigMapAtomicMoveUpdates(@TempDir Path tempDir) throws Exception {
+		Path configMap1 = createConfigMap(tempDir, "keystore.jks");
+		Path data = Files.createSymbolicLink(tempDir.resolve("..data"), configMap1);
+		Files.createSymbolicLink(tempDir.resolve("keystore.jks"), data.resolve("keystore.jks"));
+		WaitingCallback callback = new WaitingCallback();
+		this.fileWatcher.watch(Set.of(tempDir.resolve("keystore.jks")), callback);
+		// First update
+		Path configMap2 = createConfigMap(tempDir, "keystore.jks");
+		Path dataTmp = Files.createSymbolicLink(tempDir.resolve("..data_tmp"), configMap2);
+		move(dataTmp, data);
+		FileSystemUtils.deleteRecursively(configMap1);
+		callback.expectChanges();
+		callback.reset();
+		// Second update
+		Path configMap3 = createConfigMap(tempDir, "keystore.jks");
+		dataTmp = Files.createSymbolicLink(tempDir.resolve("..data_tmp"), configMap3);
+		move(dataTmp, data);
+		FileSystemUtils.deleteRecursively(configMap2);
+		callback.expectChanges();
+	}
+
 	Path createConfigMap(Path parentDir, String secretFileName) throws IOException {
 		Path configMapFolder = parentDir.resolve(".." + UUID.randomUUID());
 		Files.createDirectory(configMapFolder);
@@ -262,9 +320,19 @@ Path createConfigMap(Path parentDir, String secretFileName) throws IOException {
 		return configMapFolder;
 	}
 
+	private void move(Path source, Path target) throws IOException {
+		try {
+			Files.move(source, target, StandardCopyOption.ATOMIC_MOVE);
+		}
+		catch (AccessDeniedException ex) {
+			// Windows
+			Files.move(source, target, StandardCopyOption.REPLACE_EXISTING);
+		}
+	}
+
 	private static final class WaitingCallback implements Runnable {
 
-		private final CountDownLatch latch = new CountDownLatch(1);
+		private CountDownLatch latch = new CountDownLatch(1);
 
 		volatile boolean changed = false;
 
@@ -292,6 +360,11 @@ void waitForChanges(boolean fail) throws InterruptedException {
 			}
 		}
 
+		void reset() {
+			this.latch = new CountDownLatch(1);
+			this.changed = false;
+		}
+
 	}
 
 }