argo-cd icon indicating copy to clipboard operation
argo-cd copied to clipboard

feat: Applications in any namespace

Open jannfis opened this issue 2 years ago • 15 comments

Apps in any Namespace

Description

This change enables Application resources to exist in any namespace allowed by configuration.

The feature is not enabled by default, and has to be explicitly enabled by the administrator (see below).

The change aims to be fully backwards compatible.

This is a rather large change. It comprises changes to the controller's reconciliation logic, the API server as well as changes to the CLI and the UI. I try to outline the changes and design decisions best I can in the below description.

This PR implements proposal #6409 and supersedes the PoC PR #6537

Some terminology

  • Control plane cluster - the cluster Argo CD is installed to
  • Control plane namespace - the namespace where Argo CD is installed to (where the application controller is running in)
  • Application - an Argo CD Application resource
  • Project or AppProject - an Argo CD AppProject resource
  • User - an ordinary Argo CD user with some permissions to e.g. create Applications via CLI or UI and who is restricted by Argo CD RBAC
  • Administrator - an Argo CD administrator with access to the control plane's namespace

Status

This change is beta-quality and could require some additional testing and eyes.

My suggestion is to merge it early and keep polishing it until 2.5 release. If at time of 2.5 release there still are some known issues, we make sure to document them properly.

Things that work

  • Current end-to-end test suite runs without any changes being made to the tests. The changes to the e2e test framework are done to support testing applications in namespaces other than the control plane's one.

  • Initial set of e2e tests with applications in a different namespace have been added (not yet complete)

  • UI support is not fully finished yet (most things work, needs fine tuning)

  • CLI support is near complete (end-to-end tests are passing)

  • Documentation is not yet written, as there may be additional changes derived from the feedback cycles

Known issues

  • During my tests, I have found a bug with the annotation tracking method that I wasn't able to fix yet. It affects the controller in not noticing changes to tracked resources on the cluster under certain circumstances, when application.resourceTrackingMethod is set to annotation. I'm not yet sure whether I introduced this bug, or if it's a general bug with the tracking method. Edit: This is indeed a bug with the tracking method, not introduced through this change (reproduced it on code from master branch) - will open separate issue for that. Edit 2: Issue for this https://github.com/argoproj/argo-cd/issues/9781

Things to do

  • [ ] Adapt RBAC format in a backward compatible way
  • [ ] Documentation
  • [ ] Finalize UI changes
  • [ ] Finalize specific end-to-end tests
  • [ ] Code polishing

Breaking changes

  • None of I'd be aware of right now. If you find one, please let me know.

Change details

Enabling the feature

By default, the feature is disabled. Administrators have to explicitly enable it by giving the --application-namespaces parameter to the workloads of argocd-server and argocd-application-controller.

Open questions:

  • Should this be a setting in argocd-cm rather than a command line parameter, so list of namespaces can be changed at runtime?

Allowed namespaces

The --application-namespaces parameter takes a comma-separated list of allowed namespaces as glob patterns. The pattern * enables all namespaces on the control plane's cluster. The control plane's namespace (usually argocd) is always enabled. It cannot be disabled.

For example, setting --application-namespaces app-team1-*,app-team2-* would allow the controller and the server to handle applications in namespaces starting with app-team1- and app-team2-, but not in the namespace app-team-3.

When the feature is enabled, controller and server are switched from namespaced mode (i.e. "get and watch resources only from the control plane's namespace") to all namespaces mode. One reason is, that to support glob patterns, we don't know the namespaces to operate on at initialisation time, and second we don't want to keep a separate lister or informer for each allowed namespace due to compute and complexity constraints. Instead, we check for enabled namespaces when using the lister.

Only namespaces in the control plane's cluster can host Applications. Hosting Application resources in remote clusters are out of scope for now.

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).

Application names

Applications will now be referenced to as namespace/name in the CLI and the UI. If namespace is omitted, the control plane's namespace (argocd in most installations) will be assumed. That means when Argo CD is installed in the namespace argocd, the application names argocd/guestbook and guestbook are semantically the same. When Argo CD is installed in the namespace gitops, likewise the application names gitops/guestbook and guestbook are semantically the same.

A similar syntactic change has been made for the UI. For example, both of the following URLs would now refer to the same application in the UI:

https://argocd.example.com/applications/guestbook

and

https://argocd.example.com/applications/argocd/guestbook

