From a3438297e68284da3fa3980d6a15ab555790066e Mon Sep 17 00:00:00 2001 From: Jason Kulatunga Date: Sat, 3 Oct 2020 10:40:27 -0600 Subject: [PATCH] removeedd waitgroup, using sync/errgroup instead (to pass error messages back). returning errors in test notifications endpoint payload. Adding failure tests for webhooks, scripts & shoutrr. --- go.mod | 1 + go.sum | 1 + webapp/backend/pkg/notify/notify.go | 81 +++++++++---------- .../pkg/web/handler/send_test_notification.go | 1 + webapp/backend/pkg/web/server_test.go | 72 +++++++++++++++++ 5 files changed, 112 insertions(+), 44 deletions(-) diff --git a/go.mod b/go.mod index 4b9bbb7..c1027e5 100644 --- a/go.mod +++ b/go.mod @@ -18,5 +18,6 @@ require ( github.com/stretchr/testify v1.5.1 github.com/urfave/cli/v2 v2.2.0 golang.org/x/crypto v0.0.0-20200323165209-0ec3e9974c59 // indirect + golang.org/x/sync v0.0.0-20190423024810-112230192c58 gopkg.in/yaml.v2 v2.3.0 // indirect ) diff --git a/go.sum b/go.sum index e32dd06..df4c220 100644 --- a/go.sum +++ b/go.sum @@ -347,6 +347,7 @@ golang.org/x/sync v0.0.0-20180314180146-1d60e4601c6f/go.mod h1:RxMgew5VJxzue5/jJ golang.org/x/sync v0.0.0-20181108010431-42b317875d0f/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= golang.org/x/sync v0.0.0-20181221193216-37e7f081c4d4/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= golang.org/x/sync v0.0.0-20190227155943-e225da77a7e6/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= +golang.org/x/sync v0.0.0-20190423024810-112230192c58 h1:8gQV6CLnAEikrhgkHFbMAEhagSSnXWGV915qUMm9mrU= golang.org/x/sync v0.0.0-20190423024810-112230192c58/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= golang.org/x/sys v0.0.0-20180823144017-11551d06cbcc/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY= golang.org/x/sys v0.0.0-20180830151530-49385e6e1522/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY= diff --git a/webapp/backend/pkg/notify/notify.go b/webapp/backend/pkg/notify/notify.go index e70334d..dffece3 100644 --- a/webapp/backend/pkg/notify/notify.go +++ b/webapp/backend/pkg/notify/notify.go @@ -3,17 +3,18 @@ package notify import ( "bytes" "encoding/json" + "errors" "fmt" "github.com/analogj/go-util/utils" "github.com/analogj/scrutiny/webapp/backend/pkg/config" "github.com/containrrr/shoutrrr" shoutrrrTypes "github.com/containrrr/shoutrrr/pkg/types" "github.com/sirupsen/logrus" + "golang.org/x/sync/errgroup" "net/http" "net/url" "os" "strings" - "sync" "time" ) @@ -102,62 +103,65 @@ func (n *Notify) Send() error { n.Logger.Debugf("Configured shoutrrr: %v", notifyShoutrrr) //run all scripts, webhooks and shoutrr commands in parallel - var wg sync.WaitGroup + //var wg sync.WaitGroup + var eg errgroup.Group for _, notifyWebhook := range notifyWebhooks { // execute collection in parallel go-routines - wg.Add(1) - go n.SendWebhookNotification(&wg, notifyWebhook) + eg.Go(func() error { return n.SendWebhookNotification(notifyWebhook) }) } for _, notifyScript := range notifyScripts { // execute collection in parallel go-routines - wg.Add(1) - go n.SendScriptNotification(&wg, notifyScript) + eg.Go(func() error { return n.SendScriptNotification(notifyScript) }) } for _, shoutrrrUrl := range notifyShoutrrr { - wg.Add(1) - go n.SendShoutrrrNotification(&wg, shoutrrrUrl) + eg.Go(func() error { return n.SendShoutrrrNotification(shoutrrrUrl) }) } //and wait for completion, error or timeout. n.Logger.Debugf("Main: waiting for notifications to complete.") - //wg.Wait() - if waitTimeout(&wg, time.Minute) { //wait for 1 minute - fmt.Println("Timed out while sending notifications") + + if err := eg.Wait(); err == nil { + n.Logger.Info("Successfully sent notifications. Check logs for more information.") + return nil } else { - fmt.Println("Sent notifications. Check logs for more information.") - } - return nil + n.Logger.Error("One or more notifications failed to send successfully. See logs for more information.") + return err + } + ////wg.Wait() + //if waitTimeout(&wg, time.Minute) { //wait for 1 minute + // fmt.Println("Timed out while sending notifications") + //} else { + //} + //return nil } -func (n *Notify) SendWebhookNotification(wg *sync.WaitGroup, webhookUrl string) { - defer wg.Done() +func (n *Notify) SendWebhookNotification(webhookUrl string) error { n.Logger.Infof("Sending Webhook to %s", webhookUrl) requestBody, err := json.Marshal(n.Payload) if err != nil { n.Logger.Errorf("An error occurred while sending Webhook to %s: %v", webhookUrl, err) - return + return err } resp, err := http.Post(webhookUrl, "application/json", bytes.NewBuffer(requestBody)) if err != nil { n.Logger.Errorf("An error occurred while sending Webhook to %s: %v", webhookUrl, err) - return + return err } defer resp.Body.Close() //we don't care about resp body content, but maybe we should log it? + return nil } -func (n *Notify) SendScriptNotification(wg *sync.WaitGroup, scriptUrl string) { - defer wg.Done() - +func (n *Notify) SendScriptNotification(scriptUrl string) error { //check if the script exists. scriptPath := strings.TrimPrefix(scriptUrl, "script://") n.Logger.Infof("Executing Script %s", scriptPath) if !utils.FileExists(scriptPath) { n.Logger.Errorf("Script does not exist: %s", scriptPath) - return + return errors.New(fmt.Sprintf("custom script path does not exist: %s", scriptPath)) } copyEnv := os.Environ() @@ -171,20 +175,20 @@ func (n *Notify) SendScriptNotification(wg *sync.WaitGroup, scriptUrl string) { err := utils.CmdExec(scriptPath, []string{}, "", copyEnv, "") if err != nil { n.Logger.Errorf("An error occurred while executing script %s: %v", scriptPath, err) + return err } - return + return nil } -func (n *Notify) SendShoutrrrNotification(wg *sync.WaitGroup, shoutrrrUrl string) { +func (n *Notify) SendShoutrrrNotification(shoutrrrUrl string) error { fmt.Printf("Sending Notifications to %v", shoutrrrUrl) n.Logger.Infof("Sending notifications to %v", shoutrrrUrl) - defer wg.Done() sender, err := shoutrrr.CreateSender(shoutrrrUrl) if err != nil { n.Logger.Errorf("An error occurred while sending notifications %v: %v", shoutrrrUrl, err) - return + return err } //sender.SetLogger(n.Logger.) @@ -193,15 +197,21 @@ func (n *Notify) SendShoutrrrNotification(wg *sync.WaitGroup, shoutrrrUrl string if err != nil { n.Logger.Errorf("An error occurred occurred while generating notification payload for %s:\n %v", serviceName, shoutrrrUrl, err) + return err } errs := sender.Send(n.Payload.Message, params) if len(errs) > 0 { - n.Logger.Errorf("One or more errors occurred occurred while sending notifications for %s:\n %v", shoutrrrUrl, errs) + n.Logger.Errorf("One or more errors occurred occurred while sending notifications for %s:", shoutrrrUrl) + var errstrings []string + for _, err := range errs { n.Logger.Error(err) + errstrings = append(errstrings, err.Error()) } + return errors.New(strings.Join(errstrings, "\n")) } + return nil } func (n *Notify) GenShoutrrrNotificationParams(shoutrrrUrl string) (string, *shoutrrrTypes.Params, error) { @@ -243,20 +253,3 @@ func (n *Notify) GenShoutrrrNotificationParams(shoutrrrUrl string) (string, *sho return serviceName, params, nil } - -//utility functions -// waitTimeout waits for the waitgroup for the specified max timeout. -// Returns true if waiting timed out. -func waitTimeout(wg *sync.WaitGroup, timeout time.Duration) bool { - c := make(chan struct{}) - go func() { - defer close(c) - wg.Wait() - }() - select { - case <-c: - return false // completed normally - case <-time.After(timeout): - return true // timed out - } -} diff --git a/webapp/backend/pkg/web/handler/send_test_notification.go b/webapp/backend/pkg/web/handler/send_test_notification.go index 153c013..52ba8f5 100644 --- a/webapp/backend/pkg/web/handler/send_test_notification.go +++ b/webapp/backend/pkg/web/handler/send_test_notification.go @@ -30,6 +30,7 @@ func SendTestNotification(c *gin.Context) { logger.Errorln("An error occurred while sending test notification", err) c.JSON(http.StatusInternalServerError, gin.H{ "success": false, + "errors": []string{err.Error()}, }) } else { c.JSON(http.StatusOK, dbModels.DeviceWrapper{ diff --git a/webapp/backend/pkg/web/server_test.go b/webapp/backend/pkg/web/server_test.go index 9892424..e121a2d 100644 --- a/webapp/backend/pkg/web/server_test.go +++ b/webapp/backend/pkg/web/server_test.go @@ -188,6 +188,78 @@ func TestSendTestNotificationRoute(t *testing.T) { require.Equal(t, 200, wr.Code) } +func TestSendTestNotificationRoute_WebhookFailure(t *testing.T) { + //setup + parentPath, _ := ioutil.TempDir("", "") + defer os.RemoveAll(parentPath) + mockCtrl := gomock.NewController(t) + defer mockCtrl.Finish() + fakeConfig := mock_config.NewMockInterface(mockCtrl) + fakeConfig.EXPECT().GetString("web.database.location").AnyTimes().Return(path.Join(parentPath, "scrutiny_test.db")) + fakeConfig.EXPECT().GetString("web.src.frontend.path").AnyTimes().Return(parentPath) + fakeConfig.EXPECT().GetStringSlice("notify.urls").AnyTimes().Return([]string{"https://unroutable.domain.example.asdfghj"}) + ae := web.AppEngine{ + Config: fakeConfig, + } + router := ae.Setup(logrus.New()) + + //test + wr := httptest.NewRecorder() + req, _ := http.NewRequest("POST", "/api/health/notify", strings.NewReader("{}")) + router.ServeHTTP(wr, req) + + //assert + require.Equal(t, 500, wr.Code) +} + +func TestSendTestNotificationRoute_ScriptFailure(t *testing.T) { + //setup + parentPath, _ := ioutil.TempDir("", "") + defer os.RemoveAll(parentPath) + mockCtrl := gomock.NewController(t) + defer mockCtrl.Finish() + fakeConfig := mock_config.NewMockInterface(mockCtrl) + fakeConfig.EXPECT().GetString("web.database.location").AnyTimes().Return(path.Join(parentPath, "scrutiny_test.db")) + fakeConfig.EXPECT().GetString("web.src.frontend.path").AnyTimes().Return(parentPath) + fakeConfig.EXPECT().GetStringSlice("notify.urls").AnyTimes().Return([]string{"script:///missing/path/on/disk"}) + ae := web.AppEngine{ + Config: fakeConfig, + } + router := ae.Setup(logrus.New()) + + //test + wr := httptest.NewRecorder() + req, _ := http.NewRequest("POST", "/api/health/notify", strings.NewReader("{}")) + router.ServeHTTP(wr, req) + + //assert + require.Equal(t, 500, wr.Code) +} + +func TestSendTestNotificationRoute_ShoutrrrFailure(t *testing.T) { + //setup + parentPath, _ := ioutil.TempDir("", "") + defer os.RemoveAll(parentPath) + mockCtrl := gomock.NewController(t) + defer mockCtrl.Finish() + fakeConfig := mock_config.NewMockInterface(mockCtrl) + fakeConfig.EXPECT().GetString("web.database.location").AnyTimes().Return(path.Join(parentPath, "scrutiny_test.db")) + fakeConfig.EXPECT().GetString("web.src.frontend.path").AnyTimes().Return(parentPath) + fakeConfig.EXPECT().GetStringSlice("notify.urls").AnyTimes().Return([]string{"discord://invalidtoken@channel"}) + ae := web.AppEngine{ + Config: fakeConfig, + } + router := ae.Setup(logrus.New()) + + //test + wr := httptest.NewRecorder() + req, _ := http.NewRequest("POST", "/api/health/notify", strings.NewReader("{}")) + router.ServeHTTP(wr, req) + + //assert + require.Equal(t, 500, wr.Code) +} + func TestGetDevicesSummaryRoute_Nvme(t *testing.T) { //setup parentPath, _ := ioutil.TempDir("", "")