-
Notifications
You must be signed in to change notification settings - Fork 95
Demo: Feat/proxy connection settings #389
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
Demo: Feat/proxy connection settings #389
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #389 +/- ##
==========================================
- Coverage 78.24% 78.05% -0.20%
==========================================
Files 27 27
Lines 2740 2807 +67
==========================================
+ Hits 2144 2191 +47
- Misses 473 488 +15
- Partials 123 128 +5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
15a4521 to
37536d8
Compare
| case "http": | ||
| // FIXME: dialer.NetDialContext is currently used as a work around instead of setting dialer.Proxy as gorilla/websockets does not have 1st class support for setting proxy connect headers | ||
| // Once https://github.com/gorilla/websocket/issues/479 is complete, we should use dialer.Proxy, and dialer.ProxyConnectHeader | ||
| if len(headers) > 0 { | ||
| dialer, err := dialer.NewProxyConnectDialer(proxyURL, &net.Dialer{}, dialer.WithProxyConnectHeaders(headers)) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| c.dialer.NetDialContext = dialer.DialContext | ||
| return nil | ||
| } | ||
| c.dialer.Proxy = http.ProxyURL(proxyURL) // No connect headers, use a regular proxy | ||
| case "https": | ||
| if len(headers) > 0 { | ||
| dialer, err := dialer.NewProxyConnectDialer(proxyURL, &net.Dialer{}, dialer.WithTLS(cfg), dialer.WithProxyConnectHeaders(headers)) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| c.dialer.NetDialTLSContext = dialer.DialContext | ||
| return nil | ||
| } | ||
| c.dialer.Proxy = http.ProxyURL(proxyURL) // No connect headers, use a regular proxy |
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.
This is a little messy (as we mix setting the dialer.Proxy and dialer.NetDial*Context methods); but it's much easer to do at this point in time then it is to replace the entire websockets library.
If gorilla/websocket#988 is ever merged and released we can clean this up.
I've tested this locally using tinyproxy.
I've confirmed that when the ProxyURL and ProxyHeaders are specified, the dialer is used and the proxy is used via CONNECT.
If I only specify the ProxyURL, the c.dialer.Proxy = http.ProxyURL(proxyURL) approach is used; one thing that is interesting to note is that this still results in CONNECT request to the proxy but that may be more to do with the use of websockets.
Steps to test:
- install and start tinyproxy - no additional config is needed for basic testing
- start the example opamp server
- Add the
ProxyURLandProxyHeadersto the example agent start settings:opamp-go/internal/examples/agent/agent/agent.go
Lines 110 to 112 in b5f732b
settings := types.StartSettings{ OpAMPServerURL: "wss://127.0.0.1:4320/v1/opamp", TLSConfig: agent.tlsConfig,
For example
ProxyURL: "http://localhost:8888", // tinyproxy's default address
ProxyHeaders: http.Header{"test-proxy-key": []string{"test-val"}},- Start the example agent
- Agent should start and connect to the server; check the proxy logs to ensure that the connection has been made through the proxy.
|
@andykellr @tigrannajaryan we're able to use a custom dialer func in order to get proxy connect headers working with gorilla/websockets. The implementation is now ready for review. |
|
@andykellr I would like your confirmation as well before we go ahead with this (and the corresponding spec change). |
Add ProxyURL an ProxyHeaders support to start settings as well as the example agent.
This PR will remain as a draft until the opamp-spec for the feature is released.