chore: fix the template parser for resolving slice notations (#6449)

Signed-off-by: Gergely Brautigam <182850+Skarlso@users.noreply.github.com>

docs/guides/targeting-custom-resources.md

@@ -56,6 +56,27 @@ When working with custom resources that have complex structures, you can use `ta
 
 The `target` field accepts dot-notation paths like `spec.database` or `spec.logging` to place the rendered template output at specific locations in the resource structure. When `target` is not specified it defaults to `Data` for backward compatibility with Secrets.
 
+### Array Index Notation
+
+Paths can navigate into arrays using `[n]` index notation, for example:
+
+```yaml
+target: spec.rules[0].from[0].source.notRemoteIpBlocks
+```
+
+This templates a resource like an Istio `AuthorizationPolicy`:
+
+```yaml
+spec:
+  rules:
+    - from:
+        - source:
+            notRemoteIpBlocks:
+              - 10.0.0.0/8
+```
+
+Intermediate maps and arrays are created if they don't exist yet, and existing sibling keys in the same array element are preserved. Paths must start with a key; a bare leading index like `[0].foo` is rejected.
+
 !!! note "Using `property` when templating `data`"
     The return of `data:` isn't an object on the template scope. If templated as a `string` it will fail in finding the right key. Therefore, something like this:
     ```yaml

runtime/template/v2/template.go

@@ -20,6 +20,7 @@ import (
 	"bytes"
 	"fmt"
 	"maps"
+	"strconv"
 	"strings"
 	tpl "text/template"
 
@@ -118,41 +119,18 @@ func applyToTarget(k string, val []byte, target string, obj client.Object) error
 		}
 	case "spec":
 		if err := setField(obj, "spec", k, val); err != nil {
-			return fmt.Errorf("failed to set data field on object: %w", err)
+			return fmt.Errorf("failed to set spec field on object: %w", err)
 		}
 	default:
-		parts := strings.Split(target, ".")
-		if len(parts) == 0 {
-			return fmt.Errorf("invalid path: %s", target)
-		}
-
-		unstructured, err := runtime.DefaultUnstructuredConverter.ToUnstructured(obj)
+		tokens, err := parseTargetPath(target)
 		if err != nil {
-			return fmt.Errorf(errConvertingToUnstructured, err)
-		}
-
-		// Navigate to the parent of the target field
-		current := unstructured
-		for i := range len(parts) - 1 {
-			part := parts[i]
-			if current[part] == nil {
-				current[part] = make(map[string]any)
-			}
-			next, ok := current[part].(map[string]any)
-			if !ok {
-				return fmt.Errorf("path %s is not a map at segment %s", target, part)
-			}
-			current = next
+			return err
 		}
-
-		// Set the value at the final key
-		// Convert []byte to string to avoid base64 encoding when serializing
-		lastPart := parts[len(parts)-1]
-		current[lastPart] = tryParseYAML(string(val))
-
-		// Convert back to the original object type
-		if err := runtime.DefaultUnstructuredConverter.FromUnstructured(unstructured, obj); err != nil {
-			return fmt.Errorf(errConvertingToObject, err)
+		// Set the value at the target path, converting []byte to string to avoid
+		// base64 encoding when serializing.
+		leaf := func(any) any { return tryParseYAML(string(val)) }
+		if err := applyAtPath(obj, target, tokens, leaf); err != nil {
+			return err
 		}
 	}
 
@@ -314,67 +292,214 @@ func tryParseYAML(value any) any {
 
 // applyParsedToPath applies a parsed YAML structure to a specific path in the object.
 func applyParsedToPath(parsed any, target string, obj client.Object) error {
+	tokens, err := parseTargetPath(target)
+	if err != nil {
+		return err
+	}
+
+	// navigate to the last element of the path and apply the entire struct at that location.
+	// MERGE the parsed content into existing map content instead of replacing it.
+	leaf := func(existing any) any {
+		existingMap, existingOk := existing.(map[string]any)
+		parsedMap, parsedOk := parsed.(map[string]any)
+		if existingOk && parsedOk {
+			maps.Copy(existingMap, parsedMap)
+			return existingMap
+		}
+		// existing or parsed value is not a map, replace entirely.
+		// this might break if people are trying to overwrite
+		// fields that aren't supposed to do that. but that's
+		// on the user to keep in mind. If they are trying to
+		// update a number field with a complex value, that's
+		// going to error on update anyway.
+		return parsed
+	}
+
+	// single value, aka "spec": replace entirely to preserve historical behavior.
+	if len(tokens) == 1 && tokens[0].kind == tokenKey {
+		leaf = func(any) any { return parsed }
+	}
+
+	return applyAtPath(obj, target, tokens, leaf)
+}
+
+// applyAtPath applies leaf at the token path within obj's unstructured form and
+// writes the result back into obj. target is used only for error messages.
+func applyAtPath(obj client.Object, target string, tokens []pathToken, leaf func(existing any) any) error {
 	unstructured, err := runtime.DefaultUnstructuredConverter.ToUnstructured(obj)
 	if err != nil {
 		return fmt.Errorf(errConvertingToUnstructured, err)
 	}
 
-	parts := strings.Split(target, ".")
-	if len(parts) == 0 {
-		return fmt.Errorf("invalid path: %s", target)
+	updated, err := setAtPath(unstructured, tokens, leaf)
+	if err != nil {
+		return fmt.Errorf("failed to set path %s: %w", target, err)
 	}
 
-	// single value, aka "spec"
-	if len(parts) == 1 {
-		unstructured[parts[0]] = parsed
-	} else {
-		// navigate to the last element of the path and apply the entire struct at that location.
-		// build up the entire map structure that we are eventually going to apply.
-		current := unstructured
-		// this STOPS at the last part! That is important. for _, part := range parts does _include_ the last part
-		for i := 0; i < len(parts)-1; i++ {
-			part := parts[i]
-			if current[part] == nil {
-				current[part] = make(map[string]any)
-			}
-			next, ok := current[part].(map[string]any)
-			if !ok {
-				return fmt.Errorf("path %s is not a map at segment %s", target, part)
-			}
-			current = next
+	updatedMap, ok := updated.(map[string]any)
+	if !ok {
+		return fmt.Errorf("failed to set path %s: expected object root, got %T", target, updated)
+	}
+
+	if err := runtime.DefaultUnstructuredConverter.FromUnstructured(updatedMap, obj); err != nil {
+		return fmt.Errorf(errConvertingToObject, err)
+	}
+
+	return nil
+}
+
+// pathTokenKind distinguishes a map-key access from a slice-index access in a target path.
+type pathTokenKind int
+
+const (
+	tokenKey pathTokenKind = iota
+	tokenIndex
+)
+
+// pathToken is a single step in a parsed target path.
+type pathToken struct {
+	kind pathTokenKind
+	name string
+	idx  int
+}
+
+// maxArrayIndex limits the memory assignation for setAtPath.
+const maxArrayIndex = 10000
+
+// parseTargetPath is a pure function parsing and validating a dotted target path, with optional slice notation, into tokens.
+// Each step is a pathToken of kind "tokenKey" or "tokenIndex".
+//
+// e.g.
+//
+//		input:  "spec.rules[0].from[0].source.notRemoteIpBlocks"
+//		output: [
+//	  	{key,  "spec"},
+//	  	{key,  "rules"},
+//	  	{index, 0},
+//	  	{key,  "from"},
+//	  	{index, 0},
+//	  	{key,  "source"},
+//	  	{key,  "notRemoteIpBlocks"},
+//		]
+func parseTargetPath(target string) ([]pathToken, error) {
+	var tokens []pathToken
+	for seg := range strings.SplitSeq(target, ".") {
+		if seg == "" {
+			return nil, fmt.Errorf("invalid path %q: empty segment", target)
+		}
+
+		name, rest := seg, ""
+		if i := strings.IndexByte(seg, '['); i >= 0 {
+			name, rest = seg[:i], seg[i:]
+		}
+		if name != "" {
+			tokens = append(tokens, pathToken{kind: tokenKey, name: name})
+		} else if rest == "" {
+			return nil, fmt.Errorf("invalid path %q: empty segment", target)
 		}
 
-		// once we constructed the entire segment, we finally apply our parsed object
-		// MERGE the parsed content into existing content instead of replacing it
-		lastPart := parts[len(parts)-1]
-		if existing, exists := current[lastPart]; exists {
-			// if both existing and new values are maps, merge them
-			existingMap, existingOk := existing.(map[string]any)
-			parsedMap, parsedOk := parsed.(map[string]any)
-
-			if existingOk && parsedOk {
-				maps.Copy(existingMap, parsedMap)
-
-				current[lastPart] = existingMap
-			} else {
-				// existing or parsed value is not a map, replace entirely.
-				// this might break if people are trying to overwrite
-				// fields that aren't supposed to do that. but that's
-				// on the user to keep in mind. If they are trying to
-				// update a number field with a complex value, that's
-				// going to error on update anyway.
-				current[lastPart] = parsed
+		for rest != "" {
+			if rest[0] != '[' {
+				return nil, fmt.Errorf("invalid path %q: expected '[' near %q", target, rest)
 			}
-		} else {
-			// field doesn't exist yet, create it
-			current[lastPart] = parsed
+			end := strings.IndexByte(rest, ']')
+			if end < 0 {
+				return nil, fmt.Errorf("invalid path %q: unterminated '['", target)
+			}
+			idx, err := strconv.Atoi(rest[1:end])
+			if err != nil {
+				return nil, fmt.Errorf("invalid path %q: bad index %q", target, rest[1:end])
+			}
+			if idx < 0 {
+				return nil, fmt.Errorf("invalid path %q: negative index %d", target, idx)
+			}
+			if idx > maxArrayIndex {
+				return nil, fmt.Errorf("invalid path %q: index %d exceeds maximum (10000)", target, idx)
+			}
+			tokens = append(tokens, pathToken{kind: tokenIndex, idx: idx})
+			rest = rest[end+1:]
 		}
 	}
+	if len(tokens) == 0 {
+		return nil, fmt.Errorf("invalid path: %s", target)
+	}
+	if tokens[0].kind != tokenKey {
+		return nil, fmt.Errorf("invalid path %q: path must start with a key", target)
+	}
+	return tokens, nil
+}
+
+// setAtPath walks node following tokens, creating intermediate maps and slices as needed,
+// and applies leaf to the value at the final location. It returns the possibly-reallocated
+// node, since growing a slice replaces its header.
+func setAtPath(node any, tokens []pathToken, leaf func(existing any) any) (any, error) {
+	tok := tokens[0]
+	switch tok.kind {
+	case tokenKey:
+		return setMap(node, tokens, leaf, tok)
+	case tokenIndex:
+		return setIndex(node, tokens, leaf, tok)
+	default:
+		return nil, fmt.Errorf("unknown path token kind %d", tok.kind)
+	}
+}
 
-	// convert back to original object
-	if err := runtime.DefaultUnstructuredConverter.FromUnstructured(unstructured, obj); err != nil {
-		return fmt.Errorf(errConvertingToObject, err)
+// setIndex sets a value in a slice at index.
+func setIndex(node any, tokens []pathToken, leaf func(existing any) any, tok pathToken) (any, error) {
+	s, ok := node.([]any)
+	if !ok && node != nil {
+		return nil, fmt.Errorf("expected array at index %d but found %T", tok.idx, node)
 	}
+	if tok.idx >= len(s) {
+		grown := make([]any, tok.idx+1)
+		copy(grown, s)
+		s = grown
+	}
+	if err := setLeaf(
+		func() any { return s[tok.idx] },
+		func(v any) { s[tok.idx] = v },
+		tokens, leaf,
+	); err != nil {
+		return nil, err
+	}
+	return s, nil
+}
 
+// setMap sets a value in a map at token name.
+func setMap(node any, tokens []pathToken, leaf func(existing any) any, tok pathToken) (any, error) {
+	m, ok := node.(map[string]any)
+	if !ok && node != nil {
+		return nil, fmt.Errorf("expected map at key %q but found %T", tok.name, node)
+	}
+	if len(m) == 0 {
+		m = make(map[string]any)
+	}
+	if err := setLeaf(
+		func() any { return m[tok.name] },
+		func(v any) { m[tok.name] = v },
+		tokens, leaf,
+	); err != nil {
+		return nil, err
+	}
+	return m, nil
+}
+
+// setLeaf writes leaf into a container via get/set closures, recursing when
+// the path continues. The closures capture the container (slice or map).
+// set is called to set a value in a container, be that a slice or a map.
+// get is called to retrieve a value from a container, either a slice or a map.
+// This abstraction is necessary because there are no shared slice/map operation even
+// in Typed Parameters, so these cannot be reasonably abstracted. At least, it makes
+// the entire logic a bit more readable.
+func setLeaf(get func() any, set func(any), tokens []pathToken, leaf func(existing any) any) error {
+	if len(tokens) == 1 {
+		set(leaf(get()))
+		return nil
+	}
+	child, err := setAtPath(get(), tokens[1:], leaf)
+	if err != nil {
+		return err
+	}
+	set(child)
 	return nil
 }

runtime/template/v2/template_test.go

@@ -1476,6 +1476,104 @@ channel: {{ .new_channel }}
 				assert.False(t, lowered, "intermediate segment must not be lowercased")
 			},
 		},
+		{
+			name:   "slice notation creates nested array path from scratch",
+			target: "spec.rules[0].from[0].source.notRemoteIpBlocks",
+			scope:  esapi.TemplateScopeKeysAndValues,
+			tpl: map[string][]byte{
+				"blocks": []byte("- {{ .cidr1 }}\n- {{ .cidr2 }}\n"),
+			},
+			data: map[string][]byte{
+				"cidr1": []byte("10.0.0.0/8"),
+				"cidr2": []byte("192.168.0.0/16"),
+			},
+			verify: func(t *testing.T, obj map[string]any) {
+				rules := obj["spec"].(map[string]any)["rules"].([]any)
+				from := rules[0].(map[string]any)["from"].([]any)
+				source := from[0].(map[string]any)["source"].(map[string]any)
+				assert.Equal(t, []any{"10.0.0.0/8", "192.168.0.0/16"}, source["notRemoteIpBlocks"])
+			},
+		},
+		{
+			name:   "slice notation merges into existing array element",
+			target: "spec.rules[0].to",
+			scope:  esapi.TemplateScopeKeysAndValues,
+			tpl: map[string][]byte{
+				"to": []byte("operation:\n  methods:\n    - GET\n"),
+			},
+			data: map[string][]byte{},
+			verify: func(t *testing.T, obj map[string]any) {
+				rules := obj["spec"].(map[string]any)["rules"].([]any)
+				rule := rules[0].(map[string]any)
+				// existing sibling key in the same element is preserved
+				assert.Equal(t, "existing", rule["from"])
+				op := rule["to"].(map[string]any)["operation"].(map[string]any)
+				assert.Equal(t, []any{"GET"}, op["methods"])
+			},
+		},
+		{
+			name:   "slice notation with value scope sets scalar at index",
+			target: "spec.containers[1].image",
+			scope:  esapi.TemplateScopeValues,
+			tpl: map[string][]byte{
+				"image": []byte("{{ .img }}"),
+			},
+			data: map[string][]byte{
+				"img": []byte("nginx:latest"),
+			},
+			verify: func(t *testing.T, obj map[string]any) {
+				containers := obj["spec"].(map[string]any)["containers"].([]any)
+				// index 0 was grown and left nil, index 1 holds the value
+				assert.Nil(t, containers[0])
+				assert.Equal(t, "nginx:latest", containers[1].(map[string]any)["image"])
+			},
+		},
+		{
+			name:   "slice notation grows existing non-empty array",
+			target: "spec.containers[2].image",
+			scope:  esapi.TemplateScopeValues,
+			tpl: map[string][]byte{
+				"image": []byte("{{ .img }}"),
+			},
+			data: map[string][]byte{
+				"img": []byte("nginx:latest"),
+			},
+			verify: func(t *testing.T, obj map[string]any) {
+				containers := obj["spec"].(map[string]any)["containers"].([]any)
+				assert.Len(t, containers, 3)
+				// pre-existing element at index 0 is preserved
+				assert.Equal(t, "sidecar:v1", containers[0].(map[string]any)["image"])
+				assert.Nil(t, containers[1])
+				assert.Equal(t, "nginx:latest", containers[2].(map[string]any)["image"])
+			},
+		},
+		{
+			name:     "invalid slice notation reports error",
+			target:   "spec.rules[abc].foo",
+			scope:    esapi.TemplateScopeKeysAndValues,
+			tpl:      map[string][]byte{"x": []byte("y: z")},
+			data:     map[string][]byte{},
+			wantErr:  true,
+			errorMsg: "bad index",
+		},
+		{
+			name:     "bare leading index is rejected",
+			target:   "[0]",
+			scope:    esapi.TemplateScopeKeysAndValues,
+			tpl:      map[string][]byte{"x": []byte("y: z")},
+			data:     map[string][]byte{},
+			wantErr:  true,
+			errorMsg: "must start with a key",
+		},
+		{
+			name:     "leading index followed by key is rejected",
+			target:   "[0].test",
+			scope:    esapi.TemplateScopeKeysAndValues,
+			tpl:      map[string][]byte{"x": []byte("y: z")},
+			data:     map[string][]byte{},
+			wantErr:  true,
+			errorMsg: "must start with a key",
+		},
 	}
 
 	for _, tt := range tests {
@@ -1513,6 +1611,24 @@ channel: {{ .new_channel }}
 				}
 			}
 
+			// For the slice-merge test, pre-populate an existing array element.
+			if strings.Contains(tt.name, "merges into existing array element") {
+				obj.Object["spec"] = map[string]any{
+					"rules": []any{
+						map[string]any{"from": "existing"},
+					},
+				}
+			}
+
+			// For the slice-grow test, pre-populate a non-empty array.
+			if strings.Contains(tt.name, "grows existing non-empty array") {
+				obj.Object["spec"] = map[string]any{
+					"containers": []any{
+						map[string]any{"image": "sidecar:v1"},
+					},
+				}
+			}
+
 			err := Execute(tt.tpl, tt.data, tt.scope, tt.target, obj)
 
 			if tt.wantErr {
@@ -1529,3 +1645,107 @@ channel: {{ .new_channel }}
 		})
 	}
 }
+
+func TestNestedPathTargetingIsIdempotent(t *testing.T) {
+	tests := []struct {
+		name   string
+		target string
+		scope  esapi.TemplateScope
+		tpl    map[string][]byte
+		data   map[string][]byte
+		seed   map[string]any
+		verify func(t *testing.T, obj map[string]any)
+	}{
+		{
+			name:   "deep slice path created from scratch",
+			target: "spec.rules[0].from[0].source.notRemoteIpBlocks",
+			scope:  esapi.TemplateScopeKeysAndValues,
+			tpl: map[string][]byte{
+				"blocks": []byte("- {{ .cidr }}\n"),
+			},
+			data: map[string][]byte{
+				"cidr": []byte("10.0.0.0/8"),
+			},
+			verify: func(t *testing.T, obj map[string]any) {
+				rules := obj["spec"].(map[string]any)["rules"].([]any)
+				assert.Len(t, rules, 1, "rules slice must not grow on re-apply")
+				from := rules[0].(map[string]any)["from"].([]any)
+				assert.Len(t, from, 1, "from slice must not grow on re-apply")
+				source := from[0].(map[string]any)["source"].(map[string]any)
+				assert.Equal(t, []any{"10.0.0.0/8"}, source["notRemoteIpBlocks"])
+			},
+		},
+		{
+			name:   "map merge into existing element",
+			target: "spec.slack",
+			scope:  esapi.TemplateScopeKeysAndValues,
+			tpl: map[string][]byte{
+				"slack-config": []byte("api_url: {{ .url }}\n"),
+			},
+			data: map[string][]byte{
+				"url": []byte("https://hooks.slack.com/services/NEW"),
+			},
+			seed: map[string]any{
+				"slack": map[string]any{
+					"channel": "general",
+				},
+			},
+			verify: func(t *testing.T, obj map[string]any) {
+				slack := obj["spec"].(map[string]any)["slack"].(map[string]any)
+				assert.Equal(t, "https://hooks.slack.com/services/NEW", slack["api_url"])
+				assert.Equal(t, "general", slack["channel"], "seeded field must survive every re-apply")
+				_, nested := slack["slack"]
+				assert.False(t, nested, "merge must not nest the target key into itself")
+			},
+		},
+		{
+			name:   "scalar at sparse index",
+			target: "spec.containers[1].image",
+			scope:  esapi.TemplateScopeValues,
+			tpl: map[string][]byte{
+				"image": []byte("{{ .img }}"),
+			},
+			data: map[string][]byte{
+				"img": []byte("nginx:latest"),
+			},
+			verify: func(t *testing.T, obj map[string]any) {
+				containers := obj["spec"].(map[string]any)["containers"].([]any)
+				assert.Len(t, containers, 2, "slice must stay at index+1, not grow per apply")
+				assert.Nil(t, containers[0])
+				assert.Equal(t, "nginx:latest", containers[1].(map[string]any)["image"])
+			},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			obj := &unstructured.Unstructured{
+				Object: map[string]any{
+					"apiVersion": "example.com/v1",
+					"kind":       "TestResource",
+					"metadata": map[string]any{
+						"name":      "test-resource",
+						"namespace": "default",
+					},
+				},
+			}
+			if tt.seed != nil {
+				obj.Object["spec"] = tt.seed
+			}
+
+			var snapshot map[string]any
+			for i := range 3 {
+				require.NoError(t, Execute(tt.tpl, tt.data, tt.scope, tt.target, obj))
+				if i == 0 {
+					snapshot = obj.DeepCopy().Object
+					continue
+				}
+				if diff := cmp.Diff(snapshot, obj.Object); diff != "" {
+					t.Fatalf("apply #%d diverged from the first apply (-first +current):\n%s", i+1, diff)
+				}
+			}
+
+			tt.verify(t, obj.Object)
+		})
+	}
+}