Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Support notifications in any namespace #1520

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ArthurVardevanyan
Copy link
Contributor

What type of PR is this?

Uncomment only one /kind line, and delete the rest.
For example, > /kind bug would simply become: /kind bug

/kind bug

/kind chore
/kind cleanup
/kind failing-test
/kind enhancement
/kind documentation
/kind code-refactoring

What does this PR do / why we need it:

Adding support for the Operator to be able to use ArgoCD in Any Namespaces with The Notification Controller

See PR: argoproj/argo-cd#15702

Have you updated the necessary documentation?

  • Documentation update is required by this PR.
  • Documentation has been updated.

Which issue(s) this PR fixes:

Fixes #?

How to test changes / Special notes to the reviewer:

Before
before

After
After

@svghadi svghadi changed the title fix(Apps in Any Namespace): Notification Controller Command feat: Support notifications in any namespace Aug 26, 2024
@svghadi
Copy link
Collaborator

svghadi commented Aug 26, 2024

Thanks @ArthurVardevanyan. I will try to get to it in next few days.

@svghadi svghadi self-requested a review August 26, 2024 12:25
@svghadi
Copy link
Collaborator

svghadi commented Sep 16, 2024

Hi @ArthurVardevanyan, apologies for the delay in getting back to you. After reviewing the upstream documentation [1], I noticed a couple of things that need to be addressed to get this feature working properly:

  • The --self-service-notification-enabledCLI flag is missing.
  • Some additional permissions may be required [2].

You might be able to resolve these issues by running some tests. Please refer to appset in any ns PR[3] for details on permission roles and policy implementation.

[1] https://argo-cd.readthedocs.io/en/stable/operator-manual/notifications/#namespace-based-configuration
[2] argoproj/argo-cd#16153
[3] #1209

@ArthurVardevanyan
Copy link
Contributor Author

ArthurVardevanyan commented Sep 16, 2024

apologies for the delay

No worries, both of these flags can also be set via ENV overrides, so we have been testing with that in the interm.

  • --self-service-notification-enabled

For this first point, this technically does work without, however tenants can only use the notification controller defined globally.
A question from a flagging perspective, would you want the ability to toggle this flag independently or not?,
I updated the PR to assume the enablement of both.

For my use case it isn't a problem to have it have it enabled automatically along side the any namespace feature.

Some additional permissions may be required [2].

For the second point, as far as I could tell, ArgoCD Operator doesn't provide any RBAC for the any of the argocd in any NameSpace Features, some maybe that would be a follow up to this to get all of that rigged.

I have been defining myself:
https://github.com/ArthurVardevanyan/HomeLab/blob/main/kubernetes/argocd/apps/rbac.yaml

You might be able to resolve these issues by running some tests. Please refer to appset in any ns PR[3] for details on permission roles and policy implementation.

Will take a peak at this one.

@ArthurVardevanyan ArthurVardevanyan force-pushed the notifications branch 3 times, most recently from b49b6ed to 1dedfbc Compare September 17, 2024 15:41
@ArthurVardevanyan
Copy link
Contributor Author

ArthurVardevanyan commented Sep 17, 2024

@svghadi
Copy link
Collaborator

svghadi commented Sep 18, 2024

Thanks. Allow me some time to discuss this internally on design side. I will get back to you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants