-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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 Proxy to Sarama config #21159
Add Proxy to Sarama config #21159
Conversation
Please add a changelog @padraicbc |
Enable: true, | ||
Dialer: httpDialer, | ||
} | ||
break |
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.
What happens if both env vars are set?
Can you include a line in README about this?
Is there a way to have a unit test to check this behavior?
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 saw somewhere in the docs that any http(s)_proxy
envs set would be picked up but not in the case of sarama it seem, that is why I went with this route.
It might be a better approach to have an env specific to sarama like SARAMA_PROXY
as you are speaking Kafka, not http, so not a typical scenario. That or some optional arg you can set in the yaml. The latter is how I use it in a consumer I created, I have a WithProxy
option which sets a proxy field on a struct. If set I do the 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 am using something along these lines for my own purposes:
if p := os.Getenv("SARAMA_PROXY"); p != "" {
log.Printf("using proxy: %s", p)
httpProxyURI, err := url.Parse(p)
if err != nil {
return nil, err
}
httpDialer, err := proxy.FromURL(httpProxyURI, proxy.Direct)
if err != nil {
return nil, err
}
c.Net.Proxy = struct {
Enable bool
Dialer proxy.Dialer
}{
Enable: true,
Dialer: httpDialer,
}
}
Having an explicit env may be the cleanest way to go.
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.
Please don't reference variables fetched from the operating environment, it complicates how to configure the exporter further and can't easily be caught in within the config validation to save time providing feedback to the user (if it is set incorrectly).
please fix the failed checks @padraicbc |
Your PR appears to be in a bad state. PTAL. |
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
Closed as inactive. Feel free to reopen if this PR is still being worked on. |
Sorry, had a lot going on so this took a back seat. Can you repopen please as I am working on it now. |
1df41eb
to
73708f4
Compare
8ad303f
to
3fb5c01
Compare
@@ -37,6 +37,9 @@ type Config struct { | |||
|
|||
// Authentication defines used authentication mechanism. | |||
Authentication Authentication `mapstructure:"auth"` | |||
|
|||
// ProxyURL is the url for the proxy to be used with sarama | |||
ProxyURL string `mapstructure:"proxy_url"` |
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.
@padraicbc do we need to have support directly in the config? How is this different to HTTP_PROXY
env var supported by golang?
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.
Sarama doesn't pick up envs and you won't be using a http proxy. We had squid running and tried numerous ways to make it play nice but you need to tunnel over a TCP proxy as we are talking Kafka, not HTTP.
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 had used envs but it wasn't the preferred way. All this change does is alllow pointing to a TCP proxy you have registered. It is up to the end user to create and register it.
0523a09
to
23dc9e7
Compare
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
Closed as inactive. Feel free to reopen if this PR is still being worked on. |
@MovieStoreGuy I was waiting on some feedback. Can I get this reopened and have someone take a look to see if we are on the same page please. |
Description:
Set a Proxy URL that is picked up in Sarama to proxy Kafka connections over TCP.
Link to tracking Issue:
#21158
Testing:
Documentation: