Repository: coder/envbuilder
Pull request #249 (2024-06-25 14:50:05)

fix(remount): ensure mountpoint is a file for files
Fixes errors like:

```
error: temp remount: temp remount: bind mount /usr/bin/nvidia-smi => /.envbuilder/mnt/usr/bin/nvidia-smi: not a directory
```

Follow up to #183

maxbrunet: Apparently that's not enough, maybe files need extra flags or something, I'll try to investigate:

```
error: temp remount: temp remount: bind mount /usr/bin/nvidia-smi => /.envbuilder/mnt/usr/bin/nvidia-smi: no such file or directory
```
johnstcn: > Apparently that's not enough, maybe files need extra flags or something, I'll try to investigate:
> 
> ```
> error: temp remount: temp remount: bind mount /usr/bin/nvidia-smi => /.envbuilder/mnt/usr/bin/nvidia-smi: no such file or directory
> ```

What are you trying to accomplish exactly? Can you provide steps to reproduce?
maxbrunet: Fixed. The file mountpoint must exist

I'm experimenting with GPUs, the NVIDIA container runtime mounts a lot of things. Now I'm back to the known issue with Kaniko

<details>
<summary>pod.yaml</summary>

```yaml
apiVersion: v1
kind: Pod
metadata:
  name: envbuilder 
spec:
  containers:
  - name: envbuilder
    image: ghcr.io/coder/envbuilder-preview
    env:
    - name: FALLBACK_IMAGE
      value: busybox
    - name: INIT_SCRIPT
      value: sh 
    resources:
      limits:
        nvidia.com/gpu: "1"
    securityContext:
      privileged: true
```

</details>

These paths need privileges to be remounted, I am thinking `ignorePrefixes` should be an extension of `ignorePaths` since there are ignored, it should not be needed to remount them. I can try that in a separate PR

https://github.com/coder/envbuilder/blob/b06565690cd7a94a06b2da0858dbbf0542fd71f8/envbuilder.go#L436
maxbrunet: Thank you @johnstcn, this looks good to me. There is more to consider for NVIDIA and I am not sure `ignorePaths` will solve everything, but I'll get into the specifics on the related issue, I don't want to sidetrack this PR too much :)

