From d3ac9a5185ea10142662335c82d06aecceab43e8 Mon Sep 17 00:00:00 2001 From: Ravi Hari Date: Wed, 7 Sep 2022 20:17:34 +0530 Subject: [PATCH 1/4] fix: enable notifications without when condition Signed-off-by: Ravi Hari --- utils/record/record.go | 32 ++++++++++++++++++++++---------- utils/record/record_test.go | 27 +++++++++++++++++++++++++++ 2 files changed, 49 insertions(+), 10 deletions(-) diff --git a/utils/record/record.go b/utils/record/record.go index 3ad05671b2..c8f294b582 100644 --- a/utils/record/record.go +++ b/utils/record/record.go @@ -278,16 +278,28 @@ func (e *EventRecorderAdapter) sendNotifications(object runtime.Object, opts Eve return err } - res, err := notificationsAPI.RunTrigger(trigger, objMap) - if err != nil { - log.Errorf("Failed to execute condition of trigger %s: %v", trigger, err) - return err - } - log.Infof("Trigger %s result: %v", trigger, res) - - for _, dest := range destinations { - for _, c := range res { - if c.Triggered == true { + for _, ta := range triggerActions { + if ta.When != "" { + res, err := notificationsAPI.RunTrigger(trigger, objMap) + if err != nil { + log.Errorf("Failed to execute condition of trigger %s: %v", trigger, err) + return err + } + log.Infof("Trigger %s result: %v", trigger, res) + + for _, dest := range destinations { + for _, c := range res { + if c.Triggered == true { + err = notificationsAPI.Send(objMap, triggerActions[0].Send, dest) + if err != nil { + log.Errorf("notification error: %s", err.Error()) + return err + } + } + } + } + } else { + for _, dest := range destinations { err = notificationsAPI.Send(objMap, triggerActions[0].Send, dest) if err != nil { log.Errorf("notification error: %s", err.Error()) diff --git a/utils/record/record_test.go b/utils/record/record_test.go index 931ce42c9b..77dc1aa742 100644 --- a/utils/record/record_test.go +++ b/utils/record/record_test.go @@ -112,6 +112,33 @@ func TestSendNotifications(t *testing.T) { assert.NoError(t, err) } +func TestSendNotificationsWhenCondition(t *testing.T) { + r := v1alpha1.Rollout{ + ObjectMeta: metav1.ObjectMeta{ + Name: "guestbook", + Namespace: "default", + Annotations: map[string]string{"notifications.argoproj.io/subscribe.on-foo-reason.console": "console"}, + }, + } + mockCtrl := gomock.NewController(t) + mockAPI := mocks.NewMockAPI(mockCtrl) + cr := []triggers.ConditionResult{{ + Key: "", + Triggered: true, + Templates: []string{"my-template"}, + }} + mockAPI.EXPECT().RunTrigger(gomock.Any(), gomock.Any()).Return(cr, nil).AnyTimes() + mockAPI.EXPECT().Send(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil).AnyTimes() + mockAPI.EXPECT().GetConfig().Return(api.Config{ + Triggers: map[string][]triggers.Condition{"on-foo-reason": {triggers.Condition{When: "rollout.spec.template.spec.containers[0].image == test:blue", Send: []string{"my-template"}}}}}).AnyTimes() + apiFactory := &mocks.FakeFactory{Api: mockAPI} + rec := NewFakeEventRecorder() + rec.EventRecorderAdapter.apiFactory = apiFactory + //ch := make(chan prometheus.HistogramVec, 1) + err := rec.sendNotifications(&r, EventOptions{EventReason: "FooReason"}) + assert.NoError(t, err) +} + func TestNotificationFailedCounter(t *testing.T) { r := v1alpha1.Rollout{ ObjectMeta: metav1.ObjectMeta{ From d46a1d13c0f5abaa06783198de94564fee8204e8 Mon Sep 17 00:00:00 2001 From: Ravi Hari Date: Wed, 7 Sep 2022 22:22:42 +0530 Subject: [PATCH 2/4] fix: use trigger action item from the list Signed-off-by: Ravi Hari --- utils/record/record.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/utils/record/record.go b/utils/record/record.go index c8f294b582..3368f2e1b6 100644 --- a/utils/record/record.go +++ b/utils/record/record.go @@ -290,7 +290,7 @@ func (e *EventRecorderAdapter) sendNotifications(object runtime.Object, opts Eve for _, dest := range destinations { for _, c := range res { if c.Triggered == true { - err = notificationsAPI.Send(objMap, triggerActions[0].Send, dest) + err = notificationsAPI.Send(objMap, ta.Send, dest) if err != nil { log.Errorf("notification error: %s", err.Error()) return err @@ -300,7 +300,7 @@ func (e *EventRecorderAdapter) sendNotifications(object runtime.Object, opts Eve } } else { for _, dest := range destinations { - err = notificationsAPI.Send(objMap, triggerActions[0].Send, dest) + err = notificationsAPI.Send(objMap, ta.Send, dest) if err != nil { log.Errorf("notification error: %s", err.Error()) return err From 39d24c08b96ee890a4acb4a5351e9b365ad0b2e9 Mon Sep 17 00:00:00 2001 From: Ravi Hari Date: Wed, 14 Sep 2022 23:37:19 +0530 Subject: [PATCH 3/4] fix: add emptycondition logic to make notifications work with/without conditions Signed-off-by: Ravi Hari --- utils/record/record.go | 61 +++++++++++++++++++------------------ utils/record/record_test.go | 16 +++++++--- 2 files changed, 43 insertions(+), 34 deletions(-) diff --git a/utils/record/record.go b/utils/record/record.go index 3368f2e1b6..f255b02c24 100644 --- a/utils/record/record.go +++ b/utils/record/record.go @@ -2,6 +2,8 @@ package record import ( "context" + "crypto/sha1" + "encoding/base64" "encoding/json" "regexp" "strings" @@ -266,41 +268,32 @@ func (e *EventRecorderAdapter) sendNotifications(object runtime.Object, opts Eve return nil } - // Creates config for notifications for built-in triggers - triggerActions, ok := cfg.Triggers[trigger] - if !ok { - logCtx.Debugf("No configured template for trigger: %s", trigger) - return nil - } - objMap, err := toObjectMap(object) if err != nil { return err } - for _, ta := range triggerActions { - if ta.When != "" { - res, err := notificationsAPI.RunTrigger(trigger, objMap) - if err != nil { - log.Errorf("Failed to execute condition of trigger %s: %v", trigger, err) - return err - } - log.Infof("Trigger %s result: %v", trigger, res) - - for _, dest := range destinations { - for _, c := range res { - if c.Triggered == true { - err = notificationsAPI.Send(objMap, ta.Send, dest) - if err != nil { - log.Errorf("notification error: %s", err.Error()) - return err - } - } + emptyCondition := hash("") + + for _, destination := range destinations { + res, err := notificationsAPI.RunTrigger(trigger, objMap) + if err != nil { + log.Errorf("Failed to execute condition of trigger %s: %v", trigger, err) + return err + } + log.Infof("Trigger %s result: %v", trigger, res) + + for _, c := range res { + log.Infof("Res When Condition hash: %s, Templates: %s", c.Key, c.Templates) + s := strings.Split(c.Key, ".")[1] + if s != emptyCondition && c.Triggered == true { + err = notificationsAPI.Send(objMap, c.Templates, destination) + if err != nil { + log.Errorf("notification error: %s", err.Error()) + return err } - } - } else { - for _, dest := range destinations { - err = notificationsAPI.Send(objMap, ta.Send, dest) + } else if s == emptyCondition { + err = notificationsAPI.Send(objMap, c.Templates, destination) if err != nil { log.Errorf("notification error: %s", err.Error()) return err @@ -308,9 +301,19 @@ func (e *EventRecorderAdapter) sendNotifications(object runtime.Object, opts Eve } } } + return nil } +// This function is copied over from notification engine to make sure we honour emptyCondition +// emptyConditions today are not handled well in notification engine. +// TODO: update notification engine to handle emptyConditions and remove this function and its usage +func hash(input string) string { + h := sha1.New() + _, _ = h.Write([]byte(input)) + return base64.RawURLEncoding.EncodeToString(h.Sum(nil)) +} + // toObjectMap converts an object to a map for the purposes of sending to the notification engine func toObjectMap(object interface{}) (map[string]interface{}, error) { objBytes, err := json.Marshal(object) diff --git a/utils/record/record_test.go b/utils/record/record_test.go index 77dc1aa742..14f678c82a 100644 --- a/utils/record/record_test.go +++ b/utils/record/record_test.go @@ -96,7 +96,7 @@ func TestSendNotifications(t *testing.T) { mockCtrl := gomock.NewController(t) mockAPI := mocks.NewMockAPI(mockCtrl) cr := []triggers.ConditionResult{{ - Key: "", + Key: "1."+hash(""), Triggered: true, Templates: []string{"my-template"}, }} @@ -123,7 +123,7 @@ func TestSendNotificationsWhenCondition(t *testing.T) { mockCtrl := gomock.NewController(t) mockAPI := mocks.NewMockAPI(mockCtrl) cr := []triggers.ConditionResult{{ - Key: "", + Key: "1."+hash(""), Triggered: true, Templates: []string{"my-template"}, }} @@ -228,7 +228,7 @@ func TestSendNotificationsFails(t *testing.T) { mockCtrl := gomock.NewController(t) mockAPI := mocks.NewMockAPI(mockCtrl) cr := []triggers.ConditionResult{{ - Key: "", + Key: "1."+hash(""), Triggered: true, Templates: []string{"my-template"}, }} @@ -268,7 +268,7 @@ func TestSendNotificationsFailsWithRunTriggerError(t *testing.T) { mockCtrl := gomock.NewController(t) mockAPI := mocks.NewMockAPI(mockCtrl) cr := []triggers.ConditionResult{{ - Key: "", + Key: "1."+hash(""), Triggered: true, Templates: []string{"my-template"}, }} @@ -306,6 +306,12 @@ func TestSendNotificationsNoTrigger(t *testing.T) { mockCtrl := gomock.NewController(t) mockAPI := mocks.NewMockAPI(mockCtrl) + cr := []triggers.ConditionResult{{ + Key: "1."+hash(""), + Triggered: false, + Templates: []string{"my-template"}, + }} + mockAPI.EXPECT().RunTrigger(gomock.Any(), gomock.Any()).Return(cr, errors.New("trigger 'on-missing-reason' is not configured")).AnyTimes() mockAPI.EXPECT().GetConfig().Return(api.Config{ Triggers: map[string][]triggers.Condition{"on-foo-reason": {triggers.Condition{Send: []string{"my-template"}}}}}).AnyTimes() mockAPI.EXPECT().Send(gomock.Any(), gomock.Any(), gomock.Any()).Return(fmt.Errorf("failed to send")).Times(0) @@ -314,7 +320,7 @@ func TestSendNotificationsNoTrigger(t *testing.T) { rec.EventRecorderAdapter.apiFactory = apiFactory err := rec.sendNotifications(&r, EventOptions{EventReason: "MissingReason"}) - assert.NoError(t, err) + assert.Error(t, err) } func TestNewAPIFactorySettings(t *testing.T) { From 484664c1910e22e69b150909514501a47d7eb56c Mon Sep 17 00:00:00 2001 From: Ravi Hari Date: Wed, 14 Sep 2022 23:41:02 +0530 Subject: [PATCH 4/4] fix: linting Signed-off-by: Ravi Hari --- utils/record/record_test.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/utils/record/record_test.go b/utils/record/record_test.go index 14f678c82a..c99f55985b 100644 --- a/utils/record/record_test.go +++ b/utils/record/record_test.go @@ -96,7 +96,7 @@ func TestSendNotifications(t *testing.T) { mockCtrl := gomock.NewController(t) mockAPI := mocks.NewMockAPI(mockCtrl) cr := []triggers.ConditionResult{{ - Key: "1."+hash(""), + Key: "1." + hash(""), Triggered: true, Templates: []string{"my-template"}, }} @@ -123,7 +123,7 @@ func TestSendNotificationsWhenCondition(t *testing.T) { mockCtrl := gomock.NewController(t) mockAPI := mocks.NewMockAPI(mockCtrl) cr := []triggers.ConditionResult{{ - Key: "1."+hash(""), + Key: "1." + hash(""), Triggered: true, Templates: []string{"my-template"}, }} @@ -228,7 +228,7 @@ func TestSendNotificationsFails(t *testing.T) { mockCtrl := gomock.NewController(t) mockAPI := mocks.NewMockAPI(mockCtrl) cr := []triggers.ConditionResult{{ - Key: "1."+hash(""), + Key: "1." + hash(""), Triggered: true, Templates: []string{"my-template"}, }} @@ -268,7 +268,7 @@ func TestSendNotificationsFailsWithRunTriggerError(t *testing.T) { mockCtrl := gomock.NewController(t) mockAPI := mocks.NewMockAPI(mockCtrl) cr := []triggers.ConditionResult{{ - Key: "1."+hash(""), + Key: "1." + hash(""), Triggered: true, Templates: []string{"my-template"}, }} @@ -307,7 +307,7 @@ func TestSendNotificationsNoTrigger(t *testing.T) { mockCtrl := gomock.NewController(t) mockAPI := mocks.NewMockAPI(mockCtrl) cr := []triggers.ConditionResult{{ - Key: "1."+hash(""), + Key: "1." + hash(""), Triggered: false, Templates: []string{"my-template"}, }}