Skip to content
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

Set server name only for the current broker #1701

Merged
merged 3 commits into from May 19, 2020
Merged

Conversation

d1egoaz
Copy link
Contributor

@d1egoaz d1egoaz commented May 12, 2020

Fixes #1700

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@d1egoaz i think here we don't need separate variable and could use conf from args just checked if it nil

@dnwe
Copy link
Collaborator

dnwe commented May 14, 2020

@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 conf.Net.TLS.Config to use for all brokers, so I wonder if we should always override whatever is there by forming it with our hostname.

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)

@d1egoaz
Copy link
Contributor Author

d1egoaz commented May 14, 2020

@d1egoaz rather than the special splitting code, can't we just use net.SplitHostPort ?

for sure! thanks!

Also, I can't think of a good reason why anyone would have set a single ServerName in their conf.Net.TLS.Config to use for all brokers, so I wonder if we should always override whatever is there by forming it with our hostname.

yeah, but I wonder if we would break any other client for doing this, looks like a breaking change.

@d1egoaz
Copy link
Contributor Author

d1egoaz commented May 15, 2020

@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))

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we return with error here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@dselyuzhitskiy
Copy link

Looks good, but suggestion from @dnwe is also interesting

@d1egoaz
Copy link
Contributor Author

d1egoaz commented May 18, 2020

@dnwe let us know what do you think about the latest changes.
I'm not sure about overriding the user's configuration, looks like this change might be for v2

@dnwe
Copy link
Collaborator

dnwe commented May 18, 2020

@d1egoaz lets go ahead and merge and get another minor version bump out 👍🏻

@d1egoaz d1egoaz merged commit 4d2231e into master May 19, 2020
@d1egoaz d1egoaz deleted the diego_clone-tls-config branch May 19, 2020 15:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Connection with multiple TLS brokers fails to negotiate
3 participants