From ec171255ceae01282e075cdea9d59433726ab802 Mon Sep 17 00:00:00 2001 From: Ivan Trubach Date: Fri, 20 May 2022 12:53:23 +0300 Subject: [PATCH] do not embed http.Server in http3.Server (#3397) This change removes the embedded http.Server struct from http3.Server. It slightly changes some error behavior, in particular, it mandates TLSConfig for methods that create QUIC listeners. Before this change, only Addr, TLSConfig, Handler and MaxHeaderBytes options were used from the http.Server. These are now defined directly in http3.Server with an improved documentation. --- example/main.go | 3 +- http3/server.go | 105 +++++++++++++++++--------- http3/server_test.go | 40 +++++----- integrationtests/self/hotswap_test.go | 12 +-- integrationtests/self/http_test.go | 6 +- interop/server/main.go | 6 +- 6 files changed, 98 insertions(+), 74 deletions(-) diff --git a/example/main.go b/example/main.go index 9f53b5f9da8..e45245b5d03 100644 --- a/example/main.go +++ b/example/main.go @@ -185,7 +185,8 @@ func main() { err = http3.ListenAndServe(bCap, certFile, keyFile, handler) } else { server := http3.Server{ - Server: &http.Server{Handler: handler, Addr: bCap}, + Handler: handler, + Addr: bCap, QuicConfig: quicConf, } err = server.ListenAndServeTLS(testdata.GetCertificatePaths()) diff --git a/http3/server.go b/http3/server.go index e9eafda2fa3..efb0a530894 100644 --- a/http3/server.go +++ b/http3/server.go @@ -126,35 +126,64 @@ type listenerInfo struct { // Server is a HTTP/3 server. type Server struct { - *http.Server + // Addr optionally specifies the UDP address for the server to listen on, + // in the form "host:port". + // + // When used by ListenAndServe and ListenAndServeTLS methods, if empty, + // ":https" (port 443) is used. See net.Dial for details of the address + // format. + // + // Otherwise, if Port is not set and underlying QUIC listeners do not + // have valid port numbers, the port part is used in Alt-Svc headers set + // with SetQuicHeaders. + Addr string + + // Port is used in Alt-Svc response headers set with SetQuicHeaders. If + // needed Port can be manually set when the Server is created. + // + // This is useful when a Layer 4 firewall is redirecting UDP traffic and + // clients must use a port different from the port the Server is + // listening on. + Port int + + // TLSConfig provides a TLS configuration for use by server. It must be + // set for ListenAndServe and Serve methods. + TLSConfig *tls.Config - // By providing a quic.Config, it is possible to set parameters of the QUIC connection. - // If nil, it uses reasonable default values. + // QuicConfig provides the parameters for QUIC connection created with + // Serve. If nil, it uses reasonable default values. + // + // Configured versions are also used in Alt-Svc response header set with + // SetQuicHeaders. QuicConfig *quic.Config - // Enable support for HTTP/3 datagrams. + // Handler is the HTTP request handler to use. If not set, defaults to + // http.NotFound. + Handler http.Handler + + // EnableDatagrams enables support for HTTP/3 datagrams. // If set to true, QuicConfig.EnableDatagram will be set. // See https://datatracker.ietf.org/doc/html/draft-ietf-masque-h3-datagram-07. EnableDatagrams bool - // The port to use in Alt-Svc response headers. - // If needed Port can be manually set when the Server is created. - // This is useful when a Layer 4 firewall is redirecting UDP traffic and clients must use - // a port different from the port the Server is listening on. - Port int + // MaxHeaderBytes controls the maximum number of bytes the server will + // read parsing the request HEADERS frame. It does not limit the size of + // the request body. If zero or negative, http.DefaultMaxHeaderBytes is + // used. + MaxHeaderBytes int - // Additional HTTP/3 settings. + // AdditionalSettings specifies additional HTTP/3 settings. // It is invalid to specify any settings defined by the HTTP/3 draft and the datagram draft. AdditionalSettings map[uint64]uint64 - // When set, this callback is called for the first unknown frame parsed on a bidirectional stream. + // StreamHijacker, when set, is called for the first unknown frame parsed on a bidirectional stream. // It is called right after parsing the frame type. // Callers can either process the frame and return control of the stream back to HTTP/3 // (by returning hijacked false). // Alternatively, callers can take over the QUIC stream (by returning hijacked true). StreamHijacker func(FrameType, quic.Connection, quic.Stream) (hijacked bool, err error) - // When set, this callback is called for unknown unidirectional stream of unknown stream type. + // UniStreamHijacker, when set, is called for unknown unidirectional stream of unknown stream type. UniStreamHijacker func(StreamType, quic.Connection, quic.ReceiveStream) (hijacked bool) mutex sync.RWMutex @@ -168,14 +197,15 @@ type Server struct { } // ListenAndServe listens on the UDP address s.Addr and calls s.Handler to handle HTTP/3 requests on incoming connections. +// +// If s.Addr is blank, ":https" is used. func (s *Server) ListenAndServe() error { - if s.Server == nil { - return errors.New("use of http3.Server without http.Server") - } return s.serveConn(s.TLSConfig, nil) } // ListenAndServeTLS listens on the UDP address s.Addr and calls s.Handler to handle HTTP/3 requests on incoming connections. +// +// If s.Addr is blank, ":https" is used. func (s *Server) ListenAndServeTLS(certFile, keyFile string) error { var err error certs := make([]tls.Certificate, 1) @@ -203,10 +233,6 @@ func (s *Server) Serve(conn net.PacketConn) error { // and use it to construct a http3-friendly QUIC listener. // Closing the server does close the listener. func (s *Server) ServeListener(ln quic.EarlyListener) error { - if s.Server == nil { - return errors.New("use of http3.Server without http.Server") - } - if err := s.addListener(&ln); err != nil { return err } @@ -215,9 +241,18 @@ func (s *Server) ServeListener(ln quic.EarlyListener) error { return err } +var errServerWithoutTLSConfig = errors.New("use of http3.Server without TLSConfig") + func (s *Server) serveConn(tlsConf *tls.Config, conn net.PacketConn) error { - if s.Server == nil { - return errors.New("use of http3.Server without http.Server") + if tlsConf == nil { + return errServerWithoutTLSConfig + } + + s.mutex.Lock() + closed := s.closed + s.mutex.Unlock() + if closed { + return http.ErrServerClosed } baseConf := ConfigureTLSConfig(tlsConf) @@ -234,7 +269,11 @@ func (s *Server) serveConn(tlsConf *tls.Config, conn net.PacketConn) error { var ln quic.EarlyListener var err error if conn == nil { - ln, err = quicListenAddr(s.Addr, baseConf, quicConf) + addr := s.Addr + if addr == "" { + addr = ":https" + } + ln, err = quicListenAddr(addr, baseConf, quicConf) } else { ln, err = quicListen(conn, baseConf, quicConf) } @@ -462,10 +501,10 @@ func (s *Server) handleUnidirectionalStreams(conn quic.EarlyConnection) { } func (s *Server) maxHeaderBytes() uint64 { - if s.Server.MaxHeaderBytes <= 0 { + if s.MaxHeaderBytes <= 0 { return http.DefaultMaxHeaderBytes } - return uint64(s.Server.MaxHeaderBytes) + return uint64(s.MaxHeaderBytes) } func (s *Server) handleRequest(conn quic.Connection, str quic.Stream, decoder *qpack.Decoder, onFrameError func()) requestError { @@ -609,10 +648,8 @@ func (s *Server) SetQuicHeaders(hdr http.Header) error { // used when handler is nil. func ListenAndServeQUIC(addr, certFile, keyFile string, handler http.Handler) error { server := &Server{ - Server: &http.Server{ - Addr: addr, - Handler: handler, - }, + Addr: addr, + Handler: handler, } return server.ListenAndServeTLS(certFile, keyFile) } @@ -635,6 +672,10 @@ func ListenAndServe(addr, certFile, keyFile string, handler http.Handler) error Certificates: certs, } + if addr == "" { + addr = ":https" + } + // Open the listeners udpAddr, err := net.ResolveUDPAddr("udp", addr) if err != nil { @@ -660,13 +701,9 @@ func ListenAndServe(addr, certFile, keyFile string, handler http.Handler) error defer tlsConn.Close() // Start the servers - httpServer := &http.Server{ - Addr: addr, - TLSConfig: config, - } - + httpServer := &http.Server{} quicServer := &Server{ - Server: httpServer, + TLSConfig: config, } if handler == nil { diff --git a/http3/server_test.go b/http3/server_test.go index 4b99775a60c..bcf50268cd0 100644 --- a/http3/server_test.go +++ b/http3/server_test.go @@ -92,10 +92,8 @@ var _ = Describe("Server", func() { BeforeEach(func() { s = &Server{ - Server: &http.Server{ - TLSConfig: testdata.GetTLSConfig(), - }, - logger: utils.DefaultLogger, + TLSConfig: testdata.GetTLSConfig(), + logger: utils.DefaultLogger, } origQuicListenAddr = quicListenAddr }) @@ -530,7 +528,7 @@ var _ = Describe("Server", func() { }) It("errors when the client sends a too large header frame", func() { - s.Server.MaxHeaderBytes = 20 + s.MaxHeaderBytes = 20 s.Handler = http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { Fail("Handler should not be called.") }) @@ -758,22 +756,18 @@ var _ = Describe("Server", func() { }) }) - It("errors when ListenAndServe is called with s.Server nil", func() { - Expect((&Server{}).ListenAndServe()).To(MatchError("use of http3.Server without http.Server")) - }) - - It("errors when ListenAndServeTLS is called with s.Server nil", func() { - Expect((&Server{}).ListenAndServeTLS(testdata.GetCertificatePaths())).To(MatchError("use of http3.Server without http.Server")) + It("errors when ListenAndServe is called with s.TLSConfig nil", func() { + Expect((&Server{}).ListenAndServe()).To(MatchError(errServerWithoutTLSConfig)) }) It("should nop-Close() when s.server is nil", func() { Expect((&Server{}).Close()).To(Succeed()) }) - It("errors when ListenAndServe is called after Close", func() { - serv := &Server{Server: &http.Server{}} + It("errors when ListenAndServeTLS is called after Close", func() { + serv := &Server{} Expect(serv.Close()).To(Succeed()) - Expect(serv.ListenAndServe()).To(MatchError(http.ErrServerClosed)) + Expect(serv.ListenAndServeTLS(testdata.GetCertificatePaths())).To(MatchError(http.ErrServerClosed)) }) It("handles concurrent Serve and Close", func() { @@ -836,8 +830,9 @@ var _ = Describe("Server", func() { return ln, nil } - s := &Server{Server: &http.Server{}} - s.TLSConfig = &tls.Config{} + s := &Server{ + TLSConfig: &tls.Config{}, + } stopAccept := make(chan struct{}) ln.EXPECT().Accept(gomock.Any()).DoAndReturn(func(context.Context) (quic.Connection, error) { @@ -870,8 +865,9 @@ var _ = Describe("Server", func() { return <-lns, nil } - s := &Server{Server: &http.Server{}} - s.TLSConfig = &tls.Config{} + s := &Server{ + TLSConfig: &tls.Config{}, + } stopAccept1 := make(chan struct{}) ln1.EXPECT().Accept(gomock.Any()).DoAndReturn(func(context.Context) (quic.Connection, error) { @@ -924,8 +920,7 @@ var _ = Describe("Server", func() { return ln, nil } - s := &Server{Server: &http.Server{}} - s.TLSConfig = &tls.Config{} + s := &Server{} stopAccept := make(chan struct{}) ln.EXPECT().Accept(gomock.Any()).DoAndReturn(func(context.Context) (quic.Connection, error) { @@ -959,8 +954,7 @@ var _ = Describe("Server", func() { return <-lns, nil } - s := &Server{Server: &http.Server{}} - s.TLSConfig = &tls.Config{} + s := &Server{} stopAccept1 := make(chan struct{}) ln1.EXPECT().Accept(gomock.Any()).DoAndReturn(func(context.Context) (quic.Connection, error) { @@ -1001,7 +995,7 @@ var _ = Describe("Server", func() { Context("ListenAndServe", func() { BeforeEach(func() { - s.Server.Addr = "localhost:0" + s.Addr = "localhost:0" }) AfterEach(func() { diff --git a/integrationtests/self/hotswap_test.go b/integrationtests/self/hotswap_test.go index b76bc772ee5..fdeeb5d9e5a 100644 --- a/integrationtests/self/hotswap_test.go +++ b/integrationtests/self/hotswap_test.go @@ -87,18 +87,14 @@ var _ = Describe("HTTP3 Server hotswap test", func() { }) server1 = &http3.Server{ - Server: &http.Server{ - Handler: mux1, - TLSConfig: testdata.GetTLSConfig(), - }, + Handler: mux1, + TLSConfig: testdata.GetTLSConfig(), QuicConfig: getQuicConfig(&quic.Config{Versions: versions}), } server2 = &http3.Server{ - Server: &http.Server{ - Handler: mux2, - TLSConfig: testdata.GetTLSConfig(), - }, + Handler: mux2, + TLSConfig: testdata.GetTLSConfig(), QuicConfig: getQuicConfig(&quic.Config{Versions: versions}), } diff --git a/integrationtests/self/http_test.go b/integrationtests/self/http_test.go index e2288ea9e6b..0a4e440d3c7 100644 --- a/integrationtests/self/http_test.go +++ b/integrationtests/self/http_test.go @@ -68,10 +68,8 @@ var _ = Describe("HTTP tests", func() { }) server = &http3.Server{ - Server: &http.Server{ - Handler: mux, - TLSConfig: testdata.GetTLSConfig(), - }, + Handler: mux, + TLSConfig: testdata.GetTLSConfig(), QuicConfig: getQuicConfig(&quic.Config{Versions: versions}), } diff --git a/interop/server/main.go b/interop/server/main.go index df6c0ab04ac..2674fd5bdc6 100644 --- a/interop/server/main.go +++ b/interop/server/main.go @@ -94,10 +94,8 @@ func runHTTP09Server(quicConf *quic.Config) error { func runHTTP3Server(quicConf *quic.Config) error { server := http3.Server{ - Server: &http.Server{ - Addr: ":443", - TLSConfig: tlsConf, - }, + Addr: ":443", + TLSConfig: tlsConf, QuicConfig: quicConf, } http.DefaultServeMux.Handle("/", http.FileServer(http.Dir("/www")))