diff --git a/dist/CHANGES.txt b/dist/CHANGES.txt index 299c5b91a..a140ca257 100644 --- a/dist/CHANGES.txt +++ b/dist/CHANGES.txt @@ -4,6 +4,7 @@ CHANGES - New pprof directive for exposing process performance profile - Toggle case-sensitive path matching with environment variable - proxy: New max_conns setting to limit max connections per upstream +- proxy: Enables replaceable value for name of upstream host - Internal improvements, restructuring, and bug fixes 0.8.2 (February 25, 2016) diff --git a/middleware/log/log.go b/middleware/log/log.go index feb6182ad..4e0c2e29b 100644 --- a/middleware/log/log.go +++ b/middleware/log/log.go @@ -1,4 +1,4 @@ -// Package log implements basic but useful request (access) logging middleware. +// Package log implements request (access) logging middleware. package log import ( @@ -19,8 +19,17 @@ type Logger struct { func (l Logger) ServeHTTP(w http.ResponseWriter, r *http.Request) (int, error) { for _, rule := range l.Rules { if middleware.Path(r.URL.Path).Matches(rule.PathScope) { + // Record the response responseRecorder := middleware.NewResponseRecorder(w) + + // Attach the Replacer we'll use so that other middlewares can + // set their own placeholders if they want to. + rep := middleware.NewReplacer(r, responseRecorder, CommonLogEmptyValue) + responseRecorder.Replacer = rep + + // Bon voyage, request! status, err := l.Next.ServeHTTP(responseRecorder, r) + if status >= 400 { // There was an error up the chain, but no response has been written yet. // The error must be handled here so the log entry will record the response size. @@ -33,8 +42,10 @@ func (l Logger) ServeHTTP(w http.ResponseWriter, r *http.Request) (int, error) { } status = 0 } - rep := middleware.NewReplacer(r, responseRecorder, CommonLogEmptyValue) + + // Write log entry rule.Log.Println(rep.Replace(rule.Format)) + return status, err } } diff --git a/middleware/log/log_test.go b/middleware/log/log_test.go index 40560e4c0..0ce12b0ca 100644 --- a/middleware/log/log_test.go +++ b/middleware/log/log_test.go @@ -7,11 +7,16 @@ import ( "net/http/httptest" "strings" "testing" + + "github.com/mholt/caddy/middleware" ) type erroringMiddleware struct{} func (erroringMiddleware) ServeHTTP(w http.ResponseWriter, r *http.Request) (int, error) { + if rr, ok := w.(*middleware.ResponseRecorder); ok { + rr.Replacer.Set("testval", "foobar") + } return http.StatusNotFound, nil } @@ -20,7 +25,7 @@ func TestLoggedStatus(t *testing.T) { var next erroringMiddleware rule := Rule{ PathScope: "/", - Format: DefaultLogFormat, + Format: DefaultLogFormat + " {testval}", Log: log.New(&f, "", 0), } @@ -38,11 +43,20 @@ func TestLoggedStatus(t *testing.T) { status, err := logger.ServeHTTP(rec, r) if status != 0 { - t.Error("Expected status to be 0 - was", status) + t.Errorf("Expected status to be 0, but was %d", status) + } + + if err != nil { + t.Errorf("Expected error to be nil, instead got: %v", err) } logged := f.String() if !strings.Contains(logged, "404 13") { - t.Error("Expected 404 to be logged. Logged string -", logged) + t.Errorf("Expected log entry to contain '404 13', but it didn't: %s", logged) + } + + // check custom placeholder + if !strings.Contains(logged, "foobar") { + t.Errorf("Expected the log entry to contain 'foobar' (custom placeholder), but it didn't: %s", logged) } } diff --git a/middleware/proxy/proxy.go b/middleware/proxy/proxy.go index ed466bae1..ada456da3 100644 --- a/middleware/proxy/proxy.go +++ b/middleware/proxy/proxy.go @@ -89,6 +89,9 @@ func (p Proxy) ServeHTTP(w http.ResponseWriter, r *http.Request) (int, error) { } proxy := host.ReverseProxy r.Host = host.Name + if rr, ok := w.(*middleware.ResponseRecorder); ok && rr.Replacer != nil { + rr.Replacer.Set("upstream", host.Name) + } if baseURL, err := url.Parse(host.Name); err == nil { r.Host = baseURL.Host diff --git a/middleware/proxy/proxy_test.go b/middleware/proxy/proxy_test.go index 6bd17eae3..621582a66 100644 --- a/middleware/proxy/proxy_test.go +++ b/middleware/proxy/proxy_test.go @@ -18,6 +18,8 @@ import ( "testing" "time" + "github.com/mholt/caddy/middleware" + "golang.org/x/net/websocket" ) @@ -53,6 +55,16 @@ func TestReverseProxy(t *testing.T) { if !requestReceived { t.Error("Expected backend to receive request, but it didn't") } + + // Make sure {upstream} placeholder is set + rr := middleware.NewResponseRecorder(httptest.NewRecorder()) + rr.Replacer = middleware.NewReplacer(r, rr, "-") + + p.ServeHTTP(rr, r) + + if got, want := rr.Replacer.Replace("{upstream}"), backend.URL; got != want { + t.Errorf("Expected custom placeholder {upstream} to be set (%s), but it wasn't; got: %s", want, got) + } } func TestReverseProxyInsecureSkipVerify(t *testing.T) { diff --git a/middleware/recorder.go b/middleware/recorder.go index a94db927b..585c5c335 100644 --- a/middleware/recorder.go +++ b/middleware/recorder.go @@ -8,17 +8,24 @@ import ( "time" ) -// ResponseRecorder is a type of ResponseWriter that captures +// ResponseRecorder is a type of http.ResponseWriter that captures // the status code written to it and also the size of the body // written in the response. A status code does not have // to be written, however, in which case 200 must be assumed. // It is best to have the constructor initialize this type // with that default status code. +// +// Setting the Replacer field allows middlewares to type-assert +// the http.ResponseWriter to ResponseRecorder and set their own +// placeholder values for logging utilities to use. +// +// Beware when accessing the Replacer value; it may be nil! type ResponseRecorder struct { http.ResponseWriter - status int - size int - start time.Time + Replacer Replacer + status int + size int + start time.Time } // NewResponseRecorder makes and returns a new responseRecorder, diff --git a/middleware/replacer.go b/middleware/replacer.go index d53fdc660..85659ca0e 100644 --- a/middleware/replacer.go +++ b/middleware/replacer.go @@ -12,26 +12,38 @@ import ( // Replacer is a type which can replace placeholder // substrings in a string with actual values from a -// http.Request and responseRecorder. Always use -// NewReplacer to get one of these. +// http.Request and ResponseRecorder. Always use +// NewReplacer to get one of these. Any placeholders +// made with Set() should overwrite existing values if +// the key is already used. type Replacer interface { Replace(string) string Set(key, value string) } +// replacer implements Replacer. customReplacements +// is used to store custom replacements created with +// Set() until the time of replacement, at which point +// they will be used to overwrite other replacements +// if there is a name conflict. type replacer struct { - replacements map[string]string - emptyValue string + replacements map[string]string + customReplacements map[string]string + emptyValue string + responseRecorder *ResponseRecorder } -// NewReplacer makes a new replacer based on r and rr. -// Do not create a new replacer until r and rr have all -// the needed values, because this function copies those -// values into the replacer. rr may be nil if it is not -// available. emptyValue should be the string that is used -// in place of empty string (can still be empty string). +// NewReplacer makes a new replacer based on r and rr which +// are used for request and response placeholders, respectively. +// Request placeholders are created immediately, whereas +// response placeholders are not created until Replace() +// is invoked. rr may be nil if it is not available. +// emptyValue should be the string that is used in place +// of empty string (can still be empty string). func NewReplacer(r *http.Request, rr *ResponseRecorder, emptyValue string) Replacer { - rep := replacer{ + rep := &replacer{ + responseRecorder: rr, + customReplacements: make(map[string]string), replacements: map[string]string{ "{method}": r.Method, "{scheme}": func() string { @@ -66,9 +78,7 @@ func NewReplacer(r *http.Request, rr *ResponseRecorder, emptyValue string) Repla }(), "{uri}": r.URL.RequestURI(), "{uri_escaped}": url.QueryEscape(r.URL.RequestURI()), - "{when}": func() string { - return time.Now().Format(timeFormat) - }(), + "{when}": time.Now().Format(timeFormat), "{file}": func() string { _, file := path.Split(r.URL.Path) return file @@ -80,11 +90,6 @@ func NewReplacer(r *http.Request, rr *ResponseRecorder, emptyValue string) Repla }, emptyValue: emptyValue, } - if rr != nil { - rep.replacements["{status}"] = strconv.Itoa(rr.status) - rep.replacements["{size}"] = strconv.Itoa(rr.size) - rep.replacements["{latency}"] = time.Since(rr.start).String() - } // Header placeholders (case-insensitive) for header, values := range r.Header { @@ -96,7 +101,19 @@ func NewReplacer(r *http.Request, rr *ResponseRecorder, emptyValue string) Repla // Replace performs a replacement of values on s and returns // the string with the replaced values. -func (r replacer) Replace(s string) string { +func (r *replacer) Replace(s string) string { + // Make response placeholders now + if r.responseRecorder != nil { + r.replacements["{status}"] = strconv.Itoa(r.responseRecorder.status) + r.replacements["{size}"] = strconv.Itoa(r.responseRecorder.size) + r.replacements["{latency}"] = time.Since(r.responseRecorder.start).String() + } + + // Include custom placeholders, overwriting existing ones if necessary + for key, val := range r.customReplacements { + r.replacements[key] = val + } + // Header replacements - these are case-insensitive, so we can't just use strings.Replace() for strings.Contains(s, headerReplacer) { idxStart := strings.Index(s, headerReplacer) @@ -125,9 +142,9 @@ func (r replacer) Replace(s string) string { return s } -// Set sets key to value in the replacements map. -func (r replacer) Set(key, value string) { - r.replacements["{"+key+"}"] = value +// Set sets key to value in the r.customReplacements map. +func (r *replacer) Set(key, value string) { + r.customReplacements["{"+key+"}"] = value } const ( diff --git a/middleware/replacer_test.go b/middleware/replacer_test.go index d98bd2de1..4ffdfba12 100644 --- a/middleware/replacer_test.go +++ b/middleware/replacer_test.go @@ -16,23 +16,27 @@ func TestNewReplacer(t *testing.T) { if err != nil { t.Fatal("Request Formation Failed\n") } - replaceValues := NewReplacer(request, recordRequest, "") - - switch v := replaceValues.(type) { - case replacer: + rep := NewReplacer(request, recordRequest, "") + switch v := rep.(type) { + case *replacer: if v.replacements["{host}"] != "localhost" { t.Error("Expected host to be localhost") } if v.replacements["{method}"] != "POST" { t.Error("Expected request method to be POST") } - if v.replacements["{status}"] != "200" { - t.Error("Expected status to be 200") - } + // Response placeholders should only be set after call to Replace() + if got, want := v.replacements["{status}"], ""; got != want { + t.Errorf("Expected status to NOT be set before Replace() is called; was: %s", got) + } + rep.Replace("foobar") + if got, want := v.replacements["{status}"], "200"; got != want { + t.Errorf("Expected status to be %s, was: %s", want, got) + } default: - t.Fatal("Return Value from New Replacer expected pass type assertion into a replacer type\n") + t.Fatalf("Expected *replacer underlying Replacer type, got: %#v", rep) } }