Skip to content

Commit 4b6d47f

Browse files
lexfreilexfrei
authored
fix(engine): treat $patch:delete on absent paths as no-op (#146)
* test(engine): pin no-op semantics for $patch:delete on absent paths 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> * fix(engine): treat $patch:delete on absent paths as no-op (k8s SMP semantics) 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> --------- Signed-off-by: Aleksei Sviridkin <f@lex.la>
1 parent 741f895 commit 4b6d47f

2 files changed

Lines changed: 296 additions & 0 deletions

File tree

‎pkg/engine/engine.go‎

Lines changed: 161 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -250,6 +250,14 @@ func MergeFileAsPatch(rendered []byte, patchFile string) ([]byte, error) {
250250
patchFile,
251251
)
252252
}
253+
cleanedPatch, err = stripPatchDeleteDirectivesAbsentInTarget(cleanedPatch, cleanedRendered)
254+
if err != nil {
255+
return nil, errors.WithHintf(
256+
errors.Wrapf(err, "stripping no-op $patch:delete directives from %q", patchFile),
257+
"the node body did not parse as YAML; verify %q is well-formed",
258+
patchFile,
259+
)
260+
}
253261
prunedBytes, allPruned, err := pruneBodyIdentitiesAgainstRendered(cleanedPatch, cleanedRendered)
254262
if err != nil {
255263
return nil, errors.WithHintf(
@@ -383,6 +391,159 @@ func stripPatchDeleteDirectivesAtPaths(data []byte, paths []string) ([]byte, err
383391
return encodeAllYAMLDocuments(docs)
384392
}
385393

394+
// stripPatchDeleteDirectivesAbsentInTarget walks every YAML document
395+
// in `data` and removes $patch:delete directives whose path does not
396+
// resolve to a key in the matching `target` document. configpatcher.Apply
397+
// otherwise errors with `failed to delete path '...': lookup failed`
398+
// — its Selector-based deleteForPath walks the parsed v1alpha1.Config
399+
// struct and rejects any path segment that does not resolve. Kubernetes
400+
// strategic merge patch treats delete-of-absent as a no-op, so this
401+
// helper restores that semantic before the patch reaches the apply RPC,
402+
// which keeps the chart's own pattern (a body that re-states a chart-
403+
// emitted directive after `talm template -I`) usable on a fresh apply
404+
// where the targeted key has not yet been populated on the node.
405+
//
406+
// `target` is the rendered template AFTER stripAllPatchDeleteDirectives
407+
// has removed every chart-side directive — i.e. the structural shape
408+
// configpatcher.Apply will see as the merge target. A directive whose
409+
// path isn't reachable in that shape is a no-op by definition.
410+
//
411+
// Pairs body and target documents by identity tuple (apiVersion+kind+name,
412+
// or the legacy-root sentinel) so a body re-ordering its typed documents
413+
// relative to rendered still resolves directive paths against the right
414+
// target document. A body document with no matching target document
415+
// (no rendered counterpart at all) gets every directive stripped,
416+
// matching the upstream contract: there is nothing to delete.
417+
func stripPatchDeleteDirectivesAbsentInTarget(data, target []byte) ([]byte, error) {
418+
bodyDocs, err := decodeAllYAMLDocuments(data)
419+
if err != nil {
420+
return nil, err
421+
}
422+
if len(bodyDocs) == 0 {
423+
return data, nil
424+
}
425+
targetDocs, err := decodeAllYAMLDocuments(target)
426+
if err != nil {
427+
return nil, err
428+
}
429+
targetByID := make(map[string]*yaml.Node, len(targetDocs))
430+
for _, doc := range targetDocs {
431+
targetByID[documentIdentityFromNode(doc)] = doc
432+
}
433+
pruneSet := make(map[string]struct{})
434+
for _, bdoc := range bodyDocs {
435+
id := documentIdentityFromNode(bdoc)
436+
targetDoc := targetByID[id]
437+
for _, rel := range collectDeleteDirectivePaths(bdoc, "") {
438+
if !pathExistsInDoc(targetDoc, rel) {
439+
pruneSet[joinYAMLPath("/"+id, rel)] = struct{}{}
440+
}
441+
}
442+
}
443+
if len(pruneSet) == 0 {
444+
return data, nil
445+
}
446+
stripped := 0
447+
for _, doc := range bodyDocs {
448+
stripped += len(removePatchDeleteFromNode(doc, "/"+documentIdentityFromNode(doc), pruneSet))
449+
}
450+
if stripped == 0 {
451+
return data, nil
452+
}
453+
return encodeAllYAMLDocuments(bodyDocs)
454+
}
455+
456+
// collectDeleteDirectivePaths walks `node` and returns the
457+
// JSON-pointer-escaped paths (relative to the document root, no
458+
// identity prefix) of every $patch:delete directive it contains.
459+
// Used by stripPatchDeleteDirectivesAbsentInTarget to enumerate body's
460+
// directives so each can be checked against the target document.
461+
func collectDeleteDirectivePaths(node *yaml.Node, parentRel string) []string {
462+
if node == nil {
463+
return nil
464+
}
465+
var found []string
466+
switch node.Kind {
467+
case yaml.DocumentNode:
468+
for _, child := range node.Content {
469+
found = append(found, collectDeleteDirectivePaths(child, parentRel)...)
470+
}
471+
case yaml.MappingNode:
472+
for i := 0; i+1 < len(node.Content); i += 2 {
473+
keyNode := node.Content[i]
474+
valueNode := node.Content[i+1]
475+
if keyNode.Kind != yaml.ScalarNode {
476+
continue
477+
}
478+
childRel := joinYAMLPath(parentRel, jsonPointerEscape(keyNode.Value))
479+
if isPatchDeleteDirective(valueNode) {
480+
found = append(found, childRel)
481+
continue
482+
}
483+
if valueNode.Kind == yaml.MappingNode {
484+
found = append(found, collectDeleteDirectivePaths(valueNode, childRel)...)
485+
}
486+
}
487+
}
488+
return found
489+
}
490+
491+
// pathExistsInDoc resolves `path` (a slash-separated sequence of
492+
// JSON-pointer-escaped segments, no leading slash, no document
493+
// identity prefix) against the YAML document `doc` and returns true
494+
// when every segment names an existing key in the corresponding
495+
// mapping. An empty path resolves to the document root (true unless
496+
// doc is nil or non-mapping at the root).
497+
//
498+
// The walk is deliberately mapping-only: configpatcher.Apply's
499+
// Selector-based deleteForPath addresses scalar map fields by name
500+
// (machine.nodeLabels.<label>) and bails on the first non-matching
501+
// segment regardless of the target's kind below it. This helper
502+
// reproduces the same predicate so a path declared no-op here is
503+
// guaranteed to be the same path the apply RPC would have erred on.
504+
func pathExistsInDoc(doc *yaml.Node, path string) bool {
505+
if doc == nil {
506+
return false
507+
}
508+
cur := doc
509+
if cur.Kind == yaml.DocumentNode && len(cur.Content) > 0 {
510+
cur = cur.Content[0]
511+
}
512+
if cur == nil || cur.Kind != yaml.MappingNode {
513+
return false
514+
}
515+
if path == "" {
516+
return true
517+
}
518+
for _, escaped := range strings.Split(path, "/") {
519+
seg := jsonPointerUnescape(escaped)
520+
if cur.Kind != yaml.MappingNode {
521+
return false
522+
}
523+
found := false
524+
for i := 0; i+1 < len(cur.Content); i += 2 {
525+
if cur.Content[i].Value == seg {
526+
cur = cur.Content[i+1]
527+
found = true
528+
break
529+
}
530+
}
531+
if !found {
532+
return false
533+
}
534+
}
535+
return true
536+
}
537+
538+
// jsonPointerUnescape reverses jsonPointerEscape per RFC 6901
539+
// (~1 → /, ~0 → ~). Order matters: ~0 must be processed last so a
540+
// literal "~0" written into a YAML key survives the round-trip.
541+
func jsonPointerUnescape(s string) string {
542+
s = strings.ReplaceAll(s, "~1", "/")
543+
s = strings.ReplaceAll(s, "~0", "~")
544+
return s
545+
}
546+
386547
func decodeAllYAMLDocuments(data []byte) ([]*yaml.Node, error) {
387548
dec := yaml.NewDecoder(bytes.NewReader(data))
388549
var docs []*yaml.Node

‎pkg/engine/render_test.go‎

Lines changed: 135 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3182,6 +3182,141 @@ machine:
31823182
}
31833183
})
31843184

