From a2ccbbf0dcf0d6cd646b297b3734e31e0b4579f8 Mon Sep 17 00:00:00 2001 From: May Zhang Date: Thu, 27 Jul 2023 07:10:22 -0700 Subject: [PATCH 1/7] 1st draft Signed-off-by: May Zhang --- pkg/controller/controller.go | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/pkg/controller/controller.go b/pkg/controller/controller.go index a492526f..e9c25d9b 100644 --- a/pkg/controller/controller.go +++ b/pkg/controller/controller.go @@ -313,7 +313,19 @@ func (c *notificationController) processQueueItem() (processNext bool) { } c.processResource(api, resource, logEntry, &eventSequence) } else { - apisWithNamespace, err := c.apiFactory.GetAPIsFromNamespace(resource.GetNamespace()) + // this is for cluster support + annotations := resource.GetAnnotations() + notificationNS := annotations["notification-namespace"] + notificationCluster := annotations["notification-cluster"] + log.Infof("%s", notificationNS) + log.Infof("%s", notificationCluster) + + resourceNameSpace := resource.GetNamespace() + if notificationNS != "" && notificationCluster != "" { + resourceNameSpace = notificationCluster + "#" + notificationNS + } + + apisWithNamespace, err := c.apiFactory.GetAPIsFromNamespace(resourceNameSpace) if err != nil { logEntry.Errorf("Failed to get api with namespace: %v", err) eventSequence.addError(err) From bd94e33f57b6cc4ed34ffc443b62af301a40b322 Mon Sep 17 00:00:00 2001 From: May Zhang Date: Mon, 7 Aug 2023 10:48:26 -0700 Subject: [PATCH 2/7] to support factory based self service Signed-off-by: May Zhang --- pkg/api/factory.go | 5 ++++ pkg/controller/controller.go | 46 +++++++++++++++++++++++------------- 2 files changed, 34 insertions(+), 17 deletions(-) diff --git a/pkg/api/factory.go b/pkg/api/factory.go index e280b192..41b71fe9 100644 --- a/pkg/api/factory.go +++ b/pkg/api/factory.go @@ -33,6 +33,7 @@ type Settings struct { type Factory interface { GetAPI() (API, error) GetAPIsFromNamespace(namespace string) (map[string]API, error) + GetAPIsFromFactory(resource interface{}) (map[string]API, error) } type apiFactory struct { @@ -170,6 +171,10 @@ func (f *apiFactory) GetAPIsFromNamespace(namespace string) (map[string]API, err return apis, nil } +func (f *apiFactory) GetAPIsFromFactory(resource interface{}) (map[string]API, error) { + return nil, fmt.Errorf("not implemented") +} + func (f *apiFactory) getApiFromNamespace(namespace string) (API, error) { cm, secret, err := f.getConfigMapAndSecret(namespace) if err != nil { diff --git a/pkg/controller/controller.go b/pkg/controller/controller.go index e9c25d9b..281459bc 100644 --- a/pkg/controller/controller.go +++ b/pkg/controller/controller.go @@ -154,6 +154,18 @@ func NewControllerWithNamespaceSupport( return ctrl } +// NewControllerWithNamespaceSupport For self-service notification +func NewControllerWithFactorySupport( + client dynamic.NamespaceableResourceInterface, + informer cache.SharedIndexInformer, + apiFactory api.Factory, + opts ...Opts, +) *notificationController { + ctrl := NewController(client, informer, apiFactory, opts...) + ctrl.factorySupport = true + return ctrl +} + type notificationController struct { client dynamic.NamespaceableResourceInterface informer cache.SharedIndexInformer @@ -165,6 +177,7 @@ type notificationController struct { toUnstructured func(obj v1.Object) (*unstructured.Unstructured, error) eventCallback func(eventSequence NotificationEventSequence) namespaceSupport bool + factorySupport bool } func (c *notificationController) Run(threadiness int, stopCh <-chan struct{}) { @@ -304,28 +317,18 @@ func (c *notificationController) processQueueItem() (processNext bool) { } } - if !c.namespaceSupport { - api, err := c.apiFactory.GetAPI() + if c.factorySupport { + apisWithNamespace, err := c.apiFactory.GetAPIsFromFactory(resource) if err != nil { - logEntry.Errorf("Failed to get api: %v", err) + logEntry.Errorf("Failed to get api with namespace: %v", err) eventSequence.addError(err) - return } - c.processResource(api, resource, logEntry, &eventSequence) - } else { - // this is for cluster support - annotations := resource.GetAnnotations() - notificationNS := annotations["notification-namespace"] - notificationCluster := annotations["notification-cluster"] - log.Infof("%s", notificationNS) - log.Infof("%s", notificationCluster) - - resourceNameSpace := resource.GetNamespace() - if notificationNS != "" && notificationCluster != "" { - resourceNameSpace = notificationCluster + "#" + notificationNS + for _, api := range apisWithNamespace { + c.processResource(api, resource, logEntry, &eventSequence) } - apisWithNamespace, err := c.apiFactory.GetAPIsFromNamespace(resourceNameSpace) + } else if c.namespaceSupport { + apisWithNamespace, err := c.apiFactory.GetAPIsFromNamespace(resource.GetNamespace()) if err != nil { logEntry.Errorf("Failed to get api with namespace: %v", err) eventSequence.addError(err) @@ -333,7 +336,16 @@ func (c *notificationController) processQueueItem() (processNext bool) { for _, api := range apisWithNamespace { c.processResource(api, resource, logEntry, &eventSequence) } + } else { + api, err := c.apiFactory.GetAPI() + if err != nil { + logEntry.Errorf("Failed to get api: %v", err) + eventSequence.addError(err) + return + } + c.processResource(api, resource, logEntry, &eventSequence) } + logEntry.Info("Processing completed") return From d846c8005c4cbc7905436743e0bda6bf3995eb40 Mon Sep 17 00:00:00 2001 From: May Zhang Date: Mon, 7 Aug 2023 10:52:56 -0700 Subject: [PATCH 3/7] to support factory based self service Signed-off-by: May Zhang --- pkg/controller/controller.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/controller/controller.go b/pkg/controller/controller.go index 281459bc..f790910b 100644 --- a/pkg/controller/controller.go +++ b/pkg/controller/controller.go @@ -154,7 +154,7 @@ func NewControllerWithNamespaceSupport( return ctrl } -// NewControllerWithNamespaceSupport For self-service notification +// NewControllerWithNamespaceSupport For self-service notification getting configuration from apiFactory func NewControllerWithFactorySupport( client dynamic.NamespaceableResourceInterface, informer cache.SharedIndexInformer, From c937ac402fa97d69b170bb7c776ff98ae7b10137 Mon Sep 17 00:00:00 2001 From: May Zhang Date: Mon, 7 Aug 2023 13:32:15 -0700 Subject: [PATCH 4/7] fix test code Signed-off-by: May Zhang --- pkg/controller/controller.go | 2 +- pkg/mocks/factory.go | 9 ++++++++- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/pkg/controller/controller.go b/pkg/controller/controller.go index f790910b..878d69f8 100644 --- a/pkg/controller/controller.go +++ b/pkg/controller/controller.go @@ -320,7 +320,7 @@ func (c *notificationController) processQueueItem() (processNext bool) { if c.factorySupport { apisWithNamespace, err := c.apiFactory.GetAPIsFromFactory(resource) if err != nil { - logEntry.Errorf("Failed to get api with namespace: %v", err) + logEntry.Errorf("Failed to get api with key: %v", err) eventSequence.addError(err) } for _, api := range apisWithNamespace { diff --git a/pkg/mocks/factory.go b/pkg/mocks/factory.go index 7ebbb5a1..70b5797c 100644 --- a/pkg/mocks/factory.go +++ b/pkg/mocks/factory.go @@ -1,6 +1,9 @@ package mocks -import "github.com/argoproj/notifications-engine/pkg/api" +import ( + "fmt" + "github.com/argoproj/notifications-engine/pkg/api" +) type FakeFactory struct { Api api.API @@ -16,3 +19,7 @@ func (f *FakeFactory) GetAPIsFromNamespace(namespace string) (map[string]api.API apiMap[namespace] = f.Api return apiMap, f.Err } + +func (f *FakeFactory) GetAPIsFromFactory(resource interface{}) (map[string]api.API, error) { + return nil, fmt.Errorf("not implemented") +} From e18fc2af79b6598e31544e28931413d76fe8f575 Mon Sep 17 00:00:00 2001 From: May Zhang Date: Mon, 7 Aug 2023 13:54:32 -0700 Subject: [PATCH 5/7] --signoff Signed-off-by: May Zhang --- pkg/mocks/factory.go | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/pkg/mocks/factory.go b/pkg/mocks/factory.go index 70b5797c..9a95a853 100644 --- a/pkg/mocks/factory.go +++ b/pkg/mocks/factory.go @@ -1,9 +1,6 @@ package mocks -import ( - "fmt" - "github.com/argoproj/notifications-engine/pkg/api" -) +import "github.com/argoproj/notifications-engine/pkg/api" type FakeFactory struct { Api api.API @@ -21,5 +18,5 @@ func (f *FakeFactory) GetAPIsFromNamespace(namespace string) (map[string]api.API } func (f *FakeFactory) GetAPIsFromFactory(resource interface{}) (map[string]api.API, error) { - return nil, fmt.Errorf("not implemented") + return nil, f.Err } From 815b719a4336bcac004915911934e1688a5bc54a Mon Sep 17 00:00:00 2001 From: May Zhang Date: Mon, 7 Aug 2023 15:12:48 -0700 Subject: [PATCH 6/7] -s "adding test cases" Signed-off-by: May Zhang --- pkg/controller/controller_test.go | 27 ++++++++++++++++++++------- pkg/mocks/factory.go | 4 +++- 2 files changed, 23 insertions(+), 8 deletions(-) diff --git a/pkg/controller/controller_test.go b/pkg/controller/controller_test.go index 2f76a1c8..e43f7b31 100644 --- a/pkg/controller/controller_test.go +++ b/pkg/controller/controller_test.go @@ -65,7 +65,7 @@ func newResource(name string, modifiers ...func(app *unstructured.Unstructured)) return &app } -func newController(t *testing.T, ctx context.Context, client dynamic.Interface, opts ...Opts) (*notificationController, *mocks.MockAPI, error) { +func newController(t *testing.T, ctx context.Context, client dynamic.Interface, factorySupport bool, opts ...Opts) (*notificationController, *mocks.MockAPI, error) { mockCtrl := gomock.NewController(t) go func() { <-ctx.Done() @@ -90,7 +90,12 @@ func newController(t *testing.T, ctx context.Context, client dynamic.Interface, go informer.Run(ctx.Done()) - c := NewControllerWithNamespaceSupport(resourceClient, informer, &mocks.FakeFactory{Api: mockAPI}, opts...) + var c *notificationController + if factorySupport { + c = NewControllerWithFactorySupport(resourceClient, informer, &mocks.FakeFactory{Api: mockAPI}, opts...) + } else { + c = NewControllerWithNamespaceSupport(resourceClient, informer, &mocks.FakeFactory{Api: mockAPI}, opts...) + } if !cache.WaitForCacheSync(ctx.Done(), informer.HasSynced) { return nil, nil, errors.New("failed to sync informers") } @@ -105,7 +110,7 @@ func TestSendsNotificationIfTriggered(t *testing.T) { subscriptions.SubscribeAnnotationKey("my-trigger", "mock"): "recipient", })) - ctrl, api, err := newController(t, ctx, newFakeClient(app)) + ctrl, api, err := newController(t, ctx, newFakeClient(app), false) assert.NoError(t, err) receivedObj := map[string]interface{}{} @@ -136,7 +141,7 @@ func TestDoesNotSendNotificationIfAnnotationPresent(t *testing.T) { subscriptions.SubscribeAnnotationKey("my-trigger", "mock"): "recipient", notifiedAnnotationKey: mustToJson(state), })) - ctrl, api, err := newController(t, ctx, newFakeClient(app)) + ctrl, api, err := newController(t, ctx, newFakeClient(app), false) assert.NoError(t, err) api.EXPECT().RunTrigger("my-trigger", gomock.Any()).Return([]triggers.ConditionResult{{Triggered: true, Templates: []string{"test"}}}, nil) @@ -158,7 +163,7 @@ func TestRemovesAnnotationIfNoTrigger(t *testing.T) { subscriptions.SubscribeAnnotationKey("my-trigger", "mock"): "recipient", notifiedAnnotationKey: mustToJson(state), })) - ctrl, api, err := newController(t, ctx, newFakeClient(app)) + ctrl, api, err := newController(t, ctx, newFakeClient(app), false) assert.NoError(t, err) api.EXPECT().RunTrigger("my-trigger", gomock.Any()).Return([]triggers.ConditionResult{{Triggered: false}}, nil) @@ -173,6 +178,14 @@ func TestRemovesAnnotationIfNoTrigger(t *testing.T) { } func TestUpdatedAnnotationsSavedAsPatch(t *testing.T) { + controllerRunAndVerifyResult(t, false) +} + +func TestSendsNotificationUsingAPIFromFactory(t *testing.T) { + controllerRunAndVerifyResult(t, true) +} + +func controllerRunAndVerifyResult(t *testing.T, factorySupport bool) { ctx, cancel := context.WithCancel(context.TODO()) defer cancel() @@ -191,7 +204,7 @@ func TestUpdatedAnnotationsSavedAsPatch(t *testing.T) { patchCh <- action.(kubetesting.PatchAction).GetPatch() return true, nil, nil }) - ctrl, api, err := newController(t, ctx, client) + ctrl, api, err := newController(t, ctx, client, false) assert.NoError(t, err) api.EXPECT().RunTrigger("my-trigger", gomock.Any()).Return([]triggers.ConditionResult{{Triggered: false}}, nil) @@ -322,7 +335,7 @@ func TestWithEventCallback(t *testing.T) { subscriptions.SubscribeAnnotationKey("my-trigger", "mock"): "recipient", })) - ctrl, api, err := newController(t, ctx, newFakeClient(app), WithEventCallback(mockEventCallback)) + ctrl, api, err := newController(t, ctx, newFakeClient(app), false, WithEventCallback(mockEventCallback)) ctrl.namespaceSupport = false assert.NoError(t, err) ctrl.apiFactory = &mocks.FakeFactory{Api: api, Err: tc.apiErr} diff --git a/pkg/mocks/factory.go b/pkg/mocks/factory.go index 9a95a853..4064e418 100644 --- a/pkg/mocks/factory.go +++ b/pkg/mocks/factory.go @@ -18,5 +18,7 @@ func (f *FakeFactory) GetAPIsFromNamespace(namespace string) (map[string]api.API } func (f *FakeFactory) GetAPIsFromFactory(resource interface{}) (map[string]api.API, error) { - return nil, f.Err + apiMap := make(map[string]api.API) + apiMap["key1"] = f.Api + return apiMap, f.Err } From 976fc5a5fbae6c480d75c0c5f725306379e747c5 Mon Sep 17 00:00:00 2001 From: May Zhang Date: Mon, 7 Aug 2023 15:18:44 -0700 Subject: [PATCH 7/7] -s "adding test cases" Signed-off-by: May Zhang --- pkg/controller/controller_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/controller/controller_test.go b/pkg/controller/controller_test.go index e43f7b31..66ccba15 100644 --- a/pkg/controller/controller_test.go +++ b/pkg/controller/controller_test.go @@ -204,7 +204,7 @@ func controllerRunAndVerifyResult(t *testing.T, factorySupport bool) { patchCh <- action.(kubetesting.PatchAction).GetPatch() return true, nil, nil }) - ctrl, api, err := newController(t, ctx, client, false) + ctrl, api, err := newController(t, ctx, client, factorySupport) assert.NoError(t, err) api.EXPECT().RunTrigger("my-trigger", gomock.Any()).Return([]triggers.ConditionResult{{Triggered: false}}, nil)