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

Include a magic number in the TCP stream handshake #245

Open
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@benesch
Copy link
Contributor

benesch commented Mar 6, 2019

Higher layers may want to multiplex Timely traffic on a port that is also used for other protocols (e.g., HTTP). Sending a magic number immediately after establishing the stream makes it easy to sniff out Timely traffic.

benesch added some commits Mar 6, 2019

Avoid some mutable state in communication::networking::start_connections
There's no particularly compelling reason for this.
Include a magic number in the TCP stream handshake
Higher layers may want to multiplex Timely traffic on a port that is
also used for other protocols (e.g., HTTP). Sending a magic number
immediately after establishing the stream makes it easy to sniff out
Timely traffic.
@frankmcsherry

This comment has been minimized.

Copy link
Member

frankmcsherry commented Mar 7, 2019

In principle each timely instance should be talking to the same binaries, which we could also try to check. I wonder if it makes sense to try and generate a magic number at build time, to confirm that the binaries are talking to similar instances of themselves... ?

@benesch

This comment has been minimized.

Copy link
Contributor Author

benesch commented Mar 7, 2019

In principle each timely instance should be talking to the same binaries, which we could also try to check. I wonder if it makes sense to try and generate a magic number at build time, to confirm that the binaries are talking to similar instances of themselves... ?

Indeed it would! I was planning to do that at a higher layer of the stack (i.e., Material), since folks often frown upon doing anything too magical in a library, and Material will need to do its own version check anyway. But there's no harm in the redundancy, if you think its worth doing in Timely too.

Did you mean generating a magic number in build time in addition to or instead of the permafixed magic number? IMO its worth having both. The permafixed magic number reduces the burden on the TCP multiplexer, since it can do a blind equality comparison on the first u64 in the stream. By contrast, the PostgreSQL wire protocol, annoyingly starts a connection with one of five possible versions, with the possibility of expansion at any point; HTTP starts with one of "GET"/"HEAD"/"OPTIONS"/"POST"/"PUT'/....

You could argue that perhaps we shouldn't be multiplexing multiple protocols over the same port—that is what ports are for, after all—and you would be right, but experience has shown that when one application needs to speak multiple protocols, it is much easier on everyone involved if you can stuff all the protocols into one port. (Specifically, CockroachDB multiplexes GRPC and pgwire on 26257, but has to offload HTTP to another port for performance reasons; the separate HTTP port was a real nuisance and the multiplexed 26257 port was delightful and never caused any problems.)

if magic != &HANDSHAKE_MAGIC {
return Err(io::Error::new(io::ErrorKind::InvalidData,
"received incorrect timely handshake"));
}
let identifier = unsafe { decode::<u64>(&mut buffer) }.expect("failed to decode worker index").0.clone() as usize;

This comment has been minimized.

@frankmcsherry

frankmcsherry Mar 13, 2019

Member

This isn't going to read out the correct identifier, will it? It seems like it will just read out the magic number again.

This comment has been minimized.

@benesch

benesch Mar 18, 2019

Author Contributor

No, it works! buffer is advanced (i.e., reassigned to the remaining bytes that abomonation returns) on L118.

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.