From 99f9230e3fbd3b3ede5628eeeb8458175bc9c1a0 Mon Sep 17 00:00:00 2001 From: Noah Fontes Date: Tue, 22 Jun 2021 14:17:26 -0700 Subject: Ensure shim propagates errors over gRPC correctly This change wraps containerd's errdefs.ToGRPC function with one that understands Go 1.13-style error wrapping style, which is used pervasively throughout the shim. With this change, errors that have been marked with, e.g., `errdefs.ErrNotFound`, will be correctly propagated back to the containerd server. --- pkg/shim/BUILD | 8 ++++++- pkg/shim/errors.go | 56 +++++++++++++++++++++++++++++++++++++++++++++++++ pkg/shim/errors_test.go | 47 +++++++++++++++++++++++++++++++++++++++++ pkg/shim/service.go | 12 +++++------ 4 files changed, 116 insertions(+), 7 deletions(-) create mode 100644 pkg/shim/errors.go create mode 100644 pkg/shim/errors_test.go diff --git a/pkg/shim/BUILD b/pkg/shim/BUILD index 367765209..d8086688c 100644 --- a/pkg/shim/BUILD +++ b/pkg/shim/BUILD @@ -8,6 +8,7 @@ go_library( "api.go", "debug.go", "epoll.go", + "errors.go", "options.go", "service.go", "service_linux.go", @@ -44,6 +45,8 @@ 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", ], ) @@ -51,7 +54,10 @@ go_library( go_test( name = "shim_test", size = "small", - srcs = ["service_test.go"], + srcs = [ + "errors_test.go", + "service_test.go", + ], library = ":shim", deps = [ "//pkg/shim/utils", diff --git a/pkg/shim/errors.go b/pkg/shim/errors.go new file mode 100644 index 000000000..311a4b427 --- /dev/null +++ b/pkg/shim/errors.go @@ -0,0 +1,56 @@ +// 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 shim + +import ( + "context" + "errors" + + "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 { + if _, ok := status.FromError(err); ok { + return err + } + + switch { + case errors.Is(err, errdefs.ErrInvalidArgument): + return status.Errorf(codes.InvalidArgument, err.Error()) + case errors.Is(err, errdefs.ErrNotFound): + return status.Errorf(codes.NotFound, err.Error()) + case errors.Is(err, errdefs.ErrAlreadyExists): + return status.Errorf(codes.AlreadyExists, err.Error()) + case errors.Is(err, errdefs.ErrFailedPrecondition): + return status.Errorf(codes.FailedPrecondition, err.Error()) + case errors.Is(err, errdefs.ErrUnavailable): + return status.Errorf(codes.Unavailable, err.Error()) + case errors.Is(err, errdefs.ErrNotImplemented): + return status.Errorf(codes.Unimplemented, err.Error()) + case errors.Is(err, context.Canceled): + return status.Errorf(codes.Canceled, err.Error()) + case errors.Is(err, context.DeadlineExceeded): + return status.Errorf(codes.DeadlineExceeded, err.Error()) + } + + return errdefs.ToGRPC(err) +} diff --git a/pkg/shim/errors_test.go b/pkg/shim/errors_test.go new file mode 100644 index 000000000..3c10866cc --- /dev/null +++ b/pkg/shim/errors_test.go @@ -0,0 +1,47 @@ +// 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 shim + +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("got %+v", err) + } + }) + } +} diff --git a/pkg/shim/service.go b/pkg/shim/service.go index ea9a1ae10..0b41f0e72 100644 --- a/pkg/shim/service.go +++ b/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, errdefs.ToGRPC(err) + return nil, errToGRPC(err) } if err := process.Create(ctx, config); err != nil { - return nil, errdefs.ToGRPC(err) + return nil, errToGRPC(err) } // Set up OOM notification on the sandbox's cgroup. This is done on @@ -544,7 +544,7 @@ func (s *service) Exec(ctx context.Context, r *taskAPI.ExecProcessRequest) (*typ Spec: r.Spec, }) if err != nil { - return nil, errdefs.ToGRPC(err) + return nil, 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, errdefs.ToGRPC(err) + return nil, errToGRPC(err) } return empty, 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, errdefs.ToGRPC(err) + return nil, 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, errdefs.ToGRPC(err) + return nil, errToGRPC(err) } var processes []*task.ProcessInfo for _, pid := range pids { -- cgit v1.2.3