Don't use http.DefaultClient #702
Conversation
|
@vishalnayak Could you test the SSH stuff when you get a chance? |
This strips out http.DefaultClient everywhere I could immediately find it. Too many things use it and then modify it in incompatible ways. Fixes #700, I believe.
jefferai
force-pushed
the
no-default-httpclient
branch
from
c92534a to
0dbbef1
Compare
| @@ -26,7 +26,7 @@ func TestBackend_Config(t *testing.T) { | |||
|
|
|||
| login_data := map[string]interface{}{ | |||
| // This token has to be replaced with a working token for the test to work. | |||
| "token": "4fb5f4f0738c7aea5ee06dd595f15236ea78918b", | |||
| "token": os.Getenv("GITHUB_TOKEN"), | |||
There was a problem hiding this comment.
I'm not a big fan of this change, we should use a restricted token that we just keep around. This requires that for the test to pass we set a token. Also, this change is weirdly unrelated to the PR.
There was a problem hiding this comment.
It's related in that I was trying to ensure the acceptance tests worked since the GitHub backend is one of the ones most affected by this change. The static token isn't valid, so the acceptance tests currently always fail. GITHUB_TOKEN is already required for the acceptance tests; this just uses that same token value to attempt to run this test.
There was a problem hiding this comment.
Ah okay, if this is an acceptance test then this is okay.
There was a problem hiding this comment.
In the future this should likely be a separate PR though since it is unrealted.
|
This PR also removes the X-Vault-Token header checking everywhere. Is this intended? It just wasn't present in the description of the PR. |
| @@ -21,6 +21,7 @@ BUG FIXES: | |||
| * core: Stale leader entries will now be reaped [GH-679] | |||
| * core: Using `mount-tune` on the auth/token path did not take effect. [GH-688] | |||
| * core: Fix a potential race condition when (un)sealing the vault with metrics enabled [GH-694] | |||
| * everywhere: Don't use http.DefaultClient, as it shares state implicitly and is a source of hard-to-track-down bugs [GH-700] | |||
There was a problem hiding this comment.
split on 80 chars please
|
Sorry it just looks like it was the forwarding but since we got rid of all redirect support, its fine. LGTM |
This strips out http.DefaultClient everywhere I could immediately find
it. Too many things use it and then modify it in incompatible ways.
Unit tests pass, and additionally I ran the acceptance tests for GitHub auth.
Fixes #700, I believe.