fix: Allow notifications controller to notify on all namespaces (cont.) #15247

sthomson-wyn · 2023-08-25T17:32:06Z

This is a continuation of #13352 which addresses the following:
#13352 (comment)
#13352 (comment)
#13352 (comment)

This adds functionality to the notifications controller to be notified of and send notifications for applications in any namespace. The namespaces to watch are controlled by the same --application-namespaces and ARGOCD_APPLICATION_NAMESPACES variables as in the application controller.

Closes #12912
Closes #11252

Checklist:

This adds functionality to the notifications controller to be notified of and send notifications for applications in any namespace. The namespaces to watch are controlled by the same --application-namespaces and ARGOCD_APPLICATION_NAMESPACES variables as in the application controller. Signed-off-by: Nikolas Skoufis <nskoufis@seek.com.au>

Signed-off-by: Nikolas Skoufis <nskoufis@seek.com.au>

…rgo-cd into notifications-in-all-namespaces

Signed-off-by: Nikolas Skoufis <nskoufis@seek.com.au>

Signed-off-by: Nik Skoufis <n.skoufis@gmail.com>

Signed-off-by: Nikolas Skoufis <nskoufis@seek.com.au>

Signed-off-by: Stewart Thomson <sthomson@wynshop.com>

…ault) Signed-off-by: Stewart Thomson <sthomson@wynshop.com>

…l-namespaces # Conflicts: # manifests/install.yaml # manifests/namespace-install.yaml

…l-namespaces

codecov · 2023-08-25T17:59:59Z

Codecov Report

Attention: 13 lines in your changes are missing coverage. Please review.

Comparison is base (54601c8) 49.92% compared to head (6e8dd39) 49.91%.
Report is 113 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #15247      +/-   ##
==========================================
- Coverage   49.92%   49.91%   -0.02%     
==========================================
  Files         263      266       +3     
  Lines       45223    46089     +866     
==========================================
+ Hits        22578    23005     +427     
- Misses      20427    20824     +397     
- Partials     2218     2260      +42

Files	Coverage Δ
notification_controller/controller/controller.go	`60.15% <50.00%> (-5.37%)`	⬇️

... and 32 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Stewart Thomson <sthomson@wynshop.com>

mayzhang2000 · 2023-09-06T14:31:13Z

Thank you @sthomson-wyn for the great contribution!

I tested on my local with one finding. Here is my test case:

Two apps:

NAMESPACE	NAME
ns1	appns1
ns2	appns2

Notification controller is started with --application-namespaces=ns1

Expected Result: receive notification for appns1
Test Result: received notifications for both appns1 and appns2

Recommended Fix:
In controller.WithSkipProcessing(), add the following:
`
if namespace != app.GetNamespace() && !glob.MatchStringInList(applicationNamespaces, app.GetNamespace(), false) {
return true, "app is not in one of the application-namespaces, nor the notification controller namespace"
}

`

Signed-off-by: Stewart Thomson <sthomson@wynshop.com>

sthomson-wyn · 2023-09-11T12:38:42Z

Thanks @mayzhang2000 , I've added the suggested change via baa28c8

mayzhang2000 · 2023-09-12T20:41:26Z

Thank you @sthomson-wyn!
Tested and worked on my local. Could you please add some tests and documentation?

crenshaw-dev · 2023-09-27T17:14:57Z

notification_controller/controller/controller.go

@@ -38,8 +39,8 @@ var (
 	appProjects  = schema.GroupVersionResource{Group: application.Group, Version: "v1alpha1", Resource: application.AppProjectPlural}
 )

