Ensure that errors from the shim are properly translated

Golang wrapped errors are lost when they go through gRPC. Every error returned from `task.TaskService` interface should be translated using `errdefs.ToGRPC`. Fixes #7504 PiperOrigin-RevId: 447863123
2022-05-10 17:06:14 -07:00 · 2022-05-10 17:06:14 -07:00 · f62143f31f
parent f34e34b3c3
commit f62143f31f
6 changed files with 96 additions and 159 deletions
--- a/pkg/shim/proc/BUILD
+++ b/pkg/shim/proc/BUILD
@ -23,7 +23,6 @@ go_library(
        "//pkg/atomicbitops",
        "//pkg/cleanup",
        "//pkg/shim/runsc",
-        "//pkg/shim/utils",
        "@com_github_containerd_console//:go_default_library",
        "@com_github_containerd_containerd//errdefs:go_default_library",
        "@com_github_containerd_containerd//log:go_default_library",
--- a/pkg/shim/proc/init_state.go
+++ b/pkg/shim/proc/init_state.go
@ -23,7 +23,6 @@ import (
 	"github.com/containerd/containerd/pkg/process"
 	runc "github.com/containerd/go-runc"
 	"golang.org/x/sys/unix"
-	"gvisor.dev/gvisor/pkg/shim/utils"
 )

 type stateTransition int
@ -236,6 +235,6 @@ func handleStoppedKill(signal uint32) error {
 		// already been killed.
 		return nil
 	default:
-		return utils.ErrToGRPCf(errdefs.ErrNotFound, "process not found")
+		return errdefs.ToGRPCf(errdefs.ErrNotFound, "process not found")
 	}
 }
--- a/pkg/shim/service.go
+++ b/pkg/shim/service.go
@ -69,8 +69,6 @@ var (
 	}
 )

