From 4aa699278be6d07a28ecf28c747f99126b0305bb Mon Sep 17 00:00:00 2001 From: toby lorne Date: Mon, 3 Aug 2020 22:05:07 +0100 Subject: [PATCH 01/19] handlers: error writer interface and impls so we can render HTML error messages in future Signed-off-by: toby lorne --- handlers/error.go | 94 ++++++++++++++++++ handlers/error_test.go | 211 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 305 insertions(+) create mode 100644 handlers/error.go create mode 100644 handlers/error_test.go diff --git a/handlers/error.go b/handlers/error.go new file mode 100644 index 000000000..0fe50fe0a --- /dev/null +++ b/handlers/error.go @@ -0,0 +1,94 @@ +package handlers + +import ( + "bytes" + "fmt" + "html/template" + "net/http" + + "code.cloudfoundry.org/gorouter/logger" + "github.com/uber-go/zap" +) + +type ErrorWriter interface { + WriteError( + rw http.ResponseWriter, + code int, + message string, + logger logger.Logger, + ) +} + +type plaintextErrorWriter struct{} + +func NewPlaintextErrorWriter() ErrorWriter { + return &plaintextErrorWriter{} +} + +// WriteStatus attempts to template an error message. +func (ew *plaintextErrorWriter) WriteError( + rw http.ResponseWriter, + code int, + message string, + logger logger.Logger, +) { + body := fmt.Sprintf("%d %s: %s", code, http.StatusText(code), message) + + if code != http.StatusNotFound { + logger.Info("status", zap.String("body", body)) + } + + if code > 299 { + rw.Header().Del("Connection") + } + + rw.WriteHeader(code) + rw.Write([]byte(body)) +} + +type htmlErrorWriter struct { + tpl *template.Template +} + +func NewHTMLErrorWriter(text string) (ErrorWriter, error) { + ew := &htmlErrorWriter{} + + tpl, err := template.New("error-message").Parse(text) + if err != nil { + return nil, err + } + ew.tpl = tpl + + return ew, nil +} + +// WriteStatus attempts to template an error message. +// If the template cannot be rendered then text will be sent instead +// and the error will be returned even though the response has been sent +func (ew *htmlErrorWriter) WriteError( + rw http.ResponseWriter, + code int, + message string, + logger logger.Logger, +) { + body := fmt.Sprintf("%d %s: %s", code, http.StatusText(code), message) + + if code != http.StatusNotFound { + logger.Info("status", zap.String("body", body)) + } + + if code > 299 { + rw.Header().Del("Connection") + } + + rw.WriteHeader(code) + + var rendered bytes.Buffer + if err := ew.tpl.Execute(&rendered, nil); err != nil { + logger.Error("render-error-failed", zap.Error(err)) + rw.Write([]byte(body)) + return + } + + rw.Write(rendered.Bytes()) +} diff --git a/handlers/error_test.go b/handlers/error_test.go new file mode 100644 index 000000000..3f959ff02 --- /dev/null +++ b/handlers/error_test.go @@ -0,0 +1,211 @@ +package handlers_test + +import ( + _ "html/template" + "net/http" + "net/http/httptest" + + . "github.com/onsi/ginkgo" + . "github.com/onsi/gomega" + . "github.com/onsi/gomega/gbytes" + + . "code.cloudfoundry.org/gorouter/handlers" + loggerfakes "code.cloudfoundry.org/gorouter/logger/fakes" +) + +var _ = Describe("Plaintext ErrorWriter", func() { + var ( + errorWriter ErrorWriter + recorder *httptest.ResponseRecorder + + log *loggerfakes.FakeLogger + ) + + BeforeEach(func() { + errorWriter = NewPlaintextErrorWriter() + recorder = httptest.NewRecorder() + recorder.Header().Set("Connection", "dummy") + + log = new(loggerfakes.FakeLogger) + }) + + Context("when the response code is a success", func() { + BeforeEach(func() { + errorWriter.WriteError(recorder, http.StatusOK, "hi", log) + }) + + It("should write the status code", func() { + Expect(recorder.Result().StatusCode).To(Equal(http.StatusOK)) + }) + + It("should write the message", func() { + Eventually(BufferReader(recorder.Result().Body)).Should(Say("hi")) + }) + + It("should log the message", func() { + Expect(log.InfoCallCount()).NotTo(Equal(0)) + message, _ := log.InfoArgsForCall(0) + Expect(message).To(Equal("status")) + }) + + It("should keep the connection header", func() { + Expect(recorder.Result().Header.Get("Connection")).To(Equal("dummy")) + }) + }) + + Context("when the response code is not a success", func() { + BeforeEach(func() { + errorWriter.WriteError(recorder, http.StatusBadRequest, "bad", log) + }) + + It("should write the status code", func() { + Expect(recorder.Result().StatusCode).To(Equal(http.StatusBadRequest)) + }) + + It("should write the message", func() { + Eventually(BufferReader(recorder.Result().Body)).Should(Say("bad")) + }) + + It("should log the message", func() { + Expect(log.InfoCallCount()).NotTo(Equal(0)) + message, _ := log.InfoArgsForCall(0) + Expect(message).To(Equal("status")) + }) + + It("should delete the connection header", func() { + Expect(recorder.Result().Header.Get("Connection")).To(Equal("")) + }) + }) +}) + +var _ = Describe("HTML ErrorWriter", func() { + var ( + errorWriter ErrorWriter + recorder *httptest.ResponseRecorder + + log *loggerfakes.FakeLogger + ) + + BeforeEach(func() { + recorder = httptest.NewRecorder() + recorder.Header().Set("Connection", "dummy") + + log = new(loggerfakes.FakeLogger) + }) + + Context("when the template has invalid syntax", func() { + It("should return constructor error", func() { + var err error + _, err = NewHTMLErrorWriter("{{") + Expect(err).To(HaveOccurred()) + }) + }) + + Context("when the template errors", func() { + Context("when the response is a success", func() { + BeforeEach(func() { + var err error + errorWriter, err = NewHTMLErrorWriter(`{{template "notexists"}}`) + Expect(err).NotTo(HaveOccurred()) + + errorWriter.WriteError(recorder, http.StatusOK, "hi", log) + }) + + It("should write the status code", func() { + Expect(recorder.Result().StatusCode).To(Equal(http.StatusOK)) + }) + + It("should write the message as text", func() { + Eventually(BufferReader(recorder.Result().Body)).Should(Say("200 OK: hi")) + }) + + It("should log the message", func() { + Expect(log.InfoCallCount()).NotTo(Equal(0)) + message, _ := log.InfoArgsForCall(0) + Expect(message).To(Equal("status")) + }) + + It("should keep the connection header", func() { + Expect(recorder.Result().Header.Get("Connection")).To(Equal("dummy")) + }) + }) + + Context("when the response is not a success", func() { + BeforeEach(func() { + var err error + errorWriter, err = NewHTMLErrorWriter(`{{template "notexists"}}`) + Expect(err).NotTo(HaveOccurred()) + + errorWriter.WriteError(recorder, http.StatusBadRequest, "bad", log) + }) + + It("should write the status code", func() { + Expect(recorder.Result().StatusCode).To(Equal(http.StatusBadRequest)) + }) + + It("should write the message as text", func() { + Eventually(BufferReader(recorder.Result().Body)).Should(Say("400 Bad Request: bad")) + }) + + It("should delete the connection header", func() { + Expect(recorder.Result().Header.Get("Connection")).To(Equal("")) + }) + }) + }) + + Context("when the template renders", func() { + var ( + err error + ) + + Context("when the response is a success", func() { + BeforeEach(func() { + errorWriter, err = NewHTMLErrorWriter(`success`) + Expect(err).NotTo(HaveOccurred()) + + errorWriter.WriteError(recorder, http.StatusOK, "hi", log) + }) + + It("should not return an error", func() { + Expect(err).NotTo(HaveOccurred()) + }) + + It("should write the status code", func() { + Expect(recorder.Result().StatusCode).To(Equal(http.StatusOK)) + }) + + XIt("should write the message as text", func() { + Eventually(BufferReader(recorder.Result().Body)).Should(Say("200 OK: hi")) + }) + + It("should keep the connection header", func() { + Expect(recorder.Result().Header.Get("Connection")).To(Equal("dummy")) + }) + }) + + Context("when the response is not a success", func() { + BeforeEach(func() { + errorWriter, err = NewHTMLErrorWriter(`failure`) + Expect(err).NotTo(HaveOccurred()) + + errorWriter.WriteError(recorder, http.StatusBadRequest, "bad", log) + }) + + It("should not return an error", func() { + Expect(err).NotTo(HaveOccurred()) + }) + + It("should write the status code", func() { + Expect(recorder.Result().StatusCode).To(Equal(http.StatusBadRequest)) + }) + + XIt("should write the message as text", func() { + Eventually(BufferReader(recorder.Result().Body)).Should(Say("400 Bad Request: bad")) + }) + + It("should delete the connection header", func() { + Expect(recorder.Result().Header.Get("Connection")).To(Equal("")) + }) + }) + }) +}) From ba5e81550cbd70fdb2d00d55f8c4f076e5ceb96f Mon Sep 17 00:00:00 2001 From: toby lorne Date: Mon, 3 Aug 2020 22:44:30 +0100 Subject: [PATCH 02/19] handlers: clientcert uses ErrorWriter Signed-off-by: toby lorne --- handlers/clientcert.go | 12 ++++++++++-- handlers/clientcert_test.go | 5 +++-- 2 files changed, 13 insertions(+), 4 deletions(-) diff --git a/handlers/clientcert.go b/handlers/clientcert.go index 1794457b6..6bbfdc9b1 100644 --- a/handlers/clientcert.go +++ b/handlers/clientcert.go @@ -18,14 +18,22 @@ type clientCert struct { forceDeleteHeader func(req *http.Request) (bool, error) forwardingMode string logger logger.Logger + errorWriter ErrorWriter } -func NewClientCert(skipSanitization func(req *http.Request) bool, forceDeleteHeader func(req *http.Request) (bool, error), forwardingMode string, logger logger.Logger) negroni.Handler { +func NewClientCert( + skipSanitization func(req *http.Request) bool, + forceDeleteHeader func(req *http.Request) (bool, error), + forwardingMode string, + logger logger.Logger, + ew ErrorWriter, +) negroni.Handler { return &clientCert{ skipSanitization: skipSanitization, forceDeleteHeader: forceDeleteHeader, forwardingMode: forwardingMode, logger: logger, + errorWriter: ew, } } @@ -48,7 +56,7 @@ func (c *clientCert) ServeHTTP(rw http.ResponseWriter, r *http.Request, next htt delete, err := c.forceDeleteHeader(r) if err != nil { c.logger.Error("signature-validation-failed", zap.Error(err)) - writeStatus( + c.errorWriter.WriteError( rw, http.StatusBadRequest, "Failed to validate Route Service Signature for x-forwarded-client-cert", diff --git a/handlers/clientcert_test.go b/handlers/clientcert_test.go index 41842f37f..1ebbfc096 100644 --- a/handlers/clientcert_test.go +++ b/handlers/clientcert_test.go @@ -34,11 +34,12 @@ var _ = Describe("Clientcert", func() { errorForceDeleteHeader = func(req *http.Request) (bool, error) { return false, errors.New("forceDelete error") } skipSanitization = func(req *http.Request) bool { return true } dontSkipSanitization = func(req *http.Request) bool { return false } + errorWriter = handlers.NewPlaintextErrorWriter() ) DescribeTable("Client Cert Error Handling", func(forceDeleteHeaderFunc func(*http.Request) (bool, error), skipSanitizationFunc func(*http.Request) bool, errorCase string) { logger := new(logger_fakes.FakeLogger) - clientCertHandler := handlers.NewClientCert(skipSanitizationFunc, forceDeleteHeaderFunc, config.SANITIZE_SET, logger) + clientCertHandler := handlers.NewClientCert(skipSanitizationFunc, forceDeleteHeaderFunc, config.SANITIZE_SET, logger, errorWriter) nextHandlerWasCalled := false nextHandler := http.HandlerFunc(func(_ http.ResponseWriter, r *http.Request) { nextHandlerWasCalled = true }) @@ -72,7 +73,7 @@ var _ = Describe("Clientcert", func() { DescribeTable("Client Cert Result", func(forceDeleteHeaderFunc func(*http.Request) (bool, error), skipSanitizationFunc func(*http.Request) bool, forwardedClientCert string, noTLSCertStrip bool, TLSCertStrip bool, mTLSCertStrip string) { logger := new(logger_fakes.FakeLogger) - clientCertHandler := handlers.NewClientCert(skipSanitizationFunc, forceDeleteHeaderFunc, forwardedClientCert, logger) + clientCertHandler := handlers.NewClientCert(skipSanitizationFunc, forceDeleteHeaderFunc, forwardedClientCert, logger, errorWriter) nextReq := &http.Request{} nextHandler := http.HandlerFunc(func(_ http.ResponseWriter, r *http.Request) { nextReq = r }) From 2fa8f5436b788bb4759f697a1f7a87b2005ad113 Mon Sep 17 00:00:00 2001 From: toby lorne Date: Mon, 3 Aug 2020 22:49:37 +0100 Subject: [PATCH 03/19] handlers: WriteError impls use fmt.Fprintln like the http pkg Signed-off-by: toby lorne --- handlers/error.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/handlers/error.go b/handlers/error.go index 0fe50fe0a..e5e2c8311 100644 --- a/handlers/error.go +++ b/handlers/error.go @@ -43,7 +43,7 @@ func (ew *plaintextErrorWriter) WriteError( } rw.WriteHeader(code) - rw.Write([]byte(body)) + fmt.Fprintln(rw, body) } type htmlErrorWriter struct { @@ -86,7 +86,7 @@ func (ew *htmlErrorWriter) WriteError( var rendered bytes.Buffer if err := ew.tpl.Execute(&rendered, nil); err != nil { logger.Error("render-error-failed", zap.Error(err)) - rw.Write([]byte(body)) + fmt.Fprintln(rw, body) return } From a3b7162ef55c84b5d09e042fcf2d3e7f729bffce Mon Sep 17 00:00:00 2001 From: toby lorne Date: Mon, 3 Aug 2020 22:50:11 +0100 Subject: [PATCH 04/19] handlers: lookup handler uses ErrorWriter Signed-off-by: toby lorne --- handlers/lookup.go | 20 ++++++++++++++------ handlers/lookup_test.go | 9 +++++---- 2 files changed, 19 insertions(+), 10 deletions(-) diff --git a/handlers/lookup.go b/handlers/lookup.go index ac5749ebe..6950dcbcf 100644 --- a/handlers/lookup.go +++ b/handlers/lookup.go @@ -30,15 +30,23 @@ type lookupHandler struct { registry registry.Registry reporter metrics.ProxyReporter logger logger.Logger + errorWriter ErrorWriter EmptyPoolResponseCode503 bool } // NewLookup creates a handler responsible for looking up a route. -func NewLookup(registry registry.Registry, rep metrics.ProxyReporter, logger logger.Logger, emptyPoolResponseCode503 bool) negroni.Handler { +func NewLookup( + registry registry.Registry, + rep metrics.ProxyReporter, + logger logger.Logger, + ew ErrorWriter, + emptyPoolResponseCode503 bool, +) negroni.Handler { return &lookupHandler{ registry: registry, reporter: rep, logger: logger, + errorWriter: ew, EmptyPoolResponseCode503: emptyPoolResponseCode503, } } @@ -105,7 +113,7 @@ func (l *lookupHandler) handleInvalidInstanceHeader(rw http.ResponseWriter, r *h AddRouterErrorHeader(rw, "invalid_cf_app_instance_header") addNoCacheControlHeader(rw) - writeStatus( + l.errorWriter.WriteError( rw, http.StatusBadRequest, "Invalid X-CF-App-Instance Header", @@ -119,7 +127,7 @@ func (l *lookupHandler) handleMissingHost(rw http.ResponseWriter, r *http.Reques AddRouterErrorHeader(rw, "empty_host") addInvalidResponseCacheControlHeader(rw) - writeStatus( + l.errorWriter.WriteError( rw, http.StatusBadRequest, "Request had empty Host header", @@ -142,7 +150,7 @@ func (l *lookupHandler) handleMissingRoute(rw http.ResponseWriter, r *http.Reque returnStatus = http.StatusBadRequest } - writeStatus( + l.errorWriter.WriteError( rw, returnStatus, errorMsg, @@ -154,7 +162,7 @@ func (l *lookupHandler) handleUnavailableRoute(rw http.ResponseWriter, r *http.R AddRouterErrorHeader(rw, "no_endpoints") addInvalidResponseCacheControlHeader(rw) - writeStatus( + l.errorWriter.WriteError( rw, http.StatusServiceUnavailable, fmt.Sprintf("Requested route ('%s') has no available endpoints.", r.Host), @@ -168,7 +176,7 @@ func (l *lookupHandler) handleOverloadedRoute(rw http.ResponseWriter, r *http.Re AddRouterErrorHeader(rw, "Connection Limit Reached") - writeStatus( + l.errorWriter.WriteError( rw, http.StatusServiceUnavailable, fmt.Sprintf("Requested route ('%s') has reached the connection limit.", r.Host), diff --git a/handlers/lookup_test.go b/handlers/lookup_test.go index 13452a8ed..6f8ab5b18 100644 --- a/handlers/lookup_test.go +++ b/handlers/lookup_test.go @@ -30,6 +30,7 @@ var _ = Describe("Lookup", func() { nextCalled bool nextRequest *http.Request maxConnections int64 + ew = handlers.NewPlaintextErrorWriter() ) const fakeAppGUID = "aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee" @@ -50,7 +51,7 @@ var _ = Describe("Lookup", func() { req = test_util.NewRequest("GET", "example.com", "/", nil) resp = httptest.NewRecorder() handler.Use(handlers.NewRequestInfo()) - handler.Use(handlers.NewLookup(reg, rep, logger, true)) + handler.Use(handlers.NewLookup(reg, rep, logger, ew, true)) handler.UseHandler(nextHandler) }) @@ -172,7 +173,7 @@ var _ = Describe("Lookup", func() { emptyPoolResponseCode503 := true handler = negroni.New() handler.Use(handlers.NewRequestInfo()) - handler.Use(handlers.NewLookup(reg, rep, logger, emptyPoolResponseCode503)) + handler.Use(handlers.NewLookup(reg, rep, logger, ew, emptyPoolResponseCode503)) handler.UseHandler(nextHandler) pool = route.NewPool(&route.PoolOpts{ @@ -212,7 +213,7 @@ var _ = Describe("Lookup", func() { emptyPoolResponseCode503 := false handler = negroni.New() handler.Use(handlers.NewRequestInfo()) - handler.Use(handlers.NewLookup(reg, rep, logger, emptyPoolResponseCode503)) + handler.Use(handlers.NewLookup(reg, rep, logger, ew, emptyPoolResponseCode503)) handler.UseHandler(nextHandler) pool = route.NewPool(&route.PoolOpts{ @@ -470,7 +471,7 @@ var _ = Describe("Lookup", func() { Context("when request info is not set on the request context", func() { BeforeEach(func() { handler = negroni.New() - handler.Use(handlers.NewLookup(reg, rep, logger, true)) + handler.Use(handlers.NewLookup(reg, rep, logger, ew, true)) handler.UseHandler(nextHandler) pool := route.NewPool(&route.PoolOpts{ From b857644e58833d78a8fee1754fa9da198748df5c Mon Sep 17 00:00:00 2001 From: toby lorne Date: Mon, 3 Aug 2020 22:52:54 +0100 Subject: [PATCH 05/19] handlers: protocolcheck uses ErrorWriter Signed-off-by: toby lorne --- handlers/protocolcheck.go | 10 ++++++---- handlers/protocolcheck_test.go | 6 ++++-- 2 files changed, 10 insertions(+), 6 deletions(-) diff --git a/handlers/protocolcheck.go b/handlers/protocolcheck.go index 990a73f11..cef75e74e 100644 --- a/handlers/protocolcheck.go +++ b/handlers/protocolcheck.go @@ -13,14 +13,16 @@ import ( ) type protocolCheck struct { - logger logger.Logger + logger logger.Logger + errorWriter ErrorWriter } // NewProtocolCheck creates a handler responsible for checking the protocol of // the request -func NewProtocolCheck(logger logger.Logger) negroni.Handler { +func NewProtocolCheck(logger logger.Logger, errorWriter ErrorWriter) negroni.Handler { return &protocolCheck{ - logger: logger, + logger: logger, + errorWriter: errorWriter, } } @@ -29,7 +31,7 @@ func (p *protocolCheck) ServeHTTP(rw http.ResponseWriter, r *http.Request, next // must be hijacked, otherwise no response is sent back conn, buf, err := p.hijack(rw) if err != nil { - writeStatus( + p.errorWriter.WriteError( rw, http.StatusBadRequest, "Unsupported protocol", diff --git a/handlers/protocolcheck_test.go b/handlers/protocolcheck_test.go index 74feb66ac..152ab24fa 100644 --- a/handlers/protocolcheck_test.go +++ b/handlers/protocolcheck_test.go @@ -17,7 +17,9 @@ import ( var _ = Describe("Protocolcheck", func() { var ( - logger logger.Logger + logger logger.Logger + ew = handlers.NewPlaintextErrorWriter() + nextCalled bool server *ghttp.Server n *negroni.Negroni @@ -31,7 +33,7 @@ var _ = Describe("Protocolcheck", func() { n.UseFunc(func(rw http.ResponseWriter, req *http.Request, next http.HandlerFunc) { next(rw, req) }) - n.Use(handlers.NewProtocolCheck(logger)) + n.Use(handlers.NewProtocolCheck(logger, ew)) n.UseHandlerFunc(func(http.ResponseWriter, *http.Request) { nextCalled = true }) From 8e7e0eac93a11c0edef6550addcbba93eb42e437 Mon Sep 17 00:00:00 2001 From: toby lorne Date: Mon, 3 Aug 2020 22:55:19 +0100 Subject: [PATCH 06/19] handlers: routeservice uses ErrorWriter Signed-off-by: toby lorne --- handlers/routeservice.go | 29 ++++++++++++++++++----------- handlers/routeservice_test.go | 7 ++++--- 2 files changed, 22 insertions(+), 14 deletions(-) diff --git a/handlers/routeservice.go b/handlers/routeservice.go index 7ddbc222d..89eb22893 100644 --- a/handlers/routeservice.go +++ b/handlers/routeservice.go @@ -16,17 +16,24 @@ import ( ) type RouteService struct { - config *routeservice.RouteServiceConfig - registry registry.Registry - logger logger.Logger + config *routeservice.RouteServiceConfig + registry registry.Registry + logger logger.Logger + errorWriter ErrorWriter } // NewRouteService creates a handler responsible for handling route services -func NewRouteService(config *routeservice.RouteServiceConfig, routeRegistry registry.Registry, logger logger.Logger) negroni.Handler { +func NewRouteService( + config *routeservice.RouteServiceConfig, + routeRegistry registry.Registry, + logger logger.Logger, + errorWriter ErrorWriter, +) negroni.Handler { return &RouteService{ - config: config, - registry: routeRegistry, - logger: logger, + config: config, + registry: routeRegistry, + logger: logger, + errorWriter: errorWriter, } } @@ -52,7 +59,7 @@ func (r *RouteService) ServeHTTP(rw http.ResponseWriter, req *http.Request, next AddRouterErrorHeader(rw, "route_service_unsupported") - writeStatus( + r.errorWriter.WriteError( rw, http.StatusBadGateway, "Support for route services is disabled.", @@ -65,7 +72,7 @@ func (r *RouteService) ServeHTTP(rw http.ResponseWriter, req *http.Request, next AddRouterErrorHeader(rw, "route_service_unsupported") - writeStatus( + r.errorWriter.WriteError( rw, http.StatusServiceUnavailable, "Websocket requests are not supported for routes bound to Route Services.", @@ -86,7 +93,7 @@ func (r *RouteService) ServeHTTP(rw http.ResponseWriter, req *http.Request, next hasBeenToRouteService, err := r.ArrivedViaRouteService(req) if err != nil { r.logger.Error("signature-validation-failed", zap.Error(err)) - writeStatus( + r.errorWriter.WriteError( rw, http.StatusBadRequest, "Failed to validate Route Service Signature", @@ -106,7 +113,7 @@ func (r *RouteService) ServeHTTP(rw http.ResponseWriter, req *http.Request, next if err != nil { r.logger.Error("route-service-failed", zap.Error(err)) - writeStatus( + r.errorWriter.WriteError( rw, http.StatusInternalServerError, "Route service request failed.", diff --git a/handlers/routeservice_test.go b/handlers/routeservice_test.go index f638d25c8..ea460ad56 100644 --- a/handlers/routeservice_test.go +++ b/handlers/routeservice_test.go @@ -38,6 +38,7 @@ var _ = Describe("Route Service Handler", func() { forwardedUrl string logger *loggerfakes.FakeLogger + ew = handlers.NewPlaintextErrorWriter() reqChan chan *http.Request @@ -109,7 +110,7 @@ var _ = Describe("Route Service Handler", func() { handler = negroni.New() handler.Use(handlers.NewRequestInfo()) handler.UseFunc(testSetupHandler) - handler.Use(handlers.NewRouteService(config, reg, logger)) + handler.Use(handlers.NewRouteService(config, reg, logger, ew)) handler.UseHandlerFunc(nextHandler) }) @@ -594,7 +595,7 @@ var _ = Describe("Route Service Handler", func() { var badHandler *negroni.Negroni BeforeEach(func() { badHandler = negroni.New() - badHandler.Use(handlers.NewRouteService(config, reg, logger)) + badHandler.Use(handlers.NewRouteService(config, reg, logger, ew)) badHandler.UseHandlerFunc(nextHandler) }) It("calls Fatal on the logger", func() { @@ -609,7 +610,7 @@ var _ = Describe("Route Service Handler", func() { BeforeEach(func() { badHandler = negroni.New() badHandler.Use(handlers.NewRequestInfo()) - badHandler.Use(handlers.NewRouteService(config, reg, logger)) + badHandler.Use(handlers.NewRouteService(config, reg, logger, ew)) badHandler.UseHandlerFunc(nextHandler) }) It("calls Fatal on the logger", func() { From 4e2c0549d9b5da3e2fe549427ef7ed8ebcc701fd Mon Sep 17 00:00:00 2001 From: toby lorne Date: Mon, 3 Aug 2020 22:55:44 +0100 Subject: [PATCH 07/19] handlers: remove unused helper writeStatus Signed-off-by: toby lorne --- handlers/helpers.go | 15 --------------- 1 file changed, 15 deletions(-) diff --git a/handlers/helpers.go b/handlers/helpers.go index 619248c49..cdecd5870 100644 --- a/handlers/helpers.go +++ b/handlers/helpers.go @@ -6,8 +6,6 @@ import ( "strings" router_http "code.cloudfoundry.org/gorouter/common/http" - "code.cloudfoundry.org/gorouter/logger" - "github.com/uber-go/zap" ) const ( @@ -32,19 +30,6 @@ func addNoCacheControlHeader(rw http.ResponseWriter) { ) } -func writeStatus(rw http.ResponseWriter, code int, message string, logger logger.Logger) { - body := fmt.Sprintf("%d %s: %s", code, http.StatusText(code), message) - - if code != 404 { - logger.Info("status", zap.String("body", body)) - } - - http.Error(rw, body, code) - if code > 299 { - rw.Header().Del("Connection") - } -} - func hostWithoutPort(reqHost string) string { host := reqHost From 623c35d69a69e5f23d4b7dd570762b27b9f9997a Mon Sep 17 00:00:00 2001 From: toby lorne Date: Mon, 3 Aug 2020 23:01:37 +0100 Subject: [PATCH 08/19] proxy/handler: request handler uses ErrorWriter Signed-off-by: toby lorne --- proxy/handler/request_handler.go | 35 +++++++++++++-------------- proxy/handler/request_handler_test.go | 6 +++-- 2 files changed, 21 insertions(+), 20 deletions(-) diff --git a/proxy/handler/request_handler.go b/proxy/handler/request_handler.go index 1e3656f01..b0ae68afa 100644 --- a/proxy/handler/request_handler.go +++ b/proxy/handler/request_handler.go @@ -4,7 +4,6 @@ import ( "bufio" "crypto/tls" "errors" - "fmt" "net" "net/http" "strconv" @@ -27,8 +26,9 @@ const ( var NoEndpointsAvailable = errors.New("No endpoints available") type RequestHandler struct { - logger logger.Logger - reporter metrics.ProxyReporter + logger logger.Logger + errorWriter handlers.ErrorWriter + reporter metrics.ProxyReporter request *http.Request response utils.ProxyResponseWriter @@ -42,8 +42,18 @@ type RequestHandler struct { disableSourceIPLogging bool } -func NewRequestHandler(request *http.Request, response utils.ProxyResponseWriter, r metrics.ProxyReporter, logger logger.Logger, endpointDialTimeout time.Duration, tlsConfig *tls.Config, opts ...func(*RequestHandler)) *RequestHandler { +func NewRequestHandler( + request *http.Request, + response utils.ProxyResponseWriter, + r metrics.ProxyReporter, + logger logger.Logger, + errorWriter handlers.ErrorWriter, + endpointDialTimeout time.Duration, + tlsConfig *tls.Config, + opts ...func(*RequestHandler), +) *RequestHandler { reqHandler := &RequestHandler{ + errorWriter: errorWriter, reporter: r, request: request, response: response, @@ -107,7 +117,7 @@ func (h *RequestHandler) HandleBadGateway(err error, request *http.Request) { handlers.AddRouterErrorHeader(h.response, "endpoint_failure") - h.writeStatus(http.StatusBadGateway, "Registered endpoint failed to handle the request.") + h.errorWriter.WriteError(h.response, http.StatusBadGateway, "Registered endpoint failed to handle the request.", h.logger) h.response.Done() } @@ -118,7 +128,7 @@ func (h *RequestHandler) HandleTcpRequest(iter route.EndpointIterator) { backendStatusCode, err := h.serveTcp(iter, nil, onConnectionFailed) if err != nil { h.logger.Error("tcp-request-failed", zap.Error(err)) - h.writeStatus(http.StatusBadGateway, "TCP forwarding to endpoint failed.") + h.errorWriter.WriteError(h.response, http.StatusBadGateway, "TCP forwarding to endpoint failed.", h.logger) return } h.response.SetStatus(backendStatusCode) @@ -141,7 +151,7 @@ func (h *RequestHandler) HandleWebSocketRequest(iter route.EndpointIterator) { if err != nil { h.logger.Error("websocket-request-failed", zap.Error(err)) - h.writeStatus(http.StatusBadGateway, "WebSocket request to endpoint failed.") + h.errorWriter.WriteError(h.response, http.StatusBadGateway, "WebSocket request to endpoint failed.", h.logger) h.reporter.CaptureWebSocketFailure() return } @@ -150,17 +160,6 @@ func (h *RequestHandler) HandleWebSocketRequest(iter route.EndpointIterator) { h.reporter.CaptureWebSocketUpdate() } -func (h *RequestHandler) writeStatus(code int, message string) { - body := fmt.Sprintf("%d %s: %s", code, http.StatusText(code), message) - - h.logger.Info("status", zap.String("body", body)) - - http.Error(h.response, body, code) - if code > 299 { - h.response.Header().Del("Connection") - } -} - type connSuccessCB func(net.Conn, *route.Endpoint) error type connFailureCB func(error) diff --git a/proxy/handler/request_handler_test.go b/proxy/handler/request_handler_test.go index d6e1cf32f..777aaf3d2 100644 --- a/proxy/handler/request_handler_test.go +++ b/proxy/handler/request_handler_test.go @@ -7,6 +7,7 @@ import ( "net/url" "time" + "code.cloudfoundry.org/gorouter/handlers" metric "code.cloudfoundry.org/gorouter/metrics/fakes" "code.cloudfoundry.org/gorouter/proxy/handler" "code.cloudfoundry.org/gorouter/proxy/utils" @@ -22,6 +23,7 @@ var _ = Describe("RequestHandler", func() { var ( rh *handler.RequestHandler logger *test_util.TestZapLogger + ew = handlers.NewPlaintextErrorWriter() req *http.Request pr utils.ProxyResponseWriter ) @@ -45,7 +47,7 @@ var _ = Describe("RequestHandler", func() { } rh = handler.NewRequestHandler( req, pr, - &metric.FakeProxyReporter{}, logger, + &metric.FakeProxyReporter{}, logger, ew, time.Second*2, &tls.Config{}, handler.DisableXFFLogging(true), ) @@ -93,7 +95,7 @@ var _ = Describe("RequestHandler", func() { } rh = handler.NewRequestHandler( req, pr, - &metric.FakeProxyReporter{}, logger, + &metric.FakeProxyReporter{}, logger, ew, time.Second*2, &tls.Config{}, handler.DisableSourceIPLogging(true), ) From 06c7356f6ba9a8a37ecc16aa0efacaa9f149e998 Mon Sep 17 00:00:00 2001 From: toby lorne Date: Mon, 3 Aug 2020 23:05:36 +0100 Subject: [PATCH 09/19] proxy: uses hardcoded plaintext ErrorWriter Signed-off-by: toby lorne --- proxy/proxy.go | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/proxy/proxy.go b/proxy/proxy.go index 81a7b2f34..d91eddbf9 100644 --- a/proxy/proxy.go +++ b/proxy/proxy.go @@ -41,6 +41,7 @@ type proxy struct { ip string traceKey string logger logger.Logger + errorWriter handlers.ErrorWriter reporter metrics.ProxyReporter accessLogger accesslog.AccessLogger secureCookies bool @@ -73,11 +74,15 @@ func NewProxy( routeServicesTransport http.RoundTripper, ) http.Handler { + // TODO make configurable + ew := handlers.NewPlaintextErrorWriter() + p := &proxy{ accessLogger: accessLogger, traceKey: cfg.TraceKey, ip: cfg.Ip, logger: logger, + errorWriter: ew, reporter: reporter, secureCookies: cfg.SecureCookies, health: health, @@ -144,7 +149,7 @@ func NewProxy( ModifyResponse: p.modifyResponse, } - routeServiceHandler := handlers.NewRouteService(routeServiceConfig, registry, logger) + routeServiceHandler := handlers.NewRouteService(routeServiceConfig, registry, logger, ew) zipkinHandler := handlers.NewZipkin(cfg.Tracing.EnableZipkin, logger) w3cHandler := handlers.NewW3C(cfg.Tracing.EnableW3C, cfg.Tracing.W3CTenantID, logger) @@ -167,13 +172,14 @@ func NewProxy( n.Use(handlers.NewProxyHealthcheck(cfg.HealthCheckUserAgent, p.health, logger)) n.Use(zipkinHandler) n.Use(w3cHandler) - n.Use(handlers.NewProtocolCheck(logger)) - n.Use(handlers.NewLookup(registry, reporter, logger, cfg.EmptyPoolResponseCode503)) + n.Use(handlers.NewProtocolCheck(logger, ew)) + n.Use(handlers.NewLookup(registry, reporter, logger, ew, cfg.EmptyPoolResponseCode503)) n.Use(handlers.NewClientCert( SkipSanitize(routeServiceHandler.(*handlers.RouteService)), ForceDeleteXFCCHeader(routeServiceHandler.(*handlers.RouteService), cfg.ForwardedClientCert), cfg.ForwardedClientCert, logger, + ew, )) n.Use(&handlers.XForwardedProto{ SkipSanitization: SkipSanitizeXFP(routeServiceHandler.(*handlers.RouteService)), @@ -227,6 +233,7 @@ func (p *proxy) ServeHTTP(responseWriter http.ResponseWriter, request *http.Requ proxyWriter, p.reporter, p.logger, + p.errorWriter, p.endpointDialTimeout, p.backendTLSConfig, handler.DisableXFFLogging(p.disableXFFLogging), From 7cf0aeb0a96b05a520b123a2543ac125273b853d Mon Sep 17 00:00:00 2001 From: toby lorne Date: Thu, 6 Aug 2020 20:00:47 +0100 Subject: [PATCH 10/19] config: add html_error_template_file option Signed-off-by: toby lorne --- config/config.go | 2 ++ config/config_test.go | 11 +++++++++++ 2 files changed, 13 insertions(+) diff --git a/config/config.go b/config/config.go index 56f434e68..429833e34 100644 --- a/config/config.go +++ b/config/config.go @@ -261,6 +261,8 @@ type Config struct { HTTPRewrite HTTPRewrite `yaml:"http_rewrite,omitempty"` EmptyPoolResponseCode503 bool `yaml:"empty_pool_response_code_503,omitempty"` + + HTMLErrorTemplateFile string `yaml:"html_error_template_file,omitempty"` } var defaultConfig = Config{ diff --git a/config/config_test.go b/config/config_test.go index 0467f7820..d821b6102 100644 --- a/config/config_test.go +++ b/config/config_test.go @@ -604,6 +604,17 @@ backends: Expect(err).ToNot(HaveOccurred()) Expect(config.DisableHTTP).To(BeTrue()) }) + + It("defaults HTMLErrorTemplateFile to empty", func() { + Expect(config.HTMLErrorTemplateFile).To(Equal("")) + }) + + It("sets HTMLErrorTemplateFile", func() { + var b = []byte(`html_error_template_file: "/path/to/file"`) + err := config.Initialize(b) + Expect(err).ToNot(HaveOccurred()) + Expect(config.HTMLErrorTemplateFile).To(Equal("/path/to/file")) + }) }) Describe("Process", func() { From 770f3defb4c573ffe3a1d79effe9655e30a37384 Mon Sep 17 00:00:00 2001 From: toby lorne Date: Thu, 6 Aug 2020 20:14:15 +0100 Subject: [PATCH 11/19] handlers: errorwriter is own package Signed-off-by: toby lorne --- handlers/error.go => errorwriter/error_writer.go | 2 +- .../error_writer_test.go | 4 ++-- errorwriter/errorwriter_suite_test.go | 13 +++++++++++++ handlers/clientcert.go | 5 +++-- handlers/clientcert_test.go | 3 ++- handlers/lookup.go | 5 +++-- handlers/lookup_test.go | 3 ++- handlers/protocolcheck.go | 5 +++-- handlers/protocolcheck_test.go | 3 ++- handlers/routeservice.go | 5 +++-- handlers/routeservice_test.go | 3 ++- proxy/handler/request_handler.go | 5 +++-- proxy/handler/request_handler_test.go | 4 ++-- proxy/proxy.go | 5 +++-- 14 files changed, 44 insertions(+), 21 deletions(-) rename handlers/error.go => errorwriter/error_writer.go (98%) rename handlers/error_test.go => errorwriter/error_writer_test.go (98%) create mode 100644 errorwriter/errorwriter_suite_test.go diff --git a/handlers/error.go b/errorwriter/error_writer.go similarity index 98% rename from handlers/error.go rename to errorwriter/error_writer.go index e5e2c8311..977ed8907 100644 --- a/handlers/error.go +++ b/errorwriter/error_writer.go @@ -1,4 +1,4 @@ -package handlers +package errorwriter import ( "bytes" diff --git a/handlers/error_test.go b/errorwriter/error_writer_test.go similarity index 98% rename from handlers/error_test.go rename to errorwriter/error_writer_test.go index 3f959ff02..e6d9c6131 100644 --- a/handlers/error_test.go +++ b/errorwriter/error_writer_test.go @@ -1,4 +1,4 @@ -package handlers_test +package errorwriter_test import ( _ "html/template" @@ -9,7 +9,7 @@ import ( . "github.com/onsi/gomega" . "github.com/onsi/gomega/gbytes" - . "code.cloudfoundry.org/gorouter/handlers" + . "code.cloudfoundry.org/gorouter/errorwriter" loggerfakes "code.cloudfoundry.org/gorouter/logger/fakes" ) diff --git a/errorwriter/errorwriter_suite_test.go b/errorwriter/errorwriter_suite_test.go new file mode 100644 index 000000000..31cdd10da --- /dev/null +++ b/errorwriter/errorwriter_suite_test.go @@ -0,0 +1,13 @@ +package errorwriter_test + +import ( + "testing" + + . "github.com/onsi/ginkgo" + . "github.com/onsi/gomega" +) + +func TestErrorwriter(t *testing.T) { + RegisterFailHandler(Fail) + RunSpecs(t, "ErrorWriter Suite") +} diff --git a/handlers/clientcert.go b/handlers/clientcert.go index 6bbfdc9b1..7a8e0c95d 100644 --- a/handlers/clientcert.go +++ b/handlers/clientcert.go @@ -6,6 +6,7 @@ import ( "strings" "code.cloudfoundry.org/gorouter/config" + "code.cloudfoundry.org/gorouter/errorwriter" "code.cloudfoundry.org/gorouter/logger" "github.com/uber-go/zap" "github.com/urfave/negroni" @@ -18,7 +19,7 @@ type clientCert struct { forceDeleteHeader func(req *http.Request) (bool, error) forwardingMode string logger logger.Logger - errorWriter ErrorWriter + errorWriter errorwriter.ErrorWriter } func NewClientCert( @@ -26,7 +27,7 @@ func NewClientCert( forceDeleteHeader func(req *http.Request) (bool, error), forwardingMode string, logger logger.Logger, - ew ErrorWriter, + ew errorwriter.ErrorWriter, ) negroni.Handler { return &clientCert{ skipSanitization: skipSanitization, diff --git a/handlers/clientcert_test.go b/handlers/clientcert_test.go index 1ebbfc096..e42b1ee3a 100644 --- a/handlers/clientcert_test.go +++ b/handlers/clientcert_test.go @@ -9,6 +9,7 @@ import ( "strings" "code.cloudfoundry.org/gorouter/config" + "code.cloudfoundry.org/gorouter/errorwriter" "code.cloudfoundry.org/gorouter/handlers" logger_fakes "code.cloudfoundry.org/gorouter/logger/fakes" "code.cloudfoundry.org/gorouter/test_util" @@ -34,7 +35,7 @@ var _ = Describe("Clientcert", func() { errorForceDeleteHeader = func(req *http.Request) (bool, error) { return false, errors.New("forceDelete error") } skipSanitization = func(req *http.Request) bool { return true } dontSkipSanitization = func(req *http.Request) bool { return false } - errorWriter = handlers.NewPlaintextErrorWriter() + errorWriter = errorwriter.NewPlaintextErrorWriter() ) DescribeTable("Client Cert Error Handling", func(forceDeleteHeaderFunc func(*http.Request) (bool, error), skipSanitizationFunc func(*http.Request) bool, errorCase string) { diff --git a/handlers/lookup.go b/handlers/lookup.go index 6950dcbcf..234e3a1a1 100644 --- a/handlers/lookup.go +++ b/handlers/lookup.go @@ -8,6 +8,7 @@ import ( "fmt" router_http "code.cloudfoundry.org/gorouter/common/http" + "code.cloudfoundry.org/gorouter/errorwriter" "code.cloudfoundry.org/gorouter/logger" "code.cloudfoundry.org/gorouter/metrics" "code.cloudfoundry.org/gorouter/registry" @@ -30,7 +31,7 @@ type lookupHandler struct { registry registry.Registry reporter metrics.ProxyReporter logger logger.Logger - errorWriter ErrorWriter + errorWriter errorwriter.ErrorWriter EmptyPoolResponseCode503 bool } @@ -39,7 +40,7 @@ func NewLookup( registry registry.Registry, rep metrics.ProxyReporter, logger logger.Logger, - ew ErrorWriter, + ew errorwriter.ErrorWriter, emptyPoolResponseCode503 bool, ) negroni.Handler { return &lookupHandler{ diff --git a/handlers/lookup_test.go b/handlers/lookup_test.go index 6f8ab5b18..4406c6477 100644 --- a/handlers/lookup_test.go +++ b/handlers/lookup_test.go @@ -6,6 +6,7 @@ import ( "net/http/httptest" "time" + "code.cloudfoundry.org/gorouter/errorwriter" "code.cloudfoundry.org/gorouter/handlers" loggerfakes "code.cloudfoundry.org/gorouter/logger/fakes" "code.cloudfoundry.org/gorouter/metrics/fakes" @@ -30,7 +31,7 @@ var _ = Describe("Lookup", func() { nextCalled bool nextRequest *http.Request maxConnections int64 - ew = handlers.NewPlaintextErrorWriter() + ew = errorwriter.NewPlaintextErrorWriter() ) const fakeAppGUID = "aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee" diff --git a/handlers/protocolcheck.go b/handlers/protocolcheck.go index cef75e74e..36c66006a 100644 --- a/handlers/protocolcheck.go +++ b/handlers/protocolcheck.go @@ -8,18 +8,19 @@ import ( "fmt" + "code.cloudfoundry.org/gorouter/errorwriter" "code.cloudfoundry.org/gorouter/logger" "github.com/urfave/negroni" ) type protocolCheck struct { logger logger.Logger - errorWriter ErrorWriter + errorWriter errorwriter.ErrorWriter } // NewProtocolCheck creates a handler responsible for checking the protocol of // the request -func NewProtocolCheck(logger logger.Logger, errorWriter ErrorWriter) negroni.Handler { +func NewProtocolCheck(logger logger.Logger, errorWriter errorwriter.ErrorWriter) negroni.Handler { return &protocolCheck{ logger: logger, errorWriter: errorWriter, diff --git a/handlers/protocolcheck_test.go b/handlers/protocolcheck_test.go index 152ab24fa..e060a7751 100644 --- a/handlers/protocolcheck_test.go +++ b/handlers/protocolcheck_test.go @@ -5,6 +5,7 @@ import ( "net" "net/http" + "code.cloudfoundry.org/gorouter/errorwriter" "code.cloudfoundry.org/gorouter/handlers" "code.cloudfoundry.org/gorouter/logger" "code.cloudfoundry.org/gorouter/test_util" @@ -18,7 +19,7 @@ import ( var _ = Describe("Protocolcheck", func() { var ( logger logger.Logger - ew = handlers.NewPlaintextErrorWriter() + ew = errorwriter.NewPlaintextErrorWriter() nextCalled bool server *ghttp.Server diff --git a/handlers/routeservice.go b/handlers/routeservice.go index 89eb22893..1daa3c9a9 100644 --- a/handlers/routeservice.go +++ b/handlers/routeservice.go @@ -6,6 +6,7 @@ import ( "net/http" "net/url" + "code.cloudfoundry.org/gorouter/errorwriter" "code.cloudfoundry.org/gorouter/logger" "code.cloudfoundry.org/gorouter/registry" "code.cloudfoundry.org/gorouter/routeservice" @@ -19,7 +20,7 @@ type RouteService struct { config *routeservice.RouteServiceConfig registry registry.Registry logger logger.Logger - errorWriter ErrorWriter + errorWriter errorwriter.ErrorWriter } // NewRouteService creates a handler responsible for handling route services @@ -27,7 +28,7 @@ func NewRouteService( config *routeservice.RouteServiceConfig, routeRegistry registry.Registry, logger logger.Logger, - errorWriter ErrorWriter, + errorWriter errorwriter.ErrorWriter, ) negroni.Handler { return &RouteService{ config: config, diff --git a/handlers/routeservice_test.go b/handlers/routeservice_test.go index ea460ad56..dbf8845a0 100644 --- a/handlers/routeservice_test.go +++ b/handlers/routeservice_test.go @@ -9,6 +9,7 @@ import ( "time" "code.cloudfoundry.org/gorouter/common/secure" + "code.cloudfoundry.org/gorouter/errorwriter" "code.cloudfoundry.org/gorouter/handlers" "code.cloudfoundry.org/gorouter/route" "code.cloudfoundry.org/gorouter/routeservice" @@ -38,7 +39,7 @@ var _ = Describe("Route Service Handler", func() { forwardedUrl string logger *loggerfakes.FakeLogger - ew = handlers.NewPlaintextErrorWriter() + ew = errorwriter.NewPlaintextErrorWriter() reqChan chan *http.Request diff --git a/proxy/handler/request_handler.go b/proxy/handler/request_handler.go index b0ae68afa..7f63fd262 100644 --- a/proxy/handler/request_handler.go +++ b/proxy/handler/request_handler.go @@ -11,6 +11,7 @@ import ( "time" router_http "code.cloudfoundry.org/gorouter/common/http" + "code.cloudfoundry.org/gorouter/errorwriter" "code.cloudfoundry.org/gorouter/handlers" "code.cloudfoundry.org/gorouter/logger" "code.cloudfoundry.org/gorouter/metrics" @@ -27,7 +28,7 @@ var NoEndpointsAvailable = errors.New("No endpoints available") type RequestHandler struct { logger logger.Logger - errorWriter handlers.ErrorWriter + errorWriter errorwriter.ErrorWriter reporter metrics.ProxyReporter request *http.Request @@ -47,7 +48,7 @@ func NewRequestHandler( response utils.ProxyResponseWriter, r metrics.ProxyReporter, logger logger.Logger, - errorWriter handlers.ErrorWriter, + errorWriter errorwriter.ErrorWriter, endpointDialTimeout time.Duration, tlsConfig *tls.Config, opts ...func(*RequestHandler), diff --git a/proxy/handler/request_handler_test.go b/proxy/handler/request_handler_test.go index 777aaf3d2..343cea409 100644 --- a/proxy/handler/request_handler_test.go +++ b/proxy/handler/request_handler_test.go @@ -7,7 +7,7 @@ import ( "net/url" "time" - "code.cloudfoundry.org/gorouter/handlers" + "code.cloudfoundry.org/gorouter/errorwriter" metric "code.cloudfoundry.org/gorouter/metrics/fakes" "code.cloudfoundry.org/gorouter/proxy/handler" "code.cloudfoundry.org/gorouter/proxy/utils" @@ -23,7 +23,7 @@ var _ = Describe("RequestHandler", func() { var ( rh *handler.RequestHandler logger *test_util.TestZapLogger - ew = handlers.NewPlaintextErrorWriter() + ew = errorwriter.NewPlaintextErrorWriter() req *http.Request pr utils.ProxyResponseWriter ) diff --git a/proxy/proxy.go b/proxy/proxy.go index d91eddbf9..4f58fc1ff 100644 --- a/proxy/proxy.go +++ b/proxy/proxy.go @@ -14,6 +14,7 @@ import ( "code.cloudfoundry.org/gorouter/accesslog" router_http "code.cloudfoundry.org/gorouter/common/http" "code.cloudfoundry.org/gorouter/config" + "code.cloudfoundry.org/gorouter/errorwriter" "code.cloudfoundry.org/gorouter/handlers" "code.cloudfoundry.org/gorouter/logger" "code.cloudfoundry.org/gorouter/metrics" @@ -41,7 +42,7 @@ type proxy struct { ip string traceKey string logger logger.Logger - errorWriter handlers.ErrorWriter + errorWriter errorwriter.ErrorWriter reporter metrics.ProxyReporter accessLogger accesslog.AccessLogger secureCookies bool @@ -75,7 +76,7 @@ func NewProxy( ) http.Handler { // TODO make configurable - ew := handlers.NewPlaintextErrorWriter() + ew := errorwriter.NewPlaintextErrorWriter() p := &proxy{ accessLogger: accessLogger, From 9780d5cb17f4a9715ebdbeb69216c528d22f6f77 Mon Sep 17 00:00:00 2001 From: toby lorne Date: Thu, 6 Aug 2020 20:28:26 +0100 Subject: [PATCH 12/19] errorwriter: user a file rather than a template Signed-off-by: toby lorne --- errorwriter/error_writer.go | 10 +++++-- errorwriter/error_writer_test.go | 49 ++++++++++++++++++++++++++++---- 2 files changed, 52 insertions(+), 7 deletions(-) diff --git a/errorwriter/error_writer.go b/errorwriter/error_writer.go index 977ed8907..9017206bc 100644 --- a/errorwriter/error_writer.go +++ b/errorwriter/error_writer.go @@ -4,6 +4,7 @@ import ( "bytes" "fmt" "html/template" + "io/ioutil" "net/http" "code.cloudfoundry.org/gorouter/logger" @@ -50,10 +51,15 @@ type htmlErrorWriter struct { tpl *template.Template } -func NewHTMLErrorWriter(text string) (ErrorWriter, error) { +func NewHTMLErrorWriterFromFile(path string) (ErrorWriter, error) { ew := &htmlErrorWriter{} - tpl, err := template.New("error-message").Parse(text) + bytes, err := ioutil.ReadFile(path) + if err != nil { + return nil, fmt.Errorf("Could not read HTML error template file: %s", err) + } + + tpl, err := template.New("error-message").Parse(string(bytes)) if err != nil { return nil, err } diff --git a/errorwriter/error_writer_test.go b/errorwriter/error_writer_test.go index e6d9c6131..caeaf3864 100644 --- a/errorwriter/error_writer_test.go +++ b/errorwriter/error_writer_test.go @@ -2,8 +2,10 @@ package errorwriter_test import ( _ "html/template" + "io/ioutil" "net/http" "net/http/httptest" + "os" . "github.com/onsi/ginkgo" . "github.com/onsi/gomega" @@ -80,6 +82,8 @@ var _ = Describe("Plaintext ErrorWriter", func() { var _ = Describe("HTML ErrorWriter", func() { var ( + tmpFile *os.File + errorWriter ErrorWriter recorder *httptest.ResponseRecorder @@ -87,25 +91,51 @@ var _ = Describe("HTML ErrorWriter", func() { ) BeforeEach(func() { + var err error + tmpFile, err = ioutil.TempFile(os.TempDir(), "html-err-tpl") + Expect(err).NotTo(HaveOccurred()) + recorder = httptest.NewRecorder() recorder.Header().Set("Connection", "dummy") log = new(loggerfakes.FakeLogger) }) + AfterEach(func() { + os.Remove(tmpFile.Name()) + }) + + Context("when the template file does not exist", func() { + It("should return constructor error", func() { + var err error + _, err = NewHTMLErrorWriterFromFile("/path/to/non/file") + Expect(err).To(HaveOccurred()) + }) + }) + Context("when the template has invalid syntax", func() { + BeforeEach(func() { + _, err := tmpFile.Write([]byte("{{")) + Expect(err).NotTo(HaveOccurred()) + }) + It("should return constructor error", func() { var err error - _, err = NewHTMLErrorWriter("{{") + _, err = NewHTMLErrorWriterFromFile(tmpFile.Name()) Expect(err).To(HaveOccurred()) }) }) Context("when the template errors", func() { + BeforeEach(func() { + _, err := tmpFile.Write([]byte(`{{template "notexists"}}`)) + Expect(err).NotTo(HaveOccurred()) + }) + Context("when the response is a success", func() { BeforeEach(func() { var err error - errorWriter, err = NewHTMLErrorWriter(`{{template "notexists"}}`) + errorWriter, err = NewHTMLErrorWriterFromFile(tmpFile.Name()) Expect(err).NotTo(HaveOccurred()) errorWriter.WriteError(recorder, http.StatusOK, "hi", log) @@ -133,7 +163,10 @@ var _ = Describe("HTML ErrorWriter", func() { Context("when the response is not a success", func() { BeforeEach(func() { var err error - errorWriter, err = NewHTMLErrorWriter(`{{template "notexists"}}`) + _, err = tmpFile.Write([]byte(`{{template "notexists"}}`)) + Expect(err).NotTo(HaveOccurred()) + + errorWriter, err = NewHTMLErrorWriterFromFile(tmpFile.Name()) Expect(err).NotTo(HaveOccurred()) errorWriter.WriteError(recorder, http.StatusBadRequest, "bad", log) @@ -160,7 +193,10 @@ var _ = Describe("HTML ErrorWriter", func() { Context("when the response is a success", func() { BeforeEach(func() { - errorWriter, err = NewHTMLErrorWriter(`success`) + _, err := tmpFile.Write([]byte(`success`)) + Expect(err).NotTo(HaveOccurred()) + + errorWriter, err = NewHTMLErrorWriterFromFile(tmpFile.Name()) Expect(err).NotTo(HaveOccurred()) errorWriter.WriteError(recorder, http.StatusOK, "hi", log) @@ -185,7 +221,10 @@ var _ = Describe("HTML ErrorWriter", func() { Context("when the response is not a success", func() { BeforeEach(func() { - errorWriter, err = NewHTMLErrorWriter(`failure`) + _, err := tmpFile.Write([]byte(`failure`)) + Expect(err).NotTo(HaveOccurred()) + + errorWriter, err = NewHTMLErrorWriterFromFile(tmpFile.Name()) Expect(err).NotTo(HaveOccurred()) errorWriter.WriteError(recorder, http.StatusBadRequest, "bad", log) From 2ad334faad3e355f95a36295aeb3f387c8ea20a4 Mon Sep 17 00:00:00 2001 From: toby lorne Date: Thu, 6 Aug 2020 20:47:48 +0100 Subject: [PATCH 13/19] proxy: takes errorwriter as constructor arg Signed-off-by: toby lorne --- integration/perf_test.go | 5 ++++- main.go | 5 +++++ proxy/proxy.go | 14 ++++++-------- proxy/proxy_suite_test.go | 4 +++- proxy/proxy_unit_test.go | 4 +++- 5 files changed, 21 insertions(+), 11 deletions(-) diff --git a/integration/perf_test.go b/integration/perf_test.go index 9410b9007..d00cd28b9 100644 --- a/integration/perf_test.go +++ b/integration/perf_test.go @@ -9,6 +9,7 @@ import ( "code.cloudfoundry.org/gorouter/accesslog" "code.cloudfoundry.org/gorouter/config" + "code.cloudfoundry.org/gorouter/errorwriter" "code.cloudfoundry.org/gorouter/metrics" "code.cloudfoundry.org/gorouter/proxy" "code.cloudfoundry.org/gorouter/registry" @@ -39,10 +40,12 @@ var _ = Describe("AccessLogRecord", func() { accesslog, err := accesslog.CreateRunningAccessLogger(logger, ls, c) Expect(err).ToNot(HaveOccurred()) + ew := errorwriter.NewPlaintextErrorWriter() + rss, err := router.NewRouteServicesServer() Expect(err).ToNot(HaveOccurred()) var h *health.Health - proxy.NewProxy(logger, accesslog, c, r, combinedReporter, &routeservice.RouteServiceConfig{}, + proxy.NewProxy(logger, accesslog, ew, c, r, combinedReporter, &routeservice.RouteServiceConfig{}, &tls.Config{}, &tls.Config{}, h, rss.GetRoundTripper()) b.Time("RegisterTime", func() { diff --git a/main.go b/main.go index 3832bd5b5..f5abe179d 100644 --- a/main.go +++ b/main.go @@ -20,6 +20,7 @@ import ( "code.cloudfoundry.org/gorouter/common/schema" "code.cloudfoundry.org/gorouter/common/secure" "code.cloudfoundry.org/gorouter/config" + "code.cloudfoundry.org/gorouter/errorwriter" goRouterLogger "code.cloudfoundry.org/gorouter/logger" "code.cloudfoundry.org/gorouter/mbus" "code.cloudfoundry.org/gorouter/metrics" @@ -170,10 +171,14 @@ func main() { logger.Fatal("new-route-services-server", zap.Error(err)) } + // TODO make configurable + ew := errorwriter.NewPlaintextErrorWriter() + h = &health.Health{} proxy := proxy.NewProxy( logger, accessLogger, + ew, c, registry, compositeReporter, diff --git a/proxy/proxy.go b/proxy/proxy.go index 4f58fc1ff..c691600de 100644 --- a/proxy/proxy.go +++ b/proxy/proxy.go @@ -65,6 +65,7 @@ type proxy struct { func NewProxy( logger logger.Logger, accessLogger accesslog.AccessLogger, + errorWriter errorwriter.ErrorWriter, cfg *config.Config, registry registry.Registry, reporter metrics.ProxyReporter, @@ -75,15 +76,12 @@ func NewProxy( routeServicesTransport http.RoundTripper, ) http.Handler { - // TODO make configurable - ew := errorwriter.NewPlaintextErrorWriter() - p := &proxy{ accessLogger: accessLogger, traceKey: cfg.TraceKey, ip: cfg.Ip, logger: logger, - errorWriter: ew, + errorWriter: errorWriter, reporter: reporter, secureCookies: cfg.SecureCookies, health: health, @@ -150,7 +148,7 @@ func NewProxy( ModifyResponse: p.modifyResponse, } - routeServiceHandler := handlers.NewRouteService(routeServiceConfig, registry, logger, ew) + routeServiceHandler := handlers.NewRouteService(routeServiceConfig, registry, logger, errorWriter) zipkinHandler := handlers.NewZipkin(cfg.Tracing.EnableZipkin, logger) w3cHandler := handlers.NewW3C(cfg.Tracing.EnableW3C, cfg.Tracing.W3CTenantID, logger) @@ -173,14 +171,14 @@ func NewProxy( n.Use(handlers.NewProxyHealthcheck(cfg.HealthCheckUserAgent, p.health, logger)) n.Use(zipkinHandler) n.Use(w3cHandler) - n.Use(handlers.NewProtocolCheck(logger, ew)) - n.Use(handlers.NewLookup(registry, reporter, logger, ew, cfg.EmptyPoolResponseCode503)) + n.Use(handlers.NewProtocolCheck(logger, errorWriter)) + n.Use(handlers.NewLookup(registry, reporter, logger, errorWriter, cfg.EmptyPoolResponseCode503)) n.Use(handlers.NewClientCert( SkipSanitize(routeServiceHandler.(*handlers.RouteService)), ForceDeleteXFCCHeader(routeServiceHandler.(*handlers.RouteService), cfg.ForwardedClientCert), cfg.ForwardedClientCert, logger, - ew, + errorWriter, )) n.Use(&handlers.XForwardedProto{ SkipSanitization: SkipSanitizeXFP(routeServiceHandler.(*handlers.RouteService)), diff --git a/proxy/proxy_suite_test.go b/proxy/proxy_suite_test.go index e9a34d00f..e6ae96e40 100644 --- a/proxy/proxy_suite_test.go +++ b/proxy/proxy_suite_test.go @@ -14,6 +14,7 @@ import ( "code.cloudfoundry.org/gorouter/accesslog" "code.cloudfoundry.org/gorouter/common/secure" "code.cloudfoundry.org/gorouter/config" + "code.cloudfoundry.org/gorouter/errorwriter" "code.cloudfoundry.org/gorouter/logger" "code.cloudfoundry.org/gorouter/proxy" "code.cloudfoundry.org/gorouter/registry" @@ -52,6 +53,7 @@ var ( fakeEmitter *fake.FakeEventEmitter fakeRouteServicesClient *sharedfakes.RoundTripper skipSanitization func(req *http.Request) bool + ew = errorwriter.NewPlaintextErrorWriter() ) func TestProxy(t *testing.T) { @@ -128,7 +130,7 @@ var _ = JustBeforeEach(func() { fakeRouteServicesClient = &sharedfakes.RoundTripper{} - p = proxy.NewProxy(testLogger, al, conf, r, fakeReporter, routeServiceConfig, tlsConfig, tlsConfig, healthStatus, fakeRouteServicesClient) + p = proxy.NewProxy(testLogger, al, ew, conf, r, fakeReporter, routeServiceConfig, tlsConfig, tlsConfig, healthStatus, fakeRouteServicesClient) server := http.Server{Handler: p} go server.Serve(proxyServer) diff --git a/proxy/proxy_unit_test.go b/proxy/proxy_unit_test.go index 6ab23de5e..5a9464329 100644 --- a/proxy/proxy_unit_test.go +++ b/proxy/proxy_unit_test.go @@ -11,6 +11,7 @@ import ( "code.cloudfoundry.org/gorouter/common/health" fakelogger "code.cloudfoundry.org/gorouter/accesslog/fakes" + "code.cloudfoundry.org/gorouter/errorwriter" sharedfakes "code.cloudfoundry.org/gorouter/fakes" "code.cloudfoundry.org/gorouter/logger" "code.cloudfoundry.org/gorouter/metrics" @@ -38,6 +39,7 @@ var _ = Describe("Proxy Unit tests", func() { routeServiceConfig *routeservice.RouteServiceConfig rt *sharedfakes.RoundTripper tlsConfig *tls.Config + ew = errorwriter.NewPlaintextErrorWriter() ) Describe("ServeHTTP", func() { @@ -71,7 +73,7 @@ var _ = Describe("Proxy Unit tests", func() { conf.HealthCheckUserAgent = "HTTP-Monitor/1.1" skipSanitization = func(req *http.Request) bool { return false } - proxyObj = proxy.NewProxy(logger, fakeAccessLogger, conf, r, combinedReporter, + proxyObj = proxy.NewProxy(logger, fakeAccessLogger, ew, conf, r, combinedReporter, routeServiceConfig, tlsConfig, tlsConfig, &health.Health{}, rt) r.Register(route.Uri("some-app"), &route.Endpoint{Stats: route.NewStats()}) From 736df74582dc31b66949f478110fcd7124832043 Mon Sep 17 00:00:00 2001 From: toby lorne Date: Thu, 6 Aug 2020 21:51:54 +0100 Subject: [PATCH 14/19] integration-tests: add helper to check if started Signed-off-by: toby lorne --- integration/backend_keepalive_test.go | 4 ++-- integration/common_integration_test.go | 10 +++++++++- integration/gdpr_test.go | 8 ++++---- integration/header_test.go | 8 ++++---- integration/large_upload_test.go | 2 +- integration/redirect_test.go | 2 +- integration/route_services_test.go | 2 +- integration/tls_to_backends_test.go | 2 +- integration/w3c_tracing_test.go | 2 +- integration/web_socket_test.go | 2 +- integration/x_forwarded_proto_integration_test.go | 4 ++-- integration/xfcc_integration_test.go | 2 +- 12 files changed, 28 insertions(+), 20 deletions(-) diff --git a/integration/backend_keepalive_test.go b/integration/backend_keepalive_test.go index c71571590..9bfd1567d 100644 --- a/integration/backend_keepalive_test.go +++ b/integration/backend_keepalive_test.go @@ -49,7 +49,7 @@ var _ = Describe("KeepAlive (HTTP Persistent Connections) to backends", func() { BeforeEach(func() { testState.cfg.DisableKeepAlives = true - testState.StartGorouter() + testState.StartGorouterOrFail() testApp.Start() testState.register(testApp.Server, testAppRoute) Expect(testApp.GetConnStates()).To(BeEmpty()) @@ -84,7 +84,7 @@ var _ = Describe("KeepAlive (HTTP Persistent Connections) to backends", func() { Context("when KeepAlives are enabled", func() { BeforeEach(func() { testState.cfg.DisableKeepAlives = false - testState.StartGorouter() + testState.StartGorouterOrFail() }) Context("when connecting to a non-TLS backend", func() { diff --git a/integration/common_integration_test.go b/integration/common_integration_test.go index 11663042b..1f796b5b2 100644 --- a/integration/common_integration_test.go +++ b/integration/common_integration_test.go @@ -229,7 +229,7 @@ func (s *testState) registerAndWait(rm mbus.RegistryMessage) { }).Should(BeTrue()) } -func (s *testState) StartGorouter() { +func (s *testState) StartGorouter() *Session { Expect(s.cfg).NotTo(BeNil(), "set up test cfg before calling this function") s.natsRunner = test_util.NewNATSRunner(int(s.cfg.Nats[0].Port)) @@ -249,15 +249,23 @@ func (s *testState) StartGorouter() { s.gorouterSession, err = Start(cmd, GinkgoWriter, GinkgoWriter) Expect(err).ToNot(HaveOccurred()) + return s.gorouterSession +} + +func (s *testState) StartGorouterOrFail() { + s.StartGorouter() + Eventually(func() *Session { if s.gorouterSession.ExitCode() >= 0 { Fail("gorouter quit early!") } return s.gorouterSession }, 1*time.Minute).Should(Say("starting")) + Eventually(s.gorouterSession, 1*time.Minute).Should(Say(`Successfully-connected-to-nats.*localhost:\d+`)) Eventually(s.gorouterSession, 1*time.Minute).Should(Say(`gorouter.started`)) + var err error s.mbusClient, err = newMessageBus(s.cfg) Expect(err).ToNot(HaveOccurred()) } diff --git a/integration/gdpr_test.go b/integration/gdpr_test.go index af09382d8..eff84b80c 100644 --- a/integration/gdpr_test.go +++ b/integration/gdpr_test.go @@ -41,7 +41,7 @@ var _ = Describe("GDPR", func() { testState.cfg.AccessLog.File = filepath.Join(accessLog, "access.log") testState.cfg.Logging.DisableLogForwardedFor = true - testState.StartGorouter() + testState.StartGorouterOrFail() testApp := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusOK) @@ -74,7 +74,7 @@ var _ = Describe("GDPR", func() { testState.cfg.Status.User = "user" testState.cfg.Status.Port = 6705 testState.cfg.Logging.DisableLogForwardedFor = true - testState.StartGorouter() + testState.StartGorouterOrFail() wsApp := test.NewWebSocketApp([]route.Uri{"ws-app." + test_util.LocalhostDNS}, testState.cfg.Port, testState.mbusClient, time.Millisecond, "") wsApp.Register() @@ -118,7 +118,7 @@ var _ = Describe("GDPR", func() { testState.cfg.AccessLog.File = filepath.Join(accessLog, "access.log") testState.cfg.Logging.DisableLogSourceIP = true - testState.StartGorouter() + testState.StartGorouterOrFail() testApp := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusOK) @@ -147,7 +147,7 @@ var _ = Describe("GDPR", func() { testState.cfg.Status.User = "user" testState.cfg.Status.Port = 6706 testState.cfg.Logging.DisableLogSourceIP = true - testState.StartGorouter() + testState.StartGorouterOrFail() wsApp := test.NewWebSocketApp([]route.Uri{"ws-app." + test_util.LocalhostDNS}, testState.cfg.Port, testState.mbusClient, time.Millisecond, "") wsApp.Register() diff --git a/integration/header_test.go b/integration/header_test.go index f9d0d6826..b5e18e64d 100644 --- a/integration/header_test.go +++ b/integration/header_test.go @@ -46,7 +46,7 @@ var _ = Describe("Headers", func() { Context("Sanity Test", func() { BeforeEach(func() { - testState.StartGorouter() + testState.StartGorouterOrFail() testApp.Start() testState.register(testApp.Server, testAppRoute) }) @@ -72,7 +72,7 @@ var _ = Describe("Headers", func() { }, } - testState.StartGorouter() + testState.StartGorouterOrFail() testApp.Start() testState.register(testApp.Server, testAppRoute) }) @@ -104,7 +104,7 @@ var _ = Describe("Headers", func() { }, } - testState.StartGorouter() + testState.StartGorouterOrFail() testApp.Start() testState.register(testApp.Server, testAppRoute) }) @@ -130,7 +130,7 @@ var _ = Describe("Headers", func() { BeforeEach(func() { - testState.StartGorouter() + testState.StartGorouterOrFail() testApp.Start() testState.register(testApp.Server, testAppRoute) }) diff --git a/integration/large_upload_test.go b/integration/large_upload_test.go index e0d566c07..40df46574 100644 --- a/integration/large_upload_test.go +++ b/integration/large_upload_test.go @@ -25,7 +25,7 @@ var _ = Describe("Large upload", func() { BeforeEach(func() { testState = NewTestState() - testState.StartGorouter() + testState.StartGorouterOrFail() }) AfterEach(func() { diff --git a/integration/redirect_test.go b/integration/redirect_test.go index a627fec72..574de53f3 100644 --- a/integration/redirect_test.go +++ b/integration/redirect_test.go @@ -39,7 +39,7 @@ var _ = Describe("Headers", func() { Context("When an app returns a 3xx-redirect", func() { BeforeEach(func() { - testState.StartGorouter() + testState.StartGorouterOrFail() testApp.Start() testState.register(testApp.Server, testAppRoute) }) diff --git a/integration/route_services_test.go b/integration/route_services_test.go index 0b0808fd7..ee53d3b5e 100644 --- a/integration/route_services_test.go +++ b/integration/route_services_test.go @@ -17,7 +17,7 @@ var _ = Describe("Route services", func() { BeforeEach(func() { testState = NewTestState() - testState.StartGorouter() + testState.StartGorouterOrFail() }) AfterEach(func() { diff --git a/integration/tls_to_backends_test.go b/integration/tls_to_backends_test.go index a5fac48c1..3956df046 100644 --- a/integration/tls_to_backends_test.go +++ b/integration/tls_to_backends_test.go @@ -33,7 +33,7 @@ var _ = Describe("TLS to backends", func() { testState = NewTestState() testState.cfg.AccessLog.File = filepath.Join(accessLog, "access.log") - testState.StartGorouter() + testState.StartGorouterOrFail() }) AfterEach(func() { diff --git a/integration/w3c_tracing_test.go b/integration/w3c_tracing_test.go index 0c63145ea..0f9b6b669 100644 --- a/integration/w3c_tracing_test.go +++ b/integration/w3c_tracing_test.go @@ -48,7 +48,7 @@ var _ = Describe("W3C tracing headers", func() { }) JustBeforeEach(func() { - testState.StartGorouter() + testState.StartGorouterOrFail() testApp = httptest.NewServer( http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { diff --git a/integration/web_socket_test.go b/integration/web_socket_test.go index cf81fe559..d76c0d499 100644 --- a/integration/web_socket_test.go +++ b/integration/web_socket_test.go @@ -30,7 +30,7 @@ var _ = Describe("Websockets", func() { testState = NewTestState() testState.cfg.AccessLog.File = filepath.Join(accessLog, "access.log") - testState.StartGorouter() + testState.StartGorouterOrFail() }) AfterEach(func() { diff --git a/integration/x_forwarded_proto_integration_test.go b/integration/x_forwarded_proto_integration_test.go index 4fa722ea4..59b950bc8 100644 --- a/integration/x_forwarded_proto_integration_test.go +++ b/integration/x_forwarded_proto_integration_test.go @@ -94,7 +94,7 @@ var _ = Describe("modifications of X-Forwarded-Proto header", func() { It(fmt.Sprintf("gorouter config %+v: sets the headers correctly", goroutercfg), func() { testState.cfg.ForceForwardedProtoHttps = goroutercfg.forceForwardedProtoHTTPS testState.cfg.SanitizeForwardedProto = goroutercfg.sanitizeForwardedProto - testState.StartGorouter() + testState.StartGorouterOrFail() doRequest := func(testCase testCase, hostname string) { req := testState.newRequest(fmt.Sprintf("%s://%s", testCase.clientRequestScheme, hostname)) @@ -206,7 +206,7 @@ var _ = Describe("modifications of X-Forwarded-Proto header", func() { hostname := "basic-app.some.domain" testState.cfg.ForceForwardedProtoHttps = goroutercfg.forceForwardedProtoHTTPS testState.cfg.SanitizeForwardedProto = goroutercfg.sanitizeForwardedProto - testState.StartGorouter() + testState.StartGorouterOrFail() doRequest := func(testCase rsTestCase, hostname string) { req := testState.newRequest(fmt.Sprintf("%s://%s", testCase.clientRequestScheme, hostname)) diff --git a/integration/xfcc_integration_test.go b/integration/xfcc_integration_test.go index 17958c177..fe08917d4 100644 --- a/integration/xfcc_integration_test.go +++ b/integration/xfcc_integration_test.go @@ -113,7 +113,7 @@ var _ = Describe("modifications of X-Forwarded-Client-Cert", func() { testState.cfg.RouteServiceRecommendHttps = true } - testState.StartGorouter() + testState.StartGorouterOrFail() doRequest := func(scheme, hostname string, addXFCCHeader bool) { req := testState.newRequest(fmt.Sprintf("%s://%s", scheme, hostname)) From b1995b0a95ac414897ce84c730c9e9066049cb48 Mon Sep 17 00:00:00 2001 From: toby lorne Date: Thu, 6 Aug 2020 21:52:22 +0100 Subject: [PATCH 15/19] gorouter: can use static html templates and integration tests Signed-off-by: toby lorne --- integration/error_writer_test.go | 127 +++++++++++++++++++++++++++++++ main.go | 13 +++- 2 files changed, 137 insertions(+), 3 deletions(-) create mode 100644 integration/error_writer_test.go diff --git a/integration/error_writer_test.go b/integration/error_writer_test.go new file mode 100644 index 000000000..905cc0582 --- /dev/null +++ b/integration/error_writer_test.go @@ -0,0 +1,127 @@ +package integration + +import ( + "fmt" + "io/ioutil" + "os" + + . "github.com/onsi/ginkgo" + . "github.com/onsi/gomega" + . "github.com/onsi/gomega/gbytes" + . "github.com/onsi/gomega/gexec" +) + +var _ = Describe("Error Writers", func() { + const ( + hostname = "error-writers.cloudfoundry.org" + ) + + var ( + testState *testState + + statusCode int + body []byte + + doRequest = func() { + req := testState.newRequest(fmt.Sprintf("http://not-%s", hostname)) + + resp, err := testState.client.Do(req) + Expect(err).NotTo(HaveOccurred()) + + statusCode = resp.StatusCode + + body, err = ioutil.ReadAll(resp.Body) + Expect(err).NotTo(HaveOccurred()) + + resp.Body.Close() + } + ) + + BeforeEach(func() { + testState = NewTestState() + }) + + AfterEach(func() { + testState.StopAndCleanup() + }) + + Context("when using plaintext error writer", func() { + BeforeEach(func() { + }) + + JustBeforeEach(func() { + testState.StartGorouterOrFail() + }) + + BeforeEach(func() { + testState.cfg.HTMLErrorTemplateFile = "" + }) + + It("responds with a plaintext error message", func() { + doRequest() + + Expect(statusCode).To(Equal(404)) + + Expect(string(body)).To(Equal(fmt.Sprintf( + "404 Not Found: Requested route ('not-%s') does not exist.\n", + hostname, + ))) + }) + }) + + Context("when using HTML error writer", func() { + Context("when the template does not exist", func() { + BeforeEach(func() { + testState.cfg.HTMLErrorTemplateFile = "/path/to/non/file" + }) + + It("should log a fatal error", func() { + session := testState.StartGorouter() + + Eventually(session).Should(Say("Could not read HTML error template file")) + Eventually(session).Should(Say("/path/to/non/file")) + + Eventually(session).Should(Exit()) + Expect(session.ExitCode()).To(Equal(1)) + }) + }) + + Context("when the template exists", func() { + var ( + tmpFile *os.File + ) + + BeforeEach(func() { + tpl := `an error message` + + var err error + tmpFile, err = ioutil.TempFile(os.TempDir(), "html-err-tpl") + Expect(err).NotTo(HaveOccurred()) + + testState.cfg.HTMLErrorTemplateFile = tmpFile.Name() + + _, err = tmpFile.Write([]byte(tpl)) + Expect(err).NotTo(HaveOccurred()) + }) + + JustBeforeEach(func() { + testState.StartGorouterOrFail() + }) + + AfterEach(func() { + os.Remove(tmpFile.Name()) + }) + + It("responds with a templated error message", func() { + doRequest() + + Expect(statusCode).To(Equal(404)) + + Expect(string(body)).To(Equal( + // FIXME + "an error message", + )) + }) + }) + }) +}) diff --git a/main.go b/main.go index f5abe179d..d04e16d80 100644 --- a/main.go +++ b/main.go @@ -77,6 +77,16 @@ func main() { logger, minLagerLogLevel := createLogger(prefix, c.Logging.Level, c.Logging.Format.Timestamp) logger.Info("starting") + var ew errorwriter.ErrorWriter + if c.HTMLErrorTemplateFile != "" { + ew, err = errorwriter.NewHTMLErrorWriterFromFile(c.HTMLErrorTemplateFile) + if err != nil { + logger.Fatal("new-html-error-template-from-file", zap.Error(err)) + } + } else { + ew = errorwriter.NewPlaintextErrorWriter() + } + err = dropsonde.Initialize(c.Logging.MetronAddress, c.Logging.JobName) if err != nil { logger.Fatal("dropsonde-initialize-error", zap.Error(err)) @@ -171,9 +181,6 @@ func main() { logger.Fatal("new-route-services-server", zap.Error(err)) } - // TODO make configurable - ew := errorwriter.NewPlaintextErrorWriter() - h = &health.Health{} proxy := proxy.NewProxy( logger, From 62d2669b7429f4c3289d2c62af53ece62b4bdd4a Mon Sep 17 00:00:00 2001 From: toby lorne Date: Sun, 9 Aug 2020 21:31:40 +0100 Subject: [PATCH 16/19] errorwriter: sets content type correctly Signed-off-by: toby lorne --- errorwriter/error_writer.go | 18 ++++++++--- errorwriter/error_writer_test.go | 52 ++++++++++++++++++++++++++++---- 2 files changed, 59 insertions(+), 11 deletions(-) diff --git a/errorwriter/error_writer.go b/errorwriter/error_writer.go index 9017206bc..cba6b26b7 100644 --- a/errorwriter/error_writer.go +++ b/errorwriter/error_writer.go @@ -43,6 +43,9 @@ func (ew *plaintextErrorWriter) WriteError( rw.Header().Del("Connection") } + rw.Header().Set("Content-Type", "text/plain; charset=utf-8") + rw.Header().Set("X-Content-Type-Options", "nosniff") + rw.WriteHeader(code) fmt.Fprintln(rw, body) } @@ -87,14 +90,19 @@ func (ew *htmlErrorWriter) WriteError( rw.Header().Del("Connection") } - rw.WriteHeader(code) - + var respBytes []byte var rendered bytes.Buffer if err := ew.tpl.Execute(&rendered, nil); err != nil { logger.Error("render-error-failed", zap.Error(err)) - fmt.Fprintln(rw, body) - return + rw.Header().Set("Content-Type", "text/plain; charset=utf-8") + rw.Header().Set("X-Content-Type-Options", "nosniff") + respBytes = []byte(body) + } else { + rw.Header().Set("Content-Type", "text/html; charset=utf-8") + rw.Header().Set("X-Content-Type-Options", "nosniff") + respBytes = rendered.Bytes() } - rw.Write(rendered.Bytes()) + rw.WriteHeader(code) + rw.Write(respBytes) } diff --git a/errorwriter/error_writer_test.go b/errorwriter/error_writer_test.go index caeaf3864..c5a896cee 100644 --- a/errorwriter/error_writer_test.go +++ b/errorwriter/error_writer_test.go @@ -50,9 +50,17 @@ var _ = Describe("Plaintext ErrorWriter", func() { Expect(message).To(Equal("status")) }) - It("should keep the connection header", func() { + It("should keep the Connection header", func() { Expect(recorder.Result().Header.Get("Connection")).To(Equal("dummy")) }) + + It("should set the Content-Type header", func() { + Expect(recorder.Result().Header.Get("Content-Type")).To(Equal("text/plain; charset=utf-8")) + }) + + It("should set the X-Content-Type-Options header", func() { + Expect(recorder.Result().Header.Get("X-Content-Type-Options")).To(Equal("nosniff")) + }) }) Context("when the response code is not a success", func() { @@ -74,7 +82,7 @@ var _ = Describe("Plaintext ErrorWriter", func() { Expect(message).To(Equal("status")) }) - It("should delete the connection header", func() { + It("should delete the Connection header", func() { Expect(recorder.Result().Header.Get("Connection")).To(Equal("")) }) }) @@ -155,9 +163,17 @@ var _ = Describe("HTML ErrorWriter", func() { Expect(message).To(Equal("status")) }) - It("should keep the connection header", func() { + It("should keep the Connection header", func() { Expect(recorder.Result().Header.Get("Connection")).To(Equal("dummy")) }) + + It("should set the Content-Type header", func() { + Expect(recorder.Result().Header.Get("Content-Type")).To(Equal("text/plain; charset=utf-8")) + }) + + It("should set the X-Content-Type-Options header", func() { + Expect(recorder.Result().Header.Get("X-Content-Type-Options")).To(Equal("nosniff")) + }) }) Context("when the response is not a success", func() { @@ -180,9 +196,17 @@ var _ = Describe("HTML ErrorWriter", func() { Eventually(BufferReader(recorder.Result().Body)).Should(Say("400 Bad Request: bad")) }) - It("should delete the connection header", func() { + It("should delete the Connection header", func() { Expect(recorder.Result().Header.Get("Connection")).To(Equal("")) }) + + It("should set the Content-Type header", func() { + Expect(recorder.Result().Header.Get("Content-Type")).To(Equal("text/plain; charset=utf-8")) + }) + + It("should set the X-Content-Type-Options header", func() { + Expect(recorder.Result().Header.Get("X-Content-Type-Options")).To(Equal("nosniff")) + }) }) }) @@ -214,9 +238,17 @@ var _ = Describe("HTML ErrorWriter", func() { Eventually(BufferReader(recorder.Result().Body)).Should(Say("200 OK: hi")) }) - It("should keep the connection header", func() { + It("should keep the Connection header", func() { Expect(recorder.Result().Header.Get("Connection")).To(Equal("dummy")) }) + + It("should set the Content-Type header", func() { + Expect(recorder.Result().Header.Get("Content-Type")).To(Equal("text/html; charset=utf-8")) + }) + + It("should set the X-Content-Type-Options header", func() { + Expect(recorder.Result().Header.Get("X-Content-Type-Options")).To(Equal("nosniff")) + }) }) Context("when the response is not a success", func() { @@ -242,9 +274,17 @@ var _ = Describe("HTML ErrorWriter", func() { Eventually(BufferReader(recorder.Result().Body)).Should(Say("400 Bad Request: bad")) }) - It("should delete the connection header", func() { + It("should delete the Connection header", func() { Expect(recorder.Result().Header.Get("Connection")).To(Equal("")) }) + + It("should set the Content-Type header", func() { + Expect(recorder.Result().Header.Get("Content-Type")).To(Equal("text/html; charset=utf-8")) + }) + + It("should set the X-Content-Type-Options header", func() { + Expect(recorder.Result().Header.Get("X-Content-Type-Options")).To(Equal("nosniff")) + }) }) }) }) From 8cc7b8248365bccb72f6ab9fb210df0d2baa3703 Mon Sep 17 00:00:00 2001 From: toby lorne Date: Sun, 9 Aug 2020 21:46:58 +0100 Subject: [PATCH 17/19] errorwriter: html templates have context which includes headers, status, and message Signed-off-by: toby lorne --- errorwriter/error_writer.go | 17 ++++++++++++++++- errorwriter/error_writer_test.go | 12 ++++++++---- integration/error_writer_test.go | 10 +++++----- 3 files changed, 29 insertions(+), 10 deletions(-) diff --git a/errorwriter/error_writer.go b/errorwriter/error_writer.go index cba6b26b7..d2bf750e6 100644 --- a/errorwriter/error_writer.go +++ b/errorwriter/error_writer.go @@ -54,6 +54,13 @@ type htmlErrorWriter struct { tpl *template.Template } +type htmlErrorWriterContext struct { + Status int + StatusText string + Message string + Header http.Header +} + func NewHTMLErrorWriterFromFile(path string) (ErrorWriter, error) { ew := &htmlErrorWriter{} @@ -90,9 +97,17 @@ func (ew *htmlErrorWriter) WriteError( rw.Header().Del("Connection") } + tplContext := htmlErrorWriterContext{ + Status: code, + StatusText: http.StatusText(code), + Message: message, + Header: rw.Header(), + } + rw.Header().Set("Content-Type", "text/html; charset=utf-8") + var respBytes []byte var rendered bytes.Buffer - if err := ew.tpl.Execute(&rendered, nil); err != nil { + if err := ew.tpl.Execute(&rendered, &tplContext); err != nil { logger.Error("render-error-failed", zap.Error(err)) rw.Header().Set("Content-Type", "text/plain; charset=utf-8") rw.Header().Set("X-Content-Type-Options", "nosniff") diff --git a/errorwriter/error_writer_test.go b/errorwriter/error_writer_test.go index c5a896cee..75c1b337d 100644 --- a/errorwriter/error_writer_test.go +++ b/errorwriter/error_writer_test.go @@ -217,7 +217,9 @@ var _ = Describe("HTML ErrorWriter", func() { Context("when the response is a success", func() { BeforeEach(func() { - _, err := tmpFile.Write([]byte(`success`)) + _, err := tmpFile.Write([]byte( + `{{ .Status }} {{ .StatusText }}: {{ .Message }}`, + )) Expect(err).NotTo(HaveOccurred()) errorWriter, err = NewHTMLErrorWriterFromFile(tmpFile.Name()) @@ -234,7 +236,7 @@ var _ = Describe("HTML ErrorWriter", func() { Expect(recorder.Result().StatusCode).To(Equal(http.StatusOK)) }) - XIt("should write the message as text", func() { + It("should write the message as text", func() { Eventually(BufferReader(recorder.Result().Body)).Should(Say("200 OK: hi")) }) @@ -253,7 +255,9 @@ var _ = Describe("HTML ErrorWriter", func() { Context("when the response is not a success", func() { BeforeEach(func() { - _, err := tmpFile.Write([]byte(`failure`)) + _, err := tmpFile.Write([]byte( + `{{ .Status }} {{ .StatusText }}: {{ .Message }}`, + )) Expect(err).NotTo(HaveOccurred()) errorWriter, err = NewHTMLErrorWriterFromFile(tmpFile.Name()) @@ -270,7 +274,7 @@ var _ = Describe("HTML ErrorWriter", func() { Expect(recorder.Result().StatusCode).To(Equal(http.StatusBadRequest)) }) - XIt("should write the message as text", func() { + It("should write the message as text", func() { Eventually(BufferReader(recorder.Result().Body)).Should(Say("400 Bad Request: bad")) }) diff --git a/integration/error_writer_test.go b/integration/error_writer_test.go index 905cc0582..f4df57bb9 100644 --- a/integration/error_writer_test.go +++ b/integration/error_writer_test.go @@ -92,7 +92,7 @@ var _ = Describe("Error Writers", func() { ) BeforeEach(func() { - tpl := `an error message` + tpl := `{{ .Message }}` var err error tmpFile, err = ioutil.TempFile(os.TempDir(), "html-err-tpl") @@ -117,10 +117,10 @@ var _ = Describe("Error Writers", func() { Expect(statusCode).To(Equal(404)) - Expect(string(body)).To(Equal( - // FIXME - "an error message", - )) + Expect(string(body)).To(Equal(fmt.Sprintf( + "Requested route ('not-%s') does not exist.", + hostname, + ))) }) }) }) From 95c69957e67b3d02c76e5dda7eec9d54bd5dd66a Mon Sep 17 00:00:00 2001 From: toby lorne Date: Sun, 9 Aug 2020 22:04:51 +0100 Subject: [PATCH 18/19] errorwriter: add integration test for headers Signed-off-by: toby lorne --- integration/error_writer_test.go | 36 ++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/integration/error_writer_test.go b/integration/error_writer_test.go index f4df57bb9..ac8a78e9f 100644 --- a/integration/error_writer_test.go +++ b/integration/error_writer_test.go @@ -123,5 +123,41 @@ var _ = Describe("Error Writers", func() { ))) }) }) + + Context("when the template references an HTTP header", func() { + var ( + tmpFile *os.File + ) + + BeforeEach(func() { + tpl := `Code: {{ .Status }} ; Cause: {{ .Header.Get "X-Cf-RouterError" }}` + + var err error + tmpFile, err = ioutil.TempFile(os.TempDir(), "html-err-tpl") + Expect(err).NotTo(HaveOccurred()) + + testState.cfg.HTMLErrorTemplateFile = tmpFile.Name() + + _, err = tmpFile.Write([]byte(tpl)) + Expect(err).NotTo(HaveOccurred()) + }) + + JustBeforeEach(func() { + testState.StartGorouterOrFail() + }) + + AfterEach(func() { + os.Remove(tmpFile.Name()) + }) + + It("responds with a templated error message", func() { + doRequest() + + Expect(statusCode).To(Equal(404)) + + Expect(string(body)).To(ContainSubstring("Code: 404")) + Expect(string(body)).To(ContainSubstring("Cause: unknown_route")) + }) + }) }) }) From e60b55bccb539f3922cccc7fd904e0994f4d42b0 Mon Sep 17 00:00:00 2001 From: toby lorne Date: Tue, 11 Aug 2020 20:53:12 +0100 Subject: [PATCH 19/19] router: uses errorwriter Signed-off-by: toby lorne --- router/router_drain_test.go | 7 +++++-- router/router_test.go | 5 ++++- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/router/router_drain_test.go b/router/router_drain_test.go index fa39d1fdc..2c1ea78bc 100644 --- a/router/router_drain_test.go +++ b/router/router_drain_test.go @@ -15,6 +15,7 @@ import ( "code.cloudfoundry.org/gorouter/accesslog" "code.cloudfoundry.org/gorouter/common/schema" cfg "code.cloudfoundry.org/gorouter/config" + "code.cloudfoundry.org/gorouter/errorwriter" sharedfakes "code.cloudfoundry.org/gorouter/fakes" "code.cloudfoundry.org/gorouter/logger" "code.cloudfoundry.org/gorouter/mbus" @@ -50,6 +51,8 @@ var _ = Describe("Router", func() { subscriber ifrit.Process natsPort uint16 healthStatus *health.Health + + ew = errorwriter.NewPlaintextErrorWriter() ) testAndVerifyRouterStopsNoDrain := func(signals chan os.Signal, closeChannel chan struct{}, sigs ...os.Signal) { @@ -182,7 +185,7 @@ var _ = Describe("Router", func() { config.HealthCheckUserAgent = "HTTP-Monitor/1.1" rt := &sharedfakes.RoundTripper{} - p = proxy.NewProxy(logger, &accesslog.NullAccessLogger{}, config, registry, combinedReporter, + p = proxy.NewProxy(logger, &accesslog.NullAccessLogger{}, ew, config, registry, combinedReporter, &routeservice.RouteServiceConfig{}, &tls.Config{}, &tls.Config{}, healthStatus, rt) errChan := make(chan error, 2) @@ -414,7 +417,7 @@ var _ = Describe("Router", func() { h.SetHealth(health.Healthy) config.HealthCheckUserAgent = "HTTP-Monitor/1.1" rt := &sharedfakes.RoundTripper{} - p := proxy.NewProxy(logger, &accesslog.NullAccessLogger{}, config, registry, combinedReporter, + p := proxy.NewProxy(logger, &accesslog.NullAccessLogger{}, ew, config, registry, combinedReporter, &routeservice.RouteServiceConfig{}, &tls.Config{}, &tls.Config{}, h, rt) errChan = make(chan error, 2) diff --git a/router/router_test.go b/router/router_test.go index 8681e53c4..d57dfdf0c 100644 --- a/router/router_test.go +++ b/router/router_test.go @@ -30,6 +30,7 @@ import ( "code.cloudfoundry.org/gorouter/accesslog" "code.cloudfoundry.org/gorouter/common/schema" cfg "code.cloudfoundry.org/gorouter/config" + "code.cloudfoundry.org/gorouter/errorwriter" sharedfakes "code.cloudfoundry.org/gorouter/fakes" "code.cloudfoundry.org/gorouter/handlers" "code.cloudfoundry.org/gorouter/logger" @@ -1933,10 +1934,12 @@ func initializeRouter(config *cfg.Config, backendIdleTimeout, requestTimeout tim combinedReporter := &metrics.CompositeReporter{VarzReporter: varz, ProxyReporter: metricReporter} routeServiceConfig := routeservice.NewRouteServiceConfig(logger, true, config.RouteServicesHairpinning, config.EndpointTimeout, nil, nil, false) + ew := errorwriter.NewPlaintextErrorWriter() + proxyConfig := *config proxyConfig.EndpointTimeout = requestTimeout routeServicesTransport := &sharedfakes.RoundTripper{} - p := proxy.NewProxy(logger, &accesslog.NullAccessLogger{}, &proxyConfig, registry, combinedReporter, + p := proxy.NewProxy(logger, &accesslog.NullAccessLogger{}, ew, &proxyConfig, registry, combinedReporter, routeServiceConfig, &tls.Config{}, &tls.Config{}, &health.Health{}, routeServicesTransport) h := &health.Health{}