For the API, all relevant endpoints now offer the (optional) parameter applicationNamespace to specify the source namespace of the Application. If this parameter is not set, the control plane's namespace will be assumed.

For example, to retrieve the application guestbook in the namespace foo via the REST API, the following call can be made

GET /api/v1/applications/guestbook?appNamespace=foo

The reason for implementing this as query parameter instead of in the path is actually a limitation of grpc-gateway, which does not allow two paths to the same resource.

These changes to referencing also mean that now there may be two or more applications with the same name of guestbook, when all of them exist in a different namespace. For example:

argocd app get ns1/guestbook
argocd app get ns2/guestbook
argocd app get guestbook

Would all refer to an application named guestbook, but each of them would be in different namespaces and are unique applications.

The tracking label

For applications in the control plane's namespace, nothing changes. That means that the application guestbook in the argocd namespace will still be using the value guestbook in the tracking label. This keeps backwards compatibility and ensures that no resync of resources has to happen when a version containing this change is deployed, or when the feature is being enabled.

For applications in other namespaces, the value of the tracking label will be a combination of the namespace and the application name, concatenated with the underscore character. That means the application guestbook in the namespace foo will use foo_guestbook as the tracking label's value. The underscore character was chosen for being unambigious in this context, as it is not allowed in Kubernetes resource name.

Because this can lead to values that exceed the maximum length for label values, it is recommended to use annotation method for tracking resources. When the string combined of namespace and application name exceeds the 63 characters limit for label values, Kubernetes throws an error when trying to manage resources of such applications with Argo CD using the label tracking method.

Project association for applications

AppProjects govern much of the permissions that are granted to an Application. For this reason, Argo CD API RBAC ensures that users can only associate to projects that they are allowed to. A typical Argo CD user has no access to the control plane's namespace in Kubernetes, and is only allowed to create applications using the API, where RBAC is enforced.

The goal of this change is to allow ordinary Argo CD users to manage their Applications in a declarative way, in their own namespaces on the control plane's cluster. But in the declarative way, an Application may associate to any AppProject by just setting the field .spec.project to the name of the appropriate project. This might allow users to escalate their privileges on the cluster.

In order to prevent this privilege escalation, an Argo CD administrator will have to configure the names of allowed namespaces in the project. For this, a new field has been introduced in the AppProject's spec: .spec.sourceNamespaces. An Application wanting to associate to a given project must exist in one of the namespaces defined in this field.

As an example, when .spec.sourceNamespaces for project proj is set to foo, the Application foo/app is allowed to associate to project proj, but the application bar/app is not.

The .spec.sourceNamespaces field supports pattern matching, i.e.

spec:
  sourceNamespaces:
  - team1-source
  - team2-*

would allow Applications in the team1-source namespace as well as in any namespace starting with team2- to associate with the project.

The check for a namespace to be allowed to associate to a project has been made an integral part of the controller's reconciliation logic. Administrators can revoke the permissions at any time by removing the namespace from the sourceNamespaces field of the AppProject, and the controller will then enforce it. When the controller finds an Application with a project association that is not allowed, it will refuse to perform any reconciliation activity.

For backwards compatibility, Applications in the control plane's namespace are allowed to associate to any project.

By default, no other namespaces are allowed.

How to test manually

  • Set ARGOCD_APPLICATION_NAMESPACES to a list of namespaces that should be allowed by API server and controller and run make start or make start-local, e.g:

    ARGOCD_APPLICATION_NAMESPACES="argocd-*" make start
    
  • Create a new AppProject which has .spec.sourceNamespaces set to some allowed namespaces for that project (or modify the default project).

  • Create a new app in a different namespace, e.g. argocd app create argocd-test/test-app ...

  • Manage this app using the CLI or the UI

  • Try different things - different namespaces (including ones that are not allowed, etc), app deletion, declarative app creation in allowed namespaces and not allowed namespaces, etc

Open questions

  • [ ] Space for rent

Closes #3474

jannfis avatar Jun 22 '22 12:06 jannfis

Codecov Report

Merging #9755 (c10a381) into master (af40d52) will decrease coverage by 0.09%. The diff coverage is 37.66%.

@@            Coverage Diff             @@
##           master    #9755      +/-   ##
==========================================
- Coverage   46.18%   46.08%   -0.10%     
==========================================
  Files         226      228       +2     
  Lines       27581    27858     +277     
