From fdbf6a61600745b5cb1022117dcd9b06d392dc84 Mon Sep 17 00:00:00 2001 From: Valery Piashchynski Date: Tue, 23 Feb 2021 19:00:44 +0300 Subject: [PATCH] - Remove unneeded mutex from the `http.Workers` method. - Rename `checker` plugin package to `status`, remove `/v1` endpoint prefix (#557) - Add static, headers, status, gzip plugins to the `main.go`. Signed-off-by: Valery Piashchynski --- .github/ISSUE_TEMPLATE/feature_request.md | 2 +- .rr.yaml | 2 +- CHANGELOG.md | 3 + plugins/gzip/plugin.go | 8 +-- plugins/http/plugin.go | 17 ++--- plugins/static/plugin.go | 3 +- plugins/{checker => status}/config.go | 2 +- plugins/{checker => status}/interface.go | 2 +- plugins/{checker => status}/plugin.go | 15 ++-- plugins/{checker => status}/rpc.go | 2 +- .../checker/configs/.rr-checker-init.yaml | 3 +- tests/plugins/checker/plugin_test.go | 10 +-- tests/plugins/gzip/plugin_test.go | 4 +- tests/plugins/http/http_plugin_test.go | 2 +- tests/plugins/static/static_plugin_test.go | 71 +++---------------- 15 files changed, 44 insertions(+), 102 deletions(-) rename plugins/{checker => status}/config.go (71%) rename plugins/{checker => status}/interface.go (92%) rename plugins/{checker => status}/plugin.go (90%) rename plugins/{checker => status}/rpc.go (97%) diff --git a/.github/ISSUE_TEMPLATE/feature_request.md b/.github/ISSUE_TEMPLATE/feature_request.md index 8c4b568f0..470dd474d 100755 --- a/.github/ISSUE_TEMPLATE/feature_request.md +++ b/.github/ISSUE_TEMPLATE/feature_request.md @@ -3,7 +3,7 @@ name: Feature request about: Suggest an idea for this project title: "[FEATURE REQUEST]" labels: C-feature-request -assignees: 48d90782, wolfy-j +assignees: 48d90782 --- diff --git a/.rr.yaml b/.rr.yaml index 2ce7b5424..5e214da26 100755 --- a/.rr.yaml +++ b/.rr.yaml @@ -99,7 +99,7 @@ http: supervisor: # watch_tick defines how often to check the state of the workers (seconds) watch_tick: 1s - # ttl defines maximum time worker is allowed to live (seconds) + # ttl defines maximum time worker is allowed to live (seconds) (soft) ttl: 0 # idle_ttl defines maximum duration worker can spend in idle mode after first use. Disabled when 0 (seconds) idle_ttl: 10s diff --git a/CHANGELOG.md b/CHANGELOG.md index ef577fe30..b11ffdada 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,9 @@ v2.0.0-RC.4 (20.02.2021) - PHP tests use latest signatures (https://github.com/spiral/roadrunner/pull/550) - Endure container update to v1.0.0-RC.2 version +- Remove unneeded mutex from the `http.Workers` method. +- Rename `checker` plugin package to `status`, remove `/v1` endpoint prefix (#557) +- Add static, headers, status, gzip plugins to the `main.go`. v2.0.0-RC.3 (17.02.2021) ------------------- diff --git a/plugins/gzip/plugin.go b/plugins/gzip/plugin.go index eee6c1d36..949c68880 100644 --- a/plugins/gzip/plugin.go +++ b/plugins/gzip/plugin.go @@ -8,19 +8,19 @@ import ( const PluginName = "gzip" -type Gzip struct{} +type Plugin struct{} // needed for the Endure -func (g *Gzip) Init() error { +func (g *Plugin) Init() error { return nil } -func (g *Gzip) Middleware(next http.Handler) http.HandlerFunc { +func (g *Plugin) Middleware(next http.Handler) http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { gziphandler.GzipHandler(next).ServeHTTP(w, r) } } -func (g *Gzip) Name() string { +func (g *Plugin) Name() string { return PluginName } diff --git a/plugins/http/plugin.go b/plugins/http/plugin.go index bab03edc9..d9903d65c 100644 --- a/plugins/http/plugin.go +++ b/plugins/http/plugin.go @@ -17,12 +17,12 @@ import ( "github.com/spiral/errors" "github.com/spiral/roadrunner/v2/pkg/pool" "github.com/spiral/roadrunner/v2/pkg/worker" - "github.com/spiral/roadrunner/v2/plugins/checker" "github.com/spiral/roadrunner/v2/plugins/config" "github.com/spiral/roadrunner/v2/plugins/http/attributes" httpConfig "github.com/spiral/roadrunner/v2/plugins/http/config" "github.com/spiral/roadrunner/v2/plugins/logger" "github.com/spiral/roadrunner/v2/plugins/server" + "github.com/spiral/roadrunner/v2/plugins/status" "github.com/spiral/roadrunner/v2/utils" "golang.org/x/net/http2" "golang.org/x/net/http2/h2c" @@ -109,12 +109,9 @@ func (s *Plugin) Init(cfg config.Configurer, log logger.Logger, server server.Se func (s *Plugin) logCallback(event interface{}) { if ev, ok := event.(ResponseEvent); ok { - s.log.Debug("", + s.log.Debug(fmt.Sprintf("%d %s %s", ev.Response.Status, ev.Request.Method, ev.Request.URI), "remote", ev.Request.RemoteAddr, - "ts", ev.Elapsed().String(), - "resp.status", ev.Response.Status, - "method", ev.Request.Method, - "uri", ev.Request.URI, + "elapsed", ev.Elapsed().String(), ) } } @@ -303,8 +300,6 @@ func (s *Plugin) ServeHTTP(w http.ResponseWriter, r *http.Request) { // Workers returns associated pool workers func (s *Plugin) Workers() []worker.BaseProcess { - s.Lock() - defer s.Unlock() return s.pool.Workers() } @@ -365,17 +360,17 @@ func (s *Plugin) AddMiddleware(name endure.Named, m Middleware) { } // Status return status of the particular plugin -func (s *Plugin) Status() checker.Status { +func (s *Plugin) Status() status.Status { workers := s.Workers() for i := 0; i < len(workers); i++ { if workers[i].State().IsActive() { - return checker.Status{ + return status.Status{ Code: http.StatusOK, } } } // if there are no workers, threat this as error - return checker.Status{ + return status.Status{ Code: http.StatusInternalServerError, } } diff --git a/plugins/static/plugin.go b/plugins/static/plugin.go index 1687cf11c..5f1087012 100644 --- a/plugins/static/plugin.go +++ b/plugins/static/plugin.go @@ -47,7 +47,7 @@ func (s *Plugin) Init(cfg config.Configurer, log logger.Logger) error { err = s.cfg.Valid() if err != nil { - return errors.E(op, errors.Disabled, err) + return errors.E(op, err) } return nil @@ -88,7 +88,6 @@ func (s *Plugin) handleStatic(w http.ResponseWriter, r *http.Request) bool { f, err := s.root.Open(fPath) if err != nil { - s.log.Error("file open error", "error", err) if s.cfg.AlwaysServe(fPath) { w.WriteHeader(404) return true diff --git a/plugins/checker/config.go b/plugins/status/config.go similarity index 71% rename from plugins/checker/config.go rename to plugins/status/config.go index 5f952592e..23a6ede2d 100644 --- a/plugins/checker/config.go +++ b/plugins/status/config.go @@ -1,4 +1,4 @@ -package checker +package status type Config struct { Address string diff --git a/plugins/checker/interface.go b/plugins/status/interface.go similarity index 92% rename from plugins/checker/interface.go rename to plugins/status/interface.go index dd9dcada6..0a92bc521 100644 --- a/plugins/checker/interface.go +++ b/plugins/status/interface.go @@ -1,4 +1,4 @@ -package checker +package status // Status consists of status code from the service type Status struct { diff --git a/plugins/checker/plugin.go b/plugins/status/plugin.go similarity index 90% rename from plugins/checker/plugin.go rename to plugins/status/plugin.go index 59a376135..6fbe67cf5 100644 --- a/plugins/checker/plugin.go +++ b/plugins/status/plugin.go @@ -1,4 +1,4 @@ -package checker +package status import ( "fmt" @@ -6,7 +6,6 @@ import ( "time" "github.com/gofiber/fiber/v2" - fiberLogger "github.com/gofiber/fiber/v2/middleware/logger" endure "github.com/spiral/endure/pkg/container" "github.com/spiral/errors" "github.com/spiral/roadrunner/v2/plugins/config" @@ -43,12 +42,12 @@ func (c *Plugin) Init(log logger.Logger, cfg config.Configurer) error { func (c *Plugin) Serve() chan error { errCh := make(chan error, 1) c.server = fiber.New(fiber.Config{ - ReadTimeout: time.Second * 5, - WriteTimeout: time.Second * 5, - IdleTimeout: time.Second * 5, + ReadTimeout: time.Second * 5, + WriteTimeout: time.Second * 5, + IdleTimeout: time.Second * 5, + DisableStartupMessage: true, }) - c.server.Group("/v1", c.healthHandler) - c.server.Use(fiberLogger.New()) + c.server.Use("/health", c.healthHandler) go func() { @@ -120,7 +119,7 @@ func (c *Plugin) healthHandler(ctx *fiber.Ctx) error { if len(plugins.Plugins) == 0 { ctx.Status(http.StatusOK) - _, _ = ctx.WriteString("No plugins provided in query. Query should be in form of: /v1/health?plugin=plugin1&plugin=plugin2 \n") + _, _ = ctx.WriteString("No plugins provided in query. Query should be in form of: health?plugin=plugin1&plugin=plugin2 \n") return nil } diff --git a/plugins/checker/rpc.go b/plugins/status/rpc.go similarity index 97% rename from plugins/checker/rpc.go rename to plugins/status/rpc.go index a965dcd46..396ff4512 100644 --- a/plugins/checker/rpc.go +++ b/plugins/status/rpc.go @@ -1,4 +1,4 @@ -package checker +package status import ( "github.com/spiral/errors" diff --git a/tests/plugins/checker/configs/.rr-checker-init.yaml b/tests/plugins/checker/configs/.rr-checker-init.yaml index 11804a216..dca86efe0 100755 --- a/tests/plugins/checker/configs/.rr-checker-init.yaml +++ b/tests/plugins/checker/configs/.rr-checker-init.yaml @@ -5,13 +5,12 @@ server: command: "php ../../http/client.php echo pipes" user: "" group: "" - env: - "RR_HTTP": "true" relay: "pipes" relay_timeout: "20s" status: address: "127.0.0.1:34333" + logs: mode: development level: error diff --git a/tests/plugins/checker/plugin_test.go b/tests/plugins/checker/plugin_test.go index 5e3911580..569e73a18 100644 --- a/tests/plugins/checker/plugin_test.go +++ b/tests/plugins/checker/plugin_test.go @@ -14,12 +14,12 @@ import ( endure "github.com/spiral/endure/pkg/container" goridgeRpc "github.com/spiral/goridge/v3/pkg/rpc" - "github.com/spiral/roadrunner/v2/plugins/checker" "github.com/spiral/roadrunner/v2/plugins/config" httpPlugin "github.com/spiral/roadrunner/v2/plugins/http" "github.com/spiral/roadrunner/v2/plugins/logger" rpcPlugin "github.com/spiral/roadrunner/v2/plugins/rpc" "github.com/spiral/roadrunner/v2/plugins/server" + "github.com/spiral/roadrunner/v2/plugins/status" "github.com/stretchr/testify/assert" ) @@ -37,7 +37,7 @@ func TestStatusHttp(t *testing.T) { &logger.ZapLogger{}, &server.Plugin{}, &httpPlugin.Plugin{}, - &checker.Plugin{}, + &status.Plugin{}, ) assert.NoError(t, err) @@ -95,7 +95,7 @@ const resp = `Service: http: Status: 200 Service: rpc not found` func checkHTTPStatus(t *testing.T) { - req, err := http.NewRequest("GET", "http://127.0.0.1:34333/v1/health?plugin=http&plugin=rpc", nil) + req, err := http.NewRequest("GET", "http://127.0.0.1:34333/health?plugin=http&plugin=rpc", nil) assert.NoError(t, err) r, err := http.DefaultClient.Do(req) @@ -124,7 +124,7 @@ func TestStatusRPC(t *testing.T) { &logger.ZapLogger{}, &server.Plugin{}, &httpPlugin.Plugin{}, - &checker.Plugin{}, + &status.Plugin{}, ) assert.NoError(t, err) @@ -182,7 +182,7 @@ func checkRPCStatus(t *testing.T) { assert.NoError(t, err) client := rpc.NewClientWithCodec(goridgeRpc.NewClientCodec(conn)) - st := &checker.Status{} + st := &status.Status{} err = client.Call("status.Status", "http", &st) assert.NoError(t, err) diff --git a/tests/plugins/gzip/plugin_test.go b/tests/plugins/gzip/plugin_test.go index 3e3db0f85..9a9c760b9 100644 --- a/tests/plugins/gzip/plugin_test.go +++ b/tests/plugins/gzip/plugin_test.go @@ -34,7 +34,7 @@ func TestGzipPlugin(t *testing.T) { &logger.ZapLogger{}, &server.Plugin{}, &httpPlugin.Plugin{}, - &gzip.Gzip{}, + &gzip.Plugin{}, ) assert.NoError(t, err) @@ -126,7 +126,7 @@ func TestMiddlewareNotExist(t *testing.T) { mockLogger, &server.Plugin{}, &httpPlugin.Plugin{}, - &gzip.Gzip{}, + &gzip.Plugin{}, ) assert.NoError(t, err) diff --git a/tests/plugins/http/http_plugin_test.go b/tests/plugins/http/http_plugin_test.go index 0b6fb77a9..c243bf898 100644 --- a/tests/plugins/http/http_plugin_test.go +++ b/tests/plugins/http/http_plugin_test.go @@ -1028,7 +1028,7 @@ logs: mockLogger.EXPECT().Debug(gomock.Any()).AnyTimes() mockLogger.EXPECT().Debug("worker destructed", "pid", gomock.Any()).MinTimes(1) mockLogger.EXPECT().Debug("worker constructed", "pid", gomock.Any()).MinTimes(1) - mockLogger.EXPECT().Debug("", "remote", gomock.Any(), "ts", gomock.Any(), "resp.status", gomock.Any(), "method", gomock.Any(), "uri", gomock.Any()).MinTimes(1) + mockLogger.EXPECT().Debug("201 GET http://localhost:34999/?hello=world", "remote", "127.0.0.1", "elapsed", gomock.Any()).MinTimes(1) mockLogger.EXPECT().Debug("WORLD", "pid", gomock.Any()).MinTimes(1) mockLogger.EXPECT().Debug("worker event received", "event", events.EventWorkerLog, "worker state", gomock.Any()).MinTimes(1) mockLogger.EXPECT().Error(gomock.Any(), gomock.Any(), gomock.Any()).AnyTimes() // placeholder for the workerlogerror diff --git a/tests/plugins/static/static_plugin_test.go b/tests/plugins/static/static_plugin_test.go index d43ef765b..38562537a 100644 --- a/tests/plugins/static/static_plugin_test.go +++ b/tests/plugins/static/static_plugin_test.go @@ -38,7 +38,7 @@ func TestStaticPlugin(t *testing.T) { &logger.ZapLogger{}, &server.Plugin{}, &httpPlugin.Plugin{}, - &gzip.Gzip{}, + &gzip.Plugin{}, &static.Plugin{}, ) assert.NoError(t, err) @@ -138,7 +138,7 @@ func serveStaticSample(t *testing.T) { _ = r.Body.Close() } -func TestStaticDisabled(t *testing.T) { +func TestStaticDisabled_Error(t *testing.T) { cont, err := endure.NewContainer(nil, endure.SetLogLevel(endure.ErrorLevel)) assert.NoError(t, err) @@ -152,66 +152,11 @@ func TestStaticDisabled(t *testing.T) { &logger.ZapLogger{}, &server.Plugin{}, &httpPlugin.Plugin{}, - &gzip.Gzip{}, + &gzip.Plugin{}, &static.Plugin{}, ) assert.NoError(t, err) - - err = cont.Init() - if err != nil { - t.Fatal(err) - } - - ch, err := cont.Serve() - assert.NoError(t, err) - - sig := make(chan os.Signal, 1) - signal.Notify(sig, os.Interrupt, syscall.SIGINT, syscall.SIGTERM) - - wg := &sync.WaitGroup{} - wg.Add(1) - - stopCh := make(chan struct{}, 1) - - go func() { - defer wg.Done() - for { - select { - case e := <-ch: - assert.Fail(t, "error", e.Error.Error()) - err = cont.Stop() - if err != nil { - assert.FailNow(t, "error", err.Error()) - } - case <-sig: - err = cont.Stop() - if err != nil { - assert.FailNow(t, "error", err.Error()) - } - return - case <-stopCh: - // timeout - err = cont.Stop() - if err != nil { - assert.FailNow(t, "error", err.Error()) - } - return - } - } - }() - - time.Sleep(time.Second) - t.Run("StaticDisabled", staticDisabled) - - stopCh <- struct{}{} - wg.Wait() -} - -func staticDisabled(t *testing.T) { - _, r, err := get("http://localhost:21234/sample.txt") //nolint:bodyclose - assert.NoError(t, err) - assert.NotNil(t, r) - assert.Empty(t, r.Header.Get("X-Powered-By")) + assert.Error(t, cont.Init()) } func TestStaticFilesDisabled(t *testing.T) { @@ -228,7 +173,7 @@ func TestStaticFilesDisabled(t *testing.T) { &logger.ZapLogger{}, &server.Plugin{}, &httpPlugin.Plugin{}, - &gzip.Gzip{}, + &gzip.Plugin{}, &static.Plugin{}, ) assert.NoError(t, err) @@ -306,7 +251,9 @@ func TestStaticFilesForbid(t *testing.T) { mockLogger.EXPECT().Debug("worker destructed", "pid", gomock.Any()).AnyTimes() mockLogger.EXPECT().Debug("worker constructed", "pid", gomock.Any()).AnyTimes() - mockLogger.EXPECT().Debug("", "remote", gomock.Any(), "ts", gomock.Any(), "resp.status", gomock.Any(), "method", gomock.Any(), "uri", gomock.Any()).AnyTimes() + mockLogger.EXPECT().Debug("201 GET http://localhost:34653/http?hello=world", "remote", "127.0.0.1", "elapsed", gomock.Any()).MinTimes(1) + mockLogger.EXPECT().Debug("201 GET http://localhost:34653/client.XXX?hello=world", "remote", "127.0.0.1", "elapsed", gomock.Any()).MinTimes(1) + mockLogger.EXPECT().Debug("201 GET http://localhost:34653/client.php?hello=world", "remote", "127.0.0.1", "elapsed", gomock.Any()).MinTimes(1) mockLogger.EXPECT().Error("file open error", "error", gomock.Any()).AnyTimes() mockLogger.EXPECT().Error(gomock.Any(), gomock.Any(), gomock.Any()).AnyTimes() // placeholder for the workerlogerror @@ -315,7 +262,7 @@ func TestStaticFilesForbid(t *testing.T) { mockLogger, &server.Plugin{}, &httpPlugin.Plugin{}, - &gzip.Gzip{}, + &gzip.Plugin{}, &static.Plugin{}, ) assert.NoError(t, err)