Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: temporarily remount read-only mounts before cleaning container fs #183

Merged
merged 22 commits into from
May 14, 2024
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions envbuilder.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ import (
"syscall"
"time"

ebutil "github.com/coder/envbuilder/internal/util"

dcontext "github.com/distribution/distribution/v3/context"
"github.com/kballard/go-shellquote"
"github.com/mattn/go-isatty"
Expand Down Expand Up @@ -394,6 +396,14 @@ func Run(ctx context.Context, options Options) error {
})
}

// temp move of all ro mounts
tempRemountDest := filepath.Join("/", MagicDir)
dannykopping marked this conversation as resolved.
Show resolved Hide resolved
johnstcn marked this conversation as resolved.
Show resolved Hide resolved
ignorePrefixes := []string{tempRemountDest, "/proc", "/sys"}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should these be configurable?
Will we only ever support UNIX-based builds?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kaniko is currently Linux-only to the best of my knowledge.

remount, err := ebutil.TempRemount(options.Logger, tempRemountDest, ignorePrefixes...)
if err != nil {
return fmt.Errorf("temp remount: %w", err)
}
johnstcn marked this conversation as resolved.
Show resolved Hide resolved

skippedRebuild := false
build := func() (v1.Image, error) {
_, err := options.Filesystem.Stat(MagicFile)
Expand Down Expand Up @@ -540,6 +550,11 @@ func Run(ctx context.Context, options Options) error {
closeAfterBuild()
}

if err := remount(); err != nil {
// Don't fail at this point as it may be useful to be able to inspect the build.
options.Logger(codersdk.LogLevelError, "recreate mountpoint: %w", err)
johnstcn marked this conversation as resolved.
Show resolved Hide resolved
}

// Create the magic file to indicate that this build
// has already been ran before!
file, err := options.Filesystem.Create(MagicFile)
Expand Down
1 change: 1 addition & 0 deletions go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -262,6 +262,7 @@ require (
go.opentelemetry.io/otel/trace v1.19.0 // indirect
go.opentelemetry.io/proto/otlp v1.0.0 // indirect
go.uber.org/atomic v1.11.0 // indirect
go.uber.org/mock v0.4.0 // indirect
go4.org/intern v0.0.0-20230525184215-6c62f75575cb // indirect
go4.org/mem v0.0.0-20220726221520-4f986261bf13 // indirect
go4.org/netipx v0.0.0-20230728180743-ad4cb58a6516 // indirect
Expand Down
2 changes: 2 additions & 0 deletions go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -910,6 +910,8 @@ go.uber.org/atomic v1.11.0 h1:ZvwS0R+56ePWxUNi+Atn9dWONBPp/AUETXlHW0DxSjE=
go.uber.org/atomic v1.11.0/go.mod h1:LUxbIzbOniOlMKjJjyPfpl4v+PKK2cNJn91OQbhoJI0=
go.uber.org/goleak v1.3.0 h1:2K3zAYmnTNqV73imy9J1T3WC+gmCePx2hEGkimedGto=
go.uber.org/goleak v1.3.0/go.mod h1:CoHD4mav9JJNrW/WLlf7HGZPjdw8EucARQHekz1X6bE=
go.uber.org/mock v0.4.0 h1:VcM4ZOtdbR4f6VXfiOpwpVJDL6lCReaZ6mw31wqh7KU=
go.uber.org/mock v0.4.0/go.mod h1:a6FSlNadKUHUa9IP5Vyt1zh4fC7uAwxMutEAscFbkZc=
go.uber.org/multierr v1.6.0/go.mod h1:cdWPpRnG4AhwMwsgIHip0KRBQjJy5kYEpYjJxpXp9iU=
go.uber.org/zap v1.17.0/go.mod h1:MXVU+bhUf/A7Xi2HNOnopQOrmycQ5Ih87HtOu4q5SSo=
go4.org/intern v0.0.0-20211027215823-ae77deb06f29/go.mod h1:cS2ma+47FKrLPdXFpr7CuxiTW3eyJbWew4qx0qtQWDA=
Expand Down
98 changes: 98 additions & 0 deletions internal/util/mock_mounter_test.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

123 changes: 123 additions & 0 deletions internal/util/remount.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,123 @@
package ebutil
johnstcn marked this conversation as resolved.
Show resolved Hide resolved

import (
"fmt"
"os"
"path/filepath"
"strings"
"syscall"

"github.com/coder/coder/v2/codersdk"
"github.com/prometheus/procfs"
)

// TempRemount iterates through all read-only mounted filesystems, bind-mounts them at dest,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an interesting question, should we or should we not limit this to read-only mounted filesystems? If we think about how a docker container is run with -v /path:/path, the contents of the docker container can never populate the mounted /path.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah this would actually make sense given the below behaviour:

$ echo 'important stuff' >> /opt/important-stuff/tps-reports.txt
$ ls -l /opt/important-stuff/tps-reports.txt 
-rw-r--r-- 1 cian 16 May 13 21:12 /opt/important-stuff/tps-reports.txt
$ docker run -it --rm \
    -v /opt/important-stuff:/opt/important-stuff \
    -e GIT_URL=https://github.com/coder/envbuilder-starter-devcontainer \
    -e INIT_SCRIPT=bash \
    envbuilder:latest
[...]
devcontainer # ls -l /opt/important-stuff/
total 0
devcontainer # exit
$ cat /opt/important-stuff/tps-reports.txt
important stuff

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might merge this as-is and investigate rw mounts in a follow up, if that's OK?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

johnstcn marked this conversation as resolved.
Show resolved Hide resolved
// and unmounts them from their original source. All mount points underneath ignorePrefixes
// will not be touched.
johnstcn marked this conversation as resolved.
Show resolved Hide resolved
//
// It is the responsibility of the caller to call the returned function
// to restore the original mount points. If an error is encountered while attempting to perform
// the operation, calling the returned remount function will make a best-effort attempt to
// restore the original state.
func TempRemount(logf func(codersdk.LogLevel, string, ...any), dest string, ignorePrefixes ...string) (remount func() error, err error,
johnstcn marked this conversation as resolved.
Show resolved Hide resolved
) {
return tempRemount(&realMounter{}, logf, dest, ignorePrefixes...)
dannykopping marked this conversation as resolved.
Show resolved Hide resolved
}

func tempRemount(m mounter, logf func(codersdk.LogLevel, string, ...any), dest string, ignorePrefixes ...string) (remount func() error, err error) {
mountInfos, err := m.GetMounts()
if err != nil {
return func() error { return nil }, fmt.Errorf("get mounts: %w", err)
}

// temp move of all ro mounts
mounts := map[string]string{}
// closer to attempt to restore original mount points
remount = func() error {
for src, tgt := range mounts {
err := m.MkdirAll(src, 0o750)
if err != nil {
return fmt.Errorf("recreate original mountpoint %s: %w", src, err)
}

err = m.Mount(tgt, src, "bind", syscall.MS_BIND, "")
if err != nil {
return fmt.Errorf("bind mount %s => %s: %w", tgt, src, err)
}

err = m.Unmount(tgt, 0)
if err != nil {
return fmt.Errorf("unmount temporary dest %s: %w", tgt, err)
}
}
return nil
}

outer:
for _, mountInfo := range mountInfos {
if _, ok := mountInfo.Options["ro"]; !ok {
logf(codersdk.LogLevelTrace, "skip rw mount %s", mountInfo.MountPoint)
continue
}

for _, prefix := range ignorePrefixes {
if strings.HasPrefix(mountInfo.MountPoint, prefix) {
logf(codersdk.LogLevelTrace, "skip mount %s under ignored prefix %s", mountInfo.MountPoint, prefix)
continue outer
}
}

src := mountInfo.MountPoint
tgt := filepath.Join("/", dest, src)
err := m.MkdirAll(tgt, 0o750)
if err != nil {
return remount, fmt.Errorf("create temp mountpoint %s: %w", dest, err)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we return nil for remount here? And should we perhaps restore the mounts on error?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer to leave the responsibility to the caller. Is there a strong reason to do it ourselves?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd say the main reason is meeting expectations, i.e. returning an err and a non-nil value that is actionable can lead to some bad assumptions, especially if the behavior changes at some point.

It's OK to leave as-is, though.

}

err = m.Mount(src, tgt, "bind", syscall.MS_BIND, "")
if err != nil {
return remount, fmt.Errorf("bind mount %s => %s: %s", src, dest, err.Error())
}
err = m.Unmount(src, 0)
if err != nil {
return remount, fmt.Errorf("temp unmount src %s: %s", src, err.Error())
}
johnstcn marked this conversation as resolved.
Show resolved Hide resolved

mounts[src] = tgt
}

return remount, nil
}

// mounter is an interface to system-level calls used by TempRemount.
type mounter interface {
// GetMounts wraps procfs.GetMounts
GetMounts() ([]*procfs.MountInfo, error)
// MkdirAll wraps os.MkdirAll
MkdirAll(string, os.FileMode) error
// Mount wraps syscall.Mount
Mount(string, string, string, uintptr, string) error
// Unmount wraps syscall.Unmount
Unmount(string, int) error
}

// realMounter implements mounter and actually does the thing.
type realMounter struct{}

var _ mounter = &realMounter{}

func (m *realMounter) Mount(src string, dest string, fstype string, flags uintptr, data string) error {
return syscall.Mount(src, dest, fstype, flags, data)
}

func (m *realMounter) Unmount(tgt string, flags int) error {
return syscall.Unmount(tgt, flags)
}

func (m *realMounter) GetMounts() ([]*procfs.MountInfo, error) {
return procfs.GetMounts()
}

func (m *realMounter) MkdirAll(path string, perm os.FileMode) error {
return os.MkdirAll(path, perm)
}
Loading