Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sql error handling #2087

Merged
merged 10 commits into from May 10, 2019

Conversation

3 participants
@rdhariwal
Copy link
Contributor

commented May 6, 2019

Description

This PR filters out sql error messages so we do not leak implementation details to front end in test or production environments.

Setup

Simulate a sql error:

  1. Make sure server and tsp client are running:
  2. Cause a sql error, for example you can change the following line and then try invoking
  3. If your .enrc.local contains export env="development" you should be able to see the sql error otherwise you should get a 500 error and a generic unexpected error from db

Code Review Verification Steps

  • Have the Pivotal acceptance criteria been met for this change?

References

@@ -58,6 +63,10 @@ func ResponseForError(logger Logger, err error) middleware.Responder {
// AddCallerSkip(1) prevents log statements from listing this file and func as the caller
skipLogger := logger.WithOptions(zap.AddCallerSkip(1))

// get development flag value for more verbose data
dbEnv := os.Getenv(cli.DbEnvFlag)

This comment has been minimized.

Copy link
@rdhariwal

rdhariwal May 6, 2019

Author Contributor

I would prefer to use viper but do we want to pull in viper and cmd into this package?

This comment has been minimized.

Copy link
@rdhariwal

rdhariwal May 6, 2019

Author Contributor

see this file on how we are using this variable in viper:

mymove/cmd/milmove/main.go

Lines 308 to 323 in 6a5b9b3

err := cmd.ParseFlags(os.Args[1:])
if err != nil {
return errors.Wrap(err, "Could not parse flags")
}
flag := cmd.Flags()
v := viper.New()
err = v.BindPFlags(flag)
if err != nil {
return errors.Wrap(err, "Could not bind flags")
}
v.SetEnvKeyReplacer(strings.NewReplacer("-", "_"))
v.AutomaticEnv()
dbEnv := v.GetString(cli.DbEnvFlag)

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj May 6, 2019

Contributor

Generally what we do is use Viper in the cmd/milmove/main.go and then set the value inside a handler to pass it down. It's a security hazard to use os.Getenv() in our code (gosec would complain if we had it on, plus I did a big PR to remove all of these calls a while back). If you want a hand with figuring out how to pass the value just grab me and we can pair.

This comment has been minimized.

Copy link
@rdhariwal

rdhariwal May 6, 2019

Author Contributor

backing out environment specific changes based on this conversation: https://defensedigitalservice.slack.com/archives/G8HJSJE67/p1557174245111800

This comment has been minimized.

Copy link
@rdhariwal

rdhariwal May 6, 2019

Author Contributor
if isDev {
return newErrResponse(http.StatusInternalServerError, err)
}
return newErrResponse(http.StatusInternalServerError, errors.New("unexpected error from db"))

This comment has been minimized.

Copy link
@rdhariwal

rdhariwal May 6, 2019

Author Contributor

do we want to mention db error? on one hand this is super helpful to a dev trying to figure out what is going wrong but we are giving away information that our db query is failing? Is this a risk?

This comment has been minimized.

Copy link
@rdhariwal

rdhariwal May 6, 2019

Author Contributor

@chrisgilmerproj any opinions on this error message: "unexpected error from db"

This comment has been minimized.

Copy link
@rdhariwal

rdhariwal May 6, 2019

Author Contributor

^^ my thought process is if a dev sees this in staging or prod the fact that we mention db is going to give them a clue to go hunt for it in cloudwatch

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj May 6, 2019

Contributor

Probably you could just use the same wording in both places: Unhandled SQL error encountered.

@codecov

This comment has been minimized.

Copy link

commented May 6, 2019

Codecov Report

Merging #2087 into master will decrease coverage by 1.48%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2087      +/-   ##
==========================================
- Coverage   60.33%   58.85%   -1.48%     
==========================================
  Files         231      237       +6     
  Lines       13392    13744     +352     
==========================================
+ Hits         8080     8089       +9     
- Misses       4337     4680     +343     
  Partials      975      975
@@ -71,6 +80,13 @@ func ResponseForError(logger Logger, err error) middleware.Responder {
default:
return newErrResponse(http.StatusInternalServerError, err)
}
case *pq.Error:

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj May 6, 2019

Contributor

Help me understand why we're using pq.Error here. Is this what Pop is doing under the hood? Does putting this here mean that anything that is returned from modes.Err* will be skipped? Or is this catching things that we wouldn't see in responseForBaseErrors?

This comment has been minimized.

Copy link
@rdhariwal

rdhariwal May 6, 2019

Author Contributor

ahh great question! When pop errors out the type of struct error is using under the hood is pq.Error and it implements the Error interface, for instance see this function When this error is passed down to ResponseForError function we can use reflection to find out what kind of struct is helping to implement Error interface. So using this logic we can pretty much always capture and handle errors whose underlying cause is pop. So this should globally filter out sql exceptions for us.

@rdhariwal rdhariwal force-pushed the rd-sql-error-handling branch from 93acc56 to 789f3e4 May 6, 2019

@chrisgilmerproj
Copy link
Contributor

left a comment

Modify the language per my comment and then I think we're good to go here.

@pjdufour-truss
Copy link
Contributor

left a comment

I'd like to see some more robust tests.

@rdhariwal

This comment has been minimized.

Copy link
Contributor Author

commented May 7, 2019

I'd like to see some more robust tests.

added table like tests to cover different use cases, I am not sure this is what you had in mind. I thought about using db to fake generate errors however the ResponseForError function is taking error interface as an input and I believe pop will always return error of type pq.Error

@rdhariwal

This comment has been minimized.

Copy link
Contributor Author

commented May 7, 2019

Modify the language per my comment and then I think we're good to go here.

tis done!

}

