c2FmZQ
Workaround the quic-go v0.53 API changes
quic-go v0.53 changed its API significantly: https://github.com/quic-go/quic-go/releases/tag/v0.53.0
At first glance, it will break a lot of TLSPROXY functionality. Among other things, the instrumentation for ingress/egress metrics (and everything that depends on them, e.g. bw limits) will probably need to be removed. I don't think it will be possible to re-implement with the new API.
FYI @marten-seemann
Among other things, the instrumentation for ingress/egress metrics (and everything that depends on them, e.g. bw limits) will probably need to be removed. I don't think it will be possible to re-implement with the new API.
Why?
Among other things, the instrumentation for ingress/egress metrics (and everything that depends on them, e.g. bw limits) will probably need to be removed. I don't think it will be possible to re-implement with the new API.
Why?
I'm still assessing the impact of all the changes, but I know for sure that something like this won't work anymore, and very likely won't be possible to implement again.
func (s *QUICStream) Read(b []byte) (int, error) {
if l := s.qc.ingressLimiter; l != nil {
if err := l.WaitN(s.Context(), len(b)); err != nil {
return 0, err
}
}
n, err := s.Stream.Read(b)
if err != nil {
s.markReadDone()
}
s.qc.bytesReceived.Incr(int64(n))
s.qc.upBytesReceived.Incr(int64(n))
return n, err
}
(where QUICStream is a quic.Stream)
https://github.com/c2FmZQ/tlsproxy/blob/main/proxy/internal/netw/quic.go#L564
You can define your own interface with the methods of quic.Stream that your application needs. I believe this is the idiomatic pattern anyway: the consumer, not the provider, should define the interface.
I understand the idiomatic pattern. However, the consumer here is code that now expects a quic.Conn, for example https://pkg.go.dev/github.com/quic-go/[email protected]/http3#Server.ServeQUICConn
It is now impossible to send an instrumented quic.Conn to http3.Server.ServeQUICConn, because it only accepts a specific struct. Now, if that accepted an interface, I could still use an instrumented quic.Conn.
My implementation of quic.Connection has a lot of instrumentation for streams that are created on it.
Making quic.Conn a struct is not the problem. The problem is everything that used to accept a quic.Connection is now requiring a specific struct now.
Good point. We should return structs, but accept interfaces. So http3.Server.ServeQUICConn should accept an interface.
But what should that interface be? The HTTP/3 package could define a QUICConn interface:
type QUICConn interface {
OpenStream() (*quic.Stream, error)
OpenStreamSync(context.Context) (*quic.Stream, error)
OpenUniStream() (*quic.SendStream, error)
OpenUniStreamSync(context.Context) (*quic.SendStream, error)
AcceptStream(context.Context) (*quic.Stream, error)
AcceptUniStream(context.Context) (*quic.ReceiveStream, error)
Context() context.Context
LocalAddr() net.Addr
RemoteAddr() net.Addr
CloseWithError(quic.ApplicationErrorCode, string) error
ConnectionState() quic.ConnectionState
HandshakeComplete() <-chan struct{}
SendDatagram([]byte) error
ReceiveDatagram(context.Context) ([]byte, error)
}
However, now you still can't pass in your wrapped streams. But if we define the interface with interfaces for the streams as well, the quic.Conn doesn't satisfy this interface anymore:
type QUICConn interface {
OpenStream() (QUICStream, error)
OpenStreamSync(context.Context) (QUICStream, error)
OpenUniStream() (QUICSendStream, error)
OpenUniStreamSync(context.Context) (QUICSendStream, error)
AcceptStream(context.Context) (QUICStream, error)
AcceptUniStream(context.Context) (QUICReceiveStream, error)
// ...
}
What's the idiomatic solution?
I think this is a case where the only solution that works is the right solution: make OpenStream() and friends return interfaces. I can't think of anything else that would work.
To summarize:
The API change in quic-go and quic-go/http3 changed the quic.Connection and quic.Stream interfaces to concrete types. The motivation was to be idiomatic: return concrete types, accept interfaces.
The problem is that the http3 API now requires concrete types instead of interfaces, which makes it impossible to mock the input or use a different implementation.
The solution would be to change the http3 API to accept an interface wherever *quic.Conn is now required.
The question is what should the interface be.
If the interface itself requires concrete types, e.g.
type QUICConn interface {
OpenStream() (*quic.Stream, error)
OpenStreamSync(context.Context) (*quic.Stream, error)
OpenUniStream() (*quic.SendStream, error)
OpenUniStreamSync(context.Context) (*quic.SendStream, error)
// ...
}
we have the same problem. This QUICConn cannot be mocked or replaced with a different implementation because *quic.Stream is a concrete type that cannot be replaced.
My suggestion would be to use an interface for Stream, but that would require changing quic.Conn's OpenStream() and other functions to return an interface instead of a *quic.Stream concrete type. This breaks the return concrete types pattern, BUT it satisfies the accept interfaces pattern and restores functionality in http3.
This is not an isolated case. The same problem happens whenever concrete type functions return concrete types. The std library has many such examples where interface functions return io.Reader, io.Writer, ... There is no problem with a function that returns io.Reader instead of a concrete type if it helps with interoperability.
This model is completely valid and makes the go libraries so extendable. The quic-go library can use the same pattern for streams and other types to facilitate interoperability and extension.
This is not an isolated case. The same problem happens whenever concrete type functions return concrete types. The std library has many such examples where interface functions return
io.Reader,io.Writer, ... There is no problem with a function that returnsio.Readerinstead of a concrete type if it helps with interoperability.
Where does the standard library do that?
This model is completely valid and makes the go libraries so extendable. The quic-go library can use the same pattern for streams and other types to facilitate interoperability and extension.
With the same argument you could say that Conn should be an interface, since it should be possible to mock a quic.Listener. Where do we draw the line?
Where does the standard library do that?
The standard library provides interfaces for interoperability everywhere. The io and net packages are prime examples. These interfaces are used in other structs and interfaces. Without io.Reader and net.Conn, it would be pretty hard to build anything in go. A quic stream is not much different.
With the same argument you could say that
Connshould be an interface, since it should be possible to mock aquic.Listener. Where do we draw the line?
This is about interoperability. My personal opinion is that there was nothing wrong with the use of interfaces in v0.52. I have read your blog post about your motivation for getting rid of interfaces. I generally agree with the "return concrete types, accept interfaces" pattern, BUT the accept interfaces is the most important part. That's what makes interoperability possible.
To answer the question more directly, I do think that quic.Listener should return an interface so that it and Transport can be mocked. I don't think complex APIs can be designed properly in go without some thoughtful consideration of interfaces.
Where does the standard library do that?
The standard library provides interfaces for interoperability everywhere. The
ioandnetpackages are prime examples. These interfaces are used in other structs and interfaces. Withoutio.Readerandnet.Conn, it would be pretty hard to build anything in go. A quic stream is not much different.
Except it is... the io.Reader has 1 method, a QUIC stream has 10 (!), and with https://github.com/quic-go/quic-go/issues/4139 even more. This makes an io.Reader widely applicable across different use cases. It is hard to imagine that the massive quic.Stream interface would ever be used for something that's not a QUIC stream (a wrapped one, at best).
My question was: where does the standard library return interfaces instead of structs. bytes.NewReader returns a bytes.Reader, for example, and net.DialUDP returns a net.UDPConn.
BUT the accept interfaces is the most important part.
I agree that the current HTTP/3 API is not ideal, and I'd be happy to change it. The question is how.
That's a bad example. There are many different file systems: on disk, compressed archives, embedded, etc. This is not even remotely comparable to a QUIC stream. Please stop pushing on this, we will not change the API back to its messy prior state. Let's focus on finding a reasonable HTTP/3 API.
I'm just answering your questions, not pushing on anything. Feel free to un-CC yourself.
Let me know if you need anything else from me.
My plan for tlsproxy right now is to wait for the next quic-go release and reassess.
No hard feelings, I'm just trying to understand the use case and figure out a reasonable API for quic-go, so any feedback is appreciated!
I took some time to take a closer look at your code. I see that you're implementing some rate limiting on the stream level, and that's where being able to wrap a quic.Stream interface came in handy.
If I understand correctly, this would not be possible with the standard library HTTP/2 implementation: there's no way to gain access to the HTTP/2 stream. Hence, I'm not sure if it's a requirement for quic-go to support this use case, especially given that the API for that would be rather cumbersome (see https://github.com/quic-go/quic-go/pull/5239 for an incomplete attempt to make the http3 package work with interfaces).
I'm wondering if you could implement the rate limiting inside an HTTP handler instead. This would leave the HTTP header itself un-rate-limited, but I'm not sure if that's a deal breaker, given that the header is limited in size anyway.
For http/1, http/2, and the other TCP based connections, all the accounting is done on the incoming net.Conn. That's actually easier. For the QUIC connections, the accounting is done on streams. Not all traffic goes to http/3. So, yes, we could have an approximation of traffic with HTTP handlers, but we'd also need something else for non-HTTP traffic. This is really just one aspect of what makes wrapping the stream convenient though. I'm not going to go into that here.
The real problem is that the API change made using quic-go much more difficult. All the server code became much harder to test as you discovered yourself (https://github.com/quic-go/quic-go/issues/4968#issuecomment-2683894800). There is basically no way to mock anything other than streams now. I think you understand all that by now.
This is entirely an API problem. Designing good APIs is really hard. I know.
Let me know if you need anything else from me.
The real problem is that the API change made using quic-go much more difficult. All the server code became much harder to test as you discovered yourself (quic-go/quic-go#4968 (comment)). There is basically no way to mock anything other than streams now. I think you understand all that by now.
That's what I initially thought :) While working on the API change, I discovered that the extensive mocking made the tests very brittle and verbose. The current tests now use an actual QUIC connection, and I believe that the current tests are now a lot more meaningful (it also saved us ~4000 (!) LOC).
For http/1, http/2, and the other TCP based connections, all the accounting is done on the incoming
net.Conn.
I see. One could argue though that the QUIC equivalent would be counting bytes on the net.UDPConn, since only that way you'll be able to accurately account for the handshake, encryption and framing overhead. I wouldn't recommend doing so though since quic-go does a number of performance enhancements when it is able to operate on an unwrapped net.UDPConn.
I haven't made a decision yet on how to proceed with https://github.com/quic-go/quic-go/issues/5240. I totally understand your point that the old API was convenient. For now, I'm planning to keep the issue open and wait if other users run into similar issues.
My solution for now is to generate a set of interfaces and wrapper structs that look a lot like the old API. This is pretty straight forward using reflection.
https://pkg.go.dev/github.com/c2FmZQ/[email protected]/api
Then I updated the http3 library to use this new API instead of concrete structs (https://github.com/c2FmZQ/quic-go-api/commit/83e93f9ee56187b8c0aa78952cb8a1a2452ad234)
https://pkg.go.dev/github.com/c2FmZQ/[email protected]/http3
The result is something I can use for now. It is unfortunate that it requires a fork of the http3 library.
I split the quic-go-api repo into two, and made them compatible with mainline quic-go.
https://pkg.go.dev/github.com/c2FmZQ/quic-api https://pkg.go.dev/github.com/c2FmZQ/http3-go
quic-api contains the auto-generated interfaces and wrappers that look a lot like the pre-v0.53 API. I will keep it in sync with quic-go releases.
http3-go is a fork of quic-go/http3 that uses the quic-api interfaces instead of the concrete structs exposed by quic-go. I will also keep it in sync with quic-go releases. The code difference is mostly auto-generated too. It should be low maintenance.