Refactor OCI Spec generation to use oci.SpecOpts more

This has the nice side-effect of unifying the mount- and
non-mount-changes made due to processMode and securityMode.

Signed-off-by: Paul "TBBle" Hampson <Paul.Hampson@Pobox.com>

executor/oci/mounts.go

@@ -5,75 +5,50 @@ import (
 	"path/filepath"
 	"strings"
 
+	"github.com/containerd/containerd/containers"
+	"github.com/containerd/containerd/oci"
 	specs "github.com/opencontainers/runtime-spec/specs-go"
-	"github.com/pkg/errors"
 )
 
-// MountOpts sets oci spec specific info for mount points
-type MountOpts func([]specs.Mount) ([]specs.Mount, error)
-
-//GetMounts returns default required for buildkit
-// https://github.com/moby/buildkit/issues/429
-func GetMounts(ctx context.Context, mountOpts ...MountOpts) ([]specs.Mount, error) {
-	mounts := []specs.Mount{
-		{
-			Destination: "/proc",
-			Type:        "proc",
-			Source:      "proc",
-		},
-		{
-			Destination: "/dev",
-			Type:        "tmpfs",
-			Source:      "tmpfs",
-			Options:     []string{"nosuid", "strictatime", "mode=755", "size=65536k"},
-		},
-		{
-			Destination: "/dev/pts",
-			Type:        "devpts",
-			Source:      "devpts",
-			Options:     []string{"nosuid", "noexec", "newinstance", "ptmxmode=0666", "mode=0620", "gid=5"},
-		},
-		{
-			Destination: "/dev/shm",
-			Type:        "tmpfs",
-			Source:      "shm",
-			Options:     []string{"nosuid", "noexec", "nodev", "mode=1777", "size=65536k"},
-		},
-		{
-			Destination: "/dev/mqueue",
-			Type:        "mqueue",
-			Source:      "mqueue",
-			Options:     []string{"nosuid", "noexec", "nodev"},
-		},
-		{
-			Destination: "/sys",
-			Type:        "sysfs",
-			Source:      "sysfs",
-			Options:     []string{"nosuid", "noexec", "nodev", "ro"},
-		},
-	}
-	var err error
-	for _, o := range mountOpts {
-		mounts, err = o(mounts)
-		if err != nil {
-			return nil, err
+func withRemovedMount(destination string) oci.SpecOpts {
+	return func(_ context.Context, _ oci.Client, _ *containers.Container, s *specs.Spec) error {
+		newMounts := []specs.Mount{}
+		for _, o := range s.Mounts {
+			if o.Destination != destination {
+				newMounts = append(newMounts, o)
+			}
 		}
+		s.Mounts = newMounts
+
+		return nil
 	}
-	return mounts, nil
 }
 
-func withROBind(src, dest string) func(m []specs.Mount) ([]specs.Mount, error) {
-	return func(m []specs.Mount) ([]specs.Mount, error) {
-		m = append(m, specs.Mount{
+func withROBind(src, dest string) oci.SpecOpts {
+	return func(_ context.Context, _ oci.Client, _ *containers.Container, s *specs.Spec) error {
+		s.Mounts = append(s.Mounts, specs.Mount{
 			Destination: dest,
 			Type:        "bind",
 			Source:      src,
 			Options:     []string{"nosuid", "noexec", "nodev", "rbind", "ro"},
 		})
-		return m, nil
+		return nil
 	}
 }
 
+func withCGroup() oci.SpecOpts {
+	return func(_ context.Context, _ oci.Client, _ *containers.Container, s *specs.Spec) error {
+		s.Mounts = append(s.Mounts, specs.Mount{
+			Destination: "/sys/fs/cgroup",
+			Type:        "cgroup",
+			Source:      "cgroup",
+			Options:     []string{"ro", "nosuid", "noexec", "nodev"},
+		})
+		return nil
+	}
+
+}
+
 func hasPrefix(p, prefixDir string) bool {
 	prefixDir = filepath.Clean(prefixDir)
 	if filepath.Base(prefixDir) == string(filepath.Separator) {
@@ -93,25 +68,35 @@ func removeMountsWithPrefix(mounts []specs.Mount, prefixDir string) []specs.Moun
 	return ret
 }
 
-func withProcessMode(processMode ProcessMode) func([]specs.Mount) ([]specs.Mount, error) {
-	return func(m []specs.Mount) ([]specs.Mount, error) {
-		switch processMode {
-		case ProcessSandbox:
-			// keep the default
-		case NoProcessSandbox:
-			m = removeMountsWithPrefix(m, "/proc")
-			procMount := specs.Mount{
-				Destination: "/proc",
-				Type:        "bind",
-				Source:      "/proc",
-				// NOTE: "rbind"+"ro" does not make /proc read-only recursively.
-				// So we keep maskedPath and readonlyPaths (although not mandatory for rootless mode)
-				Options: []string{"rbind"},
-			}
-			m = append([]specs.Mount{procMount}, m...)
-		default:
-			return nil, errors.Errorf("unknown process mode: %v", processMode)
+func withBoundProc() oci.SpecOpts {
+	return func(_ context.Context, _ oci.Client, _ *containers.Container, s *specs.Spec) error {
+		s.Mounts = removeMountsWithPrefix(s.Mounts, "/proc")
+		procMount := specs.Mount{
+			Destination: "/proc",
+			Type:        "bind",
+			Source:      "/proc",
+			// NOTE: "rbind"+"ro" does not make /proc read-only recursively.
+			// So we keep maskedPath and readonlyPaths (although not mandatory for rootless mode)
+			Options: []string{"rbind"},
 		}
-		return m, nil
+		s.Mounts = append([]specs.Mount{procMount}, s.Mounts...)
+
+		var maskedPaths []string
+		for _, s := range s.Linux.MaskedPaths {
+			if !hasPrefix(s, "/proc") {
+				maskedPaths = append(maskedPaths, s)
+			}
+		}
+		s.Linux.MaskedPaths = maskedPaths
+
+		var readonlyPaths []string
+		for _, s := range s.Linux.ReadonlyPaths {
+			if !hasPrefix(s, "/proc") {
+				readonlyPaths = append(readonlyPaths, s)
+			}
+		}
+		s.Linux.ReadonlyPaths = readonlyPaths
+
+		return nil
 	}
 }

executor/oci/mounts_test.go

@@ -4,6 +4,9 @@ import (
 	"runtime"
 	"testing"
 
+	"github.com/containerd/containerd/oci"
+	"github.com/moby/buildkit/util/appcontext"
+	specs "github.com/opencontainers/runtime-spec/specs-go"
 	"github.com/stretchr/testify/assert"
 )
 
@@ -94,3 +97,58 @@ func TestHasPrefix(t *testing.T) {
 		assert.Equal(t, tc.expected, actual, "#%d: under(%q,%q)", i, tc.path, tc.prefix)
 	}
 }
+
+func TestWithRemovedMounts(t *testing.T) {
+	// The default mount-list from containerd
+	s := oci.Spec{
+		Mounts: []specs.Mount{
+			{
+				Destination: "/proc",
+				Type:        "proc",
+				Source:      "proc",
+				Options:     []string{"nosuid", "noexec", "nodev"},
+			},
+			{
+				Destination: "/dev",
+				Type:        "tmpfs",
+				Source:      "tmpfs",
+				Options:     []string{"nosuid", "strictatime", "mode=755", "size=65536k"},
+			},
+			{
+				Destination: "/dev/pts",
+				Type:        "devpts",
+				Source:      "devpts",
+				Options:     []string{"nosuid", "noexec", "newinstance", "ptmxmode=0666", "mode=0620", "gid=5"},
+			},
+			{
+				Destination: "/dev/shm",
+				Type:        "tmpfs",
+				Source:      "shm",
+				Options:     []string{"nosuid", "noexec", "nodev", "mode=1777", "size=65536k"},
+			},
+			{
+				Destination: "/dev/mqueue",
+				Type:        "mqueue",
+				Source:      "mqueue",
+				Options:     []string{"nosuid", "noexec", "nodev"},
+			},
+			{
+				Destination: "/sys",
+				Type:        "sysfs",
+				Source:      "sysfs",
+				Options:     []string{"nosuid", "noexec", "nodev", "ro"},
+			},
+			{
+				Destination: "/run",
+				Type:        "tmpfs",
+				Source:      "tmpfs",
+				Options:     []string{"nosuid", "strictatime", "mode=755", "size=65536k"},
+			},
+		},
+	}
+
+	oldLen := len(s.Mounts)
+	err := withRemovedMount("/run")(appcontext.Context(), nil, nil, &s)
+	assert.NoError(t, err)
+	assert.Equal(t, oldLen-1, len(s.Mounts))
+}

executor/oci/spec.go

@@ -14,7 +14,6 @@ import (
 	"github.com/mitchellh/hashstructure"
 	"github.com/moby/buildkit/executor"
 	"github.com/moby/buildkit/snapshot"
-	"github.com/moby/buildkit/solver/pb"
 	"github.com/moby/buildkit/util/network"
 	specs "github.com/opencontainers/runtime-spec/specs-go"
 	"github.com/pkg/errors"
@@ -47,6 +46,12 @@ func GenerateSpec(ctx context.Context, meta executor.Meta, mounts []executor.Mou
 		ctx = namespaces.WithNamespace(ctx, "buildkit")
 	}
 
+	if mountOpts, err := generateMountOpts(resolvConf, hostsFile); err == nil {
+		opts = append(opts, mountOpts...)
+	} else {
+		return nil, nil, err
+	}
+
 	if securityOpts, err := generateSecurityOpts(meta.SecurityMode); err == nil {
 		opts = append(opts, securityOpts...)
 	} else {
@@ -59,6 +64,20 @@ func GenerateSpec(ctx context.Context, meta executor.Meta, mounts []executor.Mou
 		return nil, nil, err
 	}
 
+	if idmapOpts, err := generateIDmapOpts(idmap); err == nil {
+		opts = append(opts, idmapOpts...)
+	} else {
+		return nil, nil, err
+	}
+
+	opts = append(opts,
+		oci.WithProcessArgs(meta.Args...),
+		oci.WithEnv(meta.Env),
+		oci.WithProcessCwd(meta.Cwd),
+		oci.WithNewPrivileges,
+		oci.WithHostname("buildkitsandbox"),
+	)
+
 	s, err := oci.GenerateSpec(ctx, nil, c, opts...)
 	if err != nil {
 		return nil, nil, err
@@ -66,65 +85,7 @@ func GenerateSpec(ctx context.Context, meta executor.Meta, mounts []executor.Mou
 	// set the networking information on the spec
 	namespace.Set(s)
 
-	s.Process.Args = meta.Args
-	s.Process.Env = meta.Env
-	s.Process.Cwd = meta.Cwd
-	s.Process.Rlimits = nil           // reset open files limit
-	s.Process.NoNewPrivileges = false // reset nonewprivileges
-	s.Hostname = "buildkitsandbox"
-
-	// Setup for a Linux-based container (includes LCOW)
-	if s.Linux != nil {
-		s.Mounts, err = GetMounts(ctx,
-			withProcessMode(processMode),
-			withROBind(resolvConf, "/etc/resolv.conf"),
-			withROBind(hostsFile, "/etc/hosts"),
-		)
-		if err != nil {
-			return nil, nil, err
-		}
-
-		s.Mounts = append(s.Mounts, specs.Mount{
-			Destination: "/sys/fs/cgroup",
-			Type:        "cgroup",
-			Source:      "cgroup",
-			Options:     []string{"ro", "nosuid", "noexec", "nodev"},
-		})
-
-		if processMode == NoProcessSandbox {
-			var maskedPaths []string
-			for _, s := range s.Linux.MaskedPaths {
-				if !hasPrefix(s, "/proc") {
-					maskedPaths = append(maskedPaths, s)
-				}
-			}
-			s.Linux.MaskedPaths = maskedPaths
-			var readonlyPaths []string
-			for _, s := range s.Linux.ReadonlyPaths {
-				if !hasPrefix(s, "/proc") {
-					readonlyPaths = append(readonlyPaths, s)
-				}
-			}
-			s.Linux.ReadonlyPaths = readonlyPaths
-		}
-
-		if meta.SecurityMode == pb.SecurityMode_INSECURE {
-			if err = oci.WithWriteableCgroupfs(ctx, nil, c, s); err != nil {
-				return nil, nil, err
-			}
-			if err = oci.WithWriteableSysfs(ctx, nil, c, s); err != nil {
-				return nil, nil, err
-			}
-		}
-
-		if idmap != nil {
-			s.Linux.Namespaces = append(s.Linux.Namespaces, specs.LinuxNamespace{
-				Type: specs.UserNamespace,
-			})
-			s.Linux.UIDMappings = specMapping(idmap.UIDs())
-			s.Linux.GIDMappings = specMapping(idmap.GIDs())
-		}
-	}
+	s.Process.Rlimits = nil // reset open files limit
 
 	sm := &submounts{}
 

executor/oci/spec_unix.go

@@ -5,26 +5,54 @@ package oci
 import (
 	"github.com/containerd/containerd/contrib/seccomp"
 	"github.com/containerd/containerd/oci"
+	"github.com/docker/docker/pkg/idtools"
 	"github.com/moby/buildkit/solver/pb"
 	"github.com/moby/buildkit/util/entitlements/security"
 	"github.com/moby/buildkit/util/system"
 	specs "github.com/opencontainers/runtime-spec/specs-go"
 )
 
+func generateMountOpts(resolvConf, hostsFile string) ([]oci.SpecOpts, error) {
+	return []oci.SpecOpts{
+		// https://github.com/moby/buildkit/issues/429
+		withRemovedMount("/run"),
+		withROBind(resolvConf, "/etc/resolv.conf"),
+		withROBind(hostsFile, "/etc/hosts"),
+		withCGroup(),
+	}, nil
+}
+
+// generateSecurityOpts may affect mounts, so must be called after generateMountOpts
 func generateSecurityOpts(mode pb.SecurityMode) ([]oci.SpecOpts, error) {
 	if mode == pb.SecurityMode_INSECURE {
-		return []oci.SpecOpts{security.WithInsecureSpec()}, nil
+		return []oci.SpecOpts{
+			security.WithInsecureSpec(),
+			oci.WithWriteableCgroupfs,
+			oci.WithWriteableSysfs,
+		}, nil
 	} else if system.SeccompSupported() && mode == pb.SecurityMode_SANDBOX {
 		return []oci.SpecOpts{seccomp.WithDefaultProfile()}, nil
 	}
 	return nil, nil
 }
 
+// generateProcessModeOpts may affect mounts, so must be called after generateMountOpts
 func generateProcessModeOpts(mode ProcessMode) ([]oci.SpecOpts, error) {
 	if mode == NoProcessSandbox {
-		// Mount for /proc is replaced in GetMounts() anyway
-		return []oci.SpecOpts{oci.WithHostNamespace(specs.PIDNamespace)}, nil
+		return []oci.SpecOpts{
+			oci.WithHostNamespace(specs.PIDNamespace),
+			withBoundProc(),
+		}, nil
 		// TODO(AkihiroSuda): Configure seccomp to disable ptrace (and prctl?) explicitly
 	}
 	return nil, nil
 }
+
+func generateIDmapOpts(idmap *idtools.IdentityMapping) ([]oci.SpecOpts, error) {
+	if idmap == nil {
+		return nil, nil
+	}
+	return []oci.SpecOpts{
+		oci.WithUserNamespace(specMapping(idmap.UIDs()), specMapping(idmap.GIDs())),
+	}, nil
+}

executor/oci/spec_windows.go

@@ -5,11 +5,17 @@ package oci
 import (
 	"github.com/containerd/containerd/contrib/seccomp"
 	"github.com/containerd/containerd/oci"
+	"github.com/docker/docker/pkg/idtools"
 	"github.com/moby/buildkit/solver/pb"
 	"github.com/moby/buildkit/util/system"
 	"github.com/pkg/errors"
 )
 
+func generateMountOpts(resolvConf, hostsFile string) ([]oci.SpecOpts, error) {
+	return nil, nil
+}
+
+// generateSecurityOpts may affect mounts, so must be called after generateMountOpts
 func generateSecurityOpts(mode pb.SecurityMode) ([]oci.SpecOpts, error) {
 	if mode == pb.SecurityMode_INSECURE {
 		return nil, errors.New("no support for running in insecure mode on Windows")
@@ -20,9 +26,17 @@ func generateSecurityOpts(mode pb.SecurityMode) ([]oci.SpecOpts, error) {
 	return nil, nil
 }
 
+// generateProcessModeOpts may affect mounts, so must be called after generateMountOpts
 func generateProcessModeOpts(mode ProcessMode) ([]oci.SpecOpts, error) {
 	if mode == NoProcessSandbox {
 		return nil, errors.New("no support for NoProcessSandbox on Windows")
 	}
 	return nil, nil
 }
+
+func generateIDmapOpts(idmap *idtools.IdentityMapping) ([]oci.SpecOpts, error) {
+	if idmap == nil {
+		return nil, nil
+	}
+	return nil, errors.New("no support for IdentityMapping on Windows")
+}