-
Notifications
You must be signed in to change notification settings - Fork 57
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
Add secure socks proxy to sdk #667
Conversation
|
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.
Looks good. Just some minor things left
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
backend/proxy/options.go
Outdated
var DefaultOptions = Options{ | ||
Timeout: 180 * time.Second, | ||
KeepAlive: 30 * time.Second, | ||
} |
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.
nit: you could use the DefaultTimeoutOptions
from the httpclient
package (there the timeout is 30s, not 180)
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.
For sure! Updated
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 great. Agree with Andres suggestion
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.
Great work! Just one small thing:
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.
Looks great. Some minor comments/nits
Co-authored-by: Marcus Efraimsson <marcus.efraimsson@gmail.com>
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 great
What this PR does / why we need it:
Grafana users need to be able to connect to datasources that live in different networks than where Grafana is running. This PR adds new functionality that allows datasources to connect through a socks5 proxy with TLS. This will only be available to users if it is enabled on the instance (via the config.ini:
secure_socks_datasource_proxy
.enabled
) and the datasource hasenableSecureSocksProxy=true
set in the json data. Optionally, datasources may specify different usernames and passwords to connect to the proxy.Which issue(s) this PR fixes:
Relates to grafana/grafana#15045
Closes https://github.com/grafana/hosted-grafana/issues/3369
Special notes for your reviewer: