chore(refactor): remove app destination inferrence logic

[crenshaw-dev]

The utils.ValidateDestination function has the side-effect of setting the app destination's server URL if it is not present.

This side effect makes the code hard to read and has introduced subtle bugs and race conditions.

This PR is an attempt to remove the side effect and instead have the code load in the cluster details whenever they're needed. It may result in more network calls, so we need to be aware of performance. We also need to look out for places that still assume the old side-effect behavior and fix them to explicitly pull the cluster details instead.

The code is called in five places in the app controller:

• in finalizeApplicationDeletion; since not all code paths in processAppOperationQueueItem require a destCluster, we delegate that to finalizeApplicationDeletion when an app deletion is in progress
• in processRequestedAppOperation, which is another code path accessible from processAppOperationQueueItem; we call it as late as possible, and we need it in order to call SyncAppState; there are some error paths in SyncAppState which don't require destCluster, but I think they'll be relatively rare
• in processAppRefreshQueueItem; here again there are a few code paths which could potentially short-circuit the need to get the cluster, but the most common paths require it
• in canProcessApp, which is called in 5 places; I'd much rather avoid this work for most of those cases, but that requires a bigger refactor
• in the app informer's namespace index key generation function; this is another place that call doesn't belong, but it's a bigger refactor

[bunnyshell]

🔴 Preview Environment stopped on Bunnyshell

See: Environment Details | Pipeline Logs

Available commands (reply to this comment):

• 🔵 /bns:start to start the environment
• 🚀 /bns:deploy to redeploy the environment
• ❌ /bns:delete to remove the environment

[codecov]

Codecov Report

Attention: Patch coverage is 66.33663% with 68 lines in your changes missing coverage. Please review.

Project coverage is 55.24%. Comparing base (947a7b8) to head (efd74ee).
Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
controller/state.go	50.00%	6 Missing and 3 partials ⚠️
server/application/application.go	50.00%	5 Missing and 4 partials ⚠️
cmd/argocd/commands/admin/cluster.go	27.27%	7 Missing and 1 partial ⚠️
controller/appcontroller.go	85.36%	3 Missing and 3 partials ⚠️
controller/cache/cache.go	71.42%	5 Missing and 1 partial ⚠️
server/project/project.go	33.33%	5 Missing and 1 partial ⚠️
util/argo/argo.go	83.33%	4 Missing and 2 partials ⚠️
controller/sync.go	58.33%	4 Missing and 1 partial ⚠️
cmd/argocd/commands/admin/app.go	50.00%	3 Missing and 1 partial ⚠️
pkg/apis/application/v1alpha1/app_project_types.go	66.66%	2 Missing and 1 partial ⚠️
... and 3 more

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #21189      +/-   ##
==========================================
+ Coverage   55.21%   55.24%   +0.03%     
==========================================
  Files         337      337              
  Lines       56945    56843     -102     
==========================================
- Hits        31441    31405      -36     
+ Misses      22828    22756      -72     
- Partials     2676     2682       +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[crenshaw-dev]

Note to self: there are probably a lot of code paths where we just need the destServerURL, not the whole Cluster object. Consider simplifying those code paths.

[agaudreault]

LGTM. Minor comments. Other people should review this I think because the potential side-effects are extremely hard to spot! Hopefully, we have enough unit test :)

[agaudreault]

I feel like it would be better to do the inference again in CompareAppState instead of adding a parameter here. Or maybe have a private property with an accessor in the app destination object?

[agaudreault]

As discussed, this is a refactor and can be improved further in another PR. No blocker.

[leoluz]

From the description this seems to be a breaking change. Can you please confirm?

[crenshaw-dev]

@leoluz not breaking, behavior should be 100% identical from a user's perspective.

[leoluz]

Is this constraint existing at the CRD level today? I suspect that nothing fails today if both fields are present. Can you please confirm?

[crenshaw-dev]

Not exactly at the CRD level, but it is currently enforced at runtime: https://github.com/argoproj/argo-cd/pull/21189/files#diff-87b3b573e8c40af95af4dbda83f7dbc467219b02182415ff8e1f0ad73a82dc59L518

I tested locally in 2.14.0-rc5. If I set both fields, the app goes into an Unknown state with an error application destination can't have both name and server defined.