summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorMatthew Holt <mholt@users.noreply.github.com>2019-07-09 12:58:39 -0600
committerMatthew Holt <mholt@users.noreply.github.com>2019-07-09 12:58:39 -0600
commit4a3a418156e25aae17659142a4bf9259d7702c44 (patch)
tree559461c6504b7c1be38e0ddfbf0793b473ef8d66
parent6dfba5fda82e216cffb117a62fcefbe61cd25a34 (diff)
Flatten HTTP handler config (#2662)
Differentiating middleware and responders has one benefit, namely that it's clear which module provides the response, but even then it's not a great advantage. Linear handler config makes a little more sense, giving greater flexibility and simplifying the core a bit, even though it's slightly awkward that handlers which are responders may not use the 'next' handler that is passed in at all.
-rw-r--r--modules/caddyhttp/caddyhttp.go60
-rw-r--r--modules/caddyhttp/encode/encode.go2
-rw-r--r--modules/caddyhttp/fileserver/staticfiles.go12
-rw-r--r--modules/caddyhttp/rewrite/rewrite.go2
-rw-r--r--modules/caddyhttp/routes.go130
-rw-r--r--modules/caddyhttp/server.go10
-rw-r--r--modules/caddyhttp/staticresp.go24
-rw-r--r--modules/caddyhttp/staticresp_test.go5
-rw-r--r--modules/caddyhttp/templates/templates.go2
9 files changed, 99 insertions, 148 deletions
diff --git a/modules/caddyhttp/caddyhttp.go b/modules/caddyhttp/caddyhttp.go
index d0c7540..330e135 100644
--- a/modules/caddyhttp/caddyhttp.go
+++ b/modules/caddyhttp/caddyhttp.go
@@ -322,13 +322,15 @@ func (app *App) automaticHTTPS() error {
MatchHost(domains),
},
},
- responder: Static{
- StatusCode: http.StatusTemporaryRedirect, // TODO: use permanent redirect instead
- Headers: http.Header{
- "Location": []string{redirTo},
- "Connection": []string{"close"},
+ handlers: []MiddlewareHandler{
+ Static{
+ StatusCode: strconv.Itoa(http.StatusTemporaryRedirect), // TODO: use permanent redirect instead
+ Headers: http.Header{
+ "Location": []string{redirTo},
+ "Connection": []string{"close"},
+ },
+ Close: true,
},
- Close: true,
},
})
}
@@ -381,34 +383,18 @@ func (app *App) listenerTaken(network, address string) bool {
var defaultALPN = []string{"h2", "http/1.1"}
// RequestMatcher is a type that can match to a request.
-// A route matcher MUST NOT modify the request.
+// A route matcher MUST NOT modify the request, with the
+// only exception being its context.
type RequestMatcher interface {
Match(*http.Request) bool
}
-// Middleware chains one Handler to the next by being passed
-// the next Handler in the chain.
-type Middleware func(HandlerFunc) HandlerFunc
-
-// MiddlewareHandler is a Handler that includes a reference
-// to the next middleware handler in the chain. Middleware
-// handlers MUST NOT call Write() or WriteHeader() on the
-// response writer; doing so will panic. See Handler godoc
-// for more information.
-type MiddlewareHandler interface {
- ServeHTTP(http.ResponseWriter, *http.Request, Handler) error
-}
-
// Handler is like http.Handler except ServeHTTP may return an error.
//
-// Middleware and responder handlers both implement this method.
-// Middleware must not call Write or WriteHeader on the ResponseWriter;
-// doing so will cause a panic. Responders should write to the response
-// if there was not an error.
-//
// If any handler encounters an error, it should be returned for proper
// handling. Return values should be propagated down the middleware chain
-// by returning it unchanged. Returned errors should not be re-wrapped.
+// by returning it unchanged. Returned errors should not be re-wrapped
+// if they are already HandlerError values.
type Handler interface {
ServeHTTP(http.ResponseWriter, *http.Request) error
}
@@ -421,9 +407,25 @@ func (f HandlerFunc) ServeHTTP(w http.ResponseWriter, r *http.Request) error {
return f(w, r)
}
-// emptyHandler is used as a no-op handler, which is
-// sometimes better than a nil Handler pointer.
-var emptyHandler HandlerFunc = func(w http.ResponseWriter, r *http.Request) error { return nil }
+// Middleware chains one Handler to the next by being passed
+// the next Handler in the chain.
+type Middleware func(HandlerFunc) HandlerFunc
+
+// MiddlewareHandler is like Handler except it takes as a third
+// argument the next handler in the chain. The next handler will
+// never be nil, but may be a no-op handler if this is the last
+// handler in the chain. Handlers which act as middleware should
+// call the next handler's ServeHTTP method so as to propagate
+// the request down the chain properly. Handlers which act as
+// responders (content origins) need not invoke the next handler,
+// since the last handler in the chain should be the first to
+// write the response.
+type MiddlewareHandler interface {
+ ServeHTTP(http.ResponseWriter, *http.Request, Handler) error
+}
+
+// emptyHandler is used as a no-op handler.
+var emptyHandler HandlerFunc = func(http.ResponseWriter, *http.Request) error { return nil }
const (
// DefaultHTTPPort is the default port for HTTP.
diff --git a/modules/caddyhttp/encode/encode.go b/modules/caddyhttp/encode/encode.go
index 2b45917..c78ccb9 100644
--- a/modules/caddyhttp/encode/encode.go
+++ b/modules/caddyhttp/encode/encode.go
@@ -36,7 +36,7 @@ import (
func init() {
caddy.RegisterModule(caddy.Module{
- Name: "http.middleware.encode",
+ Name: "http.handlers.encode",
New: func() interface{} { return new(Encode) },
})
}
diff --git a/modules/caddyhttp/fileserver/staticfiles.go b/modules/caddyhttp/fileserver/staticfiles.go
index 0bb910d..a7c72c9 100644
--- a/modules/caddyhttp/fileserver/staticfiles.go
+++ b/modules/caddyhttp/fileserver/staticfiles.go
@@ -35,7 +35,7 @@ func init() {
weakrand.Seed(time.Now().UnixNano())
caddy.RegisterModule(caddy.Module{
- Name: "http.responders.file_server",
+ Name: "http.handlers.file_server",
New: func() interface{} { return new(FileServer) },
})
}
@@ -108,7 +108,7 @@ func (fsrv *FileServer) Validate() error {
return nil
}
-func (fsrv *FileServer) ServeHTTP(w http.ResponseWriter, r *http.Request) error {
+func (fsrv *FileServer) ServeHTTP(w http.ResponseWriter, r *http.Request, _ caddyhttp.Handler) error {
repl := r.Context().Value(caddy.ReplacerCtxKey).(caddy.Replacer)
filesToHide := fsrv.transformHidePaths(repl)
@@ -119,7 +119,7 @@ func (fsrv *FileServer) ServeHTTP(w http.ResponseWriter, r *http.Request) error
if filename == "" {
// no files worked, so resort to fallback
if fsrv.Fallback != nil {
- fallback, w := fsrv.Fallback.BuildCompositeRoute(w, r)
+ fallback := fsrv.Fallback.BuildCompositeRoute(w, r)
return fallback.ServeHTTP(w, r)
}
return caddyhttp.Error(http.StatusNotFound, nil)
@@ -452,7 +452,7 @@ const minBackoff, maxBackoff = 2, 5
// Interface guards
var (
- _ caddy.Provisioner = (*FileServer)(nil)
- _ caddy.Validator = (*FileServer)(nil)
- _ caddyhttp.Handler = (*FileServer)(nil)
+ _ caddy.Provisioner = (*FileServer)(nil)
+ _ caddy.Validator = (*FileServer)(nil)
+ _ caddyhttp.MiddlewareHandler = (*FileServer)(nil)
)
diff --git a/modules/caddyhttp/rewrite/rewrite.go b/modules/caddyhttp/rewrite/rewrite.go
index 9060e22..ac113ff 100644
--- a/modules/caddyhttp/rewrite/rewrite.go
+++ b/modules/caddyhttp/rewrite/rewrite.go
@@ -25,7 +25,7 @@ import (
func init() {
caddy.RegisterModule(caddy.Module{
- Name: "http.middleware.rewrite",
+ Name: "http.handlers.rewrite",
New: func() interface{} { return new(Rewrite) },
})
}
diff --git a/modules/caddyhttp/routes.go b/modules/caddyhttp/routes.go
index 5e61fae..e010bb6 100644
--- a/modules/caddyhttp/routes.go
+++ b/modules/caddyhttp/routes.go
@@ -28,25 +28,19 @@ import (
type ServerRoute struct {
Group string `json:"group,omitempty"`
MatcherSets []map[string]json.RawMessage `json:"match,omitempty"`
- Apply []json.RawMessage `json:"apply,omitempty"`
- Respond json.RawMessage `json:"respond,omitempty"`
-
- Terminal bool `json:"terminal,omitempty"`
+ Handle []json.RawMessage `json:"handle,omitempty"`
+ Terminal bool `json:"terminal,omitempty"`
// decoded values
matcherSets []MatcherSet
- middleware []MiddlewareHandler
- responder Handler
+ handlers []MiddlewareHandler
}
// Empty returns true if the route has all zero/default values.
func (sr ServerRoute) Empty() bool {
return len(sr.MatcherSets) == 0 &&
- len(sr.Apply) == 0 &&
- len(sr.Respond) == 0 &&
- len(sr.matcherSets) == 0 &&
- len(sr.middleware) == 0 &&
- sr.responder == nil &&
+ len(sr.Handle) == 0 &&
+ len(sr.handlers) == 0 &&
!sr.Terminal &&
sr.Group == ""
}
@@ -98,40 +92,27 @@ func (routes RouteList) Provision(ctx caddy.Context) error {
}
routes[i].MatcherSets = nil // allow GC to deallocate - TODO: Does this help?
- // middleware
- for j, rawMsg := range route.Apply {
- mid, err := ctx.LoadModuleInline("middleware", "http.middleware", rawMsg)
- if err != nil {
- return fmt.Errorf("loading middleware module in position %d: %v", j, err)
- }
- routes[i].middleware = append(routes[i].middleware, mid.(MiddlewareHandler))
- }
- routes[i].Apply = nil // allow GC to deallocate - TODO: Does this help?
-
- // responder
- if route.Respond != nil {
- resp, err := ctx.LoadModuleInline("responder", "http.responders", route.Respond)
+ // handlers
+ for j, rawMsg := range route.Handle {
+ mh, err := ctx.LoadModuleInline("handler", "http.handlers", rawMsg)
if err != nil {
- return fmt.Errorf("loading responder module: %v", err)
+ return fmt.Errorf("loading handler module in position %d: %v", j, err)
}
- routes[i].responder = resp.(Handler)
+ routes[i].handlers = append(routes[i].handlers, mh.(MiddlewareHandler))
}
- routes[i].Respond = nil // allow GC to deallocate - TODO: Does this help?
+ routes[i].Handle = nil // allow GC to deallocate - TODO: Does this help?
}
return nil
}
-// BuildCompositeRoute creates a chain of handlers by applying all the matching
-// routes. The returned ResponseWriter should be used instead of rw.
-func (routes RouteList) BuildCompositeRoute(rw http.ResponseWriter, req *http.Request) (Handler, http.ResponseWriter) {
- mrw := &middlewareResponseWriter{ResponseWriterWrapper: &ResponseWriterWrapper{rw}}
-
+// BuildCompositeRoute creates a chain of handlers by
+// applying all of the matching routes.
+func (routes RouteList) BuildCompositeRoute(rw http.ResponseWriter, req *http.Request) Handler {
if len(routes) == 0 {
- return emptyHandler, mrw
+ return emptyHandler
}
var mid []Middleware
- var responder Handler
groups := make(map[string]struct{})
for _, route := range routes {
@@ -140,9 +121,8 @@ func (routes RouteList) BuildCompositeRoute(rw http.ResponseWriter, req *http.Re
continue
}
- // if route is part of a group, ensure only
- // the first matching route in the group is
- // applied
+ // if route is part of a group, ensure only the
+ // first matching route in the group is applied
if route.Group != "" {
_, ok := groups[route.Group]
if ok {
@@ -155,78 +135,48 @@ func (routes RouteList) BuildCompositeRoute(rw http.ResponseWriter, req *http.Re
}
// apply the rest of the route
- for _, m := range route.middleware {
- // we have to be sure to wrap m outside
- // of our current scope so that the
- // reference to this m isn't overwritten
- // on the next iteration, leaving only
- // the last middleware in the chain as
- // the ONLY middleware in the chain!
- mid = append(mid, wrapMiddleware(m))
- }
- if responder == nil {
- responder = route.responder
+ for _, mh := range route.handlers {
+ // we have to be sure to wrap mh outside
+ // of our current stack frame so that the
+ // reference to this mh isn't overwritten
+ // on the next iteration, leaving the last
+ // middleware in the chain as the ONLY
+ // middleware in the chain!
+ mid = append(mid, wrapMiddleware(mh))
}
+
+ // if this route is supposed to be last, don't
+ // compile any more into the chain
if route.Terminal {
break
}
}
- // build the middleware stack, with the responder at the end
- stack := HandlerFunc(func(w http.ResponseWriter, r *http.Request) error {
- if responder == nil {
- return nil
- }
- mrw.allowWrites = true
- return responder.ServeHTTP(w, r)
- })
+ // build the middleware chain, with the responder at the end
+ stack := emptyHandler
for i := len(mid) - 1; i >= 0; i-- {
stack = mid[i](stack)
}
- return stack, mrw
+ return stack
}
// wrapMiddleware wraps m such that it can be correctly
-// appended to a list of middleware. This separate closure
-// is necessary so that only the last middleware in a loop
-// does not become the only middleware of the stack,
-// repeatedly executed (i.e. it is necessary to keep a
-// reference to this m outside of the scope of a loop)!
-func wrapMiddleware(m MiddlewareHandler) Middleware {
+// appended to a list of middleware. We can't do this
+// directly in a loop because it relies on a reference
+// to mh not changing until the execution of its handler,
+// which is deferred by multiple func closures. In other
+// words, we need to pull this particular MiddlewareHandler
+// pointer into its own stack frame to preserve it so it
+// won't be overwritten in future loop iterations.
+func wrapMiddleware(mh MiddlewareHandler) Middleware {
return func(next HandlerFunc) HandlerFunc {
return func(w http.ResponseWriter, r *http.Request) error {
// TODO: This is where request tracing could be implemented; also
// see below to trace the responder as well
// TODO: Trace a diff of the request, would be cool too! see what changed since the last middleware (host, headers, URI...)
// TODO: see what the std lib gives us in terms of stack tracing too
- return m.ServeHTTP(w, r, next)
+ return mh.ServeHTTP(w, r, next)
}
}
}
-
-type middlewareResponseWriter struct {
- *ResponseWriterWrapper
- allowWrites bool
-}
-
-func (mrw middlewareResponseWriter) WriteHeader(statusCode int) {
- if !mrw.allowWrites {
- // technically, this is not true: middleware can write headers,
- // but only after the responder handler has returned; either the
- // responder did nothing with the response (sad face), or the
- // middleware wrapped the response and deferred the write
- panic("WriteHeader: middleware cannot write response headers")
- }
- mrw.ResponseWriterWrapper.WriteHeader(statusCode)
-}
-
-func (mrw middlewareResponseWriter) Write(b []byte) (int, error) {
- if !mrw.allowWrites {
- panic("Write: middleware cannot write to the response before responder")
- }
- return mrw.ResponseWriterWrapper.Write(b)
-}
-
-// Interface guard
-var _ HTTPInterfaces = (*middlewareResponseWriter)(nil)
diff --git a/modules/caddyhttp/server.go b/modules/caddyhttp/server.go
index d40a01d..8bc3a5a 100644
--- a/modules/caddyhttp/server.go
+++ b/modules/caddyhttp/server.go
@@ -65,9 +65,9 @@ func (s *Server) ServeHTTP(w http.ResponseWriter, r *http.Request) {
addHTTPVarsToReplacer(repl, r, w)
// build and execute the main handler chain
- stack, wrappedWriter := s.Routes.BuildCompositeRoute(w, r)
+ stack := s.Routes.BuildCompositeRoute(w, r)
stack = s.wrapPrimaryRoute(stack)
- err := s.executeCompositeRoute(wrappedWriter, r, stack)
+ err := s.executeCompositeRoute(w, r, stack)
if err != nil {
// add the raw error value to the request context
// so it can be accessed by error handlers
@@ -85,8 +85,8 @@ func (s *Server) ServeHTTP(w http.ResponseWriter, r *http.Request) {
}
if s.Errors != nil && len(s.Errors.Routes) > 0 {
- errStack, wrappedWriter := s.Errors.Routes.BuildCompositeRoute(w, r)
- err := s.executeCompositeRoute(wrappedWriter, r, errStack)
+ errStack := s.Errors.Routes.BuildCompositeRoute(w, r)
+ err := s.executeCompositeRoute(w, r, errStack)
if err != nil {
// TODO: what should we do if the error handler has an error?
log.Printf("[ERROR] [%s %s] handling error: %v", r.Method, r.RequestURI, err)
@@ -154,6 +154,8 @@ func (s *Server) enforcementHandler(w http.ResponseWriter, r *http.Request, next
return next.ServeHTTP(w, r)
}
+// listenersUseAnyPortOtherThan returns true if there are any
+// listeners in s that use a port which is not otherPort.
func (s *Server) listenersUseAnyPortOtherThan(otherPort int) bool {
for _, lnAddr := range s.Listen {
_, addrs, err := caddy.ParseListenAddr(lnAddr)
diff --git a/modules/caddyhttp/staticresp.go b/modules/caddyhttp/staticresp.go
index 3f43c92..8e4a3df 100644
--- a/modules/caddyhttp/staticresp.go
+++ b/modules/caddyhttp/staticresp.go
@@ -24,21 +24,20 @@ import (
func init() {
caddy.RegisterModule(caddy.Module{
- Name: "http.responders.static",
+ Name: "http.handlers.static",
New: func() interface{} { return new(Static) },
})
}
// Static implements a simple responder for static responses.
type Static struct {
- StatusCode int `json:"status_code"` // TODO: should we turn this into a string so that only one field is needed? (string allows replacements)
- StatusCodeStr string `json:"status_code_str"`
- Headers http.Header `json:"headers"`
- Body string `json:"body"`
- Close bool `json:"close"`
+ StatusCode string `json:"status_code"`
+ Headers http.Header `json:"headers"`
+ Body string `json:"body"`
+ Close bool `json:"close"`
}
-func (s Static) ServeHTTP(w http.ResponseWriter, r *http.Request) error {
+func (s Static) ServeHTTP(w http.ResponseWriter, r *http.Request, _ Handler) error {
repl := r.Context().Value(caddy.ReplacerCtxKey).(caddy.Replacer)
// close the connection after responding
@@ -60,16 +59,13 @@ func (s Static) ServeHTTP(w http.ResponseWriter, r *http.Request) error {
}
// get the status code
- statusCode := s.StatusCode
- if statusCode == 0 && s.StatusCodeStr != "" {
- intVal, err := strconv.Atoi(repl.ReplaceAll(s.StatusCodeStr, ""))
+ statusCode := http.StatusOK
+ if s.StatusCode != "" {
+ intVal, err := strconv.Atoi(repl.ReplaceAll(s.StatusCode, ""))
if err == nil {
statusCode = intVal
}
}
- if statusCode == 0 {
- statusCode = http.StatusOK
- }
// write headers
w.WriteHeader(statusCode)
@@ -83,4 +79,4 @@ func (s Static) ServeHTTP(w http.ResponseWriter, r *http.Request) error {
}
// Interface guard
-var _ Handler = (*Static)(nil)
+var _ MiddlewareHandler = (*Static)(nil)
diff --git a/modules/caddyhttp/staticresp_test.go b/modules/caddyhttp/staticresp_test.go
index b518bb0..111e4f3 100644
--- a/modules/caddyhttp/staticresp_test.go
+++ b/modules/caddyhttp/staticresp_test.go
@@ -19,6 +19,7 @@ import (
"io/ioutil"
"net/http"
"net/http/httptest"
+ "strconv"
"testing"
"github.com/caddyserver/caddy/v2"
@@ -29,7 +30,7 @@ func TestStaticResponseHandler(t *testing.T) {
w := httptest.NewRecorder()
s := Static{
- StatusCode: http.StatusNotFound,
+ StatusCode: strconv.Itoa(http.StatusNotFound),
Headers: http.Header{
"X-Test": []string{"Testing"},
},
@@ -37,7 +38,7 @@ func TestStaticResponseHandler(t *testing.T) {
Close: true,
}
- err := s.ServeHTTP(w, r)
+ err := s.ServeHTTP(w, r, nil)
if err != nil {
t.Errorf("did not expect an error, but got: %v", err)
}
diff --git a/modules/caddyhttp/templates/templates.go b/modules/caddyhttp/templates/templates.go
index e5bfa35..85b0bc2 100644
--- a/modules/caddyhttp/templates/templates.go
+++ b/modules/caddyhttp/templates/templates.go
@@ -28,7 +28,7 @@ import (
func init() {
caddy.RegisterModule(caddy.Module{
- Name: "http.middleware.templates",
+ Name: "http.handlers.templates",
New: func() interface{} { return new(Templates) },
})
}