RPC client should send pool ID on connection #846
Conversation
Now hybrid and mdcb clients can use secured connection. Added new boolean options to “slave_options” section: `use_ssl` and `ssl_insecure_skip_verify` MDCB part TykTechnologies/tyk-sink#10
Now we generate pool ID to unique identify Gateway instance, so MDCB can use it instead of API address. It is implemented on TCP level, by overriding Dial function. Once we have connection object, we write handshake “register” message, so Sink can distinguish legacy clients from new ones. Next we send length of ID (max 256, or single byte), and third message is ID itself.
api_definition_test.go
Outdated
| // Let gorpc handle it | ||
| if err != nil { | ||
| return | ||
| } |
There was a problem hiding this comment.
From the doc:
Read reads up to len(p) bytes into p. It returns the number of bytes read (0 <= n <= len(p)) and any error encountered.
So you could get n, err == 4, nil for example.
I believe you want io.ReadFull, which will make sure n == 8, even if multiple Read calls are necessary:
ReadFull reads exactly len(buf) bytes from r into buf.
There was a problem hiding this comment.
Also, you don't actually check that the string here is? It would also be more readable if you didn't hard-code the number 8 here, like:
const want = "register"
handshake := make([]byte, len(want))
[...]
There was a problem hiding this comment.
If client wrote 8 bytes on the wire, it will be always possible to read them in 1 single read. If it is not possible, it means that client wrote less than 8 bytes.
There was a problem hiding this comment.
Yeah we should introduce const for this.
There was a problem hiding this comment.
Right, I misunderstood what ReadFull does, you are right we can re-use it.
There was a problem hiding this comment.
Exactly - if the client wrote less, you want to error, not accept it :)
| } | ||
|
|
||
| type customListener struct { | ||
| L net.Listener |
There was a problem hiding this comment.
If you make this an embedded field, you could avoid defining ListenAddr and Close and just use Addr and Close from net.Listener.
There was a problem hiding this comment.
I just realized that I can't just embedd the field, because I need to override Accept function. Also ListenAddr is a go-rpc function, and this is actually is not net.Listener but gorpc.Listener interface, which differs.
There was a problem hiding this comment.
You can override methods from an embedded field - that's exactly what embedding is for. But you're right, I assumed the two interfaces were the same.
rpc_storage_handler.go
Outdated
|
|
||
| conn.Write([]byte("register")) | ||
| conn.Write([]byte{byte(len(RPCClientSingletonConnectionID))}) | ||
| conn.Write([]byte(RPCClientSingletonConnectionID)) |
There was a problem hiding this comment.
So this sends register, the length as a byte, and the ID itself? I assume the first two are so that we can change this "protocol" in the future more easily.
There was a problem hiding this comment.
Also, this is perhaps a bit silly, but I would add something like:
if len(RPCClientSingletonConnectionID) > 255 {
panic("RPCClientSingletonConnectionID is too long")
}
The silent overflow is a bit scary, even if we're sure that we won't touch this very often.
There was a problem hiding this comment.
the first two are so that we can change this "protocol" in the future more easily.
Exactly
The silent overflow is a bit scary, even if we're sure that we won't touch this very often.
Good point, because if we would like to change protocol, we for sure forgot about this limitation.
api_definition_test.go
Outdated
|
|
||
| if err != nil { | ||
| return | ||
| } |
There was a problem hiding this comment.
same ReadFull comment as above
api_definition_test.go
Outdated
| _, err = c.Read(idLenBuf) | ||
| if err != nil { | ||
| return | ||
| } |
There was a problem hiding this comment.
I'm not sure if the ReadFull thing applies here because you're only reading one byte, but you might as well do it out of consistency.
|
@buger approved for merge |
…mdcb_register_refactoring
If we decide to change length of the UUID in future it can never be more then 1 byte length
Now we generate pool ID to unique identify Gateway instance, so MDCB can use it instead of API address. It is implemented on TCP level, by overriding Dial function. Once we have a connection object, we write handshake “proto2” message, so Sink can distinguish legacy clients from new ones. Next, we send the length of ID (max 256, or single byte), and the third message is ID itself.
Now we generate pool ID to unique identify Gateway instance, so MDCB
can use it instead of API address.
It is implemented on TCP level, by overriding Dial function. Once we
have connection object, we write handshake “register” message, so Sink
can distinguish legacy clients from new ones. Next, we send the length of ID
(max 256, or single byte), and the third message is ID itself.
Note that tests include simple MDCB mock, which shows how we read the ID and apply it.
Part of https://github.com/TykTechnologies/tyk-sink/pull/11