==========================================
+ Hits        12737    12839     +102     
- Misses      13124    13282     +158     
- Partials     1720     1737      +17     
Impacted Files Coverage Δ
cmd/argocd/commands/app_actions.go 0.00% <0.00%> (ø)
cmd/argocd/commands/app_resources.go 7.75% <0.00%> (-0.73%) :arrow_down:
cmd/util/project.go 9.70% <0.00%> (-0.40%) :arrow_down:
controller/cache/cache.go 20.88% <0.00%> (ø)
pkg/apiclient/apiclient.go 0.95% <0.00%> (-0.02%) :arrow_down:
server/application/terminal.go 12.75% <0.00%> (-0.09%) :arrow_down:
util/settings/settings.go 51.36% <0.00%> (-0.06%) :arrow_down:
cmd/argocd/commands/app.go 19.52% <2.14%> (-0.90%) :arrow_down:
controller/clusterinfoupdater.go 33.33% <20.00%> (-2.26%) :arrow_down:
pkg/apis/application/v1alpha1/types.go 54.84% <36.36%> (+0.14%) :arrow_up:
... and 15 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

codecov[bot] avatar Jun 22 '22 12:06 codecov[bot]

For the tracking label thing for other namespace, maybe mutating webhook can be a better solution, means you don’t need to make any change in existing logic. Especially we use CRDs in such case

JasonHao123 avatar Jun 28 '22 06:06 JasonHao123

Curious about how this affects other resources (when declared outside of the control plane namespace):

  • AppProject - Suppose I created a namespace per environment, could I have projects: dev/default, staging/default, prod/default?
  • Secrets - the cluster and repository secrets being limited to their namespace seems like a natural security boundary.
  • Application Sets - Should probably be limited to their own namespace? Cluster, etc. secrets used by generators would presumably be limited to the namespace.

iamnoah avatar Jul 15 '22 15:07 iamnoah

Curious about how this affects other resources (when declared outside of the control plane namespace):

  • AppProject - Suppose I created a namespace per environment, could I have projects: dev/default, staging/default, prod/default?

AppProjects will not be allowed to exist in any other than the control plane's namespace, since they control much of the governance aspects of Argo CD. If we'd allow users to create AppProjects in their own namespaces, this would allow them to escalate privileges within Argo CD (and therefore, on the cluster). IMHO, managing AppProjects will always be an administrative task, not a user one.

  • Secrets - the cluster and repository secrets being limited to their namespace seems like a natural security boundary.

Access to clusters and repositories is governed by the AppProject, see above. This PR does not change the scope of secrets for cluster and repositories.

  • Application Sets - Should probably be limited to their own namespace? Cluster, etc. secrets used by generators would presumably be limited to the namespace.

Eventually, users could launch their own ApplicationSet controller(s) in any of the namespaces they're allowed to to generate Applications . These instances will not have access to any secrets in the Argo CD control plane's namespace, tho. So, especially cluster generators may not work. We need to think about a way to properly integrate ApplicationSet into this mechanism. OTOH, we could also extend the ApplicationSet controller to implement similar mechanisms as the application controller, i.e. watch for appset resources in the same namespace. However, for the first incarnation of this change, app set controller is out of scope.

jannfis avatar Jul 18 '22 10:07 jannfis

AppProjects will not be allowed to exist in any other than the control plane's namespace, since they control much of the governance aspects of Argo CD. If we'd allow users to create AppProjects in their own namespaces, this would allow them to escalate privileges within Argo CD (and therefore, on the cluster). IMHO, managing AppProjects will always be an administrative task, not a user one.

I would expect namespaced AppProjects to have similar restrictions to namespaced apps. i.e., they can only be assigned to applications in the same namespace, can only access secrets from that namespace, etc. So there should be no way to escalate privileges beyond the namespace. Is there any reason that would not work? (if out of scope for this PR, I can understand that.)

iamnoah avatar Jul 18 '22 14:07 iamnoah

I would expect namespaced AppProjects to have similar restrictions to namespaced apps. i.e., they can only be assigned to applications in the same namespace, can only access secrets from that namespace, etc. So there should be no way to escalate privileges beyond the namespace. Is there any reason that would not work? (if out of scope for this PR, I can understand that.)

Currently, the AppProject restricts - among other things - what kind of resources (and their scope, i.e. cluster scoped or namespace scoped) an application is allowed to deploy, and where it is allowed to deploy (clusters, and namespaces in those clusters). IMHO, this control must not be given to the user. I'm not sure how we could properly design a governance model around this. But if you have ideas, I'm all ears :)

