From 8ec0ffa4439c4585effe4ee7bf1f85cb51dd753c Mon Sep 17 00:00:00 2001 From: Gianluca Mardente Date: Tue, 12 May 2026 13:07:22 +0200 Subject: [PATCH] (bug) Fix post-render patch filtering for remove operations When using post-render patches, Sveltos filters out remove operations that target a path which doesn't exist on the object. This avoids errors when a patch is written defensively (e.g. "remove this label if it's there"). This PR fixes two issues in that logic: 1. JSON Pointer escape sequences were not decoded. 2. JSON Pointer paths use ~1 to represent a / character and ~0 to represent ~. A label key like velero.io/exclude-from-backup is written in a patch path as velero.io~1exclude-from-backup. The old code looked up that literal string as the map key, which never matched the actual key velero.io/exclude-from-backup. As a result, a remove operation targeting an existing label with a slash in its name was incorrectly treated as targeting a missing path and silently dropped. Array segments in paths were not handled. If a path traverses a list, for example /spec/containers/0/image, the old code tried to look up "0" as a map key, which always fails since list elements are not keyed by index. Any remove operation whose path passed through an array index was therefore always treated as missing and dropped. Multi-operation patches were only partially inspected. A single patch entry can contain multiple operations. Previously only the first operation was checked, so a remove on a missing path later in the same patch was never filtered and would cause an error at apply time. The fix inspects every operation individually: remove operations targeting missing paths are stripped, the rest are kept. If all operations survive, the patch is returned unchanged; if all are stripped, the patch is dropped entirely. Strategic merge patches without a resource name now produce a clear error. A strategic merge patch identifies its target resource by nam. It is essentially a partial resource object that gets merged into the real one. When such a patch has no metadata.name, kustomize rejects it with an opaque internal error that is hard to understand. This typically happens when a user writes a strategic merge patch expecting a regex target.name selector to handle the matching (which only works for JSON patches). Sveltos now detects this case early and returns a message that explains the problem and points to the JSON patch format as the solution. --- .github/workflows/main.yaml | 4 +- go.mod | 2 +- hack/tools/go.mod | 2 +- lib/patcher/export_test.go | 5 + lib/patcher/patcher.go | 186 ++++++++++++++-------- lib/patcher/patcher_test.go | 298 ++++++++++++++++++++++++++++++++++++ 6 files changed, 430 insertions(+), 67 deletions(-) diff --git a/.github/workflows/main.yaml b/.github/workflows/main.yaml index 46b36990..c498a545 100644 --- a/.github/workflows/main.yaml +++ b/.github/workflows/main.yaml @@ -18,7 +18,7 @@ jobs: - name: Set up Go uses: actions/setup-go@4a3601121dd01d1626a1e23e37211e3254c1c06c # v6.4.0 with: - go-version: 1.26.2 + go-version: 1.26.3 - name: Build run: make build - name: FMT @@ -37,7 +37,7 @@ jobs: - name: Set up Go uses: actions/setup-go@4a3601121dd01d1626a1e23e37211e3254c1c06c # v6.4.0 with: - go-version: 1.26.2 + go-version: 1.26.3 - name: ut run: make test env: diff --git a/go.mod b/go.mod index 6f254264..9502c911 100644 --- a/go.mod +++ b/go.mod @@ -1,6 +1,6 @@ module github.com/projectsveltos/libsveltos -go 1.26.2 +go 1.26.3 require ( github.com/BurntSushi/toml v1.6.0 diff --git a/hack/tools/go.mod b/hack/tools/go.mod index dcb27cf5..cc4085fc 100644 --- a/hack/tools/go.mod +++ b/hack/tools/go.mod @@ -1,6 +1,6 @@ module github.com/projectsveltos/libsveltos/hack/tools -go 1.26.2 +go 1.26.3 require ( github.com/a8m/envsubst v1.4.3 diff --git a/lib/patcher/export_test.go b/lib/patcher/export_test.go index 2a0a0c55..89e27617 100644 --- a/lib/patcher/export_test.go +++ b/lib/patcher/export_test.go @@ -18,4 +18,9 @@ package patcher var ( ParseYAMLToUnstructured = parseYAMLToUnstructured + PathExistsInObject = pathExistsInObject + FilterPatchOperations = filterPatchOperations + IsJSONPatch = isJSONPatch + ValidatePatch = validatePatch + DecodeJSONPointerToken = decodeJSONPointerToken ) diff --git a/lib/patcher/patcher.go b/lib/patcher/patcher.go index 1f5048b2..eef250ca 100644 --- a/lib/patcher/patcher.go +++ b/lib/patcher/patcher.go @@ -24,6 +24,7 @@ import ( "fmt" "io" "regexp" + "strconv" "strings" "sync" @@ -98,6 +99,9 @@ func (k *CustomPatchPostRenderer) Run(renderedManifests *bytes.Buffer) (modified // Add patches. for _, m := range k.Patches { + if err := validatePatch(m); err != nil { + return nil, err + } cfg.Patches = append(cfg.Patches, kustomizetypes.Patch{ Patch: m.Patch, Target: adaptSelector(m.Target), @@ -126,6 +130,37 @@ func (k *CustomPatchPostRenderer) Run(renderedManifests *bytes.Buffer) (modified return bytes.NewBuffer(yaml), nil } +// validatePatch catches SM patches that lack metadata.name before kustomize does, +// returning a clear error instead of kustomize's opaque "unable to parse SM or JSON patch" message. +// A strategic merge patch is identified by having apiVersion/kind at the top level; if it +// also has a target selector with a name pattern, metadata.name must be present in the patch +// body so kustomize can identify the resource. JSON patches (lists starting with '-') are exempt. +func validatePatch(p sveltosv1beta1.Patch) error { + trimmed := strings.TrimSpace(p.Patch) + if strings.HasPrefix(trimmed, "[") || strings.HasPrefix(trimmed, "- ") { + return nil // JSON patch, no metadata.name required + } + + rn, err := kyaml.Parse(trimmed) + if err != nil { + return nil // not parseable as YAML; let kustomize report the error + } + + meta, err := rn.GetMeta() + if err != nil || meta.Kind == "" { + return nil // not a Kubernetes resource shape; let kustomize handle it + } + + if meta.Name == "" { + return fmt.Errorf( + "strategic merge patch for %s/%s requires metadata.name in the patch body; "+ + "use a JSON patch (- op: ...) to match resources by regex name selector", + meta.APIVersion, meta.Kind) + } + + return nil +} + // getMatchingPatches returns patches that match the given object func (k *CustomPatchPostRenderer) getMatchingPatches(obj *unstructured.Unstructured, ) ([]sveltosv1beta1.Patch, error) { @@ -245,102 +280,127 @@ func patchMatchesObject(target *sveltosv1beta1.PatchSelector, return true, nil } -// filterApplicablePatches removes patches where operation is 'remove' and the target path doesn't exist +// filterApplicablePatches removes 'remove' operations from JSON patches where the target +// path does not exist in the object, preventing errors on no-op removals. SM patches are +// passed through unchanged. For multi-operation JSON patches, only the remove operations +// targeting missing paths are stripped; the rest are kept. func (k *CustomPatchPostRenderer) filterApplicablePatches(obj *unstructured.Unstructured, patches []sveltosv1beta1.Patch) []sveltosv1beta1.Patch { var applicable []sveltosv1beta1.Patch for _, patch := range patches { - // Parse the patch to get operation and path - op, path := extractOpAndPath(patch.Patch) - - // Only filter if operation is 'remove' and path doesn't exist - if op == "remove" && path != "" && !pathExistsInObject(obj, path) { - // Path doesn't exist, skip this remove operation - continue + filtered, keep := filterPatchOperations(patch, obj) + if keep { + applicable = append(applicable, filtered) } - - applicable = append(applicable, patch) } return applicable } -// extractOpAndPath extracts the operation and path from a patch string -// Example: "- op: remove\n path: /spec/template/spec/nodeSelector" -> ("remove", "/spec/template/spec/nodeSelector") -func extractOpAndPath(patchStr string) (op, path string) { - // We only expect one YAML list item, so we can treat the whole string as one block of fields. - // However, if the input is truly multiline, splitting is fine. - lines := strings.Split(patchStr, "\n") - - // Combine all lines into a single, space-separated string for simpler parsing. - // This allows us to search for "op:" and "path:" anywhere, regardless of line breaks. - singleLinePatch := strings.Join(lines, " ") - - // --- Extraction Logic for 'op' --- - opIndex := strings.Index(singleLinePatch, "op:") - if opIndex != -1 { - // Find the start of the value: "op:" is 3 characters long - opValueString := singleLinePatch[opIndex+3:] - - // Find the end of the value. Assume the value ends at the start of the next key - // (like "path:") or the end of the string. - // We'll use TrimSpace for simplicity, as op values are usually single words. - op = strings.TrimSpace(opValueString) - - // In case the path key follows immediately on the same line, - // trim everything after "path:" or "Path:" from the 'op' value. - // This handles cases like "remove path: /foo" - if idx := strings.Index(op, "path:"); idx != -1 { - op = op[:idx] +// filterPatchOperations filters out individual 'remove' operations where the JSON Pointer +// path does not exist in the object. Returns the (possibly modified) patch and whether it +// should be kept at all. SM patches are always kept unchanged. +func filterPatchOperations(patch sveltosv1beta1.Patch, obj *unstructured.Unstructured) (sveltosv1beta1.Patch, bool) { + if !isJSONPatch(patch.Patch) { + return patch, true + } + + ops, err := parseJSONPatchOps(patch.Patch) + if err != nil { + return patch, true // unparseable; let kustomize report the error + } + + var keepOps []map[string]interface{} + for _, op := range ops { + opStr, _ := op["op"].(string) + pathStr, _ := op["path"].(string) + if opStr == "remove" && pathStr != "" && !pathExistsInObject(obj, pathStr) { + continue } + keepOps = append(keepOps, op) + } + + if len(keepOps) == 0 { + return patch, false + } - op = strings.TrimSpace(op) - op = strings.Trim(op, `"'`) + if len(keepOps) == len(ops) { + return patch, true } - // --- Extraction Logic for 'path' --- - pathIndex := strings.Index(singleLinePatch, "path:") - if pathIndex != -1 { - // Find the start of the value: "path:" is 5 characters long - pathValueString := singleLinePatch[pathIndex+5:] + rebuilt, err := rebuildJSONPatch(keepOps) + if err != nil { + return patch, true + } - // Trim the value of path - path = strings.TrimSpace(pathValueString) + return sveltosv1beta1.Patch{Patch: rebuilt, Target: patch.Target}, true +} - // If the 'op' key follows immediately on the same line, trim everything after "op:". - // This handles cases like "/foo op: remove" - if idx := strings.Index(path, "op:"); idx != -1 { - path = path[:idx] - } +// isJSONPatch reports whether a patch string is a JSON patch (RFC 6902), i.e. a YAML/JSON +// sequence of {op, path, ...} objects, as opposed to a strategic merge patch. +func isJSONPatch(patchStr string) bool { + trimmed := strings.TrimSpace(patchStr) + return strings.HasPrefix(trimmed, "[") || strings.HasPrefix(trimmed, "- ") +} - path = strings.TrimSpace(path) - path = strings.Trim(path, `"'`) +// parseJSONPatchOps parses a JSON patch (in YAML or JSON format) into a slice of operation maps. +func parseJSONPatchOps(patchStr string) ([]map[string]interface{}, error) { + decoder := uyaml.NewYAMLToJSONDecoder(bytes.NewBufferString(patchStr)) + var ops []map[string]interface{} + if err := decoder.Decode(&ops); err != nil { + return nil, err + } + return ops, nil +} + +// rebuildJSONPatch serializes a slice of operation maps back to a JSON patch string. +// JSON is used (rather than YAML) because it is valid YAML and avoids re-encoding issues. +func rebuildJSONPatch(ops []map[string]interface{}) (string, error) { + data, err := json.Marshal(ops) + if err != nil { + return "", err } + return string(data), nil +} - return op, path +// decodeJSONPointerToken decodes a single RFC 6902 JSON Pointer token: +// ~1 → / and ~0 → ~ (in that order, per the spec). +func decodeJSONPointerToken(token string) string { + token = strings.ReplaceAll(token, "~1", "/") + token = strings.ReplaceAll(token, "~0", "~") + return token } -// pathExistsInObject checks if a JSON path exists in the unstructured object +// pathExistsInObject checks whether the JSON Pointer path exists in the unstructured object. +// It decodes JSON Pointer escape sequences (~1 → /, ~0 → ~) and handles array segments. func pathExistsInObject(obj *unstructured.Unstructured, jsonPath string) bool { keys := strings.Split(strings.Trim(jsonPath, "/"), "/") - current := obj.Object + var current interface{} = obj.Object for _, key := range keys { if key == "" { continue } + key = decodeJSONPointerToken(key) - val, found := current[key] - if !found { + switch v := current.(type) { + case map[string]interface{}: + val, found := v[key] + if !found { + return false + } + current = val + case []interface{}: + idx, err := strconv.Atoi(key) + if err != nil || idx < 0 || idx >= len(v) { + return false + } + current = v[idx] + default: return false } - - // Navigate deeper if it's a nested object - if nested, ok := val.(map[string]interface{}); ok { - current = nested - } } return true diff --git a/lib/patcher/patcher_test.go b/lib/patcher/patcher_test.go index 5134d290..60baa22f 100644 --- a/lib/patcher/patcher_test.go +++ b/lib/patcher/patcher_test.go @@ -40,6 +40,232 @@ spec: ` ) +var _ = Describe("decodeJSONPointerToken", func() { + DescribeTable("decodes RFC 6902 escape sequences", + func(input, expected string) { + Expect(patcher.DecodeJSONPointerToken(input)).To(Equal(expected)) + }, + Entry("~1 becomes /", "velero.io~1exclude-from-backup", "velero.io/exclude-from-backup"), + Entry("~0 becomes ~", "foo~0bar", "foo~bar"), + Entry("~01 becomes ~1 (spec order matters)", "~01", "~1"), + Entry("plain token unchanged", "metadata", "metadata"), + Entry("empty string unchanged", "", ""), + ) +}) + +var _ = Describe("pathExistsInObject", func() { + var obj *unstructured.Unstructured + + BeforeEach(func() { + obj = &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "v1", + "kind": "Pod", + "metadata": map[string]interface{}{ + "name": "mypod", + "labels": map[string]interface{}{ + "velero.io/exclude-from-backup": "true", + "env": "prod", + }, + }, + "spec": map[string]interface{}{ + "containers": []interface{}{ + map[string]interface{}{ + "name": "mycontainer", + "image": "myimage", + }, + }, + }, + }, + } + }) + + It("returns true for a simple path that exists", func() { + Expect(patcher.PathExistsInObject(obj, "/metadata/name")).To(BeTrue()) + }) + + It("returns false for a simple path that does not exist", func() { + Expect(patcher.PathExistsInObject(obj, "/metadata/annotations")).To(BeFalse()) + }) + + It("returns true for a label key containing a slash encoded as ~1", func() { + Expect(patcher.PathExistsInObject(obj, "/metadata/labels/velero.io~1exclude-from-backup")).To(BeTrue()) + }) + + It("returns false for a ~1-encoded path whose key does not exist", func() { + Expect(patcher.PathExistsInObject(obj, "/metadata/labels/velero.io~1missing")).To(BeFalse()) + }) + + It("returns true for a valid array index path", func() { + Expect(patcher.PathExistsInObject(obj, "/spec/containers/0/name")).To(BeTrue()) + }) + + It("returns false for an out-of-bounds array index", func() { + Expect(patcher.PathExistsInObject(obj, "/spec/containers/5/name")).To(BeFalse()) + }) + + It("returns false for a non-numeric array segment", func() { + Expect(patcher.PathExistsInObject(obj, "/spec/containers/notanindex/name")).To(BeFalse()) + }) +}) + +var _ = Describe("filterPatchOperations", func() { + var obj *unstructured.Unstructured + + BeforeEach(func() { + obj = &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "v1", + "kind": "Pod", + "metadata": map[string]interface{}{ + "name": "mypod", + "labels": map[string]interface{}{ + "velero.io/exclude-from-backup": "true", + "existing-label": "value", + }, + }, + }, + } + }) + + It("keeps an SM patch unchanged", func() { + p := sveltosv1beta1.Patch{ + Patch: `apiVersion: v1 +kind: Pod +metadata: + name: mypod + labels: + foo: bar`, + } + result, keep := patcher.FilterPatchOperations(p, obj) + Expect(keep).To(BeTrue()) + Expect(result.Patch).To(Equal(p.Patch)) + }) + + It("keeps a remove op when the path exists", func() { + p := sveltosv1beta1.Patch{ + Patch: `- op: remove + path: /metadata/labels/existing-label`, + } + result, keep := patcher.FilterPatchOperations(p, obj) + Expect(keep).To(BeTrue()) + Expect(patcher.IsJSONPatch(result.Patch)).To(BeTrue()) + }) + + It("drops a remove op when the path does not exist", func() { + p := sveltosv1beta1.Patch{ + Patch: `- op: remove + path: /metadata/labels/nonexistent`, + } + _, keep := patcher.FilterPatchOperations(p, obj) + Expect(keep).To(BeFalse()) + }) + + It("keeps an add op regardless of whether the path exists", func() { + p := sveltosv1beta1.Patch{ + Patch: `- op: add + path: /metadata/labels/new-label + value: foo`, + } + _, keep := patcher.FilterPatchOperations(p, obj) + Expect(keep).To(BeTrue()) + }) + + It("strips only the remove op targeting a missing path from a multi-op patch", func() { + p := sveltosv1beta1.Patch{ + Patch: `- op: add + path: /metadata/labels/new-label + value: foo +- op: remove + path: /metadata/labels/nonexistent`, + } + result, keep := patcher.FilterPatchOperations(p, obj) + Expect(keep).To(BeTrue()) + // Rebuilt patch should contain only the add operation + Expect(result.Patch).To(ContainSubstring(`"add"`)) + Expect(result.Patch).NotTo(ContainSubstring(`"remove"`)) + }) + + It("keeps both ops when both remove paths exist", func() { + p := sveltosv1beta1.Patch{ + Patch: `- op: remove + path: /metadata/labels/existing-label +- op: remove + path: /metadata/labels/velero.io~1exclude-from-backup`, + } + result, keep := patcher.FilterPatchOperations(p, obj) + Expect(keep).To(BeTrue()) + // Patch should be returned as-is (both ops kept) + Expect(result.Patch).To(Equal(p.Patch)) + }) + + It("keeps a remove op for a ~1-encoded path that exists", func() { + p := sveltosv1beta1.Patch{ + Patch: `- op: remove + path: /metadata/labels/velero.io~1exclude-from-backup`, + } + _, keep := patcher.FilterPatchOperations(p, obj) + Expect(keep).To(BeTrue()) + }) + + It("drops a remove op for a ~1-encoded path that does not exist", func() { + p := sveltosv1beta1.Patch{ + Patch: `- op: remove + path: /metadata/labels/velero.io~1missing`, + } + _, keep := patcher.FilterPatchOperations(p, obj) + Expect(keep).To(BeFalse()) + }) +}) + +var _ = Describe("validatePatch", func() { + It("accepts a JSON patch without metadata.name", func() { + p := sveltosv1beta1.Patch{ + Patch: `- op: add + path: /metadata/labels/velero.io~1exclude-from-backup + value: "true"`, + Target: &sveltosv1beta1.PatchSelector{ + Kind: "PersistentVolumeClaim", + Version: "v1", + Name: "data-thanos-.*", + }, + } + Expect(patcher.ValidatePatch(p)).To(Succeed()) + }) + + It("accepts an SM patch that has metadata.name", func() { + p := sveltosv1beta1.Patch{ + Patch: `apiVersion: v1 +kind: PersistentVolumeClaim +metadata: + name: data-thanos-0 + labels: + velero.io/exclude-from-backup: "true"`, + } + Expect(patcher.ValidatePatch(p)).To(Succeed()) + }) + + It("returns a clear error for an SM patch without metadata.name", func() { + p := sveltosv1beta1.Patch{ + Patch: `apiVersion: v1 +kind: PersistentVolumeClaim +metadata: + labels: + velero.io/exclude-from-backup: "true"`, + Target: &sveltosv1beta1.PatchSelector{ + Kind: "PersistentVolumeClaim", + Version: "v1", + Name: "data-thanos-.*", + }, + } + err := patcher.ValidatePatch(p) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("strategic merge patch")) + Expect(err.Error()).To(ContainSubstring("metadata.name")) + Expect(err.Error()).To(ContainSubstring("JSON patch")) + }) +}) + var _ = Describe("CustomPatchPostRenderer", func() { var renderer *patcher.CustomPatchPostRenderer var renderedManifests *bytes.Buffer @@ -126,6 +352,78 @@ metadata: Expect(obj.GetLabels()["environment"]).To(Equal("production")) }) + It("adds a label whose key contains a slash (RFC 6902 ~1 encoding)", func() { + pvc := &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "v1", + "kind": "PersistentVolumeClaim", + "metadata": map[string]interface{}{ + "name": "data-thanos-ruler-0", + "namespace": "monitoring", + "labels": map[string]interface{}{}, + }, + "spec": map[string]interface{}{}, + }, + } + + r := &patcher.CustomPatchPostRenderer{ + Patches: []sveltosv1beta1.Patch{ + { + Patch: `- op: add + path: /metadata/labels/velero.io~1exclude-from-backup + value: "true"`, + Target: &sveltosv1beta1.PatchSelector{ + Version: "v1", + Kind: "PersistentVolumeClaim", + Name: "data-thanos-.*", + }, + }, + }, + } + + result, err := r.RunUnstructured([]*unstructured.Unstructured{pvc}) + Expect(err).ToNot(HaveOccurred()) + Expect(result).To(HaveLen(1)) + Expect(result[0].GetLabels()).To(HaveKey("velero.io/exclude-from-backup")) + Expect(result[0].GetLabels()["velero.io/exclude-from-backup"]).To(Equal("true")) + }) + + It("removes a label whose key contains a slash (RFC 6902 ~1 encoding)", func() { + pvc := &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "v1", + "kind": "PersistentVolumeClaim", + "metadata": map[string]interface{}{ + "name": "data-thanos-ruler-0", + "namespace": "monitoring", + "labels": map[string]interface{}{ + "velero.io/exclude-from-backup": "true", + }, + }, + "spec": map[string]interface{}{}, + }, + } + + r := &patcher.CustomPatchPostRenderer{ + Patches: []sveltosv1beta1.Patch{ + { + Patch: `- op: remove + path: /metadata/labels/velero.io~1exclude-from-backup`, + Target: &sveltosv1beta1.PatchSelector{ + Version: "v1", + Kind: "PersistentVolumeClaim", + Name: "data-thanos-.*", + }, + }, + }, + } + + result, err := r.RunUnstructured([]*unstructured.Unstructured{pvc}) + Expect(err).ToNot(HaveOccurred()) + Expect(result).To(HaveLen(1)) + Expect(result[0].GetLabels()).ToNot(HaveKey("velero.io/exclude-from-backup")) + }) + It("with multiple resources correctly apply patches to unstructured objects and return modified objects", func() { nsYAML := `apiVersion: v1 kind: Namespace