for _, errT := range errTypes {
err.Message = errT

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj May 7, 2019

Contributor

Are the errors from Pop all just pq.Error with different messages? Or are they explicitly different types?

This comment has been minimized.

Copy link
@rdhariwal

rdhariwal May 8, 2019

Author Contributor

I believe all errors from Pop are type pq.Error with just different messages. See link for all the use cases the error interface can handle. For run time testing I dropped a table from dev db and noticed the error that bubbled up was of type pq.Error

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj May 8, 2019

Contributor

Ahh, so if they're all the same then there's not really a reason to just change the message on each one of these.

I'd actually prefer we cause an error by making a bad query and then handling the error like you're doing. Then we are relying on Pop to send back the pq.Error instead of manufacturing one.

This comment has been minimized.

Copy link
@rdhariwal

rdhariwal May 8, 2019

Author Contributor

ahh so something like this:

err := db.Where("move_id = $1", id.String()).All(&signedCertification)

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj May 8, 2019

Contributor

yeah, exactly. using the DB here I think is more robust.

This comment has been minimized.

Copy link
@rdhariwal

rdhariwal May 9, 2019

Author Contributor

done @chrisgilmerproj 👍

@rdhariwal rdhariwal force-pushed the rd-sql-error-handling branch 2 times, most recently from 145af27 to 5325982 May 9, 2019

go.mod Outdated
github.com/jung-kurt/gofpdf v1.3.3
github.com/karrick/godirwalk v1.9.1 // indirect
github.com/jung-kurt/gofpdf v1.1.1
github.com/karrick/godirwalk v1.8.2 // indirect

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj May 9, 2019

Contributor

These look like downgrades. Can you figure out if you're up to date with master? I'd hate to downgrade any dependencies.

errFK := suite.DB().Create(&invalidShipmentOffer)

// slice to hold all errors and assert against
errs := []error{errInvalidColumn, errNoTable, errInvalidArguments, errInvalidQuery, errFK}

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj May 9, 2019

Contributor

👍 - Love this. Thanks for doing a number of these.

This comment has been minimized.

Copy link
@pjdufour-truss

pjdufour-truss May 9, 2019

Contributor

👍

@rdhariwal rdhariwal force-pushed the rd-sql-error-handling branch from 5325982 to 01af733 May 9, 2019

@rdhariwal rdhariwal force-pushed the rd-sql-error-handling branch from 01af733 to 4b6a741 May 9, 2019

}

func TestErrorsSuite(t *testing.T) {
logger := zaptest.NewLogger(t)

This comment has been minimized.

Copy link
@rdhariwal

rdhariwal May 9, 2019

Author Contributor

@pjdufour-truss I thought this logger will suppress logs but I can still see sql error logs show up in console.

This comment has been minimized.

Copy link
@pjdufour-truss

pjdufour-truss May 9, 2019

Contributor

What command are you using?

This comment has been minimized.

Copy link
@rdhariwal

rdhariwal May 9, 2019

Author Contributor

ahh that might be the problem, I am just using my ide to run the test but here is a screenshot of me running them from cli using go test -v ./pkg/handlers

image

This comment has been minimized.

Copy link
@pjdufour-truss

pjdufour-truss May 9, 2019

Contributor

Yeah, you're running tests with the verbose flag. They should be printed out.

@chrisgilmerproj
Copy link
Contributor

left a comment

🚀

@pjdufour-truss pjdufour-truss self-requested a review May 9, 2019

@rdhariwal rdhariwal merged commit c94933b into master May 10, 2019

20 checks passed

Codacy/PR Quality Review Up to standards. A positive pull request.
Details
ci/circleci: acceptance_tests_experimental Your tests passed on CircleCI!
Details
ci/circleci: acceptance_tests_local Your tests passed on CircleCI!
Details
ci/circleci: acceptance_tests_staging Your tests passed on CircleCI!
Details
ci/circleci: build_app Your tests passed on CircleCI!
Details
ci/circleci: build_migrations Your tests passed on CircleCI!
Details
ci/circleci: build_tasks Your tests passed on CircleCI!
Details
ci/circleci: build_tools Your tests passed on CircleCI!
Details
ci/circleci: client_test Your tests passed on CircleCI!
Details
ci/circleci: integration_tests_api Your tests passed on CircleCI!
Details
ci/circleci: integration_tests_mymove Your tests passed on CircleCI!
Details
ci/circleci: integration_tests_office Your tests passed on CircleCI!
Details
ci/circleci: integration_tests_tsp Your tests passed on CircleCI!
Details
ci/circleci: pre_deps_golang Your tests passed on CircleCI!
Details
ci/circleci: pre_deps_yarn Your tests passed on CircleCI!
Details
ci/circleci: pre_test Your tests passed on CircleCI!
Details
ci/circleci: server_test Your tests passed on CircleCI!
Details
ci/circleci: server_test_coverage Your tests passed on CircleCI!
Details
codecov/patch 100% of diff hit (target 60.33%)
Details
codecov/project/go Absolute coverage decreased by -1.51% but relative coverage increased by +39.83% compared to 872413f
Details

@rdhariwal rdhariwal deleted the rd-sql-error-handling branch May 10, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.