Repository: google/go-containerregistry
Pull request #827 (2020-11-12 03:47:51)

Improve transport.Error text
Fixes https://github.com/google/go-containerregistry/issues/825

Return "unexpected status code" instead of "unsupported".

Instead of just printing the status code, include the http.StatusText.

While debugging this, I encountered more MISSING stuff in the debug logs
for URLs. Fix that.

Document that `remote.Head` errors can be retried with `remote.Get` to get more details.

codecov-io: # [Codecov](https://codecov.io/gh/google/go-containerregistry/pull/827?src=pr&el=h1) Report
> Merging [#827](https://codecov.io/gh/google/go-containerregistry/pull/827?src=pr&el=desc) (e98a5db) into [master](https://codecov.io/gh/google/go-containerregistry/commit/ef0e499de80f001ac57881689e9a40a2497247a6?el=desc) (ef0e499) will **increase** coverage by `0.01%`.
> The diff coverage is `100.00%`.

[![Impacted file tree graph](https://codecov.io/gh/google/go-containerregistry/pull/827/graphs/tree.svg?width=650&height=150&src=pr&token=wG7iiDCdZ4)](https://codecov.io/gh/google/go-containerregistry/pull/827?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master     #827      +/-   ##
==========================================
+ Coverage   74.63%   74.64%   +0.01%     
==========================================
  Files         103      103              
  Lines        4340     4342       +2     
==========================================
+ Hits         3239     3241       +2     
  Misses        619      619              
  Partials      482      482              
```


| [Impacted Files](https://codecov.io/gh/google/go-containerregistry/pull/827?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pkg/v1/remote/transport/error.go](https://codecov.io/gh/google/go-containerregistry/pull/827/diff?src=pr&el=tree#diff-cGtnL3YxL3JlbW90ZS90cmFuc3BvcnQvZXJyb3IuZ28=) | `100.00% <100.00%> (ø)` | |
| [pkg/v1/remote/transport/logger.go](https://codecov.io/gh/google/go-containerregistry/pull/827/diff?src=pr&el=tree#diff-cGtnL3YxL3JlbW90ZS90cmFuc3BvcnQvbG9nZ2VyLmdv) | `88.23% <100.00%> (ø)` | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/google/go-containerregistry/pull/827?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/google/go-containerregistry/pull/827?src=pr&el=footer). Last update [ef0e499...e98a5db](https://codecov.io/gh/google/go-containerregistry/pull/827?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Patch:
diff --git a/1 b/1
new file mode 100644
index 00000000..e69de29b
diff --git a/pkg/v1/remote/descriptor.go b/pkg/v1/remote/descriptor.go
index 41da9f86..653c175d 100644
--- a/pkg/v1/remote/descriptor.go
+++ b/pkg/v1/remote/descriptor.go
@@ -87,6 +87,9 @@ func Get(ref name.Reference, options ...Option) (*Descriptor, error) {
 
 // Head returns a v1.Descriptor for the given reference by issuing a HEAD
 // request.
+//
+// Note that the server response will not have a body, so any errors encountered
+// should be retried with Get to get more details.
 func Head(ref name.Reference, options ...Option) (*v1.Descriptor, error) {
 	acceptable := []types.MediaType{
 		// Just to look at them.
diff --git a/pkg/v1/remote/transport/error.go b/pkg/v1/remote/transport/error.go
index 7592590e..dba7d841 100644
--- a/pkg/v1/remote/transport/error.go
+++ b/pkg/v1/remote/transport/error.go
@@ -68,9 +68,12 @@ func (e *Error) responseErr() string {
 	switch len(e.Errors) {
 	case 0:
 		if len(e.rawBody) == 0 {
-			return fmt.Sprintf("unsupported status code %d", e.StatusCode)
+			if e.request != nil && e.request.Method == http.MethodHead {
+				return fmt.Sprintf("unexpected status code %d %s (HEAD responses have no body, use GET for details)", e.StatusCode, http.StatusText(e.StatusCode))
+			}
+			return fmt.Sprintf("unexpected status code %d %s", e.StatusCode, http.StatusText(e.StatusCode))
 		}
-		return fmt.Sprintf("unsupported status code %d; body: %s", e.StatusCode, e.rawBody)
+		return fmt.Sprintf("unexpected status code %d %s: %s", e.StatusCode, http.StatusText(e.StatusCode), e.rawBody)
 	case 1:
 		return e.Errors[0].String()
 	default:
diff --git a/pkg/v1/remote/transport/logger.go b/pkg/v1/remote/transport/logger.go
index 3d0a3865..b7e0c60d 100644
--- a/pkg/v1/remote/transport/logger.go
+++ b/pkg/v1/remote/transport/logger.go
@@ -36,16 +36,15 @@ func NewLogger(inner http.RoundTripper) http.RoundTripper {
 
 func (t *logTransport) RoundTrip(in *http.Request) (out *http.Response, err error) {
 	// Inspired by: github.com/motemen/go-loghttp
-	msg := fmt.Sprintf("--> %s %s", in.Method, in.URL)
 
 	// We redact token responses and binary blobs in response/request.
 	omitBody, reason := redact.FromContext(in.Context())
 	if omitBody {
-		msg = fmt.Sprintf("%s [body redacted: %s]", msg, reason)
+		logs.Debug.Printf("--> %s %s [body redacted: %s]", in.Method, in.URL, reason)
+	} else {
+		logs.Debug.Printf("--> %s %s", in.Method, in.URL)
 	}
 
-	logs.Debug.Printf(msg)
-
 	// Save these headers so we can redact Authorization.
 	savedHeaders := in.Header.Clone()
 	if in.Header != nil {


Test patch:
diff --git a/pkg/v1/remote/transport/error_test.go b/pkg/v1/remote/transport/error_test.go
index a6036972..a7ff77e5 100644
--- a/pkg/v1/remote/transport/error_test.go
+++ b/pkg/v1/remote/transport/error_test.go
@@ -95,16 +95,16 @@ func TestCheckErrorNotError(t *testing.T) {
 	}{{
 		code: http.StatusBadRequest,
 		body: "",
-		msg:  "unsupported status code 400",
+		msg:  "unexpected status code 400 Bad Request",
 	}, {
 		code: http.StatusUnauthorized,
 		// Valid JSON, but not a structured error -- we should still print the body.
 		body: `{"details":"incorrect username or password"}`,
-		msg:  `unsupported status code 401; body: {"details":"incorrect username or password"}`,
+		msg:  `unexpected status code 401 Unauthorized: {"details":"incorrect username or password"}`,
 	}, {
 		code: http.StatusUnauthorized,
 		body: "Not JSON",
-		msg:  "GET https://example.com/somepath?access_token=REDACTED&scope=foo&service=bar: unsupported status code 401; body: Not JSON",
+		msg:  "GET https://example.com/somepath?access_token=REDACTED&scope=foo&service=bar: unexpected status code 401 Unauthorized: Not JSON",
 		request: &http.Request{
 			Method: http.MethodGet,
 			URL: &url.URL{
@@ -118,6 +118,18 @@ func TestCheckErrorNotError(t *testing.T) {
 				}.Encode(),
 			},
 		},
+	}, {
+		code: http.StatusUnauthorized,
+		body: "",
+		msg:  "HEAD https://example.com/somepath: unexpected status code 401 Unauthorized (HEAD responses have no body, use GET for details)",
+		request: &http.Request{
+			Method: http.MethodHead,
+			URL: &url.URL{
+				Scheme: "https",
+				Host:   "example.com",
+				Path:   "somepath",
+			},
+		},
 	}}
 
 	for _, test := range tests {
@@ -166,7 +178,7 @@ func TestCheckErrorWithError(t *testing.T) {
 		error: &Error{
 			StatusCode: 400,
 		},
-		msg: "unsupported status code 400",
+		msg: "unexpected status code 400 Bad Request",
 	}, {
 		code: http.StatusBadRequest,
 		error: &Error{