Do not check /v2 endpoint on registry when RoundTripper is provided (#…

…1396)

* Bump staticcheck to latest version

Refactor tests that where Marshalling the transport.Error. This was
causing staticcheck to fail.
Instead of trying to Marshall the error we now provide the strings
version of the expected errors.

Signed-off-by: João Pereira <joaod@vmware.com>

* Wrapper transport no longer checks /v2 endpoint

When the calling the function NewWithContext, if the RoundTripper
provided is of the type transport.Wrapper, the function will assume that
the caller did the needed due diligence to ensure the registry supports
OCI v2. In this case the function will only need to return back the
provided RoundTripper without any further change.

Signed-off-by: João Pereira <joaod@vmware.com>

hack/presubmit.sh

@@ -27,7 +27,7 @@ export GOPATH="${TMP_DIR}"
 pushd ${TMP_DIR}
 trap popd EXIT
 go install golang.org/x/lint/golint@v0.0.0-20210508222113-6edffad5e616
-go install honnef.co/go/tools/cmd/staticcheck@v0.2.1
+go install honnef.co/go/tools/cmd/staticcheck@v0.3.2
 popd
 
 pushd ${PROJECT_ROOT}

pkg/v1/remote/transport/error_test.go

@@ -16,15 +16,13 @@ package transport
 
 import (
 	"bytes"
-	"encoding/json"
 	"errors"
 	"io/ioutil"
 	"net/http"
 	"net/url"
 	"testing"
 
 	"github.com/google/go-cmp/cmp"
-	"github.com/google/go-cmp/cmp/cmpopts"
 )
 
 func TestTemporary(t *testing.T) {
@@ -172,61 +170,43 @@ func TestCheckErrorNotError(t *testing.T) {
 
 func TestCheckErrorWithError(t *testing.T) {
 	tests := []struct {
-		code  int
-		error *Error
-		msg   string
+		name      string
+		code      int
+		errorBody string
+		msg       string
 	}{{
-		code: http.StatusBadRequest,
-		error: &Error{
-			Errors: []Diagnostic{{
-				Code:    NameInvalidErrorCode,
-				Message: "a message for you",
-			}},
-			StatusCode: 400,
-		},
-		msg: "NAME_INVALID: a message for you",
+		name:      "Invalid name error",
+		code:      http.StatusBadRequest,
+		errorBody: `{"errors":[{"code":"NAME_INVALID","message":"a message for you"}],"StatusCode":400}`,
+		msg:       "NAME_INVALID: a message for you",
 	}, {
-		code: http.StatusBadRequest,
-		error: &Error{
-			StatusCode: 400,
-		},
-		msg: "unexpected status code 400 Bad Request",
+		name:      "Only status code is provided",
+		code:      http.StatusBadRequest,
+		errorBody: `{"StatusCode":400}`,
+		msg:       "unexpected status code 400 Bad Request: {\"StatusCode\":400}",
 	}, {
-		code: http.StatusBadRequest,
-		error: &Error{
-			Errors: []Diagnostic{{
-				Code:    NameInvalidErrorCode,
-				Message: "a message for you",
-			}, {
-				Code:    SizeInvalidErrorCode,
-				Message: "another message for you",
-				Detail:  "with some details",
-			}},
-			StatusCode: 400,
-		},
-		msg: "multiple errors returned: NAME_INVALID: a message for you; SIZE_INVALID: another message for you; with some details",
+		name:      "Multiple diagnostics",
+		code:      http.StatusBadRequest,
+		errorBody: `{"errors":[{"code":"NAME_INVALID","message":"a message for you"}, {"code":"SIZE_INVALID","message":"another message for you", "detail": "with some details"}],"StatusCode":400,"Request":null}`,
+		msg:       "multiple errors returned: NAME_INVALID: a message for you; SIZE_INVALID: another message for you; with some details",
 	}}
 
 	for _, test := range tests {
-		b, err := json.Marshal(test.error) // nolint: staticcheck
-		if err != nil {
-			t.Errorf("json.Marshal(%v) = %v", test.error, err)
-		}
-		resp := &http.Response{
-			StatusCode: test.code,
-			Body:       ioutil.NopCloser(bytes.NewBuffer(b)),
-		}
-
-		var terr *Error
-		if err := CheckError(resp, http.StatusOK); err == nil {
-			t.Errorf("CheckError(%d, %s) = nil, wanted error", test.code, string(b))
-		} else if !errors.As(err, &terr) {
-			t.Errorf("CheckError(%d, %s) = %T, wanted *transport.Error", test.code, string(b), err)
-		} else if diff := cmp.Diff(test.error, terr, cmpopts.IgnoreUnexported(Error{})); diff != "" {
-			t.Errorf("CheckError(%d, %s); (-want +got) %s", test.code, string(b), diff)
-		} else if diff := cmp.Diff(test.msg, test.error.Error()); diff != "" {
-			t.Errorf("CheckError(%d, %s).Error(); (-want +got) %s", test.code, string(b), diff)
-		}
+		t.Run(test.name, func(t *testing.T) {
+			resp := &http.Response{
+				StatusCode: test.code,
+				Body:       ioutil.NopCloser(bytes.NewBuffer([]byte(test.errorBody))),
+			}
+
+			var terr *Error
+			if err := CheckError(resp, http.StatusOK); err == nil {
+				t.Errorf("CheckError(%d, %s) = nil, wanted error", test.code, test.errorBody)
+			} else if !errors.As(err, &terr) {
+				t.Errorf("CheckError(%d, %s) = %T, wanted *transport.Error", test.code, test.errorBody, err)
+			} else if diff := cmp.Diff(test.msg, err.Error()); diff != "" {
+				t.Errorf("CheckError(%d, %s).Error(); (-want +got) %s", test.code, test.errorBody, diff)
+			}
+		})
 	}
 }
 

pkg/v1/remote/transport/transport.go

@@ -33,9 +33,18 @@ func New(reg name.Registry, auth authn.Authenticator, t http.RoundTripper, scope
 }
 
 // NewWithContext returns a new RoundTripper based on the provided RoundTripper that has been
-// setup to authenticate with the remote registry "reg", in the capacity
+// set up to authenticate with the remote registry "reg", in the capacity
 // laid out by the specified scopes.
+// In case the RoundTripper is already of the type Wrapper it assumes
+// authentication was already done prior to this call, so it just returns
+// the provided RoundTripper without further action
 func NewWithContext(ctx context.Context, reg name.Registry, auth authn.Authenticator, t http.RoundTripper, scopes []string) (http.RoundTripper, error) {
+	// When the transport provided is of the type Wrapper this function assumes that the caller already
+	// executed the necessary login and check.
+	switch t.(type) {
+	case *Wrapper:
+		return t, nil
+	}
 	// The handshake:
 	//  1. Use "t" to ping() the registry for the authentication challenge.
 	//

pkg/v1/remote/transport/transport_test.go

@@ -15,6 +15,7 @@
 package transport
 
 import (
+	"context"
 	"errors"
 	"fmt"
 	"io/ioutil"
@@ -32,6 +33,26 @@ var (
 	testReference, _ = name.NewTag("localhost:8080/user/image:latest", name.StrictValidation)
 )
 
+func TestTransportNoActionIfTransportIsAlreadyWrapper(t *testing.T) {
+	server := httptest.NewServer(
+		http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+			w.Header().Set("WWW-Authenticate", `Bearer realm="http://foo.io"`)
+			http.Error(w, "Should not contact the server", http.StatusBadRequest)
+		}))
+	defer server.Close()
+	tprt := &http.Transport{
+		Proxy: func(req *http.Request) (*url.URL, error) {
+			return url.Parse(server.URL)
+		},
+	}
+
+	wTprt := &Wrapper{inner: tprt}
+
+	if _, err := NewWithContext(context.Background(), testReference.Context().Registry, nil, wTprt, []string{testReference.Scope(PullScope)}); err != nil {
+		t.Errorf("NewWithContext unexpected error %s", err)
+	}
+}
+
 func TestTransportSelectionAnonymous(t *testing.T) {
 	// Record the requests we get in the inner transport.
 	cannedResponse := http.Response{

pkg/v1/remote/write_test.go

@@ -19,14 +19,14 @@ import (
 	"context"
 	"crypto/sha256"
 	"encoding/hex"
-	"encoding/json"
 	"errors"
 	"fmt"
 	"io"
 	"io/ioutil"
 	"net/http"
 	"net/http/httptest"
 	"net/url"
+	"regexp"
 	"strings"
 	"sync/atomic"
 	"testing"
@@ -908,12 +908,10 @@ func TestWriteWithErrors(t *testing.T) {
 	headPathPrefix := fmt.Sprintf("/v2/%s/blobs/", expectedRepo)
 	initiatePath := fmt.Sprintf("/v2/%s/blobs/uploads/", expectedRepo)
 
-	expectedError := &transport.Error{
-		Errors: []transport.Diagnostic{{
-			Code:    transport.NameInvalidErrorCode,
-			Message: "some explanation of how things were messed up.",
-		}},
-		StatusCode: 400,
+	errorBody := `{"errors":[{"code":"NAME_INVALID","message":"some explanation of how things were messed up."}],"StatusCode":400}`
+	expectedErrMsg, err := regexp.Compile(`POST .+ NAME_INVALID: some explanation of how things were messed up.`)
+	if err != nil {
+		t.Error(err)
 	}
 
 	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
@@ -928,12 +926,9 @@ func TestWriteWithErrors(t *testing.T) {
 			if r.Method != http.MethodPost {
 				t.Errorf("Method; got %v, want %v", r.Method, http.MethodPost)
 			}
-			b, err := json.Marshal(expectedError) // nolint: staticcheck
-			if err != nil {
-				t.Fatalf("json.Marshal() = %v", err)
-			}
+
 			w.WriteHeader(http.StatusBadRequest)
-			w.Write(b)
+			w.Write([]byte(errorBody))
 		default:
 			t.Fatalf("Unexpected path: %v", r.URL.Path)
 		}
@@ -955,7 +950,8 @@ func TestWriteWithErrors(t *testing.T) {
 		t.Error("Write() = nil; wanted error")
 	} else if !errors.As(err, &terr) {
 		t.Errorf("Write() = %T; wanted *transport.Error", err)
-	} else if diff := cmp.Diff(expectedError, terr, cmpopts.IgnoreFields(transport.Error{}, "Request", "rawBody")); diff != "" {
+	} else if !expectedErrMsg.Match([]byte(terr.Error())) {
+		diff := cmp.Diff(expectedErrMsg, terr.Error())
 		t.Errorf("Write(); (-want +got) = %s", diff)
 	}