Improve error handling throughout backend

[zwass]

WIP of the new standardized format for error reporting.

The proposal is that errors throughout the codebase are reported as
KolideError where possible (with the exception of validation errors which use
validator.ValidationErrors). There is a rudimentary implementation of
ReturnError which will process the error and output public error information.
Private information is also debug logged. Having this central function allows
us to plug in any other error reporting mechanism we would like in the future.

[zwass]

This assert unfortunately fails randomly due to the fact that errors in the JSON is an array (and therefore ordered). It is generated from validator.ValidationErrors which is just a typedef for a map, and in Go maps are unordered (and actually intentionally randomized to prevent relying on any ordering).

[zwass]

We could do something shady like intentionally order the errors only when testing, or we could do some other shady stuff to do an unordered comparison, or we could change errors to a map keyed on field or something like that.

[zwass]

*The unordered comparison wouldn't be shady, would just be a hassle.

[marpaia]

what do you think about moving this into it's own errors.go file?

[zwass]

Yeah, it can totally go into it's own file. Eventually should go into it's own package I think, but we need to avoid a naming conflict with the built in errors package.

[marpaia]

It would be awesome to see us use http.Request and http.ResponseWriter directly here to remove this library's dependency on gin. That's what I did with the sessions lib and wrote a wrapper function with get the req and resp from the context with c.Request and c.Response

[zwass]

Should we make ReturnError take an http.ResponseWriter so it's called like ReturnError(c.Writer, err)?

[marpaia]

yeah, that's cool. you'll have to set the content type and all that, but as the go proverb goes: "a little copying is better than a little dependency"

https://go-proverbs.github.io/

[zwass]

On further thought, what makes this different than any of the other places throughout the codebase that we use c.JSON()? If we ever wanted to migrate off Gin we could change all of the call sites together.

[marpaia]

My hope is that we'll use the standard library http functionality whenever possible and one day, we'll look at the codebase and say "oh, we actually don't use gin for much" and cut it. But if we continue to use all of it's features, even if we don't realllllly need to, then it will feel like a more daunting task to move away from it in the future IMO.

[zwass]

@marpaia Can you confirm you are good with this API before I start converting the codebase to use it?

[marpaia]

what's the logic with this being PublicMessage vs PrivateMessage?

[zwass]

PublicMessage is safe to be returned to the client. PrivateMessage is the detailed error information. Right now we log PrivateMessage at a debug level but we could also send it to a log aggregator or any other analysis.

I should add some docstrings on all this.

[marpaia]

@zwass shouldn't there be a func New(text string) error ala https://golang.org/pkg/errors/?

[zwass]

Yeah, a basic New makes sense. I'd like to encourage returning useful errors, so perhaps it could be func New(publicMessage, privateMessage string) which would default to a 500 status?

[marpaia]

Perhaps in addition to func New(string) error? Many usages throughout the codebase already called errors.New(string) error so such a method would provide an exact API match, which would make replacing call-sites easier for sure.

[zwass]

There are actually only a few places that errors.New is used in the existing codebase. I'd really like to avoid having any errors that don't contain detailed information as it makes debugging so much harder. My intention was to find all the places we were propagating errors and make sure that the useful detailed information was appended.

Another note, we could return the PrivateMessage to the client if the code is running in debug mode. Could make debugging frontend work much easier.

$ ag errors.New
app/server.go
92:     return errors.NewFromError(err, http.StatusBadRequest, "JSON parse error")

app/auth.go
41: return 0, errors.New("No user set")

errors/errors_test.go
17: err := errors.New("Foo error")
34:     ReturnError(c, errors.New("foo"))

sessions/sessions.go
15: ErrNoActiveSession = errors.New("Active session is not present in the database")
19: ErrSessionNotCreated = errors.New("The session has not been created")
23: ErrSessionExpired = errors.New("The session has expired")
27: ErrSessionMalformed = errors.New("The session token was malformed")
208:            return nil, errors.New("Unexpected signing method")

[marpaia]

Fine with me, if the call sites are minimal and you're going to update them all. Yeah, that's awesome! I added debug state to the config, so it's config.App.Debug

[zwass]

Cool, it sounds like there is basic agreement on this, so I'll go about doing the refactoring and incorporate your comments.

[zwass]

@marpaia Please take a look at this when you get a chance. I think I am mostly done with my intended changes so now I need your comments to finish it up.

[marpaia]

Maybe we make all of these require an extra string so we can throw in a longer error which can be used as the private message?

[zwass]

Ah, yeah I wanted to mention that it would be great if we could refactor the authorization code to use KolideError to put more helpful information in at the site of the error and then eliminate this function. I felt like the scope of this PR kept creeeeping so I just shimmed this for now.

[marpaia]

if so, then this LGTM

[zwass]

I also gotta say it's awesome that you immediately zeroed in on my least favorite part of this PR :)

[marpaia]

hah, it's what i hate the most about error management at the moment, probably 30% of the errors returned aren't even the correct error. needs love for sure.

[zwass]

Noted in #57.