QUIC proxy peering #47587

espadolini · 2024-10-15T18:19:51Z

No description provided.

lib/proxy/peer/quicserver.go

lib/proxy/peer/quicclient.go

fspmarshall · 2024-10-16T17:42:07Z

proto/teleport/quicpeering/v1alpha/dial.proto

+
+// 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.


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.

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.

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.

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.

codingllama

Quick (no pun intended) look at API and docs.

proto/teleport/quicpeering/v1alpha/dial.proto

codingllama · 2024-10-16T19:48:20Z

lib/proxy/peer/quic.go

+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.


How would it know that a message is oversized, though?

We read the size first, if the message is oversized we just close the stream.

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/quic.go

lib/proxy/peer/quicclient.go

lib/service/servicecfg/proxy.go

codingllama

Server review. I think you would benefit from someone who understands QUIC (if we have any), but I did my best.

I'll wait for replies before catching up to the client, so you have time to catch up to it all.

codingllama · 2024-10-16T21:12:22Z

lib/proxy/peer/quicserver.go

+ // Note: it won't actually have an effect, since QUIC always uses TLS 1.3,
+ // and TLS 1.3 ciphersuites can't be configured in crypto/tls, but for
+ // consistency's sake this should be passed along from the agent
+ // configuration.
 CipherSuites []uint16


If this has no effect then what is the point in having it?

I thought it was the easiest way to prevent "Why aren't you using utils.TLSConfig?" questions.

Kill the field and move the comment to type itself, or next to the appropriate initialization?

lib/proxy/peer/quicserver.go

codingllama · 2024-10-16T21:24:39Z

lib/proxy/peer/quicserver.go

+ s.wg.Add(1)
+ go s.handleConn(c)


Don't spread out concurrency control, that makes ownership loose and makes it harder to reason about.

Suggested change

s.wg.Add(1)

go s.handleConn(c)

s.wg.Add(1)

go func() {

s.handleConn(c)

s.wg.Done()

}()

That would make sense if not for the fact that handleConn still needs to interact with the waitgroup when spawning goroutines for each stream - and this is actually somewhat important, because moving the wg.Done() out of handleConn would mean that the wg.Add(1) in it becomes very suspect, as it wouldn't be "guarded" by an active waitgroup count and thus could potentially race against waitgroup waiters.

I politely disagree - both Serve and handleConn could use this same pattern to take care of the goroutines they spawn. A method shouldn't have to know it's running as a goroutine, or mandate this kind of setup to be called.

lib/proxy/peer/quicserver.go

codingllama · 2024-10-16T21:50:19Z

lib/proxy/peer/quicserver.go

+ // 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 {


This is clever and all but I'd rather we just did a send() call with the corresponding "OK" message.

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.

You can marshal an empty message once and reuse if you like, but the readability is needlessly suffering here.

codingllama · 2024-10-16T21:50:52Z

lib/proxy/peer/quicserver.go

+ _, err := io.Copy(nodeConn, st)
+ return trace.Wrap(err)
+ })
+ _ = eg.Wait()


Log the error?

I'll give it a shot, I suspect it would be pretty spammy tho.

Same for debug or trace level.

codingllama · 2024-10-16T21:51:38Z

lib/proxy/peer/quicserver.go

+ // 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)


As a general comment, I'd rather this function returned an error and the caller made the choice to swallow it.

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.

Moving the logging up makes this behave like a regular erroring function, which is already a valuable readability improvement IMO.

codingllama · 2024-10-16T21:53:36Z

proto/teleport/quicpeering/v1alpha/dial.proto

+
+syntax = "proto3";
+
+package teleport.quicpeering.v1alpha;


Maybe:

Suggested change

package teleport.quicpeering.v1alpha;

package teleport.quicpeering.v1alpha1;

Bold of you to assume there will be only one alpha... ;)

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.

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.

codingllama

Thanks for the added documentation/context. Very helpful!

codingllama · 2024-10-18T15:25:47Z

proto/teleport/quicpeering/v1alpha/dial.proto

+// Sent from a proxy to a peer proxy in a fresh QUIC stream to dial a Teleport
+// resource through a QUIC proxy peering connection. The message is sent in
+// protobuf binary format, prefixed by its length encoded as a little endian
+// 32-bit integer.


Suggested change

// 32-bit integer.

// 32-bit unsigned integer.

codingllama · 2024-10-18T15:34:18Z

lib/service/service.go

@@ -4767,13 +4771,13 @@ func (process *TeleportProcess) initProxyEndpoint(conn *Connector) error {

 process.RegisterCriticalFunc("proxy.peer.quic", func() error {
 if _, err := process.WaitForEvent(process.ExitContext(), ProxyReverseTunnelReady); err != nil {
- logger.DebugContext(process.ExitContext(), "Process exiting: failed to start QUIC peer proxy service waiting for reverse tunnel server.")
+ logger.DebugContext(process.ExitContext(), "process exiting: failed to start QUIC peer proxy service waiting for reverse tunnel server")


nit: I think the "agreed" logging style is to capitalize the first word but avoid punctuation.

See the examples in RFD 0154 and this section:

The message should be a fragment, and not a full sentence. Terminating the message with punctuation should be avoided.

https://github.com/gravitational/teleport/blob/master/rfd/0154-logging-guidelines.md#logging-standards

Word of @rosstimothy is that the message shouldn't be capitalized and "the message should just be another key-value pair" (presumably with "message" as the key), although I agree that the RFD could be clearer on that.

We really should update the examples and add a section stating the preferred style, I think very few of us are getting this "right" (me included apparently).

espadolini added the no-changelog Indicates that a PR does not require a changelog entry label Oct 15, 2024

espadolini requested review from codingllama and fspmarshall October 15, 2024 18:19

fspmarshall reviewed Oct 15, 2024

View reviewed changes

lib/proxy/peer/quicserver.go Outdated Show resolved Hide resolved

lib/proxy/peer/quicclient.go Show resolved Hide resolved

espadolini force-pushed the espadolini/quic-proxy-peering branch from 7163836 to 5fcc162 Compare October 16, 2024 16:24

fspmarshall reviewed Oct 16, 2024

View reviewed changes

codingllama reviewed Oct 16, 2024

View reviewed changes

espadolini force-pushed the espadolini/quic-proxy-peering branch from 5fcc162 to a6678e0 Compare October 16, 2024 23:45

espadolini changed the base branch from master to espadolini/proxypeer-slog October 16, 2024 23:45

Base automatically changed from espadolini/proxypeer-slog to master October 17, 2024 13:14

espadolini added 13 commits October 17, 2024 16:15

Check UNSTABLE envvar in lib/config

faeaed9

QUIC proxy peering server

82ab0c5

QUIC proxy peering client

a120775

QUIC proxy peering: a brief explanation

ca1b1ee

fix deferred log

fef9fc1

Always require replay protection

5d476c2

Explain the waitgroup add in dial

f22efff

Convert client to slog

f50558a

Clarify proxy label

9829c6c

logging style

2300c20

protobuf tweaks to address comments

f804827

Avoid sending oversized messages

c32aaa5

Explanations and consts

5fad0d5

espadolini force-pushed the espadolini/quic-proxy-peering branch from a6678e0 to 5fad0d5 Compare October 17, 2024 14:16

espadolini added 3 commits October 17, 2024 16:20

Elaborate slightly further on replay protections

c03013f

Expanding comments and renaming

1cb25d5

QUICServer struct docs

e9ea132

espadolini requested a review from codingllama October 17, 2024 14:51

fix stream cancellation after 0-RTT rejection

027286f

codingllama reviewed Oct 18, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

QUIC proxy peering #47587

QUIC proxy peering #47587

espadolini commented Oct 15, 2024

fspmarshall Oct 16, 2024

espadolini Oct 16, 2024

fspmarshall Oct 16, 2024

codingllama Oct 16, 2024

codingllama left a comment

codingllama Oct 16, 2024

espadolini Oct 17, 2024

codingllama Oct 18, 2024

codingllama left a comment

codingllama Oct 16, 2024

espadolini Oct 17, 2024

codingllama Oct 18, 2024

codingllama Oct 16, 2024

espadolini Oct 17, 2024

codingllama Oct 18, 2024

codingllama Oct 16, 2024

espadolini Oct 17, 2024 •

edited

Loading

codingllama Oct 18, 2024

codingllama Oct 16, 2024

espadolini Oct 17, 2024

codingllama Oct 18, 2024

codingllama Oct 16, 2024

espadolini Oct 17, 2024

codingllama Oct 18, 2024

codingllama Oct 16, 2024

espadolini Oct 16, 2024

codingllama Oct 18, 2024

codingllama left a comment

codingllama Oct 18, 2024

codingllama Oct 18, 2024

espadolini Oct 18, 2024

codingllama Oct 18, 2024

	package teleport.quicpeering.v1alpha;
	package teleport.quicpeering.v1alpha1;

QUIC proxy peering #47587

Are you sure you want to change the base?

QUIC proxy peering #47587

Conversation

espadolini commented Oct 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codingllama left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codingllama left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

espadolini Oct 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codingllama left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

espadolini Oct 17, 2024 •

edited

Loading