fix: unable to delete an Application if its target cluster is deleted, Argo CD enters infinite app deletion reconciliation loop #6557
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…, Argo CD enters infinite app deletion reconciliation loop Signed-off-by: Jonathan West <jonwest@redhat.com>
jgwest
force-pushed
the
infinite-delete-5817
branch
from
6f48002
to
3906c5b
Compare
Codecov Report
@@ Coverage Diff @@
## master #6557 +/- ##
==========================================
+ Coverage 41.14% 41.27% +0.12%
==========================================
Files 153 156 +3
Lines 20313 20677 +364
==========================================
+ Hits 8358 8534 +176
- Misses 10796 10936 +140
- Partials 1159 1207 +48
Continue to review full report at Codecov.
|
jgwest
commented
Jun 24, 2021
|
|
||
| filteredObjs := FilterObjectsForDeletion(objs) | ||
| objsMap, err := ctrl.getPermittedAppLiveObjects(app, proj) | ||
| if err != nil { |
There was a problem hiding this comment.
For reviewers: FYI the code inside this if block is unchanged from the original (there aren't any changes hidden in there 😄 ), with the only exception being moving the call to ctrl.db.GetCluster to above the block.
jannfis
reviewed
Jun 29, 2021
Comment on lines
+1570
to
+1575
| // This call to 'ValidateDestination' ensures that the .spec.destination field of all Applications | ||
| // returned by the informer/lister will have server field set (if not already set) based on the name. | ||
| // (or, if not found, an error app condition) | ||
|
|
||
| // If the server field is not set, set it based on the cluster name; if the cluster name can't be found, | ||
| // log an error as an App Condition. |
jannfis
reviewed
Jun 29, 2021
Comment on lines
+2406
to
+2411
| // An ApplicationDestination has an 'inferred server' if the ApplicationDestination | ||
| // contains a Name, but not a Server URL. In this case it is necessary to retrieve | ||
| // the Server URL by looking up the cluster name. | ||
| // | ||
| // As of this writing, looking up the cluster name, and setting the URL, is | ||
| // performed by 'utils.ValidateDestination(...)', which then calls SetInferredServer. |
jannfis
reviewed
Jun 29, 2021
Comment on lines
+292
to
+300
| // ValidateDestination sets the 'Server' value of the ApplicationDestination, if it is not set. | ||
| // NOTE: this function WILL write to the object pointed to by the 'dest' parameter. | ||
| // | ||
| // If an ApplicationDestination has a Name field, but has an empty Server (URL) field, | ||
| // ValidationDestination will look up the cluster by name (to get the server URL), and | ||
| // set the corresponding Server field value. | ||
| // | ||
| // It also checks: | ||
| // - If we used both name and server then we return an invalid spec error |
|
Previously verified downstream: argoproj/applicationset#175 (comment) so removing tag. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
On Application deletion, this PR verifies whether the target cluster (defined in the
destinationfield of theApplicationresource) is defined within Argo CD (exists as a cluster secret, or is a local cluster). If the target cluster is not defined, it allows the Application to be deleted (rather than returning an error, as before).Details
If the target cluster is not defined, it is assumed that either:
a) the cluster was deleted from Argo CD (for example, via the Argo CD settings UI, or by deleting the cluster secret)
or
b) the Application spec is invalid (referencing a cluster that doesn't exist).
In both cases, since there are no cluster resources to clean up, we can safely allow the
Applicationresource to be deleted.So, if we detect that the Application targets a valid cluster:
OTOH, if we detect the Application contains an invalid cluster:
Applicationresource to be deleted (eg we remove the finalizer from the Application)See parent issue for reproduction steps and details.
Fixes #5817
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: