Redirect all calls from `errdefs.ToGRPC` to `utils.ErrToGRPC`

This is to ensure that Go 1.13 error wrapping is correctly
translated to gRPC errors before returning from the shim.

Updates #6225

PiperOrigin-RevId: 382120441

pkg/shim/BUILD

@@ -8,7 +8,6 @@ go_library(
         "api.go",
         "debug.go",
         "epoll.go",
-        "errors.go",
         "options.go",
         "service.go",
         "service_linux.go",
@@ -45,8 +44,6 @@ go_library(
         "@com_github_gogo_protobuf//types:go_default_library",
         "@com_github_opencontainers_runtime_spec//specs-go:go_default_library",
         "@com_github_sirupsen_logrus//:go_default_library",
-        "@org_golang_google_grpc//codes:go_default_library",
-        "@org_golang_google_grpc//status:go_default_library",
         "@org_golang_x_sys//unix:go_default_library",
     ],
 )
@@ -54,14 +51,10 @@ go_library(
 go_test(
     name = "shim_test",
     size = "small",
-    srcs = [
+    srcs = ["service_test.go"],
-        "errors_test.go",
-        "service_test.go",
-    ],
     library = ":shim",
     deps = [
         "//pkg/shim/utils",
-        "@com_github_containerd_containerd//errdefs:go_default_library",
         "@com_github_opencontainers_runtime_spec//specs-go:go_default_library",
     ],
 )

pkg/shim/proc/BUILD

@@ -21,6 +21,7 @@ go_library(
     ],
     deps = [
         "//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",

pkg/shim/proc/init_state.go

@@ -23,6 +23,7 @@ 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
@@ -235,6 +236,6 @@ func handleStoppedKill(signal uint32) error {
 		// already been killed.
 		return nil
 	default:
-		return errdefs.ToGRPCf(errdefs.ErrNotFound, "process not found")
+		return utils.ErrToGRPCf(errdefs.ErrNotFound, "process not found")
 	}
 }

pkg/shim/service.go

@@ -452,10 +452,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, errToGRPC(err)
+		return nil, utils.ErrToGRPC(err)
 	}
 	if err := process.Create(ctx, config); err != nil {
-		return nil, errToGRPC(err)
+		return nil, utils.ErrToGRPC(err)
 	}
 
 	// Set up OOM notification on the sandbox's cgroup. This is done on
