diff --git a/.github/workflows/main.yaml b/.github/workflows/main.yaml index 46b3699..c498a54 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 6f25426..9502c91 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 dcb27cf..cc4085f 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 2a0a0c5..89e2761 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 1f5048b..eef250c 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 5134d29..60baa22 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