-
Notifications
You must be signed in to change notification settings - Fork 95
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
Bug 1820595: Support for specific http proxy for the service #87
Bug 1820595: Support for specific http proxy for the service #87
Conversation
/retest |
/retest |
5 similar comments
/retest |
/retest |
/retest |
/retest |
/retest |
}, | ||
{ | ||
Name: "Env set, specific proxy set noproxy, request without noproxy", | ||
EnvValues: map[string]interface{}{"HTTPS_PROXY": "envsecproxy.to", "NO_PROXY": "envnoproxy.to"}, |
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 about HTTP/S_PROXY
values, but shouldn't the NO_PROXY
support multiple values?
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.
Hi, it should, there is just no test for that..
/assign smarterclayton |
} | ||
|
||
// FromConfig is setting HttpProxy from HttpConfig in support secret, if it is used | ||
func FromConfig(c config.HTTPConfig) func(req *http.Request) (*url.URL, error) { |
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.
ProxyFromConfig
as name
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.
Actually, these two methods are confusingly named in general. This should probably be NewProxyFromConfig()
in the line above.
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 removed them when I moved the logic to authorizer and removed options to simplify a bit.
@@ -40,6 +43,7 @@ type Client struct { | |||
|
|||
type Authorizer interface { | |||
Authorize(req *http.Request) error | |||
HTTPConfig() config.HTTPConfig |
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.
Why is the authorizer not handling the proxy setup? It already has all the config info.
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 moved it there. If you ment something like a Roundtripper with dynamic injectable Proxy, I can prepare it as well, but at first it seemed a bit too much code.
/lgtm |
@martinkunc we need this in 4.3 as well. |
…rtinkunc/insights-operator into allow-specific-http-proxy-from-secret
I tried to squeeze the changes, which invalidated the lgtms, sorry. |
/retest |
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.
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alexandrevicenzi, martinkunc 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 |
/retest Please review the full test history for this PR and help us cut down flakes. |
2 similar comments
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
@martinkunc: Bugzilla bug 1805940 is in an unrecognized state (VERIFIED) and will not be 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. |
@martinkunc: All pull requests linked via external trackers have merged: . Bugzilla bug 1820595 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. |
@martinkunc: #87 failed to apply on top of branch "release-4.4":
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. |
@martinkunc: #87 failed to apply on top of branch "release-4.3":
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. |
/bugzilla refresh |
@martinkunc: Bugzilla bug 1820595 is in an unrecognized state (MODIFIED) and will not be 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. |
/bugzilla refresh |
@martinkunc: All pull requests linked via external trackers have merged: . Bugzilla bug 1820595 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. |
This change allows to override proxy passed with Env using support secret.
It uses golang build-in proxy settings, including noproxy logic.