jannfis avatar Jul 18 '22 15:07 jannfis

@crenshaw-dev Thanks for the review! I have addressed almost all of your comments.

For the RBAC change (which has not yet been made), I was twisting my head. I see two possible options to extend the RBAC syntax to address applications:

  1. project/namespace/name
  2. namespace/project/name

I think option 1 would be the one to go for, as it provides the least amount of surprise. However, there are few problems due to the fact that our glob matcher doesn't treat the forward-slash as separator. So, */foobar would match the application name foobar in any project, and any namespace. Similarly, default/* would match any application in any namespace in the project default. I'm not sure if that'd be expected, but it would definitely be backwards compatible. To allow application foobar in the namespace barbar in any project, the RBAC identifier would look like */barbar/foobar. This behavior could lead to unwanted behavior, tho, if people forget about the namespace.

We could introduce a slightly breaking change here and provide / as the path separator. So, */foobar would actually never match. It would have to be */*/foobar or **/foobar.

Maybe you have some thoughts on all that?

jannfis avatar Jul 19 '22 15:07 jannfis

For the RBAC change (which has not yet been made), I was twisting my head. I see two possible options to extend the RBAC syntax to address applications:

  1. project/namespace/name
  2. namespace/project/name

I think option 1 would be the one to go for, as it provides the least amount of surprise. However, there are few problems due to the fact that our glob matcher doesn't treat the forward-slash as separator. So, /foobar would match the application name foobar in any project, and any namespace. Similarly, default/ would match any application in any namespace in the project default. I'm not sure if that'd be expected, but it would definitely be backwards compatible. To allow application foobar in the namespace barbar in any project, the RBAC identifier would look like */barbar/foobar. This behavior could lead to unwanted behavior, tho, if people forget about the namespace.

We could introduce a slightly breaking change here and provide / as the path separator. So, */foobar would actually never match. It would have to be //foobar or **/foobar.

I like option 1.

Considering the change without adding the separator:

  • */app will match both project/app and project/namespace/app - I think I'm okay with that
  • project/* will match both project/app and /project/namespace/app - also seems fine
  • */* and * will continue to match everything - makes sense

Since the admin has to opt in to adding namespaces, I think we can just add notes to the docs about how RBAC matching works. The behavior above seems fairly intuitive.

Am I correct that project/app will still match apps in the default ns? Or will the new RBAC have to be project/argocd/app?

crenshaw-dev avatar Jul 22 '22 14:07 crenshaw-dev

For the RBAC change (which has not yet been made), I was twisting my head. I see two possible options to extend the RBAC syntax to address applications:

  1. project/namespace/name
  2. namespace/project/name

I think option 1 would be the one to go for, as it provides the least amount of surprise. However, there are few problems due to the fact that our glob matcher doesn't treat the forward-slash as separator. So, /foobar would match the application name foobar in any project, and any namespace. Similarly, default/ would match any application in any namespace in the project default. I'm not sure if that'd be expected, but it would definitely be backwards compatible. To allow application foobar in the namespace barbar in any project, the RBAC identifier would look like */barbar/foobar. This behavior could lead to unwanted behavior, tho, if people forget about the namespace. We could introduce a slightly breaking change here and provide / as the path separator. So, */foobar would actually never match. It would have to be //foobar or **/foobar.

I like option 1.

Considering the change without adding the separator:

  • */app will match both project/app and project/namespace/app - I think I'm okay with that
  • project/* will match both project/app and /project/namespace/app - also seems fine
  • */* and * will continue to match everything - makes sense

Since the admin has to opt in to adding namespaces, I think we can just add notes to the docs about how RBAC matching works. The behavior above seems fairly intuitive.

Am I correct that project/app will still match apps in the default ns? Or will the new RBAC have to be project/argocd/app?

I also agree with option 1. I think project/namespace/name makes more intuitive sense to an admin by sticking with the norm of generic to specific. I think the small breaking change in add the / as a seperator is also worth it.

zachaller avatar Jul 22 '22 15:07 zachaller

I think the small breaking change in add the / as a seperator is also worth it.

This part I'm less certain about, because I think it would break anything with fewer than two slashes (i.e. *, */*, */app, proj/*).

