Close channel on handshake error with old version #56989
Merged
Conversation
This file contains 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
Today a transport response uses the same wire format version as the corresponding request. This mostly works since we mostly know we are communicating with a node with a compatible version. TCP handshakes don't have this guarantee since they use `Version.CURRENT.minimumCompatibilityVersion()` to let us handshake with older nodes. This results in the strange situation of a node of major version `N` responding to a node of major version `N-1` using a wire format of version `N-2`. We put extra effort into the longer BWC requirements for successful responses, but we do not offer the same guarantees for error responses since they may be rather complicated to serialize. This can result in the sender misinterpreting the response which may have unpredictable consequences. Rather than strengthening the guarantees in this area, this commit simply logs the exception and closes the connection on a handshake error with a node that uses an incompatible wire format. Closes elastic#54337
DaveCTurner
added
>bug
:Distributed Coordination/Network
|
Pinging @elastic/es-distributed (:Distributed/Network) |
elasticmachine
added
the
Team:Distributed (Obsolete)
|
The change to production code here is small and seems reasonable, but I really struggled to come up with sensible tests. Perhaps there's some appropriate seam that I'm missing? Anyway, this is what I ended up with, suggestions for alternative testing approaches are very welcome. |
There was a problem hiding this comment.
Maybe we should just build this as a unit test for InboundHandler? The whole approach of testing this using the full transport services seems quite convoluted and in the end we're not mocking out the internals starting from where the message passes to InboundHandler anyway so it's not like we add coverage by doing that? (just a suggestion though, I realise it's a bit of extra effort to do this just for the sake of a having a nicer test)
Otherwise looks good I think from a quick read, just a suggestion on shortening things a little.
server/src/test/java/org/elasticsearch/transport/TransportServiceHandshakeTests.java
Outdated
Show resolved
Hide resolved
|
Ugh you're quite right this is now really just focussed on |
original-brownbear
approved these changes
May 20, 2020
|
CI failure is unrelated, I opened #57010. @elasticmachine please run elasticsearch-ci/1 |
DaveCTurner
added a commit
that referenced
this pull request
May 21, 2020
Today a transport response uses the same wire format version as the corresponding request. This mostly works since we mostly know we are communicating with a node with a compatible version. TCP handshakes don't have this guarantee since they use `Version.CURRENT.minimumCompatibilityVersion()` to let us handshake with older nodes. This results in the strange situation of a node of major version `N` responding to a node of major version `N-1` using a wire format of version `N-2`. We put extra effort into the longer BWC requirements for successful responses, but we do not offer the same guarantees for error responses since they may be rather complicated to serialize. This can result in the request sender misinterpreting the response which may have unpredictable consequences. Rather than strengthening the guarantees in this area, this commit simply logs the exception and closes the connection on a handshake error with a node that uses an incompatible wire format. Closes #54337
DaveCTurner
added a commit
that referenced
this pull request
May 21, 2020
This reverts commit c81a189.
DaveCTurner
added a commit
to DaveCTurner/elasticsearch
that referenced
this pull request
May 21, 2020
Today a transport response uses the same wire format version as the corresponding request. This mostly works since we mostly know we are communicating with a node with a compatible version. TCP handshakes don't have this guarantee since they use `Version.CURRENT.minimumCompatibilityVersion()` to let us handshake with older nodes. This results in the strange situation of a node of major version `N` responding to a node of major version `N-1` using a wire format of version `N-2`. We put extra effort into the longer BWC requirements for successful responses, but we do not offer the same guarantees for error responses since they may be rather complicated to serialize. This can result in the request sender misinterpreting the response which may have unpredictable consequences. Rather than strengthening the guarantees in this area, this commit simply logs the exception and closes the connection on a handshake error with a node that uses an incompatible wire format. Closes elastic#54337
DaveCTurner
added a commit
that referenced
this pull request
May 21, 2020
Today a transport response uses the same wire format version as the corresponding request. This mostly works since we mostly know we are communicating with a node with a compatible version. TCP handshakes don't have this guarantee since they use `Version.CURRENT.minimumCompatibilityVersion()` to let us handshake with older nodes. This results in the strange situation of a node of major version `N` responding to a node of major version `N-1` using a wire format of version `N-2`. We put extra effort into the longer BWC requirements for successful responses, but we do not offer the same guarantees for error responses since they may be rather complicated to serialize. This can result in the request sender misinterpreting the response which may have unpredictable consequences. Rather than strengthening the guarantees in this area, this commit simply logs the exception and closes the connection on a handshake error with a node that uses an incompatible wire format. Closes #54337
DaveCTurner
added a commit
that referenced
this pull request
May 21, 2020
Today a transport response uses the same wire format version as the corresponding request. This mostly works since we mostly know we are communicating with a node with a compatible version. TCP handshakes don't have this guarantee since they use `Version.CURRENT.minimumCompatibilityVersion()` to let us handshake with older nodes. This results in the strange situation of a node of major version `N` responding to a node of major version `N-1` using a wire format of version `N-2`. We put extra effort into the longer BWC requirements for successful responses, but we do not offer the same guarantees for error responses since they may be rather complicated to serialize. This can result in the request sender misinterpreting the response which may have unpredictable consequences. Rather than strengthening the guarantees in this area, this commit simply logs the exception and closes the connection on a handshake error with a node that uses an incompatible wire format. Closes #54337
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Today a transport response uses the same wire format version as the
corresponding request. This mostly works since we mostly know we are
communicating with a node with a compatible version. TCP handshakes don't have
this guarantee since they use
Version.CURRENT.minimumCompatibilityVersion()to let us handshake with older nodes. This results in the strange situation of
a node of major version
Nresponding to a node of major versionN-1using awire format of version
N-2.We put extra effort into the longer BWC requirements for successful responses,
but we do not offer the same guarantees for error responses since they may be
rather complicated to serialize. This can result in the request sender misinterpreting
the response which may have unpredictable consequences.
Rather than strengthening the guarantees in this area, this commit simply logs
the exception and closes the connection on a handshake error with a node that
uses an incompatible wire format.
Closes #54337