Skip to content

Commit

Permalink
refactor: reduce noise in the logs
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@MikelAlejoBR
MikelAlejoBR committed Nov 29, 2024
1 parent 2d2754d commit 03488aa
Show file tree
Hide file tree
Showing 6 changed files with 26 additions and 44 deletions.
12 changes: 6 additions & 6 deletions helpers_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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")
}
}
Expand Down Expand Up @@ -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")
}
}
Expand All @@ -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")
}
}
Expand Down Expand Up @@ -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")
}
}
Expand Down Expand Up @@ -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")
}
}
Expand All @@ -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")
}
}
Expand Down
12 changes: 6 additions & 6 deletions middleware/authorization.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand All @@ -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
Expand All @@ -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"))
}
}

Expand All @@ -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)
Expand Down
4 changes: 2 additions & 2 deletions middleware/error_handling.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
2 changes: 1 addition & 1 deletion middleware/tenancy.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down
6 changes: 3 additions & 3 deletions util/echo/binder_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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)
Expand Down Expand Up @@ -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))
}
}
Expand Down
34 changes: 8 additions & 26 deletions util/error_document.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,6 @@ package util
import (
"fmt"
"reflect"

l "github.com/RedHatInsights/sources-api-go/logger"
)

var ErrNotFoundEmpty = NewErrNotFound("")
Expand All @@ -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,
},
},
}
}

Expand All @@ -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}
}

Expand All @@ -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 := ""

Expand All @@ -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}
}

0 comments on commit 03488aa

Please sign in to comment.