-var _ = (taskAPI.TaskService)(&service{})
-
 const (
 	// configFile is the default config file name. For containerd 1.2,
 	// we assume that a config.toml should exist in the runtime root.
@ -189,6 +187,8 @@ type service struct {
 	shimAddress string
 }

+var _ shim.Shim = (*service)(nil)
+
 func (s *service) newCommand(ctx context.Context, containerdBinary, containerdAddress string) (*exec.Cmd, error) {
 	ns, err := namespaces.NamespaceRequired(ctx)
 	if err != nil {
@ -323,6 +323,11 @@ func (s *service) Cleanup(ctx context.Context) (*taskAPI.DeleteResponse, error)
 // Create creates a new initial process and container with the underlying OCI
 // runtime.
 func (s *service) Create(ctx context.Context, r *taskAPI.CreateTaskRequest) (*taskAPI.CreateTaskResponse, error) {
+	resp, err := s.create(ctx, r)
+	return resp, errdefs.ToGRPC(err)
+}
+
+func (s *service) create(ctx context.Context, r *taskAPI.CreateTaskRequest) (*taskAPI.CreateTaskResponse, error) {
 	s.mu.Lock()
 	defer s.mu.Unlock()

@ -481,10 +486,10 @@ func (s *service) Create(ctx context.Context, r *taskAPI.CreateTaskRequest) (*ta
 	}
 	process, err := newInit(r.Bundle, filepath.Join(r.Bundle, "work"), ns, s.platform, config, &s.opts, st.Rootfs)
 	if err != nil {
-		return nil, utils.ErrToGRPC(err)
+		return nil, err
 	}
 	if err := process.Create(ctx, config); err != nil {
-		return nil, utils.ErrToGRPC(err)
+		return nil, err
 	}

 	// Set up OOM notification on the sandbox's cgroup. This is done on
@ -522,6 +527,11 @@ func (s *service) Create(ctx context.Context, r *taskAPI.CreateTaskRequest) (*ta

 // Start starts a process.
 func (s *service) Start(ctx context.Context, r *taskAPI.StartRequest) (*taskAPI.StartResponse, error) {
+	resp, err := s.start(ctx, r)
+	return resp, errdefs.ToGRPC(err)
+}
+
+func (s *service) start(ctx context.Context, r *taskAPI.StartRequest) (*taskAPI.StartResponse, error) {
 	log.L.Debugf("Start, id: %s, execID: %s", r.ID, r.ExecID)

 	p, err := s.getProcess(r.ExecID)
@ -540,6 +550,11 @@ func (s *service) Start(ctx context.Context, r *taskAPI.StartRequest) (*taskAPI.

 // Delete deletes the initial process and container.
 func (s *service) Delete(ctx context.Context, r *taskAPI.DeleteRequest) (*taskAPI.DeleteResponse, error) {
+	resp, err := s.delete(ctx, r)
+	return resp, errdefs.ToGRPC(err)
+}
+
+func (s *service) delete(ctx context.Context, r *taskAPI.DeleteRequest) (*taskAPI.DeleteResponse, error) {
 	log.L.Debugf("Delete, id: %s, execID: %s", r.ID, r.ExecID)

 	p, err := s.getProcess(r.ExecID)
@ -565,16 +580,21 @@ func (s *service) Delete(ctx context.Context, r *taskAPI.DeleteRequest) (*taskAP

 // Exec spawns an additional process inside the container.
 func (s *service) Exec(ctx context.Context, r *taskAPI.ExecProcessRequest) (*types.Empty, error) {
+	resp, err := s.exec(ctx, r)
+	return resp, errdefs.ToGRPC(err)
+}
+
+func (s *service) exec(ctx context.Context, r *taskAPI.ExecProcessRequest) (*types.Empty, error) {
 	log.L.Debugf("Exec, id: %s, execID: %s", r.ID, r.ExecID)

 	s.mu.Lock()
 	p := s.processes[r.ExecID]
 	s.mu.Unlock()
 	if p != nil {
-		return nil, utils.ErrToGRPCf(errdefs.ErrAlreadyExists, "id %s", r.ExecID)
+		return nil, errdefs.ToGRPCf(errdefs.ErrAlreadyExists, "id %s", r.ExecID)
 	}
 	if s.task == nil {
-		return nil, utils.ErrToGRPCf(errdefs.ErrFailedPrecondition, "container must be created")
+		return nil, errdefs.ToGRPCf(errdefs.ErrFailedPrecondition, "container must be created")
 	}
 	process, err := s.task.Exec(ctx, s.bundle, &proc.ExecConfig{
 		ID:       r.ExecID,
@ -585,7 +605,7 @@ func (s *service) Exec(ctx context.Context, r *taskAPI.ExecProcessRequest) (*typ
 		Spec:     r.Spec,
 	})
 	if err != nil {
-		return nil, utils.ErrToGRPC(err)
+		return nil, err
 	}
 	s.mu.Lock()
 	s.processes[r.ExecID] = process
@ -595,6 +615,11 @@ func (s *service) Exec(ctx context.Context, r *taskAPI.ExecProcessRequest) (*typ

 // ResizePty resizes the terminal of a process.
 func (s *service) ResizePty(ctx context.Context, r *taskAPI.ResizePtyRequest) (*types.Empty, error) {
+	resp, err := s.resizePty(ctx, r)
+	return resp, errdefs.ToGRPC(err)
+}
+
+func (s *service) resizePty(ctx context.Context, r *taskAPI.ResizePtyRequest) (*types.Empty, error) {
 	log.L.Debugf("ResizePty, id: %s, execID: %s, dimension: %dx%d", r.ID, r.ExecID, r.Height, r.Width)

 	p, err := s.getProcess(r.ExecID)
@ -606,13 +631,18 @@ func (s *service) ResizePty(ctx context.Context, r *taskAPI.ResizePtyRequest) (*
 		Height: uint16(r.Height),
 	}
 	if err := p.Resize(ws); err != nil {
-		return nil, utils.ErrToGRPC(err)
+		return nil, err
 	}
 	return empty, nil
 }

 // State returns runtime state information for a process.
 func (s *service) State(ctx context.Context, r *taskAPI.StateRequest) (*taskAPI.StateResponse, error) {
+	resp, err := s.state(ctx, r)
+	return resp, errdefs.ToGRPC(err)
+}
+
+func (s *service) state(ctx context.Context, r *taskAPI.StateRequest) (*taskAPI.StateResponse, error) {
 	log.L.Debugf("State, id: %s, execID: %s", r.ID, r.ExecID)

 	p, err := s.getProcess(r.ExecID)
@ -653,10 +683,15 @@ func (s *service) State(ctx context.Context, r *taskAPI.StateRequest) (*taskAPI.

 // Pause the container.
 func (s *service) Pause(ctx context.Context, r *taskAPI.PauseRequest) (*types.Empty, error) {
+	resp, err := s.pause(ctx, r)
+	return resp, errdefs.ToGRPC(err)
+}
+
+func (s *service) pause(ctx context.Context, r *taskAPI.PauseRequest) (*types.Empty, error) {
 	log.L.Debugf("Pause, id: %s", r.ID)
 	if s.task == nil {
 		log.L.Debugf("Pause error, id: %s: container not created", r.ID)
-		return nil, utils.ErrToGRPCf(errdefs.ErrFailedPrecondition, "container must be created")
+		return nil, errdefs.ToGRPCf(errdefs.ErrFailedPrecondition, "container must be created")
 	}
 	err := s.task.Runtime().Pause(ctx, r.ID)
 	if err != nil {
@ -667,10 +702,15 @@ func (s *service) Pause(ctx context.Context, r *taskAPI.PauseRequest) (*types.Em

 // Resume the container.
 func (s *service) Resume(ctx context.Context, r *taskAPI.ResumeRequest) (*types.Empty, error) {
+	resp, err := s.resume(ctx, r)
+	return resp, errdefs.ToGRPC(err)
+}
+
+func (s *service) resume(ctx context.Context, r *taskAPI.ResumeRequest) (*types.Empty, error) {
 	log.L.Debugf("Resume, id: %s", r.ID)
 	if s.task == nil {
 		log.L.Debugf("Resume error, id: %s: container not created", r.ID)
-		return nil, utils.ErrToGRPCf(errdefs.ErrFailedPrecondition, "container must be created")
+		return nil, errdefs.ToGRPCf(errdefs.ErrFailedPrecondition, "container must be created")
 	}
 	err := s.task.Runtime().Resume(ctx, r.ID)
 	if err != nil {
@ -681,6 +721,11 @@ func (s *service) Resume(ctx context.Context, r *taskAPI.ResumeRequest) (*types.

 // Kill a process with the provided signal.
 func (s *service) Kill(ctx context.Context, r *taskAPI.KillRequest) (*types.Empty, error) {
+	resp, err := s.kill(ctx, r)
+	return resp, errdefs.ToGRPC(err)
+}
+
+func (s *service) kill(ctx context.Context, r *taskAPI.KillRequest) (*types.Empty, error) {
 	log.L.Debugf("Kill, id: %s, execID: %s, signal: %d, all: %t", r.ID, r.ExecID, r.Signal, r.All)

 	p, err := s.getProcess(r.ExecID)
@ -689,7 +734,7 @@ func (s *service) Kill(ctx context.Context, r *taskAPI.KillRequest) (*types.Empt
 	}
 	if err := p.Kill(ctx, r.Signal, r.All); err != nil {
 		log.L.Debugf("Kill failed: %v", err)
-		return nil, utils.ErrToGRPC(err)
+		return nil, err
 	}
 	log.L.Debugf("Kill succeeded")
 	return empty, nil
@ -697,11 +742,16 @@ func (s *service) Kill(ctx context.Context, r *taskAPI.KillRequest) (*types.Empt

 // Pids returns all pids inside the container.
 func (s *service) Pids(ctx context.Context, r *taskAPI.PidsRequest) (*taskAPI.PidsResponse, error) {
+	resp, err := s.pids(ctx, r)
+	return resp, errdefs.ToGRPC(err)
+}
+
+func (s *service) pids(ctx context.Context, r *taskAPI.PidsRequest) (*taskAPI.PidsResponse, error) {
 	log.L.Debugf("Pids, id: %s", r.ID)

 	pids, err := s.getContainerPids(ctx, r.ID)
 	if err != nil {
-		return nil, utils.ErrToGRPC(err)
+		return nil, err
 	}
 	var processes []*task.ProcessInfo
 	for _, pid := range pids {
@ -730,6 +780,11 @@ func (s *service) Pids(ctx context.Context, r *taskAPI.PidsRequest) (*taskAPI.Pi

 // CloseIO closes the I/O context of a process.
 func (s *service) CloseIO(ctx context.Context, r *taskAPI.CloseIORequest) (*types.Empty, error) {
+	resp, err := s.closeIO(ctx, r)
+	return resp, errdefs.ToGRPC(err)
+}
+
+func (s *service) closeIO(ctx context.Context, r *taskAPI.CloseIORequest) (*types.Empty, error) {
 	log.L.Debugf("CloseIO, id: %s, execID: %s, stdin: %t", r.ID, r.ExecID, r.Stdin)

 	p, err := s.getProcess(r.ExecID)
@ -747,11 +802,16 @@ func (s *service) CloseIO(ctx context.Context, r *taskAPI.CloseIORequest) (*type
 // Checkpoint checkpoints the container.
 func (s *service) Checkpoint(ctx context.Context, r *taskAPI.CheckpointTaskRequest) (*types.Empty, error) {
 	log.L.Debugf("Checkpoint, id: %s", r.ID)
-	return empty, utils.ErrToGRPC(errdefs.ErrNotImplemented)
+	return empty, errdefs.ToGRPC(errdefs.ErrNotImplemented)
 }

 // Connect returns shim information such as the shim's pid.
 func (s *service) Connect(ctx context.Context, r *taskAPI.ConnectRequest) (*taskAPI.ConnectResponse, error) {
+	resp, err := s.connect(ctx, r)
+	return resp, errdefs.ToGRPC(err)
+}
+
+func (s *service) connect(ctx context.Context, r *taskAPI.ConnectRequest) (*taskAPI.ConnectResponse, error) {
 	log.L.Debugf("Connect, id: %s", r.ID)

 	var pid int
@ -765,6 +825,11 @@ func (s *service) Connect(ctx context.Context, r *taskAPI.ConnectRequest) (*task
 }

 func (s *service) Shutdown(ctx context.Context, r *taskAPI.ShutdownRequest) (*types.Empty, error) {
+	resp, err := s.shutdown(ctx, r)
+	return resp, errdefs.ToGRPC(err)
+}
+
+func (s *service) shutdown(ctx context.Context, r *taskAPI.ShutdownRequest) (*types.Empty, error) {
 	log.L.Debugf("Shutdown, id: %s", r.ID)
 	s.cancel()
 	if s.shimAddress != "" {
@ -775,10 +840,15 @@ func (s *service) Shutdown(ctx context.Context, r *taskAPI.ShutdownRequest) (*ty
 }

 func (s *service) Stats(ctx context.Context, r *taskAPI.StatsRequest) (*taskAPI.StatsResponse, error) {
+	resp, err := s.stats(ctx, r)
+	return resp, errdefs.ToGRPC(err)
+}
+
+func (s *service) stats(ctx context.Context, r *taskAPI.StatsRequest) (*taskAPI.StatsResponse, error) {
 	log.L.Debugf("Stats, id: %s", r.ID)
 	if s.task == nil {
 		log.L.Debugf("Stats error, id: %s: container not created", r.ID)
-		return nil, utils.ErrToGRPCf(errdefs.ErrFailedPrecondition, "container must be created")
+		return nil, errdefs.ToGRPCf(errdefs.ErrFailedPrecondition, "container must be created")
 	}
 	stats, err := s.task.Stats(ctx, s.id)
 	if err != nil {
@ -852,11 +922,16 @@ func (s *service) Stats(ctx context.Context, r *taskAPI.StatsRequest) (*taskAPI.

 // Update updates a running container.
 func (s *service) Update(ctx context.Context, r *taskAPI.UpdateTaskRequest) (*types.Empty, error) {
-	return empty, utils.ErrToGRPC(errdefs.ErrNotImplemented)
+	return empty, errdefs.ToGRPC(errdefs.ErrNotImplemented)
 }

 // Wait waits for a process to exit.
 func (s *service) Wait(ctx context.Context, r *taskAPI.WaitRequest) (*taskAPI.WaitResponse, error) {
+	resp, err := s.wait(ctx, r)
+	return resp, errdefs.ToGRPC(err)
+}
+
+func (s *service) wait(ctx context.Context, r *taskAPI.WaitRequest) (*taskAPI.WaitResponse, error) {
 	log.L.Debugf("Wait, id: %s, execID: %s", r.ID, r.ExecID)

 	p, err := s.getProcess(r.ExecID)
@ -949,14 +1024,14 @@ func (s *service) getProcess(execID string) (process.Process, error) {

 	if execID == "" {
 		if s.task == nil {
-			return nil, utils.ErrToGRPCf(errdefs.ErrFailedPrecondition, "container must be created")
+			return nil, errdefs.ToGRPCf(errdefs.ErrFailedPrecondition, "container must be created")
 		}
 		return s.task, nil
 	}

 	p := s.processes[execID]
 	if p == nil {
-		return nil, utils.ErrToGRPCf(errdefs.ErrNotFound, "process does not exist %s", execID)
+		return nil, errdefs.ToGRPCf(errdefs.ErrNotFound, "process does not exist %s", execID)
 	}
 	return p, nil
 }
--- a/pkg/shim/utils/BUILD
+++ b/pkg/shim/utils/BUILD
@ -6,7 +6,6 @@ go_library(
    name = "utils",
    srcs = [
        "annotations.go",
-        "errors.go",
        "utils.go",
        "volumes.go",
    ],
@ -14,24 +13,13 @@ go_library(
        "//pkg/shim:__subpackages__",
        "//shim:__subpackages__",
    ],
-    deps = [
-        "@com_github_containerd_containerd//errdefs:go_default_library",
-        "@com_github_opencontainers_runtime_spec//specs-go:go_default_library",
-        "@org_golang_google_grpc//codes:go_default_library",
-        "@org_golang_google_grpc//status:go_default_library",
-    ],
+    deps = ["@com_github_opencontainers_runtime_spec//specs-go:go_default_library"],
 )

 go_test(
    name = "utils_test",
    size = "small",
-    srcs = [
-        "errors_test.go",
-        "volumes_test.go",
-    ],
+    srcs = ["volumes_test.go"],
    library = ":utils",
-    deps = [
-        "@com_github_containerd_containerd//errdefs:go_default_library",
-        "@com_github_opencontainers_runtime_spec//specs-go:go_default_library",
-    ],
+    deps = ["@com_github_opencontainers_runtime_spec//specs-go:go_default_library"],
 )
--- a/pkg/shim/utils/errors.go
+++ b/pkg/shim/utils/errors.go
@ -1,74 +0,0 @@
-// Copyright 2021 The gVisor Authors.
-//
-// Licensed under the Apache License, Version 2.0 (the "License");
-// you may not use this file except in compliance with the License.
-// You may obtain a copy of the License at
-//
-//     https://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing, software
-// distributed under the License is distributed on an "AS IS" BASIS,
-// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-// See the License for the specific language governing permissions and
-// limitations under the License.
-
-package utils
-
-import (
-	"context"
-	"errors"
-	"fmt"
-
-	"github.com/containerd/containerd/errdefs"
-	"google.golang.org/grpc/codes"
-	"google.golang.org/grpc/status"
-)
-
-// ErrToGRPC wraps containerd's ToGRPC error mapper which depends on
-// github.com/pkg/errors to work correctly. Once we upgrade to containerd v1.4,
-// this function can go away and we can use errdefs.ToGRPC directly instead.
-//
-// TODO(gvisor.dev/issue/6232): Remove after upgrading to containerd v1.4
-func ErrToGRPC(err error) error {
-	return errToGRPCMsg(err, err.Error())
-}
-
-// ErrToGRPCf maps the error to grpc error codes, assembling the formatting
-// string and combining it with the target error string.
-//
-// TODO(gvisor.dev/issue/6232): Remove after upgrading to containerd v1.4
-func ErrToGRPCf(err error, format string, args ...interface{}) error {
-	formatted := fmt.Sprintf(format, args...)
-	msg := fmt.Sprintf("%s: %s", formatted, err.Error())
-	return errToGRPCMsg(err, msg)
-}
-
-func errToGRPCMsg(err error, msg string) error {
-	if err == nil {
-		return nil
-	}
-	if _, ok := status.FromError(err); ok {
-		return err
-	}
-
-	switch {
-	case errors.Is(err, errdefs.ErrInvalidArgument):
-		return status.Errorf(codes.InvalidArgument, msg)
-	case errors.Is(err, errdefs.ErrNotFound):
-		return status.Errorf(codes.NotFound, msg)
-	case errors.Is(err, errdefs.ErrAlreadyExists):
-		return status.Errorf(codes.AlreadyExists, msg)
-	case errors.Is(err, errdefs.ErrFailedPrecondition):
-		return status.Errorf(codes.FailedPrecondition, msg)
-	case errors.Is(err, errdefs.ErrUnavailable):
-		return status.Errorf(codes.Unavailable, msg)
-	case errors.Is(err, errdefs.ErrNotImplemented):
-		return status.Errorf(codes.Unimplemented, msg)
-	case errors.Is(err, context.Canceled):
-		return status.Errorf(codes.Canceled, msg)
-	case errors.Is(err, context.DeadlineExceeded):
-		return status.Errorf(codes.DeadlineExceeded, msg)
-	}
-
-	return errdefs.ToGRPC(err)
-}
--- a/pkg/shim/utils/errors_test.go
+++ b/pkg/shim/utils/errors_test.go
@ -1,50 +0,0 @@
-// Copyright 2021 The gVisor Authors.
-//
-// Licensed under the Apache License, Version 2.0 (the "License");
-// you may not use this file except in compliance with the License.
-// You may obtain a copy of the License at
-//
-//     https://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing, software
-// distributed under the License is distributed on an "AS IS" BASIS,
-// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-// See the License for the specific language governing permissions and
-// limitations under the License.
-
-package utils
-
-import (
-	"fmt"
-	"testing"
-
-	"github.com/containerd/containerd/errdefs"
-)
-
-func TestGRPCRoundTripsErrors(t *testing.T) {
-	for _, tc := range []struct {
-		name string
-		err  error
-		test func(err error) bool
-	}{
-		{
-			name: "passthrough",
-			err:  errdefs.ErrNotFound,
-			test: errdefs.IsNotFound,
-		},
-		{
-			name: "wrapped",
-			err:  fmt.Errorf("oh no: %w", errdefs.ErrNotFound),
-			test: errdefs.IsNotFound,
-		},
-	} {
-		t.Run(tc.name, func(t *testing.T) {
-			if err := errdefs.FromGRPC(ErrToGRPC(tc.err)); !tc.test(err) {
-				t.Errorf("errToGRPC got %+v", err)
-			}
-			if err := errdefs.FromGRPC(ErrToGRPCf(tc.err, "testing %s", "123")); !tc.test(err) {
-				t.Errorf("errToGRPCf got %+v", err)
-			}
-		})
-	}
-}