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: changes to support the feature apps in any namespaces #794

Merged
merged 16 commits into from
Nov 16, 2022

Conversation

ishitasequeira
Copy link
Collaborator

@ishitasequeira ishitasequeira commented Oct 12, 2022

What type of PR is this?
/kind enhancement

What does this PR do / why we need it:
The PR adds support for the feature apps in any namespace added upstream. To do so, the PR adds new command args --additional-namespace to ArgoCD ApplicationController and ArgoCD Server components. Also, the PR adds new roles in the respective namespaces for ArgoCD Server to perform actions in the namespace.

Have you updated the necessary documentation?

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

Which issue(s) this PR fixes:
https://issues.redhat.com/browse/GITOPS-2341

How to test changes / Special notes to the reviewer:

  • Run make install run
  • Create namespaces argocd-test and argocd-1
  • Deploy an ArgoCD instance using below yaml file
apiVersion: argoproj.io/v1alpha1
kind: ArgoCD
metadata:
  name: example-argocd
spec:
  controller:
    sourceNamespaces:
    - argocd
    - argocd-1
  notifications:
    enabled: True
    env:
      - name: foo
        value: bar
  server:
    ingress:
      enabled: true
    sourceNamespaces:
    - argocd-test
    - argocd-1
  • Check if the ArgoCD pods came up successfully using command kubectl get pods | grep example-argocd

  • Check whether the new roles were created in namespaces argocd-test and argocd-1

  • Check if the namespaces argocd-test and argocd-1 have a new label argocd.argoproj.io/managed-by-cluster-argocd

  • e2e test for the PR.
    kubectl kuttl test ./tests/k8s --config ./tests/kuttl-tests.yaml --test 1-024_validate_apps_in_any_namespace

@ishitasequeira ishitasequeira force-pushed the apps-in-any-namespaces branch 3 times, most recently from dcf2f00 to 88634f0 Compare October 14, 2022 12:51
@ishitasequeira ishitasequeira marked this pull request as ready for review October 14, 2022 18:03
@jaideepr97
Copy link
Collaborator

@ishitasequeira

what happens if the same namespace features in the "sourceNamespaces" list of 2 different argo-cd instances?
would both instances try to manage the same application?

I think it would also be good to have some e2e tests written out that can cover scenarios like this

@ishitasequeira
Copy link
Collaborator Author

@ishitasequeira

what happens if the same namespace features in the "sourceNamespaces" list of 2 different argo-cd instances? would both instances try to manage the same application?

Yes, we would reconcile the roles for both the argo-cd instances.

@jaideepr97
Copy link
Collaborator

jaideepr97 commented Oct 24, 2022

@ishitasequeira that might be a problem, no?
having multiple argo-cd instances managing the same application doesn't sound like a good idea to me

should we maybe consider some kind of mechanism to check if an app is already being managed by some other instance?

@ishitasequeira
Copy link
Collaborator Author

@ishitasequeira that might be a problem, no? having multiple argo-cd instances managing the same application doesn't sound like a good idea to me

should we maybe consider some kind of mechanism to check if an app is already being managed by some other instance?

As mentioned in @jannfis's upstream PR description,

If there are multiple instances of Argo CD installed to the same cluster, the behavior when more than one instance specifies same allowed namespace patterns (and have access to those namespaces) is undefined. This scenario should be avoided (needs to be documented).

I will update our documentation with this comment.

@jaideepr97
Copy link
Collaborator

@ishitasequeira
I see
I suppose we could leave it to the users to be careful...however, I think since we are an operator (as opposed to upstream) maybe there is some level of expectation that we would go further to make the users' experience more convenient?
I'm fine with leaving it up to the user, but I think it might be worth discussing if we should try and put some guardrails in place all the same

@sbose78
Copy link
Contributor

sbose78 commented Oct 26, 2022

@jaideepr97 Agreed. A little over-engineered solution could be having an admission webhook disallow specification of a namespace in sourceNamespaces if it was already specified for a different ArgoCD instance.

@jaideepr97
Copy link
Collaborator

@sbose78 that could work
maybe a slightly more straightforward way could be to just maintain a map to track which namespaces are watched by which instances, and not create roles when we encounter a namespace that's already being watched by a different instance
(Also log it the operator and emit a warning event if necessary mentioning that xyz namespace is already being watched?)

@ishitasequeira
Copy link
Collaborator Author

@sbose78 that could work maybe a slightly more straightforward way could be to just maintain a map to track which namespaces are watched by which instances, and not create roles when we encounter a namespace that's already being watched by a different instance (Also log it the operator and emit a warning event if necessary mentioning that xyz namespace is already being watched?)

