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

KIP-368 : Allow SASL Connections to Periodically Re-Authenticate #2197

Merged
merged 1 commit into from
Apr 13, 2022

Conversation

k-wall
Copy link
Contributor

@k-wall k-wall commented Mar 29, 2022

Proposed fix for #2060.

If the server uses KIP-368, the client will cause itself to reauthenticate by emitting the necessary SASL handshake/authenticate with the next client to server interaction.

2022/03/29 18:09:50 Completed pre-auth SASL handshake. Available mechanisms: [PLAIN OAUTHBEARER]
2022/03/29 18:09:50 Session expiration in 299000 ms and session re-authentication on or after 283079 ms

@k-wall
Copy link
Contributor Author

k-wall commented Mar 31, 2022

The tests that are failing on Go 1.18/Kafka 2.8.1

TestFuncConsumerGroupPartitioningStateful
TestFuncConsumerGroupPartitioning

don't appear to be related to my change.

Copy link
Contributor

@mimaison mimaison left a comment

Choose a reason for hiding this comment

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

Thanks @k-wall for the PR. It looks good overall, I've left a couple of small suggestions

@@ -1288,6 +1298,10 @@ func (b *Broker) sendAndReceiveV1SASLPlainAuth() error {
return nil
}

func currentUnixMilli() int64 {
return time.Now().UnixNano() / int64(time.Millisecond)
Copy link
Contributor

Choose a reason for hiding this comment

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

The project aims at supporting the last couple of Golang version, so we could use time.Now().UnixMilli() that was introduced in 1.17. That said, it may be preferable to keep the current code for now to support older environments.

Copy link
Contributor Author

@k-wall k-wall Apr 8, 2022

Choose a reason for hiding this comment

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

The project is still on go 1.16, so I cannot adopt this API yet. Also, if I understand the project's "two releases and two months" right it'd be too soon to bump Go right now (1.18 only came out last month (March 2022), so it is too soon for the project to adopt 1.17).

broker.go Outdated Show resolved Hide resolved
broker.go Outdated Show resolved Hide resolved
@mimaison mimaison requested a review from dnwe April 6, 2022 14:24
Copy link
Collaborator

@dnwe dnwe left a comment

Choose a reason for hiding this comment

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

Excellent! Thank you for implementing this KIP. LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants