-
Notifications
You must be signed in to change notification settings - Fork 116
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Don't delete resources unless field managers match #3407
base: master
Are you sure you want to change the base?
Conversation
Does the PR have any schema changes?Looking good! No breaking changes found. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3407 +/- ##
==========================================
+ Coverage 41.08% 41.12% +0.03%
==========================================
Files 87 87
Lines 12900 12917 +17
==========================================
+ Hits 5300 5312 +12
- Misses 7205 7208 +3
- Partials 395 397 +2 ☔ View full report in Codecov by Sentry. |
0a9b5d5
to
4817d79
Compare
4817d79
to
c0839d3
Compare
} | ||
actualSSAManagers = actualSSAManagers.Insert(f.Manager) | ||
} | ||
if c.ServerSideApply && !actualSSAManagers.Has(c.FieldManager) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As written this would also no-op in cases where the object was updated with CSA.
We can narrow this by also checking actualSSAManagers
is non-empty, so we know the live object was at least touched by pulumi.
Note that Helm uses labels to track ownership and to drive adoption. |
This fixes the delete-after-create problem by reviving #2999 using field managers (as Eron suggested) instead of annotations to confirm ownership before deleting a resource.
If the resource was created with SSA, and we don't find the expected field manager, then we no-op instead of deleting the resource. The intended effect is to remove it from our state so the "upserted" manager can continue owning it.
This is technically a breaking change in behavior for some edge cases, but I think it is justified by the potential severity of our delete-after-create bug.