diff --git a/pkg/copy/copy.go b/pkg/copy/copy.go index 2bf8f91c8..d8bd1cdf6 100644 --- a/pkg/copy/copy.go +++ b/pkg/copy/copy.go @@ -1,6 +1,7 @@ package copy import ( + "errors" "fmt" "io" "io/fs" @@ -40,22 +41,31 @@ func ChownR(path string, userName string) error { uidInt, _ := strconv.Atoi(userID.Uid) gidInt, _ := strconv.Atoi(userID.Gid) - return filepath.WalkDir(path, func(name string, dirEntry fs.DirEntry, err error) error { + // #nosec G115 -- a resolved system uid is non-negative and fits uint32. + uidU32 := uint32(uidInt) + + // A single un-chownable entry (e.g. a read-only file on a virtiofs share) + // must not abort the walk and leave the rest of the tree unowned. + var errs []error + _ = filepath.WalkDir(path, func(name string, dirEntry fs.DirEntry, err error) error { if err != nil { - return err + errs = append(errs, err) + return nil } - info, err := dirEntry.Info() if err != nil { return nil } - - if IsUID(info, uint32(uidInt)) { + if IsUID(info, uidU32) { return nil } - - return os.Lchown(name, uidInt, gidInt) + // #nosec G122 -- best-effort chown of a freshly provisioned tree we own; WalkDir yields real paths. + if err := os.Lchown(name, uidInt, gidInt); err != nil { + errs = append(errs, err) + } + return nil }) + return errors.Join(errs...) } func MkdirAllChown(path string, perm os.FileMode, userName string) error { diff --git a/pkg/devcontainer/setup.go b/pkg/devcontainer/setup.go index 0b3c8c850..d01c8ac5b 100644 --- a/pkg/devcontainer/setup.go +++ b/pkg/devcontainer/setup.go @@ -355,17 +355,27 @@ func (r *runner) addSetupFlags(args *[]string) { } func (r *runner) addChownFlag(args *[]string, isDockerDriver bool) { - if shouldChownWorkspace(runtime.GOOS, isDockerDriver, r.isPodmanRuntime()) { + if shouldChownWorkspace( + runtime.GOOS, + isDockerDriver, + r.isPodmanRuntime(), + r.driverRequiresWorkspaceChown(), + ) { *args = append(*args, names.Flag(names.ChownWorkspace)) } } // shouldChownWorkspace reports whether the agent should chown the workspace -// folder to the remote user during setup. Podman needs it on any host OS: -// its `podman machine` bind mounts are root-owned inside the container, so a -// non-root remote user can't enter the workspace folder otherwise. -func shouldChownWorkspace(goos string, isDockerDriver, isPodman bool) bool { - return goos == goosLinux || !isDockerDriver || isPodman +// folder to the remote user during setup. Podman and drivers reporting +// driverNeedsChown expose bind mounts as root-owned in the guest, so a non-root +// remote user can't enter the workspace folder otherwise. +func shouldChownWorkspace(goos string, isDockerDriver, isPodman, driverNeedsChown bool) bool { + return goos == goosLinux || !isDockerDriver || isPodman || driverNeedsChown +} + +func (r *runner) driverRequiresWorkspaceChown() bool { + c, ok := r.driver.(driver.WorkspaceChowner) + return ok && c.RequiresWorkspaceChown() } // isPodmanRuntime reports whether the docker driver is backed by the Podman diff --git a/pkg/devcontainer/setup/setup.go b/pkg/devcontainer/setup/setup.go index 785c3557d..524877b49 100644 --- a/pkg/devcontainer/setup/setup.go +++ b/pkg/devcontainer/setup/setup.go @@ -336,9 +336,11 @@ func chownWorkspace(setupInfo *config.Result, recursive bool) error { setupInfo.SubstitutionContext.ContainerWorkspaceFolder, ) err = copy2.ChownR(setupInfo.SubstitutionContext.ContainerWorkspaceFolder, user) - // do not exit on error, we can have non-fatal errors + // Best effort: some entries (e.g. read-only .git pack files on a + // virtiofs share) legitimately cannot be chowned. The remote user can + // still work in the tree, so this is not worth a warning. if err != nil { - log.Warn(err) + log.Debugf("chown workspace: some entries could not be chowned: %v", err) } } diff --git a/pkg/devcontainer/setup_test.go b/pkg/devcontainer/setup_test.go index 9a4a51846..11941937a 100644 --- a/pkg/devcontainer/setup_test.go +++ b/pkg/devcontainer/setup_test.go @@ -11,11 +11,12 @@ import ( func TestShouldChownWorkspace(t *testing.T) { cases := []struct { - name string - goos string - isDockerDriver bool - isPodman bool - want bool + name string + goos string + isDockerDriver bool + isPodman bool + driverNeedsChown bool + want bool }{ { name: "linux docker host always chowns", @@ -47,13 +48,23 @@ func TestShouldChownWorkspace(t *testing.T) { name: "podman on linux chowns", goos: goosLinux, isDockerDriver: true, isPodman: true, want: true, }, + { + // microsandbox shares the workspace over virtiofs root-owned, so a + // non-root remote user needs the chown even on macOS. + name: "driver needing chown chowns despite docker driver on macOS", + goos: goosDarwin, + isDockerDriver: true, + isPodman: false, + driverNeedsChown: true, + want: true, + }, } for _, c := range cases { t.Run(c.name, func(t *testing.T) { - got := shouldChownWorkspace(c.goos, c.isDockerDriver, c.isPodman) + got := shouldChownWorkspace(c.goos, c.isDockerDriver, c.isPodman, c.driverNeedsChown) if got != c.want { - t.Errorf("shouldChownWorkspace(%q, %v, %v) = %v, want %v", - c.goos, c.isDockerDriver, c.isPodman, got, c.want) + t.Errorf("shouldChownWorkspace(%q, %v, %v, %v) = %v, want %v", + c.goos, c.isDockerDriver, c.isPodman, c.driverNeedsChown, got, c.want) } }) } diff --git a/pkg/driver/microsandbox/cliclient.go b/pkg/driver/microsandbox/cliclient.go index 0585cbdde..1826be27f 100644 --- a/pkg/driver/microsandbox/cliclient.go +++ b/pkg/driver/microsandbox/cliclient.go @@ -202,6 +202,12 @@ func mountArgs(mounts []volumeMount) []string { args = append(args, "--tmpfs", m.Target) case m.Volume != "": args = append(args, "--mount-named", m.Volume+":"+m.Target) + case m.Source != "": + spec := m.Source + ":" + m.Target + if m.ReadOnly { + spec += ":ro" + } + args = append(args, "--mount-dir", spec) } } return args diff --git a/pkg/driver/microsandbox/cliclient_test.go b/pkg/driver/microsandbox/cliclient_test.go index a7136f899..569ab1585 100644 --- a/pkg/driver/microsandbox/cliclient_test.go +++ b/pkg/driver/microsandbox/cliclient_test.go @@ -72,6 +72,8 @@ func TestMountArgsAndNamedVolumes(t *testing.T) { {Target: "/a", Volume: "vol-a"}, {Target: "/b", Tmpfs: true}, {Target: "/c", Volume: "vol-c"}, + {Target: testBindDst, Source: testBindSrc}, + {Target: "/ro", Source: "/host/ro", ReadOnly: true}, } if got := namedVolumes(mounts); !slices.Equal(got, []string{"vol-a", "vol-c"}) { t.Errorf("namedVolumes = %v, want [vol-a vol-c]", got) @@ -79,7 +81,9 @@ func TestMountArgsAndNamedVolumes(t *testing.T) { args := mountArgs(mounts) if !hasFlagValue(args, "--mount-named", "vol-a:/a") || !hasFlagValue(args, "--tmpfs", "/b") || - !hasFlagValue(args, "--mount-named", "vol-c:/c") { + !hasFlagValue(args, "--mount-named", "vol-c:/c") || + !hasFlagValue(args, "--mount-dir", testBindSrc+":"+testBindDst) || + !hasFlagValue(args, "--mount-dir", "/host/ro:/ro:ro") { t.Errorf("mountArgs = %v", args) } } diff --git a/pkg/driver/microsandbox/client.go b/pkg/driver/microsandbox/client.go index aa2000cbc..1528f95a8 100644 --- a/pkg/driver/microsandbox/client.go +++ b/pkg/driver/microsandbox/client.go @@ -22,9 +22,11 @@ type sandboxSpec struct { } type volumeMount struct { - Target string - Volume string - Tmpfs bool + Target string + Source string + Volume string + Tmpfs bool + ReadOnly bool } type sandboxInfo struct { diff --git a/pkg/driver/microsandbox/microsandbox.go b/pkg/driver/microsandbox/microsandbox.go index ba254b717..fc60e173b 100644 --- a/pkg/driver/microsandbox/microsandbox.go +++ b/pkg/driver/microsandbox/microsandbox.go @@ -243,6 +243,12 @@ func (d *microsandboxDriver) UpdateContainerUserUID( return nil } +// RequiresWorkspaceChown reports that the virtiofs workspace share is +// root-owned in the guest, so the agent must chown it to the remote user. +func (d *microsandboxDriver) RequiresWorkspaceChown() bool { + return true +} + func (d *microsandboxDriver) StartDevContainer(ctx context.Context, workspaceID string) error { if err := d.client.Start(ctx, sandboxName(workspaceID)); err != nil { return fmt.Errorf("start microsandbox VM: %w", err) @@ -350,29 +356,50 @@ func (d *microsandboxDriver) buildSpec(workspaceID string, options *driver.RunOp Labels: labels, Ephemeral: d.defaults.ephemeral, IdleTimeout: d.defaults.idleTimeout, - Mounts: volumeMounts(options.Mounts), + Mounts: volumeMounts(options), MaxMemory: d.defaults.maxMemory, MaxCPUs: d.defaults.maxCPUs, BlockEgress: d.defaults.blockEgress, } } -func volumeMounts(mounts []*config.Mount) []volumeMount { +func volumeMounts(options *driver.RunOptions) []volumeMount { var out []volumeMount - for _, m := range mounts { - if m == nil || m.Target == "" { - continue - } - switch m.Type { - case driver.MountTypeVolume: - out = append(out, volumeMount{Target: m.Target, Volume: m.Source}) - case driver.MountTypeTmpfs: - out = append(out, volumeMount{Target: m.Target, Tmpfs: true}) + if b := bindMount(options.WorkspaceMount); b != nil { + out = append(out, *b) + } + for _, m := range options.Mounts { + if vm, ok := toVolumeMount(m); ok { + out = append(out, vm) } } return out } +func toVolumeMount(m *config.Mount) (volumeMount, bool) { + if m == nil || m.Target == "" { + return volumeMount{}, false + } + switch m.Type { + case driver.MountTypeBind: + if b := bindMount(m); b != nil { + return *b, true + } + case driver.MountTypeVolume: + return volumeMount{Target: m.Target, Volume: m.Source}, true + case driver.MountTypeTmpfs: + return volumeMount{Target: m.Target, Tmpfs: true}, true + } + return volumeMount{}, false +} + +func bindMount(m *config.Mount) *volumeMount { + if m == nil || m.Source == "" || m.Target == "" { + return nil + } + return &volumeMount{Target: m.Target, Source: m.Source, ReadOnly: m.IsReadOnly()} +} + func warnUnsupportedOptions(options *driver.RunOptions) { var ignored []string if isPrivileged(options) { diff --git a/pkg/driver/microsandbox/microsandbox_test.go b/pkg/driver/microsandbox/microsandbox_test.go index d4fc75d3b..edcbd731c 100644 --- a/pkg/driver/microsandbox/microsandbox_test.go +++ b/pkg/driver/microsandbox/microsandbox_test.go @@ -15,14 +15,16 @@ import ( ) const ( - wsName = "devsy-ws1" - testImage = "example:latest" - testUser = "vscode" - imgX = "x:1" - testImg = "img:1" - shPath = "/bin/sh" - callFind = "find:" + wsName - callRemove = "remove:" + wsName + wsName = "devsy-ws1" + testImage = "example:latest" + testUser = "vscode" + imgX = "x:1" + testImg = "img:1" + shPath = "/bin/sh" + callFind = "find:" + wsName + callRemove = "remove:" + wsName + testBindSrc = "/host/proj" + testBindDst = "/workspaces/proj" ) // fakeClient is an in-memory sandboxClient that records calls, so the driver's @@ -448,25 +450,40 @@ func TestParseDuration(t *testing.T) { } } -func TestBuildSpecMapsVolumeMountsSkipsBind(t *testing.T) { +func TestBuildSpecMapsAllMountTypes(t *testing.T) { d := newDriver(newFakeClient(), nil, specDefaults{}) spec := d.buildSpec("ws1", &driver.RunOptions{ Image: imgX, Mounts: []*config.Mount{ {Type: driver.MountTypeVolume, Source: "vol1", Target: "/data"}, {Type: driver.MountTypeTmpfs, Target: "/scratch"}, - {Type: driver.MountTypeBind, Source: "/host", Target: "/mnt"}, + {Type: driver.MountTypeBind, Source: testBindSrc, Target: "/mnt"}, nil, }, }) - if len(spec.Mounts) != 2 { - t.Fatalf("got %d mounts, want 2 (bind + nil skipped): %+v", len(spec.Mounts), spec.Mounts) + want := []volumeMount{ + {Target: "/data", Volume: "vol1"}, + {Target: "/scratch", Tmpfs: true}, + {Target: "/mnt", Source: testBindSrc}, } - if spec.Mounts[0].Target != "/data" || spec.Mounts[0].Volume != "vol1" || spec.Mounts[0].Tmpfs { - t.Errorf("volume mount mapped wrong: %+v", spec.Mounts[0]) + if !slices.Equal(spec.Mounts, want) { + t.Errorf("mounts = %+v, want %+v", spec.Mounts, want) } - if spec.Mounts[1].Target != "/scratch" || !spec.Mounts[1].Tmpfs { - t.Errorf("tmpfs mount mapped wrong: %+v", spec.Mounts[1]) +} + +func TestBuildSpecMapsWorkspaceMount(t *testing.T) { + d := newDriver(newFakeClient(), nil, specDefaults{}) + spec := d.buildSpec("ws1", &driver.RunOptions{ + Image: imgX, + WorkspaceMount: &config.Mount{ + Type: driver.MountTypeBind, + Source: testBindSrc, + Target: testBindDst, + }, + }) + want := []volumeMount{{Target: testBindDst, Source: testBindSrc}} + if !slices.Equal(spec.Mounts, want) { + t.Errorf("mounts = %+v, want %+v", spec.Mounts, want) } } diff --git a/pkg/driver/types.go b/pkg/driver/types.go index eb0ff0059..cf4d509cc 100644 --- a/pkg/driver/types.go +++ b/pkg/driver/types.go @@ -47,6 +47,13 @@ type RunOptionsDriver interface { RunDevContainer(ctx context.Context, workspaceID string, options *RunOptions) error } +// WorkspaceChowner is a capability interface for drivers whose workspace bind +// mount is root-owned in the guest, so the agent must chown it to the remote +// user during setup rather than relying on a UID remap. +type WorkspaceChowner interface { + RequiresWorkspaceChown() bool +} + // ReprovisioningDriver is a capability interface for drivers that can reprovision // an existing devcontainer in place. Reprovisioning re-runs the container, so it // embeds RunOptionsDriver. diff --git a/providers/microsandbox/provider.yaml b/providers/microsandbox/provider.yaml index 6c432d3de..b07e72e25 100644 --- a/providers/microsandbox/provider.yaml +++ b/providers/microsandbox/provider.yaml @@ -17,6 +17,7 @@ optionGroups: options: MICROSANDBOX_MEMORY: description: "Guest memory limit in MiB. Empty uses the runtime default." + default: "2048" MICROSANDBOX_CPUS: description: "Number of virtual CPUs. Empty uses the runtime default." MICROSANDBOX_MAX_MEMORY: