proxy: simplified packetIO and proxyv2 en(de)coding #17
Conversation
xhebox
changed the title
1. add `WError` to to wrap one error with another error. Refer the example for `errors.Wrap(..) error`. 2. add `errors.Collect(...) error` to wrap one error with multiple errors. 3. let all API return `error` directly instead of pointers of struct. Also remove `err.WithStack()` to keep errors imutable. Mutable errors may have race if they are initialized as global variables of general usage.
1. delete `buffered_read_conn.go`, use bufioRW direct.ly 2. close connection. 3. add methods to fake `initialHandshake`, expect `SSLRequest`. 4. add methods to read/write mysql packet. 5. add methods for proxy protocol encoding/decoding Signed-off-by: xhe <xw897002528@gmail.com>
xhebox
changed the title
pkg/proxy/net/packetio.go
Outdated
|
|
||
| // probe proxy V2 | ||
| refill := false | ||
| if p.proxy == nil && bytes.Compare(header[:], proxyV2Magic[:4]) == 0 { |
There was a problem hiding this comment.
How can you guarantee that the combination of length+sequence doesn't just happen to match proxyV2Magic?
There was a problem hiding this comment.
There is an extra check in parseProxyV2. proxyV2Magic has 12 bytes in total with a length 0xD0A0D and sequence 0xA. The only possible matches are OK response which are sent by server, or COM_SLEEP which is not long enough, and will failed to match on the second packet due to unknown command.
So yes, as long as the client behave correctly, there is no way. And if it does not, server will abort the connection eventually because of invalid proxy header or mysql packet.
I can make it safer: semantically speaking, this header is prepended at the beginning of handshake. So instead of p.proxy == nil, one-shot boolean will make it even more safer. And for server, we can just disable the probing.
pkg/proxy/net/packetio.go
Outdated
| return nil, false, errors.WithStack(errors.Wrap(ErrReadConn, err)) | ||
| } | ||
| return data, nil | ||
| return data, length == 0xffffff, nil |
There was a problem hiding this comment.
What's the difference of judging length == 0xffffff and len(data) < mysql.MaxPayloadLen?
There was a problem hiding this comment.
Not much. This change comes from my preference:
- It is more natural for me to move the juding of
hasMoreData?intoReadOnePacket, if the packet does said truth about following packets. ThenReadPackethas no assumptions on whether there is and why there is consecutive packets. - I personally prefer
==over<.
Signed-off-by: xhe <xw897002528@gmail.com>
Signed-off-by: xhe <xw897002528@gmail.com>
Signed-off-by: xhe <xw897002528@gmail.com>
Signed-off-by: xhe <xw897002528@gmail.com>
if write with a multiple `MaxPayload`, an extra zero-length packet is needed to end the packet continuous hint. Signed-off-by: xhe <xw897002528@gmail.com>
Signed-off-by: xhe <xw897002528@gmail.com>
| serverPkt, serverCapability, err = auth.readInitialHandshake(backendIO) | ||
| if serverPkt != nil { | ||
| writeErr := clientIO.WritePacket(serverPkt) | ||
| writeErr := clientIO.WritePacket(serverPkt, false) |
There was a problem hiding this comment.
You can merge the write and following flush() together here. But it's OK, I can do it in the next PR.
2 participants
What problem does this PR solve?
Issue Number: ref #14
Problem Summary: Simplifying the interface of
PacketIOwill enable us to remove redundantauthenticator.go.What is changed and how it works: Included in the commit message.
Check List
Tests
Notable changes
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.