We could auto-upgrade those patterns on the backend:

  • * and */* -> */*/*]
  • project/* -> project/*/*
  • */app -> */*/app

But there are some patterns which would be trickier to upgrade:

  • *-dev -> */*/*-dev
  • dev-* -> dev-*/*/*
  • proj/dev-* -> proj/*/dev-*
  • proj/*-dev -> proj/*/*-dev
  • nightmare-*-rbac-*-yikes -> I don't even know... probably a combination of a bunch of patterns

For typical patterns, I think auto-upgrade is easy. For atypical patterns, I think it's a pretty big challenge.

@jannfis what's the RBAC pattern for an Application in the argocd namespace (regardless of whether we add the separator)? proj/argocd/app or just proj/app?

crenshaw-dev avatar Jul 22 '22 15:07 crenshaw-dev

We could

  1. Introduce an optional formatVersion field to argocd-rbac-cm (defaulting to v1 if not present)
  2. Have v2 opt them in to validating for fully-slashed RBAC format and enabling the separator in casbin
  3. Refuse to allow apps in non-argocd namespaces until the user changes their formatVersion to v2

crenshaw-dev avatar Jul 22 '22 16:07 crenshaw-dev

Can I test this with a specific image tag instead of manually compiling the source?

FireDrunk avatar Jul 29 '22 06:07 FireDrunk

@FireDrunk I think you'll have to compile. I don't believe images in PRs are pushed anywhere.

But the make image make target is very easy/reliable in my experience.

crenshaw-dev avatar Jul 29 '22 15:07 crenshaw-dev

@jannfis what's the RBAC pattern for an Application in the argocd namespace (regardless of whether we add the separator)? proj/argocd/app or just proj/app?

It would be proj/app in this case, to keep backwards compatibility for now. I have just moved the RBAC name generation from apputil to a receiver of the Application type (with 0d9eea7e3014d0b98e4cb27b3a60ada3956ee350), and it will take the default namespace as an argument.

We could

  1. Introduce an optional formatVersion field to argocd-rbac-cm (defaulting to v1 if not present)
  2. Have v2 opt them in to validating for fully-slashed RBAC format and enabling the separator in casbin
  3. Refuse to allow apps in non-argocd namespaces until the user changes their formatVersion to v2

I like this idea, I'm just not sure if argocd-rbac-cm would be the right place for it. I think it should be part of argocd-cm and be made available in the settings, so we will have access from (virtually) everywhere to that option. Also, I'm not sure if it should affect the ability to create applications in other namespaces. There basically is a feature flag for that already with --application-namespaces parameter to the API server and the application controller.

Maybe we can also implicitly assume v2 RBAC if that parameter is set? So if user specifies --application-namespaces, then we enable slash-separator in the glob match. Hm, let me think about it.

However, since I spend over an hour resolving merge conflicts again this morning, I would kindly ask to postpone any possible changes to this to another PR, once we thoroughly discussed the pros and cons, and merge this PR as early as possible. Also, the feature in this PR is opt-in for now, and I would like to also mark it as "beta" for 2.5.

jannfis avatar Aug 01 '22 10:08 jannfis

I'm not sure if it should affect the ability to create applications in other namespaces... Maybe we can also implicitly assume v2 RBAC if that parameter is set?

That makes sense!

I would kindly ask to postpone any possible changes to this to another PR

Agreed! I'll look over things again and approve ASAP.

crenshaw-dev avatar Aug 01 '22 14:08 crenshaw-dev

I decided to make two shared informer factories out of the one: One for Applications, and one for AppProjects. The one for AppProjects will always be scoped to the control plane's namespaces, since we don't plan have AppProjects elsewhere. As for the informer for Applications, I have implemented what you suggested. This way, we need to include AppProjects in the cluster RBAC, only Applications.

Need to make a note in the documentation that multiple application source namespaces will only be supported in cluster install mode, because it will need an informer for cluster-scoped resources (which we obviously won't install ootb with namespace-scoped installations).

jannfis avatar Aug 05 '22 10:08 jannfis

Probably should do the same in the application controller, let me check.

jannfis avatar Aug 05 '22 10:08 jannfis

Funnily enough, the controller has this implementation already :)

jannfis avatar Aug 05 '22 11:08 jannfis

@FireDrunk I think you'll have to compile. I don't believe images in PRs are pushed anywhere.

But the make image make target is very easy/reliable in my experience.

Unfortunately building the images locally is not working. In our 'enterprise' environment we have to work with a HTTP_PROXY which is not working correctly :(

This means I can't easily test this feature. But I'm hoping this will get released soon :)

