Quadlet - add full support for Symlinks

Use os.ReadDir recursively instead of filepath.WalkDir
Use map instead of list to easily find looped Symlinks
Update existing tests and add a more elaborate one
Update the man page

Signed-off-by: Ygal Blum <ygal.blum@gmail.com>

cmd/quadlet/main.go

@@ -107,9 +107,11 @@ func Debugf(format string, a ...interface{}) {
 // For system generators these are in /usr/share/containers/systemd (for distro files)
 // and /etc/containers/systemd (for sysadmin files).
 // For user generators these can live in $XDG_RUNTIME_DIR/containers/systemd, /etc/containers/systemd/users, /etc/containers/systemd/users/$UID, and $XDG_CONFIG_HOME/containers/systemd
-func getUnitDirs(rootless bool) []string {
+func getUnitDirs(rootless bool) map[string]struct{} {
+	dirs := make(map[string]struct{}, 0)
+
 	// Allow overriding source dir, this is mainly for the CI tests
-	if varExist, dirs := getDirsFromEnv(); varExist {
+	if getDirsFromEnv(dirs) {
 		return dirs
 	}
 
@@ -119,61 +121,57 @@ func getUnitDirs(rootless bool) []string {
 	if rootless {
 		systemUserDirLevel := len(strings.Split(resolvedUnitDirAdminUser, string(os.PathSeparator)))
 		nonNumericFilter := getNonNumericFilter(resolvedUnitDirAdminUser, systemUserDirLevel)
-		return getRootlessDirs(nonNumericFilter, userLevelFilter)
+		getRootlessDirs(dirs, nonNumericFilter, userLevelFilter)
+	} else {
+		getRootDirs(dirs, userLevelFilter)
 	}
-
+	return dirs
-	return getRootDirs(userLevelFilter)
 }
 
-func getDirsFromEnv() (bool, []string) {
+func getDirsFromEnv(dirs map[string]struct{}) bool {
 	unitDirsEnv := os.Getenv("QUADLET_UNIT_DIRS")
 	if len(unitDirsEnv) == 0 {
-		return false, nil
+		return false
 	}
 
-	dirs := make([]string, 0)
 	for _, eachUnitDir := range strings.Split(unitDirsEnv, ":") {
 		if !filepath.IsAbs(eachUnitDir) {
 			Logf("%s not a valid file path", eachUnitDir)
-			return true, nil
+			break
 		}
-		dirs = appendSubPaths(dirs, eachUnitDir, false, nil)
+		appendSubPaths(dirs, eachUnitDir, false, nil)
 	}
-	return true, dirs
+	return true
 }
 
-func getRootlessDirs(nonNumericFilter, userLevelFilter func(string, bool) bool) []string {
+func getRootlessDirs(dirs map[string]struct{}, nonNumericFilter, userLevelFilter func(string, bool) bool) {
-	dirs := make([]string, 0)
-
 	runtimeDir, found := os.LookupEnv("XDG_RUNTIME_DIR")
 	if found {
-		dirs = appendSubPaths(dirs, path.Join(runtimeDir, "containers/systemd"), false, nil)
+		appendSubPaths(dirs, path.Join(runtimeDir, "containers/systemd"), false, nil)
 	}
 
 	configDir, err := os.UserConfigDir()
 	if err != nil {
 		fmt.Fprintf(os.Stderr, "Warning: %v", err)
-		return nil
+		return
 	}
-	dirs = appendSubPaths(dirs, path.Join(configDir, "containers/systemd"), false, nil)
+	appendSubPaths(dirs, path.Join(configDir, "containers/systemd"), false, nil)
 
 	u, err := user.Current()
 	if err == nil {
-		dirs = appendSubPaths(dirs, filepath.Join(quadlet.UnitDirAdmin, "users"), true, nonNumericFilter)
+		appendSubPaths(dirs, filepath.Join(quadlet.UnitDirAdmin, "users"), true, nonNumericFilter)
-		dirs = appendSubPaths(dirs, filepath.Join(quadlet.UnitDirAdmin, "users", u.Uid), true, userLevelFilter)
+		appendSubPaths(dirs, filepath.Join(quadlet.UnitDirAdmin, "users", u.Uid), true, userLevelFilter)
 	} else {
 		fmt.Fprintf(os.Stderr, "Warning: %v", err)
 	}
 
-	return append(dirs, filepath.Join(quadlet.UnitDirAdmin, "users"))
+	dirs[filepath.Join(quadlet.UnitDirAdmin, "users")] = struct{}{}
 }
 
-func getRootDirs(userLevelFilter func(string, bool) bool) []string {
+func getRootDirs(dirs map[string]struct{}, userLevelFilter func(string, bool) bool) {
-	dirs := make([]string, 0)
+	appendSubPaths(dirs, quadlet.UnitDirTemp, false, userLevelFilter)
-
+	appendSubPaths(dirs, quadlet.UnitDirAdmin, false, userLevelFilter)
-	dirs = appendSubPaths(dirs, quadlet.UnitDirTemp, false, userLevelFilter)
+	appendSubPaths(dirs, quadlet.UnitDirDistro, false, nil)
-	dirs = appendSubPaths(dirs, quadlet.UnitDirAdmin, false, userLevelFilter)
-	return appendSubPaths(dirs, quadlet.UnitDirDistro, false, nil)
 }
 
 func resolveUnitDirAdminUser() string {
@@ -189,7 +187,7 @@ func resolveUnitDirAdminUser() string {
 	return resolvedUnitDirAdminUser
 }
 
-func appendSubPaths(dirs []string, path string, isUserFlag bool, filterPtr func(string, bool) bool) []string {
+func appendSubPaths(dirs map[string]struct{}, path string, isUserFlag bool, filterPtr func(string, bool) bool) {
 	resolvedPath, err := filepath.EvalSymlinks(path)
 	if err != nil {
 		if !errors.Is(err, fs.ErrNotExist) {
@@ -197,27 +195,55 @@ func appendSubPaths(dirs []string, path string, isUserFlag bool, filterPtr func(
 		}
 		// Despite the failure add the path to the list for logging purposes
 		// This is the equivalent of adding the path when info==nil below
-		dirs = append(dirs, path)
+		dirs[path] = struct{}{}
-		return dirs
+		return
 	}
 
-	err = filepath.WalkDir(resolvedPath, func(_path string, info os.DirEntry, err error) error {
+	// If the resolvedPath is already in the map no need to read it again
-		// Ignore drop-in directory subpaths
+	if _, already := dirs[resolvedPath]; already {
-		if !strings.HasSuffix(_path, ".d") {
+		return
-			if info == nil || info.IsDir() {
+	}
-				if filterPtr == nil || filterPtr(_path, isUserFlag) {
+
-					dirs = append(dirs, _path)
+	// Don't traverse drop-in directories
-				}
+	if strings.HasSuffix(resolvedPath, ".d") {
-			}
+		return
+	}
+
+	// Check if the directory should be filtered out
+	if filterPtr != nil && !filterPtr(resolvedPath, isUserFlag) {
+		return
+	}
+
+	stat, err := os.Stat(resolvedPath)
+	if err != nil {
+		if !errors.Is(err, fs.ErrNotExist) {
+			Debugf("Error occurred resolving path %q: %s", path, err)
 		}
-		return err
+		return
-	})
+	}
+
+	// Not a directory nothing to add
+	if !stat.IsDir() {
+		return
+	}
+
+	// Add the current directory
+	dirs[resolvedPath] = struct{}{}
+
+	// Read the contents of the directory
+	entries, err := os.ReadDir(resolvedPath)
 	if err != nil {
 		if !errors.Is(err, os.ErrNotExist) {
 			Debugf("Error occurred walking sub directories %q: %s", path, err)
 		}
+		return
+	}
+
+	// Recursively run through the contents of the directory
+	for _, entry := range entries {
+		fullPath := filepath.Join(resolvedPath, entry.Name())
+		appendSubPaths(dirs, fullPath, isUserFlag, filterPtr)
 	}
-	return dirs
 }
 
 func getNonNumericFilter(resolvedUnitDirAdminUser string, systemUserDirLevel int) func(string, bool) bool {
@@ -297,7 +323,7 @@ func loadUnitsFromDir(sourcePath string) ([]*parser.UnitFile, error) {
 	return units, prevError
 }
 
-func loadUnitDropins(unit *parser.UnitFile, sourcePaths []string) error {
+func loadUnitDropins(unit *parser.UnitFile, sourcePaths map[string]struct{}) error {
 	var prevError error
 	reportError := func(err error) {
 		if prevError != nil {
@@ -309,7 +335,7 @@ func loadUnitDropins(unit *parser.UnitFile, sourcePaths []string) error {
 	dropinDirs := []string{}
 	unitDropinPaths := unit.GetUnitDropinPaths()
 
-	for _, sourcePath := range sourcePaths {
+	for sourcePath := range sourcePaths {
 		for _, dropinPath := range unitDropinPaths {
 			dropinDirs = append(dropinDirs, path.Join(sourcePath, dropinPath))
 		}
@@ -620,10 +646,10 @@ func process() error {
 		Debugf("Starting quadlet-generator, output to: %s", outputPath)
 	}
 
-	sourcePaths := getUnitDirs(isUserFlag)
+	sourcePathsMap := getUnitDirs(isUserFlag)
 
 	var units []*parser.UnitFile
-	for _, d := range sourcePaths {
+	for d := range sourcePathsMap {
 		if result, err := loadUnitsFromDir(d); err != nil {
 			reportError(err)
 		} else {
@@ -634,12 +660,12 @@ func process() error {
 	if len(units) == 0 {
 		// containers/podman/issues/17374: exit cleanly but log that we
 		// had nothing to do
-		Debugf("No files parsed from %s", sourcePaths)
+		Debugf("No files parsed from %s", sourcePathsMap)
 		return prevError
 	}
 
 	for _, unit := range units {
-		if err := loadUnitDropins(unit, sourcePaths); err != nil {
+		if err := loadUnitDropins(unit, sourcePathsMap); err != nil {
 			reportError(err)
 		}
 	}

cmd/quadlet/main_test.go

@@ -64,31 +64,36 @@ func TestUnitDirs(t *testing.T) {
 
 		resolvedUnitDirAdminUser := resolveUnitDirAdminUser()
 		userLevelFilter := getUserLevelFilter(resolvedUnitDirAdminUser)
-		rootDirs := []string{}
+		rootDirs := make(map[string]struct{}, 0)
-		rootDirs = appendSubPaths(rootDirs, quadlet.UnitDirTemp, false, userLevelFilter)
+		appendSubPaths(rootDirs, quadlet.UnitDirTemp, false, userLevelFilter)
-		rootDirs = appendSubPaths(rootDirs, quadlet.UnitDirAdmin, false, userLevelFilter)
+		appendSubPaths(rootDirs, quadlet.UnitDirAdmin, false, userLevelFilter)
-		rootDirs = appendSubPaths(rootDirs, quadlet.UnitDirDistro, false, userLevelFilter)
+		appendSubPaths(rootDirs, quadlet.UnitDirDistro, false, userLevelFilter)
-		assert.Equal(t, unitDirs, rootDirs, "rootful unit dirs should match")
+		assert.Equal(t, rootDirs, unitDirs, "rootful unit dirs should match")
 
 		configDir, err := os.UserConfigDir()
 		assert.Nil(t, err)
 
-		rootlessDirs := []string{}
+		rootlessDirs := make(map[string]struct{}, 0)
 
 		systemUserDirLevel := len(strings.Split(resolvedUnitDirAdminUser, string(os.PathSeparator)))
 		nonNumericFilter := getNonNumericFilter(resolvedUnitDirAdminUser, systemUserDirLevel)
 
 		runtimeDir, found := os.LookupEnv("XDG_RUNTIME_DIR")
 		if found {
-			rootlessDirs = appendSubPaths(rootlessDirs, path.Join(runtimeDir, "containers/systemd"), false, nil)
+			appendSubPaths(rootlessDirs, path.Join(runtimeDir, "containers/systemd"), false, nil)
 		}
-		rootlessDirs = appendSubPaths(rootlessDirs, path.Join(configDir, "containers/systemd"), false, nil)
+		appendSubPaths(rootlessDirs, path.Join(configDir, "containers/systemd"), false, nil)
-		rootlessDirs = appendSubPaths(rootlessDirs, filepath.Join(quadlet.UnitDirAdmin, "users"), true, nonNumericFilter)
+		appendSubPaths(rootlessDirs, filepath.Join(quadlet.UnitDirAdmin, "users"), true, nonNumericFilter)
-		rootlessDirs = appendSubPaths(rootlessDirs, filepath.Join(quadlet.UnitDirAdmin, "users", u.Uid), true, userLevelFilter)
+		appendSubPaths(rootlessDirs, filepath.Join(quadlet.UnitDirAdmin, "users", u.Uid), true, userLevelFilter)
-		rootlessDirs = append(rootlessDirs, filepath.Join(quadlet.UnitDirAdmin, "users"))
+		rootlessDirs[filepath.Join(quadlet.UnitDirAdmin, "users")] = struct{}{}
 
 		unitDirs = getUnitDirs(true)
-		assert.Equal(t, unitDirs, rootlessDirs, "rootless unit dirs should match")
+		assert.Equal(t, rootlessDirs, unitDirs, "rootless unit dirs should match")
+
+		// Test that relative path returns an empty list
+		t.Setenv("QUADLET_UNIT_DIRS", "./relative/path")
+		unitDirs = getUnitDirs(false)
+		assert.Equal(t, map[string]struct{}{}, unitDirs)
 
 		name, err := os.MkdirTemp("", "dir")
 		assert.Nil(t, err)
@@ -97,10 +102,10 @@ func TestUnitDirs(t *testing.T) {
 
 		t.Setenv("QUADLET_UNIT_DIRS", name)
 		unitDirs = getUnitDirs(false)
-		assert.Equal(t, unitDirs, []string{name}, "rootful should use environment variable")
+		assert.Equal(t, map[string]struct{}{name: {}}, unitDirs, "rootful should use environment variable")
 
 		unitDirs = getUnitDirs(true)
-		assert.Equal(t, unitDirs, []string{name}, "rootless should use environment variable")
+		assert.Equal(t, map[string]struct{}{name: {}}, unitDirs, "rootless should use environment variable")
 
 		symLinkTestBaseDir, err := os.MkdirTemp("", "podman-symlinktest")
 		assert.Nil(t, err)
@@ -118,7 +123,72 @@ func TestUnitDirs(t *testing.T) {
 		assert.Nil(t, err)
 		t.Setenv("QUADLET_UNIT_DIRS", symlink)
 		unitDirs = getUnitDirs(true)
-		assert.Equal(t, unitDirs, []string{actualDir, innerDir}, "directory resolution should follow symlink")
+		assert.Equal(t, map[string]struct{}{actualDir: {}, innerDir: {}}, unitDirs, "directory resolution should follow symlink")
+
+		// Make a more elborate test with the following structure:
+		// <BASE>/linkToDir - real directory to link to
+		// <BASE>/linkToDir/a - real directory
+		// <BASE>/linkToDir/b - link to <BASE>/unitDir/b/a should be ignored
+		// <BASE>/linkToDir/c - link to <BASE>/unitDir should be ignored
+		// <BASE>/unitDir - start from here
+		// <BASE>/unitDir/a - real directory
+		// <BASE>/unitDir/a/a - real directory
+		// <BASE>/unitDir/a/a/a - real directory
+		// <BASE>/unitDir/b/a - real directory
+		// <BASE>/unitDir/b/b - link to <BASE>/unitDir/a/a should be ignored
+		// <BASE>/unitDir/c - link to <BASE>/linkToDir
+		symLinkRecursiveTestBaseDir, err := os.MkdirTemp("", "podman-symlink-recursive-test")
+		assert.Nil(t, err)
+		// remove the temporary directory at the end of the program
+		defer os.RemoveAll(symLinkRecursiveTestBaseDir)
+
+		createDir := func(path, name string, dirs map[string]struct{}) string {
+			dirName := filepath.Join(path, name)
+			assert.NotContains(t, dirs, dirName)
+			err = os.Mkdir(dirName, 0755)
+			assert.Nil(t, err)
+			dirs[dirName] = struct{}{}
+			return dirName
+		}
+		expectedDirs := make(map[string]struct{}, 0)
+		// Create <BASE>/linkToDir
+		linkToDirPath := createDir(symLinkRecursiveTestBaseDir, "linkToDir", expectedDirs)
+		// Create <BASE>/linkToDir/a
+		createDir(linkToDirPath, "a", expectedDirs)
+		// Create <BASE>/unitDir
+		unitsDirPath := createDir(symLinkRecursiveTestBaseDir, "unitsDir", expectedDirs)
+		// Create <BASE>/unitDir/a
+		aDirPath := createDir(unitsDirPath, "a", expectedDirs)
+		// Create <BASE>/unitDir/a/a
+		aaDirPath := createDir(aDirPath, "a", expectedDirs)
+		// Create <BASE>/unitDir/a/b
+		createDir(aDirPath, "b", expectedDirs)
+		// Create <BASE>/unitDir/a/a/a
+		createDir(aaDirPath, "a", expectedDirs)
+		// Create <BASE>/unitDir/b
+		bDirPath := createDir(unitsDirPath, "b", expectedDirs)
+		// Create <BASE>/unitDir/b/a
+		baDirPath := createDir(bDirPath, "a", expectedDirs)
+
+		linkDir := func(path, name, target string) {
+			linkName := filepath.Join(path, name)
+			err = os.Symlink(target, linkName)
+			assert.Nil(t, err)
+		}
+		// Link <BASE>/unitDir/b/b to <BASE>/unitDir/a/a
+		linkDir(bDirPath, "b", aaDirPath)
+		// Link <BASE>/linkToDir/b to <BASE>/unitDir/b/a
+		linkDir(linkToDirPath, "b", baDirPath)
+		// Link <BASE>/linkToDir/c to <BASE>/unitDir
+		linkDir(linkToDirPath, "c", unitsDirPath)
+		// Link <BASE>/unitDir/c to <BASE>/linkToDir
+		linkDir(unitsDirPath, "c", linkToDirPath)
+
+		t.Setenv("QUADLET_UNIT_DIRS", unitsDirPath)
+		unitDirs = getUnitDirs(true)
+		assert.Equal(t, expectedDirs, unitDirs, "directory resolution should follow symlink")
+		// remove the temporary directory at the end of the program
+		defer os.RemoveAll(symLinkTestBaseDir)
 
 		// because chroot is only available for root,
 		// unshare the namespace and map user to root

docs/source/markdown/podman-systemd.unit.5.md

@@ -35,8 +35,7 @@ Quadlet files for non-root users can be placed in the following directories
 
 ### Using symbolic links
 
-Quadlet supports using symbolic links for the base of the search paths.
+Quadlet supports using symbolic links for the base of the search paths and inside them.
-Symbolic links below the search paths are not supported.
 
 ## DESCRIPTION
 

test/e2e/quadlet_test.go

@@ -713,7 +713,7 @@ var _ = Describe("quadlet system generator", func() {
 			Expect(session).Should(Exit(0))
 
 			current := session.ErrorToStringArray()
-			expected := "No files parsed from [/something]"
+			expected := "No files parsed from map[/something:{}]"
 
 			found := false
 			for _, line := range current {