-
Notifications
You must be signed in to change notification settings - Fork 15.2k
KAFKA-3618: Handle ApiVersionsRequest before SASL authentication #1286
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
Closed
Closed
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -42,13 +42,10 @@ public static Selector createSelector(ChannelBuilder channelBuilder) { | |
|
|
||
| public static void checkClientConnection(Selector selector, String node, int minMessageSize, int messageCount) throws Exception { | ||
|
|
||
| waitForChannelReady(selector, node); | ||
| String prefix = TestUtils.randomString(minMessageSize); | ||
| int requests = 0; | ||
| int responses = 0; | ||
| // wait for handshake to finish | ||
| while (!selector.isChannelReady(node)) { | ||
| selector.poll(1000L); | ||
| } | ||
| selector.send(new NetworkSend(node, ByteBuffer.wrap((prefix + "-0").getBytes()))); | ||
| requests++; | ||
| while (responses < messageCount) { | ||
|
|
@@ -66,6 +63,15 @@ public static void checkClientConnection(Selector selector, String node, int min | |
| } | ||
| } | ||
|
|
||
| public static void waitForChannelReady(Selector selector, String node) throws IOException { | ||
| // wait for handshake to finish | ||
| int secondsLeft = 30; | ||
| while (!selector.isChannelReady(node) && secondsLeft-- > 0) { | ||
| selector.poll(1000L); | ||
| } | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't we check that the channel is ready once we exit the while loop and if it is not, throw an exception?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
| assertTrue(selector.isChannelReady(node)); | ||
| } | ||
|
|
||
| public static void waitForChannelClose(Selector selector, String node) throws IOException { | ||
| boolean closed = false; | ||
| for (int i = 0; i < 30; i++) { | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
One impact of this implementation is that we allow a client to send any number of ApiVersion requests before the SaslHandshake request. It doesn't seem to hurt. However, it seems that requiring that a client issue a SaslHandshake request immediately after ApiVersion request probably makes the protocol simpler and easier to understand?
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.
If there isnt a strong reason to constrain the semantics to max one request prior to handshake I dont think we should.
In my view adding constraints like that actually make the protocol less simple, but it might make the broker implementation more simple. From the client's point of view a less strict protocol is easier to work with.
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.
Also, it will complicate the implementation quite a bit. Count requests per connection? We don't do it for any other request type...
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.
The broker implementation is more complicated, but not too complicated, we'd have to update
SaslStateor something like that. However, I also think that adding this restriction doesn't necessarily make things simpler overall (I can see Jun's point, but also Magnus's), so I'm +0 on leaving as is given that any change at this stage adds some risk.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.
@edenhill : Is there a use case for a client to ask for ApiVersionRequest more than once before sasl handshake? I thought a more strict protocol is easier for a client to deal with since there are few code paths that a client has to test.
@gwenshap : Yes, the implementation on the server side will be a bit more complicated to restrict the number of times ApiVersionRequest is issued before SaslHandshakeRequest, but not too much. The requests before authentication are already treated specially anyway. My main concern is on documenting the protocol. It's simpler if the protocol is
ApiVersionRequest? SaslHandShakeRequest SaslTokens
instead of
ApiVersionRequest* SaslHandShakeRequest SaslTokens
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.
@rajinisivaram : Yes, if you could submit a followup patch on this, that will be great. Thanks,
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 about the case where we add a new version of
ApiVersionsRequest. In that case, a client could issue anApiVersionsRequestwith a newer version and a 0.10 broker would reply with a response containing an error code. The client would then issue anApiVersionsRequestwith version 0.We are not taking advantage of this yet, but it's something we discussed. For clients, it's fine to add this support later, but we can't do that for brokers so they need the flexibility from the start.
Or am I missing something?
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.
Yes, very good point. To support that, we can allow multiple ApiVersionRequests before SaslHandshakeRequest as it is. However, we need to fix a few things in the code.
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.
Sounds good @junrao. @rajinisivaram, maybe you can add some tests to verify the desired behaviour as described by Jun along with the code changes? Thanks!
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.
@junrao @ijuma Implementation changes with unit tests are in the PR #1310