fix(engine): idempotent talm apply for object-array and merge:replace fields#139
Merged
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces idempotent apply logic to prevent entry duplication during repeated configuration runs. The core change involves enhancing the pruning mechanism to recursively descend into object arrays using identity-based matching (e.g., for network interfaces and VLANs) and skipping pruning for paths marked with merge:"replace" to avoid data loss on partial edits. The PR includes comprehensive tests covering these new behaviors and updates the documentation to reflect the idempotent apply process. I have no feedback to provide.
IvanHunters
approved these changes
May 7, 2026
|
Warning Rate limit exceeded
To continue reviewing without waiting, purchase usage credits in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
IvanHunters
approved these changes
May 7, 2026
Add four MergeFileAsPatch subtests covering the scenarios where Talos's strategic merge appends primitive lists nested inside object arrays: - machine.network.interfaces[].addresses + routes (partial edit) - machine.network.interfaces[].vlans[].addresses (nested) - cluster.apiServer.admissionControl[].configuration.exemptions.namespaces - regression-safety: body adds a new interface, rendered interface's addresses must not duplicate All four currently fail. The matched object-array element (by interface, vlanId, or name) reaches configpatcher.Apply with rendered-side primitives still present; strategic merge appends them, doubling every rendered entry per apply round-trip. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
Extend pruneIdenticalKeys to descend into object-array elements
matched by their identity field, recurse through the matched pair,
drop items reduced to nothing, and re-attach identity keys that the
inner deep-equal pass stripped so the upstream merge can still match.
Without this descent, configpatcher.Apply matches an object-array
element by its identity field upstream (interface, vlanId, name) and
recurses inside, but the inner primitive arrays nested in the matched
element append rather than replace. Every apply round-trip then
doubles every interface address, route, vlan address, and
admissionControl exemption namespace.
Identity fields are looked up by YAML path against a static table
covering the v1alpha1 List types whose custom Merge method matches
by identity AND whose elements would mishandle inner-primitive
append on partial edit:
- machine/network/interfaces — NetworkDeviceList.Merge by interface
or deviceSelector (body-driven switch).
- machine/network/interfaces/vlans — VlanList.Merge by vlanId.
- cluster/apiServer/admissionControl — AdmissionPluginConfigList.Merge
by name.
Other object arrays (extraVolumes, kernel.modules, wireguard.peers,
authorizationConfig, ...) have no custom upstream Merge; the patcher
appends body to rendered, so re-attaching an identity key would not
avoid the append — those paths fall through to the deep-equal
fallback in matchObjectArrayItem, which drops body items that
byte-equal a rendered counterpart and covers the dominant
full-restate case.
ConfigFileList (typed ExtensionServiceConfig.configFiles, identity
mountPath) is intentionally omitted: ConfigFile carries only string
fields, so the upstream merge.Merge field-by-field plus the
deep-equal fallback already produce the right result without a
path-keyed identity match.
Object arrays without a single primary key (most notably
machine/network/interfaces[].routes) sit in the same fallback bucket
— two items are the same only if every field matches, the only
"same item" semantic the schema admits when no key is declared.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
Add three MergeFileAsPatch subtests covering the v1alpha1 fields tagged `merge:"replace"` upstream: - cluster.network.podSubnets - cluster.network.serviceSubnets - cluster.apiServer.auditPolicy (Unstructured map) For these fields the upstream patcher overwrites rendered's value with body's verbatim — unless body is the zero value, in which case rendered survives. The current pruneIdenticalKeysAt subtracts rendered-side primitives from body's slice (and recursively strips deep-equal keys from body's map), reducing body to just the user's deltas. The upstream replace then writes those deltas as the entire final value, and rendered's other entries / map keys silently vanish — partial-edit on podSubnets that adds a CIDR ends up losing the original; partial-edit on auditPolicy that adds a rule loses the rendered apiVersion/kind/other rules. All three currently fail. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
Add replaceSemanticPaths — the YAML paths in the v1alpha1 schema where the upstream merge layer is annotated `merge:"replace"`. At those paths the patcher overwrites rendered's value with body's verbatim (unless body is zero-valued), so any prune that subtracts entries from body's primitive list, recurses into body's map, or descends into body's object array reduces body to the user's deltas only — the upstream replace then writes those deltas as the final value and rendered's other entries / map keys silently vanish. Skip the partial-edit branches at those paths so body reaches the upstream merge intact. The deep-equal short-circuit at the top of pruneIdenticalKeysAt is still safe: when body byte-equals rendered, deleting the body key reduces body to the zero value at that path and the upstream replace leaves rendered untouched. Three v1alpha1 fields are tagged today: - cluster.network.podSubnets - cluster.network.serviceSubnets - cluster.apiServer.auditPolicy Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
Repeated talm apply runs against an already-configured node do not duplicate entries: the engine prunes from the body every primitive list entry the rendered template already carries and every object-array element that deep-equals a rendered counterpart, descending into pairs that the upstream patcher merges by identity to dedupe their inner primitive lists too. Pin this in the per-node patches section so users running 'talm template -I' do not have to discover from the test suite that the contract exists. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
Add coverage for behavioural contracts that are otherwise only exercised indirectly through the existing integration tests: - TestHasIdentityValue pins the boundary cases of the body-driven identity selector (empty string, empty map, empty slice, nil, zero int, bool) so a future change cannot collapse a body without identity onto the wrong rendered element. - TestObjectArrayMergeKeysMatchesUpstreamMergerSurface and TestReplaceSemanticPathsMatchesUpstreamReplaceTags lock both lookup tables against the upstream merger / merge-tag surface so a stray addition or omission surfaces here at test time rather than as a silent idempotence regression. - NetworkDefaultActionConfig.ingress integration test pins that the flat path-based replace-skip is harmless when a different typed-doc kind shares the bare path 'ingress' with NetworkRuleConfig: the collision is benign as long as the colliding field stays scalar. - NetworkRuleConfig multi-doc pairing test pins that the documentIdentity tuple includes 'name', so two NetworkRuleConfig docs with different names are not collapsed by the prune. - 'interface body without identity field reaches merge intact' test pins that hasIdentityValue rejection of empty fields prevents a body item with no interface or deviceSelector from being matched onto the wrong rendered element. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
lexfrei
force-pushed
the
fix/object-array-dedup-on-apply
branch
from
0de4081 to
29f645c
Compare
IvanHunters
approved these changes
May 7, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changed
talm applynow preserves rendered entries when the user re-states or partially edits per-node config. The engine's pre-merge prune was correctly deduplicating primitive arrays at the top level, but had two gaps that landed as silent data loss across repeated applies:merge:"replace"fields were silently overwritten on partial edit. The prune subtracted rendered entries from body, reducing body to just the user's deltas; the upstream replace then wrote those deltas as the entire final value, losing rendered's other entries.Why
Both regressions are reachable through the standard
talm template -Iworkflow that writes the rendered template back as the per-node body.How
merge:"replace"fields so body reaches the patcher verbatim.Tests
Eleven new integration tests + unit tests for the identity selector and table contracts. README documents the round-trip dedup contract.
Closes #138.
Closes #77.
Closes #28.