Bug 2048451: Fix proxy dial to pick all proxies #5743
Conversation
openshift-ci
bot
added
bugzilla/severity-medium
|
@rna-afk: This pull request references Bugzilla bug 2048451, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker. 3 validation(s) were run on this bug
No GitHub users were found matching the public email listed for the QA contact in Bugzilla ([email protected]), skipping review request. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
| if err == nil { | ||
| resp, err := http.DefaultClient.Do(req.Request) | ||
| if err == nil { | ||
| if resp.StatusCode == 200 { |
There was a problem hiding this comment.
A GET to the endpoint itself will not necessarily result in a 200.
For example,
$ curl -I https://elasticloadbalancing.us-east-1.amazonaws.com
HTTP/1.1 404 Not FoundThere was a problem hiding this comment.
I was hoping to change that to CONNECT and forgot to do that. :(
There was a problem hiding this comment.
$ curl -I https://elasticloadbalancing.us-east-1.amazonaws.com -X CONNECT
HTTP/1.1 400 Bad Request
rna-afk
force-pushed
the
fix_proxy_dial_aws
branch
from
f1bf537 to
45bb469
Compare
|
/hold See https://go.dev/doc/go1.16#net/http:
Also golang/go#40909 It looks to me like the proxy environment could be misconfigured: |
openshift-ci
bot
added
the
do-not-merge/hold
|
Where did context for the |
|
I asked @yunjiang29 on how he tested it and he used the http_proxy and the https_proxy env variable to test the proxy connection. We still need to fix the existing code that only checks NO_PROXY and ALL_PROXY variables since proxy.Dial function does not check the http proxies. |
Oh, I see. /hold cancel
|
openshift-ci
bot
removed
the
do-not-merge/hold
|
Oh, and I now see that is essentially what you are doing. |
rna-afk
force-pushed
the
fix_proxy_dial_aws
branch
3 times, most recently
from
250dcdb to
ad8c333
Compare
rna-afk
force-pushed
the
fix_proxy_dial_aws
branch
from
ad8c333 to
82d8d7b
Compare
|
/approve Would like to get @staebler's sanity check. |
openshift-ci
bot
added
the
approved
rna-afk
force-pushed
the
fix_proxy_dial_aws
branch
from
82d8d7b to
2f69ea5
Compare
rna-afk
force-pushed
the
fix_proxy_dial_aws
branch
from
2f69ea5 to
4665c5e
Compare
rna-afk
force-pushed
the
fix_proxy_dial_aws
branch
from
4665c5e to
5add020
Compare
rna-afk
force-pushed
the
fix_proxy_dial_aws
branch
from
5add020 to
96cddf2
Compare
|
@rna-afk could we add unit tests to verify behavior when proxy env variables are set? |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: patrickdillon The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
The current proxy dial function does not consider proxy information provided in some environment variables like http_proxy etc when checking if the service principal provided is reachable. Adding a function that checks a few env variables and dials the service to see if it is reachable.
Removing net packages as they are no longer being directly used. Moving them to indirect packages.
rna-afk
force-pushed
the
fix_proxy_dial_aws
branch
from
7f9720b to
56f3264
Compare
openshift-ci
bot
removed
the
needs-rebase
| if test.proxy != "" { | ||
| os.Setenv("HTTP_PROXY", test.proxy) | ||
| } else { | ||
| os.Unsetenv("HTTP_PROXY") | ||
| } |
There was a problem hiding this comment.
As it stands, I don't think test.proxy is worth including in these tests. We're not testing whether the proxy is being used or not. So I don't think including the proxy in the test is doing much.
If we were going to write a test for the proxy, I would imagine it would use something like [httpTest](https://pkg.go.dev/net/http/httptest#example-Server) and create a proxy server and make sure the proxy is being called when trying to connect to the endpoint.
While that could be cool, it seems like a fair amount of effort to test code that is mostly in the stdlib--not code we're writing. So I think it might be best to just remove these parts from the test.
rna-afk
force-pushed
the
fix_proxy_dial_aws
branch
from
56f3264 to
88269d0
Compare
openshift-ci
bot
added
the
needs-rebase
rna-afk
force-pushed
the
fix_proxy_dial_aws
branch
from
88269d0 to
cd24144
Compare
openshift-ci
bot
removed
the
needs-rebase
rna-afk
force-pushed
the
fix_proxy_dial_aws
branch
from
cd24144 to
56f3264
Compare
|
/lgtm |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
3 similar comments
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
@rna-afk: The following tests failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
8 similar comments
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
@rna-afk: All pull requests linked via external trackers have merged: Bugzilla bug 2048451 has been moved to the MODIFIED state. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
The current proxy dial function does not consider proxy
information provided in some environment variables like
http_proxy etc when checking if the service principal
provided is reachable. Adding a function that checks a few
env variables and dials the service to see if it is reachable.