From 05f559e6d1c216af6d130c1f9855c242c9b6fc8b Mon Sep 17 00:00:00 2001 From: Oren Shomron Date: Fri, 2 Jul 2021 09:29:04 -0700 Subject: [PATCH] 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 --- pkg/mutation/mutators/assign_mutator.go | 4 +- pkg/mutation/mutators/assign_mutator_test.go | 232 +++++++++++++++++- .../mutators/core/mutation_function.go | 12 +- pkg/mutation/path/parser/errors.go | 42 ++++ pkg/mutation/path/parser/node.go | 54 ++-- pkg/mutation/path/parser/parser.go | 35 ++- pkg/mutation/path/parser/parser_test.go | 135 ++++++++-- pkg/mutation/path/token/scanner.go | 79 +++--- pkg/mutation/path/token/scanner_test.go | 104 +++++++- pkg/mutation/path/token/token.go | 1 + pkg/mutation/schema/schema.go | 8 +- pkg/mutation/schema/schema_test.go | 4 +- 12 files changed, 611 insertions(+), 99 deletions(-) create mode 100644 pkg/mutation/path/parser/errors.go diff --git a/pkg/mutation/mutators/assign_mutator.go b/pkg/mutation/mutators/assign_mutator.go index 028282dfe7c..bed9ff33e03 100644 --- a/pkg/mutation/mutators/assign_mutator.go +++ b/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 diff --git a/pkg/mutation/mutators/assign_mutator_test.go b/pkg/mutation/mutators/assign_mutator_test.go index de71025e840..52c27ae37aa 100644 --- a/pkg/mutation/mutators/assign_mutator_test.go +++ b/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 +} diff --git a/pkg/mutation/mutators/core/mutation_function.go b/pkg/mutation/mutators/core/mutation_function.go index 33be7a091a8..ab29d972647 100644 --- a/pkg/mutation/mutators/core/mutation_function.go +++ b/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) } diff --git a/pkg/mutation/path/parser/errors.go b/pkg/mutation/path/parser/errors.go new file mode 100644 index 00000000000..3fa2ffc4190 --- /dev/null +++ b/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 +} diff --git a/pkg/mutation/path/parser/node.go b/pkg/mutation/path/parser/node.go index ff45db548e5..412d98d7a5b 100644 --- a/pkg/mutation/path/parser/node.go +++ b/pkg/mutation/path/parser/node.go @@ -106,7 +106,7 @@ func (o Object) String() string { type List struct { KeyField string - KeyValue *string + KeyValue interface{} Glob bool } @@ -125,38 +125,50 @@ func (l List) DeepCopy() List { out := List{} out.KeyField = l.KeyField out.Glob = l.Glob - if l.KeyValue != nil { - out.KeyValue = new(string) - *out.KeyValue = *l.KeyValue - } + // KeyValue (interface{}) will be one of: [string, int, nil] + out.KeyValue = l.KeyValue return out } -func (l List) Value() (string, bool) { - if l.KeyValue == nil { - return "", false - } - return *l.KeyValue, true -} - func (l List) String() string { key := quote(l.KeyField) if l.Glob { return fmt.Sprintf("[%s: *]", key) } - if l.KeyValue != nil { - value := quote(*l.KeyValue) - return fmt.Sprintf("[%s: %s]", key, value) + switch v := l.KeyValue.(type) { + case string: + q := quote(v) + return fmt.Sprintf("[%s: %s]", key, q) + + case int, int64: + return fmt.Sprintf("[%s: %d]", key, v) + + case nil: + default: } // Represents an improperly specified List node. return fmt.Sprintf("[%s: ]", key) } -// quote adds double quotes around the passed string. +// quote optionally adds double quotes around the passed string if needed. +// Quotes are needed for: +// * Strings containing whitespace, quotes, or other "ambiguous" characters that will +// be tokenized as non-strings and need escaping. +// * Strings starting digits, that would otherwise be tokenized as an integer +// * Empty strings func quote(s string) string { - // Using fmt.Sprintf with %q converts whitespace to escape sequences, and we - // don't want that. - s = strings.ReplaceAll(s, `\`, `\\`) - s = strings.ReplaceAll(s, `"`, `\"`) - return `"` + s + `"` + if len(s) == 0 { + return `""` + } + switch { + case strings.ContainsAny(s, "'\"\t\n \\*[]:."), + strings.ContainsAny(s[0:1], "0123456789"): + // Using fmt.Sprintf with %q converts whitespace to escape sequences, and we + // don't want that. + s = strings.ReplaceAll(s, `\`, `\\`) + s = strings.ReplaceAll(s, `"`, `\"`) + return `"` + s + `"` + } + + return s } diff --git a/pkg/mutation/path/parser/parser.go b/pkg/mutation/path/parser/parser.go index ecdc2d6eda6..2a5e1830d58 100644 --- a/pkg/mutation/path/parser/parser.go +++ b/pkg/mutation/path/parser/parser.go @@ -16,18 +16,11 @@ limitations under the License. package parser import ( - "errors" "fmt" "github.com/open-policy-agent/gatekeeper/pkg/mutation/path/token" ) -// Base errors for -var ( - ErrTrailingSeparator = errors.New("trailing separators are forbidden") - ErrUnexpectedToken = errors.New("unexpected token") -) - type parser struct { input string scanner *token.Scanner @@ -141,8 +134,15 @@ func (p *parser) parseList() Node { case p.expect(token.GLOB): out.Glob = true case p.expect(token.IDENT): - val := p.curToken.Literal - out.KeyValue = &val + out.KeyValue = p.curToken.Literal + case p.expect(token.INT): + val, err := parseInt64(p.curToken.Literal) + if err != nil { + p.setError(fmt.Errorf("%w: parsing key value for key: %s", err, out.KeyField)) + return nil + + } + out.KeyValue = val default: p.setError(fmt.Errorf("%w: expected key value or glob in listSpec, got: %s", ErrUnexpectedToken, p.peekToken.String())) return nil @@ -167,3 +167,20 @@ func (p *parser) setError(err error) { } p.err = err } + +// parseInt64 will return the int64 representation of the decimal encoded in the string s. +// This function was written because strconv.ParseInt() parses octal and hexadecimal representations +// which we are not supporting in our syntax. +func parseInt64(s string) (int64, error) { + var result int64 + for _, d := range s { + if d < '0' || d > '9' { + return 0, invalidIntegerError{s: fmt.Sprintf("unexpected digit: %c", d)} + } + result = result*10 + int64(d-'0') + if result < 0 { + return 0, invalidIntegerError{s: fmt.Sprintf("overflow in integer string: %s", s)} + } + } + return result, nil +} diff --git a/pkg/mutation/path/parser/parser_test.go b/pkg/mutation/path/parser/parser_test.go index 1e0575bd051..c0f217cf6af 100644 --- a/pkg/mutation/path/parser/parser_test.go +++ b/pkg/mutation/path/parser/parser_test.go @@ -74,7 +74,7 @@ func TestParser(t *testing.T) { expected: []Node{ &Object{Reference: "spec"}, &Object{Reference: "containers"}, - &List{KeyField: "name", KeyValue: strPtr("*"), Glob: false}, + &List{KeyField: "name", KeyValue: "*", Glob: false}, &Object{Reference: "securityContext"}, }, }, @@ -83,7 +83,7 @@ func TestParser(t *testing.T) { expected: []Node{ &Object{Reference: "spec"}, &Object{Reference: "containers"}, - &List{KeyField: "name", KeyValue: strPtr("foo")}, + &List{KeyField: "name", KeyValue: "foo"}, &Object{Reference: "securityContext"}, }, }, @@ -92,7 +92,7 @@ func TestParser(t *testing.T) { expected: []Node{ &Object{Reference: "spec"}, &Object{Reference: "containers"}, - &List{KeyField: "my key", KeyValue: strPtr("foo bar")}, + &List{KeyField: "my key", KeyValue: "foo bar"}, }, }, { @@ -110,7 +110,7 @@ func TestParser(t *testing.T) { expected: []Node{ &Object{Reference: "spec"}, &Object{Reference: "containers"}, - &List{KeyField: "name", KeyValue: strPtr("")}, + &List{KeyField: "name", KeyValue: ""}, &Object{Reference: "securityContext"}, }, }, @@ -119,7 +119,7 @@ func TestParser(t *testing.T) { expected: []Node{ &Object{Reference: "spec"}, &Object{Reference: "containers"}, - &List{KeyField: "", KeyValue: strPtr("someValue")}, + &List{KeyField: "", KeyValue: "someValue"}, &Object{Reference: "securityContext"}, }, }, @@ -199,6 +199,44 @@ func TestParser(t *testing.T) { input: `spec.[foo: bar]`, wantErr: ErrUnexpectedToken, }, + { + // Integer keyValues + input: `spec.containers[name: opa].ports[containerPort: 8888].name`, + expected: []Node{ + &Object{Reference: "spec"}, + &Object{Reference: "containers"}, + &List{KeyField: "name", KeyValue: "opa"}, + &Object{Reference: "ports"}, + &List{KeyField: "containerPort", KeyValue: int64(8888)}, + &Object{Reference: "name"}, + }, + }, + { + // Integer keyFields not supported + input: `spec.containers[123: opa]`, + wantErr: ErrUnexpectedToken, + }, + { + // Maximum 64bit integer + input: `spec[bignum: 9223372036854775807]`, + expected: []Node{ + &Object{Reference: "spec"}, + &List{KeyField: "bignum", KeyValue: int64(9223372036854775807)}, + }, + }, + { + // Integer overflow + input: `spec[bignum: 9223372036854775808]`, + wantErr: ErrInvalidInteger, + }, + { + // Quoted integers are parsed as strings + input: `spec[quoted: "123"]`, + expected: []Node{ + &Object{Reference: "spec"}, + &List{KeyField: "quoted", KeyValue: "123"}, + }, + }, { // allow leading dash input: `-123-_456_`, @@ -207,12 +245,56 @@ func TestParser(t *testing.T) { }, }, { - // allow leading digits - input: `012345`, + // allow trailing digits + input: `area51`, + expected: []Node{ + &Object{Reference: "area51"}, + }, + }, + { + // field names cannot be integers + input: `012345`, + wantErr: ErrUnexpectedToken, + }, + { + input: `spec.123.bar`, + wantErr: ErrUnexpectedToken, + }, + { + // ...but they can be quoted strings that look like integers + input: `"012345"`, expected: []Node{ &Object{Reference: "012345"}, }, }, + { + input: `spec."123"`, + expected: []Node{ + &Object{Reference: "spec"}, + &Object{Reference: "123"}, + }, + }, + { + input: `spec.studio54.bar`, + expected: []Node{ + &Object{Reference: "spec"}, + &Object{Reference: "studio54"}, + &Object{Reference: "bar"}, + }, + }, + { + // Hexadecimal notation not supported + input: `spec[foo: 0x123]`, + wantErr: ErrUnexpectedToken, + }, + { + // Octal notation not supported, interpreted as decimal. + input: `spec[not_octal: 0123]`, + expected: []Node{ + &Object{Reference: "spec"}, + &List{KeyField: "not_octal", KeyValue: int64(123)}, // rather than 83 + }, + }, { // whitespace must be quoted input: `spec.foo bar`, @@ -262,7 +344,7 @@ func TestParser(t *testing.T) { &Object{Reference: "spec"}, &Object{Reference: "this object"}, &Object{Reference: "is very"}, - &List{KeyField: "much full", KeyValue: strPtr("of everyone's")}, + &List{KeyField: "much full", KeyValue: "of everyone's"}, &Object{Reference: "favorite thing"}, }, }, @@ -278,6 +360,31 @@ func TestParser(t *testing.T) { &Object{Reference: `token-with-\embedded-backslash`}, }, }, + // Verify round-tripping on strings-that-look-like-other-tokens + { + input: `'foo[bar: baz]'`, + expected: []Node{ + &Object{Reference: `foo[bar: baz]`}, + }, + }, + { + input: `'foo[bar:baz]'`, + expected: []Node{ + &Object{Reference: `foo[bar:baz]`}, + }, + }, + { + input: `'foo[bar:*]'`, + expected: []Node{ + &Object{Reference: `foo[bar:*]`}, + }, + }, + { + input: `'dot..dot'`, + expected: []Node{ + &Object{Reference: `dot..dot`}, + }, + }, } for i, tc := range tests { @@ -324,7 +431,7 @@ func TestDeepCopy(t *testing.T) { }, { name: "test list deepcopy", - input: &List{KeyField: "much full", KeyValue: strPtr("of everyone's")}, + input: &List{KeyField: "much full", KeyValue: "of everyone's"}, }, { name: "test list deepcopy with nil nexted pointer", @@ -334,12 +441,12 @@ func TestDeepCopy(t *testing.T) { name: "test path deepcopy", input: &Path{ Nodes: []Node{ - &List{KeyField: "much full", KeyValue: strPtr("of everyone's")}, - &List{KeyField: "name", KeyValue: strPtr("*"), Glob: false}, + &List{KeyField: "much full", KeyValue: "of everyone's"}, + &List{KeyField: "name", KeyValue: "*", Glob: false}, &Object{Reference: "foo\nbar"}, &Path{ Nodes: []Node{ - &List{KeyField: "innername", KeyValue: strPtr("*"), Glob: false}, + &List{KeyField: "innername", KeyValue: "*", Glob: false}, }, }, }, @@ -355,7 +462,3 @@ func TestDeepCopy(t *testing.T) { }) } } - -func strPtr(s string) *string { - return &s -} diff --git a/pkg/mutation/path/token/scanner.go b/pkg/mutation/path/token/scanner.go index cd2c35bb606..b49590648d5 100644 --- a/pkg/mutation/path/token/scanner.go +++ b/pkg/mutation/path/token/scanner.go @@ -45,46 +45,50 @@ func NewScanner(input string) *Scanner { } func (s *Scanner) Next() Token { - var tok Token + var err error + var tok = Token{Type: ERROR} s.skipWhitespace() - switch s.ch { - case eof: - tok = Token{Type: EOF, Literal: ""} - case '.': - tok = Token{Type: SEPARATOR, Literal: string(s.ch)} - case '[': - tok = Token{Type: LBRACKET, Literal: string(s.ch)} - case ']': - tok = Token{Type: RBRACKET, Literal: string(s.ch)} - case '*': - tok = Token{Type: GLOB, Literal: string(s.ch)} - case ':': - tok = Token{Type: COLON, Literal: string(s.ch)} - case '"', '\'': - tok.Type = IDENT - str, err := s.readString() - if err != nil { - tok.Type = ERROR + switch { + // A match on these first set of cases leaves s.ch positioned at the next character to process. + case isDigit(s.ch): + if tok.Literal, err = s.readInt(); err == nil { + tok.Type = INT } - tok.Literal = str - default: - if isAlphaNum(s.ch) { + case isAlphaNum(s.ch): + if tok.Literal, err = s.readIdent(); err == nil { tok.Type = IDENT - str, err := s.readIdent() - if err != nil { - tok.Type = ERROR + } + + default: + // Any of these cases require a subsequent call to s.read() (below) to position the next character. + switch s.ch { + case eof: + tok = Token{Type: EOF, Literal: ""} + case '.': + tok = Token{Type: SEPARATOR, Literal: string(s.ch)} + case '[': + tok = Token{Type: LBRACKET, Literal: string(s.ch)} + case ']': + tok = Token{Type: RBRACKET, Literal: string(s.ch)} + case '*': + tok = Token{Type: GLOB, Literal: string(s.ch)} + case ':': + tok = Token{Type: COLON, Literal: string(s.ch)} + case '"', '\'': + if tok.Literal, err = s.readString(); err == nil { + tok.Type = IDENT } - tok.Literal = str - return tok // Return early to avoid consuming the separator we may be positioned on below. + default: + // default: current character is invalid at this location + s.setError(ErrInvalidCharacter) + tok = Token{Type: ERROR, Literal: string(s.ch)} } - // default: current character is invalid at this location - s.setError(ErrInvalidCharacter) - tok = Token{Type: ERROR, Literal: string(s.ch)} + // Make progress + s.read() } - s.read() return tok } @@ -139,6 +143,15 @@ func (s *Scanner) readIdent() (string, error) { return s.input[start:s.pos], s.err } +// readInt scans a (positive) integer. Signs are not supported. +func (s *Scanner) readInt() (string, error) { + start := s.pos + for isDigit(s.ch) { + s.read() + } + return s.input[start:s.pos], s.err +} + func (s *Scanner) setError(err error) { s.err = ScanError{ Inner: err, @@ -166,6 +179,10 @@ func isAlphaNum(r rune) bool { return true } +func isDigit(r rune) bool { + return '0' <= r && r <= '9' +} + func (s *Scanner) skipWhitespace() { for isSpace(s.ch) { s.read() diff --git a/pkg/mutation/path/token/scanner_test.go b/pkg/mutation/path/token/scanner_test.go index dd638bd6d07..1f5afa5bd8a 100644 --- a/pkg/mutation/path/token/scanner_test.go +++ b/pkg/mutation/path/token/scanner_test.go @@ -81,18 +81,101 @@ func TestScanner(t *testing.T) { {Type: EOF, Literal: ""}, }, }, + { + input: `"one"two`, + expected: []Token{ + {Type: IDENT, Literal: "one"}, + {Type: IDENT, Literal: "two"}, + {Type: EOF, Literal: ""}, + }, + }, + // Integer token support + { + input: `0123`, + expected: []Token{ + {Type: INT, Literal: "0123"}, + {Type: EOF, Literal: ""}, + }, + }, + { + input: `9876543210`, + expected: []Token{ + {Type: INT, Literal: "9876543210"}, + {Type: EOF, Literal: ""}, + }, + }, + // Tokenizer doesn't care about bit length + { + input: `99918446744073709551615`, + expected: []Token{ + {Type: INT, Literal: "99918446744073709551615"}, + {Type: EOF, Literal: ""}, + }, + }, + // Hexadecimal not supported + { + input: `0x1A`, + expected: []Token{ + {Type: INT, Literal: "0"}, + {Type: IDENT, Literal: "x1A"}, + {Type: EOF, Literal: ""}, + }, + }, + // Signs not supported + { + input: `-1024`, + expected: []Token{ + {Type: IDENT, Literal: "-1024"}, + {Type: EOF, Literal: ""}, + }, + }, + { + input: `+1024`, + expected: []Token{ + {Type: ERROR, Literal: "+"}, + }, + wantErr: ErrInvalidCharacter, + }, + // Decimals are not supported + { + input: `3.14`, + expected: []Token{ + {Type: INT, Literal: "3"}, + {Type: SEPARATOR, Literal: "."}, + {Type: INT, Literal: "14"}, + {Type: EOF, Literal: ""}, + }, + }, + // Identifiers can no longer lead with digits { input: `0123_foobar_baz`, + expected: []Token{ + {Type: INT, Literal: "0123"}, + {Type: IDENT, Literal: "_foobar_baz"}, + {Type: EOF, Literal: ""}, + }, + }, + // Quoted numbers are identifiers + { + input: `"0123_foobar_baz"`, expected: []Token{ {Type: IDENT, Literal: "0123_foobar_baz"}, {Type: EOF, Literal: ""}, }, }, + { + input: `"299792458"`, + expected: []Token{ + {Type: IDENT, Literal: "299792458"}, + {Type: EOF, Literal: ""}, + }, + }, { input: ` .0123_foobar_baz .`, expected: []Token{ {Type: SEPARATOR, Literal: "."}, - {Type: IDENT, Literal: "0123_foobar_baz"}, + {Type: INT, Literal: "0123"}, + {Type: IDENT, Literal: "_foobar_baz"}, {Type: SEPARATOR, Literal: "."}, {Type: EOF, Literal: ""}, }, @@ -100,7 +183,24 @@ func TestScanner(t *testing.T) { { input: `0123_foobar-baz`, expected: []Token{ - {Type: IDENT, Literal: "0123_foobar-baz"}, + {Type: INT, Literal: "0123"}, + {Type: IDENT, Literal: "_foobar-baz"}, + {Type: EOF, Literal: ""}, + }, + }, + // Digits can be part of an unquoted identifier, just not at the start. + { + input: `Nexus6`, + expected: []Token{ + {Type: IDENT, Literal: "Nexus6"}, + {Type: EOF, Literal: ""}, + }, + }, + { + input: `ItWasTheSummerOf69'🎸'`, + expected: []Token{ + {Type: IDENT, Literal: "ItWasTheSummerOf69"}, + {Type: IDENT, Literal: "🎸"}, {Type: EOF, Literal: ""}, }, }, diff --git a/pkg/mutation/path/token/token.go b/pkg/mutation/path/token/token.go index 61d0ca96d0b..1a452d9eb61 100644 --- a/pkg/mutation/path/token/token.go +++ b/pkg/mutation/path/token/token.go @@ -22,6 +22,7 @@ const ( ERROR = "ERROR" EOF = "EOF" IDENT = "IDENT" + INT = "INT" LBRACKET = "LBRACKET" RBRACKET = "RBRACKET" SEPARATOR = "SEPARATOR" diff --git a/pkg/mutation/schema/schema.go b/pkg/mutation/schema/schema.go index 6aae4b5197b..7ecd7a11ac6 100644 --- a/pkg/mutation/schema/schema.go +++ b/pkg/mutation/schema/schema.go @@ -371,14 +371,8 @@ func wrapObjErr(obj *parser.Object, err error) *Error { } func wrapListErr(list *parser.List, err error) *Error { - var value string - if list.Glob { - value = "*" - } else { - value = fmt.Sprintf("\"%s\"", *list.KeyValue) - } return &Error{ childError: err, - nodeName: fmt.Sprintf("[\"%s\": %s]", list.KeyField, value), + nodeName: list.String(), } } diff --git a/pkg/mutation/schema/schema_test.go b/pkg/mutation/schema/schema_test.go index acd635600a6..18178db518c 100644 --- a/pkg/mutation/schema/schema_test.go +++ b/pkg/mutation/schema/schema_test.go @@ -1202,13 +1202,13 @@ func TestErrors(t *testing.T) { name: "Long path with conflict at end", first: "spec.intermediate.someValue[hey: \"there\"].buddy.hallo", second: "spec.intermediate.someValue[hey: again].buddy[key: *]", - expectedErr: `spec.intermediate.someValue["hey": "again"].buddy: node type conflict: Object vs List`, + expectedErr: `spec.intermediate.someValue[hey: again].buddy: node type conflict: Object vs List`, }, { name: "Long path with conflict at end, globbed", first: "spec.intermediate.someValue[hey: \"there\"].buddy.hallo", second: "spec.intermediate.someValue[hey: *].buddy[key: *]", - expectedErr: `spec.intermediate.someValue["hey": *].buddy: node type conflict: Object vs List`, + expectedErr: `spec.intermediate.someValue[hey: *].buddy: node type conflict: Object vs List`, }, { name: "Long path with conflict at beginning",