3185+
t.Run("body $patch:delete on absent path is a no-op", func(t *testing.T) {
3186+
// Kubernetes strategic merge patch treats a $patch:delete on an
3187+
// absent path as a no-op (the key is already absent, nothing to
3188+
// delete). Talos's configpatcher.Apply does not: its Selector-
3189+
// based deleteForPath walks the parsed v1alpha1.Config struct and
3190+
// returns ErrLookupFailed when any path segment doesn't resolve,
3191+
// surfacing as `failed to delete path '...': lookup failed` from
3192+
// the apply RPC.
3193+
//
3194+
// Stripping these no-op directives at the talm side before the
3195+
// patch reaches configpatcher.Apply matches the k8s SMP semantic
3196+
// and stops a real-world failure: a node body restating a chart-
3197+
// emitted directive (e.g. machine.nodeLabels.<label>: $patch:
3198+
// delete) errors out when rendered for the first time on a
3199+
// freshly generated config that hasn't yet acquired the label.
3200+
// Without this strip, the chart's own pattern fails on every
3201+
// fresh apply and bootstrap is broken.
3202+
const renderedTemplate = `version: v1alpha1
3203+
machine:
3204+
type: controlplane
3205+
install:
3206+
disk: /dev/sda
3207+
cluster:
3208+
controlPlane:
3209+
endpoint: https://10.0.0.10:6443
3210+
`
3211+
const userBody = `# talm: nodes=["10.0.0.1"]
3212+
machine:
3213+
nodeLabels:
3214+
node.kubernetes.io/exclude-from-external-load-balancers:
3215+
$patch: delete
3216+
`
3217+
dir := t.TempDir()
3218+
nodeFile := filepath.Join(dir, "node0.yaml")
3219+
if err := os.WriteFile(nodeFile, []byte(userBody), 0o644); err != nil {
3220+
t.Fatalf("write node file: %v", err)
3221+
}
3222+
3223+
merged, err := MergeFileAsPatch([]byte(renderedTemplate), nodeFile)
3224+
if err != nil {
3225+
t.Fatalf("MergeFileAsPatch must accept a delete directive on an absent path as a no-op, got error: %v", err)
3226+
}
3227+
out := string(merged)
3228+
if strings.Contains(out, "$patch: delete") {
3229+
t.Errorf("merged output still carries the directive literal:\n%s", out)
3230+
}
3231+
if strings.Contains(out, "exclude-from-external-load-balancers") {
3232+
t.Errorf("merged output mentions the never-rendered key — the body's no-op directive should not have re-introduced it:\n%s", out)
3233+
}
3234+
})
3235+
3236+
t.Run("body $patch:delete on partially-present path is a no-op when leaf is absent", func(t *testing.T) {
3237+
// A subtler form of the absent-path case: the body's directive
3238+
// addresses a leaf under a parent that DOES exist in rendered,
3239+
// but the leaf itself doesn't. configpatcher.Apply walks the
3240+
// path segment-by-segment and fails on the missing leaf with
3241+
// the same ErrLookupFailed. The fix must treat any path whose
3242+
// final segment doesn't resolve as a no-op, not just paths
3243+
// missing at the top level.
3244+
const renderedTemplate = `version: v1alpha1
3245+
machine:
3246+
type: controlplane
3247+
install:
3248+
disk: /dev/sda
3249+
nodeLabels:
3250+
other-label: present
3251+
cluster:
3252+
controlPlane:
3253+
endpoint: https://10.0.0.10:6443
3254+
`
3255+
const userBody = `# talm: nodes=["10.0.0.1"]
3256+
machine:
3257+
nodeLabels:
3258+
node.kubernetes.io/exclude-from-external-load-balancers:
3259+
$patch: delete
3260+
`
3261+
dir := t.TempDir()
3262+
nodeFile := filepath.Join(dir, "node0.yaml")
3263+
if err := os.WriteFile(nodeFile, []byte(userBody), 0o644); err != nil {
3264+
t.Fatalf("write node file: %v", err)
3265+
}
3266+
3267+
merged, err := MergeFileAsPatch([]byte(renderedTemplate), nodeFile)
3268+
if err != nil {
3269+
t.Fatalf("MergeFileAsPatch: %v", err)
3270+
}
3271+
out := string(merged)
3272+
if strings.Contains(out, "$patch: delete") {
3273+
t.Errorf("merged output still carries the directive literal:\n%s", out)
3274+
}
3275+
if !strings.Contains(out, "other-label: present") {
3276+
t.Errorf("rendered sibling key under nodeLabels was incorrectly stripped along with the absent target:\n%s", out)
3277+
}
3278+
})
3279+
3280+
t.Run("body $patch:delete on present path still removes the key", func(t *testing.T) {
3281+
// Regression-safety probe: the no-op-on-absent fix must not
3282+
// over-trigger and silently drop directives whose target IS
3283+
// present in rendered. The user-intent delete must still land
3284+
// as a Selector and remove the key from the merged config.
3285+
const renderedTemplate = `version: v1alpha1
3286+
machine:
3287+
type: controlplane
3288+
install:
3289+
disk: /dev/sda
3290+
nodeLabels:
3291+
user-label: please-delete-me
3292+
cluster:
3293+
controlPlane:
3294+
endpoint: https://10.0.0.10:6443
3295+
`
3296+
const userBody = `# talm: nodes=["10.0.0.1"]
3297+
machine:
3298+
nodeLabels:
3299+
user-label:
3300+
$patch: delete
3301+
`
3302+
dir := t.TempDir()
3303+
nodeFile := filepath.Join(dir, "node0.yaml")
3304+
if err := os.WriteFile(nodeFile, []byte(userBody), 0o644); err != nil {
3305+
t.Fatalf("write node file: %v", err)
3306+
}
3307+
3308+
merged, err := MergeFileAsPatch([]byte(renderedTemplate), nodeFile)
3309+
if err != nil {
3310+
t.Fatalf("MergeFileAsPatch: %v", err)
3311+
}
3312+
out := string(merged)
3313+
if strings.Contains(out, "$patch: delete") {
3314+
t.Errorf("merged output still carries the directive literal:\n%s", out)
3315+
}
3316+
if strings.Contains(out, "user-label") {
3317+
t.Errorf("user-intent delete on a present path was suppressed; the key still appears in merged output:\n%s", out)
3318+
}
3319+
})
31853320
}
31863321

31873322
// TestMergeFileAsPatch_PreservesUserIntentPatchDelete pins the contract

0 commit comments

Comments
 (0)