Fix Labels and Annotations reconcile #4253
Conversation
Signed-off-by: Yuri Oliveira <[email protected]>
| if err := mergeWithOverride(&existing.Labels, desired.Labels); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| if err := mergeWithOverride(&existing.Annotations, desired.Annotations); err != nil { | ||
| return err | ||
| } | ||
|
|
There was a problem hiding this comment.
@yuriolisa there was a reason we did this that i'm trying to remember... AH! it was because if the user has an external controller like argocd, OPA, istio which adds labels/annotations we could inadvertently override them causing an infinite cycle. I think it's something in this pr where we talked about this. @davidhaja may remember better...
There was a problem hiding this comment.
@jaronoff97 , thank you for the heads-up. I'm just wondering why the PR says that would be changing to mergeWithOverwriteWithEmptyValue, but it remains mergeWithOverride. And also, how should we handle this situation because on one hand, we solve an issue related to the external controller, but if a user wants to set some annotations/labels, it won't be a good experience.
There was a problem hiding this comment.
so they can set annotations and labels fine, the issue is the removal. I'm not sure there's a good way to balance this... going to think on this.
There was a problem hiding this comment.
One idea would be creating a new field to set which labels and annotations prefixes should be preserved.
preservedAnnotationPrefixes:
- argocd.argoproj.io
- istio.io
E2E Test Results 33 files ±0 221 suites ±0 4h 6m 38s ⏱️ + 16m 49s For more details on these failures, see this check. Results for commit 9f6815f. ± Comparison against base commit 5fd4c4b. |
Description:
Fixing a issue during the reconcile process for Labels and Annotations.
Link to tracking Issue(s):
Testing:
Documentation: