Set server name only for the current broker #1701
Conversation
broker.go
Outdated
| @@ -1440,3 +1422,25 @@ func (b *Broker) registerCounter(name string) metrics.Counter { | |||
| b.registeredMetrics = append(b.registeredMetrics, nameForBroker) | |||
| return metrics.GetOrRegisterCounter(nameForBroker, b.conf.MetricRegistry) | |||
| } | |||
|
|
|||
| func validServerNameTLS(addr string, conf *tls.Config) *tls.Config { | |||
| cfg := conf | |||
There was a problem hiding this comment.
@d1egoaz i think here we don't need separate variable and could use conf from args just checked if it nil
|
@d1egoaz rather than the special splitting code, can't we just use net.SplitHostPort ? Also, I can't think of a good reason why anyone would have set a single ServerName in their So I wonder if we'd just do something like this: var cfg *tls.Config
if config.Net.TLS.Config == nil {
cfg = &tls.Config{}
} else {
cfg = config.Net.TLS.Config.Clone()
}
if cfg.ServerName, _, err = net.SplitHostPort(b.addr); err != nil {
return fmt.Errorf("failed to extract SNI got %w", err)
}
b.conn = tls.Client(b.conn, cfg) |
for sure! thanks!
yeah, but I wonder if we would break any other client for doing this, looks like a breaking change. |
|
@dnwe @dselyuzhitskiy let me know what you guys think about the latest commits, and if it's approved I'll create a new release today. I'm testing this branch in one of my apps, and no issues so far |
| c := cfg.Clone() | ||
| sn, _, err := net.SplitHostPort(addr) | ||
| if err != nil { | ||
| Logger.Println(fmt.Errorf("failed to get ServerName from addr %w", err)) |
There was a problem hiding this comment.
Should we return with error here?
There was a problem hiding this comment.
I should have mentioned this, as this was on purpose.
Currently, this code is called inside a go routine:
https://github.com/Shopify/sarama/blob/master/broker.go#L154
and currently the pattern inside this routine is to log the errors as there is no way to return an error to the caller.
|
Looks good, but suggestion from @dnwe is also interesting |
|
@dnwe let us know what do you think about the latest changes. |
|
@d1egoaz lets go ahead and merge and get another minor version bump out 👍🏻 |
Fixes #1700