From 0b20b2fec11852951f0d8d0d55765ae2cd4ec552 Mon Sep 17 00:00:00 2001 From: Alan Parra Date: Tue, 21 Mar 2017 11:34:40 +0000 Subject: [PATCH] Adding TrillianError and related utilities. (#450) * Adding TrillianError and related utilities. TrillianError allows us to represent errors with associated status code without having to rely on GRPC. Utilities for creating errors and converting to GRPC have been added as well. * Review comments * Fetch test dependencies on Travis * Review comments (pt2) --- .travis.yml | 2 +- errors/doc.go | 20 ++++ errors/errors.go | 175 ++++++++++++++++++++++++++++++ errors/errors_test.go | 96 ++++++++++++++++ server/errors/doc.go | 16 +++ server/errors/errors.go | 34 ++++++ server/errors/errors_test.go | 55 ++++++++++ server/sequencer_manager_test.go | 2 +- storage/mysql/log_storage.go | 2 +- storage/mysql/log_storage_test.go | 6 +- 10 files changed, 402 insertions(+), 6 deletions(-) create mode 100644 errors/doc.go create mode 100644 errors/errors.go create mode 100644 errors/errors_test.go create mode 100644 server/errors/doc.go create mode 100644 server/errors/errors.go create mode 100644 server/errors/errors_test.go diff --git a/.travis.yml b/.travis.yml index 478c2d9c41..b0bf48f3bd 100644 --- a/.travis.yml +++ b/.travis.yml @@ -27,7 +27,7 @@ install: - export PATH=$(pwd)/../protoc/bin:$PATH # googleapis is not Go code, but it's required for .pb.go regeneration because of API dependencies. - git clone https://github.com/googleapis/googleapis.git $GOPATH/src/github.com/googleapis/googleapis - - go get -d ./... + - go get -d -t ./... - if [[ $TRAVIS_OS_NAME == "osx" ]]; then brew update > /dev/null && brew install mariadb && mysql.server start; fi - go get -u github.com/client9/misspell/cmd/misspell - go get -u github.com/fzipp/gocyclo diff --git a/errors/doc.go b/errors/doc.go new file mode 100644 index 0000000000..52409ce5c8 --- /dev/null +++ b/errors/doc.go @@ -0,0 +1,20 @@ +// Copyright 2017 Google Inc. All Rights Reserved. +// +// 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 +// +// http://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 errors defines an error representation that associates an error +// message to an error code. +// It's meant to allow translation to other kinds of errors (e.g., gRPC errors) +// without information loss, while at the same time keeping the implementation +// independent of specific libraries. +package errors diff --git a/errors/errors.go b/errors/errors.go new file mode 100644 index 0000000000..e7ac7e3331 --- /dev/null +++ b/errors/errors.go @@ -0,0 +1,175 @@ +// Copyright 2017 Google Inc. All Rights Reserved. +// +// 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 +// +// http://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 errors + +import "fmt" + +// Code mirrors gRPC's codes.Code. +type Code uint32 + +// Descriptions are copied from +// https://github.com/grpc/grpc-go/blob/master/codes/codes.go for developer +// convenience. +const ( + // Canceled indicates the operation was cancelled (typically by the caller). + Canceled Code = 1 + + // Unknown error. An example of where this error may be returned is + // if a Status value received from another address space belongs to + // an error-space that is not known in this address space. Also + // errors raised by APIs that do not return enough error information + // may be converted to this error. + Unknown Code = 2 + + // InvalidArgument indicates client specified an invalid argument. + // Note that this differs from FailedPrecondition. It indicates arguments + // that are problematic regardless of the state of the system + // (e.g., a malformed file name). + InvalidArgument Code = 3 + + // DeadlineExceeded means operation expired before completion. + // For operations that change the state of the system, this error may be + // returned even if the operation has completed successfully. For + // example, a successful response from a server could have been delayed + // long enough for the deadline to expire. + DeadlineExceeded Code = 4 + + // NotFound means some requested entity (e.g., file or directory) was + // not found. + NotFound Code = 5 + + // AlreadyExists means an attempt to create an entity failed because one + // already exists. + AlreadyExists Code = 6 + + // PermissionDenied indicates the caller does not have permission to + // execute the specified operation. It must not be used for rejections + // caused by exhausting some resource (use ResourceExhausted + // instead for those errors). It must not be + // used if the caller cannot be identified (use Unauthenticated + // instead for those errors). + PermissionDenied Code = 7 + + // Unauthenticated indicates the request does not have valid + // authentication credentials for the operation. + Unauthenticated Code = 16 + + // ResourceExhausted indicates some resource has been exhausted, perhaps + // a per-user quota, or perhaps the entire file system is out of space. + ResourceExhausted Code = 8 + + // FailedPrecondition indicates operation was rejected because the + // system is not in a state required for the operation's execution. + // For example, directory to be deleted may be non-empty, an rmdir + // operation is applied to a non-directory, etc. + // + // A litmus test that may help a service implementor in deciding + // between FailedPrecondition, Aborted, and Unavailable: + // (a) Use Unavailable if the client can retry just the failing call. + // (b) Use Aborted if the client should retry at a higher-level + // (e.g., restarting a read-modify-write sequence). + // (c) Use FailedPrecondition if the client should not retry until + // the system state has been explicitly fixed. E.g., if an "rmdir" + // fails because the directory is non-empty, FailedPrecondition + // should be returned since the client should not retry unless + // they have first fixed up the directory by deleting files from it. + // (d) Use FailedPrecondition if the client performs conditional + // REST Get/Update/Delete on a resource and the resource on the + // server does not match the condition. E.g., conflicting + // read-modify-write on the same resource. + FailedPrecondition Code = 9 + + // Aborted indicates the operation was aborted, typically due to a + // concurrency issue like sequencer check failures, transaction aborts, + // etc. + // + // See litmus test above for deciding between FailedPrecondition, + // Aborted, and Unavailable. + Aborted Code = 10 + + // OutOfRange means operation was attempted past the valid range. + // E.g., seeking or reading past end of file. + // + // Unlike InvalidArgument, this error indicates a problem that may + // be fixed if the system state changes. For example, a 32-bit file + // system will generate InvalidArgument if asked to read at an + // offset that is not in the range [0,2^32-1], but it will generate + // OutOfRange if asked to read from an offset past the current + // file size. + // + // There is a fair bit of overlap between FailedPrecondition and + // OutOfRange. We recommend using OutOfRange (the more specific + // error) when it applies so that callers who are iterating through + // a space can easily look for an OutOfRange error to detect when + // they are done. + OutOfRange Code = 11 + + // Unimplemented indicates operation is not implemented or not + // supported/enabled in this service. + Unimplemented Code = 12 + + // Internal errors. Means some invariants expected by underlying + // system has been broken. If you see one of these errors, + // something is very broken. + Internal Code = 13 + + // Unavailable indicates the service is currently unavailable. + // This is a most likely a transient condition and may be corrected + // by retrying with a backoff. + // + // See litmus test above for deciding between FailedPrecondition, + // Aborted, and Unavailable. + Unavailable Code = 14 + + // DataLoss indicates unrecoverable data loss or corruption. + DataLoss Code = 15 + + // Codes are expected to map 1:1 to gRPC codes. If you want to add a new + // value that's not on gRPC consider carefully the implications before + // you do so. +) + +// TrillianError associates an error message with a failure code in order to +// make error translation possible by other layers (e.g., TrillianError to +// gRPC). +type TrillianError interface { + error + + // Code returns the corresponding code to the error. + Code() Code +} + +type trillianError struct { + code Code + message string +} + +func (e *trillianError) Error() string { + return e.message +} + +func (e *trillianError) Code() Code { + return e.code +} + +// Errorf creates a TrillianError from the specified code and message. +func Errorf(code Code, format string, a ...interface{}) error { + return &trillianError{code, fmt.Sprintf(format, a...)} +} + +// New creates a TrillianError from the specified code and message. +func New(code Code, msg string) error { + return &trillianError{code, msg} +} diff --git a/errors/errors_test.go b/errors/errors_test.go new file mode 100644 index 0000000000..dd8d84a4a2 --- /dev/null +++ b/errors/errors_test.go @@ -0,0 +1,96 @@ +// Copyright 2017 Google Inc. All Rights Reserved. +// +// 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 +// +// http://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 errors + +import ( + "testing" + + "google.golang.org/grpc/codes" +) + +func TestCodes(t *testing.T) { + tests := []struct { + got Code + want codes.Code + }{ + {got: Canceled, want: codes.Canceled}, + {got: Unknown, want: codes.Unknown}, + {got: InvalidArgument, want: codes.InvalidArgument}, + {got: DeadlineExceeded, want: codes.DeadlineExceeded}, + {got: NotFound, want: codes.NotFound}, + {got: AlreadyExists, want: codes.AlreadyExists}, + {got: PermissionDenied, want: codes.PermissionDenied}, + {got: Unauthenticated, want: codes.Unauthenticated}, + {got: ResourceExhausted, want: codes.ResourceExhausted}, + {got: FailedPrecondition, want: codes.FailedPrecondition}, + {got: Aborted, want: codes.Aborted}, + {got: OutOfRange, want: codes.OutOfRange}, + {got: Unimplemented, want: codes.Unimplemented}, + {got: Internal, want: codes.Internal}, + {got: Unavailable, want: codes.Unavailable}, + {got: DataLoss, want: codes.DataLoss}, + } + for _, test := range tests { + if uint64(test.got) != uint64(test.want) { + t.Errorf("got = %v, want = %v", test.got, test.want) + } + } +} + +func TestErrorf(t *testing.T) { + tests := []struct { + code Code + msg string + param string + wantMsg string + }{ + // No need to test all values, just a couple is enough. + {code: InvalidArgument, msg: "InvalidArgument: %v", param: "foo", wantMsg: "InvalidArgument: foo"}, + {code: NotFound, msg: "NotFound: %v", param: "bar", wantMsg: "NotFound: bar"}, + } + for _, test := range tests { + err := Errorf(test.code, test.msg, test.param) + assertError(t, err, test.code, test.wantMsg) + } +} + +func TestNew(t *testing.T) { + tests := []struct { + code Code + msg string + }{ + // No need to test all values, just a couple is enough. + {code: InvalidArgument, msg: "err InvalidArgument"}, + {code: NotFound, msg: "err NotFound"}, + } + for _, test := range tests { + err := New(test.code, test.msg) + assertError(t, err, test.code, test.msg) + } +} + +func assertError(t *testing.T, err error, wantCode Code, wantMsg string) { + if got := err.Error(); got != wantMsg { + t.Errorf("Error() = %v, want = %v", got, wantMsg) + } + terr, ok := err.(TrillianError) + if !ok { + t.Errorf("err is not a TrillianError: %T", err) + return + } + if got := terr.Code(); got != wantCode { + t.Errorf("Code() = %v, want = %v", got, wantCode) + } +} diff --git a/server/errors/doc.go b/server/errors/doc.go new file mode 100644 index 0000000000..0bd025888f --- /dev/null +++ b/server/errors/doc.go @@ -0,0 +1,16 @@ +// Copyright 2017 Google Inc. All Rights Reserved. +// +// 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 +// +// http://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 errors contains utilities to translate TrillianErrors to gRPC errors. +package errors diff --git a/server/errors/errors.go b/server/errors/errors.go new file mode 100644 index 0000000000..fdf6fe7af6 --- /dev/null +++ b/server/errors/errors.go @@ -0,0 +1,34 @@ +// Copyright 2017 Google Inc. All Rights Reserved. +// +// 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 +// +// http://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 errors + +import ( + te "github.com/google/trillian/errors" + "google.golang.org/grpc" + "google.golang.org/grpc/codes" +) + +// WrapError wraps err as a gRPC error if err is a TrillianError, else err is +// returned unmodified. +func WrapError(err error) error { + switch err := err.(type) { + case te.TrillianError: + return grpc.Errorf(codes.Code(err.Code()), err.Error()) + default: + // Nothing to do: if it's a gRPC error it's already correct, if not gRPC will assume + // codes.Unknown. + return err + } +} diff --git a/server/errors/errors_test.go b/server/errors/errors_test.go new file mode 100644 index 0000000000..a5906d6e9c --- /dev/null +++ b/server/errors/errors_test.go @@ -0,0 +1,55 @@ +// Copyright 2017 Google Inc. All Rights Reserved. +// +// 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 +// +// http://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 errors + +import ( + "errors" + "testing" + + te "github.com/google/trillian/errors" + "github.com/kylelemons/godebug/pretty" + "google.golang.org/grpc" + "google.golang.org/grpc/codes" +) + +func TestWrapError(t *testing.T) { + trillianErr := te.New(te.InvalidArgument, "invalid argument err") + grpcErr := grpc.Errorf(codes.NotFound, "not found err") + err := errors.New("generic error") + + tests := []struct { + err error + wantErr error + }{ + { + err: trillianErr, + wantErr: grpc.Errorf(codes.InvalidArgument, trillianErr.Error()), + }, + { + err: grpcErr, + wantErr: grpcErr, + }, + { + err: err, + wantErr: err, + }, + } + for _, test := range tests { + // We can't use == for rpcErrors because grpc.Errorf returns *rpcError. + if diff := pretty.Compare(WrapError(test.err), test.wantErr); diff != "" { + t.Errorf("WrapError('%T') diff:\n%s", test.err, diff) + } + } +} diff --git a/server/sequencer_manager_test.go b/server/sequencer_manager_test.go index f880287ce4..eedd6432eb 100644 --- a/server/sequencer_manager_test.go +++ b/server/sequencer_manager_test.go @@ -109,7 +109,7 @@ func TestSequencerManagerNothingToDo(t *testing.T) { defer mockCtrl.Finish() registry := extension.Registry{ - LogStorage: storage.NewMockLogStorage(mockCtrl), + LogStorage: storage.NewMockLogStorage(mockCtrl), } sm := NewSequencerManager(registry, zeroDuration) diff --git a/storage/mysql/log_storage.go b/storage/mysql/log_storage.go index 9c2250064d..45e1e14f6a 100644 --- a/storage/mysql/log_storage.go +++ b/storage/mysql/log_storage.go @@ -702,4 +702,4 @@ func isDuplicateErr(err error) bool { } return false -} \ No newline at end of file +} diff --git a/storage/mysql/log_storage_test.go b/storage/mysql/log_storage_test.go index faed1790b1..5c08990a7e 100644 --- a/storage/mysql/log_storage_test.go +++ b/storage/mysql/log_storage_test.go @@ -651,14 +651,14 @@ func TestGetLeafDataByIdentityHash(t *testing.T) { want: []*trillian.LogLeaf{leaf}, }, { - hashes: [][]byte{[]byte{0x01, 0x02}}, + hashes: [][]byte{{0x01, 0x02}}, }, { hashes: [][]byte{ dummyRawHash, - []byte{0x01, 0x02}, + {0x01, 0x02}, dummyHash2, - []byte{0x01, 0x02}, + {0x01, 0x02}, }, // Note: leaves not necessarily returned in order requested. want: []*trillian.LogLeaf{leaf2, leaf},