From bd6699f4217e18069c43728827203f6045e9277b Mon Sep 17 00:00:00 2001 From: taik0 Date: Tue, 28 Aug 2018 13:37:51 +0200 Subject: [PATCH 01/10] Run the http.Server from another func --- router/gin/router.go | 19 ++----------------- router/mux/router.go | 18 ++---------------- router/router.go | 27 +++++++++++++++++++++++++++ 3 files changed, 31 insertions(+), 33 deletions(-) diff --git a/router/gin/router.go b/router/gin/router.go index 0db310691..666fea4aa 100644 --- a/router/gin/router.go +++ b/router/gin/router.go @@ -3,8 +3,6 @@ package gin import ( "context" - "fmt" - "net/http" "github.com/gin-gonic/gin" @@ -87,23 +85,10 @@ func (r ginRouter) Run(cfg config.ServiceConfig) { c.Header(router.CompleteResponseHeaderName, router.HeaderIncompleteResponseValue) }) - s := &http.Server{ - Addr: fmt.Sprintf(":%d", cfg.Port), - Handler: r.cfg.Engine, - ReadTimeout: cfg.ReadTimeout, - WriteTimeout: cfg.WriteTimeout, - ReadHeaderTimeout: cfg.ReadHeaderTimeout, - IdleTimeout: cfg.IdleTimeout, - } - - go func() { - r.cfg.Logger.Critical(s.ListenAndServe()) - }() - - <-r.ctx.Done() - if err := s.Shutdown(context.Background()); err != nil { + if err := router.RunServer(r.ctx, cfg, r.cfg.Engine); err != nil { r.cfg.Logger.Error(err.Error()) } + r.cfg.Logger.Info("Router execution ended") } diff --git a/router/mux/router.go b/router/mux/router.go index 8bba1a8fc..95ba06a06 100644 --- a/router/mux/router.go +++ b/router/mux/router.go @@ -3,7 +3,6 @@ package mux import ( "context" - "fmt" "net/http" "github.com/devopsfaith/krakend/config" @@ -81,23 +80,10 @@ func (r httpRouter) Run(cfg config.ServiceConfig) { r.registerKrakendEndpoints(cfg.Endpoints) - server := http.Server{ - Addr: fmt.Sprintf(":%d", cfg.Port), - Handler: r.handler(), - ReadTimeout: cfg.ReadTimeout, - WriteTimeout: cfg.WriteTimeout, - ReadHeaderTimeout: cfg.ReadHeaderTimeout, - IdleTimeout: cfg.IdleTimeout, - } - - go func() { - r.cfg.Logger.Critical(server.ListenAndServe()) - }() - - <-r.ctx.Done() - if err := server.Shutdown(context.Background()); err != nil { + if err := router.RunServer(r.ctx, cfg, r.handler()); err != nil { r.cfg.Logger.Error(err.Error()) } + r.cfg.Logger.Info("Router execution ended") } diff --git a/router/router.go b/router/router.go index a7f0e8540..6bcee084c 100644 --- a/router/router.go +++ b/router/router.go @@ -4,6 +4,7 @@ package router import ( "context" "errors" + "fmt" "net" "net/http" "sync" @@ -80,3 +81,29 @@ func InitHTTPDefaultTransport(cfg config.ServiceConfig) { } }) } + +// RunServer runs a http.Server with the given handler and configuration +func RunServer(ctx context.Context, cfg config.ServiceConfig, handler http.Handler) error { + done := make(chan error) + defer close(done) + s := &http.Server{ + Addr: fmt.Sprintf(":%d", cfg.Port), + Handler: handler, + ReadTimeout: cfg.ReadTimeout, + WriteTimeout: cfg.WriteTimeout, + ReadHeaderTimeout: cfg.ReadHeaderTimeout, + IdleTimeout: cfg.IdleTimeout, + } + + go func() { + done <- s.ListenAndServe() + }() + + select { + case err := <-done: + return err + case <-ctx.Done(): + return s.Shutdown(context.Background()) + } + +} From 6978f1c1871e20e3d67a23afc5be5db0bfaae0a6 Mon Sep 17 00:00:00 2001 From: taik0 Date: Tue, 28 Aug 2018 15:42:46 +0200 Subject: [PATCH 02/10] Setup the Runserver via factory configuration. --- router/mux/router.go | 13 +++++++++---- router/router.go | 2 +- 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/router/mux/router.go b/router/mux/router.go index 95ba06a06..c19947102 100644 --- a/router/mux/router.go +++ b/router/mux/router.go @@ -14,6 +14,8 @@ import ( // DefaultDebugPattern is the default pattern used to define the debug endpoint const DefaultDebugPattern = "/__debug/" +type RunServerFunc func(context.Context, config.ServiceConfig, http.Handler) error + // Config is the struct that collects the parts the router should be builded from type Config struct { Engine Engine @@ -22,6 +24,7 @@ type Config struct { ProxyFactory proxy.Factory Logger logging.Logger DebugPattern string + RunServer RunServerFunc } // HandlerMiddleware is the interface for the decorators over the http.Handler @@ -39,6 +42,7 @@ func DefaultFactory(pf proxy.Factory, logger logging.Logger) router.Factory { ProxyFactory: pf, Logger: logger, DebugPattern: DefaultDebugPattern, + RunServer: router.RunServer, }, } } @@ -57,17 +61,18 @@ type factory struct { // New implements the factory interface func (rf factory) New() router.Router { - return httpRouter{rf.cfg, context.Background()} + return httpRouter{rf.cfg, context.Background(), rf.cfg.RunServer} } // NewWithContext implements the factory interface func (rf factory) NewWithContext(ctx context.Context) router.Router { - return httpRouter{rf.cfg, ctx} + return httpRouter{rf.cfg, ctx, rf.cfg.RunServer} } type httpRouter struct { - cfg Config - ctx context.Context + cfg Config + ctx context.Context + RunServer func(context.Context, config.ServiceConfig, http.Handler) error } // Run implements the router interface diff --git a/router/router.go b/router/router.go index 6bcee084c..20f22e692 100644 --- a/router/router.go +++ b/router/router.go @@ -85,7 +85,6 @@ func InitHTTPDefaultTransport(cfg config.ServiceConfig) { // RunServer runs a http.Server with the given handler and configuration func RunServer(ctx context.Context, cfg config.ServiceConfig, handler http.Handler) error { done := make(chan error) - defer close(done) s := &http.Server{ Addr: fmt.Sprintf(":%d", cfg.Port), Handler: handler, @@ -101,6 +100,7 @@ func RunServer(ctx context.Context, cfg config.ServiceConfig, handler http.Handl select { case err := <-done: + close(done) return err case <-ctx.Done(): return s.Shutdown(context.Background()) From de54f675cec81d93e12a6ccdfe0b14515f8d075d Mon Sep 17 00:00:00 2001 From: taik0 Date: Tue, 28 Aug 2018 16:38:26 +0200 Subject: [PATCH 03/10] New returns NewWithContext --- router/mux/router.go | 2 +- router/router.go | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/router/mux/router.go b/router/mux/router.go index c19947102..0f9be955e 100644 --- a/router/mux/router.go +++ b/router/mux/router.go @@ -61,7 +61,7 @@ type factory struct { // New implements the factory interface func (rf factory) New() router.Router { - return httpRouter{rf.cfg, context.Background(), rf.cfg.RunServer} + return rf.NewWithContext(context.Background()) } // NewWithContext implements the factory interface diff --git a/router/router.go b/router/router.go index 20f22e692..9141a6a73 100644 --- a/router/router.go +++ b/router/router.go @@ -100,7 +100,6 @@ func RunServer(ctx context.Context, cfg config.ServiceConfig, handler http.Handl select { case err := <-done: - close(done) return err case <-ctx.Done(): return s.Shutdown(context.Background()) From 4aa3e2be7b19acb29d385424da30c97c83ae2a44 Mon Sep 17 00:00:00 2001 From: taik0 Date: Tue, 28 Aug 2018 16:53:25 +0200 Subject: [PATCH 04/10] Use the router RunServer --- router/mux/router.go | 2 +- router/mux/router_test.go | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/router/mux/router.go b/router/mux/router.go index 0f9be955e..98fc49706 100644 --- a/router/mux/router.go +++ b/router/mux/router.go @@ -85,7 +85,7 @@ func (r httpRouter) Run(cfg config.ServiceConfig) { r.registerKrakendEndpoints(cfg.Endpoints) - if err := router.RunServer(r.ctx, cfg, r.handler()); err != nil { + if err := r.RunServer(r.ctx, cfg, r.handler()); err != nil { r.cfg.Logger.Error(err.Error()) } diff --git a/router/mux/router_test.go b/router/mux/router_test.go index f4e4d804c..5f4bd2f5d 100644 --- a/router/mux/router_test.go +++ b/router/mux/router_test.go @@ -141,6 +141,7 @@ func TestDefaultFactory_ko(t *testing.T) { HandlerFactory: EndpointHandler, ProxyFactory: noopProxyFactory(map[string]interface{}{"supu": "tupu"}), Logger: logger, + RunServer: router.RunServer, }).NewWithContext(ctx) serviceCfg := config.ServiceConfig{ From 2167dab5a193a5efa511b7cf2c835f9d65df2a77 Mon Sep 17 00:00:00 2001 From: taik0 Date: Tue, 28 Aug 2018 17:07:53 +0200 Subject: [PATCH 05/10] Update gin and gorilla/negroni adapters to use RunServer --- router/gin/router.go | 14 ++++++++++---- router/gorilla/router.go | 1 + 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/router/gin/router.go b/router/gin/router.go index 666fea4aa..25d58b271 100644 --- a/router/gin/router.go +++ b/router/gin/router.go @@ -3,6 +3,7 @@ package gin import ( "context" + "net/http" "github.com/gin-gonic/gin" @@ -12,6 +13,8 @@ import ( "github.com/devopsfaith/krakend/router" ) +type RunServerFunc func(context.Context, config.ServiceConfig, http.Handler) error + // Config is the struct that collects the parts the router should be builded from type Config struct { Engine *gin.Engine @@ -19,6 +22,7 @@ type Config struct { HandlerFactory HandlerFactory ProxyFactory proxy.Factory Logger logging.Logger + Runserver RunServerFunc } // DefaultFactory returns a gin router factory with the injected proxy factory and logger. @@ -31,6 +35,7 @@ func DefaultFactory(proxyFactory proxy.Factory, logger logging.Logger) router.Fa HandlerFactory: EndpointHandler, ProxyFactory: proxyFactory, Logger: logger, + Runserver: router.RunServer, }, ) } @@ -51,12 +56,13 @@ func (rf factory) New() router.Router { // NewWithContext implements the factory interface func (rf factory) NewWithContext(ctx context.Context) router.Router { - return ginRouter{rf.cfg, ctx} + return ginRouter{rf.cfg, ctx, rf.cfg.Runserver} } type ginRouter struct { - cfg Config - ctx context.Context + cfg Config + ctx context.Context + RunServer RunServerFunc } // Run implements the router interface @@ -85,7 +91,7 @@ func (r ginRouter) Run(cfg config.ServiceConfig) { c.Header(router.CompleteResponseHeaderName, router.HeaderIncompleteResponseValue) }) - if err := router.RunServer(r.ctx, cfg, r.cfg.Engine); err != nil { + if err := r.RunServer(r.ctx, cfg, r.cfg.Engine); err != nil { r.cfg.Logger.Error(err.Error()) } diff --git a/router/gorilla/router.go b/router/gorilla/router.go index f7dc1b234..2ffdce9b6 100644 --- a/router/gorilla/router.go +++ b/router/gorilla/router.go @@ -26,6 +26,7 @@ func DefaultConfig(pf proxy.Factory, logger logging.Logger) mux.Config { ProxyFactory: pf, Logger: logger, DebugPattern: "/__debug/{params}", + RunServer: router.RunServer, } } From 40d81a7dcfeb406378c90c2b1ab4fd0feb0884ea Mon Sep 17 00:00:00 2001 From: taik0 Date: Tue, 28 Aug 2018 18:28:49 +0200 Subject: [PATCH 06/10] Add comment to RunServerFunc type --- router/gin/router.go | 1 + router/mux/router.go | 1 + 2 files changed, 2 insertions(+) diff --git a/router/gin/router.go b/router/gin/router.go index 25d58b271..33d701f73 100644 --- a/router/gin/router.go +++ b/router/gin/router.go @@ -13,6 +13,7 @@ import ( "github.com/devopsfaith/krakend/router" ) +// RunServerFunc is a func that will run the http Server with the given params. type RunServerFunc func(context.Context, config.ServiceConfig, http.Handler) error // Config is the struct that collects the parts the router should be builded from diff --git a/router/mux/router.go b/router/mux/router.go index 98fc49706..52ed2237d 100644 --- a/router/mux/router.go +++ b/router/mux/router.go @@ -14,6 +14,7 @@ import ( // DefaultDebugPattern is the default pattern used to define the debug endpoint const DefaultDebugPattern = "/__debug/" +// RunServerFunc is a func that will run the http Server with the given params. type RunServerFunc func(context.Context, config.ServiceConfig, http.Handler) error // Config is the struct that collects the parts the router should be builded from From 8ad78d6155906c93e370d09b581839c7b7507baf Mon Sep 17 00:00:00 2001 From: taik0 Date: Wed, 29 Aug 2018 21:51:37 +0200 Subject: [PATCH 07/10] Use the type RunServerFunc --- router/mux/router.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/router/mux/router.go b/router/mux/router.go index 52ed2237d..8835db3d6 100644 --- a/router/mux/router.go +++ b/router/mux/router.go @@ -73,7 +73,7 @@ func (rf factory) NewWithContext(ctx context.Context) router.Router { type httpRouter struct { cfg Config ctx context.Context - RunServer func(context.Context, config.ServiceConfig, http.Handler) error + RunServer RunServerFunc } // Run implements the router interface From a1d55a960fb84a5562d0a4f39025cddedecc9957 Mon Sep 17 00:00:00 2001 From: taik0 Date: Wed, 29 Aug 2018 23:12:18 +0200 Subject: [PATCH 08/10] Use RunServer with capitalized S everywhere --- router/gin/router.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/router/gin/router.go b/router/gin/router.go index 33d701f73..fcdb6c697 100644 --- a/router/gin/router.go +++ b/router/gin/router.go @@ -23,7 +23,7 @@ type Config struct { HandlerFactory HandlerFactory ProxyFactory proxy.Factory Logger logging.Logger - Runserver RunServerFunc + RunServer RunServerFunc } // DefaultFactory returns a gin router factory with the injected proxy factory and logger. @@ -36,7 +36,7 @@ func DefaultFactory(proxyFactory proxy.Factory, logger logging.Logger) router.Fa HandlerFactory: EndpointHandler, ProxyFactory: proxyFactory, Logger: logger, - Runserver: router.RunServer, + RunServer: router.RunServer, }, ) } @@ -57,7 +57,7 @@ func (rf factory) New() router.Router { // NewWithContext implements the factory interface func (rf factory) NewWithContext(ctx context.Context) router.Router { - return ginRouter{rf.cfg, ctx, rf.cfg.Runserver} + return ginRouter{rf.cfg, ctx, rf.cfg.RunServer} } type ginRouter struct { From d62854ccbe4c2d4e04d33a9ee702d01698c8f4e2 Mon Sep 17 00:00:00 2001 From: taik0 Date: Wed, 29 Aug 2018 23:13:13 +0200 Subject: [PATCH 09/10] Add tests for an errored RunServer and improve code coverage --- router/gin/router_test.go | 38 ++++++++++++++++++++++++++++++++++++++ router/mux/router_test.go | 38 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 76 insertions(+) diff --git a/router/gin/router_test.go b/router/gin/router_test.go index c3aa6a56c..6cfe80310 100644 --- a/router/gin/router_test.go +++ b/router/gin/router_test.go @@ -5,9 +5,11 @@ package gin import ( "bytes" "context" + "errors" "fmt" "io/ioutil" "net/http" + "regexp" "testing" "time" @@ -15,6 +17,7 @@ import ( "github.com/devopsfaith/krakend/logging" "github.com/devopsfaith/krakend/proxy" "github.com/devopsfaith/krakend/router" + "github.com/gin-gonic/gin" ) func TestDefaultFactory_ok(t *testing.T) { @@ -221,6 +224,41 @@ func TestDefaultFactory_proxyFactoryCrash(t *testing.T) { } } +func TestRunServer_ko(t *testing.T) { + buff := new(bytes.Buffer) + logger, err := logging.NewLogger("ERROR", buff, "") + if err != nil { + t.Error("building the logger:", err.Error()) + return + } + + errorMsg := "runServer error" + runServerFunc := func(_ context.Context, _ config.ServiceConfig, _ http.Handler) error { + return errors.New(errorMsg) + } + + pf := noopProxyFactory(map[string]interface{}{"supu": "tupu"}) + r := NewFactory( + Config{ + Engine: gin.Default(), + Middlewares: []gin.HandlerFunc{}, + HandlerFactory: EndpointHandler, + ProxyFactory: pf, + Logger: logger, + RunServer: runServerFunc, + }, + ).New() + + serviceCfg := config.ServiceConfig{} + + go func() { r.Run(serviceCfg) }() + time.Sleep(5 * time.Millisecond) + re := regexp.MustCompile(errorMsg) + if !re.MatchString(string(buff.Bytes())) { + t.Errorf("the logger doesn't contain the expected msg: %s", buff.Bytes()) + } +} + func checkResponseIs404(t *testing.T, req *http.Request) { expectedBody := "404 page not found" resp, err := http.DefaultClient.Do(req) diff --git a/router/mux/router_test.go b/router/mux/router_test.go index 5f4bd2f5d..ab59832b1 100644 --- a/router/mux/router_test.go +++ b/router/mux/router_test.go @@ -5,9 +5,11 @@ package mux import ( "bytes" "context" + "errors" "fmt" "io/ioutil" "net/http" + "regexp" "testing" "time" @@ -228,6 +230,42 @@ func TestDefaultFactory_proxyFactoryCrash(t *testing.T) { } } +func TestRunServer_ko(t *testing.T) { + buff := new(bytes.Buffer) + logger, err := logging.NewLogger("DEBUG", buff, "") + if err != nil { + t.Error("building the logger:", err.Error()) + return + } + + errorMsg := "runServer error" + runServerFunc := func(_ context.Context, _ config.ServiceConfig, _ http.Handler) error { + return errors.New(errorMsg) + } + + pf := noopProxyFactory(map[string]interface{}{"supu": "tupu"}) + r := NewFactory( + Config{ + Engine: DefaultEngine(), + Middlewares: []HandlerMiddleware{}, + HandlerFactory: EndpointHandler, + ProxyFactory: pf, + Logger: logger, + DebugPattern: DefaultDebugPattern, + RunServer: runServerFunc, + }, + ).New() + + serviceCfg := config.ServiceConfig{} + + go func() { r.Run(serviceCfg) }() + time.Sleep(5 * time.Millisecond) + re := regexp.MustCompile(errorMsg) + if !re.MatchString(string(buff.Bytes())) { + t.Errorf("the logger doesn't contain the expected msg: %s", buff.Bytes()) + } +} + func checkResponseIs404(t *testing.T, req *http.Request) { expectedBody := "404 page not found\n" resp, err := http.DefaultClient.Do(req) From 016b0ce236a27106cc0e6a632abec6884e473bd6 Mon Sep 17 00:00:00 2001 From: taik0 Date: Thu, 30 Aug 2018 18:20:42 +0200 Subject: [PATCH 10/10] Run r.Run outside a go routine since we expect it to fail (no need to sleep) --- router/gin/router_test.go | 4 +--- router/mux/router_test.go | 4 +--- 2 files changed, 2 insertions(+), 6 deletions(-) diff --git a/router/gin/router_test.go b/router/gin/router_test.go index 6cfe80310..9031a92c9 100644 --- a/router/gin/router_test.go +++ b/router/gin/router_test.go @@ -250,9 +250,7 @@ func TestRunServer_ko(t *testing.T) { ).New() serviceCfg := config.ServiceConfig{} - - go func() { r.Run(serviceCfg) }() - time.Sleep(5 * time.Millisecond) + r.Run(serviceCfg) re := regexp.MustCompile(errorMsg) if !re.MatchString(string(buff.Bytes())) { t.Errorf("the logger doesn't contain the expected msg: %s", buff.Bytes()) diff --git a/router/mux/router_test.go b/router/mux/router_test.go index ab59832b1..7edd46a56 100644 --- a/router/mux/router_test.go +++ b/router/mux/router_test.go @@ -257,9 +257,7 @@ func TestRunServer_ko(t *testing.T) { ).New() serviceCfg := config.ServiceConfig{} - - go func() { r.Run(serviceCfg) }() - time.Sleep(5 * time.Millisecond) + r.Run(serviceCfg) re := regexp.MustCompile(errorMsg) if !re.MatchString(string(buff.Bytes())) { t.Errorf("the logger doesn't contain the expected msg: %s", buff.Bytes())