From 324096c9799980c6a85d9e7eb55d6cc718bb6440 Mon Sep 17 00:00:00 2001 From: Evan Lezar Date: Mon, 4 Nov 2024 15:22:41 -0800 Subject: [PATCH] Force symlink creation in create-symlink hook This change updates the create-symlink hook to be equivalent to ln -f -s target link This ensures that links are updated even if they exist in the container being run. Signed-off-by: Evan Lezar --- .../create-symlinks/create-symlinks.go | 27 +++--- .../create-symlinks/create-symlinks_test.go | 97 +++++++++++-------- internal/lookup/symlinks/symlink.go | 15 +++ 3 files changed, 87 insertions(+), 52 deletions(-) diff --git a/cmd/nvidia-cdi-hook/create-symlinks/create-symlinks.go b/cmd/nvidia-cdi-hook/create-symlinks/create-symlinks.go index 5ab5ade2..28b0626e 100644 --- a/cmd/nvidia-cdi-hook/create-symlinks/create-symlinks.go +++ b/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 } diff --git a/cmd/nvidia-cdi-hook/create-symlinks/create-symlinks_test.go b/cmd/nvidia-cdi-hook/create-symlinks/create-symlinks_test.go index eb673382..f2bdefcf 100644 --- a/cmd/nvidia-cdi-hook/create-symlinks/create-symlinks_test.go +++ b/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,43 +192,55 @@ func TestCreateLinkAbsolutePath(t *testing.T) { } func TestCreateLinkAlreadyExists(t *testing.T) { - tmpDir := t.TempDir() - hostRoot := filepath.Join(tmpDir, "/host-root/") - containerRoot := filepath.Join(tmpDir, "/container-root") + 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"}, + }, + } - require.NoError(t, makeFs(hostRoot)) - require.NoError(t, makeFs(containerRoot, dirOrLink{path: "/lib/libfoo.so", target: "libfoo.so.1"})) + 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, tc.containerContents...)) - // nvidia-cdi-hook create-symlinks --link libfoo.so.1::/lib/libfoo.so - err := getTestCommand().createLink(containerRoot, "libfoo.so.1", "/lib/libfoo.so") - require.NoError(t, err) - target, err := symlinks.Resolve(filepath.Join(containerRoot, "lib/libfoo.so")) - require.NoError(t, err) - require.Equal(t, "libfoo.so.1", target) -} + // nvidia-cdi-hook create-symlinks --link libfoo.so.1::/lib/libfoo.so + err := getTestCommand().createLink(containerRoot, "libfoo.so.1", "/lib/libfoo.so") + require.NoError(t, err) + 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 { diff --git a/internal/lookup/symlinks/symlink.go b/internal/lookup/symlinks/symlink.go index 1929aaa9..f9151a2f 100644 --- a/internal/lookup/symlinks/symlink.go +++ b/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) +}