-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Add proxyprotocol directive and listener middleware plugin type #1349
Changes from 5 commits
fa9f002
eaf987c
a77f339
9107466
e93ed95
a3918ce
2042d1a
10c6de5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,6 +2,7 @@ | |
package httpserver | ||
|
||
import ( | ||
"context" | ||
"crypto/tls" | ||
"fmt" | ||
"io" | ||
|
@@ -27,9 +28,8 @@ type Server struct { | |
listener net.Listener | ||
listenerMu sync.Mutex | ||
sites []*SiteConfig | ||
connTimeout time.Duration // max time to wait for a connection before force stop | ||
connWg sync.WaitGroup // one increment per connection | ||
tlsGovChan chan struct{} // close to stop the TLS maintenance goroutine | ||
connTimeout time.Duration // max time to wait for a connection before force stop | ||
tlsGovChan chan struct{} // close to stop the TLS maintenance goroutine | ||
vhosts *vhostTrie | ||
} | ||
|
||
|
@@ -46,16 +46,6 @@ func NewServer(addr string, group []*SiteConfig) (*Server, error) { | |
connTimeout: GracefulTimeout, | ||
} | ||
s.Server.Handler = s // this is weird, but whatever | ||
s.Server.ConnState = func(c net.Conn, cs http.ConnState) { | ||
if cs == http.StateIdle { | ||
s.listenerMu.Lock() | ||
// server stopped, close idle connection | ||
if s.listener == nil { | ||
c.Close() | ||
} | ||
s.listenerMu.Unlock() | ||
} | ||
} | ||
|
||
// Disable HTTP/2 if desired | ||
if !HTTP2 { | ||
|
@@ -68,14 +58,6 @@ func NewServer(addr string, group []*SiteConfig) (*Server, error) { | |
s.Server.Handler = s.wrapWithSvcHeaders(s.Server.Handler) | ||
} | ||
|
||
// We have to bound our wg with one increment | ||
// to prevent a "race condition" that is hard-coded | ||
// into sync.WaitGroup.Wait() - basically, an add | ||
// with a positive delta must be guaranteed to | ||
// occur before Wait() is called on the wg. | ||
// In a way, this kind of acts as a safety barrier. | ||
s.connWg.Add(1) | ||
|
||
// Set up TLS configuration | ||
var tlsConfigs []*caddytls.Config | ||
for _, site := range group { | ||
|
@@ -140,9 +122,20 @@ func (s *Server) Listen() (net.Listener, error) { | |
} | ||
} | ||
|
||
if tcpLn, ok := ln.(*net.TCPListener); ok { | ||
ln = tcpKeepAliveListener{TCPListener: tcpLn} | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @mastercactapus This is already done in the call to Serve() on (now-)line 249. Is there a reason we do this twice? I do not think this is desirable... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @mastercactapus Ready to merge other than this. :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Pulled it out of Serve, since a TCPListener is not necessarily returned from Listen anymore. |
||
|
||
cln := ln.(caddy.Listener) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this type assertion really needed? A caddy.Listener is also a net.Listener, and there's a type assertion at the return statement. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's needed to cast the net.Listener into a caddy.Listener for use in the middleware immediately loop below it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh yeah, duh, because the middleware wrapper takes in a caddy.Listener too. 👍 |
||
for _, site := range s.sites { | ||
for _, m := range site.listenerMiddleware { | ||
cln = m(cln) | ||
} | ||
} | ||
|
||
// Very important to return a concrete caddy.Listener | ||
// implementation for graceful restarts. | ||
return ln.(*net.TCPListener), nil | ||
return cln.(caddy.Listener), nil | ||
} | ||
|
||
// ListenPacket creates udp connection for QUIC if it is enabled, | ||
|
@@ -163,8 +156,6 @@ func (s *Server) Serve(ln net.Listener) error { | |
ln = tcpKeepAliveListener{TCPListener: tcpLn} | ||
} | ||
|
||
ln = newGracefulListener(ln, &s.connWg) | ||
|
||
s.listenerMu.Lock() | ||
s.listener = ln | ||
s.listenerMu.Unlock() | ||
|
@@ -306,40 +297,21 @@ func (s *Server) Address() string { | |
|
||
// Stop stops s gracefully (or forcefully after timeout) and | ||
// closes its listener. | ||
func (s *Server) Stop() (err error) { | ||
s.Server.SetKeepAlivesEnabled(false) | ||
|
||
if runtime.GOOS != "windows" { | ||
// force connections to close after timeout | ||
done := make(chan struct{}) | ||
go func() { | ||
s.connWg.Done() // decrement our initial increment used as a barrier | ||
s.connWg.Wait() | ||
close(done) | ||
}() | ||
|
||
// Wait for remaining connections to finish or | ||
// force them all to close after timeout | ||
select { | ||
case <-time.After(s.connTimeout): | ||
case <-done: | ||
} | ||
} | ||
func (s *Server) Stop() error { | ||
ctx, cancel := context.WithTimeout(context.Background(), s.connTimeout) | ||
defer cancel() | ||
|
||
// Close the listener now; this stops the server without delay | ||
s.listenerMu.Lock() | ||
if s.listener != nil { | ||
err = s.listener.Close() | ||
s.listener = nil | ||
err := s.Server.Shutdown(ctx) | ||
if err != nil { | ||
return err | ||
} | ||
s.listenerMu.Unlock() | ||
|
||
// Closing this signals any TLS governor goroutines to exit | ||
// signal any TLS governor goroutines to exit | ||
if s.tlsGovChan != nil { | ||
close(s.tlsGovChan) | ||
} | ||
|
||
return | ||
return nil | ||
} | ||
|
||
// sanitizePath collapses any ./ ../ /// madness | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't remember, did we already talk about whether this should be defined and implemented here in the httpserver package or whether it should be in the
caddy
package directly? Would it make sense to register listener wrappers/middlewares for any/all server types or just a specific one (like HTTP) at a time?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. I don't recall. I could see a case either way. Anything using TCP could make use of the proxyprotocol middleware, I haven't dug in to see how/where to implement it.
Are you thinking the middleware stack would live on caddy, and server types (like caddyhttp) would opt-in and use them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes to the first part, and no to the second part because it's unnecessary (except for having the directive in its list of recognized directive). Remember, the HTTP server is given a Listener as input when it's time to Serve. So I think we might be able to move the listener middleware plugin functionality to the
caddy
package, because it can wrap the listeners there and then just pass the resulting Listener into Serve().Right?
I mean, we could try it. I don't see why we would have to limit this to just the HTTP server.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At serve time it does, but httpserver provides the initial Listen:
https://github.com/mastercactapus/caddy/blob/9107466685fafa24fc87bb5c7cd99e5da7f0dfc7/caddyhttp/httpserver/server.go#L104
I think that's because the server type could want UDP, or TCP, etc.. DNS vs HTTP for example. Breaking that apart could yield some interesting use cases though. Like getting a net.Conn some other way, like Chrome's socket API. gopherjs is still the only way I know of to run a full http stack client-side in the browser. Believe it or not, I've done that in the past to solve a problem. Though, not sure if we should target Caddy to that :)
Anyway, if we do move it, I think we'd also have to be careful not to break APIs. Maybe httpserver Server.Listen internally calls caddy.GetListener(net,addr) or something instead of net.Listen. It could give us the possibility of moving the graceful wrapping stuff to caddy and out of httpserver.
All in all, a few things to consider.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I follow needing something caddy.GetListener() -- if we move this listener plugin code to the caddy package, we just wrap whatever listener we put into Serve(). If the server type supports graceful restarts, it needs to return a caddy.Listener (as you noticed in your change here) but that's its own responsibility.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mastercactapus Have you had a chance to look into moving it into the caddy package? What do you think? (Again, not sure we need a GetListener function...)