feat: handle UID mismatch between incoming and existing resources #242
Conversation
chetan-rns
requested review from
jannfis,
jgwest and
ishitasequeira
as code owners
Signed-off-by: Chetan Banavikalmutt <[email protected]>
chetan-rns
force-pushed
the
source-uid-annotation
branch
from
4c92f1b to
61b7ef0
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #242 +/- ##
==========================================
+ Coverage 47.21% 48.56% +1.34%
==========================================
Files 55 55
Lines 4882 4973 +91
==========================================
+ Hits 2305 2415 +110
+ Misses 2401 2361 -40
- Partials 176 197 +21 ☔ View full report in Codecov by Sentry. |
| if !sourceUIDMatch { | ||
| logCtx.Debug("An app already exists with a different source UID. Deleting the existing app") | ||
| if err := a.deleteApplication(incomingApp); err != nil { | ||
| return err |
There was a problem hiding this comment.
I think we should give more context to the returned error here.
The caller will expect an application to be created, and probably reflect that in any error message logged, but could potentially receive an error from the delete operation.
For example, the following case would be confusing (admittedly, the chance for this happening is very slim, but it should ):
- A create event is sent for an application where
sourceUIDMatchis false - The application is deleted on the cluster by a third party
a.deleteApplicationfails because the resource already is deleted. The error would be "the requested resource could not be found"- That error is passed back to the caller
- The error log for the caller would look similar to "Could not create app foo: requested resource not found"
Or think about RBAC - agent might have RBAC to create applications, but not delete applications, but the create call would return a permission denied. Troubleshooting that would be fun :)
So it would make more sense to augment the returned error by more information, e.g. fmt.Errorf("could not delete existing resource prior to creation: %w", err) or something similar.
| if !sourceUIDMatch { | ||
| logCtx.Debug("Source UID mismatch between the incoming app and existing app. Deleting the existing app") | ||
| if err := a.deleteApplication(incomingApp); err != nil { | ||
| return err |
There was a problem hiding this comment.
Same about augmenting the error as above.
| if !sourceUIDMatch { | ||
| logCtx.Debug("An appProject already exists with a different source UID. Deleting the existing appProject") | ||
| if err := a.deleteAppProject(incomingAppProject); err != nil { | ||
| return err |
There was a problem hiding this comment.
Same about augmenting the error as above.
| if !sourceUIDMatch { | ||
| logCtx.Debug("Source UID mismatch between the incoming and existing appProject. Deleting the existing appProject") | ||
| if err := a.deleteAppProject(incomingAppProject); err != nil { | ||
| return err |
There was a problem hiding this comment.
Same about augmenting the error as above.
| @@ -287,7 +334,7 @@ func (a *Agent) deleteAppProject(project *v1alpha1.AppProject) error { | |||
| // means that we're out-of-sync from the control plane. | |||
| // | |||
| // TODO(jannfis): Handle this situation properly instead of throwing an error. | |||
| if !a.appManager.IsManaged(project.Name) { | |||
| if !a.projectManager.IsManaged(project.Name) { | |||
What does this PR do / why we need it:
Currently, we don't handle a case where the incoming app/app project could have a different source UID than the resource on the cluster with the same name and namespace. This indicates that the existing resource in the agent was created from a different resource in principal which no longer exists. We annotate the resources in the agent with the source UID to track their source of truth.
For more details: #225 (comment)
Which issue(s) this PR fixes:
Fixes #225 (comment)
How to test changes / Special notes to the reviewer:
Checklist