fix: update normalization for ignoreDifferences on server-side diff #747
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #747 +/- ##
==========================================
- Coverage 54.26% 47.58% -6.68%
==========================================
Files 64 64
Lines 6164 6573 +409
==========================================
- Hits 3345 3128 -217
- Misses 2549 3188 +639
+ Partials 270 257 -13 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: Peter Jiang <[email protected]>
Signed-off-by: Peter Jiang <[email protected]>
Signed-off-by: Peter Jiang <[email protected]>
Signed-off-by: Peter Jiang <[email protected]>
pjiang-dev
force-pushed
the
pjiang/ignorediff-ssd
branch
from
c229d82 to
164adfc
Compare
Signed-off-by: Peter Jiang <[email protected]>
Signed-off-by: Peter Jiang <[email protected]>
pjiang-dev
changed the title
Signed-off-by: Peter Jiang <[email protected]>
|
| // Skip the normalizer (ignoreDifferences + knownTypes) for server-side diff | ||
| // In the case an ignoreDifferences field is required, it needs to be be present in the config | ||
| // before server-side diff is calculated and normalized before final comparison. | ||
| if o.applyIgnoreDifferences { | ||
| err := o.normalizer.Normalize(un) | ||
| if err != nil { | ||
| o.log.Error(err, fmt.Sprintf("Failed to normalize %s/%s/%s", un.GroupVersionKind(), un.GetNamespace(), un.GetName())) | ||
| } |
There was a problem hiding this comment.
This is a public function and is likely called by Argo CD. Maybe we should add a configuration to skip the normalization instead of applying it. This way, it will be backward compatible and we only set the skip option during server-side diff. WDYT?
There was a problem hiding this comment.
What this is doing is skipping this public Normalization function before calculating predicted live for server-side diff. Then after predicted live is calculated, we do the public Normalize:
gitops-engine/pkg/diff/diff.go
Line 180 in 41d976c
I don't think we can fully skip the Normalization for server-side diff still as it would then show diffs for ignoreDifferences fields.
| } | ||
| if live != nil { | ||
| live = remarshal(live, o) | ||
| Normalize(live, opts...) | ||
| Normalize(live, append(opts, WithApplyIgnoreDifferences(false))...) |
There was a problem hiding this comment.
This might be causing a regression with the 2-way diff and 3-way diff below in this method. When 3-way diff is called, the Normalization with IgnoreDifference=true will be done before calling ThreeWayDiff only for the obj in the last-applied-configuration. However, Normalize is not called anymore for live and config, so o.normalizer.Normalize(un) will never be called anymore for 2-way and 3-way diffs. While ServerSideDiff does call Normalize again for live and config, it is not the case for the TwoWayDiff/ThreeWayDiff method.
Normalize(orig, opts...)
dr, err := ThreeWayDiff(orig, config, live)
return TwoWayDiff(config, live)
My suggestion would be to call normalize based on serverSideDiff option, and preserve the behaviour for other kind of diff. I assume RespectIgnoreDifference is important for 2-way and 3-way diff, and I feel tests are missing for this.
preDiffOpts := opts
o := applyOptions(opts)
if o.serverSideDiff {
preDiffOpts = append(preDiffOpts, WithApplyIgnoreDifferences(false))
}
if config != nil {
config = remarshal(config, o)
Normalize(config, preDiffOpts...)
}
if live != nil {
live = remarshal(live, o)
Normalize(live, preDiffOpts...)
}There was a problem hiding this comment.
Right. I missed that this Diff function is shared with the other diff methods.
fixes argoproj/argo-cd#17362 (comment)
When ignoreDifferences normalization removes fields before server-side apply, it causes validation errors because required fields (like image) are missing from the config sent to the Kubernetes API.
To avoid this issue we will
applyIgnoreDifferencewhen performing server-side apply --dry-run