AppProject deny namespaces does not respect cluster whitelist

[zbialik]

Checklist:

• I've searched in the docs and FAQ for my answer: https://bit.ly/argocd-faq.
• I've included steps to reproduce the bug.
• I've pasted the output of argocd version.

Describe the bug

I create an AppProject for each of our development teams and whitelist the cluster(s) they may deploy to using the spec.destinations[].server field. Additionally, I restrict what Namespaces they may deploy to. Since I don't care what Namespaces they create, a blacklist works better for me. As such, I started using the feature implemented by #9652, but have discovered a bug where it's not respecting the whitelisted cluster endpoints.

To Reproduce

1. create an AppProject like:

apiVersion: argoproj.io/v1alpha1
kind: AppProject
metadata:
  name: team01
spec:
  sourceRepos:
  - '*'
  destinations:
  # blacklist namespaces for all clusters
  - namespace: '!default'
    server: '*'
  - namespace: '!kube-system'
    server: '*'
  - namespace: '!monitoring'
    server: '*'
  - namespace: '!logging'
    server: '*'
  # whitelist clusters to deploy to (for non-blacklisted namespaces)
  - namespace: '*'
    server: 'cluster01'

2. create an Application like:

apiVersion: argoproj.io/v1alpha1
kind: Application
metadata:
  name: my-app
  namespace: argocd
spec:
  destination:
    name: cluster02 # this should be DENIED -- it maps to a server that is not whitelisted
    namespace: non-blacklisted-namespace
  project: team01

Expected behavior

The Application should not get deployed because the destination.name does NOT map to a spec.destinations[].server that's been whitelisted in the AppProject.

Version

v2.5.4

[zbialik]

@blakepettersson @crenshaw-dev as I know you worked on implementing the initial feature a bit.

Thanks for the help!

[crenshaw-dev]

I agree that the rules are a bit difficult to read, but I think the behavior matches the documentation.

As with sources, a destination is considered valid if the following conditions hold:

1. Any allow destination rule (i.e. a rule which isn't prefixed with !) permits the destination
2. AND no deny destination (i.e. a rule which is prefixed with !) rejects the destination

Your hypothetical app matches the first four destinations, because they each specify allow any destination server, so long as the app does not deploy to a disallowed namespace.

I think to solve your use case, you should duplicate the cluster1 destination server name in each destination.

[crenshaw-dev]

I think the example in the docs is misleading. The final destination of */* is unnecessary, because of one of the other two rules matches, there’s no need for an additional catch-all “allow” rule.

[zbialik]

I think to solve your use case, you should duplicate the cluster1 destination server name in each destination.
Ahh thanks yeah that'd probably do it. That's easy enough for me to implement.

I do think the rules are a bit difficult to understand. As you pointed out, the final destination of / isn't necessary and for me, that actually confused me more because I thought it was providing something to the rule.

I think showing maybe a few more examples of the deny rules might be beneficial. Or at least maybe the one I am using -- I imagine a rule to allow an AppProject to deploy to a single cluster but NOT these Namespaces is a common one folks may want.

[blakepettersson]

What examples would you want? I can remove the final "catch-all" rule to hopefully reduce some confusion, but what else? 😄

[crenshaw-dev]

a rule to allow an AppProject to deploy to a single cluster but NOT these Namespaces

Agreed. I need to double-check our glob library, but I think namespace: !{kube-system,argocd} should be possible.

[flaviomoringa]

I would love something like:
namespace: !caas*,!kube-system

But if:
namespace: !{caas*,kube-system}
works, then that is fine. Just please add it to the docs.

I was also adding the * as a catch all for every other namespaces :-(
.

[crenshaw-dev]

Project destination checking uses the gobwas glob library, which does allow {} lists of patterns.

(The fact that our code base uses multiple glob libraries is a problem that I want to fix.)

[crenshaw-dev]

@flaviomoringa what do you think of this?

[zbialik]

a rule to allow an AppProject to deploy to a single cluster but NOT these Namespaces

to accomplish this, is it true we'd only need the deny rule for the single cluster since the rule would inherit an allow rule for all other namespaces?

For example:

spec:
  destinations:
  - namespace: '!{default,kube-system,monitoring,logging}'
    server: 'cluster01'

I believe the above would only allow the AppProject to deploy to cluster01 and only to Namespaces that aren't included in that blacklist.

[flaviomoringa]

@flaviomoringa what do you think of this?

I would just make it even clearer that a single deny rule is enough to allow everything else with an example even simpler than the one you present (but also keeping that to show a more complicated case). Something like:

destinations
- namespace: '!kube-system'
   server: ' https://mycluster.local'

This allows a deploying anything to any server in any namespace, except deploying to namespace kube-system on mycluster.local.

But maybe we are starting to put too much information :-)

[crenshaw-dev]

This allows a deploying anything to any server in any namespace, except deploying to namespace kube-system on mycluster.local.

For the rule given, I don't think that's correct.

I think it allows deploying anything to the given server in any namespace except for kube-system on mycluster.local.

[flaviomoringa]

a rule to allow an AppProject to deploy to a single cluster but NOT these Namespaces

to accomplish this, is it true we'd only need the deny rule for the single cluster since the rule would inherit an allow rule for all other namespaces?

For example:

spec:
  destinations:
  - namespace: '!{default,kube-system,monitoring,logging}'
    server: 'cluster01'

I believe the above would only allow the AppProject to deploy to cluster01 and only to Namespaces that aren't included in that blacklist.

See... even I failed lolll... I was thinking a single deny rule existing would be enough to allow everything else and just deny what that rule said... being one or more clusters.

[blakepettersson]

I think #20045 would allow for a more intuitive behaviour.