Write location parser for Mutations #946

shomron · 2020-11-07T04:16:16Z

The location parser parses the location specification language used
for matching Kubernetes object fields.

Example:

p := NewParser("spec.containers[name:*].securityContext")
root, err := p.Parse()

Closes #915

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

shomron · 2020-11-07T04:22:45Z

@maxsmythe @fedepaol @ritazh @sozercan @brycecr here's a first go at the location parser.
There are a couple of questions in the parser test cases as to whether we should prevent certain syntax.

Also - this language needs a name. Let me know your thoughts! 🙂

fedepaol · 2020-11-09T11:12:42Z

Also - this language needs a name. Let me know your thoughts! slightly_smiling_face

Wasn't it the max's syntax :P ?

fedepaol

Added a few comments

pkg/path/parser/doc.go

fedepaol · 2020-11-09T11:20:19Z

pkg/path/parser/parser.go

+	err       error
+}
+
+func NewParser(input string) *Parser {


From a consumer perspective, the flow will always be

p := NewParser("string") p.Parse()

I think it would be better to have a single exported Parse function that hides the parser.

Sounds good!

pkg/path/parser/parser_test.go

fedepaol · 2020-11-09T11:23:53Z

pkg/path/token/scanner_test.go

+			},
+		},
+		{
+			input: `"🤔☕️❗️"`,


fedepaol · 2020-11-09T13:59:03Z

pkg/path/parser/parser.go

+// expect returns whether the next token matches our expectation,
+// and if so advances to that token.
+// Otherwise returns false and doesn't advance.
+func (p *Parser) expect(t token.Type) bool {


The for loop below is a bit hard to get without reading this comment first.
Would it make sense to use only expectPeek (and maybe change to something less "assertive" like peekMatches) and explicitly advance via p.next() ?

Or even better, take the "peek" value and use it as the value to switch on? I think the result will be more understandable.

Combining assertion with advancing through the tokens seems like a pattern that's out there, and combining that with optional matching felt like it streamlined the code and removed boilerplate.
I'm not married to this - but are there other ways of improving clarify? Can the function name be more descriptive? Can the comments be better, or would a description of the parser at the top help?

Yes I think the thing that confused me the most was the hidden side effect of advancing when the expectation is matched. A comment before the switch, or a better name will probably make it clearer.

Now, the hard part is to find a decent name :-)
Something like advanceWhenNext, or advanceIfNext maybe. But I am also fine with adding a comment before the switch if we don't find a better alternative, I don't want this to be blocking.

pkg/path/parser/node.go

maxsmythe

Thanks for this! Very clean.

re: Name

"Max's Syntax" <- ugh :p that'll either work real well or really poorly!

"Max's unplanned location-focused operator controls" -> Muloc?

"Shomron's Folly"? :p

"Put things here" -> PTH

"Yet another path language" -> YAPL

pkg/path/parser/doc.go

pkg/path/parser/node.go

maxsmythe · 2020-11-10T02:41:17Z

pkg/path/token/scanner.go

+}
+
+func isSpace(r rune) bool {
+	return r == ' ' || r == '\t'


Should we also skip newlines?

Easy enough.

maxsmythe · 2020-11-10T02:49:19Z

pkg/path/token/scanner.go

+	case ':':
+		tok = Token{Type: COLON, Literal: string(s.ch)}
+	case '"', '\'':
+		tok.Type = IDENT


Is IDENT a well-known abbreviation? I think it means "identity" or "identifier" in this case?

Not sure that that is the most apt name given it can hold:

A proper identifier in the form of a field name

A value in the form of a key-field value

No better name is coming to mind though.

IDENT is short for identifier, and in this case means a name that is not a keyword. I've seen that shorthand in many parsers, for example here's a search across the go source.

Our language doesn't need to differentiate between a string and an identifier so I decide not to define a separate string token. Not sure whether it's textbook but it seemed to make sense at the time ¯_(ツ)_/¯

SGTM, if we think of a better name we can change this later. literal maybe? I don't love any of `em

maxsmythe · 2020-11-10T02:57:25Z

pkg/path/token/scanner_test.go

+			},
+		},
+		{
+			input: `Moo🐙`,


This should be a syntax error.

The octopus does NOT say moo

pkg/path/parser/node.go

maxsmythe · 2020-11-10T03:15:13Z

pkg/path/parser/parser_test.go

+			},
+		},
+		{
+			// TODO: Is this useful or should we make it a parsing error?


IMO it's a parsing error.

I could see someone needing to reference a nil-valued var, would we be able to have a "nil" keyword?

Part of the reason for requiring quotes post-colon is to reserve the possibility of other special keywords.

Another option could be to have an anti-quote (e.g. a backtic) to signify "this value is a JS keyword"

maxsmythe · 2020-11-10T03:19:07Z

pkg/path/parser/parser_test.go

+			},
+		},
+		{
+			// TODO: Should this be allowed?


It should not be allowed. It's impossible to have a list-of-lists, as an intermediate object is required to store the key fields:

{ "spec": [ { "myKey": "some-value", "nestedList": [ {"secondKey": "secondValue"} ] }

Note the need to access nestedList

maxsmythe · 2020-11-14T01:47:39Z

pkg/mutation/mutator.go

 	"github.com/pkg/errors"
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/runtime"
+


nit: consolidate imports

pkg/path/parser/node.go

mmirecki · 2020-11-16T13:30:17Z

pkg/mutation/path/parser/node.go

+		return "", false
+	}
+	return *l.KeyValue, true
+}


The naming is a bit of what we have in the [interface design doc] (also the additional Path type)(https://docs.google.com/document/d/1c5Z3g6Zsfmga7xod4--t6RrXamdAaMaqYzPNujtZb10/edit?ts=5f98003c)
Can we either update the PR or the doc?

Updated the doc

maxsmythe · 2020-11-17T05:16:57Z

pkg/mutation/path/parser/parser_test.go

+			expectErr: true,
+		},
+		{
+			// List cannot follow valid list


Do we also want to check pathentries that end in a globbed list? That should also be disallowed, as then we would be setting multiple objects to the exact same value.

Hmm - I'm not sure that be a parse error, but perhaps a validation to apply at a higher level?

That's a fair thought, I think the path tests will allow glob-terminated locations.

codecov-io · 2020-11-17T22:40:35Z

Codecov Report

Merging #946 (15cb728) into master (5320da3) will increase coverage by 2.18%.
The diff coverage is 90.13%.

@@            Coverage Diff             @@
##           master     #946      +/-   ##
==========================================
+ Coverage   43.57%   45.76%   +2.18%     
==========================================
  Files          52       56       +4     
  Lines        3222     3374     +152     
==========================================
+ Hits         1404     1544     +140     
- Misses       1622     1634      +12     
  Partials      196      196

Flag	Coverage Δ
unittests	`45.76% <90.13%> (+2.18%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
pkg/mutation/mutator.go	`77.77% <ø> (ø)`
pkg/mutation/path/parser/node.go	`0.00% <0.00%> (ø)`
pkg/mutation/path/token/token.go	`0.00% <0.00%> (ø)`
pkg/mutation/path/token/scanner.go	`90.24% <90.24%> (ø)`
pkg/mutation/path/parser/parser.go	`100.00% <100.00%> (ø)`
...onstrainttemplate/constrainttemplate_controller.go	`55.70% <0.00%> (-0.98%)`	⬇️
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5320da3...15cb728. Read the comment docs.

shomron · 2020-11-17T22:43:10Z

I think I've incorporated all feedback - let me know if there's any other changes pending.

maxsmythe

LGTM, letting this sit overnight in case @fedepaol has any more feedback.

shomron · 2020-11-18T05:32:57Z

Sounds good - thank you for the thorough review!

fedepaol · 2020-11-18T11:43:14Z

pkg/mutation/path/parser/parser.go

+	return p.peekToken.Type == t
+}
+
+func (p *Parser) Parse() (*Path, error) {


nit: this can be private?

I'll keep the method exported in case the parser is exported in the future. It is the public-facing interface to the parser, even if the parser is not public.

fedepaol · 2020-11-18T11:43:36Z

pkg/mutation/path/parser/parser.go

+	"github.com/open-policy-agent/gatekeeper/pkg/mutation/path/token"
+)
+
+type Parser struct {


nit: can be the parser private?

Sure, fixed in latest commit.

fedepaol · 2020-11-18T11:52:15Z

Just a comment about the visibility of the parser.
Really really neat! I like this!

The location parser parses the location specification language used for matching Kubernetes object fields. Example: ``` p := NewParser("spec.containers[name:*].securityContext") root, err := p.Parse() ... Closes open-policy-agent#915 Signed-off-by: Oren Shomron <shomron@gmail.com>

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

* Relocated location parser under pkg/mutation * Rename Object.Value -> Object.Reference * Rename Root -> Path * Support newlines as whitespace * Disallow listSpec that does not follow object path segment * In listSpec, disallow nil fieldValue if glob is not specified * Octopus can still say Moo Signed-off-by: Oren Shomron <shomron@gmail.com>

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

The parser functionality is exposed behind the Parse function. The parser struct itself is not currently needed outside the package, so we will unexport it for now. Signed-off-by: Oren Shomron <shomron@gmail.com>

maxsmythe

LGTM,

ritazh

lgtm

shomron force-pushed the location-parser branch from be72fdb to d2fbc8c Compare November 7, 2020 04:34

fedepaol reviewed Nov 9, 2020

View reviewed changes

maxsmythe reviewed Nov 10, 2020

View reviewed changes

shomron force-pushed the location-parser branch from 4a60a4d to 41b90ab Compare November 13, 2020 23:07

maxsmythe reviewed Nov 14, 2020

View reviewed changes

mmirecki reviewed Nov 16, 2020

View reviewed changes

maxsmythe reviewed Nov 17, 2020

View reviewed changes

shomron force-pushed the location-parser branch from 5421857 to 3f293c2 Compare November 17, 2020 22:37

maxsmythe approved these changes Nov 18, 2020

View reviewed changes

fedepaol reviewed Nov 18, 2020

View reviewed changes

shomron force-pushed the location-parser branch from dfc2df4 to 1d20dbf Compare November 18, 2020 19:08

shomron added 5 commits November 18, 2020 11:51

Simplify location parser exported interface

2e43274

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

Consolidate imports

abeadff

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

Unexport location parser

15cb728

The parser functionality is exposed behind the Parse function. The parser struct itself is not currently needed outside the package, so we will unexport it for now. Signed-off-by: Oren Shomron <shomron@gmail.com>

shomron force-pushed the location-parser branch from 1d20dbf to 15cb728 Compare November 18, 2020 19:52

maxsmythe approved these changes Nov 18, 2020

View reviewed changes

ritazh approved these changes Nov 18, 2020

View reviewed changes

maxsmythe merged commit 3444c16 into open-policy-agent:master Nov 18, 2020

Write location parser for Mutations #946

Write location parser for Mutations #946

Conversation

shomron commented Nov 7, 2020 • edited by ritazh Loading

shomron commented Nov 7, 2020 • edited Loading

fedepaol commented Nov 9, 2020

fedepaol left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shomron Nov 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxsmythe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Nov 17, 2020 • edited Loading

Codecov Report

shomron commented Nov 17, 2020

maxsmythe left a comment

Choose a reason for hiding this comment

shomron commented Nov 18, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fedepaol commented Nov 18, 2020

maxsmythe left a comment

Choose a reason for hiding this comment

ritazh left a comment

Choose a reason for hiding this comment

shomron commented Nov 7, 2020 •

edited by ritazh

Loading

shomron commented Nov 7, 2020 •

edited

Loading

shomron Nov 9, 2020 •

edited

Loading

codecov-io commented Nov 17, 2020 •

edited

Loading