feat: add deny destinations for projects (#9464)

[blakepettersson]

This adds the ability to selectively deny destinations, by prefixing
either its namespace or server with a !. Closes #9464.

Note on DCO:

If the DCO action in the integration test fails, one or more of your commits are not signed off. Please click on the Details link next to the DCO action for instructions on how to resolve this.

Checklist:

• Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this does not need to be in the release notes.
• The title of the PR states what changed and the related issues number (used for the release note).
• I've included "Closes [ISSUE #]" or "Fixes [ISSUE #]" in the description to automatically close the associated issue.
• [ ] I've updated both the CLI and UI to expose my feature, or I plan to submit a second PR with them.
• Does this PR require documentation updates?
• I've updated documentation as required by this PR.
• Optional. My organization is added to USERS.md.
• I have signed off all my commits as required by DCO
• I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
• My build is green (troubleshooting builds).

[codecov]

Codecov Report

Merging #9652 (945fda9) into master (2a3c692) will increase coverage by 0.04%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #9652      +/-   ##
==========================================
+ Coverage   45.75%   45.80%   +0.04%     
==========================================
  Files         227      227              
  Lines       26961    26976      +15     
==========================================
+ Hits        12337    12356      +19     
+ Misses      12943    12939       -4     
  Partials     1681     1681              

Impacted Files	Coverage Δ
pkg/apis/application/v1alpha1/app_project_types.go	57.28% <100.00%> (+5.44%)	⬆️
pkg/apis/application/v1alpha1/types.go	54.70% <100.00%> (ø)
util/settings/settings.go	48.22% <0.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2a3c692...945fda9. Read the comment docs.

[crenshaw-dev]

Thanks so much for working on this!

I think, as currently written, we have a design problem to overcome. Current behavior of the destinations field is that if any destination matches, the resource is allowed (IsDestinationPermitted is a for loop, exiting on the first matching destination). So the feature doesn't work as documented. Any destination will match the first / destination spec, and the loop will exit.

The suggestion in #9464 was to switch the destinations field to "all must match" mode if all the destinations were deny-type (start with a !). But 1) the implicit flip in behavior is weird to me, and 2) it doesn't account for cases like your example where one destination is "allow-type" and another is "deny-type".

Suggestion: Add a boolean field called allDestinationsMustMatch.

[blakepettersson]

Thanks so much for working on this!

Thanks for your feedback!

The suggestion in #9464 was to switch the destinations field to "all must match" mode if all the destinations were deny-type (start with a !). But 1) the implicit flip in behavior is weird to me, and 2) it doesn't account for cases like your example where one destination is "allow-type" and another is "deny-type".

Good points.

Suggestion: Add a boolean field called allDestinationsMustMatch.

Sounds good to me, you mean as a field on the AppProjectSpec right?

[crenshaw-dev]

you mean as a field on the AppProjectSpec right?

Yep! Adding fields can be tricky with all the codegen logic. Just lmk if you need help.

[blakepettersson]

Yep! Adding fields can be tricky with all the codegen logic. Just lmk if you need help.

Alright, so I added the field AllDestinationsMustMatch to AppProjectSpec and ran make codegen and I think everything has been added where it should have been added 🙏

[rishabh625]

i also think a e2e test should be added for this

[crenshaw-dev]

I'm gonna quibble on the docs, 'cause I think this is such an important feature that we definitely want to get the messaging right. But overall, let me reemphasize that this is awesome work. Literally sitting here grinning about this PR. 😁

[blakepettersson]

I'll also take a look into adding some E2E tests for this feature

[crenshaw-dev]

lgtm! I'm going to re-request a review from @rishabh625 and request another review from @leoluz since it's really important we get this right.

If you don't have time to write e2e tests at the moment, we can add a follow-up ticket to do so.

[blakepettersson]

lgtm! I'm going to re-request a review from @rishabh625 and request another review from @leoluz since it's really important we get this right.

Awesome, good stuff!! 🎉 🎉

If you don't have time to write e2e tests at the moment, we can add a follow-up ticket to do so.

In-between $dayjob and life I haven't had much time for anything else this week, but I did manage to do a few basic e2e tests. I'd be happy to punt the more involved ones for another PR though 😄

I also exposed allDestinationsMustMatch as a cli flag on project. I'll punt the CLI flag for a subsequent PR

[crenshaw-dev]

No worries, Life > $dayjob > Argo CD. :-)

[rishabh625]

Thanks @blakepettersson LGTM , just a nit

[crenshaw-dev]

@blakepettersson this is still very much on my radar. Leo is swamped, so I'm currently finding a third reviewer just to give it one more pass.

[jannfis]

Thanks a lot for this effort, @blakepettersson !

I was wondering whether we could just change the logic behind the decision making for allowing or denying a destination a little.

I find the allDestinationsMustMatch toggle a little counter-intuitive. Could we make the decision maybe like follows:

(Any allow pattern matches) AND (none of the deny pattern matches)

I think this would be more intuitive.

WDYT?

[crenshaw-dev]

+1, I think this would save us an unwieldy config param.

[blakepettersson]

@jannfis thanks for the review!

(Any allow pattern matches) AND (none of the deny pattern matches)

That's definitely more intuitive, and simplifies things quite a bit. I've now re-implemented that bit to do just that. I didn't really know how to best document the feature so I suspect that will need some TLC 😃

[crenshaw-dev]

One slight concern, otherwise lgtm!

[blakepettersson]

@crenshaw-dev @jannfis @rishabh625 does anything else need to be done on this PR?

[crenshaw-dev]

LGTM, thanks @blakepettersson!

I'll ping Jann for one last review.

[flaviomoringa]

Really hoping to get this on the next release ... "fingers-crossed"

[jannfis]

LGTM!

Thank you @blakepettersson and apologies for the delay in coming back to this!

[blakepettersson]

Awesome @jannfis and @crenshaw-dev, thanks a lot! 🎉 🎉 🎉 🎉

Thanks for approving this, and no worries about any delays 😄

[zbialik]

Thank you all!