QUIC proxy peering #47587
QUIC proxy peering #47587
Conversation
espadolini
added
the
no-changelog
espadolini
force-pushed
the
espadolini/quic-proxy-peering
branch
from
7163836
to
5fcc162
Compare
|
|
||
| // Sent from the server to the client as a response to a DialRequest. The | ||
| // message is likewise sent in protobuf binary format, prefixed by its length | ||
| // encoded as a little endian uint32. |
There was a problem hiding this comment.
nit: conventionally data sent over the wire is big endian. GRPC performs length prefixing using big endian uint32s. unless there is some compelling reason to not to, I'd suggest sticking with that convention.
There was a problem hiding this comment.
Counterpoint: it's 2024.
gRPC over HTTP/2 uses big endian for length prefixes because the HTTP/2 spec uses big endian and that's just how they happened to write the spec; protobuf itself uses little endian for any fixed-length integers, so "convention" should clearly not be a factor in any new protocol.
There was a problem hiding this comment.
Alright, ignoring convention... little endian byte order is an affront to god and nature and has no place in a civilized codebase. Especially for the case of a home-brewed API that we might be called upon to debug at some point, since visually parsing little endian data is annoying/weird.
There was a problem hiding this comment.
I also have a slight preference for big endian for over-the-wire data, if nothing else because I would expect it to be the case. That said, as long as it's well documented I'm OK with it.
| If the status is ok (signifying no error) then the stream will stay open, | ||
| carrying the data for the connection between the user and the agent, otherwise | ||
| the stream will be closed. For sanity's sake, the size of both messages is | ||
| limited and any oversized message is treated as a protocol violation. |
There was a problem hiding this comment.
How would it know that a message is oversized, though?
There was a problem hiding this comment.
We read the size first, if the message is oversized we just close the stream.
There was a problem hiding this comment.
I thought we capped at uint32, but later I saw we have a limit on top of the uint32, which then made this line make sense. Maybe we should mention that we have an arbitrary limit, so the messages can't use the full uint32 length?
lib/proxy/peer/quicserver.go
Outdated
| // an empty protobuf message has an empty wire encoding, so by sending a | ||
| // size of 0 (i.e. four zero bytes) we are sending an empty DialResponse | ||
| // with an empty Status, which signifies a successful dial | ||
| if _, err := st.Write(binary.LittleEndian.AppendUint32(nil, 0)); err != nil { |
There was a problem hiding this comment.
This is clever and all but I'd rather we just did a send() call with the corresponding "OK" message.
There was a problem hiding this comment.
I don't want to bring in a fallible operation that allocates a bunch of times and initializes hidden state just to send four NULs.
There was a problem hiding this comment.
You can marshal an empty message once and reuse if you like, but the readability is needlessly suffering here.
lib/proxy/peer/quicserver.go
Outdated
| // available streams during a connection (so we can set it to 0) | ||
| st, err := c.AcceptStream(context.Background()) | ||
| if err != nil { | ||
| log.DebugContext(c.Context(), "Got an error accepting a stream.", "error", err) |
There was a problem hiding this comment.
As a general comment, I'd rather this function returned an error and the caller made the choice to swallow it.
There was a problem hiding this comment.
The only possible error is caused by the connection getting closed - which is also why the log line is at debug level. I'm not convinced that moving the error logging one layer above will do much for the clarity of the code, at least while this is the only exit point for the function.
There was a problem hiding this comment.
Moving the logging up makes this behave like a regular erroring function, which is already a valuable readability improvement IMO.
There was a problem hiding this comment.
The logging happens before the defers, logging after returning would mean that the log line is related to an error that happened potentially much earlier, seeing as now we're waiting for the per-connection waitgroup to end.
|
|
||
| syntax = "proto3"; | ||
|
|
||
| package teleport.quicpeering.v1alpha; |
There was a problem hiding this comment.
Maybe:
| package teleport.quicpeering.v1alpha; | |
| package teleport.quicpeering.v1alpha1; |
Bold of you to assume there will be only one alpha... ;)
There was a problem hiding this comment.
v1alpha seems to edge out (slightly) v1alpha1 in terms of google search result count - I wouldn't be opposed to v1alpha2 as a followup to v1alpha, either.
There was a problem hiding this comment.
I guess it really depends on whether we expect more alphas. If yes, then go v1alpha1. Otherwise we can do v1alpha to v1alpha2 like you said.
espadolini
force-pushed
the
espadolini/quic-proxy-peering
branch
from
5fcc162
to
a6678e0
Compare
espadolini
force-pushed
the
espadolini/quic-proxy-peering
branch
from
a6678e0
to
5fad0d5
Compare
No description provided.