From 8bd2dc63944c15f51a054cb681c7eb50a41dd671 Mon Sep 17 00:00:00 2001 From: wang yan Date: Thu, 2 Apr 2020 17:15:52 +0800 Subject: [PATCH] Add trace information into internal error Fixes #10839 Add a StackTrace func in to Error, and log it when Harbor gets a internal Signed-off-by: wang yan --- src/lib/errors/const.go | 66 +++++++++++++++++++++++++++++ src/lib/errors/errors.go | 76 +++++----------------------------- src/lib/errors/errors_test.go | 34 +++++++++++++++ src/lib/errors/stack.go | 48 +++++++++++++++++++++ src/lib/errors/stack_test.go | 30 ++++++++++++++ src/lib/log/logger.go | 1 + src/server/error/error.go | 11 +++-- src/server/error/error_test.go | 24 ++++++----- 8 files changed, 210 insertions(+), 80 deletions(-) create mode 100644 src/lib/errors/const.go create mode 100644 src/lib/errors/stack.go create mode 100644 src/lib/errors/stack_test.go diff --git a/src/lib/errors/const.go b/src/lib/errors/const.go new file mode 100644 index 000000000..6a3186595 --- /dev/null +++ b/src/lib/errors/const.go @@ -0,0 +1,66 @@ +package errors + +const ( + // NotFoundCode is code for the error of no object found + NotFoundCode = "NOT_FOUND" + // ConflictCode ... + ConflictCode = "CONFLICT" + // UnAuthorizedCode ... + UnAuthorizedCode = "UNAUTHORIZED" + // BadRequestCode ... + BadRequestCode = "BAD_REQUEST" + // ForbiddenCode ... + ForbiddenCode = "FORBIDDEN" + // PreconditionCode ... + PreconditionCode = "PRECONDITION" + // GeneralCode ... + GeneralCode = "UNKNOWN" + // DENIED it's used by middleware(readonly, vul and content trust) and returned to docker client to index the request is denied. + DENIED = "DENIED" + // PROJECTPOLICYVIOLATION ... + PROJECTPOLICYVIOLATION = "PROJECTPOLICYVIOLATION" + // ViolateForeignKeyConstraintCode is the error code for violating foreign key constraint error + ViolateForeignKeyConstraintCode = "VIOLATE_FOREIGN_KEY_CONSTRAINT" + // DIGESTINVALID ... + DIGESTINVALID = "DIGEST_INVALID" +) + +// NotFoundError is error for the case of object not found +func NotFoundError(err error) *Error { + return New(err).WithCode(NotFoundCode).WithMessage("resource not found") +} + +// ConflictError is error for the case of object conflict +func ConflictError(err error) *Error { + return New(err).WithCode(ConflictCode).WithMessage("resource conflict") +} + +// DeniedError is error for the case of denied +func DeniedError(err error) *Error { + return New(err).WithCode(DENIED).WithMessage("denied") +} + +// UnauthorizedError is error for the case of unauthorized accessing +func UnauthorizedError(err error) *Error { + return New(err).WithCode(UnAuthorizedCode).WithMessage("unauthorized") +} + +// BadRequestError is error for the case of bad request +func BadRequestError(err error) *Error { + return New(err).WithCode(BadRequestCode).WithMessage("bad request") +} + +// ForbiddenError is error for the case of forbidden +func ForbiddenError(err error) *Error { + return New(err).WithCode(ForbiddenCode).WithMessage("forbidden") +} + +// PreconditionFailedError is error for the case of precondition failed +func PreconditionFailedError(err error) *Error { + return New(err).WithCode(PreconditionCode).WithMessage("precondition failed") +} + +// UnknownError ... +func UnknownError(err error) *Error { + return New(err).WithCode(GeneralCode).WithMessage("unknown") +} diff --git a/src/lib/errors/errors.go b/src/lib/errors/errors.go index 036245cb5..4876829b2 100644 --- a/src/lib/errors/errors.go +++ b/src/lib/errors/errors.go @@ -35,9 +35,10 @@ type Error struct { Cause error `json:"-"` Code string `json:"code"` Message string `json:"message"` + Stack *stack `json:"-"` } -// Error returns a human readable error. +// Error returns a human readable error, error.Error() will not contains the track information. Needs it? just call error.StackTrace() func (e *Error) Error() string { var parts []string @@ -58,6 +59,11 @@ func (e *Error) Error() string { return strings.Join(parts, ", ") } +// StackTrace ... +func (e *Error) StackTrace() string { + return e.Stack.frames().format() +} + // WithMessage ... func (e *Error) WithMessage(format string, v ...interface{}) *Error { e.Message = fmt.Sprintf(format, v...) @@ -120,31 +126,6 @@ func NewErrs(err error) Errors { return Errors{err} } -const ( - // NotFoundCode is code for the error of no object found - NotFoundCode = "NOT_FOUND" - // ConflictCode ... - ConflictCode = "CONFLICT" - // UnAuthorizedCode ... - UnAuthorizedCode = "UNAUTHORIZED" - // BadRequestCode ... - BadRequestCode = "BAD_REQUEST" - // ForbiddenCode ... - ForbiddenCode = "FORBIDDEN" - // PreconditionCode ... - PreconditionCode = "PRECONDITION" - // GeneralCode ... - GeneralCode = "UNKNOWN" - // DENIED it's used by middleware(readonly, vul and content trust) and returned to docker client to index the request is denied. - DENIED = "DENIED" - // PROJECTPOLICYVIOLATION ... - PROJECTPOLICYVIOLATION = "PROJECTPOLICYVIOLATION" - // ViolateForeignKeyConstraintCode is the error code for violating foreign key constraint error - ViolateForeignKeyConstraintCode = "VIOLATE_FOREIGN_KEY_CONSTRAINT" - // DIGESTINVALID ... - DIGESTINVALID = "DIGEST_INVALID" -) - // New ... func New(in interface{}) *Error { var err error @@ -159,6 +140,7 @@ func New(in interface{}) *Error { return &Error{ Cause: err, Message: err.Error(), + Stack: newStack(), } } @@ -170,6 +152,7 @@ func Wrap(err error, message string) *Error { e := &Error{ Cause: err, Message: message, + Stack: newStack(), } return e } @@ -181,6 +164,7 @@ func Wrapf(err error, format string, args ...interface{}) *Error { } e := &Error{ Cause: err, + Stack: newStack(), } return e.WithMessage(format, args...) } @@ -192,46 +176,6 @@ func Errorf(format string, args ...interface{}) *Error { } } -// NotFoundError is error for the case of object not found -func NotFoundError(err error) *Error { - return New(err).WithCode(NotFoundCode).WithMessage("resource not found") -} - -// ConflictError is error for the case of object conflict -func ConflictError(err error) *Error { - return New(err).WithCode(ConflictCode).WithMessage("resource conflict") -} - -// DeniedError is error for the case of denied -func DeniedError(err error) *Error { - return New(err).WithCode(DENIED).WithMessage("denied") -} - -// UnauthorizedError is error for the case of unauthorized accessing -func UnauthorizedError(err error) *Error { - return New(err).WithCode(UnAuthorizedCode).WithMessage("unauthorized") -} - -// BadRequestError is error for the case of bad request -func BadRequestError(err error) *Error { - return New(err).WithCode(BadRequestCode).WithMessage("bad request") -} - -// ForbiddenError is error for the case of forbidden -func ForbiddenError(err error) *Error { - return New(err).WithCode(ForbiddenCode).WithMessage("forbidden") -} - -// PreconditionFailedError is error for the case of precondition failed -func PreconditionFailedError(err error) *Error { - return New(err).WithCode(PreconditionCode).WithMessage("precondition failed") -} - -// UnknownError ... -func UnknownError(err error) *Error { - return New(err).WithCode(GeneralCode).WithMessage("unknown") -} - // Cause gets the root error func Cause(err error) error { for err != nil { diff --git a/src/lib/errors/errors_test.go b/src/lib/errors/errors_test.go index bd3d80d1f..2b776a62b 100644 --- a/src/lib/errors/errors_test.go +++ b/src/lib/errors/errors_test.go @@ -44,6 +44,31 @@ func TestErrCode(t *testing.T) { } } +const ( + caller1Stack = "errors.caller1" + caller2Stack = "errors.caller2" + caller3Stack = "errors.caller3" +) + +func caller1() error { + err := caller2() + return err +} + +func caller2() error { + err := caller3() + return err +} + +func caller3() error { + err := caller4() + return New(err).WithMessage("it's caller 3.") +} + +func caller4() error { + return errors.New("it's caller 4") +} + type ErrorTestSuite struct { suite.Suite } @@ -55,6 +80,15 @@ func (suite *ErrorTestSuite) TestNewCompatibleWithStdlib() { suite.Equal(err2.Error(), err1.Error()) } +func (suite *ErrorTestSuite) TestStackTrace() { + err := caller1() + suite.Contains(err.(*Error).StackTrace(), caller1Stack) + suite.Contains(err.(*Error).StackTrace(), caller2Stack) + suite.Contains(err.(*Error).StackTrace(), caller3Stack) + suite.Contains(err.Error(), "it's caller 3.") + suite.Contains(err.Error(), "it's caller 4") +} + func TestErrorTestSuite(t *testing.T) { suite.Run(t, &ErrorTestSuite{}) } diff --git a/src/lib/errors/stack.go b/src/lib/errors/stack.go new file mode 100644 index 000000000..c4700f47f --- /dev/null +++ b/src/lib/errors/stack.go @@ -0,0 +1,48 @@ +package errors + +import ( + "fmt" + "runtime" + "strings" +) + +const maxDeepth = 50 + +type stack []uintptr + +func (s *stack) frames() StackFrames { + var stackFrames StackFrames + frames := runtime.CallersFrames(*s) + for { + frame, next := frames.Next() + // filter out runtime + if !strings.Contains(frame.File, "runtime/") { + stackFrames = append(stackFrames, frame) + } + if !next { + break + } + } + return stackFrames +} + +// newStack ... +func newStack() *stack { + var pcs [maxDeepth]uintptr + n := runtime.Callers(3, pcs[:]) + var st stack = pcs[0:n] + return &st +} + +// StackFrames ... +// ToDo we can define an Harbor frame to customize trace message, but it depends on requirement +type StackFrames []runtime.Frame + +// Output: :, +func (frames StackFrames) format() string { + var msg string + for _, frame := range frames { + msg = msg + fmt.Sprintf("\n%v:%v, %v", frame.File, frame.Line, frame.Function) + } + return msg +} diff --git a/src/lib/errors/stack_test.go b/src/lib/errors/stack_test.go new file mode 100644 index 000000000..836e39b07 --- /dev/null +++ b/src/lib/errors/stack_test.go @@ -0,0 +1,30 @@ +package errors + +import ( + "fmt" + "github.com/stretchr/testify/suite" + "testing" +) + +type stackTestSuite struct { + suite.Suite +} + +func (c *stackTestSuite) SetupTest() {} + +func (c *stackTestSuite) TestFrame() { + stack := newStack() + frames := stack.frames() + c.Equal(len(frames), 4) + fmt.Println(frames.format()) +} + +func (c *stackTestSuite) TestFormat() { + stack := newStack() + frames := stack.frames() + c.Contains(frames[len(frames)-1].Function, "testing.tRunner") +} + +func TestStackTestSuite(t *testing.T) { + suite.Run(t, &stackTestSuite{}) +} diff --git a/src/lib/log/logger.go b/src/lib/log/logger.go index d56f2dc33..bb0b9c00f 100644 --- a/src/lib/log/logger.go +++ b/src/lib/log/logger.go @@ -330,6 +330,7 @@ func line(callDepth int) string { _, file, line, ok := runtime.Caller(callDepth) if !ok { file = "???" + line = 0 } l := strings.SplitN(file, srcSeparator, 2) diff --git a/src/server/error/error.go b/src/server/error/error.go index 7a23ebfdd..80cbb72c8 100644 --- a/src/server/error/error.go +++ b/src/server/error/error.go @@ -47,10 +47,11 @@ var ( // And the error is logged as well func SendError(w http.ResponseWriter, err error) { w.Header().Set("Content-Type", "application/json; charset=utf-8") - statusCode, errPayload := apiError(err) + statusCode, errPayload, stackTrace := apiError(err) // the error detail is logged only, and will not be sent to the client to avoid leaking server information if statusCode >= http.StatusInternalServerError { log.Error(errPayload) + log.Debug(stackTrace) err = errors.New(nil).WithCode(errors.GeneralCode).WithMessage("internal server error") errPayload = errors.NewErrs(err).Error() } else { @@ -63,7 +64,7 @@ func SendError(w http.ResponseWriter, err error) { // generates the HTTP status code based on the specified error, // envelops the error into an error array as the payload and return them -func apiError(err error) (statusCode int, errPayload string) { +func apiError(err error) (statusCode int, errPayload, stackTrace string) { code := 0 var openAPIErr openapi.Error if errors.As(err, &openAPIErr) { @@ -85,7 +86,11 @@ func apiError(err error) (statusCode int, errPayload string) { if code == 0 { code = http.StatusInternalServerError } - return code, errors.NewErrs(err).Error() + fullStack := "" + if _, ok := err.(*errors.Error); ok { + fullStack = err.(*errors.Error).StackTrace() + } + return code, errors.NewErrs(err).Error(), fullStack } var _ middleware.Responder = &ErrResponder{} diff --git a/src/server/error/error_test.go b/src/server/error/error_test.go index 6204f5064..f91a9a71b 100644 --- a/src/server/error/error_test.go +++ b/src/server/error/error_test.go @@ -18,6 +18,7 @@ import ( openapi "github.com/go-openapi/errors" commonhttp "github.com/goharbor/harbor/src/common/http" "github.com/goharbor/harbor/src/lib/errors" + pkg_errors "github.com/pkg/errors" "github.com/stretchr/testify/assert" "net/http" "net/http/httptest" @@ -51,32 +52,33 @@ func TestAPIError(t *testing.T) { var err error // open API error: github.com/go-openapi/errors.Error err = openapi.New(400, "bad request") - statusCode, payload := apiError(err) + statusCode, payload, stacktrace := apiError(err) assert.Equal(t, http.StatusBadRequest, statusCode) assert.Equal(t, `{"errors":[{"code":"BAD_REQUEST","message":"bad request"}]}`, payload) + assert.Contains(t, stacktrace, `error.apiError`) // legacy error err = &commonhttp.Error{ Code: http.StatusNotFound, Message: "not found", } - statusCode, payload = apiError(err) + statusCode, payload, stacktrace = apiError(err) assert.Equal(t, http.StatusNotFound, statusCode) assert.Equal(t, `{"errors":[{"code":"NOT_FOUND","message":"not found"}]}`, payload) + assert.Contains(t, stacktrace, `error.apiError`) // errors.Error - err = &errors.Error{ - Cause: nil, - Code: errors.NotFoundCode, - Message: "resource not found", - } - statusCode, payload = apiError(err) + err = errors.New(nil).WithCode(errors.NotFoundCode).WithMessage("resource not found") + statusCode, payload, stacktrace = apiError(err) assert.Equal(t, http.StatusNotFound, statusCode) assert.Equal(t, `{"errors":[{"code":"NOT_FOUND","message":"resource not found"}]}`, payload) + assert.Contains(t, stacktrace, `error.TestAPIError`) - // common error - e := errors.New("customized error") - statusCode, payload = apiError(e) + // common error, common error has no stacktrace + e := pkg_errors.New("customized error") + statusCode, payload, stacktrace = apiError(e) assert.Equal(t, http.StatusInternalServerError, statusCode) assert.Equal(t, `{"errors":[{"code":"UNKNOWN","message":"customized error"}]}`, payload) + assert.Contains(t, stacktrace, ``) + }