Add integer keyValue support to mutation path parser / mutators (#1394)

* Add integer keyValue support to mutation path parser / mutators.

Previously, the mutation path parser only supported string types for
keyValues selecting items in a list. This PR adds support for integers,
such that we can now express paths like this:

```
spec.containers[name: opa].ports[containerPort: 8888].name
```

Integers are assumed to be represented in decimal and are interpreted as
positive int64 values.

The mutation code was updated accordingly to match based on type.
Additionally, string serialization of paths was updated to be more
selective on which strings are quoted.

Fixes #1355

* Add additional tests, resolve lint warnings.

* Additional test cases for digits in unquoted identifiers

* Fixed additional parsing  roundtripping cases

Signed-off-by: Oren Shomron <shomron@gmail.com>

pkg/mutation/mutators/assign_mutator.go

@@ -323,8 +323,8 @@ func validateObjectAssignedToList(p parser.Path, value interface{}, assignName s
 	if !ok {
 		return errors.New("only full objects can be appended to lists, Assign: " + assignName)
 	}
-	if *listNode.KeyValue != valueMap[listNode.KeyField] {
-		return fmt.Errorf("adding object to list with different key %s: list key %s, object key %s, assign: %s", listNode.KeyField, *listNode.KeyValue, valueMap[listNode.KeyField], assignName)
+	if listNode.KeyValue != valueMap[listNode.KeyField] {
+		return fmt.Errorf("adding object to list with different key %s: list key %v, object key %v, assign: %s", listNode.KeyField, listNode.KeyValue, valueMap[listNode.KeyField], assignName)
 	}
 
 	return nil

pkg/mutation/mutators/assign_mutator_test.go

@@ -2,6 +2,7 @@ package mutators
 
 import (
 	"encoding/json"
+	"errors"
 	"fmt"
 	"reflect"
 	"testing"
@@ -11,6 +12,7 @@ import (
 	mutationsv1alpha1 "github.com/open-policy-agent/gatekeeper/apis/mutations/v1alpha1"
 	"github.com/open-policy-agent/gatekeeper/pkg/mutation/match"
 	path "github.com/open-policy-agent/gatekeeper/pkg/mutation/path/tester"
+	v1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/runtime"
@@ -67,9 +69,9 @@ func newObj(value interface{}, path ...string) map[string]interface{} {
 	root := map[string]interface{}{}
 	current := root
 	for _, node := range path {
-		new := map[string]interface{}{}
-		current[node] = new
-		current = new
+		m := map[string]interface{}{}
+		current[node] = m
+		current = m
 	}
 	if err := unstructured.SetNestedField(root, value, path...); err != nil {
 		panic(err)
@@ -91,6 +93,14 @@ func newFoo(spec map[string]interface{}) *unstructured.Unstructured {
 	return &unstructured.Unstructured{Object: data}
 }
 
+func newPod(pod *v1.Pod) *unstructured.Unstructured {
+	u, err := runtime.DefaultUnstructuredConverter.ToUnstructured(pod)
+	if err != nil {
+		panic(fmt.Sprintf("converting pod to unstructured: %v", err))
+	}
+	return &unstructured.Unstructured{Object: u}
+}
+
 func ensureObj(u *unstructured.Unstructured, expected interface{}, path ...string) error {
 	v, exists, err := unstructured.NestedFieldNoCopy(u.Object, path...)
 	if err != nil {
@@ -1499,3 +1509,219 @@ func TestApplyTo(t *testing.T) {
 		})
 	}
 }
+
+var testPod = &v1.Pod{
+	TypeMeta: metav1.TypeMeta{
+		APIVersion: "v1",
+		Kind:       "Pod",
+	},
+	ObjectMeta: metav1.ObjectMeta{
+		Name:      "opa",
+		Namespace: "production",
+		Labels:    map[string]string{"owner": "me.agilebank.demo"},
+	},
+	Spec: v1.PodSpec{
+		Containers: []v1.Container{
+			{
+				Name:  "opa",
+				Image: "openpolicyagent/opa:0.9.2",
+				Args: []string{
+					"run",
+					"--server",
+					"--addr=localhost:8080",
+				},
+				Ports: []v1.ContainerPort{
+					{
+						ContainerPort: 8080,
+						Name:          "out-of-scope",
+					},
+					{
+						ContainerPort: 8888,
+						Name:          "unchanged",
+					},
+				},
+			},
+		},
+	},
+}
+
+func TestAssign(t *testing.T) {
+	tests := []struct {
+		name string
+		obj  *unstructured.Unstructured
+		cfg  *assignTestCfg
+		fn   func(*unstructured.Unstructured) error
+	}{
+		{
+			name: "integer key value",
+			cfg: &assignTestCfg{
+				applyTo: []match.ApplyTo{{Groups: []string{""}, Versions: []string{"v1"}, Kinds: []string{"Pod"}}},
+				path:    `spec.containers[name: opa].ports[containerPort: 8888].name`,
+				value:   makeValue("modified"),
+			},
+			obj: newPod(testPod),
+			fn: func(u *unstructured.Unstructured) error {
+				var pod v1.Pod
+				err := runtime.DefaultUnstructuredConverter.FromUnstructured(u.Object, &pod)
+				if err != nil {
+					return err
+				}
+
+				if len(pod.Spec.Containers) != 1 {
+					return fmt.Errorf("incorrect number of containers: %d", len(pod.Spec.Containers))
+				}
+				c := pod.Spec.Containers[0]
+				if len(c.Ports) != 2 {
+					return fmt.Errorf("incorrect number of ports: %d", len(c.Ports))
+				}
+				p := c.Ports[1]
+				if p.ContainerPort != int32(8888) {
+					return fmt.Errorf("incorrect containerPort: %d", p.ContainerPort)
+				}
+				if p.Name != "modified" {
+					return fmt.Errorf("incorrect port name: %s", p.Name)
+				}
+				return nil
+			},
+		},
+		{
+			name: "new integer key value",
+			cfg: &assignTestCfg{
+				applyTo: []match.ApplyTo{{Groups: []string{""}, Versions: []string{"v1"}, Kinds: []string{"Pod"}}},
+				path:    `spec.containers[name: opa].ports[containerPort: 2001].name`,
+				value:   makeValue("added"),
+			},
+			obj: newPod(testPod),
+			fn: func(u *unstructured.Unstructured) error {
+				var pod v1.Pod
+				err := runtime.DefaultUnstructuredConverter.FromUnstructured(u.Object, &pod)
+				if err != nil {
+					return err
+				}
+
+				if len(pod.Spec.Containers) != 1 {
+					return fmt.Errorf("incorrect number of containers: %d", len(pod.Spec.Containers))
+				}
+				c := pod.Spec.Containers[0]
+				if len(c.Ports) != 3 {
+					return fmt.Errorf("incorrect number of ports: %d", len(c.Ports))
+				}
+				p := c.Ports[2]
+				if p.ContainerPort != int32(2001) {
+					return fmt.Errorf("incorrect containerPort: %d", p.ContainerPort)
+				}
+				if p.Name != "added" {
+					return fmt.Errorf("incorrect port name: %s", p.Name)
+				}
+				return nil
+			},
+		},
+		{
+			name: "truncated integer key value",
+			cfg: &assignTestCfg{
+				applyTo: []match.ApplyTo{{Groups: []string{""}, Versions: []string{"v1"}, Kinds: []string{"Pod"}}},
+				path:    `spec.containers[name: opa].ports[containerPort: 4294967297].name`,
+				value:   makeValue("added"),
+			},
+			obj: newPod(testPod),
+			fn: func(u *unstructured.Unstructured) error {
+				var pod v1.Pod
+				err := runtime.DefaultUnstructuredConverter.FromUnstructured(u.Object, &pod)
+				if err != nil {
+					return err
+				}
+
+				if len(pod.Spec.Containers) != 1 {
+					return fmt.Errorf("incorrect number of containers: %d", len(pod.Spec.Containers))
+				}
+				c := pod.Spec.Containers[0]
+				if len(c.Ports) != 3 {
+					return fmt.Errorf("incorrect number of ports: %d", len(c.Ports))
+				}
+				p := c.Ports[2]
+				// Note in this test case, the UnstructuredConverter truncates our 64bit containerPort down to 32bit.
+				// The actual mutation was done in 64bit.
+				if p.ContainerPort != int32(1) {
+					return fmt.Errorf("incorrect containerPort: %d", p.ContainerPort)
+				}
+				if p.Name != "added" {
+					return fmt.Errorf("incorrect port name: %s", p.Name)
+				}
+				return nil
+			},
+		},
+		{
+			name: "type mismatch for key value",
+			cfg: &assignTestCfg{
+				applyTo: []match.ApplyTo{{Groups: []string{""}, Versions: []string{"v1"}, Kinds: []string{"Pod"}}},
+				path:    `spec.containers[name: opa].ports[containerPort: "8888"].name`,
+				value:   makeValue("modified"),
+			},
+			obj: newPod(testPod),
+			fn: func(u *unstructured.Unstructured) error {
+				var pod v1.Pod
+				err := runtime.DefaultUnstructuredConverter.FromUnstructured(u.Object, &pod)
+				if err == nil {
+					return errors.New("expected type mismatch when deserializing mutated pod")
+				}
+
+				containers, err := nestedMapSlice(u.Object, "spec", "containers")
+				if err != nil {
+					return fmt.Errorf("fetching containers: %w", err)
+				}
+				if len(containers) != 1 {
+					return fmt.Errorf("incorrect number of containers: %d", len(containers))
+				}
+				ports, err := nestedMapSlice(containers[0], "ports")
+				if err != nil {
+					return fmt.Errorf("fetching ports: %w", err)
+				}
+				if len(ports) != 3 {
+					return fmt.Errorf("incorrect number of ports: %d", len(containers))
+				}
+				if ports[1]["containerPort"] != 8888 && ports[1]["name"] != "unchanged" {
+					return fmt.Errorf("port was incorrectly modified: %v", ports[1])
+				}
+				if ports[2]["containerPort"] != "8888" && ports[1]["name"] != "modified" {
+					return fmt.Errorf("type mismatched port was not added as expected: %v", ports[1])
+				}
+
+				return nil
+			},
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			mutator := newAssignMutator(test.cfg)
+			obj := test.obj.DeepCopy()
+			_, err := mutator.Mutate(obj)
+			if err != nil {
+				t.Fatalf("failed mutation: %s", err)
+			}
+			if err := test.fn(obj); err != nil {
+				t.Errorf("failed test: %v", err)
+			}
+		})
+	}
+}
+
+func nestedMapSlice(u map[string]interface{}, fields ...string) ([]map[string]interface{}, error) {
+	lst, ok, err := unstructured.NestedSlice(u, fields...)
+	if !ok {
+		return nil, fmt.Errorf("not found")
+	}
+	if err != nil {
+		return nil, err
+	}
+
+	out := make([]map[string]interface{}, len(lst))
+	for i := range lst {
+		v, ok := lst[i].(map[string]interface{})
+		if !ok {
+			return nil, fmt.Errorf("unexpected type: %T, expected map[string]interface{}", lst[i])
+		}
+		out[i] = v
+	}
+	return out, nil
+}

pkg/mutation/mutators/core/mutation_function.go

@@ -112,7 +112,7 @@ func (s *mutatorState) mutateInternal(current interface{}, depth int) (bool, int
 				elementFound = true
 			} else if listElementAsObject, ok := listElement.(map[string]interface{}); ok {
 				if elementValue, ok := listElementAsObject[key]; ok {
-					if *castPathEntry.KeyValue == elementValue {
+					if castPathEntry.KeyValue == elementValue {
 						if !s.tester.ExistsOkay(depth) {
 							return false, nil, nil
 						}
@@ -166,10 +166,10 @@ func (s *mutatorState) setListElementToValue(currentAsList []interface{}, listPa
 	if listPathEntry.KeyValue == nil {
 		return false, nil, errors.New("encountered nil key value when setting a new list element")
 	}
-	keyValue := *listPathEntry.KeyValue
+	keyValue := listPathEntry.KeyValue
 
 	for i, listElement := range currentAsList {
-		if elementValue, found, err := nestedString(listElement, key); err != nil {
+		if elementValue, found, err := nestedFieldNoCopy(listElement, key); err != nil {
 			return false, nil, err
 		} else if found && keyValue == elementValue {
 			newKeyValue, ok := newValueAsObject[key]
@@ -211,15 +211,15 @@ func (s *mutatorState) createMissingElement(depth int) (interface{}, error) {
 		if castPathEntry.KeyValue == nil {
 			return nil, fmt.Errorf("list entry has no key value")
 		}
-		nextAsObject[castPathEntry.KeyField] = *castPathEntry.KeyValue
+		nextAsObject[castPathEntry.KeyField] = castPathEntry.KeyValue
 	}
 	return next, nil
 }
 
-func nestedString(current interface{}, key string) (string, bool, error) {
+func nestedFieldNoCopy(current interface{}, key string) (interface{}, bool, error) {
 	currentAsMap, ok := current.(map[string]interface{})
 	if !ok {
 		return "", false, fmt.Errorf("cast error, unable to case %T to map[string]interface{}", current)
 	}
-	return unstructured.NestedString(currentAsMap, key)
+	return unstructured.NestedFieldNoCopy(currentAsMap, key)
 }

pkg/mutation/path/parser/errors.go

@@ -0,0 +1,42 @@
+/*
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package parser
+
+import (
+	"errors"
+	"fmt"
+)
+
+var (
+	// ErrTrailingSeparator indicates a parsing error due to an illegal trailing separator.
+	ErrTrailingSeparator = errors.New("trailing separators are forbidden")
+	// ErrUnexpectedToken indicates a parsing error due to an unexpected token.
+	ErrUnexpectedToken = errors.New("unexpected token")
+	// ErrInvalidInteger indicates a parsing error due to an invalid integer, such as integer overflow.
+	ErrInvalidInteger = invalidIntegerError{}
+)
+
+type invalidIntegerError struct {
+	s string
+}
+
+func (e invalidIntegerError) Error() string {
+	return fmt.Sprintf("invalid integer: %s", e.s)
+}
+func (e invalidIntegerError) Is(target error) bool {
+	_, ok := target.(invalidIntegerError)
+	return ok
+}