Merge pull request #11618 from k8s-infra-cherrypick-robot/cherry-pick-11569-to-release/2.0

[release/2.0] update taskOptions based on runtimeOptions when creating a task

client/container.go

@@ -280,6 +280,7 @@ func (c *container) NewTask(ctx context.Context, ioCreate cio.Creator, opts ...N
 	}
 	info := TaskInfo{
 		runtime:        r.Runtime.Name,
+		runtimeOptions: r.Runtime.Options,
 	}
 	for _, o := range opts {
 		if err := o(ctx, c.client, &info); err != nil {

client/task.go

@@ -146,6 +146,11 @@ type TaskInfo struct {
 
 	// runtime is the runtime name for the container, and cannot be changed.
 	runtime string
+
+	// runtimeOptions is the runtime options for the container, and when task options are set,
+	// they will be based on the runtimeOptions.
+	// https://github.com/containerd/containerd/issues/11568
+	runtimeOptions typeurl.Any
 }
 
 // Runtime name for the container
@@ -153,6 +158,29 @@ func (i *TaskInfo) Runtime() string {
 	return i.runtime
 }
 
+// getRuncOptions returns a reference to the runtime options for use by the task.
+// If the set of options is not set by the opts passed into the NewTask creation
+// this function first attempts to initialize the runtime options with a copy of the runtimeOptions,
+// otherwise an empty set of options is assigned and returned
+func (i *TaskInfo) getRuncOptions() (*options.Options, error) {
+	if i.Options != nil {
+		opts, ok := i.Options.(*options.Options)
+		if !ok {
+			return nil, errors.New("invalid runtime v2 options format")
+		}
+		return opts, nil
+	}
+
+	opts := &options.Options{}
+	if i.runtimeOptions != nil && i.runtimeOptions.GetValue() != nil {
+		if err := typeurl.UnmarshalTo(i.runtimeOptions, opts); err != nil {
+			return nil, fmt.Errorf("failed to get runtime v2 options: %w", err)
+		}
+	}
+	i.Options = opts
+	return opts, nil
+}
+
 // Task is the executable object within containerd
 type Task interface {
 	Process

client/task_opts.go

@@ -54,12 +54,9 @@ func WithRuntimePath(absRuntimePath string) NewTaskOpts {
 // usually it is served inside a sandbox, and we can get it from sandbox status.
 func WithTaskAPIEndpoint(address string, version uint32) NewTaskOpts {
 	return func(ctx context.Context, client *Client, info *TaskInfo) error {
-		if info.Options == nil {
+		opts, err := info.getRuncOptions()
-			info.Options = &options.Options{}
+		if err != nil {
-		}
+			return err
-		opts, ok := info.Options.(*options.Options)
-		if !ok {
-			return errors.New("invalid runtime v2 options format")
 		}
 		opts.TaskApiAddress = address
 		opts.TaskApiVersion = version
@@ -119,12 +116,9 @@ func WithCheckpointImagePath(path string) CheckpointTaskOpts {
 // WithRestoreImagePath sets image path for create option
 func WithRestoreImagePath(path string) NewTaskOpts {
 	return func(ctx context.Context, c *Client, ti *TaskInfo) error {
-		if ti.Options == nil {
+		opts, err := ti.getRuncOptions()
-			ti.Options = &options.Options{}
+		if err != nil {
-		}
+			return err
-		opts, ok := ti.Options.(*options.Options)
-		if !ok {
-			return errors.New("invalid runtime v2 options format")
 		}
 		opts.CriuImagePath = path
 		return nil
@@ -134,12 +128,9 @@ func WithRestoreImagePath(path string) NewTaskOpts {
 // WithRestoreWorkPath sets criu work path for create option
 func WithRestoreWorkPath(path string) NewTaskOpts {
 	return func(ctx context.Context, c *Client, ti *TaskInfo) error {
-		if ti.Options == nil {
+		opts, err := ti.getRuncOptions()
-			ti.Options = &options.Options{}
+		if err != nil {
-		}
+			return err
-		opts, ok := ti.Options.(*options.Options)
-		if !ok {
-			return errors.New("invalid runtime v2 options format")
 		}
 		opts.CriuWorkPath = path
 		return nil

client/task_opts_unix.go

@@ -20,20 +20,14 @@ package client
 
 import (
 	"context"
-	"errors"
-
-	"github.com/containerd/containerd/api/types/runc/options"
 )
 
 // WithNoNewKeyring causes tasks not to be created with a new keyring for secret storage.
 // There is an upper limit on the number of keyrings in a linux system
 func WithNoNewKeyring(ctx context.Context, c *Client, ti *TaskInfo) error {
-	if ti.Options == nil {
+	opts, err := ti.getRuncOptions()
-		ti.Options = &options.Options{}
+	if err != nil {
-	}
+		return err
-	opts, ok := ti.Options.(*options.Options)
-	if !ok {
-		return errors.New("invalid v2 shim create options format")
 	}
 	opts.NoNewKeyring = true
 	return nil
@@ -41,12 +35,9 @@ func WithNoNewKeyring(ctx context.Context, c *Client, ti *TaskInfo) error {
 
 // WithNoPivotRoot instructs the runtime not to you pivot_root
 func WithNoPivotRoot(_ context.Context, _ *Client, ti *TaskInfo) error {
-	if ti.Options == nil {
+	opts, err := ti.getRuncOptions()
-		ti.Options = &options.Options{}
+	if err != nil {
-	}
+		return err
-	opts, ok := ti.Options.(*options.Options)
-	if !ok {
-		return errors.New("invalid v2 shim create options format")
 	}
 	opts.NoPivotRoot = true
 	return nil
@@ -55,12 +46,9 @@ func WithNoPivotRoot(_ context.Context, _ *Client, ti *TaskInfo) error {
 // WithShimCgroup sets the existing cgroup for the shim
 func WithShimCgroup(path string) NewTaskOpts {
 	return func(ctx context.Context, c *Client, ti *TaskInfo) error {
-		if ti.Options == nil {
+		opts, err := ti.getRuncOptions()
-			ti.Options = &options.Options{}
+		if err != nil {
-		}
+			return err
-		opts, ok := ti.Options.(*options.Options)
-		if !ok {
-			return errors.New("invalid v2 shim create options format")
 		}
 		opts.ShimCgroup = path
 		return nil
@@ -70,12 +58,9 @@ func WithShimCgroup(path string) NewTaskOpts {
 // WithUIDOwner allows console I/O to work with the remapped UID in user namespace
 func WithUIDOwner(uid uint32) NewTaskOpts {
 	return func(ctx context.Context, c *Client, ti *TaskInfo) error {
-		if ti.Options == nil {
+		opts, err := ti.getRuncOptions()
-			ti.Options = &options.Options{}
+		if err != nil {
-		}
+			return err
-		opts, ok := ti.Options.(*options.Options)
-		if !ok {
-			return errors.New("invalid v2 shim create options format")
 		}
 		opts.IoUid = uid
 		return nil
@@ -85,12 +70,9 @@ func WithUIDOwner(uid uint32) NewTaskOpts {
 // WithGIDOwner allows console I/O to work with the remapped GID in user namespace
 func WithGIDOwner(gid uint32) NewTaskOpts {
 	return func(ctx context.Context, c *Client, ti *TaskInfo) error {
-		if ti.Options == nil {
+		opts, err := ti.getRuncOptions()
-			ti.Options = &options.Options{}
+		if err != nil {
-		}
+			return err
-		opts, ok := ti.Options.(*options.Options)
-		if !ok {
-			return errors.New("invalid v2 shim create options format")
 		}
 		opts.IoGid = gid
 		return nil

core/runtime/v2/task_manager.go

@@ -266,12 +266,12 @@ func (m *TaskManager) validateRuntimeFeatures(ctx context.Context, opts runtime.
 		return nil
 	}
 
-	ropts := opts.RuntimeOptions
+	topts := opts.TaskOptions
-	if ropts == nil || ropts.GetValue() == nil {
+	if topts == nil || topts.GetValue() == nil {
-		ropts = opts.TaskOptions
+		topts = opts.RuntimeOptions
 	}
 
-	pInfo, err := m.PluginInfo(ctx, &apitypes.RuntimeRequest{RuntimePath: opts.Runtime, Options: typeurl.MarshalProto(ropts)})
+	pInfo, err := m.PluginInfo(ctx, &apitypes.RuntimeRequest{RuntimePath: opts.Runtime, Options: typeurl.MarshalProto(topts)})
 	if err != nil {
 		return fmt.Errorf("runtime info: %w", err)
 	}

integration/client/client_unix_test.go

@@ -19,12 +19,26 @@
 package client
 
 import (
+	"context"
+	"strings"
+	"sync"
 	"testing"
 
+	"github.com/containerd/containerd/api/services/tasks/v1"
+	"github.com/containerd/containerd/api/types/runc/options"
 	. "github.com/containerd/containerd/v2/client"
 	"github.com/containerd/containerd/v2/integration/images"
 	"github.com/containerd/containerd/v2/pkg/deprecation"
+	"github.com/containerd/containerd/v2/pkg/oci"
+	"github.com/containerd/containerd/v2/pkg/protobuf"
+	"github.com/containerd/containerd/v2/plugins"
+	"github.com/containerd/errdefs"
+	"github.com/containerd/errdefs/pkg/errgrpc"
 	"github.com/containerd/platforms"
+	"github.com/containerd/typeurl/v2"
+	"github.com/google/go-cmp/cmp"
+	"github.com/stretchr/testify/require"
+	"google.golang.org/grpc"
 )
 
 var (
@@ -63,3 +77,118 @@ func TestImagePullSchema1WithEmptyLayers(t *testing.T) {
 		t.Fatal(err)
 	}
 }
+
+func TestNewTaskWithRuntimeOption(t *testing.T) {
+	t.Parallel()
+
+	fakeTasks := &fakeTaskService{
+		TasksClient:    tasks.NewTasksClient(nil),
+		createRequests: map[string]*tasks.CreateTaskRequest{},
+	}
+
+	cli, err := newClient(t, address,
+		WithServices(WithTaskClient(fakeTasks)),
+	)
+	require.NoError(t, err)
+	defer cli.Close()
+
+	var (
+		image       Image
+		ctx, cancel = testContext(t)
+	)
+	defer cancel()
+
+	image, err = cli.GetImage(ctx, testImage)
+	require.NoError(t, err)
+
+	for _, tc := range []struct {
+		name            string
+		runtimeOption   *options.Options
+		taskOpts        []NewTaskOpts
+		expectedOptions *options.Options
+	}{
+		{
+			name: "should be empty options",
+			runtimeOption: &options.Options{
+				BinaryName: "no-runc",
+			},
+			expectedOptions: nil,
+		},
+		{
+			name: "should overwrite IOUid/ShimCgroup",
+			runtimeOption: &options.Options{
+				BinaryName:    "no-runc",
+				ShimCgroup:    "/abc",
+				IoUid:         1000,
+				SystemdCgroup: true,
+			},
+			taskOpts: []NewTaskOpts{
+				WithUIDOwner(2000),
+				WithGIDOwner(3000),
+				WithShimCgroup("/def"),
+			},
+			expectedOptions: &options.Options{
+				BinaryName:    "no-runc",
+				ShimCgroup:    "/def",
+				IoUid:         2000,
+				IoGid:         3000,
+				SystemdCgroup: true,
+			},
+		},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			id := strings.Replace(t.Name(), "/", "_", -1)
+
+			container, err := cli.NewContainer(
+				ctx,
+				id,
+				WithNewSnapshotView(id, image),
+				WithNewSpec(oci.WithImageConfig(image), withExitStatus(7)),
+				WithRuntime(plugins.RuntimeRuncV2, tc.runtimeOption),
+			)
+			require.NoError(t, err)
+			defer container.Delete(ctx, WithSnapshotCleanup)
+
+			_, err = container.NewTask(ctx, empty(), tc.taskOpts...)
+			require.NoError(t, err)
+
+			fakeTasks.Lock()
+			req := fakeTasks.createRequests[id]
+			fakeTasks.Unlock()
+
+			if tc.expectedOptions == nil {
+				require.Nil(t, req.Options)
+				return
+			}
+
+			gotOptions := &options.Options{}
+			require.NoError(t, typeurl.UnmarshalTo(req.Options, gotOptions))
+			require.True(t, cmp.Equal(tc.expectedOptions, gotOptions, protobuf.Compare))
+		})
+	}
+}
+
+type fakeTaskService struct {
+	sync.Mutex
+	createRequests map[string]*tasks.CreateTaskRequest
+	tasks.TasksClient
+}
+
+func (ts *fakeTaskService) Create(ctx context.Context, in *tasks.CreateTaskRequest, opts ...grpc.CallOption) (*tasks.CreateTaskResponse, error) {
+	ts.Lock()
+	defer ts.Unlock()
+
+	ts.createRequests[in.ContainerID] = in
+	return &tasks.CreateTaskResponse{
+		ContainerID: in.ContainerID,
+		Pid:         1,
+	}, nil
+}
+
+func (ts *fakeTaskService) Get(ctx context.Context, in *tasks.GetRequest, opts ...grpc.CallOption) (*tasks.GetResponse, error) {
+	return nil, errgrpc.ToGRPC(errdefs.ErrNotFound)
+}
+
+func (ts *fakeTaskService) Delete(ctx context.Context, in *tasks.DeleteTaskRequest, opts ...grpc.CallOption) (*tasks.DeleteResponse, error) {
+	return nil, errgrpc.ToGRPC(errdefs.ErrNotFound)
+}

integration/client/container_linux_test.go

@@ -1090,6 +1090,19 @@ func TestContainerRuntimeOptionsv2(t *testing.T) {
 	if !strings.Contains(err.Error(), `"no-runc"`) {
 		t.Errorf("task creation should have failed because of lack of executable. Instead failed with: %v", err.Error())
 	}
+
+	// It doesn't matter what the NewTaskOpts function is. We are using an existing function in the client package,
+	// which will cause the TaskOptions in the new task request to be non-empty.
+	// https://github.com/containerd/containerd/issues/11568
+	task, err = container.NewTask(ctx, empty(), WithNoNewKeyring)
+	if err == nil {
+		t.Errorf("task creation should have failed")
+		task.Delete(ctx)
+		return
+	}
+	if !strings.Contains(err.Error(), `"no-runc"`) {
+		t.Errorf("task creation should have failed because of lack of executable. Instead failed with: %v", err.Error())
+	}
 }
 
 func TestContainerKillInitPidHost(t *testing.T) {