From f62143f31fe5596241a844aead15aefe9db9c816 Mon Sep 17 00:00:00 2001 From: Fabricio Voznika Date: Tue, 10 May 2022 17:06:14 -0700 Subject: [PATCH] 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 --- pkg/shim/proc/BUILD | 1 - pkg/shim/proc/init_state.go | 3 +- pkg/shim/service.go | 109 ++++++++++++++++++++++++++++------ pkg/shim/utils/BUILD | 18 +----- pkg/shim/utils/errors.go | 74 ----------------------- pkg/shim/utils/errors_test.go | 50 ---------------- 6 files changed, 96 insertions(+), 159 deletions(-) delete mode 100644 pkg/shim/utils/errors.go delete mode 100644 pkg/shim/utils/errors_test.go diff --git a/pkg/shim/proc/BUILD b/pkg/shim/proc/BUILD index 653596ea5..eec851936 100644 --- 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", diff --git a/pkg/shim/proc/init_state.go b/pkg/shim/proc/init_state.go index 5347ddefe..d65020e76 100644 --- 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") } } diff --git a/pkg/shim/service.go b/pkg/shim/service.go index 68966afdf..3ae2816eb 100644 --- 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 } diff --git a/pkg/shim/utils/BUILD b/pkg/shim/utils/BUILD index 2eb82f63c..cc98b1e02 100644 --- 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"], ) diff --git a/pkg/shim/utils/errors.go b/pkg/shim/utils/errors.go deleted file mode 100644 index 971d68c36..000000000 --- a/pkg/shim/utils/errors.go +++ /dev/null @@ -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) -} diff --git a/pkg/shim/utils/errors_test.go b/pkg/shim/utils/errors_test.go deleted file mode 100644 index 0a8fe34c8..000000000 --- a/pkg/shim/utils/errors_test.go +++ /dev/null @@ -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) - } - }) - } -}