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

server: compatible to old handshake protocol #8812

Merged
merged 6 commits into from Dec 26, 2018

Conversation

Projects
None yet
3 participants
@imtbkcat
Copy link
Contributor

imtbkcat commented Dec 25, 2018

What problem does this PR solve?

When using HAProxy connect to TiDB server, option mysql-check user root will cause handshake fail error because TiDB do not support Protocol::HandshakeResponse320

What is changed and how it works?

ParserProtocol::HandshakeResponse320 to Protocol::HandshakeResponse41, which is supported by TiDB.

Check List

Tests

  • Unit test

Code changes

  • Has exported function/method change

Side effects

  • Increased code complexity

Related changes

  • Need to cherry-pick to the release branch

This change is Reviewable

@imtbkcat imtbkcat requested a review from jackysp Dec 25, 2018

Show resolved Hide resolved server/conn.go

@imtbkcat imtbkcat force-pushed the imtbkcat:handshake320 branch from 7c09c6c to eae108c Dec 25, 2018

@imtbkcat imtbkcat requested a review from lysu Dec 25, 2018

@imtbkcat

This comment has been minimized.

Copy link
Contributor Author

imtbkcat commented Dec 25, 2018

/run-all-tests

@imtbkcat

This comment has been minimized.

Copy link
Contributor Author

imtbkcat commented Dec 25, 2018

/run-unit-test

@lysu lysu self-assigned this Dec 25, 2018

@imtbkcat imtbkcat force-pushed the imtbkcat:handshake320 branch from 3ffb782 to 43db3f5 Dec 26, 2018

@imtbkcat

This comment has been minimized.

Copy link
Contributor Author

imtbkcat commented Dec 26, 2018

/run-all-tests

var resp handshakeResponse41

pos, err := parseHandshakeResponseHeader(&resp, data)
if err != nil {
return errors.Trace(err)
if err == mysql.ErrMalformPacket {

This comment has been minimized.

Copy link
@lysu

lysu Dec 26, 2018

Member

Protocol::HandshakeResponse41:
Handshake Response Packet sent by 4.1+ clients supporting CLIENT_PROTOCOL_41 capability, if the server announced it in its Initial Handshake Packet. Otherwise (talking to an old server) the Protocol::HandshakeResponse320 packet must be used.

https://dev.mysql.com/doc/internals/en/connection-phase-packets.html

@imtbkcat can we direct use CLIENT_PROTOCOL_41 capability to decide use 41 or 32, because our Initial Handshake Packet always contain ClientProtocol41?

parse handshake twice has risk

@lysu
Copy link
Member

lysu left a comment

LGTM

// parseHandshakeResponseHeader parses the common header of SSLRequest and HandshakeResponse41.
func parseHandshakeResponseHeader(packet *handshakeResponse41, data []byte) (parsedBytes int, err error) {
// Ensure there are enough data to read:
// http://dev.mysql.com/doc/internals/en/connection-phase-packets.html#packet-Protocol::SSLRequest
if len(data) < 4+4+1+23 {
log.Errorf("Got malformed handshake response, packet data: %v", data)

This comment has been minimized.

Copy link
@jackysp

jackysp Dec 26, 2018

Member

It's better to add it back if we don't parse protocol 41 first.

This comment has been minimized.

Copy link
@imtbkcat

imtbkcat Dec 26, 2018

Author Contributor

ok

This comment has been minimized.

Copy link
@imtbkcat

imtbkcat Dec 26, 2018

Author Contributor

@jackysp addressed

This comment has been minimized.

Copy link
@jackysp

jackysp Dec 26, 2018

Member

Thanks!

@jackysp
Copy link
Member

jackysp left a comment

LGTM

@jackysp

This comment has been minimized.

Copy link
Member

jackysp commented Dec 26, 2018

/run-all-tests

@imtbkcat imtbkcat merged commit eedc6af into pingcap:master Dec 26, 2018

12 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
idc-jenkins-ci-tidb/build Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/common-test job succeeded
Details
idc-jenkins-ci-tidb/integration-common-test Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/integration-compatibility-test Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/integration-ddl-test Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/mybatis-test job succeeded
Details
idc-jenkins-ci-tidb/sqllogic-test-1 Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/sqllogic-test-2 Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/unit-test Jenkins job succeeded.
Details
license/cla Contributor License Agreement is signed.
Details

imtbkcat added a commit to imtbkcat/tidb that referenced this pull request Dec 27, 2018

imtbkcat added a commit to imtbkcat/tidb that referenced this pull request Dec 27, 2018

lamxTyler added a commit to lamxTyler/tidb that referenced this pull request Jan 2, 2019

yu34po added a commit to yu34po/tidb that referenced this pull request Jan 2, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.