fix(engine): treat $patch:delete on absent paths as no-op#146
Conversation
Add three MergeFileAsPatch subtests covering body-side $patch:delete directives whose target path does not exist in the rendered template: - absent at the top level (machine.nodeLabels missing entirely from rendered): MergeFileAsPatch must accept the directive as a no-op. - partially present (parent map present in rendered but the targeted leaf key absent): same no-op contract; sibling keys under the parent must survive untouched. - present (sanity probe): the user-intent delete on a path the rendered template does populate must still land as a Selector and remove the key from the merged config. The first two currently fail with `failed to delete path ... lookup failed` from configpatcher.Apply: its Selector-based deleteForPath walks the parsed v1alpha1.Config struct and errors when any path segment does not resolve. Kubernetes strategic merge patch treats delete-of-absent as a no-op; the fix must match that semantic so a chart-emitted directive (e.g. machine.nodeLabels.<label>: $patch: delete) does not break a fresh apply where the target struct has not yet acquired the key. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
…mantics)
Add stripPatchDeleteDirectivesAbsentInTarget — runs after the chart-side
strip pass and removes any body $patch:delete directive whose path does
not resolve to a key in the cleaned rendered template. configpatcher.Apply
otherwise errors with `failed to delete path '...': lookup failed`
because its Selector-based deleteForPath walks the parsed v1alpha1.Config
struct and rejects any path segment that does not resolve.
Kubernetes strategic merge patch treats delete-of-absent as a no-op; the
fix restores that semantic so the chart's own pattern (a body that
re-states a chart-emitted directive after `talm template -I`) does not
break a fresh apply where the targeted key has not yet been populated on
the node. The user-facing failure mode: a controlplane node-body restating
machine.nodeLabels.node.kubernetes.io/exclude-from-external-load-balancers:
$patch: delete failed on every fresh apply against a freshly generated
config because the freshly generated config does not contain
machine.nodeLabels at all.
The new helper pairs body and target documents by identity tuple
(apiVersion+kind+name, or the legacy-root sentinel) so a body re-ordering
its typed documents relative to rendered still resolves directive paths
against the right target document. A body document with no matching
target document gets every directive stripped — matching the upstream
contract: there is nothing to delete.
Three supporting helpers carry the bulk of the logic:
- collectDeleteDirectivePaths walks a YAML tree and returns the
JSON-pointer-escaped paths of every $patch:delete directive,
relative to the document root.
- pathExistsInDoc resolves a relative path against a document by
walking mappings segment by segment, deliberately mapping-only to
mirror upstream's deleteForPath predicate.
- jsonPointerUnescape reverses jsonPointerEscape per RFC 6901 so the
walk treats keys with literal `/` or `~` characters
(machine.nodeLabels uses such keys for FQDN-style label names)
consistently with the strip pass that emitted the path.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
|
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 (2)
✨ 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces logic to strip $patch:delete directives from YAML patches when the target path is absent in the base document, preventing lookup errors during the apply phase and aligning behavior with Kubernetes strategic merge patches. Feedback focuses on the need to support SequenceNode traversal in both collectDeleteDirectivePaths and pathExistsInDoc to ensure directives nested within lists or paths containing sequence indices are correctly handled.
| switch node.Kind { | ||
| case yaml.DocumentNode: | ||
| for _, child := range node.Content { | ||
| found = append(found, collectDeleteDirectivePaths(child, parentRel)...) | ||
| } | ||
| case yaml.MappingNode: | ||
| for i := 0; i+1 < len(node.Content); i += 2 { | ||
| keyNode := node.Content[i] | ||
| valueNode := node.Content[i+1] | ||
| if keyNode.Kind != yaml.ScalarNode { | ||
| continue | ||
| } | ||
| childRel := joinYAMLPath(parentRel, jsonPointerEscape(keyNode.Value)) | ||
| if isPatchDeleteDirective(valueNode) { | ||
| found = append(found, childRel) | ||
| continue | ||
| } | ||
| if valueNode.Kind == yaml.MappingNode { | ||
| found = append(found, collectDeleteDirectivePaths(valueNode, childRel)...) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The collectDeleteDirectivePaths function does not traverse SequenceNodes. If a $patch:delete directive is nested within a sequence (e.g., in a list of mappings), it will be missed, potentially causing the directive to be incorrectly left in the patch. Please add support for SequenceNode traversal.
switch node.Kind {
case yaml.DocumentNode:
for _, child := range node.Content {
found = append(found, collectDeleteDirectivePaths(child, parentRel)...)
}
case yaml.SequenceNode:
for i, child := range node.Content {
found = append(found, collectDeleteDirectivePaths(child, fmt.Sprintf("%s/%d", parentRel, i))...)
}
case yaml.MappingNode:
for i := 0; i+1 < len(node.Content); i += 2 {
keyNode := node.Content[i]
valueNode := node.Content[i+1]
if keyNode.Kind != yaml.ScalarNode {
continue
}
childRel := joinYAMLPath(parentRel, jsonPointerEscape(keyNode.Value))
if isPatchDeleteDirective(valueNode) {
found = append(found, childRel)
continue
}
if valueNode.Kind == yaml.MappingNode || valueNode.Kind == yaml.SequenceNode {
found = append(found, collectDeleteDirectivePaths(valueNode, childRel)...)
}
}
}| for _, escaped := range strings.Split(path, "/") { | ||
| seg := jsonPointerUnescape(escaped) | ||
| if cur.Kind != yaml.MappingNode { | ||
| return false | ||
| } | ||
| found := false | ||
| for i := 0; i+1 < len(cur.Content); i += 2 { | ||
| if cur.Content[i].Value == seg { | ||
| cur = cur.Content[i+1] | ||
| found = true | ||
| break | ||
| } | ||
| } | ||
| if !found { | ||
| return false | ||
| } | ||
| } |
There was a problem hiding this comment.
The pathExistsInDoc function assumes all path segments refer to MappingNode keys. If a path includes a sequence index (e.g., list/0/key), this function will return false prematurely. Please add support for SequenceNode traversal to correctly resolve paths that include sequence indices.
for _, escaped := range strings.Split(path, "/") {
seg := jsonPointerUnescape(escaped)
if cur.Kind == yaml.MappingNode {
found := false
for i := 0; i+1 < len(cur.Content); i += 2 {
if cur.Content[i].Value == seg {
cur = cur.Content[i+1]
found = true
break
}
}
if !found {
return false
}
} else if cur.Kind == yaml.SequenceNode {
var idx int
if n, err := fmt.Sscanf(seg, "%d", &idx); n != 1 || err != nil || idx < 0 || idx >= len(cur.Content) {
return false
}
cur = cur.Content[idx]
} else {
return false
}
}
3 participants
What changed
MergeFileAsPatch now strips body-side
$patch:deletedirectives whose path does not resolve to a key in the cleaned rendered template. Previously these directives reachedconfigpatcher.Applyand tripped its Selector-baseddeleteForPathwithfailed to delete path '...': lookup failed.Why
Kubernetes strategic merge patch treats delete-of-absent as a no-op. The chart pattern that triggered this depends on that semantic: a controlplane node-body restating
machine.nodeLabels.<label>: $patch: deletefailed on every fresh apply because the freshly generated Talos config does not containmachine.nodeLabelsat all — there is nothing to delete, but the apply errored anyway.How
A new helper
stripPatchDeleteDirectivesAbsentInTargetruns after the chart-side strip pass. It pairs body and target documents by identity tuple (apiVersion+kind+name, or the legacy-root sentinel) and strips any directive whose path does not resolve in the matching target. Three supporting helpers carry the logic:collectDeleteDirectivePathsenumerates body directives,pathExistsInDocwalks mappings to check for a key,jsonPointerUnescapereverses the existing escape so keys with literal/(FQDN-style label names likenode.kubernetes.io/exclude-from-external-load-balancers) round-trip correctly.Tests
Three new MergeFileAsPatch subtests:
body $patch:delete on absent path is a no-op— top-level absent path.body $patch:delete on partially-present path is a no-op when leaf is absent— parent map present, leaf absent; sibling keys must survive.body $patch:delete on present path still removes the key— regression-safety probe that the no-op-on-absent fix does not over-trigger.Closes #144.