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

Fix response error handling and slightly improve the digest auth code #1102

Merged
merged 7 commits into from
Aug 6, 2019
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
67 changes: 67 additions & 0 deletions lib/netext/httpext/httpdebug_transport.go
Original file line number Diff line number Diff line change
@@ -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 <http://www.gnu.org/licenses/>.
*
*/

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
Copy link
Contributor

@mstoykov mstoykov Aug 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be done in the debug* methods and I would really like a test for this, as it will be very bad if we break .. the http when you are debugging it for example ;)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure I understand... Can you elaborate?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't want to waste any time in writing of tests that would be obsolete by the time we properly fix the http-debug functionality, since it's currently a pile of 💩 . Much like the digest authentication, I haven't changed anything significant from before, I've only slightly rearranged and moved the pile of 💩 to a separate place so it handles all cases and is easier to properly fix in the future. But I don't want to test something whose mode of working we plan to significantly change in the near future...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am/was left with the impression you intend on setting the state in the struct .. while I think it is okay to always get it from the request's context.
And pointing out that a bug in this code will result in the k6/http code breaking when you try to debug it which makes it important to be tested with at least a couple of tests ;) For which being able to change the log will be useful ;)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Being a pile of shit is the exact reason why tests are good idea ... if it wasn't it would've had less reason for it.

Also I don't think the change will make the tests obsolete if anything the tests will show and document the change when it happens and will mostly do with (probably) small change in what we output and some caching ... both of which will not be significant problems , unlike if this code breaks or is broken in a way that nobody tested for.

Lastly "near" and "soon" are relative terms. If you say that this will be fixed in vX.Y.Z in two weeks .. I might be fine .. but given how it usually goes I much prefer if we have tests for things that we intend to change "soon" as "soon" is usually in few months ..

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... I respectfully disagree with both points. I see no point if getting the state from the requests' context every time, since we already have it and we can just inject in the struct - much simpler, no contexts and no type asserts required.

But that point is completely moot, since I have no intentions whatsoever of dealing with any of that in this pull request, given the fact that we agreed to leave the proper fixes of the http-debug mess for another time... I won't dump whole requetss the logger, because I have no idea what issues that will bring - we'll deal with that properly when we fix the http-debug properly. This is just a preliminary step so it tracks every request, so that we don't make things even worse...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FFS, now codecov complains 🤦‍♂️

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is as far as I'm willing to invest in testing HttpDebug for now: 411be0c

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
mstoykov marked this conversation as resolved.
Show resolved Hide resolved
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...
}
}
29 changes: 6 additions & 23 deletions lib/netext/httpext/request.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@ import (
"net"
"net/http"
"net/http/cookiejar"
"net/http/httputil"
"net/url"
"strconv"
"strings"
Expand Down Expand Up @@ -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}
Expand All @@ -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 {
Expand All @@ -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))
Expand Down Expand Up @@ -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)
}
14 changes: 0 additions & 14 deletions lib/netext/httpext/response.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)

Expand Down Expand Up @@ -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
Expand Down