refactor(field_mutator): migrate dot notation to JSON Pointer syntax (#118)

Replaces custom dot notation parser with [RFC 6901 JSON Pointer](https://datatracker.ietf.org/doc/html/rfc6901) syntax to handle Kubernetes label keys containing dots (e.g., `app.kubernetes.io/instance`). 

Migration enables proper Service selector transformations where previous dot notation failed on keys like `spec.selector["app.kubernetes.io/instance"]`, unblocking dynamic Service-to-Deployment label matching.


Approved-by: leseb

Approved-by: rhdedgar

Author: mfleader

pkg/deploy/kustomizer.go

@@ -211,19 +211,19 @@ func applyPlugins(resMap *resmap.ResMap, ownerInstance *llamav1alpha1.LlamaStack
 			{
 				SourceValue:       getStorageSize(ownerInstance),
 				DefaultValue:      llamav1alpha1.DefaultStorageSize.String(),
-				TargetField:       "spec.resources.requests.storage",
+				TargetField:       "/spec/resources/requests/storage",
 				TargetKind:        "PersistentVolumeClaim",
 				CreateIfNotExists: true,
 			},
 			{
 				SourceValue:       ownerInstance.GetNamespace(),
-				TargetField:       "subjects[0].namespace",
+				TargetField:       "/subjects/0/namespace",
 				TargetKind:        "ClusterRoleBinding",
 				CreateIfNotExists: true,
 			},
 			{
 				SourceValue:       ownerInstance.GetName() + "-sa",
-				TargetField:       "subjects[0].name",
+				TargetField:       "/subjects/0/name",
 				TargetKind:        "ClusterRoleBinding",
 				CreateIfNotExists: true,
 			},

pkg/deploy/plugins/field_mutator.go

@@ -2,10 +2,9 @@ package plugins
 
 import (
 	"fmt"
-	"regexp"
-	"strconv"
 	"strings"
 
+	"github.com/go-openapi/jsonpointer"
 	"sigs.k8s.io/kustomize/api/resmap"
 	"sigs.k8s.io/kustomize/api/resource"
 	"sigs.k8s.io/yaml"
@@ -18,8 +17,10 @@ type FieldMapping struct {
 	// DefaultValue is the value to use if SourceValue is empty.
 	// This provides a fallback mechanism, making transformations more robust.
 	DefaultValue any `json:"defaultValue,omitempty"`
-	// TargetField is the dot-notation path to the field in the target object.
-	// Supports array indices like "spec.ports[0].port"
+	// TargetField is the JSON Pointer path to the field in the target object.
+	// Uses RFC 6901 JSON Pointer syntax like "/spec/ports/0/port"
+	// Special characters in field names are escaped: ~0 for ~ and ~1 for /
+	// Example: "/spec/selector/app.kubernetes.io~1instance"
 	TargetField string `json:"targetField"`
 	// TargetKind is the kind of resource to apply the transformation to.
 	TargetKind string `json:"targetKind"`
@@ -43,46 +44,6 @@ type fieldMutator struct {
 	config FieldMutatorConfig
 }
 
-type fieldSegment struct {
-	name     string // field name (e.g., "ports")
-	isArray  bool   // true if this segment has an array index
-	index    int    // array index if isArray is true
-	original string // original segment string for error messages
-}
-
-func parseFieldPath(path string) ([]fieldSegment, error) {
-	// Regular expression to match field names with optional array indices
-	// Matches: "fieldname" or "fieldname[123]"
-	re := regexp.MustCompile(`^([a-zA-Z_][a-zA-Z0-9_]*)\[(\d+)\]$`)
-
-	parts := strings.Split(path, ".")
-	segments := make([]fieldSegment, len(parts))
-
-	for i, part := range parts {
-		if matches := re.FindStringSubmatch(part); matches != nil {
-			index, err := strconv.Atoi(matches[2])
-			if err != nil {
-				return nil, fmt.Errorf("failed to parse array index in field path segment %q: %w", part, err)
-			}
-			segments[i] = fieldSegment{
-				name:     matches[1],
-				isArray:  true,
-				index:    index,
-				original: part,
-			}
-		} else {
-			segments[i] = fieldSegment{
-				name:     part,
-				isArray:  false,
-				index:    -1,
-				original: part,
-			}
-		}
-	}
-
-	return segments, nil
-}
-
 // isEmpty checks if a value is nil or an empty string, slice, or map.
 func isEmpty(v any) bool {
 	if v == nil {
@@ -133,176 +94,116 @@ func (t *fieldMutator) Config(h *resmap.PluginHelpers, _ []byte) error {
 }
 
 // setTargetField modifies the resource by setting the specified value at the
-// given dot-notation path with support for array indices.
+// given JSON Pointer path.
 func setTargetField(res *resource.Resource, value any, mapping FieldMapping) error {
 	yamlBytes, err := res.AsYAML()
 	if err != nil {
 		return fmt.Errorf("failed to get YAML: %w", err)
 	}
 
-	var data map[string]any
+	var data any
 	if unmarshalErr := yaml.Unmarshal(yamlBytes, &data); unmarshalErr != nil {
 		return fmt.Errorf("failed to unmarshal YAML: %w", unmarshalErr)
 	}
 
-	segments, err := parseFieldPath(mapping.TargetField)
+	ptr, err := jsonpointer.New(mapping.TargetField)
 	if err != nil {
-		return fmt.Errorf("failed to parse field path %q: %w", mapping.TargetField, err)
+		return fmt.Errorf("failed to parse JSON Pointer path %q: %w", mapping.TargetField, err)
 	}
 
-	// Navigate to parent first, then set value - arrays vs maps need different handling.
-	current, err := navigateToParent(data, segments, mapping.CreateIfNotExists)
-	if err != nil {
-		return err
-	}
-
-	lastSegment := segments[len(segments)-1]
-	if setErr := setFieldValue(current, lastSegment, value, mapping.CreateIfNotExists); setErr != nil {
-		return fmt.Errorf("failed to set field %q: %w", lastSegment.original, setErr)
+	var updatedData any
+	if mapping.CreateIfNotExists {
+		updatedData, err = setWithPathCreation(data, ptr, value)
+	} else {
+		updatedData, err = ptr.Set(data, value)
 	}
 
-	// After modifying the map, we must marshal it back to YAML and create a new
-	// resource object to ensure the internal state is consistent.
-	updatedYAML, err := yaml.Marshal(data)
-	if err != nil {
-		return fmt.Errorf("failed to marshal updated YAML: %w", err)
-	}
-
-	rf := resource.NewFactory(nil)
-	newRes, err := rf.FromBytes(updatedYAML)
 	if err != nil {
-		return fmt.Errorf("failed to create resource from updated YAML: %w", err)
+		return fmt.Errorf("failed to set field at path %q: %w", mapping.TargetField, err)
 	}
 
-	// Atomically replace the old resource content with the new, fully updated content
-	// to prevent partial updates or data loss.
-	res.ResetRNode(newRes)
-	return nil
+	return updateResource(res, updatedData)
 }
 
-// navigateToParent walks through all field segments except the last one,
-// returning the parent container where the final field should be set.
-func navigateToParent(data map[string]any, segments []fieldSegment, createIfNotExists bool) (map[string]any, error) {
-	current := data
-	// This loop navigates to the parent of the target field. We must stop one
-	// level short because Golang does not allow taking a pointer to a map value
-	// (e.g., `&myMap["key"]`). To mutate the map, we need a reference to the
-	// parent container to use the `parent[key] = value` syntax.
-	//
-	// This "stop at the parent" approach also gives us the `CreateIfNotExists`
-	// behavior for free, as we can create missing parent maps during traversal.
-	for _, segment := range segments[:len(segments)-1] {
-		next, navErr := navigateToField(current, segment, createIfNotExists)
-		if navErr != nil {
-			if strings.Contains(navErr.Error(), "failed to find field") {
-				return nil, navErr
-			}
-			return nil, fmt.Errorf("failed to navigate to field %q: %w", segment.original, navErr)
-		}
-		currentMap, ok := next.(map[string]any)
-		if !ok {
-			return nil, fmt.Errorf("failed to convert field %s to map", segment.name)
-		}
-		current = currentMap
+func setWithPathCreation(data any, ptr jsonpointer.Pointer, value any) (any, error) {
+	// try setting value if the path already exists
+	if updatedData, err := ptr.Set(data, value); err == nil {
+		return updatedData, nil
 	}
-	return current, nil
-}
 
-// navigateToField returns the value at the specified field, creating it if needed.
-func navigateToField(current any, segment fieldSegment, createIfNotExists bool) (any, error) {
-	currentMap, ok := current.(map[string]any)
-	if !ok {
-		return nil, fmt.Errorf("failed to convert current value to map[string]any, got %T", current)
+	// otherwise, we need to create the path first
+	tokens := ptr.DecodedTokens()
+	if len(tokens) == 0 {
+		return value, nil
 	}
+	result := data
 
-	next, exists := currentMap[segment.name]
-	if !exists {
-		if !createIfNotExists {
-			return nil, fmt.Errorf("failed to find field %s", segment.name)
+	// JSON Pointer Set() fails if intermediate paths don't exist, so we must
+	// build the path incrementally from root to target, creating missing containers
+	for i := 1; i <= len(tokens)-1; i++ {
+		partialPath := "/" + strings.Join(tokens[:i], "/")
+		partialPtr, err := jsonpointer.New(partialPath)
+		if err != nil {
+			return nil, fmt.Errorf("failed to create partial pointer: %w", err)
 		}
+		// Get() is used as existence test - error means path doesn't exist and needs creation
+		_, _, err = partialPtr.Get(result)
+		if err != nil {
+			nextToken := tokens[i]
+			var newContainer any
+			// create array if next token is numeric (e.g., "/ports/0")
+			if isNumericString(nextToken) {
+				newContainer = make([]any, 0)
+			} else {
+				// create map otherwise (e.g., "/spec/strategy")
+				newContainer = make(map[string]any)
+			}
 
-		if segment.isArray {
-			next = make([]any, segment.index+1)
-		} else {
-			next = make(map[string]any)
+			// create the missing path segment
+			result, err = partialPtr.Set(result, newContainer)
+			if err != nil {
+				return nil, fmt.Errorf("failed to create intermediate path %q: %w", partialPath, err)
+			}
 		}
-		currentMap[segment.name] = next
 	}
 
-	if segment.isArray {
-		return handleArrayAccess(currentMap, segment, next, createIfNotExists)
-	}
-
-	return next, nil
-}
-
-// handleArrayAccess navigates to a specific array index, expanding the array and
-// creating missing elements if needed. Returns the element at the specified index.
-func handleArrayAccess(currentMap map[string]any, segment fieldSegment, next any, createIfNotExists bool) (any, error) {
-	arr, err := ensureArrayWithCapacity(currentMap, segment, next, createIfNotExists)
+	result, err := ptr.Set(result, value)
 	if err != nil {
-		return nil, err
+		return nil, fmt.Errorf("failed to set final value: %w", err)
 	}
 
-	if arr[segment.index] == nil {
-		if !createIfNotExists {
-			return nil, fmt.Errorf("failed to access array element at index %d for field %q", segment.index, segment.name)
-		}
-		arr[segment.index] = make(map[string]any)
-	}
-
-	return arr[segment.index], nil
+	return result, nil
 }
 
-// ensureArrayWithCapacity ensures an array exists at the specified field with sufficient capacity.
-// If the array doesn't exist or is too small, it creates or expands it as needed.
-func ensureArrayWithCapacity(currentMap map[string]any, segment fieldSegment, field any, createIfNotExists bool) ([]any, error) {
-	if field == nil {
-		if !createIfNotExists {
-			return nil, fmt.Errorf("failed to find array field %q", segment.name)
-		}
-		arr := make([]any, segment.index+1)
-		currentMap[segment.name] = arr
-		return arr, nil
-	}
-
-	arr, ok := field.([]any)
-	if !ok {
-		return nil, fmt.Errorf("failed to convert field %q to array, got %T", segment.name, field)
+func updateResource(res *resource.Resource, updatedData any) error {
+	// After modifying the map, we must marshal it back to YAML and create a new
+	// resource object to ensure the internal state is consistent.
+	updatedYAML, err := yaml.Marshal(updatedData)
+	if err != nil {
+		return fmt.Errorf("failed to marshal updated YAML: %w", err)
 	}
 
-	if segment.index >= len(arr) {
-		if !createIfNotExists {
-			return nil, fmt.Errorf("failed to access array index %d for field %q (length: %d)", segment.index, segment.name, len(arr))
-		}
-		newArr := make([]any, segment.index+1)
-		copy(newArr, arr)
-		for i := len(arr); i <= segment.index; i++ {
-			newArr[i] = make(map[string]any)
-		}
-		currentMap[segment.name] = newArr
-		return newArr, nil
+	rf := resource.NewFactory(nil)
+	newRes, err := rf.FromBytes(updatedYAML)
+	if err != nil {
+		return fmt.Errorf("failed to create resource from updated YAML: %w", err)
 	}
 
-	return arr, nil
+	// Atomically replace the old resource content with the new, fully updated content
+	// to prevent partial updates or data loss.
+	res.ResetRNode(newRes)
+	return nil
 }
 
-func setFieldValue(current any, segment fieldSegment, value any, createIfNotExists bool) error {
-	currentMap, ok := current.(map[string]any)
-	if !ok {
-		return fmt.Errorf("failed to convert current value to map[string]any, got %T", current)
+// isNumericString checks if a string represents a valid non-negative integer.
+func isNumericString(s string) bool {
+	if s == "" {
+		return false
 	}
-
-	if segment.isArray {
-		field := currentMap[segment.name]
-		arr, err := ensureArrayWithCapacity(currentMap, segment, field, createIfNotExists)
-		if err != nil {
-			return err
+	for _, r := range s {
+		if r < '0' || r > '9' {
+			return false
 		}
-		arr[segment.index] = value
-		return nil
 	}
-
-	currentMap[segment.name] = value
-	return nil
+	return true
 }