Skip to content

Commit 8c5c50c

Browse files
maxbrunetjohnstcn
maxbrunet
and
johnstcn
authored
fix(remount): ensure mountpoint is a file for files (#249)
Ensures that read-only bind mounts of single files are created properly. Co-authored-by: Cian Johnston <cian@coder.com>
1 parent b065656 commit 8c5c50c

File tree

5 files changed

+148
-13
lines changed

5 files changed

+148
-13
lines changed

‎envbuilder.go‎

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -394,12 +394,15 @@ func Run(ctx context.Context, options Options) error {
394394
// https://github.com/GoogleContainerTools/kaniko/blob/63be4990ca5a60bdf06ddc4d10aa4eca0c0bc714/cmd/executor/cmd/root.go#L136
395395
ignorePaths := append([]string{
396396
MagicDir,
397-
options.LayerCacheDir,
398397
options.WorkspaceFolder,
399398
// See: https://github.com/coder/envbuilder/issues/37
400399
"/etc/resolv.conf",
401400
}, options.IgnorePaths...)
402401

402+
if options.LayerCacheDir != "" {
403+
ignorePaths = append(ignorePaths, options.LayerCacheDir)
404+
}
405+
403406
for _, ignorePath := range ignorePaths {
404407
util.AddToDefaultIgnoreList(util.IgnoreListEntry{
405408
Path: ignorePath,
@@ -433,7 +436,9 @@ ENTRYPOINT [%q]`, exePath, exePath, exePath)
433436

434437
// temp move of all ro mounts
435438
tempRemountDest := filepath.Join("/", MagicDir, "mnt")
436-
ignorePrefixes := []string{tempRemountDest, "/proc", "/sys"}
439+
// ignorePrefixes is a superset of ignorePaths that we pass to kaniko's
440+
// IgnoreList.
441+
ignorePrefixes := append([]string{"/proc", "/sys"}, ignorePaths...)
437442
restoreMounts, err := ebutil.TempRemount(options.Logger, tempRemountDest, ignorePrefixes...)
438443
defer func() { // restoreMounts should never be nil
439444
if err := restoreMounts(); err != nil {

‎integration/integration_test.go‎

Lines changed: 29 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,7 @@ import (
4141
"github.com/google/go-containerregistry/pkg/registry"
4242
"github.com/google/go-containerregistry/pkg/v1/remote"
4343
"github.com/google/go-containerregistry/pkg/v1/remote/transport"
44+
"github.com/google/uuid"
4445
"github.com/stretchr/testify/assert"
4546
"github.com/stretchr/testify/require"
4647
)
@@ -402,19 +403,37 @@ func TestBuildIgnoreVarRunSecrets(t *testing.T) {
402403
},
403404
})
404405
dir := t.TempDir()
405-
err := os.WriteFile(filepath.Join(dir, "secret"), []byte("test"), 0o644)
406+
secretVal := uuid.NewString()
407+
err := os.WriteFile(filepath.Join(dir, "secret"), []byte(secretVal), 0o644)
406408
require.NoError(t, err)
407-
ctr, err := runEnvbuilder(t, options{
408-
env: []string{
409-
envbuilderEnv("GIT_URL", srv.URL),
410-
envbuilderEnv("DOCKERFILE_PATH", "Dockerfile"),
411-
},
412-
binds: []string{fmt.Sprintf("%s:/var/run/secrets", dir)},
409+
410+
t.Run("ReadWrite", func(t *testing.T) {
411+
ctr, err := runEnvbuilder(t, options{
412+
env: []string{
413+
envbuilderEnv("GIT_URL", srv.URL),
414+
envbuilderEnv("DOCKERFILE_PATH", "Dockerfile"),
415+
},
416+
binds: []string{fmt.Sprintf("%s:/var/run/secrets:rw", dir)},
417+
})
418+
require.NoError(t, err)
419+
420+
output := execContainer(t, ctr, "cat /var/run/secrets/secret")
421+
require.Equal(t, secretVal, strings.TrimSpace(output))
413422
})
414-
require.NoError(t, err)
415423

416-
output := execContainer(t, ctr, "echo hello")
417-
require.Equal(t, "hello", strings.TrimSpace(output))
424+
t.Run("ReadOnly", func(t *testing.T) {
425+
ctr, err := runEnvbuilder(t, options{
426+
env: []string{
427+
envbuilderEnv("GIT_URL", srv.URL),
428+
envbuilderEnv("DOCKERFILE_PATH", "Dockerfile"),
429+
},
430+
binds: []string{fmt.Sprintf("%s:/var/run/secrets:ro", dir)},
431+
})
432+
require.NoError(t, err)
433+
434+
output := execContainer(t, ctr, "cat /var/run/secrets/secret")
435+
require.Equal(t, secretVal, strings.TrimSpace(output))
436+
})
418437
}
419438

420439
func TestBuildWithSetupScript(t *testing.T) {

‎internal/ebutil/mock_mounter_test.go‎

Lines changed: 30 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎internal/ebutil/remount.go‎

Lines changed: 30 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -88,9 +88,26 @@ outer:
8888
}
8989

9090
func remount(m mounter, src, dest string) error {
91-
if err := m.MkdirAll(dest, 0o750); err != nil {
91+
stat, err := m.Stat(src)
92+
if err != nil {
93+
return fmt.Errorf("stat %s: %w", src, err)
94+
}
95+
var destDir string
96+
if stat.IsDir() {
97+
destDir = dest
98+
} else {
99+
destDir = filepath.Dir(dest)
100+
}
101+
if err := m.MkdirAll(destDir, 0o750); err != nil {
92102
return fmt.Errorf("ensure path: %w", err)
93103
}
104+
if !stat.IsDir() {
105+
f, err := m.OpenFile(dest, os.O_CREATE, 0o640)
106+
if err != nil {
107+
return fmt.Errorf("ensure file path: %w", err)
108+
}
109+
defer f.Close()
110+
}
94111
if err := m.Mount(src, dest, "bind", syscall.MS_BIND, ""); err != nil {
95112
return fmt.Errorf("bind mount %s => %s: %w", src, dest, err)
96113
}
@@ -104,8 +121,12 @@ func remount(m mounter, src, dest string) error {
104121
type mounter interface {
105122
// GetMounts wraps procfs.GetMounts
106123
GetMounts() ([]*procfs.MountInfo, error)
124+
// Stat wraps os.Stat
125+
Stat(string) (os.FileInfo, error)
107126
// MkdirAll wraps os.MkdirAll
108127
MkdirAll(string, os.FileMode) error
128+
// OpenFile wraps os.OpenFile
129+
OpenFile(string, int, os.FileMode) (*os.File, error)
109130
// Mount wraps syscall.Mount
110131
Mount(string, string, string, uintptr, string) error
111132
// Unmount wraps syscall.Unmount
@@ -132,3 +153,11 @@ func (m *realMounter) GetMounts() ([]*procfs.MountInfo, error) {
132153
func (m *realMounter) MkdirAll(path string, perm os.FileMode) error {
133154
return os.MkdirAll(path, perm)
134155
}
156+
157+
func (m *realMounter) OpenFile(name string, flag int, perm os.FileMode) (*os.File, error) {
158+
return os.OpenFile(name, flag, perm)
159+
}
160+
161+
func (m *realMounter) Stat(path string) (os.FileInfo, error) {
162+
return os.Stat(path)
163+
}

‎internal/ebutil/remount_internal_test.go‎

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import (
55
"strings"
66
"syscall"
77
"testing"
8+
time "time"
89

910
"github.com/coder/envbuilder/internal/notcodersdk"
1011
"github.com/stretchr/testify/assert"
@@ -25,9 +26,11 @@ func Test_tempRemount(t *testing.T) {
2526
mounts := fakeMounts("/home", "/var/lib/modules:ro", "/proc", "/sys")
2627

2728
mm.EXPECT().GetMounts().Return(mounts, nil)
29+
mm.EXPECT().Stat("/var/lib/modules").Return(&fakeFileInfo{isDir: true}, nil)
2830
mm.EXPECT().MkdirAll("/.test/var/lib/modules", os.FileMode(0o750)).Times(1).Return(nil)
2931
mm.EXPECT().Mount("/var/lib/modules", "/.test/var/lib/modules", "bind", uintptr(syscall.MS_BIND), "").Times(1).Return(nil)
3032
mm.EXPECT().Unmount("/var/lib/modules", 0).Times(1).Return(nil)
33+
mm.EXPECT().Stat("/.test/var/lib/modules").Return(&fakeFileInfo{isDir: true}, nil)
3134
mm.EXPECT().MkdirAll("/var/lib/modules", os.FileMode(0o750)).Times(1).Return(nil)
3235
mm.EXPECT().Mount("/.test/var/lib/modules", "/var/lib/modules", "bind", uintptr(syscall.MS_BIND), "").Times(1).Return(nil)
3336
mm.EXPECT().Unmount("/.test/var/lib/modules", 0).Times(1).Return(nil)
@@ -40,6 +43,33 @@ func Test_tempRemount(t *testing.T) {
4043
_ = remount()
4144
})
4245

46+
t.Run("OKFile", func(t *testing.T) {
47+
t.Parallel()
48+
49+
ctrl := gomock.NewController(t)
50+
mm := NewMockmounter(ctrl)
51+
mounts := fakeMounts("/home", "/usr/bin/utility:ro", "/proc", "/sys")
52+
53+
mm.EXPECT().GetMounts().Return(mounts, nil)
54+
mm.EXPECT().Stat("/usr/bin/utility").Return(&fakeFileInfo{isDir: false}, nil)
55+
mm.EXPECT().MkdirAll("/.test/usr/bin", os.FileMode(0o750)).Times(1).Return(nil)
56+
mm.EXPECT().OpenFile("/.test/usr/bin/utility", os.O_CREATE, os.FileMode(0o640)).Times(1).Return(new(os.File), nil)
57+
mm.EXPECT().Mount("/usr/bin/utility", "/.test/usr/bin/utility", "bind", uintptr(syscall.MS_BIND), "").Times(1).Return(nil)
58+
mm.EXPECT().Unmount("/usr/bin/utility", 0).Times(1).Return(nil)
59+
mm.EXPECT().Stat("/.test/usr/bin/utility").Return(&fakeFileInfo{isDir: false}, nil)
60+
mm.EXPECT().MkdirAll("/usr/bin", os.FileMode(0o750)).Times(1).Return(nil)
61+
mm.EXPECT().OpenFile("/usr/bin/utility", os.O_CREATE, os.FileMode(0o640)).Times(1).Return(new(os.File), nil)
62+
mm.EXPECT().Mount("/.test/usr/bin/utility", "/usr/bin/utility", "bind", uintptr(syscall.MS_BIND), "").Times(1).Return(nil)
63+
mm.EXPECT().Unmount("/.test/usr/bin/utility", 0).Times(1).Return(nil)
64+
65+
remount, err := tempRemount(mm, fakeLog(t), "/.test")
66+
require.NoError(t, err)
67+
err = remount()
68+
require.NoError(t, err)
69+
// sync.Once should handle multiple remount calls
70+
_ = remount()
71+
})
72+
4373
t.Run("IgnorePrefixes", func(t *testing.T) {
4474
t.Parallel()
4575

@@ -75,6 +105,7 @@ func Test_tempRemount(t *testing.T) {
75105
mounts := fakeMounts("/home", "/var/lib/modules:ro", "/proc", "/sys")
76106

77107
mm.EXPECT().GetMounts().Return(mounts, nil)
108+
mm.EXPECT().Stat("/var/lib/modules").Return(&fakeFileInfo{isDir: true}, nil)
78109
mm.EXPECT().MkdirAll("/.test/var/lib/modules", os.FileMode(0o750)).Times(1).Return(assert.AnError)
79110

80111
remount, err := tempRemount(mm, fakeLog(t), "/.test")
@@ -91,6 +122,7 @@ func Test_tempRemount(t *testing.T) {
91122
mounts := fakeMounts("/home", "/var/lib/modules:ro", "/proc", "/sys")
92123

93124
mm.EXPECT().GetMounts().Return(mounts, nil)
125+
mm.EXPECT().Stat("/var/lib/modules").Return(&fakeFileInfo{isDir: true}, nil)
94126
mm.EXPECT().MkdirAll("/.test/var/lib/modules", os.FileMode(0o750)).Times(1).Return(nil)
95127
mm.EXPECT().Mount("/var/lib/modules", "/.test/var/lib/modules", "bind", uintptr(syscall.MS_BIND), "").Times(1).Return(assert.AnError)
96128

@@ -108,6 +140,7 @@ func Test_tempRemount(t *testing.T) {
108140
mounts := fakeMounts("/home", "/var/lib/modules:ro", "/proc", "/sys")
109141

110142
mm.EXPECT().GetMounts().Return(mounts, nil)
143+
mm.EXPECT().Stat("/var/lib/modules").Return(&fakeFileInfo{isDir: true}, nil)
111144
mm.EXPECT().MkdirAll("/.test/var/lib/modules", os.FileMode(0o750)).Times(1).Return(nil)
112145
mm.EXPECT().Mount("/var/lib/modules", "/.test/var/lib/modules", "bind", uintptr(syscall.MS_BIND), "").Times(1).Return(nil)
113146
mm.EXPECT().Unmount("/var/lib/modules", 0).Times(1).Return(assert.AnError)
@@ -126,9 +159,11 @@ func Test_tempRemount(t *testing.T) {
126159
mounts := fakeMounts("/home", "/var/lib/modules:ro", "/proc", "/sys")
127160

128161
mm.EXPECT().GetMounts().Return(mounts, nil)
162+
mm.EXPECT().Stat("/var/lib/modules").Return(&fakeFileInfo{isDir: true}, nil)
129163
mm.EXPECT().MkdirAll("/.test/var/lib/modules", os.FileMode(0o750)).Times(1).Return(nil)
130164
mm.EXPECT().Mount("/var/lib/modules", "/.test/var/lib/modules", "bind", uintptr(syscall.MS_BIND), "").Times(1).Return(nil)
131165
mm.EXPECT().Unmount("/var/lib/modules", 0).Times(1).Return(nil)
166+
mm.EXPECT().Stat("/.test/var/lib/modules").Return(&fakeFileInfo{isDir: true}, nil)
132167
mm.EXPECT().MkdirAll("/var/lib/modules", os.FileMode(0o750)).Times(1).Return(assert.AnError)
133168

134169
remount, err := tempRemount(mm, fakeLog(t), "/.test")
@@ -145,9 +180,11 @@ func Test_tempRemount(t *testing.T) {
145180
mounts := fakeMounts("/home", "/var/lib/modules:ro", "/proc", "/sys")
146181

147182
mm.EXPECT().GetMounts().Return(mounts, nil)
183+
mm.EXPECT().Stat("/var/lib/modules").Return(&fakeFileInfo{isDir: true}, nil)
148184
mm.EXPECT().MkdirAll("/.test/var/lib/modules", os.FileMode(0o750)).Times(1).Return(nil)
149185
mm.EXPECT().Mount("/var/lib/modules", "/.test/var/lib/modules", "bind", uintptr(syscall.MS_BIND), "").Times(1).Return(nil)
150186
mm.EXPECT().Unmount("/var/lib/modules", 0).Times(1).Return(nil)
187+
mm.EXPECT().Stat("/.test/var/lib/modules").Return(&fakeFileInfo{isDir: true}, nil)
151188
mm.EXPECT().MkdirAll("/var/lib/modules", os.FileMode(0o750)).Times(1).Return(nil)
152189
mm.EXPECT().Mount("/.test/var/lib/modules", "/var/lib/modules", "bind", uintptr(syscall.MS_BIND), "").Times(1).Return(assert.AnError)
153190

@@ -165,9 +202,11 @@ func Test_tempRemount(t *testing.T) {
165202
mounts := fakeMounts("/home", "/var/lib/modules:ro", "/proc", "/sys")
166203

167204
mm.EXPECT().GetMounts().Return(mounts, nil)
205+
mm.EXPECT().Stat("/var/lib/modules").Return(&fakeFileInfo{isDir: true}, nil)
168206
mm.EXPECT().MkdirAll("/.test/var/lib/modules", os.FileMode(0o750)).Times(1).Return(nil)
169207
mm.EXPECT().Mount("/var/lib/modules", "/.test/var/lib/modules", "bind", uintptr(syscall.MS_BIND), "").Times(1).Return(nil)
170208
mm.EXPECT().Unmount("/var/lib/modules", 0).Times(1).Return(nil)
209+
mm.EXPECT().Stat("/.test/var/lib/modules").Return(&fakeFileInfo{isDir: true}, nil)
171210
mm.EXPECT().MkdirAll("/var/lib/modules", os.FileMode(0o750)).Times(1).Return(nil)
172211
mm.EXPECT().Mount("/.test/var/lib/modules", "/var/lib/modules", "bind", uintptr(syscall.MS_BIND), "").Times(1).Return(nil)
173212
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) {
200239
t.Logf(s, a...)
201240
}
202241
}
242+
243+
type fakeFileInfo struct {
244+
isDir bool
245+
}
246+
247+
func (fi *fakeFileInfo) Name() string { return "" }
248+
func (fi *fakeFileInfo) Size() int64 { return 0 }
249+
func (fi *fakeFileInfo) Mode() os.FileMode { return 0 }
250+
func (fi *fakeFileInfo) ModTime() time.Time { return time.Time{} }
251+
func (fi *fakeFileInfo) IsDir() bool { return fi.isDir }
252+
func (fi *fakeFileInfo) Sys() any { return nil }
253+
254+
var _ os.FileInfo = &fakeFileInfo{}

0 commit comments

Comments
 (0)