Skip to content
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

Jump to bottom

Add proxyprotocol directive and listener middleware plugin type #1349

Merged
merged 8 commits into from Mar 10, 2017
Merged

Add proxyprotocol directive and listener middleware plugin type #1349

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
fa9f002
Merge remote-tracking branch 'origin/go18shutdown' into listener-plug…
mastercactapus Feb 5, 2017
eaf987c
add support for listener middleware
mastercactapus Feb 5, 2017
a77f339
add proxyprotocol directive
mastercactapus Feb 14, 2017
9107466
make caddy.Listener interface required
mastercactapus Feb 16, 2017
e93ed95
Merge remote-tracking branch 'origin/master' into proxy_protocol
mastercactapus Mar 4, 2017
a3918ce
Merge remote-tracking branch 'origin/master' into proxy_protocol
mastercactapus Mar 4, 2017
2042d1a
Merge branch 'master' into proxy_protocol
mastercactapus Mar 10, 2017
10c6de5
Remove tcpKeepAliveListener wrapper from Serve()
mastercactapus Mar 10, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 3 additions & 2 deletions caddyhttp/httpserver/middleware.go
View file
Open in desktop
Expand Up @@ -2,11 +2,12 @@ package httpserver

import (
"fmt"
"net"
"net/http"
"os"
"path"
"time"

"github.com/mholt/caddy"
)

func init() {
Expand All @@ -21,7 +22,7 @@ type (

// ListenerMiddleware is similar to the Middleware type, except it
// chains one net.Listener to the next.
ListenerMiddleware func(net.Listener) net.Listener
ListenerMiddleware func(caddy.Listener) caddy.Listener
Copy link
Member

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?

Copy link
Author

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you thinking the middleware stack would live on caddy, and server types (like caddyhttp) would opt-in and use them?

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.

Copy link
Author

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.

Copy link
Member

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.

Copy link
Member

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


// Handler is like http.Handler except ServeHTTP may return a status
// code and/or error.
Expand Down
5 changes: 3 additions & 2 deletions caddyhttp/httpserver/server.go
View file
Open in desktop
Expand Up @@ -126,15 +126,16 @@ func (s *Server) Listen() (net.Listener, error) {
ln = tcpKeepAliveListener{TCPListener: tcpLn}
}
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mastercactapus Ready to merge other than this. :)

Copy link
Author

Choose a reason for hiding this comment

The 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)
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

The 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 {
ln = m(ln)
cln = m(cln)
}
}

// Very important to return a concrete caddy.Listener
// implementation for graceful restarts.
return ln.(caddy.Listener), nil
return cln.(caddy.Listener), nil
}

// ListenPacket creates udp connection for QUIC if it is enabled,
Expand Down