FireDrunk avatar Aug 08 '22 13:08 FireDrunk

this feature will be awesome !

aelbarkani avatar Aug 08 '22 14:08 aelbarkani

@jannfis After some hassle I've been able to build the image and test it in our regular ArgoCD Project. I've build the image from your branch, and applied it to in our current Kustomize project which is running ArgoCD v2.4.7

I'm not sure if this is related to the PR, but I'm getting errors:

time="2022-08-09T11:12:24Z" level=info msg="ArgoCD API Server is starting" built="2022-08-09T09:32:06Z" commit=08a33053288f554010093d8d3ae38386095c6ff5 namespace=argocd port=8080 version=v2.4.0+08a3305
time="2022-08-09T11:12:24Z" level=info msg="Starting configmap/secret informers"
W0809 11:13:24.329838       1 reflector.go:324] pkg/mod/k8s.io/[email protected]/tools/cache/reflector.go:167: failed to list *v1.Secret: Get "https://10.96.0.1:443/api/v1/namespaces/argocd/secrets?limit=500&resourceVersion=0": context deadline exceeded
I0809 11:13:24.329949       1 trace.go:205] Trace[1296404218]: "Reflector ListAndWatch" name:pkg/mod/k8s.io/[email protected]/tools/cache/reflector.go:167 (09-Aug-2022 11:12:24.328) (total time: 60001ms):
Trace[1296404218]: ---"Objects listed" error:Get "https://10.96.0.1:443/api/v1/namespaces/argocd/secrets?limit=500&resourceVersion=0": context deadline exceeded 60000ms (11:13:24.329)
Trace[1296404218]: [1m0.001037166s] [1m0.001037166s] END
E0809 11:13:24.329973       1 reflector.go:138] pkg/mod/k8s.io/[email protected]/tools/cache/reflector.go:167: Failed to watch *v1.Secret: failed to list *v1.Secret: Get "https://10.96.0.1:443/api/v1/namespaces/argocd/secrets?limit=500&resourceVersion=0": context deadline exceeded
W0809 11:13:24.329840       1 reflector.go:324] pkg/mod/k8s.io/[email protected]/tools/cache/reflector.go:167: failed to list *v1.ConfigMap: Get "https://10.96.0.1:443/api/v1/namespaces/argocd/configmaps?labelSelector=app.kubernetes.io%2Fpart-of%3Dargocd&limit=500&resourceVersion=0": context deadline exceeded
I0809 11:13:24.330019       1 trace.go:205] Trace[636545905]: "Reflector ListAndWatch" name:pkg/mod/k8s.io/[email protected]/tools/cache/reflector.go:167 (09-Aug-2022 11:12:24.328) (total time: 60001ms):
Trace[636545905]: ---"Objects listed" error:Get "https://10.96.0.1:443/api/v1/namespaces/argocd/configmaps?labelSelector=app.kubernetes.io%2Fpart-of%3Dargocd&limit=500&resourceVersion=0": context deadline exceeded 60000ms (11:13:24.329)
Trace[636545905]: [1m0.001119162s] [1m0.001119162s] END
E0809 11:13:24.330032       1 reflector.go:138] pkg/mod/k8s.io/[email protected]/tools/cache/reflector.go:167: Failed to watch *v1.ConfigMap: failed to list *v1.ConfigMap: Get "https://10.96.0.1:443/api/v1/namespaces/argocd/configmaps?labelSelector=app.kubernetes.io%2Fpart-of%3Dargocd&limit=500&resourceVersion=0": context deadline exceeded

Edit: Ah, I can reproduce this issue on v2.4.0. Perhaps it's good to rebase this PR on the v2.4.7 release?

FireDrunk avatar Aug 09 '22 11:08 FireDrunk

@FireDrunk thanks for testing! This PR should contain all fixes where are in v2.4.7, since I believe Jann is rebasing on master.

@jannfis looks like this PR was a victim of my merge spree yesterday. More conflicts. :-P

crenshaw-dev avatar Aug 09 '22 14:08 crenshaw-dev

Thanks for the heads-up. I rebased and also added more e2e tests. I found one bug during the testing regarding Helm applications in a different namespace, which I addressed with https://github.com/argoproj/argo-cd/pull/9755/commits/dcde56cc30a7bd4761e513b8e896f5a964c4841e

jannfis avatar Aug 09 '22 17:08 jannfis

