ssa: add DriftIgnoreRules to ApplyOptions for field-level ignore on apply#1157
ssa: add DriftIgnoreRules to ApplyOptions for field-level ignore on apply#1157dipti-pai wants to merge 1 commit intofluxcd:mainfrom
Conversation
877f705 to
4849129
Compare
matheuscscp
left a comment
There was a problem hiding this comment.
The code is looking great to me, but I'd like to better understand how applying the ignore rules after dry-run works. I'm not saying it's not correct at all, just saying that it's probably correct and I need to catch up and understand the whole flow. It would be great if we could talk about this in the next dev meeting 🙏
|
Thanks @matheuscscp for reviewing. Adding a few notes below regarding dry run behavior with ssa and potential improvements in the current version of the code to make it easier to discuss during the dev meeting tomorrow. Dry-run behavior with DriftIgnoreRules dryRunApply always sends the complete desired object (including fields targeted by DriftIgnoreRules) to the API server with ForceOwnership. This is intentional - The API server validates the full payload — missing required fields like The ignored fields are only stripped later — from the apply payload (not the dry-run), and only for existing objects (not creates). Summarizing the full behavior for different scenarios and couple of improvements -
My opinion is to take the improvements in 5. Improvement 6 is debatable - if a field is in |
|
I think we should strive to avoid applying objects if only the ignored fields have drifted, and always return |
If I understand this correctly, this is what @stefanprodan commented about above. I agree, we should make this improvement and do not apply if only ignored fields have drifted.
This improvement also seems quite important. |
4849129 to
43c3b28
Compare
|
With the latest iteration, the improvements 5. and 6. in this comment are implemented. Summarizing the new behavior
Before comparing the existing object with the dry-run result,
When an apply does happen (because non-ignored fields changed), we compute which ignored paths have actually drifted between live object and dry-run. Only those drifted ignored paths are removed from the apply payload via JSON patch. This means --
Because of these changes, a new scenario arises which I want to call out - Flux does not update an ignored field even when its own source changes. For example,
With the current changes, if |
Small but important correction here (if I understood the current iteration correctly; please correct me if I'm wrong, @dipti-pai): when values match, Flux does reclaim ownership even if another manager already owns the field. Stripping only on real drift keeps things simple (no need to parse |
@matheuscscp, yes this is true because we only strip the fields when they drift. So, if they converge, Flux reclaims ownership. Note this happens only when |
LGTM 👍 Are there any outstanding items left for us to address from your initial list? |
…pply Signed-off-by: Dipti Pai <diptipai89@outlook.com>
43c3b28 to
2088409
Compare
The things we discussed last dev meeting are addressed in the latest iteration. Made the changes for the AI-generated review comments, they look solid and rebased on main. Thanks so much for reviewing! |
Changes include
[]jsondiff.IgnoreRuletossaApplyOptionscreatecode paths. InApplyandApplyAllcode paths that correspond toupdate, remove ignored fields matching the ignored rules from the object.Includes tests for various scenarios for optional and mandatory fields, claiming shared/forceful ownership and orphaning owned fields without a manager.
Associated kustomize controller draft PR testing the changes from controller perspective - fluxcd/kustomize-controller#1627
Fixes: #696