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.

Sign up for GitHub

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

Jump to bottom

KAFKA-7505: Process incoming bytes on write error to report SSL failures #5800

Merged
merged 5 commits into from Oct 18, 2018
Merged

KAFKA-7505: Process incoming bytes on write error to report SSL failures #5800

merged 5 commits into from Oct 18, 2018

Conversation

rajinisivaram
Copy link
Contributor

Committer Checklist (excluded from commit message)

  • Verify design and implementation
  • Verify test coverage and CI build status
  • Verify documentation (including upgrade notes)

ijuma
ijuma reviewed Oct 15, 2018
View reviewed changes
clients/src/main/java/org/apache/kafka/common/network/SslTransportLayer.java Outdated
// exception, but continue to handle the handshake failure as an authentication exception.
try {
if (flush)
flush(netWriteBuffer);
Copy link
Contributor

Choose a reason for hiding this comment

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

Previously, we would not throw the exception if flush returned false, but now we do. I assume that's intentional? May be worth explaining.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ijuma Thanks for the review. Sorry that was unintentional, updated code and comment.

@ijuma
Copy link
Contributor

ijuma commented Oct 15, 2018

For my understanding, the behaviour in Java 11 has changed?

@rajinisivaram
Copy link
Contributor Author

@ijuma I think there are changes in Java 11 which is causing these failures - I can recreate very easily with Java 11 but not with other versions. And we haven't seen these failures in the other builds.

ijuma
ijuma approved these changes Oct 16, 2018
View reviewed changes
Copy link
Contributor

@ijuma ijuma left a comment

Choose a reason for hiding this comment

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

LGTM, just a minor comment. No re-review needed.

clients/src/main/java/org/apache/kafka/common/network/SslTransportLayer.java Outdated
} catch (SSLException e1) {
maybeProcessHandshakeFailure(e1, false, e);
}
}

} while (netReadBuffer.position() > 0 && read > 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

Discussed this offline with @rajinisivaram and we think we think the first condition can be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ijuma Thanks for the review, updated.

@ijuma
Copy link
Contributor

ijuma commented Oct 16, 2018 •
edited

@rajinisivaram, it seems like this doesn't fix all issues:

java.lang.AssertionError: expected:<AUTHENTICATION_FAILED> but was:<AUTHENTICATE>
	at org.junit.Assert.fail(Assert.java:88)
	at org.junit.Assert.failNotEquals(Assert.java:834)
	at org.junit.Assert.assertEquals(Assert.java:118)
	at org.junit.Assert.assertEquals(Assert.java:144)
	at org.apache.kafka.common.network.NetworkTestUtils.waitForChannelClose(NetworkTestUtils.java:104)
	at org.apache.kafka.common.network.NetworkTestUtils.waitForChannelClose(NetworkTestUtils.java:109)
	at org.apache.kafka.common.network.SslTransportLayerTest.testListenerConfigOverride(SslTransportLayerTest.java:319)

@rajinisivaram
Copy link
Contributor Author

@ijuma handshakeUnwrap has a do-while loop that terminates if handshakeStatus != NEED_UNWRAP. For the exception case, we need to process any data remaining in the buffer as long as it can be unwrapped, regardless of handshakeStatus. Updated the code, will see how the PR build goes.

@rajinisivaram
Copy link
Contributor Author

retest this please

@rajinisivaram
Copy link
Contributor Author

retest this please

@rajinisivaram
Copy link
Contributor Author

@ijuma Haven't hit the issue with the last 3 builds though there have been some unrelated build failures. If you are ok with the change, shall I merge it?

@rajinisivaram
Copy link
Contributor Author

retest this please

ijuma
ijuma reviewed Oct 17, 2018
View reviewed changes
clients/src/main/java/org/apache/kafka/common/network/SslTransportLayer.java Outdated
// handshakeUnwrap unwraps data in the buffer, looping until handshakeStatus != NEED_UNWRAP. Here we
// want to process all incoming data regardless of handshakeStatus. If some data was unwrapped, but
// there is more remaining, try again while `handshakeUnwrap` is able to unwrap any data.
unwrappedAvailable = position != netReadBuffer.position() && netReadBuffer.position() != 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be simpler if we passed a parameter to handshakeUnwrap to control whether the handshake status was checked or not?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ijuma Thanks for the review, updated.

ijuma
ijuma approved these changes Oct 18, 2018
View reviewed changes
Copy link
Contributor

@ijuma ijuma left a comment

Choose a reason for hiding this comment

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

LGTM

@rajinisivaram
Copy link
Contributor Author

@ijuma Thanks for the review, merging to trunk and 2.1.

@rajinisivaram rajinisivaram merged commit 53d0674 into apache:trunk Oct 18, 2018
rajinisivaram added a commit that referenced this pull request Oct 18, 2018
@rajinisivaram
KAFKA-7505: Process incoming bytes on write error to report SSL failu…
75fdae2 
…res (#5800)

Reviewers: Ismael Juma <ismael@juma.me.uk>
Pengxiaolong pushed a commit to Pengxiaolong/kafka that referenced this pull request Jun 14, 2019
@rajinisivaram @Pengxiaolong
KAFKA-7505: Process incoming bytes on write error to report SSL failu…
4df54a9 
…res (apache#5800)

Reviewers: Ismael Juma <ismael@juma.me.uk>
lmr3796 pushed a commit to linkedin/kafka that referenced this pull request Jun 4, 2021
@rajinisivaram @lmr3796
KAFKA-7505: Process incoming bytes on write error to report SSL failu…
795aefb 
…res (apache#5800)

Reviewers: Ismael Juma <ismael@juma.me.uk>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ijuma ijuma ijuma approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@rajinisivaram @ijuma