-func newAppProjClient(client dynamic.Interface, namespace string) dynamic.ResourceInterface {
-	resClient := client.Resource(appProjects).Namespace(namespace)


Is this necessary? AppProjects are still restricted to only living in the controller namespace.

crenshaw-dev · 2023-09-27T17:15:49Z

...es/k8s-rbac/argocd-server-applications/argocd-notifications-controller-rbac-clusterrole.yaml

+  - "argoproj.io"
+  resources:
+  - "applications"
+  - "appprojects"


Since AppProjects only exist in the controller's namespace, I don't think this is necessary at the cluster level.

crenshaw-dev · 2023-09-27T17:17:16Z

...es/k8s-rbac/argocd-server-applications/argocd-notifications-controller-rbac-clusterrole.yaml

+    app.kubernetes.io/name: argocd-notifications-controller-cluster-apps
+    app.kubernetes.io/part-of: argocd
+    app.kubernetes.io/component: notifications-controller
+  name: argocd-notifications-controller-cluster-apps


Should the apps-in-any-namespace docs be updated to mention the two new resources?

crenshaw-dev · 2023-09-27T17:19:16Z

notification_controller/controller/controller.go

+			if namespace != app.GetNamespace() && !glob.MatchStringInList(applicationNamespaces, app.GetNamespace(), false) {
+				return true, "app is not in one of the application-namespaces, nor the notification controller namespace"
+			}


Can we get test coverage on this? If a full test of the notificationController would be prohibitively complex, we could pull out this function and just unit-test that.

crenshaw-dev · 2023-09-27T17:19:48Z

notification_controller/controller/controller.go

-				return resClient.List(context.Background(), options)
+				appList, err := resClient.List(context.TODO(), options)
+				if err != nil {
+					return nil, err


Suggested change

return nil, err

return nil, fmt.Errorf("failed to list applications: %w", err)

crenshaw-dev · 2023-09-27T17:21:21Z

notification_controller/controller/controller_test.go

@@ -115,6 +115,7 @@ func TestInit(t *testing.T) {
 		dynamicClient,
 		nil,
 		"default",
+		[]string{},


Would you mind adding some unit tests to cover the apps-in-any-namespace behavior? I think at the minimum we should make sure the notifications controller refuses to manage apps outside the specified namespaces.

mayzhang2000 · 2023-09-27T20:58:24Z

Thank you @sthomson-wyn and @crenshaw-dev !

I created below PR as a continuation of this PR.
#15702

…ml's resources Signed-off-by: May Zhang <may_zhang@intuit.com>

crenshaw-dev · 2023-10-06T20:41:10Z

Superseded by #15702

Niksko and others added 14 commits April 26, 2023 09:20

Merge branch 'master' into notifications-in-all-namespaces

91446f9

Add SEEK to users.md

0398dfc

Signed-off-by: Nikolas Skoufis <nskoufis@seek.com.au>

Merge branch 'notifications-in-all-namespaces' of github.com:Niksko/a…

ca54f25

…rgo-cd into notifications-in-all-namespaces

Remove unused fields

6f5979e

Signed-off-by: Nikolas Skoufis <nskoufis@seek.com.au>

Merge branch 'master' into notifications-in-all-namespaces

36467a6

Merge branch 'master' into notifications-in-all-namespaces

da6be0e

Revert changes to Procfile

c3c1603

Signed-off-by: Nik Skoufis <n.skoufis@gmail.com>

Fix unit tests

ee4ab23

Signed-off-by: Nikolas Skoufis <nskoufis@seek.com.au>

Merge branch 'master' into notifications-in-all-namespaces

6f7ecf4

- add argocd namespaces environment variable to notifications controller

863d6d4

Signed-off-by: Stewart Thomson <sthomson@wynshop.com>

- add example cluster role rbac

b36ed87

Signed-off-by: Stewart Thomson <sthomson@wynshop.com>

- only look for projects in the controller's namespace (argocd by def…

e4d963e

…ault) Signed-off-by: Stewart Thomson <sthomson@wynshop.com>

Merge remote-tracking branch 'origin/master' into notifications-in-al…

46b0404

…l-namespaces # Conflicts: # manifests/install.yaml # manifests/namespace-install.yaml

sthomson-wyn force-pushed the notifications-in-all-namespaces branch from 13cfca6 to 46b0404 Compare August 25, 2023 17:46

Merge remote-tracking branch 'origin/master' into notifications-in-al…

362a89c

…l-namespaces

sthomson-wyn mentioned this pull request Aug 25, 2023

fix: Allow notifications controller to notify on all namespaces #13352

Closed

13 tasks

- update base manifest

4996eea

Signed-off-by: Stewart Thomson <sthomson@wynshop.com>

sthomson-wyn force-pushed the notifications-in-all-namespaces branch from 21a3c10 to 4996eea Compare August 25, 2023 18:11

crenshaw-dev requested a review from mayzhang2000 August 25, 2023 18:17

sthomson-wyn added 2 commits September 11, 2023 14:36

Merge branch 'master' into notifications-in-all-namespaces

d0c25c8

- skip app processing in notification controller

baa28c8

Signed-off-by: Stewart Thomson <sthomson@wynshop.com>

crenshaw-dev requested changes Sep 27, 2023

View reviewed changes

mayzhang2000 mentioned this pull request Sep 27, 2023

fix(notifications): Allow notifications controller to notify on all namespaces #15702

Merged

13 tasks

updated examples/k8s-rbac/argocd-server-applications/kustomization.ya…

6e8dd39

…ml's resources Signed-off-by: May Zhang <may_zhang@intuit.com>

mayzhang2000 requested a review from a team as a code owner October 4, 2023 21:30

crenshaw-dev closed this Oct 6, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Allow notifications controller to notify on all namespaces (cont.) #15247

fix: Allow notifications controller to notify on all namespaces (cont.) #15247

sthomson-wyn commented Aug 25, 2023

codecov bot commented Aug 25, 2023 •

edited

Loading

mayzhang2000 commented Sep 6, 2023 •

edited

Loading

sthomson-wyn commented Sep 11, 2023

mayzhang2000 commented Sep 12, 2023

crenshaw-dev Sep 27, 2023

crenshaw-dev Sep 27, 2023

crenshaw-dev Sep 27, 2023

crenshaw-dev Sep 27, 2023

crenshaw-dev Sep 27, 2023

crenshaw-dev Sep 27, 2023

mayzhang2000 commented Sep 27, 2023 •

edited

Loading

crenshaw-dev commented Oct 6, 2023

	return nil, err
	return nil, fmt.Errorf("failed to list applications: %w", err)

fix: Allow notifications controller to notify on all namespaces (cont.) #15247

fix: Allow notifications controller to notify on all namespaces (cont.) #15247

Conversation

sthomson-wyn commented Aug 25, 2023

codecov bot commented Aug 25, 2023 • edited Loading

Codecov Report

mayzhang2000 commented Sep 6, 2023 • edited Loading

sthomson-wyn commented Sep 11, 2023

mayzhang2000 commented Sep 12, 2023

crenshaw-dev Sep 27, 2023

Choose a reason for hiding this comment

crenshaw-dev Sep 27, 2023

Choose a reason for hiding this comment

crenshaw-dev Sep 27, 2023

Choose a reason for hiding this comment

crenshaw-dev Sep 27, 2023

Choose a reason for hiding this comment

crenshaw-dev Sep 27, 2023

Choose a reason for hiding this comment

crenshaw-dev Sep 27, 2023

Choose a reason for hiding this comment

mayzhang2000 commented Sep 27, 2023 • edited Loading

crenshaw-dev commented Oct 6, 2023

codecov bot commented Aug 25, 2023 •

edited

Loading

mayzhang2000 commented Sep 6, 2023 •

edited

Loading

mayzhang2000 commented Sep 27, 2023 •

edited

Loading