I've pulled & rebuilt the image, but I'm still getting the startup errors on the argocd-server. I'm also still seeing 2.4.0 as the version number.

time="2022-08-10T10:00:36Z" level=info msg="ArgoCD API Server is starting" built="2022-08-10T09:51:46Z" commit=6ea79b6e456d262b7946691c008a90dc4a250f27 namespace=argocd port=8080 version=v2.4.0+6ea79b6

Does that mean that the rebase didn't work? Or is that expected?

FireDrunk avatar Aug 10 '22 10:08 FireDrunk

The 2.4.0 version is still expected at this point in time. We should probably bump it to 2.5.0 in the meanwhile :) However, the Git commit SHA in your version identifier seems odd.

As for RBAC errors, you will also need to apply the new RBAC rules included with this PR if you haven't applied the full set of manifests, i.e.

kubectl apply -n argocd -k manifests/cluster-rbac/

Since I just merged this into master branch, you should be able to build off the master branch (or use quay.io/argoproj/argocd:latest now that it's been built & pushed) .

jannfis avatar Aug 10 '22 10:08 jannfis

Aah, I missed those new RBAC rules! I was just pulling in the latest image, and will try to verify again! Thanks!

FireDrunk avatar Aug 10 '22 11:08 FireDrunk

@jannfis I'm trying to understand the --application-namespaces setting that I'm required to set. Does this mean that I have to update this parameter for every namespace in every AppProject that I'm also adding as a sourceNamespace? Because that's really inconvenient, because that would mean that ArgoCD would need a restart everytime I add an AppProject, and I would also have to add all those namespaces (which could be a lot in our case) comma seperated to that variable (which would make it very error-prone)?


I've run into a secondary issue: It seems that it's no longer possible to create Applications via the UI when using this option. ArgoCD attempts to (correctly) create the Application Resource Object in the sourceNamespace of the project, but it's not allowed via it's Role + RoleBinding. I think that's because it should be a ClusterRoleBinding? The error in the UI:

Unable to create application: error creating application: applications.argoproj.io is forbidden: 
User "system:serviceaccount:argocd:argocd-server" cannot create resource "applications" in API group "argoproj.io" in the namespace "<sourceNamespace>"

The clusterrole currently in master:

kind: ClusterRole
metadata:
  labels:
    app.kubernetes.io/name: argocd-server
    app.kubernetes.io/part-of: argocd
    app.kubernetes.io/component: server
  name: argocd-server
rules:
- apiGroups:
  - '*'
  resources:
  - '*'
  verbs:
  - delete  # supports deletion a live object in UI
  - get     # supports viewing live object manifest in UI
  - patch   # supports `argocd app patch`
- apiGroups:
  - ""
  resources:
  - events
  verbs:
  - list    # supports listing events in UI
- apiGroups:
  - ""
  resources:
  - pods
  - pods/log
  verbs:
  - get     # supports viewing pod logs from UI
- apiGroups:
  - "argoproj.io"
  resources:
    - "applications"
  verbs:
    - get
    - list
    - watch

Doesn't contain the create verb.

FireDrunk avatar Aug 12 '22 08:08 FireDrunk

@FireDrunk Thanks a lot for testing in this early stage and for providing feedback! Much appreciated!

The --application-namespaces parameter can contain wildcards. To enable all namespaces in the cluster where Argo CD is running in, you can use --application-namespaces "*", or to allow only a subset of namespaces you can use --application-namespaces dev-* to allow only namespaces prefixed with dev-. Then, you can also use any values for the sourceNamespaces setting in the AppProject,

As for the ClusterRole, good catch! The permissions should probably include create, delete , update and patch as well, I will submit a PR.

jannfis avatar Aug 12 '22 09:08 jannfis

Ah, that makes a lot more sense! I'll use the wildcard!

FireDrunk avatar Aug 12 '22 10:08 FireDrunk

As for the ClusterRole, good catch! The permissions should probably include create, delete , update and patch as well, I will submit a PR.

@jannfis not sure how you planned to implement this, but rather than allowing create/update/delete in all namespaces by default, could we just document the RBAC change?

I'm thinking of things like Intuit's main Argo CD instance, where we maintain Argo CD instances that live in different namespaces on the same cluster. It would be nice to limit the write capability to least-necessary unless we decided to enable this feature.

crenshaw-dev avatar Aug 12 '22 14:08 crenshaw-dev