containerd · AkihiroSuda · May 3, 2024 · Sep 7, 2023
diff --git a/cmd/containerd-shim-runc-v2/process/init.go b/cmd/containerd-shim-runc-v2/process/init.go
@@ -21,7 +21,6 @@ package process
 import (
 	"context"
 	"encoding/json"
-	"errors"
 	"fmt"
 	"io"
 	"os"
@@ -142,12 +141,6 @@ func (p *Init) Create(ctx context.Context, r *CreateConfig) error {
 		opts.ConsoleSocket = socket
 	}
 
-	// runc ignores silently features it doesn't know about, so for things that this is
-	// problematic let's check if this runc version supports them.
-	if err := p.validateRuncFeatures(ctx, r.Bundle); err != nil {
-		return fmt.Errorf("failed to detect OCI runtime features: %w", err)
-	}
-
 	if err := p.runtime.Create(ctx, r.ID, r.Bundle, opts); err != nil {
 		return p.runtimeError(err, "OCI runtime create failed")
 	}
@@ -181,60 +174,6 @@ func (p *Init) Create(ctx context.Context, r *CreateConfig) error {
 	return nil
 }
 
-func (p *Init) validateRuncFeatures(ctx context.Context, bundle string) error {
-	// TODO: We should remove the logic from here and rebase on #8509.
-	// This way we can avoid the call to readConfig() here and the call to p.runtime.Features()
-	// in validateIDMapMounts().
-	// But that PR is not yet merged nor it is clear if it will be refactored.
-	// Do this contained hack for now.
-	spec, err := readConfig(bundle)
-	if err != nil {
-		return fmt.Errorf("failed to read config: %w", err)
-	}
-
-	if err := p.validateIDMapMounts(ctx, spec); err != nil {
-		return fmt.Errorf("OCI runtime doesn't support idmap mounts: %w", err)
-	}
-
-	return nil
-}
-
-func (p *Init) validateIDMapMounts(ctx context.Context, spec *specs.Spec) error {
-	var used bool
-	for _, m := range spec.Mounts {
-		if m.UIDMappings != nil || m.GIDMappings != nil {
-			used = true
-			break
-		}
-		if sliceContainsStr(m.Options, "idmap") || sliceContainsStr(m.Options, "ridmap") {
-			used = true
-			break
-		}
-	}
-
-	if !used {
-		return nil
-	}
-
-	// From here onwards, we require idmap mounts. So if we fail to check, we return an error.
-	features, err := p.runtime.Features(ctx)
-	if err != nil {
-		// If the features command is not implemented, then runc is too old.
-		return fmt.Errorf("features command failed: %w", err)
-
-	}
-
-	if features.Linux.MountExtensions == nil || features.Linux.MountExtensions.IDMap == nil {
-		return errors.New("missing `mountExtensions.idmap` entry in `features` command")
-	}
-
-	if enabled := features.Linux.MountExtensions.IDMap.Enabled; enabled == nil || !*enabled {
-		return errors.New("idmap mounts not supported")
-	}
-
-	return nil
-}
-
 func (p *Init) openStdin(path string) error {
 	sc, err := fifo.OpenFifo(context.Background(), path, unix.O_WRONLY|unix.O_NONBLOCK, 0)
 	if err != nil {
@@ -556,12 +495,3 @@ func withConditionalIO(c stdio.Stdio) runc.IOOpt {
 		o.OpenStderr = c.Stderr != ""
 	}
 }
-
-func sliceContainsStr(s []string, str string) bool {
-	for _, s := range s {
-		if s == str {
-			return true
-		}
-	}
-	return false
-}
diff --git a/cmd/containerd-shim-runc-v2/process/utils.go b/cmd/containerd-shim-runc-v2/process/utils.go
@@ -21,7 +21,6 @@ package process
 import (
 	"context"
 	"encoding/json"
-	"errors"
 	"fmt"
 	"io"
 	"os"
@@ -32,7 +31,6 @@ import (
 
 	"github.com/containerd/errdefs"
 	runc "github.com/containerd/go-runc"
-	specs "github.com/opencontainers/runtime-spec/specs-go"
 	"golang.org/x/sys/unix"
 )
 
@@ -41,8 +39,6 @@ const (
 	RuncRoot = "/run/containerd/runc"
 	// InitPidFile name of the file that contains the init pid
 	InitPidFile = "init.pid"
-	// configFile is the name of the runc config file
-	configFile = "config.json"
 )
 
 // safePid is a thread safe wrapper for pid.
@@ -188,23 +184,3 @@ func stateName(v interface{}) string {
 	}
 	panic(fmt.Errorf("invalid state %v", v))
 }
-
-func readConfig(path string) (spec *specs.Spec, err error) {
-	cfg := filepath.Join(path, configFile)
-	f, err := os.Open(cfg)
-	if err != nil {
-		if os.IsNotExist(err) {
-			return nil, fmt.Errorf("JSON specification file %s not found", cfg)
-		}
-		return nil, err
-	}
-	defer f.Close()
-
-	if err = json.NewDecoder(f).Decode(&spec); err != nil {
-		return nil, fmt.Errorf("failed to parse config: %w", err)
-	}
-	if spec == nil {
-		return nil, errors.New("config cannot be null")
-	}
-	return spec, nil
-}
diff --git a/core/runtime/v2/task_manager.go b/core/runtime/v2/task_manager.go
@@ -19,13 +19,18 @@ package v2
 import (
 	"bytes"
 	"context"
+	"errors"
 	"fmt"
 	"os"
 	"os/exec"
+	"slices"
 
 	"github.com/containerd/errdefs"
 	"github.com/containerd/plugin"
 	"github.com/containerd/plugin/registry"
+	"github.com/containerd/typeurl/v2"
+	"github.com/opencontainers/runtime-spec/specs-go"
+	"github.com/opencontainers/runtime-spec/specs-go/features"
 
 	apitypes "github.com/containerd/containerd/v2/api/types"
 	"github.com/containerd/containerd/v2/core/runtime"
@@ -111,6 +116,12 @@ func (m *TaskManager) Create(ctx context.Context, taskID string, opts runtime.Cr
 		return nil, err
 	}
 
+	// runc ignores silently features it doesn't know about, so for things that this is
+	// problematic let's check if this runc version supports them.
+	if err := m.validateRuntimeFeatures(ctx, opts); err != nil {
+		return nil, fmt.Errorf("failed to validate OCI runtime features: %w", err)
+	}
+
 	t, err := shimTask.Create(ctx, opts)
 	if err != nil {
 		// NOTE: ctx contains required namespace information.
@@ -224,3 +235,71 @@ func (m *TaskManager) PluginInfo(ctx context.Context, request interface{}) (inte
 	}
 	return &info, nil
 }
+
+func (m *TaskManager) validateRuntimeFeatures(ctx context.Context, opts runtime.CreateOpts) error {
+	var spec specs.Spec
+	if err := typeurl.UnmarshalTo(opts.Spec, &spec); err != nil {
+		return fmt.Errorf("unmarshal spec: %w", err)
+	}
+
+	// Only ask for the PluginInfo if idmap mounts are used.
+	if !usesIDMapMounts(spec) {
+		return nil
+	}
+
+	pInfo, err := m.PluginInfo(ctx, &apitypes.RuntimeRequest{RuntimePath: opts.Runtime})
+	if err != nil {
+		return fmt.Errorf("runtime info: %w", err)
+	}
+
+	pluginInfo, ok := pInfo.(*apitypes.RuntimeInfo)
+	if !ok {
+		return fmt.Errorf("invalid runtime info type: %T", pInfo)
+	}
+
+	feat, err := typeurl.UnmarshalAny(pluginInfo.Features)
+	if err != nil {
+		return fmt.Errorf("unmarshal runtime features: %w", err)
+	}
+
+	// runc-compatible runtimes silently ignores features it doesn't know about. But ignoring
+	// our request to use idmap mounts can break permissions in the volume, so let's make sure
+	// it supports it. For more info, see:
+	//	https://github.com/opencontainers/runtime-spec/pull/1219
+	//
+	features, ok := feat.(*features.Features)
+	if !ok {
+		// Leave alone non runc-compatible runtimes that don't provide the features info,
+		// they might not be affected by this.
+		return nil
+	}
+
+	if err := supportsIDMapMounts(features); err != nil {
+		return fmt.Errorf("idmap mounts not supported: %w", err)
+	}
+
+	return nil
+}
+
+func usesIDMapMounts(spec specs.Spec) bool {
+	for _, m := range spec.Mounts {
+		if m.UIDMappings != nil || m.GIDMappings != nil {
+			return true
+		}
+		if slices.Contains(m.Options, "idmap") || slices.Contains(m.Options, "ridmap") {
+			return true
+		}
+
+	}
+	return false
+}
+
+func supportsIDMapMounts(features *features.Features) error {
+	if features.Linux.MountExtensions == nil || features.Linux.MountExtensions.IDMap == nil {
+		return errors.New("missing `mountExtensions.idmap` entry in `features` command")
+	}
+	if enabled := features.Linux.MountExtensions.IDMap.Enabled; enabled == nil || !*enabled {
+		return errors.New("entry `mountExtensions.idmap.Enabled` in `features` command not present or disabled")
+	}
+	return nil
+}