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

Add support for SASL plain text authentication #648

Merged
merged 11 commits into from
May 5, 2016
Merged

Add support for SASL plain text authentication #648

merged 11 commits into from
May 5, 2016

Conversation

rshriram
Copy link

Hi
This is a simple patch that adds support for SASL plain text authentication to Kafka brokers, if enabled. This feature is useful when people want to leverage hosted Kafka services such as IBM Bluemix's Message Hub service (Kafka 0.9 with SASL plain text authentication). By default, the authentication is disabled.

@@ -222,6 +233,7 @@ func NewConfig() *Config {
c.Net.DialTimeout = 30 * time.Second
c.Net.ReadTimeout = 30 * time.Second
c.Net.WriteTimeout = 30 * time.Second
c.Net.SASL.Enable = false
Copy link
Contributor

Choose a reason for hiding this comment

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

you don't need to set this, false is the default for go booleans

@rshriram
Copy link
Author

rshriram commented May 2, 2016

Hi, I have addressed all of your comments. See if this version of PR is good enough.

if conf.Net.SASL.Enable {
err = b.doSASLPlainAuth()
if err != nil {
Logger.Printf("SASL authentication with broker %s failed\n", b.addr)
Copy link
Contributor

Choose a reason for hiding this comment

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

you log every case in doSASLPlainAuth and both cases here, that seems a bit much?

@rshriram
Copy link
Author

rshriram commented May 3, 2016

Any pointers as to why the build fails with Go 1.6.1 ? The log seems to go on for ever.

@eapache
Copy link
Contributor

eapache commented May 3, 2016

It's just a test that occasionally gets stuck; I've restarted it and it should pass this time.

@eapache
Copy link
Contributor

eapache commented May 3, 2016

This looks OK to me now I think; @wvanbergen ?

// When credentials are valid, Kafka returns a 4 byte array of null characters.
// When credentials are invalid, Kafka closes the connection. This does not seem to be the ideal way
// of responding to bad credentials but thats how its being done today.
func (b *Broker) doSASLPlainAuth() error {
Copy link
Contributor

@wvanbergen wvanbergen May 3, 2016

Choose a reason for hiding this comment

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

as name, I prefer sendAndReceiveSASLPlainAuth which is more in line with naming we have elsewhere

Copy link
Author

Choose a reason for hiding this comment

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

fixed

@rshriram
Copy link
Author

rshriram commented May 5, 2016

Hi
Would you like any more changes before merging this PR?

@wvanbergen
Copy link
Contributor

I am ok with merging this 👍. @eapache I will let you do the honours

@eapache
Copy link
Contributor

eapache commented May 5, 2016

👍 thanks

@eapache eapache merged commit 89bd629 into IBM:master May 5, 2016
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.

3 participants