From 8c27b6b0b970ceb03d0930073bc303e32d6debfc Mon Sep 17 00:00:00 2001 From: Nedyalko Andreev Date: Mon, 5 Aug 2019 22:22:29 +0300 Subject: [PATCH 1/7] Fix response error handling and slightly improve the digest auth code Previously, there was an uncaught error (sometimes leading to panics) in the automatic response body decompression. This patch handles these decompression errors and even assigns a custom error code for them. As a bonus, k6 now also properly handles errors due to improper redirects, or too many redirects, and tags the resulting metric samples accordingly. By necessity, I had to also move the digest authentication handling to its own http.RoundTripper layer. This by no means solves the issues of https://github.com/loadimpact/k6/issues/800, but it hopefully slightly improves the situation. A proper authentication cache and likely a different library still have to be used there eventually... --- js/modules/k6/http/request_test.go | 44 ++++++-- lib/netext/httpext/digest_transport.go | 74 +++++++++++++ lib/netext/httpext/error_codes.go | 34 ++++++ lib/netext/httpext/request.go | 142 ++++++++++++------------- lib/netext/httpext/transport.go | 11 +- 5 files changed, 218 insertions(+), 87 deletions(-) create mode 100644 lib/netext/httpext/digest_transport.go diff --git a/js/modules/k6/http/request_test.go b/js/modules/k6/http/request_test.go index 6b2e3719799..5abea7b5a27 100644 --- a/js/modules/k6/http/request_test.go +++ b/js/modules/k6/http/request_test.go @@ -1592,20 +1592,16 @@ func TestErrorCodes(t *testing.T) { script: `let res = http.request("GET", "HTTPBIN_URL/redirect-to?url=http://dafsgdhfjg/");`, }, { - name: "Non location redirect", - expectedErrorCode: 0, - expectedErrorMsg: "", - script: `let res = http.request("GET", "HTTPBIN_URL/no-location-redirect");`, - expectedScriptError: sr(`GoError: Get HTTPBIN_URL/no-location-redirect: 302 response missing Location header`), + name: "Non location redirect", + expectedErrorCode: 1000, + expectedErrorMsg: "302 response missing Location header", + script: `let res = http.request("GET", "HTTPBIN_URL/no-location-redirect");`, }, { name: "Bad location redirect", - expectedErrorCode: 0, - expectedErrorMsg: "", + expectedErrorCode: 1000, + expectedErrorMsg: "failed to parse Location header \"h\\t:/\": parse h\t:/: net/url: invalid control character in URL", //nolint: lll script: `let res = http.request("GET", "HTTPBIN_URL/bad-location-redirect");`, - expectedScriptError: sr( - "GoError: Get HTTPBIN_URL/bad-location-redirect: failed to parse Location header" + - " \"h\\t:/\": parse h\t:/: net/url: invalid control character in URL"), }, { name: "Missing protocol", @@ -1641,7 +1637,7 @@ func TestErrorCodes(t *testing.T) { _, err := common.RunString(rt, sr(testCase.script+"\n"+fmt.Sprintf(` if (res.status != %d) { throw new Error("wrong status: "+ res.status);} - if (res.error != '%s') { throw new Error("wrong error: "+ res.error);} + if (res.error != %q) { throw new Error("wrong error: '" + res.error + "'");} if (res.error_code != %d) { throw new Error("wrong error_code: "+ res.error_code);} `, testCase.status, testCase.expectedErrorMsg, testCase.expectedErrorCode))) if testCase.expectedScriptError == "" { @@ -1824,3 +1820,29 @@ func BenchmarkHandlingOfResponseBodies(b *testing.B) { b.Run("binary", testResponseType("binary")) b.Run("none", testResponseType("none")) } + +func TestErrorsWithDecompression(t *testing.T) { + t.Parallel() + tb, state, _, rt, _ := newRuntime(t) + defer tb.Cleanup() + + state.Options.Throw = null.BoolFrom(false) + + tb.Mux.HandleFunc("/broken-archive", http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + enc := r.URL.Query()["encoding"][0] + w.Header().Set("Content-Encoding", enc) + _, _ = fmt.Fprintf(w, "Definitely not %s, but it's all cool...", enc) + })) + + _, err := common.RunString(rt, tb.Replacer.Replace(` + function handleResponseEncodingError (encoding) { + let resp = http.get("HTTPBIN_URL/broken-archive?encoding=" + encoding); + if (resp.error_code != 1701) { + throw new Error("Expected error_code 1701 for '" + encoding +"', but got " + resp.error_code); + } + } + + ["gzip", "deflate", "br", "zstd"].forEach(handleResponseEncodingError); + `)) + assert.NoError(t, err) +} diff --git a/lib/netext/httpext/digest_transport.go b/lib/netext/httpext/digest_transport.go new file mode 100644 index 00000000000..269d900333f --- /dev/null +++ b/lib/netext/httpext/digest_transport.go @@ -0,0 +1,74 @@ +/* + * + * k6 - a next-generation load testing tool + * Copyright (C) 2019 Load Impact + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as + * published by the Free Software Foundation, either version 3 of the + * License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + * + */ + +package httpext + +import ( + "io/ioutil" + "net/http" + + digest "github.com/Soontao/goHttpDigestClient" +) + +type digestTransport struct { + originalTransport http.RoundTripper +} + +// RoundTrip is handles digest auth by behaving like an http.RoundTripper +// +// TODO: fix - this is a preliminary solution and is somewhat broken! we're +// always making 2 HTTP requests when digest authentication is enabled... we +// should cache the nonces and behave more like a browser... or we should +// ditch the hacky http.RoundTripper approach and write our own client... +// +// Github issue: https://github.com/loadimpact/k6/issues/800 +func (t digestTransport) RoundTrip(req *http.Request) (*http.Response, error) { + // Make an initial request authentication params to compute the + // authorization header + username := req.URL.User.Username() + password, _ := req.URL.User.Password() + + // Removing user from URL to avoid sending the authorization header fo basic auth + req.URL.User = nil + + noAuthResponse, err := t.originalTransport.RoundTrip(req) + if err != nil || noAuthResponse.StatusCode != http.StatusUnauthorized { + return noAuthResponse, err + } + + respBody, err := ioutil.ReadAll(noAuthResponse.Body) + if err != nil { + return nil, err + } + _ = noAuthResponse.Body.Close() + + challenge := digest.GetChallengeFromHeader(&noAuthResponse.Header) + challenge.ComputeResponse(req.Method, req.URL.RequestURI(), string(respBody), username, password) + authorization := challenge.ToAuthorizationStr() + + req.Header.Set(digest.KEY_AUTHORIZATION, authorization) + if req.GetBody != nil { + req.Body, err = req.GetBody() + if err != nil { + return nil, err + } + } + return t.originalTransport.RoundTrip(req) +} diff --git a/lib/netext/httpext/error_codes.go b/lib/netext/httpext/error_codes.go index 18d441780f5..4741cdf57ce 100644 --- a/lib/netext/httpext/error_codes.go +++ b/lib/netext/httpext/error_codes.go @@ -35,6 +35,10 @@ import ( "golang.org/x/net/http2" ) +// TODO: mave rename the type errorCode, so we can have errCode variables? and +// also the constants would probably be better of if `ErrorCode` was a prefix, +// not a suffix - they would be much easier for auto-autocompletion at least... + type errCode uint32 const ( @@ -72,6 +76,10 @@ const ( // HTTP2 Connection errors unknownHTTP2ConnectionErrorCode errCode = 1650 // errors till 1651 + 13 are other HTTP2 Connection errors with a specific errCode + + // Custom k6 content errors, i.e. when the magic fails + //defaultContentError errCode = 1700 // reserved for future use + responseDecompressionErrorCode errCode = 1701 ) const ( @@ -99,6 +107,8 @@ func http2ErrCodeOffset(code http2.ErrCode) errCode { // errorCodeForError returns the errorCode and a specific error message for given error. func errorCodeForError(err error) (errCode, string) { switch e := errors.Cause(err).(type) { + case K6Error: + return e.Code, e.Message case *net.DNSError: switch e.Err { case "no such host": // defined as private in the go stdlib @@ -170,3 +180,27 @@ func errorCodeForError(err error) (errCode, string) { return defaultErrorCode, err.Error() } } + +// K6Error is a helper struct that enhances Go errors with custom k6-specific +// error-codes and more user-readable error messages. +type K6Error struct { + Code errCode + Message string + OriginalError error +} + +// NewK6Error is the constructor for K6Error +func NewK6Error(code errCode, msg string, originalErr error) K6Error { + return K6Error{code, msg, originalErr} +} + +// Error implements the `error` interface, so K6Errors are normal Go errors. +func (k6Err K6Error) Error() string { + return k6Err.Message +} + +// Unwrap implements the `xerrors.Wrapper` interface, so K6Errors are a bit +// future-proof Go 2 errors. +func (k6Err K6Error) Unwrap() error { + return k6Err.OriginalError +} diff --git a/lib/netext/httpext/request.go b/lib/netext/httpext/request.go index 6c12ed029d4..33a18c26d30 100644 --- a/lib/netext/httpext/request.go +++ b/lib/netext/httpext/request.go @@ -38,7 +38,6 @@ import ( "time" ntlmssp "github.com/Azure/go-ntlmssp" - digest "github.com/Soontao/goHttpDigestClient" "github.com/andybalholm/brotli" "github.com/klauspost/compress/zstd" "github.com/loadimpact/k6/lib" @@ -193,6 +192,43 @@ func compressBody(algos []CompressionType, body io.ReadCloser) (*bytes.Buffer, s return buf, contentEncoding, body.Close() } +//nolint:gochecknoglobals +var decompressionErrors = [...]error{ + zlib.ErrChecksum, zlib.ErrDictionary, zlib.ErrHeader, + gzip.ErrChecksum, gzip.ErrHeader, + //TODO: handle brotli errors - currently unexported + zstd.ErrReservedBlockType, zstd.ErrCompressedSizeTooBig, zstd.ErrBlockTooSmall, zstd.ErrMagicMismatch, + zstd.ErrWindowSizeExceeded, zstd.ErrWindowSizeTooSmall, zstd.ErrDecoderSizeExceeded, zstd.ErrUnknownDictionary, + zstd.ErrFrameSizeExceeded, zstd.ErrCRCMismatch, zstd.ErrDecoderClosed, +} + +func newDecompressionError(originalErr error) K6Error { + return NewK6Error( + responseDecompressionErrorCode, + fmt.Sprintf("error decompressing response body (%s)", originalErr.Error()), + originalErr, + ) +} + +func wrapDecompressionError(err error) error { + if err == nil { + return nil + } + + // TODO: something more optimized? for example, we won't get zstd errors if + // we don't use it... maybe the code that builds the decompression readers + // could also add an appropriate error-wrapper layer? + for _, decErr := range decompressionErrors { + if err == decErr { + return newDecompressionError(err) + } + } + if strings.HasPrefix(err.Error(), "brotli: ") { //TODO: submit an upstream patch and fix... + return newDecompressionError(err) + } + return err +} + func readResponseBody( state *lib.State, respType ResponseType, resp *http.Response, respErr error, ) (interface{}, error) { @@ -220,30 +256,30 @@ func readResponseBody( // Transparently decompress the body if it's has a content-encoding we // support. If not, simply return it as it is. contentEncoding := strings.TrimSpace(resp.Header.Get("Content-Encoding")) + //TODO: support stacked compressions, e.g. `deflate, gzip` if compression, err := CompressionTypeString(contentEncoding); err == nil { - var decoder io.ReadCloser + var decoder io.Reader + var err error switch compression { case CompressionTypeDeflate: - decoder, respErr = zlib.NewReader(resp.Body) - rc = &readCloser{decoder} + decoder, err = zlib.NewReader(resp.Body) case CompressionTypeGzip: - decoder, respErr = gzip.NewReader(resp.Body) - rc = &readCloser{decoder} + decoder, err = gzip.NewReader(resp.Body) case CompressionTypeZstd: - var zstdecoder *zstd.Decoder - zstdecoder, respErr = zstd.NewReader(resp.Body) - rc = &readCloser{zstdecoder} + decoder, err = zstd.NewReader(resp.Body) case CompressionTypeBr: - var brdecoder *brotli.Reader - brdecoder = brotli.NewReader(resp.Body) - rc = &readCloser{brdecoder} + decoder = brotli.NewReader(resp.Body) default: // We have not implemented a compression ... :( - respErr = fmt.Errorf( - "unsupported compression type %s for decompression - this is a bug in k6, please report it", + err = fmt.Errorf( + "unsupported compression type %s - this is a bug in k6, please report it", compression, ) } + if err != nil { + return nil, newDecompressionError(err) + } + rc = &readCloser{decoder} } buf := state.BPool.Get() @@ -251,11 +287,12 @@ func readResponseBody( buf.Reset() _, err := io.Copy(buf, rc.Reader) if err != nil { - respErr = err + respErr = wrapDecompressionError(err) } + err = rc.Close() - if err != nil { - respErr = err + if err != nil && respErr == nil { // Don't overwrite previous errors + respErr = wrapDecompressionError(err) } var result interface{} @@ -276,6 +313,7 @@ func readResponseBody( return result, respErr } +//TODO: move as a response method? or constructor? func updateK6Response(k6Response *Response, finishedReq *finishedRequest) { k6Response.ErrorCode = int(finishedReq.errorCode) k6Response.Error = finishedReq.errorMsg @@ -394,10 +432,17 @@ func MakeRequest(ctx context.Context, preq *ParsedHTTPRequest) (*Response, error tracerTransport := newTransport(state, tags) var transport http.RoundTripper = tracerTransport - if preq.Auth == "ntlm" { - transport = ntlmssp.Negotiator{ - RoundTripper: tracerTransport, - } + + // TODO: if HttpDebug is enabled, inject the debug tranport here? or use + // something like a virtual proxy for more accurate results, so we can catch + // things like HTTP/2 and exact headers? Connected issues: + // https://github.com/loadimpact/k6/issues/986, + // https://github.com/loadimpact/k6/issues/1042 + + if preq.Auth == "digest" { + transport = digestTransport{originalTransport: transport} + } else if preq.Auth == "ntlm" { + transport = ntlmssp.Negotiator{RoundTripper: transport} } resp := &Response{ctx: ctx, URL: preq.URL.URL, Request: *respReq} @@ -434,62 +479,13 @@ func MakeRequest(ctx context.Context, preq *ParsedHTTPRequest) (*Response, error }, } - // if digest authentication option is passed, make an initial request - // to get the authentication params to compute the authorization header - if preq.Auth == "digest" { - // TODO: fix - this is very broken! we're always making 2 HTTP requests - // when digest authentication is enabled... we should refactor it as a - // separate transport, like how NTLM auth works - // - // Github issue: https://github.com/loadimpact/k6/issues/800 - username := preq.URL.u.User.Username() - password, _ := preq.URL.u.User.Password() - - // removing user from URL to avoid sending the authorization header fo basic auth - preq.Req.URL.User = nil - - debugRequest(state, preq.Req, "DigestRequest") - res, err := client.Do(preq.Req.WithContext(ctx)) - debugResponse(state, res, "DigestResponse") - body, err := readResponseBody(state, ResponseTypeText, res, err) - finishedReq := tracerTransport.processLastSavedRequest() - if finishedReq != nil { - resp.ErrorCode = int(finishedReq.errorCode) - resp.Error = finishedReq.errorMsg - } - - if err != nil { - // Do *not* log errors about the contex being cancelled. - select { - case <-ctx.Done(): - default: - state.Logger.WithField("error", err).Warn("Digest request failed") - } - - // In case we have an error but resp.Error is not set it means the error is not from - // the transport. For all such errors currently we just return them as if throw is true - if preq.Throw || resp.Error == "" { - return nil, err - } - - return resp, nil - } - - if res.StatusCode == http.StatusUnauthorized { - challenge := digest.GetChallengeFromHeader(&res.Header) - challenge.ComputeResponse(preq.Req.Method, preq.Req.URL.RequestURI(), body.(string), username, password) - authorization := challenge.ToAuthorizationStr() - preq.Req.Header.Set(digest.KEY_AUTHORIZATION, authorization) - } - } - debugRequest(state, preq.Req, "Request") mreq := preq.Req.WithContext(ctx) res, resErr := client.Do(mreq) debugResponse(state, res, "Response") resp.Body, resErr = readResponseBody(state, preq.ResponseType, res, resErr) - finishedReq := tracerTransport.processLastSavedRequest() + finishedReq := tracerTransport.processLastSavedRequest(wrapDecompressionError(resErr)) if finishedReq != nil { updateK6Response(resp, finishedReq) } @@ -538,9 +534,7 @@ func MakeRequest(ctx context.Context, preq *ParsedHTTPRequest) (*Response, error state.Logger.WithField("error", resErr).Warn("Request Failed") } - // In case we have an error but resp.Error is not set it means the error is not from - // the transport. For all such errors currently we just return them as if throw is true - if preq.Throw || resp.Error == "" { + if preq.Throw { return nil, resErr } } diff --git a/lib/netext/httpext/transport.go b/lib/netext/httpext/transport.go index 8a06d8a9dd2..afb8619b400 100644 --- a/lib/netext/httpext/transport.go +++ b/lib/netext/httpext/transport.go @@ -162,13 +162,20 @@ func (t *transport) saveCurrentRequest(currentRequest *unfinishedRequest) { } } -func (t *transport) processLastSavedRequest() *finishedRequest { +func (t *transport) processLastSavedRequest(lastErr error) *finishedRequest { t.lastRequestLock.Lock() unprocessedRequest := t.lastRequest t.lastRequest = nil t.lastRequestLock.Unlock() if unprocessedRequest != nil { + // We don't want to overwrite any previous errors, but if there were + // none and we (i.e. the MakeRequest() function) have one, save it + // before we emit the metrics. + if unprocessedRequest.err == nil && lastErr != nil { + unprocessedRequest.err = lastErr + } + return t.measureAndEmitMetrics(unprocessedRequest) } return nil @@ -176,7 +183,7 @@ func (t *transport) processLastSavedRequest() *finishedRequest { // RoundTrip is the implementation of http.RoundTripper func (t *transport) RoundTrip(req *http.Request) (*http.Response, error) { - t.processLastSavedRequest() + t.processLastSavedRequest(nil) ctx := req.Context() tracer := &Tracer{} From ec493b9af229d8807485ad88562481ae1fd633f4 Mon Sep 17 00:00:00 2001 From: Nedyalko Andreev Date: Tue, 6 Aug 2019 12:52:19 +0300 Subject: [PATCH 2/7] Fix some comment typos --- lib/netext/httpext/digest_transport.go | 2 +- lib/netext/httpext/error_codes.go | 2 +- lib/netext/httpext/request.go | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/netext/httpext/digest_transport.go b/lib/netext/httpext/digest_transport.go index 269d900333f..6dda9eaaffb 100644 --- a/lib/netext/httpext/digest_transport.go +++ b/lib/netext/httpext/digest_transport.go @@ -31,7 +31,7 @@ type digestTransport struct { originalTransport http.RoundTripper } -// RoundTrip is handles digest auth by behaving like an http.RoundTripper +// RoundTrip handles digest auth by behaving like an http.RoundTripper // // TODO: fix - this is a preliminary solution and is somewhat broken! we're // always making 2 HTTP requests when digest authentication is enabled... we diff --git a/lib/netext/httpext/error_codes.go b/lib/netext/httpext/error_codes.go index 4741cdf57ce..d71f0318d1a 100644 --- a/lib/netext/httpext/error_codes.go +++ b/lib/netext/httpext/error_codes.go @@ -35,7 +35,7 @@ import ( "golang.org/x/net/http2" ) -// TODO: mave rename the type errorCode, so we can have errCode variables? and +// TODO: maybe rename the type errorCode, so we can have errCode variables? and // also the constants would probably be better of if `ErrorCode` was a prefix, // not a suffix - they would be much easier for auto-autocompletion at least... diff --git a/lib/netext/httpext/request.go b/lib/netext/httpext/request.go index 33a18c26d30..b807128041f 100644 --- a/lib/netext/httpext/request.go +++ b/lib/netext/httpext/request.go @@ -433,7 +433,7 @@ func MakeRequest(ctx context.Context, preq *ParsedHTTPRequest) (*Response, error tracerTransport := newTransport(state, tags) var transport http.RoundTripper = tracerTransport - // TODO: if HttpDebug is enabled, inject the debug tranport here? or use + // TODO: if HttpDebug is enabled, inject the debug transport here? or use // something like a virtual proxy for more accurate results, so we can catch // things like HTTP/2 and exact headers? Connected issues: // https://github.com/loadimpact/k6/issues/986, From 911c60c68eb737516138f46d66911fdca9599992 Mon Sep 17 00:00:00 2001 From: Nedyalko Andreev Date: Tue, 6 Aug 2019 12:52:43 +0300 Subject: [PATCH 3/7] Add a test for digest auth with POST body --- js/modules/k6/http/request_test.go | 34 ++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/js/modules/k6/http/request_test.go b/js/modules/k6/http/request_test.go index 5abea7b5a27..e1f9a348339 100644 --- a/js/modules/k6/http/request_test.go +++ b/js/modules/k6/http/request_test.go @@ -45,6 +45,7 @@ import ( "github.com/loadimpact/k6/lib/metrics" "github.com/loadimpact/k6/lib/testutils" "github.com/loadimpact/k6/stats" + "github.com/mccutchen/go-httpbin/httpbin" "github.com/oxtoacart/bpool" "github.com/sirupsen/logrus" logtest "github.com/sirupsen/logrus/hooks/test" @@ -1846,3 +1847,36 @@ func TestErrorsWithDecompression(t *testing.T) { `)) assert.NoError(t, err) } + +func TestDigestAuthWithBody(t *testing.T) { + t.Parallel() + tb, state, samples, rt, _ := newRuntime(t) + defer tb.Cleanup() + + state.Options.Throw = null.BoolFrom(true) + + tb.Mux.HandleFunc("/digest-auth-with-post/", http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + require.Equal(t, "POST", r.Method) + body, err := ioutil.ReadAll(r.Body) + require.NoError(t, err) + require.Equal(t, "super secret body", string(body)) + httpbin.New().DigestAuth(w, r) // this doesn't read the body + })) + + // TODO: fix, the metric tags shouldn't have credentials (https://github.com/loadimpact/k6/issues/1103) + urlWithCreds := tb.Replacer.Replace( + "http://testuser:testpwd@HTTPBIN_IP:HTTPBIN_PORT/digest-auth-with-post/auth/testuser/testpwd", + ) + + _, err := common.RunString(rt, fmt.Sprintf(` + let res = http.post(%q, "super secret body", { auth: "digest" }); + if (res.status !== 200) { throw new Error("wrong status: " + res.status); } + if (res.error_code !== 0) { throw new Error("wrong error code: " + res.error_code); } + `, urlWithCreds)) + require.NoError(t, err) + + expectedURL := tb.Replacer.Replace("HTTPBIN_IP_URL/digest-auth-with-post/auth/testuser/testpwd") + sampleContainers := stats.GetBufferedSamples(samples) + assertRequestMetricsEmitted(t, sampleContainers[0:1], "POST", expectedURL, urlWithCreds, 401, "") + assertRequestMetricsEmitted(t, sampleContainers[1:2], "POST", expectedURL, urlWithCreds, 200, "") +} From 0b8e41269776c931b58a5e14dfa154e89b50f7e7 Mon Sep 17 00:00:00 2001 From: Nedyalko Andreev Date: Tue, 6 Aug 2019 14:01:30 +0300 Subject: [PATCH 4/7] Refactor the http-debug code in its own low-level transport This makes the code slighly saner and fixes the bug where the first NTLM and digest authentication requests weren't displayed. But by no means does this fix all of the bugs in the http-debug code... For those, see: - https://github.com/loadimpact/k6/issues/986 - https://github.com/loadimpact/k6/issues/1042 - https://github.com/loadimpact/k6/issues/774 A discussion how some or all of them can be fixed can be found here: https://github.com/loadimpact/k6/pull/1102#discussion_r310902305 --- lib/netext/httpext/httpdebug_transport.go | 67 +++++++++++++++++++++++ lib/netext/httpext/request.go | 29 ++-------- lib/netext/httpext/response.go | 14 ----- 3 files changed, 73 insertions(+), 37 deletions(-) create mode 100644 lib/netext/httpext/httpdebug_transport.go diff --git a/lib/netext/httpext/httpdebug_transport.go b/lib/netext/httpext/httpdebug_transport.go new file mode 100644 index 00000000000..a0878b4fdd3 --- /dev/null +++ b/lib/netext/httpext/httpdebug_transport.go @@ -0,0 +1,67 @@ +/* + * + * k6 - a next-generation load testing tool + * Copyright (C) 2019 Load Impact + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as + * published by the Free Software Foundation, either version 3 of the + * License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + * + */ + +package httpext + +import ( + "fmt" + "net/http" + "net/http/httputil" + + log "github.com/sirupsen/logrus" +) + +type httpDebugTransport struct { + //TODO: get the state and log to its Logger + originalTransport http.RoundTripper + httpDebugOption string +} + +// RoundTrip prints passing HTTP requests and received responses +// +// TODO: massively improve this, because the printed information can be wrong: +// - https://github.com/loadimpact/k6/issues/986 +// - https://github.com/loadimpact/k6/issues/1042 +// - https://github.com/loadimpact/k6/issues/774 +func (t httpDebugTransport) RoundTrip(req *http.Request) (*http.Response, error) { + // Make an initial reques + t.debugRequest(req) + resp, err := t.originalTransport.RoundTrip(req) + t.debugResponse(resp) + return resp, err +} + +func (t httpDebugTransport) debugRequest(req *http.Request) { + dump, err := httputil.DumpRequestOut(req, t.httpDebugOption == "full") + if err != nil { + log.Fatal(err) //TODO: fix... + } + fmt.Printf("Request:\n%s\n", dump) //TODO: fix... +} + +func (t httpDebugTransport) debugResponse(res *http.Response) { + if res != nil { + dump, err := httputil.DumpResponse(res, t.httpDebugOption == "full") + if err != nil { + log.Fatal(err) //TODO: fix... + } + fmt.Printf("Response:\n%s\n", dump) //TODO: fix... + } +} diff --git a/lib/netext/httpext/request.go b/lib/netext/httpext/request.go index b807128041f..f9630ed5555 100644 --- a/lib/netext/httpext/request.go +++ b/lib/netext/httpext/request.go @@ -31,7 +31,6 @@ import ( "net" "net/http" "net/http/cookiejar" - "net/http/httputil" "net/url" "strconv" "strings" @@ -433,11 +432,12 @@ func MakeRequest(ctx context.Context, preq *ParsedHTTPRequest) (*Response, error tracerTransport := newTransport(state, tags) var transport http.RoundTripper = tracerTransport - // TODO: if HttpDebug is enabled, inject the debug transport here? or use - // something like a virtual proxy for more accurate results, so we can catch - // things like HTTP/2 and exact headers? Connected issues: - // https://github.com/loadimpact/k6/issues/986, - // https://github.com/loadimpact/k6/issues/1042 + if state.Options.HttpDebug.String != "" { + transport = httpDebugTransport{ + originalTransport: transport, + httpDebugOption: state.Options.HttpDebug.String, + } + } if preq.Auth == "digest" { transport = digestTransport{originalTransport: transport} @@ -451,7 +451,6 @@ func MakeRequest(ctx context.Context, preq *ParsedHTTPRequest) (*Response, error Timeout: preq.Timeout, CheckRedirect: func(req *http.Request, via []*http.Request) error { resp.URL = req.URL.String() - debugResponse(state, req.Response, "RedirectResponse") // Update active jar with cookies found in "Set-Cookie" header(s) of redirect response if preq.ActiveJar != nil { @@ -474,15 +473,12 @@ func MakeRequest(ctx context.Context, preq *ParsedHTTPRequest) (*Response, error } return http.ErrUseLastResponse } - debugRequest(state, req, "RedirectRequest") return nil }, } - debugRequest(state, preq.Req, "Request") mreq := preq.Req.WithContext(ctx) res, resErr := client.Do(mreq) - debugResponse(state, res, "Response") resp.Body, resErr = readResponseBody(state, preq.ResponseType, res, resErr) finishedReq := tracerTransport.processLastSavedRequest(wrapDecompressionError(resErr)) @@ -558,16 +554,3 @@ func SetRequestCookies(req *http.Request, jar *cookiejar.Jar, reqCookies map[str } } } - -func debugRequest(state *lib.State, req *http.Request, description string) { - if state.Options.HttpDebug.String != "" { - dump, err := httputil.DumpRequestOut(req, state.Options.HttpDebug.String == "full") - if err != nil { - log.Fatal(err) - } - logDump(description, dump) - } -} -func logDump(description string, dump []byte) { - fmt.Printf("%s:\n%s\n", description, dump) -} diff --git a/lib/netext/httpext/response.go b/lib/netext/httpext/response.go index 0cb9a83a517..c9151b2d459 100644 --- a/lib/netext/httpext/response.go +++ b/lib/netext/httpext/response.go @@ -24,13 +24,9 @@ import ( "context" "crypto/tls" "encoding/json" - "net/http" - "net/http/httputil" - "github.com/loadimpact/k6/lib" "github.com/loadimpact/k6/lib/netext" "github.com/pkg/errors" - log "github.com/sirupsen/logrus" "github.com/tidwall/gjson" ) @@ -115,16 +111,6 @@ func (res *Response) GetCtx() context.Context { return res.ctx } -func debugResponse(state *lib.State, res *http.Response, description string) { - if state.Options.HttpDebug.String != "" && res != nil { - dump, err := httputil.DumpResponse(res, state.Options.HttpDebug.String == "full") - if err != nil { - log.Fatal(err) - } - logDump(description, dump) - } -} - // JSON parses the body of a response as json and returns it to the goja VM func (res *Response) JSON(selector ...string) (interface{}, error) { hasSelector := len(selector) > 0 From 6c85afd94f731bc771204cf7e20db242ac5d649b Mon Sep 17 00:00:00 2001 From: Nedyalko Andreev Date: Tue, 6 Aug 2019 14:09:23 +0300 Subject: [PATCH 5/7] Improve the code comments in the new helper roundtrippers --- lib/netext/httpext/digest_transport.go | 16 +++++++++++++--- lib/netext/httpext/httpdebug_transport.go | 1 - 2 files changed, 13 insertions(+), 4 deletions(-) diff --git a/lib/netext/httpext/digest_transport.go b/lib/netext/httpext/digest_transport.go index 6dda9eaaffb..dc7fa0333e6 100644 --- a/lib/netext/httpext/digest_transport.go +++ b/lib/netext/httpext/digest_transport.go @@ -40,16 +40,19 @@ type digestTransport struct { // // Github issue: https://github.com/loadimpact/k6/issues/800 func (t digestTransport) RoundTrip(req *http.Request) (*http.Response, error) { - // Make an initial request authentication params to compute the + // Make the initial request authentication params to compute the // authorization header username := req.URL.User.Username() password, _ := req.URL.User.Password() - // Removing user from URL to avoid sending the authorization header fo basic auth + // Remove the user data from the URL to avoid sending the authorization + // header for basic auth req.URL.User = nil noAuthResponse, err := t.originalTransport.RoundTrip(req) if err != nil || noAuthResponse.StatusCode != http.StatusUnauthorized { + // If there was an error, or if the remote server didn't respond with + // status 401, we simply return, so the upsteam code can deal with it. return noAuthResponse, err } @@ -59,16 +62,23 @@ func (t digestTransport) RoundTrip(req *http.Request) (*http.Response, error) { } _ = noAuthResponse.Body.Close() + // Calculate the Authorization header + // TODO: determine if we actually need the body, since I'm not sure that's + // what the `entity` means... maybe a moot point if we change the used + // digest auth library... challenge := digest.GetChallengeFromHeader(&noAuthResponse.Header) challenge.ComputeResponse(req.Method, req.URL.RequestURI(), string(respBody), username, password) authorization := challenge.ToAuthorizationStr() - req.Header.Set(digest.KEY_AUTHORIZATION, authorization) + if req.GetBody != nil { + // Reset the request body if we need to req.Body, err = req.GetBody() if err != nil { return nil, err } } + + // Actually make the HTTP request with the propper Authorization return t.originalTransport.RoundTrip(req) } diff --git a/lib/netext/httpext/httpdebug_transport.go b/lib/netext/httpext/httpdebug_transport.go index a0878b4fdd3..7441aeaf8fa 100644 --- a/lib/netext/httpext/httpdebug_transport.go +++ b/lib/netext/httpext/httpdebug_transport.go @@ -41,7 +41,6 @@ type httpDebugTransport struct { // - https://github.com/loadimpact/k6/issues/1042 // - https://github.com/loadimpact/k6/issues/774 func (t httpDebugTransport) RoundTrip(req *http.Request) (*http.Response, error) { - // Make an initial reques t.debugRequest(req) resp, err := t.originalTransport.RoundTrip(req) t.debugResponse(resp) From c668d7a541157117c4dfd17af47d3d5183ed7872 Mon Sep 17 00:00:00 2001 From: Nedyalko Andreev Date: Tue, 6 Aug 2019 14:12:55 +0300 Subject: [PATCH 6/7] Fix some typos in comments --- lib/netext/httpext/digest_transport.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/netext/httpext/digest_transport.go b/lib/netext/httpext/digest_transport.go index dc7fa0333e6..5eece2379e7 100644 --- a/lib/netext/httpext/digest_transport.go +++ b/lib/netext/httpext/digest_transport.go @@ -52,7 +52,7 @@ func (t digestTransport) RoundTrip(req *http.Request) (*http.Response, error) { noAuthResponse, err := t.originalTransport.RoundTrip(req) if err != nil || noAuthResponse.StatusCode != http.StatusUnauthorized { // If there was an error, or if the remote server didn't respond with - // status 401, we simply return, so the upsteam code can deal with it. + // status 401, we simply return, so the upstream code can deal with it. return noAuthResponse, err } @@ -79,6 +79,6 @@ func (t digestTransport) RoundTrip(req *http.Request) (*http.Response, error) { } } - // Actually make the HTTP request with the propper Authorization + // Actually make the HTTP request with the proper Authorization return t.originalTransport.RoundTrip(req) } From 411be0cace3dd2c5fa72087bfebc0c4e039d1421 Mon Sep 17 00:00:00 2001 From: Nedyalko Andreev Date: Tue, 6 Aug 2019 15:12:22 +0300 Subject: [PATCH 7/7] Enable HttpDebug in a single test This should hopefully ensure we're not accidentally breaking something with it... --- js/modules/k6/http/request_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/js/modules/k6/http/request_test.go b/js/modules/k6/http/request_test.go index e1f9a348339..25af8aa106b 100644 --- a/js/modules/k6/http/request_test.go +++ b/js/modules/k6/http/request_test.go @@ -1854,6 +1854,7 @@ func TestDigestAuthWithBody(t *testing.T) { defer tb.Cleanup() state.Options.Throw = null.BoolFrom(true) + state.Options.HttpDebug = null.StringFrom("full") tb.Mux.HandleFunc("/digest-auth-with-post/", http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { require.Equal(t, "POST", r.Method)