@@ -530,10 +530,10 @@ func (s *service) Exec(ctx context.Context, r *taskAPI.ExecProcessRequest) (*typ
 	p := s.processes[r.ExecID]
 	s.mu.Unlock()
 	if p != nil {
-		return nil, errdefs.ToGRPCf(errdefs.ErrAlreadyExists, "id %s", r.ExecID)
+		return nil, utils.ErrToGRPCf(errdefs.ErrAlreadyExists, "id %s", r.ExecID)
 	}
 	if s.task == nil {
-		return nil, errdefs.ToGRPCf(errdefs.ErrFailedPrecondition, "container must be created")
+		return nil, utils.ErrToGRPCf(errdefs.ErrFailedPrecondition, "container must be created")
 	}
 	process, err := s.task.Exec(ctx, s.bundle, &proc.ExecConfig{
 		ID:       r.ExecID,
@@ -544,7 +544,7 @@ func (s *service) Exec(ctx context.Context, r *taskAPI.ExecProcessRequest) (*typ
 		Spec:     r.Spec,
 	})
 	if err != nil {
-		return nil, errToGRPC(err)
+		return nil, utils.ErrToGRPC(err)
 	}
 	s.mu.Lock()
 	s.processes[r.ExecID] = process
@@ -565,7 +565,7 @@ func (s *service) ResizePty(ctx context.Context, r *taskAPI.ResizePtyRequest) (*
 		Height: uint16(r.Height),
 	}
 	if err := p.Resize(ws); err != nil {
-		return nil, errToGRPC(err)
+		return nil, utils.ErrToGRPC(err)
 	}
 	return empty, nil
 }
@@ -615,7 +615,7 @@ func (s *service) Pause(ctx context.Context, r *taskAPI.PauseRequest) (*types.Em
 	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, errdefs.ToGRPCf(errdefs.ErrFailedPrecondition, "container must be created")
+		return nil, utils.ErrToGRPCf(errdefs.ErrFailedPrecondition, "container must be created")
 	}
 	err := s.task.Runtime().Pause(ctx, r.ID)
 	if err != nil {
@@ -629,7 +629,7 @@ func (s *service) Resume(ctx context.Context, r *taskAPI.ResumeRequest) (*types.
 	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, errdefs.ToGRPCf(errdefs.ErrFailedPrecondition, "container must be created")
+		return nil, utils.ErrToGRPCf(errdefs.ErrFailedPrecondition, "container must be created")
 	}
 	err := s.task.Runtime().Resume(ctx, r.ID)
 	if err != nil {
@@ -648,7 +648,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, errToGRPC(err)
+		return nil, utils.ErrToGRPC(err)
 	}
 	log.L.Debugf("Kill succeeded")
 	return empty, nil
@@ -660,7 +660,7 @@ func (s *service) Pids(ctx context.Context, r *taskAPI.PidsRequest) (*taskAPI.Pi
 
 	pids, err := s.getContainerPids(ctx, r.ID)
 	if err != nil {
-		return nil, errToGRPC(err)
+		return nil, utils.ErrToGRPC(err)
 	}
 	var processes []*task.ProcessInfo
 	for _, pid := range pids {
@@ -706,7 +706,7 @@ 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, errdefs.ToGRPC(errdefs.ErrNotImplemented)
+	return empty, utils.ErrToGRPC(errdefs.ErrNotImplemented)
 }
 
 // Connect returns shim information such as the shim's pid.
@@ -737,7 +737,7 @@ func (s *service) Stats(ctx context.Context, r *taskAPI.StatsRequest) (*taskAPI.
 	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, errdefs.ToGRPCf(errdefs.ErrFailedPrecondition, "container must be created")
+		return nil, utils.ErrToGRPCf(errdefs.ErrFailedPrecondition, "container must be created")
 	}
 	stats, err := s.task.Stats(ctx, s.id)
 	if err != nil {
@@ -811,7 +811,7 @@ 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, errdefs.ToGRPC(errdefs.ErrNotImplemented)
+	return empty, utils.ErrToGRPC(errdefs.ErrNotImplemented)
 }
 
 // Wait waits for a process to exit.
@@ -908,14 +908,14 @@ func (s *service) getProcess(execID string) (process.Process, error) {
 
 	if execID == "" {
 		if s.task == nil {
-			return nil, errdefs.ToGRPCf(errdefs.ErrFailedPrecondition, "container must be created")
+			return nil, utils.ErrToGRPCf(errdefs.ErrFailedPrecondition, "container must be created")
 		}
 		return s.task, nil
 	}
 
 	p := s.processes[execID]
 	if p == nil {
-		return nil, errdefs.ToGRPCf(errdefs.ErrNotFound, "process does not exist %s", execID)
+		return nil, utils.ErrToGRPCf(errdefs.ErrNotFound, "process does not exist %s", execID)
 	}
 	return p, nil
 }

pkg/shim/utils/BUILD

@@ -6,6 +6,7 @@ go_library(
     name = "utils",
     srcs = [
         "annotations.go",
+        "errors.go",
         "utils.go",
         "volumes.go",
     ],
@@ -14,14 +15,23 @@ go_library(
         "//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",
     ],
 )
 
 go_test(
     name = "utils_test",
     size = "small",
-    srcs = ["volumes_test.go"],
+    srcs = [
+        "errors_test.go",
+        "volumes_test.go",
+    ],
     library = ":utils",
-    deps = ["@com_github_opencontainers_runtime_spec//specs-go:go_default_library"],
+    deps = [
+        "@com_github_containerd_containerd//errdefs:go_default_library",
+        "@com_github_opencontainers_runtime_spec//specs-go:go_default_library",
+    ],
 )

pkg/shim/utils/errors.go

@@ -12,23 +12,38 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-package shim
+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
+// 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 {
+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
 	}
@@ -38,21 +53,21 @@ func errToGRPC(err error) error {
 
 	switch {
 	case errors.Is(err, errdefs.ErrInvalidArgument):
-		return status.Errorf(codes.InvalidArgument, err.Error())
+		return status.Errorf(codes.InvalidArgument, msg)
 	case errors.Is(err, errdefs.ErrNotFound):
-		return status.Errorf(codes.NotFound, err.Error())
+		return status.Errorf(codes.NotFound, msg)
 	case errors.Is(err, errdefs.ErrAlreadyExists):
-		return status.Errorf(codes.AlreadyExists, err.Error())
+		return status.Errorf(codes.AlreadyExists, msg)
 	case errors.Is(err, errdefs.ErrFailedPrecondition):
-		return status.Errorf(codes.FailedPrecondition, err.Error())
+		return status.Errorf(codes.FailedPrecondition, msg)
 	case errors.Is(err, errdefs.ErrUnavailable):
-		return status.Errorf(codes.Unavailable, err.Error())
+		return status.Errorf(codes.Unavailable, msg)
 	case errors.Is(err, errdefs.ErrNotImplemented):
-		return status.Errorf(codes.Unimplemented, err.Error())
+		return status.Errorf(codes.Unimplemented, msg)
 	case errors.Is(err, context.Canceled):
-		return status.Errorf(codes.Canceled, err.Error())
+		return status.Errorf(codes.Canceled, msg)
 	case errors.Is(err, context.DeadlineExceeded):
-		return status.Errorf(codes.DeadlineExceeded, err.Error())
+		return status.Errorf(codes.DeadlineExceeded, msg)
 	}
 
 	return errdefs.ToGRPC(err)

pkg/shim/utils/errors_test.go

@@ -12,7 +12,7 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-package shim
+package utils
 
 import (
 	"fmt"
@@ -39,8 +39,11 @@ func TestGRPCRoundTripsErrors(t *testing.T) {
 		},
 	} {
 		t.Run(tc.name, func(t *testing.T) {
-			if err := errdefs.FromGRPC(errToGRPC(tc.err)); !tc.test(err) {
+			if err := errdefs.FromGRPC(ErrToGRPC(tc.err)); !tc.test(err) {
-				t.Errorf("got %+v", 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)
 			}
 		})
 	}