From 03488aa83e4479f5fce9bb7bd3e5e7f1140caa16 Mon Sep 17 00:00:00 2001 From: MikeAlejoBR Date: Fri, 29 Nov 2024 08:55:58 +0100 Subject: [PATCH] refactor: reduce noise in the logs The "Bad Request", "Not Found" and "Unauthorized" errors were logging the exact same message that was being returned to the callers, without any context. Therefore, they feel more like "access logs" than actual error logs that we should be warned about. And "access logs", if we had them here, should just log "info" statements with all sorts of data about the caller, plus the response that was given to them. RHCLOUD-36529 --- helpers_test.go | 12 ++++++------ middleware/authorization.go | 12 ++++++------ middleware/error_handling.go | 4 ++-- middleware/tenancy.go | 2 +- util/echo/binder_test.go | 6 +++--- util/error_document.go | 34 ++++++++-------------------------- 6 files changed, 26 insertions(+), 44 deletions(-) diff --git a/helpers_test.go b/helpers_test.go index dd18297e6..75884b363 100644 --- a/helpers_test.go +++ b/helpers_test.go @@ -161,7 +161,7 @@ func TestGetLimitNegativeLimit(t *testing.T) { t.Errorf(`want 0, got "%d"`, got) } - if !errors.Is(err, util.ErrBadRequestEmpty) { + if !errors.As(err, &util.ErrBadRequest{}) { t.Error("ghosts infected the return") } } @@ -189,7 +189,7 @@ func TestGetLimitBadLimit(t *testing.T) { t.Errorf(`want 0, got "%d"`, got) } - if !errors.Is(err, util.ErrBadRequestEmpty) { + if !errors.As(err, &util.ErrBadRequest{}) { t.Error("ghosts infected the return") } } @@ -214,7 +214,7 @@ func TestGetLimitMissingLimit(t *testing.T) { t.Errorf(`want 0, got "%d"`, got) } - if !errors.Is(err, util.ErrBadRequestEmpty) { + if !errors.As(err, &util.ErrBadRequest{}) { t.Error("ghosts infected the return") } } @@ -269,7 +269,7 @@ func TestGetOffsetNegativeOffset(t *testing.T) { t.Errorf(`want 0, got "%d"`, got) } - if !errors.Is(err, util.ErrBadRequestEmpty) { + if !errors.As(err, &util.ErrBadRequest{}) { t.Error("ghosts infected the return") } } @@ -297,7 +297,7 @@ func TestGetOffsetBadOffset(t *testing.T) { t.Errorf(`want 0, got "%d"`, got) } - if !errors.Is(err, util.ErrBadRequestEmpty) { + if !errors.As(err, &util.ErrBadRequest{}) { t.Error("ghosts infected the return") } } @@ -322,7 +322,7 @@ func TestGetOffsetMissingOffset(t *testing.T) { t.Errorf(`want 0, got "%d"`, got) } - if !errors.Is(err, util.ErrBadRequestEmpty) { + if !errors.As(err, &util.ErrBadRequest{}) { t.Error("ghosts infected the return") } } diff --git a/middleware/authorization.go b/middleware/authorization.go index 8f37738b0..d62937f81 100644 --- a/middleware/authorization.go +++ b/middleware/authorization.go @@ -44,7 +44,7 @@ func PermissionCheck(next echo.HandlerFunc) echo.HandlerFunc { } if !pskMatches(psk) { - return c.JSON(http.StatusUnauthorized, util.ErrorDoc("Unauthorized Action: Incorrect PSK", "401")) + return c.JSON(http.StatusUnauthorized, util.NewErrorDoc("Unauthorized Action: Incorrect PSK", "401")) } case c.Get(h.XRHID) != nil: @@ -64,12 +64,12 @@ func PermissionCheck(next echo.HandlerFunc) echo.HandlerFunc { method := c.Request().Method if method != http.MethodGet && method != http.MethodPost && method != http.MethodDelete { c.Response().Header().Set("Allow", "GET, POST, DELETE") - return c.JSON(http.StatusMethodNotAllowed, util.ErrorDoc("Method not allowed", "405")) + return c.JSON(http.StatusMethodNotAllowed, util.NewErrorDoc("Method not allowed", "405")) } // Secondary check for delete - we could move this to middleware if method == http.MethodDelete && !certDeleteAllowed(c) { c.Response().Header().Set("Allow", "GET, POST") - return c.JSON(http.StatusMethodNotAllowed, util.ErrorDoc("Method not allowed", "405")) + return c.JSON(http.StatusMethodNotAllowed, util.NewErrorDoc("Method not allowed", "405")) } // basically we're checking whether cn or cluster_id is set in @@ -80,7 +80,7 @@ func PermissionCheck(next echo.HandlerFunc) echo.HandlerFunc { if id.Identity.System.ClusterId != "" || id.Identity.System.CommonName != "" { return next(c) } else { - return c.JSON(http.StatusUnauthorized, util.ErrorDoc("Unauthorized Action: system authorization only supports cn/cluster_id authorization", "401")) + return c.JSON(http.StatusUnauthorized, util.NewErrorDoc("Unauthorized Action: system authorization only supports cn/cluster_id authorization", "401")) } } @@ -96,11 +96,11 @@ func PermissionCheck(next echo.HandlerFunc) echo.HandlerFunc { } if !allowed { - return c.JSON(http.StatusUnauthorized, util.ErrorDoc("Unauthorized Action: Missing RBAC permissions", "401")) + return c.JSON(http.StatusUnauthorized, util.NewErrorDoc("Unauthorized Action: Missing RBAC permissions", "401")) } default: - return c.JSON(http.StatusUnauthorized, util.ErrorDoc("Authentication required by either [x-rh-identity] or [x-rh-sources-psk]", "401")) + return c.JSON(http.StatusUnauthorized, util.NewErrorDoc("Authentication required by either [x-rh-identity] or [x-rh-sources-psk]", "401")) } return next(c) diff --git a/middleware/error_handling.go b/middleware/error_handling.go index 79e367246..14bb5a73d 100644 --- a/middleware/error_handling.go +++ b/middleware/error_handling.go @@ -19,10 +19,10 @@ func HandleErrors(next echo.HandlerFunc) echo.HandlerFunc { switch err.(type) { case util.ErrNotFound: statusCode = http.StatusNotFound - message = util.ErrorDocWithoutLogging(err.Error(), "404") + message = util.NewErrorDoc(err.Error(), "404") case util.ErrBadRequest: statusCode = http.StatusBadRequest - message = util.ErrorDocWithoutLogging(err.Error(), "400") + message = util.NewErrorDoc(err.Error(), "400") default: c.Logger().Error(err) uuid, ok := c.Get(h.InsightsRequestID).(string) diff --git a/middleware/tenancy.go b/middleware/tenancy.go index 32f147d7f..41991a0dd 100644 --- a/middleware/tenancy.go +++ b/middleware/tenancy.go @@ -27,7 +27,7 @@ func Tenancy(next echo.HandlerFunc) echo.HandlerFunc { // EbsAccount numbers, and might not work otherwise. if id.Identity.AccountNumber == "" { if id.Identity.OrgID == "" { - return c.JSON(http.StatusUnauthorized, util.ErrorDoc("the ebs account number and the org id are missing", "401")) + return c.JSON(http.StatusUnauthorized, util.NewErrorDoc("the ebs account number and the org id are missing", "401")) } else { c.Logger().Warnf(`[org_id: %s] potential anemic tenant found`, id.Identity.OrgID) } diff --git a/util/echo/binder_test.go b/util/echo/binder_test.go index 194cb07fc..fd185ed66 100644 --- a/util/echo/binder_test.go +++ b/util/echo/binder_test.go @@ -54,7 +54,7 @@ func TestNilBody(t *testing.T) { err := c.Bind(&TestStruct{}) // Check that returned err is Bad request with "no body" message - if !errors.Is(err, util.ErrBadRequestEmpty) { + if !errors.As(err, &util.ErrBadRequest{}) { t.Errorf("Expected Bad request err, got %s", err) } else if !strings.Contains(err.Error(), "no body") { t.Errorf("Expected that err message contains 'no body' but got '%s'", err) @@ -72,7 +72,7 @@ func TestNoBody(t *testing.T) { err := c.Bind(&TestStruct{}) // Check that returned err is Bad request with "no body" message - if !errors.Is(err, util.ErrBadRequestEmpty) { + if !errors.As(err, &util.ErrBadRequest{}) { t.Errorf("Expected Bad request err, got %s", err) } else if !strings.Contains(err.Error(), "no body") { t.Errorf("Expected that err message contains 'no body' but got '%s'", err) @@ -103,7 +103,7 @@ func TestEmptyJsonBody(t *testing.T) { t.Error("No error was found when there should have been a no body error") } - if !errors.Is(err, util.ErrBadRequestEmpty) { + if !errors.As(err, &util.ErrBadRequest{}) { t.Errorf(`bad request error expected when passing it an empty JSON body, got "%s"`, reflect.TypeOf(err)) } } diff --git a/util/error_document.go b/util/error_document.go index 05cba9928..ae444e2a4 100644 --- a/util/error_document.go +++ b/util/error_document.go @@ -3,8 +3,6 @@ package util import ( "fmt" "reflect" - - l "github.com/RedHatInsights/sources-api-go/logger" ) var ErrNotFoundEmpty = NewErrNotFound("") @@ -20,23 +18,19 @@ type ErrorDocument struct { } func ErrorDocWithRequestId(message, status, uuid string) *ErrorDocument { - e := ErrorDocWithoutLogging(message, status) + e := NewErrorDoc(message, status) e.Errors[0].RequestId = uuid return e } -func ErrorDoc(message, status string) *ErrorDocument { - l.Log.Error(message) - - return ErrorDocWithoutLogging(message, status) -} - -func ErrorDocWithoutLogging(message, status string) *ErrorDocument { +func NewErrorDoc(message, status string) *ErrorDocument { return &ErrorDocument{ - []Error{{ - Detail: message, - Status: status, - }}, + []Error{ + { + Detail: message, + Status: status, + }, + }, } } @@ -53,10 +47,6 @@ func (e ErrNotFound) Is(err error) bool { } func NewErrNotFound(t string) error { - if l.Log != nil { - l.Log.Error(t) - } - return ErrNotFound{Type: t} } @@ -68,10 +58,6 @@ func (e ErrBadRequest) Error() string { return fmt.Sprintf("bad request: %s", e.Message) } -func (e ErrBadRequest) Is(err error) bool { - return reflect.TypeOf(err) == reflect.TypeOf(e) -} - func NewErrBadRequest(t interface{}) error { errorMessage := "" @@ -84,9 +70,5 @@ func NewErrBadRequest(t interface{}) error { panic("bad interface type for bad request: " + reflect.ValueOf(t).String()) } - if l.Log != nil { - l.Log.Error(errorMessage) - } - return ErrBadRequest{Message: errorMessage} }