Patch:
diff --git a/envbuilder.go b/envbuilder.go
index 9b5b3cc..fdbfde9 100644
--- a/envbuilder.go
+++ b/envbuilder.go
@@ -394,12 +394,15 @@ func Run(ctx context.Context, options Options) error {
 	// https://github.com/GoogleContainerTools/kaniko/blob/63be4990ca5a60bdf06ddc4d10aa4eca0c0bc714/cmd/executor/cmd/root.go#L136
 	ignorePaths := append([]string{
 		MagicDir,
-		options.LayerCacheDir,
 		options.WorkspaceFolder,
 		// See: https://github.com/coder/envbuilder/issues/37
 		"/etc/resolv.conf",
 	}, options.IgnorePaths...)
 
+	if options.LayerCacheDir != "" {
+		ignorePaths = append(ignorePaths, options.LayerCacheDir)
+	}
+
 	for _, ignorePath := range ignorePaths {
 		util.AddToDefaultIgnoreList(util.IgnoreListEntry{
 			Path:            ignorePath,
@@ -433,7 +436,9 @@ ENTRYPOINT [%q]`, exePath, exePath, exePath)
 
 	// temp move of all ro mounts
 	tempRemountDest := filepath.Join("/", MagicDir, "mnt")
-	ignorePrefixes := []string{tempRemountDest, "/proc", "/sys"}
+	// ignorePrefixes is a superset of ignorePaths that we pass to kaniko's
+	// IgnoreList.
+	ignorePrefixes := append([]string{"/proc", "/sys"}, ignorePaths...)
 	restoreMounts, err := ebutil.TempRemount(options.Logger, tempRemountDest, ignorePrefixes...)
 	defer func() { // restoreMounts should never be nil
 		if err := restoreMounts(); err != nil {
diff --git a/internal/ebutil/remount.go b/internal/ebutil/remount.go
index 77da0e6..f4a2b41 100644
--- a/internal/ebutil/remount.go
+++ b/internal/ebutil/remount.go
@@ -88,9 +88,26 @@ outer:
 }
 
 func remount(m mounter, src, dest string) error {
-	if err := m.MkdirAll(dest, 0o750); err != nil {
+	stat, err := m.Stat(src)
+	if err != nil {
+		return fmt.Errorf("stat %s: %w", src, err)
+	}
+	var destDir string
+	if stat.IsDir() {
+		destDir = dest
+	} else {
+		destDir = filepath.Dir(dest)
+	}
+	if err := m.MkdirAll(destDir, 0o750); err != nil {
 		return fmt.Errorf("ensure path: %w", err)
 	}
+	if !stat.IsDir() {
+		f, err := m.OpenFile(dest, os.O_CREATE, 0o640)
+		if err != nil {
+			return fmt.Errorf("ensure file path: %w", err)
+		}
+		defer f.Close()
+	}
 	if err := m.Mount(src, dest, "bind", syscall.MS_BIND, ""); err != nil {
 		return fmt.Errorf("bind mount %s => %s: %w", src, dest, err)
 	}
@@ -104,8 +121,12 @@ func remount(m mounter, src, dest string) error {
 type mounter interface {
 	// GetMounts wraps procfs.GetMounts
 	GetMounts() ([]*procfs.MountInfo, error)
+	// Stat wraps os.Stat
+	Stat(string) (os.FileInfo, error)
 	// MkdirAll wraps os.MkdirAll
 	MkdirAll(string, os.FileMode) error
+	// OpenFile wraps os.OpenFile
+	OpenFile(string, int, os.FileMode) (*os.File, error)
 	// Mount wraps syscall.Mount
 	Mount(string, string, string, uintptr, string) error
 	// Unmount wraps syscall.Unmount
@@ -132,3 +153,11 @@ func (m *realMounter) GetMounts() ([]*procfs.MountInfo, error) {
 func (m *realMounter) MkdirAll(path string, perm os.FileMode) error {
 	return os.MkdirAll(path, perm)
 }
+
+func (m *realMounter) OpenFile(name string, flag int, perm os.FileMode) (*os.File, error) {
+	return os.OpenFile(name, flag, perm)
+}
+
+func (m *realMounter) Stat(path string) (os.FileInfo, error) {
+	return os.Stat(path)
+}


Test patch:
diff --git a/integration/integration_test.go b/integration/integration_test.go
index 02b0906..decc5bd 100644
--- a/integration/integration_test.go
+++ b/integration/integration_test.go
@@ -41,6 +41,7 @@ import (
 	"github.com/google/go-containerregistry/pkg/registry"
 	"github.com/google/go-containerregistry/pkg/v1/remote"
 	"github.com/google/go-containerregistry/pkg/v1/remote/transport"
+	"github.com/google/uuid"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
@@ -402,19 +403,37 @@ func TestBuildIgnoreVarRunSecrets(t *testing.T) {
 		},
 	})
 	dir := t.TempDir()
-	err := os.WriteFile(filepath.Join(dir, "secret"), []byte("test"), 0o644)
+	secretVal := uuid.NewString()
+	err := os.WriteFile(filepath.Join(dir, "secret"), []byte(secretVal), 0o644)
 	require.NoError(t, err)
-	ctr, err := runEnvbuilder(t, options{
-		env: []string{
-			envbuilderEnv("GIT_URL", srv.URL),
-			envbuilderEnv("DOCKERFILE_PATH", "Dockerfile"),
-		},
-		binds: []string{fmt.Sprintf("%s:/var/run/secrets", dir)},
+
+	t.Run("ReadWrite", func(t *testing.T) {
+		ctr, err := runEnvbuilder(t, options{
+			env: []string{
+				envbuilderEnv("GIT_URL", srv.URL),
+				envbuilderEnv("DOCKERFILE_PATH", "Dockerfile"),
+			},
+			binds: []string{fmt.Sprintf("%s:/var/run/secrets:rw", dir)},
+		})
+		require.NoError(t, err)
+
+		output := execContainer(t, ctr, "cat /var/run/secrets/secret")
+		require.Equal(t, secretVal, strings.TrimSpace(output))
 	})
-	require.NoError(t, err)
 
-	output := execContainer(t, ctr, "echo hello")
-	require.Equal(t, "hello", strings.TrimSpace(output))
+	t.Run("ReadOnly", func(t *testing.T) {
+		ctr, err := runEnvbuilder(t, options{
+			env: []string{
+				envbuilderEnv("GIT_URL", srv.URL),
+				envbuilderEnv("DOCKERFILE_PATH", "Dockerfile"),
+			},
+			binds: []string{fmt.Sprintf("%s:/var/run/secrets:ro", dir)},
+		})
+		require.NoError(t, err)
+
+		output := execContainer(t, ctr, "cat /var/run/secrets/secret")
+		require.Equal(t, secretVal, strings.TrimSpace(output))
+	})
 }
 
 func TestBuildWithSetupScript(t *testing.T) {
diff --git a/internal/ebutil/mock_mounter_test.go b/internal/ebutil/mock_mounter_test.go
index 3386d56..7445376 100644
--- a/internal/ebutil/mock_mounter_test.go
+++ b/internal/ebutil/mock_mounter_test.go
@@ -85,6 +85,36 @@ func (mr *MockmounterMockRecorder) Mount(arg0, arg1, arg2, arg3, arg4 any) *gomo
 	return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Mount", reflect.TypeOf((*Mockmounter)(nil).Mount), arg0, arg1, arg2, arg3, arg4)
 }
 
+// OpenFile mocks base method.
+func (m *Mockmounter) OpenFile(arg0 string, arg1 int, arg2 os.FileMode) (*os.File, error) {
+	m.ctrl.T.Helper()
+	ret := m.ctrl.Call(m, "OpenFile", arg0, arg1, arg2)
+	ret0, _ := ret[0].(*os.File)
+	ret1, _ := ret[1].(error)
+	return ret0, ret1
+}
+
+// OpenFile indicates an expected call of OpenFile.
+func (mr *MockmounterMockRecorder) OpenFile(arg0, arg1, arg2 any) *gomock.Call {
+	mr.mock.ctrl.T.Helper()
+	return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "OpenFile", reflect.TypeOf((*Mockmounter)(nil).OpenFile), arg0, arg1, arg2)
+}
+
+// Stat mocks base method.
+func (m *Mockmounter) Stat(arg0 string) (os.FileInfo, error) {
+	m.ctrl.T.Helper()
+	ret := m.ctrl.Call(m, "Stat", arg0)
+	ret0, _ := ret[0].(os.FileInfo)
+	ret1, _ := ret[1].(error)
+	return ret0, ret1
+}
+
+// Stat indicates an expected call of Stat.
+func (mr *MockmounterMockRecorder) Stat(arg0 any) *gomock.Call {
+	mr.mock.ctrl.T.Helper()
+	return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Stat", reflect.TypeOf((*Mockmounter)(nil).Stat), arg0)
+}
+
 // Unmount mocks base method.
 func (m *Mockmounter) Unmount(arg0 string, arg1 int) error {
 	m.ctrl.T.Helper()
diff --git a/internal/ebutil/remount_internal_test.go b/internal/ebutil/remount_internal_test.go
index 736c50b..4103617 100644
--- a/internal/ebutil/remount_internal_test.go
+++ b/internal/ebutil/remount_internal_test.go
@@ -5,6 +5,7 @@ import (
 	"strings"
 	"syscall"
 	"testing"
+	time "time"
 
 	"github.com/coder/envbuilder/internal/notcodersdk"
 	"github.com/stretchr/testify/assert"
@@ -25,9 +26,11 @@ func Test_tempRemount(t *testing.T) {
 		mounts := fakeMounts("/home", "/var/lib/modules:ro", "/proc", "/sys")
 
 		mm.EXPECT().GetMounts().Return(mounts, nil)
+		mm.EXPECT().Stat("/var/lib/modules").Return(&fakeFileInfo{isDir: true}, nil)
 		mm.EXPECT().MkdirAll("/.test/var/lib/modules", os.FileMode(0o750)).Times(1).Return(nil)
 		mm.EXPECT().Mount("/var/lib/modules", "/.test/var/lib/modules", "bind", uintptr(syscall.MS_BIND), "").Times(1).Return(nil)
 		mm.EXPECT().Unmount("/var/lib/modules", 0).Times(1).Return(nil)
+		mm.EXPECT().Stat("/.test/var/lib/modules").Return(&fakeFileInfo{isDir: true}, nil)
 		mm.EXPECT().MkdirAll("/var/lib/modules", os.FileMode(0o750)).Times(1).Return(nil)
 		mm.EXPECT().Mount("/.test/var/lib/modules", "/var/lib/modules", "bind", uintptr(syscall.MS_BIND), "").Times(1).Return(nil)
 		mm.EXPECT().Unmount("/.test/var/lib/modules", 0).Times(1).Return(nil)
@@ -40,6 +43,33 @@ func Test_tempRemount(t *testing.T) {
 		_ = remount()
 	})
 
+	t.Run("OKFile", func(t *testing.T) {
+		t.Parallel()
+
+		ctrl := gomock.NewController(t)
+		mm := NewMockmounter(ctrl)
+		mounts := fakeMounts("/home", "/usr/bin/utility:ro", "/proc", "/sys")
+
+		mm.EXPECT().GetMounts().Return(mounts, nil)
+		mm.EXPECT().Stat("/usr/bin/utility").Return(&fakeFileInfo{isDir: false}, nil)
+		mm.EXPECT().MkdirAll("/.test/usr/bin", os.FileMode(0o750)).Times(1).Return(nil)
+		mm.EXPECT().OpenFile("/.test/usr/bin/utility", os.O_CREATE, os.FileMode(0o640)).Times(1).Return(new(os.File), nil)
+		mm.EXPECT().Mount("/usr/bin/utility", "/.test/usr/bin/utility", "bind", uintptr(syscall.MS_BIND), "").Times(1).Return(nil)
+		mm.EXPECT().Unmount("/usr/bin/utility", 0).Times(1).Return(nil)
+		mm.EXPECT().Stat("/.test/usr/bin/utility").Return(&fakeFileInfo{isDir: false}, nil)
+		mm.EXPECT().MkdirAll("/usr/bin", os.FileMode(0o750)).Times(1).Return(nil)
+		mm.EXPECT().OpenFile("/usr/bin/utility", os.O_CREATE, os.FileMode(0o640)).Times(1).Return(new(os.File), nil)
+		mm.EXPECT().Mount("/.test/usr/bin/utility", "/usr/bin/utility", "bind", uintptr(syscall.MS_BIND), "").Times(1).Return(nil)
+		mm.EXPECT().Unmount("/.test/usr/bin/utility", 0).Times(1).Return(nil)
+
+		remount, err := tempRemount(mm, fakeLog(t), "/.test")
+		require.NoError(t, err)
+		err = remount()
+		require.NoError(t, err)
+		// sync.Once should handle multiple remount calls
+		_ = remount()
+	})
+
 	t.Run("IgnorePrefixes", func(t *testing.T) {
 		t.Parallel()
 
@@ -75,6 +105,7 @@ func Test_tempRemount(t *testing.T) {
 		mounts := fakeMounts("/home", "/var/lib/modules:ro", "/proc", "/sys")
 
 		mm.EXPECT().GetMounts().Return(mounts, nil)
+		mm.EXPECT().Stat("/var/lib/modules").Return(&fakeFileInfo{isDir: true}, nil)
 		mm.EXPECT().MkdirAll("/.test/var/lib/modules", os.FileMode(0o750)).Times(1).Return(assert.AnError)
 
 		remount, err := tempRemount(mm, fakeLog(t), "/.test")
@@ -91,6 +122,7 @@ func Test_tempRemount(t *testing.T) {
 		mounts := fakeMounts("/home", "/var/lib/modules:ro", "/proc", "/sys")
 
 		mm.EXPECT().GetMounts().Return(mounts, nil)
+		mm.EXPECT().Stat("/var/lib/modules").Return(&fakeFileInfo{isDir: true}, nil)
 		mm.EXPECT().MkdirAll("/.test/var/lib/modules", os.FileMode(0o750)).Times(1).Return(nil)
 		mm.EXPECT().Mount("/var/lib/modules", "/.test/var/lib/modules", "bind", uintptr(syscall.MS_BIND), "").Times(1).Return(assert.AnError)
 
@@ -108,6 +140,7 @@ func Test_tempRemount(t *testing.T) {
 		mounts := fakeMounts("/home", "/var/lib/modules:ro", "/proc", "/sys")
 
 		mm.EXPECT().GetMounts().Return(mounts, nil)
+		mm.EXPECT().Stat("/var/lib/modules").Return(&fakeFileInfo{isDir: true}, nil)
 		mm.EXPECT().MkdirAll("/.test/var/lib/modules", os.FileMode(0o750)).Times(1).Return(nil)
 		mm.EXPECT().Mount("/var/lib/modules", "/.test/var/lib/modules", "bind", uintptr(syscall.MS_BIND), "").Times(1).Return(nil)
 		mm.EXPECT().Unmount("/var/lib/modules", 0).Times(1).Return(assert.AnError)
@@ -126,9 +159,11 @@ func Test_tempRemount(t *testing.T) {
 		mounts := fakeMounts("/home", "/var/lib/modules:ro", "/proc", "/sys")
 
 		mm.EXPECT().GetMounts().Return(mounts, nil)
+		mm.EXPECT().Stat("/var/lib/modules").Return(&fakeFileInfo{isDir: true}, nil)
 		mm.EXPECT().MkdirAll("/.test/var/lib/modules", os.FileMode(0o750)).Times(1).Return(nil)
 		mm.EXPECT().Mount("/var/lib/modules", "/.test/var/lib/modules", "bind", uintptr(syscall.MS_BIND), "").Times(1).Return(nil)
 		mm.EXPECT().Unmount("/var/lib/modules", 0).Times(1).Return(nil)
+		mm.EXPECT().Stat("/.test/var/lib/modules").Return(&fakeFileInfo{isDir: true}, nil)
 		mm.EXPECT().MkdirAll("/var/lib/modules", os.FileMode(0o750)).Times(1).Return(assert.AnError)
 
 		remount, err := tempRemount(mm, fakeLog(t), "/.test")
@@ -145,9 +180,11 @@ func Test_tempRemount(t *testing.T) {
 		mounts := fakeMounts("/home", "/var/lib/modules:ro", "/proc", "/sys")
 
 		mm.EXPECT().GetMounts().Return(mounts, nil)
+		mm.EXPECT().Stat("/var/lib/modules").Return(&fakeFileInfo{isDir: true}, nil)
 		mm.EXPECT().MkdirAll("/.test/var/lib/modules", os.FileMode(0o750)).Times(1).Return(nil)
 		mm.EXPECT().Mount("/var/lib/modules", "/.test/var/lib/modules", "bind", uintptr(syscall.MS_BIND), "").Times(1).Return(nil)
 		mm.EXPECT().Unmount("/var/lib/modules", 0).Times(1).Return(nil)
+		mm.EXPECT().Stat("/.test/var/lib/modules").Return(&fakeFileInfo{isDir: true}, nil)
 		mm.EXPECT().MkdirAll("/var/lib/modules", os.FileMode(0o750)).Times(1).Return(nil)
 		mm.EXPECT().Mount("/.test/var/lib/modules", "/var/lib/modules", "bind", uintptr(syscall.MS_BIND), "").Times(1).Return(assert.AnError)
 
@@ -165,9 +202,11 @@ func Test_tempRemount(t *testing.T) {
 		mounts := fakeMounts("/home", "/var/lib/modules:ro", "/proc", "/sys")
 
 		mm.EXPECT().GetMounts().Return(mounts, nil)
+		mm.EXPECT().Stat("/var/lib/modules").Return(&fakeFileInfo{isDir: true}, nil)
 		mm.EXPECT().MkdirAll("/.test/var/lib/modules", os.FileMode(0o750)).Times(1).Return(nil)
 		mm.EXPECT().Mount("/var/lib/modules", "/.test/var/lib/modules", "bind", uintptr(syscall.MS_BIND), "").Times(1).Return(nil)
 		mm.EXPECT().Unmount("/var/lib/modules", 0).Times(1).Return(nil)
+		mm.EXPECT().Stat("/.test/var/lib/modules").Return(&fakeFileInfo{isDir: true}, nil)
 		mm.EXPECT().MkdirAll("/var/lib/modules", os.FileMode(0o750)).Times(1).Return(nil)
 		mm.EXPECT().Mount("/.test/var/lib/modules", "/var/lib/modules", "bind", uintptr(syscall.MS_BIND), "").Times(1).Return(nil)
 		mm.EXPECT().Unmount("/.test/var/lib/modules", 0).Times(1).Return(assert.AnError)
@@ -200,3 +239,16 @@ func fakeLog(t *testing.T) func(notcodersdk.LogLevel, string, ...any) {
 		t.Logf(s, a...)
 	}
 }
+
+type fakeFileInfo struct {
+	isDir bool
+}
+
+func (fi *fakeFileInfo) Name() string       { return "" }
+func (fi *fakeFileInfo) Size() int64        { return 0 }
+func (fi *fakeFileInfo) Mode() os.FileMode  { return 0 }
+func (fi *fakeFileInfo) ModTime() time.Time { return time.Time{} }
+func (fi *fakeFileInfo) IsDir() bool        { return fi.isDir }
+func (fi *fakeFileInfo) Sys() any           { return nil }
+
+var _ os.FileInfo = &fakeFileInfo{}