Set extra proxy headers in all tsh HTTP requests#19766
Conversation
Before this commit, the `tsh` HTTP requests that had the extra headers were those that did not use `roundtrip`. This commit leverages `http.RoundTripper.RoundTrip` to ensure that all requests have the the extra headers.
ibeckermayer
left a comment
ibeckermayer
left a comment
There was a problem hiding this comment.
Some tests testing the logic of whether requests get downgraded or not would be nice.
Also try a grep -rn NewInsecureWebClient\(\), there seem to be some stragglers left over.
Co-authored-by: Isaiah Becker-Mayer <isaiah@goteleport.com>
vitorenesduarte
force-pushed
the
vitor/extra-proxy-headers
branch
from
9f71114 to
0107a14
Compare
…ational/teleport into vitor/extra-proxy-headers
|
Thanks for the review @ibeckermayer. Could you please take another look?
I improved existing tests and added new ones in 9641dd1.
Thanks. I've added |
| defer httpSvr.Close() | ||
| // TestHttpRoundTripperDowngrade tests that the round tripper downgrades https requests to http | ||
| // when HTTP_PROXY is set to "http://localhost:*" (i.e. there's an http proxy running on localhost). | ||
| func TestHttpRoundTripperDowngrade(t *testing.T) { |
There was a problem hiding this comment.
I get that this is testing the HttpRoundTripper under the hood, however its bad practice to write tests for one package in another. For example, consider if we swap out the underlying transport mechanism in SSHAgentLogin. Now this test is bunk, and might just get deleted. At the very least, we would need to rewrite it to test HTTPRoundTripper in its own package at that point.
Another reason this is bad practice is that tests are a form of documentation. If somebody wants to use HTTPRoundTripper for something else, it's best that the tests testing its core behavior are close by so they can see how it works.
I would say move these comprehensive tests of HTTPRoundTripper's behavior close to TestProxyAwareRoundTripper. And if you think its worthwhile to also have unit tests ensuring this behavior for client.SSHAgentLogin and client.HostCredentials, then add those to the client package.
There was a problem hiding this comment.
Thanks for the suggestion. Moved the tests in 451646b.
|
@vitorenesduarte See the table below for backport results.
|
3 participants
Fixes #16541.
Before this commit, the
tshHTTP requests that had the extra headers were those that did not useroundtrip.This commit leverages
http.RoundTripper.RoundTripto ensure that all requests have the the extra headers.