From fafa94969cc8a048e10edcf79d1b0c0dbb0dcc1d Mon Sep 17 00:00:00 2001 From: Jan Losinski Date: Thu, 17 Aug 2023 00:03:53 +0200 Subject: [PATCH 01/20] fix: allow envbuilder on sysbox container runtime This fixes #50 by temporary bind-mounting all readonly mounts within the MagicDir to keep them out of the way for kaniko. After kaniko finished it's build, the original mountpoints are restored at their original location. Signed-off-by: Jan Losinski --- envbuilder.go | 78 +++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 78 insertions(+) diff --git a/envbuilder.go b/envbuilder.go index f8a88580..0aa07b5d 100644 --- a/envbuilder.go +++ b/envbuilder.go @@ -9,6 +9,7 @@ import ( "encoding/json" "errors" "fmt" + "github.com/prometheus/procfs" "io" "net" "net/http" @@ -194,6 +195,11 @@ type Options struct { Filesystem billy.Filesystem } +type TempRemount struct { + Source string + Target string +} + // DockerConfig represents the Docker configuration file. type DockerConfig configfile.ConfigFile @@ -516,6 +522,60 @@ func Run(ctx context.Context, options Options) error { }) } + // temp move of all ro mounts + var tempRemounts []TempRemount + + mountInfos, err := procfs.GetMounts() + if err != nil { + logf(codersdk.LogLevelError, "Failed to read mountinfo: %s", err) + } else { + prefixes := []string{filepath.Join("/", MagicDir), "/proc", "/sys"} + for _, mountInfo := range mountInfos { + logf(codersdk.LogLevelTrace, "Found mountpoint %s", mountInfo.MountPoint) + if _, ok := mountInfo.Options["ro"]; ok { + logf(codersdk.LogLevelTrace, "Found ro mountpoint %s ", mountInfo.MountPoint) + + relevantMountpoint := true + for _, prefix := range prefixes { + if strings.HasPrefix(mountInfo.MountPoint, prefix) { + logf(codersdk.LogLevelTrace, "Mountpoint %s is NOT relevant (prefix: %s)", mountInfo.MountPoint, prefix) + relevantMountpoint = false + break + } + } + if relevantMountpoint { + logf(codersdk.LogLevelTrace, "Mountpoint %s is relevant", mountInfo.MountPoint) + + remount := TempRemount{ + Source: mountInfo.MountPoint, + Target: filepath.Join("/", MagicDir, mountInfo.MountPoint), + } + + logf(codersdk.LogLevelTrace, "Remount mountpoint %s to %s", mountInfo.MountPoint, remount.Target) + + err := os.MkdirAll(remount.Target, 0750) + if err == nil { + err := syscall.Mount(remount.Source, remount.Target, "bind", syscall.MS_BIND, "") + if err == nil { + err := syscall.Unmount(remount.Source, 0) + if err != nil { + logf(codersdk.LogLevelError, "Could not unmount %s", remount.Source) + } + tempRemounts = append(tempRemounts, remount) + + } else { + logf(codersdk.LogLevelError, "Could not bindmount %s to %s", remount.Source, remount.Target) + } + } else { + logf(codersdk.LogLevelError, "Could not create temp mountpoint %s for %s", remount.Target, remount.Source) + } + + } + + } + } + } + build := func() (v1.Image, error) { _, err := options.Filesystem.Stat(MagicFile) if err == nil && options.SkipRebuild { @@ -652,6 +712,24 @@ func Run(ctx context.Context, options Options) error { closeAfterBuild() } + for _, remount := range tempRemounts { + logf(codersdk.LogLevelTrace, "Cleanup mountpoint %s", remount.Target) + err := os.MkdirAll(remount.Source, 0750) + if err == nil { + err := syscall.Mount(remount.Target, remount.Source, "bind", syscall.MS_BIND, "") + if err == nil { + err := syscall.Unmount(remount.Target, 0) + if err != nil { + logf(codersdk.LogLevelError, "Could not unmount %s", remount.Target) + } + } else { + logf(codersdk.LogLevelError, "Could not bindmount %s to %s", remount.Target, remount.Source) + } + } else { + logf(codersdk.LogLevelError, "Could not create mountpoint %s for %s", remount.Source, remount.Target) + } + } + // Create the magic file to indicate that this build // has already been ran before! file, err := options.Filesystem.Create(MagicFile) From 39b422248dce3b42563f519d30fd9e0442c3bfc7 Mon Sep 17 00:00:00 2001 From: Colin Adler Date: Mon, 21 Aug 2023 20:04:13 +0000 Subject: [PATCH 02/20] formatting --- envbuilder.go | 125 +++++++++++++++++++++++++------------------------- 1 file changed, 62 insertions(+), 63 deletions(-) diff --git a/envbuilder.go b/envbuilder.go index 0aa07b5d..d49094e2 100644 --- a/envbuilder.go +++ b/envbuilder.go @@ -9,7 +9,6 @@ import ( "encoding/json" "errors" "fmt" - "github.com/prometheus/procfs" "io" "net" "net/http" @@ -24,18 +23,13 @@ import ( "syscall" "time" - dcontext "github.com/distribution/distribution/v3/context" - "github.com/kballard/go-shellquote" - "github.com/mattn/go-isatty" - "github.com/GoogleContainerTools/kaniko/pkg/config" "github.com/GoogleContainerTools/kaniko/pkg/creds" "github.com/GoogleContainerTools/kaniko/pkg/executor" "github.com/GoogleContainerTools/kaniko/pkg/util" - "github.com/coder/coder/codersdk" - "github.com/coder/envbuilder/devcontainer" "github.com/containerd/containerd/platforms" "github.com/distribution/distribution/v3/configuration" + dcontext "github.com/distribution/distribution/v3/context" "github.com/distribution/distribution/v3/registry/handlers" _ "github.com/distribution/distribution/v3/registry/storage/driver/filesystem" "github.com/docker/cli/cli/config/configfile" @@ -45,9 +39,15 @@ import ( githttp "github.com/go-git/go-git/v5/plumbing/transport/http" v1 "github.com/google/go-containerregistry/pkg/v1" "github.com/google/go-containerregistry/pkg/v1/remote" + "github.com/kballard/go-shellquote" + "github.com/mattn/go-isatty" + "github.com/prometheus/procfs" "github.com/sirupsen/logrus" "github.com/tailscale/hujson" "golang.org/x/xerrors" + + "github.com/coder/coder/codersdk" + "github.com/coder/envbuilder/devcontainer" ) const ( @@ -195,11 +195,6 @@ type Options struct { Filesystem billy.Filesystem } -type TempRemount struct { - Source string - Target string -} - // DockerConfig represents the Docker configuration file. type DockerConfig configfile.ConfigFile @@ -523,7 +518,7 @@ func Run(ctx context.Context, options Options) error { } // temp move of all ro mounts - var tempRemounts []TempRemount + tempRemounts := map[string]string{} mountInfos, err := procfs.GetMounts() if err != nil { @@ -532,47 +527,48 @@ func Run(ctx context.Context, options Options) error { prefixes := []string{filepath.Join("/", MagicDir), "/proc", "/sys"} for _, mountInfo := range mountInfos { logf(codersdk.LogLevelTrace, "Found mountpoint %s", mountInfo.MountPoint) - if _, ok := mountInfo.Options["ro"]; ok { - logf(codersdk.LogLevelTrace, "Found ro mountpoint %s ", mountInfo.MountPoint) - - relevantMountpoint := true - for _, prefix := range prefixes { - if strings.HasPrefix(mountInfo.MountPoint, prefix) { - logf(codersdk.LogLevelTrace, "Mountpoint %s is NOT relevant (prefix: %s)", mountInfo.MountPoint, prefix) - relevantMountpoint = false - break - } + if _, ok := mountInfo.Options["ro"]; !ok { + continue + } + logf(codersdk.LogLevelTrace, "Found ro mountpoint %s ", mountInfo.MountPoint) + + relevantMountpoint := true + for _, prefix := range prefixes { + if strings.HasPrefix(mountInfo.MountPoint, prefix) { + logf(codersdk.LogLevelTrace, "Mountpoint %s is NOT relevant (prefix: %s)", mountInfo.MountPoint, prefix) + relevantMountpoint = false + break } - if relevantMountpoint { - logf(codersdk.LogLevelTrace, "Mountpoint %s is relevant", mountInfo.MountPoint) + } + if !relevantMountpoint { + continue + } - remount := TempRemount{ - Source: mountInfo.MountPoint, - Target: filepath.Join("/", MagicDir, mountInfo.MountPoint), - } + src := mountInfo.MountPoint + tgt := filepath.Join("/", MagicDir, mountInfo.MountPoint) - logf(codersdk.LogLevelTrace, "Remount mountpoint %s to %s", mountInfo.MountPoint, remount.Target) - - err := os.MkdirAll(remount.Target, 0750) - if err == nil { - err := syscall.Mount(remount.Source, remount.Target, "bind", syscall.MS_BIND, "") - if err == nil { - err := syscall.Unmount(remount.Source, 0) - if err != nil { - logf(codersdk.LogLevelError, "Could not unmount %s", remount.Source) - } - tempRemounts = append(tempRemounts, remount) - - } else { - logf(codersdk.LogLevelError, "Could not bindmount %s to %s", remount.Source, remount.Target) - } - } else { - logf(codersdk.LogLevelError, "Could not create temp mountpoint %s for %s", remount.Target, remount.Source) - } + logf(codersdk.LogLevelTrace, "Mountpoint %s is relevant", src) - } + logf(codersdk.LogLevelTrace, "Remount mountpoint %s to %s", src, tgt) + + err := os.MkdirAll(tgt, 0750) + if err != nil { + logf(codersdk.LogLevelError, "Could not create temp mountpoint %s for %s: %s", tgt, src, err.Error()) + continue + } + err = syscall.Mount(src, tgt, "bind", syscall.MS_BIND, "") + if err != nil { + logf(codersdk.LogLevelError, "Could not bindmount %s to %s: %s", src, tgt, err.Error()) + continue } + err = syscall.Unmount(src, 0) + if err != nil { + logf(codersdk.LogLevelError, "Could not unmount %s: %s", src, err.Error()) + continue + } + + tempRemounts[src] = tgt } } @@ -712,21 +708,24 @@ func Run(ctx context.Context, options Options) error { closeAfterBuild() } - for _, remount := range tempRemounts { - logf(codersdk.LogLevelTrace, "Cleanup mountpoint %s", remount.Target) - err := os.MkdirAll(remount.Source, 0750) - if err == nil { - err := syscall.Mount(remount.Target, remount.Source, "bind", syscall.MS_BIND, "") - if err == nil { - err := syscall.Unmount(remount.Target, 0) - if err != nil { - logf(codersdk.LogLevelError, "Could not unmount %s", remount.Target) - } - } else { - logf(codersdk.LogLevelError, "Could not bindmount %s to %s", remount.Target, remount.Source) - } - } else { - logf(codersdk.LogLevelError, "Could not create mountpoint %s for %s", remount.Source, remount.Target) + for src, tgt := range tempRemounts { + logf(codersdk.LogLevelTrace, "Cleanup mountpoint %s", tgt) + err := os.MkdirAll(src, 0750) + if err != nil { + logf(codersdk.LogLevelError, "Could not create mountpoint %s for %s: %w", src, tgt, err.Error()) + continue + } + + err = syscall.Mount(tgt, src, "bind", syscall.MS_BIND, "") + if err != nil { + logf(codersdk.LogLevelError, "Could not bindmount %s to %s: %s", tgt, src, err.Error()) + continue + } + + err = syscall.Unmount(tgt, 0) + if err != nil { + logf(codersdk.LogLevelError, "Could not unmount %s: %s", tgt, err.Error()) + continue } } From 3ee8a9e07ab0e8627ff1eb30791791c5ac9a3ad9 Mon Sep 17 00:00:00 2001 From: Colin Adler Date: Mon, 21 Aug 2023 20:06:24 +0000 Subject: [PATCH 03/20] fixup! formatting --- envbuilder.go | 80 +++++++++++++++++++++++++-------------------------- 1 file changed, 40 insertions(+), 40 deletions(-) diff --git a/envbuilder.go b/envbuilder.go index d49094e2..edd7af8c 100644 --- a/envbuilder.go +++ b/envbuilder.go @@ -523,53 +523,53 @@ func Run(ctx context.Context, options Options) error { mountInfos, err := procfs.GetMounts() if err != nil { logf(codersdk.LogLevelError, "Failed to read mountinfo: %s", err) - } else { - prefixes := []string{filepath.Join("/", MagicDir), "/proc", "/sys"} - for _, mountInfo := range mountInfos { - logf(codersdk.LogLevelTrace, "Found mountpoint %s", mountInfo.MountPoint) - if _, ok := mountInfo.Options["ro"]; !ok { - continue - } - logf(codersdk.LogLevelTrace, "Found ro mountpoint %s ", mountInfo.MountPoint) - - relevantMountpoint := true - for _, prefix := range prefixes { - if strings.HasPrefix(mountInfo.MountPoint, prefix) { - logf(codersdk.LogLevelTrace, "Mountpoint %s is NOT relevant (prefix: %s)", mountInfo.MountPoint, prefix) - relevantMountpoint = false - break - } - } - if !relevantMountpoint { - continue - } + } - src := mountInfo.MountPoint - tgt := filepath.Join("/", MagicDir, mountInfo.MountPoint) + prefixes := []string{filepath.Join("/", MagicDir), "/proc", "/sys"} + for _, mountInfo := range mountInfos { + logf(codersdk.LogLevelTrace, "Found mountpoint %s", mountInfo.MountPoint) + if _, ok := mountInfo.Options["ro"]; !ok { + continue + } + logf(codersdk.LogLevelTrace, "Found ro mountpoint %s ", mountInfo.MountPoint) + + relevantMountpoint := true + for _, prefix := range prefixes { + if strings.HasPrefix(mountInfo.MountPoint, prefix) { + logf(codersdk.LogLevelTrace, "Mountpoint %s is NOT relevant (prefix: %s)", mountInfo.MountPoint, prefix) + relevantMountpoint = false + break + } + } + if !relevantMountpoint { + continue + } - logf(codersdk.LogLevelTrace, "Mountpoint %s is relevant", src) + src := mountInfo.MountPoint + tgt := filepath.Join("/", MagicDir, mountInfo.MountPoint) - logf(codersdk.LogLevelTrace, "Remount mountpoint %s to %s", src, tgt) + logf(codersdk.LogLevelTrace, "Mountpoint %s is relevant", src) - err := os.MkdirAll(tgt, 0750) - if err != nil { - logf(codersdk.LogLevelError, "Could not create temp mountpoint %s for %s: %s", tgt, src, err.Error()) - continue - } + logf(codersdk.LogLevelTrace, "Remount mountpoint %s to %s", src, tgt) - err = syscall.Mount(src, tgt, "bind", syscall.MS_BIND, "") - if err != nil { - logf(codersdk.LogLevelError, "Could not bindmount %s to %s: %s", src, tgt, err.Error()) - continue - } - err = syscall.Unmount(src, 0) - if err != nil { - logf(codersdk.LogLevelError, "Could not unmount %s: %s", src, err.Error()) - continue - } + err := os.MkdirAll(tgt, 0750) + if err != nil { + logf(codersdk.LogLevelError, "Could not create temp mountpoint %s for %s: %s", tgt, src, err.Error()) + continue + } - tempRemounts[src] = tgt + err = syscall.Mount(src, tgt, "bind", syscall.MS_BIND, "") + if err != nil { + logf(codersdk.LogLevelError, "Could not bindmount %s to %s: %s", src, tgt, err.Error()) + continue } + err = syscall.Unmount(src, 0) + if err != nil { + logf(codersdk.LogLevelError, "Could not unmount %s: %s", src, err.Error()) + continue + } + + tempRemounts[src] = tgt } build := func() (v1.Image, error) { From f0086662c65f34fb67b5174590aa2786bea5f78b Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Fri, 10 May 2024 17:13:18 +0100 Subject: [PATCH 04/20] fixup! Merge remote-tracking branch 'origin/main' into cj/janl/sysbox-issues --- envbuilder.go | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/envbuilder.go b/envbuilder.go index 6af84258..a492b123 100644 --- a/envbuilder.go +++ b/envbuilder.go @@ -402,21 +402,21 @@ func Run(ctx context.Context, options Options) error { mountInfos, err := procfs.GetMounts() if err != nil { - logf(codersdk.LogLevelError, "Failed to read mountinfo: %s", err) + options.Logger(codersdk.LogLevelError, "Failed to read mountinfo: %s", err) } prefixes := []string{filepath.Join("/", MagicDir), "/proc", "/sys"} for _, mountInfo := range mountInfos { - logf(codersdk.LogLevelTrace, "Found mountpoint %s", mountInfo.MountPoint) + options.Logger(codersdk.LogLevelTrace, "Found mountpoint %s", mountInfo.MountPoint) if _, ok := mountInfo.Options["ro"]; !ok { continue } - logf(codersdk.LogLevelTrace, "Found ro mountpoint %s ", mountInfo.MountPoint) + options.Logger(codersdk.LogLevelTrace, "Found ro mountpoint %s ", mountInfo.MountPoint) relevantMountpoint := true for _, prefix := range prefixes { if strings.HasPrefix(mountInfo.MountPoint, prefix) { - logf(codersdk.LogLevelTrace, "Mountpoint %s is NOT relevant (prefix: %s)", mountInfo.MountPoint, prefix) + options.Logger(codersdk.LogLevelTrace, "Mountpoint %s is NOT relevant (prefix: %s)", mountInfo.MountPoint, prefix) relevantMountpoint = false break } @@ -428,24 +428,24 @@ func Run(ctx context.Context, options Options) error { src := mountInfo.MountPoint tgt := filepath.Join("/", MagicDir, mountInfo.MountPoint) - logf(codersdk.LogLevelTrace, "Mountpoint %s is relevant", src) + options.Logger(codersdk.LogLevelTrace, "Mountpoint %s is relevant", src) - logf(codersdk.LogLevelTrace, "Remount mountpoint %s to %s", src, tgt) + options.Logger(codersdk.LogLevelTrace, "Remount mountpoint %s to %s", src, tgt) err := os.MkdirAll(tgt, 0750) if err != nil { - logf(codersdk.LogLevelError, "Could not create temp mountpoint %s for %s: %s", tgt, src, err.Error()) + options.Logger(codersdk.LogLevelError, "Could not create temp mountpoint %s for %s: %s", tgt, src, err.Error()) continue } err = syscall.Mount(src, tgt, "bind", syscall.MS_BIND, "") if err != nil { - logf(codersdk.LogLevelError, "Could not bindmount %s to %s: %s", src, tgt, err.Error()) + options.Logger(codersdk.LogLevelError, "Could not bindmount %s to %s: %s", src, tgt, err.Error()) continue } err = syscall.Unmount(src, 0) if err != nil { - logf(codersdk.LogLevelError, "Could not unmount %s: %s", src, err.Error()) + options.Logger(codersdk.LogLevelError, "Could not unmount %s: %s", src, err.Error()) continue } @@ -599,22 +599,22 @@ func Run(ctx context.Context, options Options) error { } for src, tgt := range tempRemounts { - logf(codersdk.LogLevelTrace, "Cleanup mountpoint %s", tgt) + options.Logger(codersdk.LogLevelTrace, "Cleanup mountpoint %s", tgt) err := os.MkdirAll(src, 0750) if err != nil { - logf(codersdk.LogLevelError, "Could not create mountpoint %s for %s: %w", src, tgt, err.Error()) + options.Logger(codersdk.LogLevelError, "Could not create mountpoint %s for %s: %w", src, tgt, err.Error()) continue } err = syscall.Mount(tgt, src, "bind", syscall.MS_BIND, "") if err != nil { - logf(codersdk.LogLevelError, "Could not bindmount %s to %s: %s", tgt, src, err.Error()) + options.Logger(codersdk.LogLevelError, "Could not bindmount %s to %s: %s", tgt, src, err.Error()) continue } err = syscall.Unmount(tgt, 0) if err != nil { - logf(codersdk.LogLevelError, "Could not unmount %s: %s", tgt, err.Error()) + options.Logger(codersdk.LogLevelError, "Could not unmount %s: %s", tgt, err.Error()) continue } } From 9c5b0056293bd80863374a8d5882994c44340245 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Fri, 10 May 2024 20:29:08 +0100 Subject: [PATCH 05/20] fixup! Merge remote-tracking branch 'origin/main' into cj/janl/sysbox-issues --- envbuilder.go | 3 --- 1 file changed, 3 deletions(-) diff --git a/envbuilder.go b/envbuilder.go index a492b123..7eac3a1a 100644 --- a/envbuilder.go +++ b/envbuilder.go @@ -50,9 +50,6 @@ import ( "github.com/sirupsen/logrus" "github.com/tailscale/hujson" "golang.org/x/xerrors" - - "github.com/coder/coder/codersdk" - "github.com/coder/envbuilder/devcontainer" ) const ( From 4c1060c2e484b1cacc48ebea75df3a5a0fd0b1f3 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Fri, 10 May 2024 21:17:08 +0100 Subject: [PATCH 06/20] reduce diff size --- envbuilder.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/envbuilder.go b/envbuilder.go index 7eac3a1a..6eecffb3 100644 --- a/envbuilder.go +++ b/envbuilder.go @@ -26,6 +26,10 @@ import ( "syscall" "time" + dcontext "github.com/distribution/distribution/v3/context" + "github.com/kballard/go-shellquote" + "github.com/mattn/go-isatty" + "github.com/GoogleContainerTools/kaniko/pkg/config" "github.com/GoogleContainerTools/kaniko/pkg/creds" "github.com/GoogleContainerTools/kaniko/pkg/executor" @@ -34,7 +38,6 @@ import ( "github.com/coder/envbuilder/devcontainer" "github.com/containerd/containerd/platforms" "github.com/distribution/distribution/v3/configuration" - dcontext "github.com/distribution/distribution/v3/context" "github.com/distribution/distribution/v3/registry/handlers" _ "github.com/distribution/distribution/v3/registry/storage/driver/filesystem" "github.com/docker/cli/cli/config/configfile" @@ -44,8 +47,6 @@ import ( "github.com/go-git/go-git/v5/plumbing/transport" v1 "github.com/google/go-containerregistry/pkg/v1" "github.com/google/go-containerregistry/pkg/v1/remote" - "github.com/kballard/go-shellquote" - "github.com/mattn/go-isatty" "github.com/prometheus/procfs" "github.com/sirupsen/logrus" "github.com/tailscale/hujson" From 7f223a0dfc70f68bbf2b6fa1993783f5dc529a1f Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Mon, 13 May 2024 14:24:13 +0100 Subject: [PATCH 07/20] refactor temp remount logic into util package, add unit tests --- envbuilder.go | 80 ++-------- go.mod | 1 + go.sum | 2 + internal/util/mock_mounter_test.go | 98 ++++++++++++ internal/util/remount.go | 120 +++++++++++++++ internal/util/remount_internal_test.go | 199 +++++++++++++++++++++++++ 6 files changed, 429 insertions(+), 71 deletions(-) create mode 100644 internal/util/mock_mounter_test.go create mode 100644 internal/util/remount.go create mode 100644 internal/util/remount_internal_test.go diff --git a/envbuilder.go b/envbuilder.go index 6eecffb3..46c6ae40 100644 --- a/envbuilder.go +++ b/envbuilder.go @@ -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" @@ -47,7 +49,6 @@ import ( "github.com/go-git/go-git/v5/plumbing/transport" v1 "github.com/google/go-containerregistry/pkg/v1" "github.com/google/go-containerregistry/pkg/v1/remote" - "github.com/prometheus/procfs" "github.com/sirupsen/logrus" "github.com/tailscale/hujson" "golang.org/x/xerrors" @@ -396,58 +397,11 @@ func Run(ctx context.Context, options Options) error { } // temp move of all ro mounts - tempRemounts := map[string]string{} - - mountInfos, err := procfs.GetMounts() + tempRemountDest := filepath.Join("/", MagicDir) + ignorePrefixes := []string{tempRemountDest, "/proc", "/sys"} + remount, err := ebutil.TempRemount(options.Logger, tempRemountDest, ignorePrefixes...) if err != nil { - options.Logger(codersdk.LogLevelError, "Failed to read mountinfo: %s", err) - } - - prefixes := []string{filepath.Join("/", MagicDir), "/proc", "/sys"} - for _, mountInfo := range mountInfos { - options.Logger(codersdk.LogLevelTrace, "Found mountpoint %s", mountInfo.MountPoint) - if _, ok := mountInfo.Options["ro"]; !ok { - continue - } - options.Logger(codersdk.LogLevelTrace, "Found ro mountpoint %s ", mountInfo.MountPoint) - - relevantMountpoint := true - for _, prefix := range prefixes { - if strings.HasPrefix(mountInfo.MountPoint, prefix) { - options.Logger(codersdk.LogLevelTrace, "Mountpoint %s is NOT relevant (prefix: %s)", mountInfo.MountPoint, prefix) - relevantMountpoint = false - break - } - } - if !relevantMountpoint { - continue - } - - src := mountInfo.MountPoint - tgt := filepath.Join("/", MagicDir, mountInfo.MountPoint) - - options.Logger(codersdk.LogLevelTrace, "Mountpoint %s is relevant", src) - - options.Logger(codersdk.LogLevelTrace, "Remount mountpoint %s to %s", src, tgt) - - err := os.MkdirAll(tgt, 0750) - if err != nil { - options.Logger(codersdk.LogLevelError, "Could not create temp mountpoint %s for %s: %s", tgt, src, err.Error()) - continue - } - - err = syscall.Mount(src, tgt, "bind", syscall.MS_BIND, "") - if err != nil { - options.Logger(codersdk.LogLevelError, "Could not bindmount %s to %s: %s", src, tgt, err.Error()) - continue - } - err = syscall.Unmount(src, 0) - if err != nil { - options.Logger(codersdk.LogLevelError, "Could not unmount %s: %s", src, err.Error()) - continue - } - - tempRemounts[src] = tgt + return fmt.Errorf("temp remount: %w", err) } skippedRebuild := false @@ -596,25 +550,9 @@ func Run(ctx context.Context, options Options) error { closeAfterBuild() } - for src, tgt := range tempRemounts { - options.Logger(codersdk.LogLevelTrace, "Cleanup mountpoint %s", tgt) - err := os.MkdirAll(src, 0750) - if err != nil { - options.Logger(codersdk.LogLevelError, "Could not create mountpoint %s for %s: %w", src, tgt, err.Error()) - continue - } - - err = syscall.Mount(tgt, src, "bind", syscall.MS_BIND, "") - if err != nil { - options.Logger(codersdk.LogLevelError, "Could not bindmount %s to %s: %s", tgt, src, err.Error()) - continue - } - - err = syscall.Unmount(tgt, 0) - if err != nil { - options.Logger(codersdk.LogLevelError, "Could not unmount %s: %s", tgt, err.Error()) - continue - } + 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) } // Create the magic file to indicate that this build diff --git a/go.mod b/go.mod index 4a61ec6c..5c99d40f 100644 --- a/go.mod +++ b/go.mod @@ -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 diff --git a/go.sum b/go.sum index 9fecac8b..d957a9b4 100644 --- a/go.sum +++ b/go.sum @@ -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= diff --git a/internal/util/mock_mounter_test.go b/internal/util/mock_mounter_test.go new file mode 100644 index 00000000..19a20380 --- /dev/null +++ b/internal/util/mock_mounter_test.go @@ -0,0 +1,98 @@ +// Code generated by MockGen. DO NOT EDIT. +// Source: ./internal/util/remount.go +// +// Generated by this command: +// +// mockgen -source ./internal/util/remount.go -package ebutil mounter +// + +// Package util is a generated GoMock package. +package ebutil + +import ( + os "os" + reflect "reflect" + + procfs "github.com/prometheus/procfs" + gomock "go.uber.org/mock/gomock" +) + +// Mockmounter is a mock of mounter interface. +type Mockmounter struct { + ctrl *gomock.Controller + recorder *MockmounterMockRecorder +} + +// MockmounterMockRecorder is the mock recorder for Mockmounter. +type MockmounterMockRecorder struct { + mock *Mockmounter +} + +// NewMockmounter creates a new mock instance. +func NewMockmounter(ctrl *gomock.Controller) *Mockmounter { + mock := &Mockmounter{ctrl: ctrl} + mock.recorder = &MockmounterMockRecorder{mock} + return mock +} + +// EXPECT returns an object that allows the caller to indicate expected use. +func (m *Mockmounter) EXPECT() *MockmounterMockRecorder { + return m.recorder +} + +// GetMounts mocks base method. +func (m *Mockmounter) GetMounts() ([]*procfs.MountInfo, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "GetMounts") + ret0, _ := ret[0].([]*procfs.MountInfo) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// GetMounts indicates an expected call of GetMounts. +func (mr *MockmounterMockRecorder) GetMounts() *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetMounts", reflect.TypeOf((*Mockmounter)(nil).GetMounts)) +} + +// MkdirAll mocks base method. +func (m *Mockmounter) MkdirAll(arg0 string, arg1 os.FileMode) error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "MkdirAll", arg0, arg1) + ret0, _ := ret[0].(error) + return ret0 +} + +// MkdirAll indicates an expected call of MkdirAll. +func (mr *MockmounterMockRecorder) MkdirAll(arg0, arg1 any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "MkdirAll", reflect.TypeOf((*Mockmounter)(nil).MkdirAll), arg0, arg1) +} + +// Mount mocks base method. +func (m *Mockmounter) Mount(arg0, arg1, arg2 string, arg3 uintptr, arg4 string) error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "Mount", arg0, arg1, arg2, arg3, arg4) + ret0, _ := ret[0].(error) + return ret0 +} + +// Mount indicates an expected call of Mount. +func (mr *MockmounterMockRecorder) Mount(arg0, arg1, arg2, arg3, arg4 any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Mount", reflect.TypeOf((*Mockmounter)(nil).Mount), arg0, arg1, arg2, arg3, arg4) +} + +// Unmount mocks base method. +func (m *Mockmounter) Unmount(arg0 string, arg1 int) error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "Unmount", arg0, arg1) + ret0, _ := ret[0].(error) + return ret0 +} + +// Unmount indicates an expected call of Unmount. +func (mr *MockmounterMockRecorder) Unmount(arg0, arg1 any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Unmount", reflect.TypeOf((*Mockmounter)(nil).Unmount), arg0, arg1) +} diff --git a/internal/util/remount.go b/internal/util/remount.go new file mode 100644 index 00000000..5d0dec5c --- /dev/null +++ b/internal/util/remount.go @@ -0,0 +1,120 @@ +package ebutil + +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, +// and unmounts them from their original source. All mount points underneath ignorePrefixes +// will not be touched. +// +// 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, +) { + return tempRemount(&realMounter{}, logf, dest, ignorePrefixes...) +} + +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, 0750) + 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, 0750) + if err != nil { + return remount, fmt.Errorf("create temp mountpoint %s: %w", dest, err) + } + + 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()) + } + + 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) +} diff --git a/internal/util/remount_internal_test.go b/internal/util/remount_internal_test.go new file mode 100644 index 00000000..2fe01972 --- /dev/null +++ b/internal/util/remount_internal_test.go @@ -0,0 +1,199 @@ +package ebutil + +import ( + "github.com/coder/coder/v2/codersdk" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.uber.org/mock/gomock" + "os" + "strings" + "syscall" + "testing" + + "github.com/prometheus/procfs" +) + +func Test_tempRemount(t *testing.T) { + t.Parallel() + + t.Run("OK", func(t *testing.T) { + t.Parallel() + + ctrl := gomock.NewController(t) + mm := NewMockmounter(ctrl) + mounts := fakeMounts("/home", "/var/lib/modules:ro", "/proc", "/sys") + + mm.EXPECT().GetMounts().Return(mounts, nil) + mm.EXPECT().MkdirAll("/.test/var/lib/modules", os.FileMode(0750)).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().MkdirAll("/var/lib/modules", os.FileMode(0750)).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) + + remount, err := tempRemount(mm, fakeLog(t), "/.test") + require.NoError(t, err) + err = remount() + require.NoError(t, err) + }) + + t.Run("IgnorePrefixes", func(t *testing.T) { + t.Parallel() + + ctrl := gomock.NewController(t) + mm := NewMockmounter(ctrl) + mounts := fakeMounts("/home", "/var/lib/modules:ro", "/proc", "/sys") + + mm.EXPECT().GetMounts().Return(mounts, nil) + + remount, err := tempRemount(mm, fakeLog(t), "/.test", "/var/lib") + require.NoError(t, err) + err = remount() + require.NoError(t, err) + }) + + t.Run("ErrGetMounts", func(t *testing.T) { + t.Parallel() + + ctrl := gomock.NewController(t) + mm := NewMockmounter(ctrl) + mm.EXPECT().GetMounts().Return(nil, assert.AnError) + remount, err := tempRemount(mm, fakeLog(t), "/.test", "/var/lib") + require.ErrorContains(t, err, assert.AnError.Error()) + err = remount() + require.NoError(t, err) + }) + + t.Run("ErrMkdirAll", func(t *testing.T) { + t.Parallel() + + ctrl := gomock.NewController(t) + mm := NewMockmounter(ctrl) + mounts := fakeMounts("/home", "/var/lib/modules:ro", "/proc", "/sys") + + mm.EXPECT().GetMounts().Return(mounts, nil) + mm.EXPECT().MkdirAll("/.test/var/lib/modules", os.FileMode(0750)).Times(1).Return(assert.AnError) + + remount, err := tempRemount(mm, fakeLog(t), "/.test") + require.ErrorContains(t, err, assert.AnError.Error()) + err = remount() + require.NoError(t, err) + }) + + t.Run("ErrMountBind", func(t *testing.T) { + t.Parallel() + + ctrl := gomock.NewController(t) + mm := NewMockmounter(ctrl) + mounts := fakeMounts("/home", "/var/lib/modules:ro", "/proc", "/sys") + + mm.EXPECT().GetMounts().Return(mounts, nil) + mm.EXPECT().MkdirAll("/.test/var/lib/modules", os.FileMode(0750)).Times(1).Return(nil) + mm.EXPECT().Mount("/var/lib/modules", "/.test/var/lib/modules", "bind", uintptr(syscall.MS_BIND), "").Times(1).Return(assert.AnError) + + remount, err := tempRemount(mm, fakeLog(t), "/.test") + require.ErrorContains(t, err, assert.AnError.Error()) + err = remount() + require.NoError(t, err) + }) + + t.Run("ErrUnmount", func(t *testing.T) { + t.Parallel() + + ctrl := gomock.NewController(t) + mm := NewMockmounter(ctrl) + mounts := fakeMounts("/home", "/var/lib/modules:ro", "/proc", "/sys") + + mm.EXPECT().GetMounts().Return(mounts, nil) + mm.EXPECT().MkdirAll("/.test/var/lib/modules", os.FileMode(0750)).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) + + remount, err := tempRemount(mm, fakeLog(t), "/.test") + require.ErrorContains(t, err, assert.AnError.Error()) + err = remount() + require.NoError(t, err) + }) + + t.Run("ErrRemountMkdirAll", func(t *testing.T) { + t.Parallel() + + ctrl := gomock.NewController(t) + mm := NewMockmounter(ctrl) + mounts := fakeMounts("/home", "/var/lib/modules:ro", "/proc", "/sys") + + mm.EXPECT().GetMounts().Return(mounts, nil) + mm.EXPECT().MkdirAll("/.test/var/lib/modules", os.FileMode(0750)).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().MkdirAll("/var/lib/modules", os.FileMode(0750)).Times(1).Return(assert.AnError) + + remount, err := tempRemount(mm, fakeLog(t), "/.test") + require.NoError(t, err) + err = remount() + require.ErrorContains(t, err, assert.AnError.Error()) + }) + + t.Run("ErrRemountMountBind", func(t *testing.T) { + t.Parallel() + + ctrl := gomock.NewController(t) + mm := NewMockmounter(ctrl) + mounts := fakeMounts("/home", "/var/lib/modules:ro", "/proc", "/sys") + + mm.EXPECT().GetMounts().Return(mounts, nil) + mm.EXPECT().MkdirAll("/.test/var/lib/modules", os.FileMode(0750)).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().MkdirAll("/var/lib/modules", os.FileMode(0750)).Times(1).Return(nil) + mm.EXPECT().Mount("/.test/var/lib/modules", "/var/lib/modules", "bind", uintptr(syscall.MS_BIND), "").Times(1).Return(assert.AnError) + + remount, err := tempRemount(mm, fakeLog(t), "/.test") + require.NoError(t, err) + err = remount() + require.ErrorContains(t, err, assert.AnError.Error()) + }) + + t.Run("ErrRemountUnmount", func(t *testing.T) { + t.Parallel() + + ctrl := gomock.NewController(t) + mm := NewMockmounter(ctrl) + mounts := fakeMounts("/home", "/var/lib/modules:ro", "/proc", "/sys") + + mm.EXPECT().GetMounts().Return(mounts, nil) + mm.EXPECT().MkdirAll("/.test/var/lib/modules", os.FileMode(0750)).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().MkdirAll("/var/lib/modules", os.FileMode(0750)).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) + + remount, err := tempRemount(mm, fakeLog(t), "/.test") + require.NoError(t, err) + err = remount() + require.ErrorContains(t, err, assert.AnError.Error()) + }) +} + +// convenience function for generating a slice of *procfs.MountInfo +func fakeMounts(mounts ...string) []*procfs.MountInfo { + m := make([]*procfs.MountInfo, 0) + for _, s := range mounts { + mp := s + o := make(map[string]string) + if strings.HasSuffix(mp, ":ro") { + mp = strings.TrimSuffix(mp, ":ro") + o["ro"] = "true" + } + m = append(m, &procfs.MountInfo{MountPoint: mp, Options: o}) + } + return m +} + +func fakeLog(t *testing.T) func(codersdk.LogLevel, string, ...any) { + t.Helper() + return func(_ codersdk.LogLevel, s string, a ...any) { + t.Logf(s, a...) + } +} From 3f0e02233c19afe50b02bc6dab04326c96018d02 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Mon, 13 May 2024 14:28:41 +0100 Subject: [PATCH 08/20] fmt --- internal/util/remount.go | 7 ++++-- internal/util/remount_internal_test.go | 31 +++++++++++++------------- 2 files changed, 21 insertions(+), 17 deletions(-) diff --git a/internal/util/remount.go b/internal/util/remount.go index 5d0dec5c..a15f3029 100644 --- a/internal/util/remount.go +++ b/internal/util/remount.go @@ -35,7 +35,7 @@ func tempRemount(m mounter, logf func(codersdk.LogLevel, string, ...any), dest s // closer to attempt to restore original mount points remount = func() error { for src, tgt := range mounts { - err := m.MkdirAll(src, 0750) + err := m.MkdirAll(src, 0o750) if err != nil { return fmt.Errorf("recreate original mountpoint %s: %w", src, err) } @@ -69,7 +69,7 @@ outer: src := mountInfo.MountPoint tgt := filepath.Join("/", dest, src) - err := m.MkdirAll(tgt, 0750) + err := m.MkdirAll(tgt, 0o750) if err != nil { return remount, fmt.Errorf("create temp mountpoint %s: %w", dest, err) } @@ -109,12 +109,15 @@ 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) } diff --git a/internal/util/remount_internal_test.go b/internal/util/remount_internal_test.go index 2fe01972..07a407dd 100644 --- a/internal/util/remount_internal_test.go +++ b/internal/util/remount_internal_test.go @@ -1,15 +1,16 @@ package ebutil import ( - "github.com/coder/coder/v2/codersdk" - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" - "go.uber.org/mock/gomock" "os" "strings" "syscall" "testing" + "github.com/coder/coder/v2/codersdk" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.uber.org/mock/gomock" + "github.com/prometheus/procfs" ) @@ -24,10 +25,10 @@ func Test_tempRemount(t *testing.T) { mounts := fakeMounts("/home", "/var/lib/modules:ro", "/proc", "/sys") mm.EXPECT().GetMounts().Return(mounts, nil) - mm.EXPECT().MkdirAll("/.test/var/lib/modules", os.FileMode(0750)).Times(1).Return(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().MkdirAll("/var/lib/modules", os.FileMode(0750)).Times(1).Return(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) @@ -72,7 +73,7 @@ func Test_tempRemount(t *testing.T) { mounts := fakeMounts("/home", "/var/lib/modules:ro", "/proc", "/sys") mm.EXPECT().GetMounts().Return(mounts, nil) - mm.EXPECT().MkdirAll("/.test/var/lib/modules", os.FileMode(0750)).Times(1).Return(assert.AnError) + mm.EXPECT().MkdirAll("/.test/var/lib/modules", os.FileMode(0o750)).Times(1).Return(assert.AnError) remount, err := tempRemount(mm, fakeLog(t), "/.test") require.ErrorContains(t, err, assert.AnError.Error()) @@ -88,7 +89,7 @@ func Test_tempRemount(t *testing.T) { mounts := fakeMounts("/home", "/var/lib/modules:ro", "/proc", "/sys") mm.EXPECT().GetMounts().Return(mounts, nil) - mm.EXPECT().MkdirAll("/.test/var/lib/modules", os.FileMode(0750)).Times(1).Return(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) remount, err := tempRemount(mm, fakeLog(t), "/.test") @@ -105,7 +106,7 @@ func Test_tempRemount(t *testing.T) { mounts := fakeMounts("/home", "/var/lib/modules:ro", "/proc", "/sys") mm.EXPECT().GetMounts().Return(mounts, nil) - mm.EXPECT().MkdirAll("/.test/var/lib/modules", os.FileMode(0750)).Times(1).Return(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) @@ -123,10 +124,10 @@ func Test_tempRemount(t *testing.T) { mounts := fakeMounts("/home", "/var/lib/modules:ro", "/proc", "/sys") mm.EXPECT().GetMounts().Return(mounts, nil) - mm.EXPECT().MkdirAll("/.test/var/lib/modules", os.FileMode(0750)).Times(1).Return(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().MkdirAll("/var/lib/modules", os.FileMode(0750)).Times(1).Return(assert.AnError) + mm.EXPECT().MkdirAll("/var/lib/modules", os.FileMode(0o750)).Times(1).Return(assert.AnError) remount, err := tempRemount(mm, fakeLog(t), "/.test") require.NoError(t, err) @@ -142,10 +143,10 @@ func Test_tempRemount(t *testing.T) { mounts := fakeMounts("/home", "/var/lib/modules:ro", "/proc", "/sys") mm.EXPECT().GetMounts().Return(mounts, nil) - mm.EXPECT().MkdirAll("/.test/var/lib/modules", os.FileMode(0750)).Times(1).Return(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().MkdirAll("/var/lib/modules", os.FileMode(0750)).Times(1).Return(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) remount, err := tempRemount(mm, fakeLog(t), "/.test") @@ -162,10 +163,10 @@ func Test_tempRemount(t *testing.T) { mounts := fakeMounts("/home", "/var/lib/modules:ro", "/proc", "/sys") mm.EXPECT().GetMounts().Return(mounts, nil) - mm.EXPECT().MkdirAll("/.test/var/lib/modules", os.FileMode(0750)).Times(1).Return(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().MkdirAll("/var/lib/modules", os.FileMode(0750)).Times(1).Return(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) From 6caad1de728d88e0bd3cf7570d0937daf16a7dc6 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Mon, 13 May 2024 21:06:03 +0100 Subject: [PATCH 09/20] rename internal/util -> internal/ebutil --- envbuilder.go | 3 +-- internal/{util => ebutil}/mock_mounter_test.go | 6 +++--- internal/{util => ebutil}/remount.go | 2 ++ internal/{util => ebutil}/remount_internal_test.go | 0 4 files changed, 6 insertions(+), 5 deletions(-) rename internal/{util => ebutil}/mock_mounter_test.go (94%) rename internal/{util => ebutil}/remount.go (97%) rename internal/{util => ebutil}/remount_internal_test.go (100%) diff --git a/envbuilder.go b/envbuilder.go index 46c6ae40..cfeb260e 100644 --- a/envbuilder.go +++ b/envbuilder.go @@ -26,8 +26,6 @@ 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" @@ -38,6 +36,7 @@ import ( "github.com/GoogleContainerTools/kaniko/pkg/util" "github.com/coder/coder/v2/codersdk" "github.com/coder/envbuilder/devcontainer" + "github.com/coder/envbuilder/internal/ebutil" "github.com/containerd/containerd/platforms" "github.com/distribution/distribution/v3/configuration" "github.com/distribution/distribution/v3/registry/handlers" diff --git a/internal/util/mock_mounter_test.go b/internal/ebutil/mock_mounter_test.go similarity index 94% rename from internal/util/mock_mounter_test.go rename to internal/ebutil/mock_mounter_test.go index 19a20380..d67c7eb3 100644 --- a/internal/util/mock_mounter_test.go +++ b/internal/ebutil/mock_mounter_test.go @@ -1,12 +1,12 @@ // Code generated by MockGen. DO NOT EDIT. -// Source: ./internal/util/remount.go +// Source: remount.go // // Generated by this command: // -// mockgen -source ./internal/util/remount.go -package ebutil mounter +// mockgen -source remount.go -package ebutil mounter -write_generate_directive // -// Package util is a generated GoMock package. +// Package ebutil is a generated GoMock package. package ebutil import ( diff --git a/internal/util/remount.go b/internal/ebutil/remount.go similarity index 97% rename from internal/util/remount.go rename to internal/ebutil/remount.go index a15f3029..052b4361 100644 --- a/internal/util/remount.go +++ b/internal/ebutil/remount.go @@ -1,5 +1,7 @@ package ebutil +//go:generate mockgen -source remount.go -package ebutil mounter -destination mock_mounter_test.go + import ( "fmt" "os" diff --git a/internal/util/remount_internal_test.go b/internal/ebutil/remount_internal_test.go similarity index 100% rename from internal/util/remount_internal_test.go rename to internal/ebutil/remount_internal_test.go From adc6c9c9d7e3d23ff091a7ec5781917aca0f794f Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Mon, 13 May 2024 21:06:58 +0100 Subject: [PATCH 10/20] rename remount func to restore --- internal/ebutil/remount.go | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/internal/ebutil/remount.go b/internal/ebutil/remount.go index 052b4361..6650f865 100644 --- a/internal/ebutil/remount.go +++ b/internal/ebutil/remount.go @@ -19,14 +19,14 @@ import ( // // 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, +// the operation, calling the returned function will make a best-effort attempt to restore +// the original state. +func TempRemount(logf func(codersdk.LogLevel, string, ...any), dest string, ignorePrefixes ...string) (restore func() error, err error, ) { return tempRemount(&realMounter{}, logf, dest, ignorePrefixes...) } -func tempRemount(m mounter, logf func(codersdk.LogLevel, string, ...any), dest string, ignorePrefixes ...string) (remount func() error, err error) { +func tempRemount(m mounter, logf func(codersdk.LogLevel, string, ...any), dest string, ignorePrefixes ...string) (restore func() error, err error) { mountInfos, err := m.GetMounts() if err != nil { return func() error { return nil }, fmt.Errorf("get mounts: %w", err) @@ -35,7 +35,7 @@ func tempRemount(m mounter, logf func(codersdk.LogLevel, string, ...any), dest s // temp move of all ro mounts mounts := map[string]string{} // closer to attempt to restore original mount points - remount = func() error { + restore = func() error { for src, tgt := range mounts { err := m.MkdirAll(src, 0o750) if err != nil { @@ -73,22 +73,22 @@ outer: tgt := filepath.Join("/", dest, src) err := m.MkdirAll(tgt, 0o750) if err != nil { - return remount, fmt.Errorf("create temp mountpoint %s: %w", dest, err) + return restore, fmt.Errorf("create temp mountpoint %s: %w", dest, err) } 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()) + return restore, 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()) + return restore, fmt.Errorf("temp unmount src %s: %s", src, err.Error()) } mounts[src] = tgt } - return remount, nil + return restore, nil } // mounter is an interface to system-level calls used by TempRemount. From 752e25db84336ba83958fc8adea2fc50391eb48b Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Mon, 13 May 2024 21:32:27 +0100 Subject: [PATCH 11/20] add clarifying comment --- internal/ebutil/remount.go | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/internal/ebutil/remount.go b/internal/ebutil/remount.go index 6650f865..b5ef4c38 100644 --- a/internal/ebutil/remount.go +++ b/internal/ebutil/remount.go @@ -17,6 +17,18 @@ import ( // and unmounts them from their original source. All mount points underneath ignorePrefixes // will not be touched. // +// Some container runtimes such as sysbox-runc will mount in `/lib/modules` read-only. +// See https://github.com/nestybox/sysbox/issues/564 +// This trips us up because: +// 1. We call a Kaniko library function `util.DeleteFilesystem` that does exactly what it says +// on the tin. If this hits a read-only volume mounted in, unhappiness is the result. +// 2. After deleting the filesystem and building the image, we extract it to the filesystem. +// If some paths mounted in via volume are present at that time, unhappiness is also likely +// to result -- especially in case of read-only mounts. +// +// To work around this we move the mounts out of the way temporarily by bind-mounting them +// while we do our thing, and move them back when we're done. +// // 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 function will make a best-effort attempt to restore From 8cc42614d435c54a651109fcfe45fe683e64fa84 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Mon, 13 May 2024 21:35:31 +0100 Subject: [PATCH 12/20] fix mockgen invocation --- internal/ebutil/mock_mounter_test.go | 4 +++- internal/ebutil/remount.go | 2 -- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/internal/ebutil/mock_mounter_test.go b/internal/ebutil/mock_mounter_test.go index d67c7eb3..3386d56e 100644 --- a/internal/ebutil/mock_mounter_test.go +++ b/internal/ebutil/mock_mounter_test.go @@ -3,7 +3,7 @@ // // Generated by this command: // -// mockgen -source remount.go -package ebutil mounter -write_generate_directive +// mockgen -source=remount.go -package=ebutil -destination=mock_mounter_test.go -write_generate_directive // // Package ebutil is a generated GoMock package. @@ -17,6 +17,8 @@ import ( gomock "go.uber.org/mock/gomock" ) +//go:generate mockgen -source=remount.go -package=ebutil -destination=mock_mounter_test.go -write_generate_directive + // Mockmounter is a mock of mounter interface. type Mockmounter struct { ctrl *gomock.Controller diff --git a/internal/ebutil/remount.go b/internal/ebutil/remount.go index b5ef4c38..bbb72a6f 100644 --- a/internal/ebutil/remount.go +++ b/internal/ebutil/remount.go @@ -1,7 +1,5 @@ package ebutil -//go:generate mockgen -source remount.go -package ebutil mounter -destination mock_mounter_test.go - import ( "fmt" "os" From 5af94b79957c221ea9b5d5481320d359ba88b696 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Mon, 13 May 2024 21:46:46 +0100 Subject: [PATCH 13/20] DRY remount func --- internal/ebutil/remount.go | 50 ++++++++++++++++---------------------- 1 file changed, 21 insertions(+), 29 deletions(-) diff --git a/internal/ebutil/remount.go b/internal/ebutil/remount.go index bbb72a6f..25187a08 100644 --- a/internal/ebutil/remount.go +++ b/internal/ebutil/remount.go @@ -36,7 +36,7 @@ func TempRemount(logf func(codersdk.LogLevel, string, ...any), dest string, igno return tempRemount(&realMounter{}, logf, dest, ignorePrefixes...) } -func tempRemount(m mounter, logf func(codersdk.LogLevel, string, ...any), dest string, ignorePrefixes ...string) (restore func() error, err error) { +func tempRemount(m mounter, logf func(codersdk.LogLevel, string, ...any), base string, ignorePrefixes ...string) (restore func() error, err error) { mountInfos, err := m.GetMounts() if err != nil { return func() error { return nil }, fmt.Errorf("get mounts: %w", err) @@ -46,20 +46,9 @@ func tempRemount(m mounter, logf func(codersdk.LogLevel, string, ...any), dest s mounts := map[string]string{} // closer to attempt to restore original mount points restore = 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) + for orig, moved := range mounts { + if err := remount(m, moved, orig); err != nil { + return fmt.Errorf("restore mount: %w", err) } } return nil @@ -80,27 +69,30 @@ outer: } src := mountInfo.MountPoint - tgt := filepath.Join("/", dest, src) - err := m.MkdirAll(tgt, 0o750) - if err != nil { - return restore, fmt.Errorf("create temp mountpoint %s: %w", dest, err) - } - - err = m.Mount(src, tgt, "bind", syscall.MS_BIND, "") - if err != nil { - return restore, fmt.Errorf("bind mount %s => %s: %s", src, dest, err.Error()) - } - err = m.Unmount(src, 0) - if err != nil { - return restore, fmt.Errorf("temp unmount src %s: %s", src, err.Error()) + dest := filepath.Join("/", base, src) + if err := remount(m, src, dest); err != nil { + return restore, fmt.Errorf("temp remount: %w", err) } - mounts[src] = tgt + mounts[src] = dest } return restore, nil } +func remount(m mounter, src, dest string) error { + if err := m.MkdirAll(dest, 0o750); err != nil { + return fmt.Errorf("ensure path: %w", err) + } + if err := m.Mount(src, dest, "bind", syscall.MS_BIND, ""); err != nil { + return fmt.Errorf("bind mount %s => %s: %w", src, dest, err) + } + if err := m.Unmount(src, 0); err != nil { + return fmt.Errorf("unmount orig src %s: %w", src, err) + } + return nil +} + // mounter is an interface to system-level calls used by TempRemount. type mounter interface { // GetMounts wraps procfs.GetMounts From a07c597bbc75081753a9ece9c29b378961e1cf0f Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Mon, 13 May 2024 21:47:38 +0100 Subject: [PATCH 14/20] restore multierror --- go.mod | 6 +++--- internal/ebutil/remount.go | 6 ++++-- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/go.mod b/go.mod index 5c99d40f..703ee109 100644 --- a/go.mod +++ b/go.mod @@ -30,14 +30,17 @@ require ( github.com/go-git/go-billy/v5 v5.5.0 github.com/go-git/go-git/v5 v5.12.0 github.com/google/go-containerregistry v0.15.2 + github.com/hashicorp/go-multierror v1.1.1 github.com/kballard/go-shellquote v0.0.0-20180428030007-95032a82bc51 github.com/mattn/go-isatty v0.0.20 github.com/moby/buildkit v0.11.6 github.com/otiai10/copy v1.14.0 + github.com/prometheus/procfs v0.12.0 github.com/sirupsen/logrus v1.9.3 github.com/skeema/knownhosts v1.2.2 github.com/stretchr/testify v1.9.0 github.com/tailscale/hujson v0.0.0-20221223112325-20486734a56a + go.uber.org/mock v0.4.0 golang.org/x/crypto v0.22.0 golang.org/x/sync v0.7.0 golang.org/x/xerrors v0.0.0-20231012003039-104605ab7028 @@ -155,7 +158,6 @@ require ( github.com/hashicorp/go-hclog v1.5.0 // indirect github.com/hashicorp/go-immutable-radix v1.3.1 // indirect github.com/hashicorp/go-memdb v1.3.2 // indirect - github.com/hashicorp/go-multierror v1.1.1 // indirect github.com/hashicorp/go-uuid v1.0.3 // indirect github.com/hashicorp/go-version v1.6.0 // indirect github.com/hashicorp/golang-lru v1.0.2 // indirect @@ -221,7 +223,6 @@ require ( github.com/prometheus/client_golang v1.18.0 // indirect github.com/prometheus/client_model v0.5.0 // indirect github.com/prometheus/common v0.46.0 // indirect - github.com/prometheus/procfs v0.12.0 // indirect github.com/richardartoul/molecule v1.0.1-0.20221107223329-32cfee06a052 // indirect github.com/rivo/uniseg v0.4.4 // indirect github.com/robfig/cron/v3 v3.0.1 // indirect @@ -262,7 +263,6 @@ 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 diff --git a/internal/ebutil/remount.go b/internal/ebutil/remount.go index 25187a08..90f7d58e 100644 --- a/internal/ebutil/remount.go +++ b/internal/ebutil/remount.go @@ -8,6 +8,7 @@ import ( "syscall" "github.com/coder/coder/v2/codersdk" + "github.com/hashicorp/go-multierror" "github.com/prometheus/procfs" ) @@ -46,12 +47,13 @@ func tempRemount(m mounter, logf func(codersdk.LogLevel, string, ...any), base s mounts := map[string]string{} // closer to attempt to restore original mount points restore = func() error { + var merr error for orig, moved := range mounts { if err := remount(m, moved, orig); err != nil { - return fmt.Errorf("restore mount: %w", err) + merr = multierror.Append(merr, fmt.Errorf("restore mount: %w", err)) } } - return nil + return merr } outer: From a3b89aad88ea221c3fd6fbcbc2edd417f45eab5d Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Mon, 13 May 2024 21:50:49 +0100 Subject: [PATCH 15/20] add sync.Once to remount func() --- internal/ebutil/remount.go | 12 ++++++++---- internal/ebutil/remount_internal_test.go | 2 ++ 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/internal/ebutil/remount.go b/internal/ebutil/remount.go index 90f7d58e..176accb4 100644 --- a/internal/ebutil/remount.go +++ b/internal/ebutil/remount.go @@ -5,6 +5,7 @@ import ( "os" "path/filepath" "strings" + "sync" "syscall" "github.com/coder/coder/v2/codersdk" @@ -46,13 +47,16 @@ func tempRemount(m mounter, logf func(codersdk.LogLevel, string, ...any), base s // temp move of all ro mounts mounts := map[string]string{} // closer to attempt to restore original mount points + var restoreOnce sync.Once restore = func() error { var merr error - for orig, moved := range mounts { - if err := remount(m, moved, orig); err != nil { - merr = multierror.Append(merr, fmt.Errorf("restore mount: %w", err)) + restoreOnce.Do(func() { + for orig, moved := range mounts { + if err := remount(m, moved, orig); err != nil { + merr = multierror.Append(merr, fmt.Errorf("restore mount: %w", err)) + } } - } + }) return merr } diff --git a/internal/ebutil/remount_internal_test.go b/internal/ebutil/remount_internal_test.go index 07a407dd..911aabee 100644 --- a/internal/ebutil/remount_internal_test.go +++ b/internal/ebutil/remount_internal_test.go @@ -36,6 +36,8 @@ func Test_tempRemount(t *testing.T) { 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) { From d3f41df048e172bf9a9b3fab016252c865a7d01a Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Mon, 13 May 2024 21:58:14 +0100 Subject: [PATCH 16/20] defer restoreMounts() --- envbuilder.go | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/envbuilder.go b/envbuilder.go index cfeb260e..b9f4d364 100644 --- a/envbuilder.go +++ b/envbuilder.go @@ -398,7 +398,12 @@ func Run(ctx context.Context, options Options) error { // temp move of all ro mounts tempRemountDest := filepath.Join("/", MagicDir) ignorePrefixes := []string{tempRemountDest, "/proc", "/sys"} - remount, err := ebutil.TempRemount(options.Logger, tempRemountDest, ignorePrefixes...) + restoreMounts, err := ebutil.TempRemount(options.Logger, tempRemountDest, ignorePrefixes...) + defer func() { // restoreMounts should never be nil + if err := restoreMounts(); err != nil { + options.Logger(codersdk.LogLevelError, "restore mounts: %s", err.Error()) + } + }() if err != nil { return fmt.Errorf("temp remount: %w", err) } @@ -549,8 +554,7 @@ 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. + if err := restoreMounts(); err != nil { options.Logger(codersdk.LogLevelError, "recreate mountpoint: %w", err) } From e1298893d749dfb41702c6b4281c90d8f92aa47b Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Mon, 13 May 2024 22:00:47 +0100 Subject: [PATCH 17/20] move to /MagicDir/mnt subdir --- envbuilder.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/envbuilder.go b/envbuilder.go index b9f4d364..e76dd513 100644 --- a/envbuilder.go +++ b/envbuilder.go @@ -396,7 +396,7 @@ func Run(ctx context.Context, options Options) error { } // temp move of all ro mounts - tempRemountDest := filepath.Join("/", MagicDir) + tempRemountDest := filepath.Join("/", MagicDir, "mnt") ignorePrefixes := []string{tempRemountDest, "/proc", "/sys"} restoreMounts, err := ebutil.TempRemount(options.Logger, tempRemountDest, ignorePrefixes...) defer func() { // restoreMounts should never be nil From c225ef9def98e346402749649ff3f1ec81e7cab1 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Mon, 13 May 2024 22:02:17 +0100 Subject: [PATCH 18/20] TODO --- internal/ebutil/remount.go | 1 + 1 file changed, 1 insertion(+) diff --git a/internal/ebutil/remount.go b/internal/ebutil/remount.go index 176accb4..f46f1a1e 100644 --- a/internal/ebutil/remount.go +++ b/internal/ebutil/remount.go @@ -62,6 +62,7 @@ func tempRemount(m mounter, logf func(codersdk.LogLevel, string, ...any), base s outer: for _, mountInfo := range mountInfos { + // TODO: do this for all mounts if _, ok := mountInfo.Options["ro"]; !ok { logf(codersdk.LogLevelTrace, "skip rw mount %s", mountInfo.MountPoint) continue From 797a4d9ab9dfb2829da5e861519d70e3292a915a Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Mon, 13 May 2024 22:03:51 +0100 Subject: [PATCH 19/20] fixup! defer restoreMounts() --- envbuilder.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/envbuilder.go b/envbuilder.go index e76dd513..1aceb8cc 100644 --- a/envbuilder.go +++ b/envbuilder.go @@ -555,7 +555,7 @@ func Run(ctx context.Context, options Options) error { } if err := restoreMounts(); err != nil { - options.Logger(codersdk.LogLevelError, "recreate mountpoint: %w", err) + return fmt.Errorf("restore mounts: %w", err) } // Create the magic file to indicate that this build From 34d809fd6a83b44be911915cbcd14e165035c81a Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Tue, 14 May 2024 12:19:11 +0100 Subject: [PATCH 20/20] address comments --- internal/ebutil/remount.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/internal/ebutil/remount.go b/internal/ebutil/remount.go index f46f1a1e..056a1057 100644 --- a/internal/ebutil/remount.go +++ b/internal/ebutil/remount.go @@ -46,10 +46,10 @@ func tempRemount(m mounter, logf func(codersdk.LogLevel, string, ...any), base s // temp move of all ro mounts mounts := map[string]string{} - // closer to attempt to restore original mount points var restoreOnce sync.Once + var merr error + // closer to attempt to restore original mount points restore = func() error { - var merr error restoreOnce.Do(func() { for orig, moved := range mounts { if err := remount(m, moved, orig); err != nil { @@ -76,7 +76,7 @@ outer: } src := mountInfo.MountPoint - dest := filepath.Join("/", base, src) + dest := filepath.Join(base, src) if err := remount(m, src, dest); err != nil { return restore, fmt.Errorf("temp remount: %w", err) }