Merge pull request #773 from elezar/fix-libcuda-symlink

Force creation of symlinks in create-symlink hook

cmd/nvidia-cdi-hook/create-symlinks/create-symlinks.go

@@ -63,7 +63,7 @@ func (m command) build() *cli.Command {
 	c.Flags = []cli.Flag{
 		&cli.StringSliceFlag{
 			Name:        "link",
-			Usage:       "Specify a specific link to create. The link is specified as target::link",
+			Usage:       "Specify a specific link to create. The link is specified as target::link. If the link exists in the container root, it is removed.",
 			Destination: &cfg.links,
 		},
 		// The following flags are testing-only flags.
@@ -112,18 +112,19 @@ func (m command) run(c *cli.Context, cfg *config) error {
 // createLink creates a symbolic link in the specified container root.
 // This is equivalent to:
 //
-//	chroot {{ .containerRoot }} ln -s {{ .target }} {{ .link }}
+//	chroot {{ .containerRoot }} ln -f -s {{ .target }} {{ .link }}
 //
 // If the specified link already exists and points to the same target, this
-// operation is a no-op. If the link points to a different target, an error is
-// returned.
+// operation is a no-op.
+// If a file exists at the link path or the link points to a different target
+// this file is removed before creating the link.
 //
 // Note that if the link path resolves to an absolute path oudside of the
 // specified root, this is treated as an absolute path in this root.
 func (m command) createLink(containerRoot string, targetPath string, link string) error {
 	linkPath := filepath.Join(containerRoot, link)
 
-	exists, err := doesLinkExist(targetPath, linkPath)
+	exists, err := linkExists(targetPath, linkPath)
 	if err != nil {
 		return fmt.Errorf("failed to check if link exists: %w", err)
 	}
@@ -132,17 +133,21 @@ func (m command) createLink(containerRoot string, targetPath string, link string
 		return nil
 	}
 
-	resolvedLinkPath, err := symlink.FollowSymlinkInScope(linkPath, containerRoot)
+	// We resolve the parent of the symlink that we're creating in the container root.
+	// If we resolve the full link path, an existing link at the location itself
+	// is also resolved here and we are unable to force create the link.
+	resolvedLinkParent, err := symlink.FollowSymlinkInScope(filepath.Dir(linkPath), containerRoot)
 	if err != nil {
 		return fmt.Errorf("failed to follow path for link %v relative to %v: %w", link, containerRoot, err)
 	}
+	resolvedLinkPath := filepath.Join(resolvedLinkParent, filepath.Base(linkPath))
 
 	m.logger.Infof("Symlinking %v to %v", resolvedLinkPath, targetPath)
 	err = os.MkdirAll(filepath.Dir(resolvedLinkPath), 0755)
 	if err != nil {
 		return fmt.Errorf("failed to create directory: %v", err)
 	}
-	err = os.Symlink(targetPath, resolvedLinkPath)
+	err = symlinks.ForceCreate(targetPath, resolvedLinkPath)
 	if err != nil {
 		return fmt.Errorf("failed to create symlink: %v", err)
 	}
@@ -150,9 +155,9 @@ func (m command) createLink(containerRoot string, targetPath string, link string
 	return nil
 }
 
-// doesLinkExist returns true if link exists and points to target.
-// An error is returned if link exists but points to a different target.
-func doesLinkExist(target string, link string) (bool, error) {
+// linkExists checks whether the specified link exists.
+// A link exists if the path exists, is a symlink, and points to the specified target.
+func linkExists(target string, link string) (bool, error) {
 	currentTarget, err := symlinks.Resolve(link)
 	if errors.Is(err, os.ErrNotExist) {
 		return false, nil
@@ -163,5 +168,5 @@ func doesLinkExist(target string, link string) (bool, error) {
 	if currentTarget == target {
 		return true, nil
 	}
-	return true, fmt.Errorf("unexpected link target: %s", currentTarget)
+	return false, nil
 }

cmd/nvidia-cdi-hook/create-symlinks/create-symlinks_test.go

@@ -12,7 +12,7 @@ import (
 	"github.com/NVIDIA/nvidia-container-toolkit/internal/lookup/symlinks"
 )
 
-func TestDoesLinkExist(t *testing.T) {
+func TestLinkExist(t *testing.T) {
 	tmpDir := t.TempDir()
 	require.NoError(
 		t,
@@ -22,21 +22,23 @@ func TestDoesLinkExist(t *testing.T) {
 		),
 	)
 
-	exists, err := doesLinkExist("d", filepath.Join(tmpDir, "/a/b/c"))
+	exists, err := linkExists("d", filepath.Join(tmpDir, "/a/b/c"))
 	require.NoError(t, err)
 	require.True(t, exists)
 
-	exists, err = doesLinkExist("/a/b/f", filepath.Join(tmpDir, "/a/b/e"))
+	exists, err = linkExists("/a/b/f", filepath.Join(tmpDir, "/a/b/e"))
 	require.NoError(t, err)
 	require.True(t, exists)
 
-	_, err = doesLinkExist("different-target", filepath.Join(tmpDir, "/a/b/c"))
-	require.Error(t, err)
+	exists, err = linkExists("different-target", filepath.Join(tmpDir, "/a/b/c"))
+	require.NoError(t, err)
+	require.False(t, exists)
 
-	_, err = doesLinkExist("/a/b/d", filepath.Join(tmpDir, "/a/b/c"))
-	require.Error(t, err)
+	exists, err = linkExists("/a/b/d", filepath.Join(tmpDir, "/a/b/c"))
+	require.NoError(t, err)
+	require.False(t, exists)
 
-	exists, err = doesLinkExist("foo", filepath.Join(tmpDir, "/a/b/does-not-exist"))
+	exists, err = linkExists("foo", filepath.Join(tmpDir, "/a/b/does-not-exist"))
 	require.NoError(t, err)
 	require.False(t, exists)
 }
@@ -190,12 +192,30 @@ func TestCreateLinkAbsolutePath(t *testing.T) {
 }
 
 func TestCreateLinkAlreadyExists(t *testing.T) {
+	testCases := []struct {
+		description       string
+		containerContents []dirOrLink
+		shouldExist       []string
+	}{
+		{
+			description:       "link already exists with correct target",
+			containerContents: []dirOrLink{{path: "/lib/libfoo.so", target: "libfoo.so.1"}},
+			shouldExist:       []string{},
+		},
+		{
+			description:       "link already exists with different target",
+			containerContents: []dirOrLink{{path: "/lib/libfoo.so", target: "different-target"}, {path: "different-target"}},
+			shouldExist:       []string{"{{ .containerRoot }}/different-target"},
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.description, func(t *testing.T) {
 			tmpDir := t.TempDir()
 			hostRoot := filepath.Join(tmpDir, "/host-root/")
 			containerRoot := filepath.Join(tmpDir, "/container-root")
-
 			require.NoError(t, makeFs(hostRoot))
-	require.NoError(t, makeFs(containerRoot, dirOrLink{path: "/lib/libfoo.so", target: "libfoo.so.1"}))
+			require.NoError(t, makeFs(containerRoot, tc.containerContents...))
 
 			// nvidia-cdi-hook create-symlinks --link libfoo.so.1::/lib/libfoo.so
 			err := getTestCommand().createLink(containerRoot, "libfoo.so.1", "/lib/libfoo.so")
@@ -203,30 +223,24 @@ func TestCreateLinkAlreadyExists(t *testing.T) {
 			target, err := symlinks.Resolve(filepath.Join(containerRoot, "lib/libfoo.so"))
 			require.NoError(t, err)
 			require.Equal(t, "libfoo.so.1", target)
-}
 
-func TestCreateLinkAlreadyExistsDifferentTarget(t *testing.T) {
-	tmpDir := t.TempDir()
-	hostRoot := filepath.Join(tmpDir, "/host-root/")
-	containerRoot := filepath.Join(tmpDir, "/container-root")
-
-	require.NoError(t, makeFs(hostRoot))
-	require.NoError(t, makeFs(containerRoot, dirOrLink{path: "/lib/libfoo.so", target: "different-target"}))
-
-	// nvidia-cdi-hook create-symlinks --link libfoo.so.1::/lib/libfoo.so
-	err := getTestCommand().createLink(containerRoot, "libfoo.so.1", "/lib/libfoo.so")
-	require.Error(t, err)
-	target, err := symlinks.Resolve(filepath.Join(containerRoot, "lib/libfoo.so"))
-	require.NoError(t, err)
-	require.Equal(t, "different-target", target)
+			for _, p := range tc.shouldExist {
+				require.DirExists(t, strings.ReplaceAll(p, "{{ .containerRoot }}", containerRoot))
+			}
+		})
+	}
 }
 
 func TestCreateLinkOutOfBounds(t *testing.T) {
 	tmpDir := t.TempDir()
-	hostRoot := filepath.Join(tmpDir, "/host-root/")
+	hostRoot := filepath.Join(tmpDir, "/host-root")
 	containerRoot := filepath.Join(tmpDir, "/container-root")
 
-	require.NoError(t, makeFs(hostRoot))
+	require.NoError(t,
+		makeFs(hostRoot,
+			dirOrLink{path: "libfoo.so"},
+		),
+	)
 	require.NoError(t,
 		makeFs(containerRoot,
 			dirOrLink{path: "/lib"},
@@ -240,12 +254,13 @@ func TestCreateLinkOutOfBounds(t *testing.T) {
 
 	// nvidia-cdi-hook create-symlinks --link ../libfoo.so.1::/lib/foo/libfoo.so
 	_ = getTestCommand().createLink(containerRoot, "../libfoo.so.1", "/lib/foo/libfoo.so")
-	// TODO: We need to enabled this check once we have updated the implementation.
-	// require.Error(t, err)
-	_, err = os.Lstat(filepath.Join(hostRoot, "libfoo.so"))
-	require.ErrorIs(t, err, os.ErrNotExist)
-	_, err = os.Lstat(filepath.Join(containerRoot, hostRoot, "libfoo.so"))
 	require.NoError(t, err)
+
+	target, err := symlinks.Resolve(filepath.Join(containerRoot, hostRoot, "libfoo.so"))
+	require.NoError(t, err)
+	require.Equal(t, "../libfoo.so.1", target)
+
+	require.DirExists(t, filepath.Join(hostRoot, "libfoo.so"))
 }
 
 type dirOrLink struct {

internal/lookup/symlinks/symlink.go

@@ -33,3 +33,18 @@ func Resolve(filename string) (string, error) {
 
 	return os.Readlink(filename)
 }
+
+// ForceCreate creates a specified symlink.
+// If a file (or empty directory) exists at the path it is removed.
+func ForceCreate(target string, link string) error {
+	_, err := os.Lstat(link)
+	if err != nil && !os.IsNotExist(err) {
+		return fmt.Errorf("failed to get file info: %w", err)
+	}
+	if !os.IsNotExist(err) {
+		if err := os.Remove(link); err != nil {
+			return fmt.Errorf("failed to remove existing file: %w", err)
+		}
+	}
+	return os.Symlink(target, link)
+}