I think managing a map might be difficult as we would have to take care of cleaning it up too if the ArgoCD instance is updated later and a namespace is removed from it.. I was wondering if we could use a label like [argocd.argoproj.io/managed-by](http://argocd.argoproj.io/managed-by) and we check if the namespace has this label or not.

@jaideepr97
Copy link
Collaborator

I think managing a map might be difficult as we would have to take care of cleaning it up too if the ArgoCD instance is updated later and a namespace is removed from it.

@ishitasequeira FWIW I don't think cleanup would be too bad, we would just need to watch for changes in the CR when a namespace is removed from the watch list, and when a namespace is deleted - in both cases we check if that namespace has any roles that need to be cleaned up & then remove that entry from the map

(We already have similar watches in place to react to different events like removal of SSO in CR or deletion of managed namespace)

@ishitasequeira
Copy link
Collaborator Author

As agreed in the slack conversation, we are attaching a label argocd.argoproj.io/managed-by-cluster-argocd to every namespace added as a SourceNamespace.

@ishitasequeira
Copy link
Collaborator Author

@iam-veeramalla @jopit @jaideepr97 could you review the PR?

Copy link
Collaborator

@jaideepr97 jaideepr97 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ishitasequeira it seems like we may not be handling clean up in this PR. When someone removes a namespace from a source list, I think the label should be removed from that namespace and the roles and rolebindings will need to be cleaned up as well.

Another point that occurred to me was that if one instance tries to watch a namespace that is already being watched by a different instance we could consider alerting them through a warning event or something else

api/v1alpha1/argocd_types.go Outdated Show resolved Hide resolved
controllers/argocd/role.go Outdated Show resolved Hide resolved
controllers/argocd/role.go Outdated Show resolved Hide resolved
controllers/argocd/role.go Outdated Show resolved Hide resolved
@ishitasequeira ishitasequeira force-pushed the apps-in-any-namespaces branch 3 times, most recently from ae96967 to a6219c1 Compare November 7, 2022 12:12
@ishitasequeira ishitasequeira changed the title made changes to support the feature apps in any namespaces changes to support the feature apps in any namespaces Nov 8, 2022
@jaideepr97
Copy link
Collaborator

I have tested this PR locally and it works as expected for the most part
Couple things I have discussed with @ishitasequeira that we should document :

  1. No namespace can be managed by multiple argo-cd instances (cluster scoped or namespace scoped) i.e, only one of either managed-by or managed-by-cluster-argocd labels can be applied to a given namespace. We will be prioritizing managed-by label in case of a conflict as this feature is currently in beta, so the new roles/rolebindings will not be created if namespace is already labelled with managed-by label, and they will be deleted if a namespace is first added to the sourceNamespacs list and is later also labelled with managed-by label

  2. This also means users cannot currently create/manage apps and create app resources in the same non-control plane namespace (as they both require their own labels) out of the box
    as a workaround users will have to create custom roles

@ishitasequeira please update the e2e test to ensure proper cleanup of roles/rolebindings as discussed

@jaideepr97
Copy link
Collaborator

other than that this LGTM, thanks a lot @ishitasequeira !!

@jaideepr97
Copy link
Collaborator

/lgtm

Signed-off-by: ishitasequeira <ishiseq29@gmail.com>
Signed-off-by: ishitasequeira <ishiseq29@gmail.com>
Signed-off-by: ishitasequeira <ishiseq29@gmail.com>
Signed-off-by: ishitasequeira <ishiseq29@gmail.com>
Signed-off-by: ishitasequeira <ishiseq29@gmail.com>
Signed-off-by: ishitasequeira <ishiseq29@gmail.com>
Signed-off-by: ishitasequeira <ishiseq29@gmail.com>
Signed-off-by: ishitasequeira <ishiseq29@gmail.com>
Signed-off-by: ishitasequeira <ishiseq29@gmail.com>
Signed-off-by: ishitasequeira <ishiseq29@gmail.com>
Signed-off-by: ishitasequeira <ishiseq29@gmail.com>
Signed-off-by: ishitasequeira <ishiseq29@gmail.com>
Signed-off-by: ishitasequeira <ishiseq29@gmail.com>
Signed-off-by: ishitasequeira <ishiseq29@gmail.com>
@ishitasequeira
Copy link
Collaborator Author

@jaideepr97 I have updated the documentation and the e2e test.

@iam-veeramalla @wtam2018 PTAL.

Copy link
Collaborator

@jaideepr97 jaideepr97 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ishitasequeira Please add a step in the e2e test that removes a namespace from a source list, and then checks that the role/rolebinding is removed from the namespace that was removed from the list

Signed-off-by: ishitasequeira <ishiseq29@gmail.com>
Signed-off-by: ishitasequeira <ishiseq29@gmail.com>
@jaideepr97 jaideepr97 merged commit 0e3ccd5 into argoproj-labs:master Nov 16, 2022
@jaideepr97
Copy link
Collaborator

Merged, thanks again @ishitasequeira !

@iam-veeramalla iam-veeramalla changed the title changes to support the feature apps in any namespaces feat: changes to support the feature apps in any namespaces Dec 8, 2022
@iam-veeramalla iam-veeramalla added this to the 0.5.0 milestone Dec 8, 2022
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.

4 participants