-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
8c27b6b
Fix response error handling and slightly improve the digest auth code
na-- ec493b9
Fix some comment typos
na-- 911c60c
Add a test for digest auth with POST body
na-- 0b8e412
Refactor the http-debug code in its own low-level transport
na-- 6c85afd
Improve the code comments in the new helper roundtrippers
na-- c668d7a
Fix some typos in comments
na-- 411be0c
Enable HttpDebug in a single test
na-- File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,84 @@ | ||
/* | ||
* | ||
* 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 ( | ||
"io/ioutil" | ||
"net/http" | ||
|
||
digest "github.com/Soontao/goHttpDigestClient" | ||
) | ||
|
||
type digestTransport struct { | ||
originalTransport 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 | ||
// 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 the initial request authentication params to compute the | ||
// authorization header | ||
username := req.URL.User.Username() | ||
password, _ := req.URL.User.Password() | ||
|
||
// 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 upstream code can deal with it. | ||
return noAuthResponse, err | ||
} | ||
|
||
respBody, err := ioutil.ReadAll(noAuthResponse.Body) | ||
if err != nil { | ||
return nil, err | ||
} | ||
_ = 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 proper Authorization | ||
return t.originalTransport.RoundTrip(req) | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,66 @@ | ||
/* | ||
* | ||
* 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 | ||
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) { | ||
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... | ||
} | ||
} |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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 ;)There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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...There was a problem hiding this comment.
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 ;)
There was a problem hiding this comment.
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 ..
There was a problem hiding this comment.
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...There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FFS, now codecov complains 🤦♂️
There was a problem hiding this comment.
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