-
Notifications
You must be signed in to change notification settings - Fork 98
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
Changes from 3 commits
6d35d54
f77afd7
c503b45
147dca3
d19d8d8
65e2183
c62662d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,10 +12,12 @@ import ( | |
"net" | ||
"net/http" | ||
"net/textproto" | ||
"net/url" | ||
"os" | ||
"strconv" | ||
"time" | ||
|
||
"golang.org/x/net/http/httpproxy" | ||
knet "k8s.io/apimachinery/pkg/util/net" | ||
"k8s.io/client-go/pkg/version" | ||
"k8s.io/client-go/transport" | ||
|
@@ -27,6 +29,7 @@ import ( | |
configv1 "github.com/openshift/api/config/v1" | ||
|
||
"github.com/openshift/insights-operator/pkg/authorizer" | ||
"github.com/openshift/insights-operator/pkg/config" | ||
) | ||
|
||
type Client struct { | ||
|
@@ -40,6 +43,7 @@ type Client struct { | |
|
||
type Authorizer interface { | ||
Authorize(req *http.Request) error | ||
HTTPConfig() config.HTTPConfig | ||
} | ||
|
||
type ClusterVersionInfo interface { | ||
|
@@ -88,10 +92,15 @@ func getTrustedCABundle() (*x509.CertPool, error) { | |
return certs, nil | ||
} | ||
|
||
func clientTransport() http.RoundTripper { | ||
// default transport, proxy from env | ||
func proxyFromEnvironment() func(*http.Request) (*url.URL, error) { | ||
return http.ProxyFromEnvironment | ||
} | ||
|
||
// clientTransport creates new http.Transport based on httpConfig if used, or Env | ||
func clientTransport(httpConfig config.HTTPConfig) http.RoundTripper { | ||
// default transport, proxy from configmap or env | ||
clientTransport := &http.Transport{ | ||
Proxy: knet.NewProxierWithNoProxyCIDR(http.ProxyFromEnvironment), | ||
Proxy: NewProxier(knet.NewProxierWithNoProxyCIDR(proxyFromEnvironment()), FromConfig(httpConfig)), | ||
DialContext: (&net.Dialer{ | ||
Timeout: 30 * time.Second, | ||
KeepAlive: 30 * time.Second, | ||
|
@@ -113,6 +122,39 @@ func clientTransport() http.RoundTripper { | |
return transport.DebugWrappers(clientTransport) | ||
} | ||
|
||
// ConfigProxier is creating a Proxier from proxy set in HttpConfig | ||
func ConfigProxier(c config.HTTPConfig) func(req *http.Request) (*url.URL, error) { | ||
proxyConfig := httpproxy.Config{ | ||
HTTPProxy: c.HTTPProxy, | ||
HTTPSProxy: c.HTTPSProxy, | ||
NoProxy: c.NoProxy, | ||
} | ||
// The golang ProxyFunc seems to have NoProxy already built in | ||
return func(req *http.Request) (*url.URL, error) { | ||
return proxyConfig.ProxyFunc()(req.URL) | ||
} | ||
} | ||
|
||
// 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 commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
if len(c.HTTPProxy) > 0 || len(c.HTTPSProxy) > 0 || len(c.NoProxy) > 0 { | ||
return ConfigProxier(c) | ||
} | ||
return nil | ||
} | ||
|
||
// NewProxier will create new http.Proxier function. | ||
// If any of customProxiers is set, it will use it, otherwise will use system | ||
func NewProxier(system func(req *http.Request) (*url.URL, error), customProxiers ...func(req *http.Request) (*url.URL, error)) func(req *http.Request) (*url.URL, error) { | ||
for _, p := range customProxiers { | ||
if p != nil { | ||
return p | ||
} | ||
} | ||
|
||
return system | ||
} | ||
|
||
func (c *Client) Send(ctx context.Context, endpoint string, source Source) error { | ||
cv := c.clusterInfo.ClusterVersion() | ||
if cv == nil { | ||
|
@@ -158,14 +200,14 @@ func (c *Client) Send(ctx context.Context, endpoint string, source Source) error | |
req.Body = pr | ||
|
||
// dynamically set the proxy environment | ||
c.client.Transport = clientTransport() | ||
c.client.Transport = clientTransport(c.authorizer.HTTPConfig()) | ||
|
||
klog.V(4).Infof("Uploading %s to %s", source.Type, req.URL.String()) | ||
resp, err := c.client.Do(req) | ||
if err != nil { | ||
klog.V(4).Infof("Unable to build a request, possible invalid token: %v", err) | ||
// if the request is not build, for example because of invalid endpoint,(maybe some problem with DNS), we want to have record about it in metrics as well. | ||
counterRequestSend.WithLabelValues(c.metricsName, "0").Inc() | ||
counterRequestSend.WithLabelValues(c.metricsName, "0").Inc() | ||
return fmt.Errorf("unable to build request to connect to Insights server: %v", err) | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,143 @@ | ||
package insightsclient | ||
|
||
import ( | ||
"fmt" | ||
"net/http" | ||
"net/http/httptest" | ||
"net/url" | ||
"os" | ||
"testing" | ||
|
||
"github.com/openshift/insights-operator/pkg/config" | ||
"golang.org/x/net/http/httpproxy" | ||
knet "k8s.io/apimachinery/pkg/util/net" | ||
) | ||
|
||
// nonCachedProxyFromEnvironment creates Proxier if Proxy is set. It uses always fresh Env | ||
func nonCachedProxyFromEnvironment() func(*http.Request) (*url.URL, error) { | ||
return func(req *http.Request) (*url.URL, error) { | ||
return httpproxy.FromEnvironment().ProxyFunc()(req.URL) | ||
} | ||
} | ||
|
||
func TestProxy(tt *testing.T) { | ||
testCases := []struct { | ||
Name string | ||
EnvValues map[string]interface{} | ||
RequestURL string | ||
HttpConfig config.HTTPConfig | ||
ProxyURL string | ||
}{ | ||
{ | ||
Name: "No env set, no specific proxy", | ||
EnvValues: map[string]interface{}{"HTTP_PROXY": nil}, | ||
RequestURL: "http://google.com", | ||
ProxyURL: "", | ||
}, | ||
{ | ||
Name: "Env set, no specific proxy", | ||
EnvValues: map[string]interface{}{"HTTP_PROXY": "proxy.to"}, | ||
RequestURL: "http://google.com", | ||
ProxyURL: "http://proxy.to", | ||
}, | ||
{ | ||
Name: "Env set with HTTPS, no specific proxy", | ||
EnvValues: map[string]interface{}{"HTTPS_PROXY": "secproxy.to"}, | ||
RequestURL: "https://google.com", | ||
ProxyURL: "http://secproxy.to", | ||
}, | ||
{ | ||
Name: "Env not set, specific proxy set", | ||
EnvValues: map[string]interface{}{"HTTP_PROXY": nil}, | ||
RequestURL: "http://google.com", | ||
HttpConfig: config.HTTPConfig{HTTPProxy: "specproxy.to"}, | ||
ProxyURL: "http://specproxy.to", | ||
}, | ||
{ | ||
Name: "Env set, specific proxy set http", | ||
EnvValues: map[string]interface{}{"HTTP_PROXY": "envproxy.to"}, | ||
RequestURL: "http://google.com", | ||
HttpConfig: config.HTTPConfig{HTTPProxy: "specproxy.to"}, | ||
ProxyURL: "http://specproxy.to", | ||
}, | ||
{ | ||
Name: "Env set, specific proxy set https", | ||
EnvValues: map[string]interface{}{"HTTPS_PROXY": "envsecproxy.to"}, | ||
RequestURL: "https://google.com", | ||
HttpConfig: config.HTTPConfig{HTTPProxy: "specsecproxy.to"}, | ||
ProxyURL: "http://specsecproxy.to", | ||
}, | ||
{ | ||
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 commentThe reason will be displayed to describe this comment to others. Learn more. Not sure about There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hi, it should, there is just no test for that.. |
||
RequestURL: "https://google.com", | ||
HttpConfig: config.HTTPConfig{HTTPProxy: "specsecproxy.to", NoProxy: "specnoproxy.to"}, | ||
ProxyURL: "http://specsecproxy.to", | ||
}, | ||
{ | ||
Name: "Env set, specific proxy set noproxy, request with noproxy", | ||
EnvValues: map[string]interface{}{"HTTPS_PROXY": "envsecproxy.to", "NO_PROXY": "envnoproxy.to"}, | ||
RequestURL: "https://specnoproxy.to", | ||
HttpConfig: config.HTTPConfig{HTTPProxy: "specsecproxy.to", NoProxy: "specnoproxy.to"}, | ||
ProxyURL: "", | ||
}, | ||
} | ||
for _, tcase := range testCases { | ||
tc := tcase | ||
tt.Run(tc.Name, func(t *testing.T) { | ||
for k, v := range tc.EnvValues { | ||
defer SafeRestoreEnv(k)() | ||
// nil will indicate the need to unset Env | ||
if v != nil { | ||
vv := v.(string) | ||
os.Setenv(k, vv) | ||
} else { | ||
os.Unsetenv(k) | ||
} | ||
} | ||
p := NewProxier(knet.NewProxierWithNoProxyCIDR(nonCachedProxyFromEnvironment()), FromConfig(tc.HttpConfig)) | ||
req := httptest.NewRequest("GET", tc.RequestURL, nil) | ||
url, err := p(req) | ||
|
||
if err != nil { | ||
t.Fatalf("unexpected err %s", err) | ||
} | ||
if (tc.ProxyURL == "" && url != nil) || | ||
(len(tc.ProxyURL) > 0 && (url == nil || tc.ProxyURL != url.String())) { | ||
t.Fatalf("Unexpected value of Proxy Url. Test %s Expected Url %s Received Url %s", tc.Name, tc.ProxyURL, url) | ||
} | ||
}) | ||
} | ||
} | ||
|
||
// func TestEnvNoSpecProxy(t *testing.T) { | ||
martinkunc marked this conversation as resolved.
Show resolved
Hide resolved
|
||
// proxy := "HTTP_PROXY" | ||
// defer SafeRestoreEnv(proxy) | ||
// exp := "proxy.to" | ||
// os.Setenv(proxy, exp) | ||
// httpConfig := config.HTTPConfig{} | ||
// p := NewProxier(knet.NewProxierWithNoProxyCIDR(http.ProxyFromEnvironment), FromConfig(httpConfig)) | ||
// req := httptest.NewRequest("GET", "http://google.com", nil) | ||
// url, err := p(req) | ||
|
||
// t.Logf("url %s err %s", url, err) | ||
// if err != nil { | ||
// t.Fatalf("unexpected err %s", err) | ||
// } | ||
// if nil != url { | ||
// t.Fatalf("no HTTP_PROXY env should return no proxy url.") | ||
// } | ||
// } | ||
|
||
func SafeRestoreEnv(key string) func() { | ||
originalVal, wasSet := os.LookupEnv(key) | ||
return func() { | ||
if !wasSet { | ||
fmt.Printf("unsetting key %s", key) | ||
os.Unsetenv(key) | ||
} else { | ||
fmt.Printf("restoring key %s", key) | ||
os.Setenv(key, originalVal) | ||
} | ||
} | ||
} |
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.