Improve error message for prepare step #375

sukhil-suresh · 2020-05-08T18:05:44Z

wrap unhandled server errors returned by container registry with a more reasonable error message
removed redundant code from test file

Before:

[prepare] GET https://dev.registry.pivotal.io/service/token?scope=repository%3Agarbage%3Apush%2Cpull&service=harbor-registry: unsupported status code 500

After:

[prepare] Error validating write permission to dev.registry.pivotal.io/garbage. GET https://dev.registry.pivotal.io/service/token?scope=repository%3Agarbage%3Apush%2Cpull&service=harbor-registry: unsupported status code 500

Signed-off-by: Sukhil Suresh ssuresh@pivotal.io

thisisnotashwin · 2020-05-08T18:39:06Z

pkg/dockercreds/access_checker_test.go

+		it("wraps unhandled server errors with a reasonable error message", func() {
+			handler.HandleFunc("/v2/", func(writer http.ResponseWriter, request *http.Request) {
+				writer.WriteHeader(500)
+			})

 			hasAccess, err := HasWriteAccess(testKeychain{}, tagName)
 			require.Error(t, err)


I think there is a matcher called EqualError that allows one to also assert on the error message in the same require, but that is a minor nit.

codecov-io · 2020-05-08T18:47:40Z

Codecov Report

Merging #375 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #375   +/-   ##
=======================================
  Coverage   67.52%   67.53%           
=======================================
  Files          81       81           
  Lines        3307     3308    +1     
=======================================
+ Hits         2233     2234    +1     
  Misses        800      800           
  Partials      274      274

Impacted Files	Coverage Δ
pkg/dockercreds/access_checker.go	`80.48% <100.00%> (+0.48%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 019fd44...09939ba. Read the comment docs.

matthewmcnew · 2020-05-08T19:29:57Z

pkg/dockercreds/access_checker.go

@@ -39,6 +39,7 @@ func HasWriteAccess(keychain authn.Keychain, tag string) (bool, error) {
 			}
 		}

+		err = errors.Errorf("Error validating write permission to %s. %s", tag, err.Error())


Errors are always confusing in go but, this should be an error wrap.

errors.Wrapf(err, "message %s", thing)

matthewmcnew · 2020-05-08T19:31:57Z

Right now we have two mechanisms to print errors to the user. Either the HasWriteAccess returns false or we get an unexpected error and print that out.

I wonder if HasWriteAccess should just become VerifyWriteAccess that only returns an error and print that error out if it is not nil.

thisisnotashwin · 2020-05-08T19:33:23Z

Right now we have two mechanisms to print errors to the user. Either the HasWriteAccess returns false or we get an unexpected error and print that out.

I wonder if HasWriteAccess should just become VerifyWriteAccess that only returns an error and print that error out if it is not nil.

im into this!!

sukhil-suresh · 2020-05-08T19:39:30Z

Right now we have two mechanisms to print errors to the user. Either the HasWriteAccess returns false or we get an unexpected error and print that out.

I wonder if HasWriteAccess should just become VerifyWriteAccess that only returns an error and print that error out if it is not nil.

Aye. that makes sense. Apply the same approach to HasReadAccess check too? i.e return only an error and rename to VerifyReadAccess

matthewmcnew · 2020-05-08T20:05:00Z

VerifyReadAccess sounds like a great idea, @sukhil-suresh !

matthewmcnew · 2020-05-11T20:28:25Z

Awesome!

What does the updated output look like for 401 and 500 cases?

sukhil-suresh · 2020-05-11T20:52:35Z

What does the updated output look like for 401 and 500 cases?

I captured the output for 500 in the commit message. Also below:

[prepare] Error verifying write access to "dev.registry.pivotal.io/garbage": GET https://dev.registry.pivotal.io/service/token?scope=repository%3Agarbage%3Apush%2Cpull&service=harbor-registry: unsupported status code 500

Did not keep a copy of the output for a 401.. But, the message should be prefixed identically [prepare] Error verifying write access to

Let me grab the output from an actual run and will update soon.

sukhil-suresh · 2020-05-11T21:04:17Z

Checked against dockerhub with invalid creds for the 401 case:

[prepare] Error verifying write access to "index.docker.io/sukhilsuresh/spring-petclinic": Unauthorized: GET https://auth.docker.io/token?scope=repository%3Asukhilsuresh%2Fspring-petclinic%3Apush%2Cpull&service=registry.docker.io: unsupported status code 401

sukhil-suresh · 2020-05-11T21:17:35Z

(Harbor specific weird case)

also the dev.registry.pivotal.io/garbage/garbage, which previously failed with the message [prepare] invalid credentials to build to dev.registry.pivotal.io/garbage/garbage

now fails with the consistent prefix and a lot more verbose error message. The additional error message was previously lost behind the false boolean, nil error return

[prepare] Error verifying write access to "dev.registry.pivotal.io/garbage/garbage": POST https://dev.registry.pivotal.io/v2/garbage/garbage/blobs/uploads/: UNAUTHORIZED: authentication required; [map[Action:pull Class: Name:garbage/garbage Type:repository] map[Action:push Class: Name:garbage/garbage Type:repository]]

matthewmcnew · 2020-05-11T21:52:15Z

I think we can merge.

I know in the past we explicitly made the decision to not show the entire verbose error message on common 401s because it is a bit confusing message with information they don't need to understand. Do you think that extra information is valuable in the 401 case?

sukhil-suresh · 2020-05-11T22:21:14Z

I know in the past we explicitly made the decision to not show the entire verbose error message on common 401s because it is a bit confusing message with information they don't need to understand. Do you think that extra information is valuable in the 401 case?

Sorry, was not aware of the preference to limit the verbose error message!
The original error was wrapped and bubbled up with the Unauthorized message, as part of dropping the boolean return.

From what, I understand it's a case of seeing

[prepare] Error verifying write access to "index.docker.io/sukhilsuresh/spring-petclinic": Unauthorized

vs

[prepare] Error verifying write access to "index.docker.io/sukhilsuresh/spring-petclinic": Unauthorized: GET https://auth.docker.io/token?scope=repository%3Asukhilsuresh%2Fspring-petclinic%3Apush%2Cpull&service=registry.docker.io: unsupported status code 401`

To be honest, I like seeing as much detail as possible :)
Not strongly opinionated about it, so I am very open to making the error less verbose. Let me know, very minor change - drop the wrapping of error @ https://github.com/pivotal/kpack/blob/issue_357/pkg/dockercreds/access_checker.go#L33-L38

sukhil-suresh · 2020-05-12T18:10:19Z

Made error messages less verbose for known errors

How errors look now:

500

[prepare] Error verifying write access to "dev.registry.pivotal.io/garbage": GET https://dev.registry.pivotal.io/service/token?scope=repository%3Agarbage%3Apush%2Cpull&service=harbor-registry: unsupported status code 500

401

[prepare] Error verifying write access to "index.docker.io/sukhilsuresh/spring-petclinic": UNAUTHORIZED

harbor special case (diagnosed as unauthorized)

[prepare] Error verifying write access to "dev.registry.pivotal.io/garbage/garbage": UNAUTHORIZED

- wrap unhandled server errors returnd by container registry with a more reasonable error message - removed redundant code from test file Before: ``` [prepare] GET https://dev.registry.pivotal.io/service/token?scope=repository%3Agarbage%3Apush%2Cpull&service=harbor-registry: unsupported status code 500 ``` After: ``` [prepare] Error validating write permission to dev.registry.pivotal.io/garbage. GET https://dev.registry.pivotal.io/service/token?scope=repository%3Agarbage%3Apush%2Cpull&service=harbor-registry: unsupported status code 500 ``` Signed-off-by: Sukhil Suresh <ssuresh@pivotal.io>

- Refactor `HasWriteAccess` and `HasReadAccess` to `VerfiyWriteAccess` and `VerifyReadAccess`. Both methods now only return an error instead of an error and bool. - make use of `errors.Wrapf(..)` - make use of `assert.EqualError(..)` Updated error message format: ``` [prepare] Error verifying write access to "dev.registry.pivotal.io/garbage": GET https://dev.registry.pivotal.io/service/token?scope=repository%3Agarbage%3Apush%2Cpull&service=harbor-registry: unsupported status code 500 ``` Signed-off-by: Sukhil Suresh <ssuresh@pivotal.io>

How errors look now: 500 ``` [prepare] Error verifying write access to "dev.registry.pivotal.io/garbage": GET https://dev.registry.pivotal.io/service/token?scope=repository%3Agarbage%3Apush%2Cpull&service=harbor-registry: unsupported status code 500 ``` 401 ``` [prepare] Error verifying write access to "index.docker.io/sukhilsuresh/spring-petclinic": UNAUTHORIZED ``` harbor special case (diagnosed as unauthorized) ``` [prepare] Error verifying write access to "dev.registry.pivotal.io/garbage/garbage": UNAUTHORIZED ``` Signed-off-by: Sukhil Suresh <ssuresh@pivotal.io>

sukhil-suresh · 2020-05-13T13:20:12Z

harbor special case (diagnosed as unauthorized)
[prepare] Error verifying write access to "dev.registry.pivotal.io/garbage/garbage": UNAUTHORIZED

I know it's merged, but noticed when testing (again) that the above statement is inaccurate. The harbor special case dev.registry.pivotal.io/garbage/garbage behaves differently.

[prepare] Error verifying write access to "dev.registry.pivotal.io/garbage/garbage": POST https://dev.registry.pivotal.io/v2/garbage/garbage/blobs/uploads/: UNAUTHORIZED: authentication required; [map[Action:pull Class: Name:garbage/garbage Type:repository] map[Action:push Class: Name:garbage/garbage Type:repository]]

To be fair, this is bit of an outlier case/error, but still... :(

The 401s and 500 work as expected.

sukhil-suresh requested review from thisisnotashwin, matthewmcnew and djoyahoy May 8, 2020 18:05

sukhil-suresh linked an issue May 8, 2020 that may be closed by this pull request

Prepare step's write access check fails on a 500 with harbor #357

Closed

thisisnotashwin reviewed May 8, 2020

View reviewed changes

thisisnotashwin approved these changes May 8, 2020

View reviewed changes

matthewmcnew reviewed May 8, 2020

View reviewed changes

sukhil-suresh force-pushed the issue_357 branch from 127e506 to e9ba538 Compare May 11, 2020 19:04

matthewmcnew approved these changes May 11, 2020

View reviewed changes

thisisnotashwin approved these changes May 11, 2020

View reviewed changes

sukhil-suresh force-pushed the issue_357 branch from 4de7038 to 9bffd38 Compare May 12, 2020 18:08

sukhil-suresh added 3 commits May 12, 2020 15:58

sukhil-suresh force-pushed the issue_357 branch from 9bffd38 to 6fb3178 Compare May 12, 2020 19:59

sukhil-suresh merged commit de0c577 into master May 12, 2020

sukhil-suresh deleted the issue_357 branch May 12, 2020 20:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve error message for prepare step #375

Improve error message for prepare step #375

sukhil-suresh commented May 8, 2020

thisisnotashwin May 8, 2020

codecov-io commented May 8, 2020

matthewmcnew May 8, 2020 •

edited

Loading

matthewmcnew commented May 8, 2020

thisisnotashwin commented May 8, 2020

sukhil-suresh commented May 8, 2020 •

edited

Loading

matthewmcnew commented May 8, 2020

matthewmcnew commented May 11, 2020

sukhil-suresh commented May 11, 2020 •

edited

Loading

sukhil-suresh commented May 11, 2020 •

edited

Loading

sukhil-suresh commented May 11, 2020 •

edited

Loading

matthewmcnew commented May 11, 2020

sukhil-suresh commented May 11, 2020

sukhil-suresh commented May 12, 2020

sukhil-suresh commented May 13, 2020 •

edited

Loading

Improve error message for prepare step #375

Improve error message for prepare step #375

Conversation

sukhil-suresh commented May 8, 2020

thisisnotashwin May 8, 2020

Choose a reason for hiding this comment

codecov-io commented May 8, 2020

Codecov Report

matthewmcnew May 8, 2020 • edited Loading

Choose a reason for hiding this comment

matthewmcnew commented May 8, 2020

thisisnotashwin commented May 8, 2020

sukhil-suresh commented May 8, 2020 • edited Loading

matthewmcnew commented May 8, 2020

matthewmcnew commented May 11, 2020

sukhil-suresh commented May 11, 2020 • edited Loading

sukhil-suresh commented May 11, 2020 • edited Loading

sukhil-suresh commented May 11, 2020 • edited Loading

matthewmcnew commented May 11, 2020

sukhil-suresh commented May 11, 2020

sukhil-suresh commented May 12, 2020

sukhil-suresh commented May 13, 2020 • edited Loading

matthewmcnew May 8, 2020 •

edited

Loading

sukhil-suresh commented May 8, 2020 •

edited

Loading

sukhil-suresh commented May 11, 2020 •

edited

Loading

sukhil-suresh commented May 11, 2020 •

edited

Loading

sukhil-suresh commented May 11, 2020 •

edited

Loading

sukhil-suresh commented May 13, 2020 •

edited

Loading