Add pathtest functionality to assign mutators #1101

maxsmythe · 2021-01-29T05:15:05Z

Signed-off-by: Max Smythe smythe@google.com

What this PR does / why we need it:

Implementing path tests per #1079

Which issue(s) this PR fixes (optional, using fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when the PR gets merged):
Fixes #1079

Special notes for your reviewer:

shomron

Very preliminary first pass, haven't gone through all the code yet - will circle back for complete review.

shomron · 2021-02-01T17:30:16Z

pkg/mutation/assign_mutator.go

+	for _, pt := range pts {
+		p, err := parser.Parse(pt.SubPath)
+		if err != nil {
+			return nil, err


I'm not certain the parser will include enough context as to which path failed - we might want to wrap and include that.

SGTM, added a wrap

shomron · 2021-02-01T17:33:09Z

pkg/mutation/assign_mutator.go

+
+func (m *AssignMutator) getTester() (*patht.Tester, error) {
+	if m.tester != nil {
+		return m.tester, nil


What's the concurrency story for this mutator? Do we need to guard here?

I don't think this should be accessed in a concurrent manner, but no reason not to throw a lock around this.

Added an RWMutex

This version is missing the second nil check of the double-checked locking pattern - two threads that saw tester == nil above can serially enter the code below and initialize it twice. Not a huge deal, but probably not what we intended.

I'd suggest using a sync.Once to simplify and fix that issue all at once.

I can definitely add the second nil check.

WRT sync.Once, I think we probably want to run the code every time if the value is nil, so that we can continue to return an appropriate error if nil is the result of an error.

The other option would be to cache the error as well, but that has the potential to make otherwise transient bugs sticky.

It sounds like we have a path forward here but removing lazy initialization.

Yep! Committed a change that switches to eager initialization, should be good now.

pkg/mutation/assign_mutator.go

pkg/mutation/path/tester/tester.go

Signed-off-by: Max Smythe <smythe@google.com>

shomron · 2021-02-03T03:08:07Z

pkg/mutation/assign_mutator.go

+
+func (m *AssignMutator) getTester() (*patht.Tester, error) {
+	if m.tester != nil {
+		return m.tester, nil


This version is missing the second nil check of the double-checked locking pattern - two threads that saw tester == nil above can serially enter the code below and initialize it twice. Not a huge deal, but probably not what we intended.

I'd suggest using a sync.Once to simplify and fix that issue all at once.

shomron · 2021-02-03T03:12:38Z

pkg/mutation/assign_mutator.go

@@ -91,6 +135,9 @@ func (m *AssignMutator) DeepCopy() types.Mutator {
 	}
 	copy(res.path.Nodes, m.path.Nodes)
 	copy(res.bindings, m.bindings)
+	m.mux.RLock()


Can you clarify why we need a lock for this field and not others?

Sure, the reason is that the others are created on start-up via MutatorForAssign.

I could move the initialization there instead of lazy-loading to avoid locking entirely?

That would have the added benefit of avoiding the need to have as many error returns as we do currently.

Agreed - I feel like it simplifies things if we only need to deal with a fully initialized mutator. 👍🏻

shomron · 2021-02-03T03:51:55Z

pkg/mutation/mutation_function.go

+// mutateInternal mutates the resource recursively. It returns false if there has been no change
+// to any downstream objects in the tree, indicating that the mutation should not be persisted
+func (s *mutatorState) mutateInternal(current interface{}, depth int) (bool, interface{}, error) {
+	pathEntry := s.mutator.Path().Nodes[depth]


Should we add bounds checks? It's possible for len(path.Nodes) == 0 with an empty location for example. Are we depending on prior validations?

I think making sure len(path) != 0 is a good idea, as a mutation with no path makes no sense and we should catch that anywhere it leaks.

Looking at other validation code, it appears we are only validating whether location parses, which it would if it were empty. So, I think there are two work items:

Validate Mutate inputs using the minimum standards necessary for making a coherent mutate call

Add more location validation to Assign (I think AssignMeta enforces using a metadata prefix)

SGTM. Assuming we apply additional validations earlier on, are you thinking we should still do bounds checking in places like this or build on the preconditions from validation?

I think we should do "minimum sane input" bounds checking.

If there is no reason a function should see a given input (e.g. mutating a zero length path has no semantic meaning other than maybe overwriting the entire object), we should validate that so we can surface bugs like uninitialized structs.

I don't think we should go stricter than that though, to avoid needing to tweak validation in too many places if we add functionality.

added the sanity checks to the mutate() function

shomron · 2021-02-04T05:14:42Z

pkg/mutation/mutation_function.go

 		nextAsObject, ok := next.(map[string]interface{})
 		if !ok { // Path entry type does not match current object
-			return fmt.Errorf("two consecutive list path entries not allowed: %+v %+v", castPathEntry, nextPathEntry)
+			return nil, fmt.Errorf("two consecutive list path entries not allowed: %+v %+v", castPathEntry, nextPathEntry)
 		}
 		nextAsObject[castPathEntry.KeyField] = *castPathEntry.KeyValue


Is this dereference safe?

It should be... I don't think it's possible to trigger a create against a globbed path entry as a missing entry just doesn't fan out.

We can return an error per above, if you prefer?

Yes, I'd prefer not depending on too many far-away assumptions to avoid panics, as they have a habit of changing under your feet. And even when they don't, it makes it harder to reason about correctness of the code in isolation.

added nil checking

shomron · 2021-02-04T05:17:51Z

pkg/mutation/mutation_function.go

 	}
 	newValueAsObject, ok := newValue.(map[string]interface{})
 	if !ok {
-		return fmt.Errorf("last path entry of type list requires an object value, pathEntry: %+v", listPathEntry)
+		return false, nil, fmt.Errorf("last path entry of type list requires an object value, pathEntry: %+v", listPathEntry)
 	}

 	key := listPathEntry.KeyField
 	keyValue := *listPathEntry.KeyValue


I think this dereference is safe because we know Glob==false, but that requires a lot of knowledge of the parser. It might be worth adding a KeyValue accessor that is nil-safe just in case.

+1

I think the correct answer is to return an error if we get an unexpected nil here.

added nil checking

pkg/mutation/assign_mutator.go

Signed-off-by: Max Smythe <smythe@google.com>

codecov-io · 2021-02-06T03:11:37Z

Codecov Report

Merging #1101 (118abb7) into master (be2f5b4) will increase coverage by 0.30%.
The diff coverage is 57.48%.

@@            Coverage Diff             @@
##           master    #1101      +/-   ##
==========================================
+ Coverage   48.08%   48.39%   +0.30%     
==========================================
  Files          62       63       +1     
  Lines        4274     4319      +45     
==========================================
+ Hits         2055     2090      +35     
- Misses       1964     1974      +10     
  Partials      255      255

Flag	Coverage Δ
unittests	`48.39% <57.48%> (+0.30%)`	⬆️

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

Impacted Files	Coverage Δ
pkg/audit/manager.go	`0.00% <0.00%> (ø)`
pkg/controller/constraint/constraint_controller.go	`4.22% <0.00%> (-0.02%)`	⬇️
pkg/controller/sync/sync_controller.go	`0.00% <0.00%> (ø)`
pkg/metrics/exporter.go	`0.00% <0.00%> (ø)`
pkg/readiness/ready_tracker.go	`69.09% <ø> (-0.73%)`	⬇️
pkg/util/pack.go	`0.00% <0.00%> (ø)`
pkg/watch/replay.go	`71.08% <ø> (+0.96%)`	⬆️
pkg/webhook/mutation.go	`12.68% <0.00%> (ø)`
pkg/webhook/policy.go	`27.31% <0.00%> (ø)`
pkg/target/target.go	`66.03% <25.00%> (ø)`
... and 16 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 9d424ff...118abb7. Read the comment docs.

shomron

LGTM!

maxsmythe · 2021-02-09T19:52:07Z

🎊

* Add pathtest functionality to assign mutators Signed-off-by: Max Smythe <smythe@google.com> * Add locking, deepcopy path tester memoization Signed-off-by: Max Smythe <smythe@google.com> * Add path parsing error context Signed-off-by: Max Smythe <smythe@google.com> * Tester should also have a DeepCopy method Signed-off-by: Max Smythe <smythe@google.com> * Eager initialize path tester, guard against nil Signed-off-by: Max Smythe <smythe@google.com>

maxsmythe force-pushed the pathtest branch from 7b0c991 to 95075ea Compare January 29, 2021 05:24

maxsmythe requested review from brycecr, ritazh, shomron and sozercan January 29, 2021 05:39

shomron reviewed Feb 1, 2021

View reviewed changes

maxsmythe force-pushed the pathtest branch 2 times, most recently from 40a6dc1 to a7ee588 Compare February 2, 2021 05:06

maxsmythe added 3 commits February 2, 2021 11:04

Add pathtest functionality to assign mutators

c16f77b

Signed-off-by: Max Smythe <smythe@google.com>

Add locking, deepcopy path tester memoization

a1e855f

Signed-off-by: Max Smythe <smythe@google.com>

Add path parsing error context

fd849b5

Signed-off-by: Max Smythe <smythe@google.com>

maxsmythe force-pushed the pathtest branch from a7ee588 to fec7e92 Compare February 2, 2021 19:04

Tester should also have a DeepCopy method

95dffd7

Signed-off-by: Max Smythe <smythe@google.com>

maxsmythe force-pushed the pathtest branch from fec7e92 to 95dffd7 Compare February 3, 2021 01:04

maxsmythe mentioned this pull request Feb 4, 2021

Add value testing to the assign mutator #1113

Merged

shomron reviewed Feb 4, 2021

View reviewed changes

Eager initialize path tester, guard against nil

f00fd3c

Signed-off-by: Max Smythe <smythe@google.com>

shomron approved these changes Feb 9, 2021

View reviewed changes

Merge branch 'master' into pathtest

118abb7

maxsmythe merged commit e8ed13c into open-policy-agent:master Feb 9, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add pathtest functionality to assign mutators #1101

Add pathtest functionality to assign mutators #1101

maxsmythe commented Jan 29, 2021

shomron left a comment

shomron Feb 1, 2021

maxsmythe Feb 1, 2021

shomron Feb 1, 2021

maxsmythe Feb 1, 2021

maxsmythe Feb 1, 2021

shomron Feb 3, 2021

maxsmythe Feb 4, 2021

shomron Feb 6, 2021

maxsmythe Feb 6, 2021

shomron Feb 3, 2021

shomron Feb 3, 2021

maxsmythe Feb 4, 2021

maxsmythe Feb 4, 2021

shomron Feb 6, 2021

maxsmythe Feb 6, 2021

shomron Feb 3, 2021

maxsmythe Feb 4, 2021

shomron Feb 6, 2021

maxsmythe Feb 6, 2021

maxsmythe Feb 6, 2021 •

edited

shomron Feb 4, 2021

maxsmythe Feb 4, 2021

shomron Feb 6, 2021

maxsmythe Feb 6, 2021

maxsmythe Feb 6, 2021

shomron Feb 4, 2021

maxsmythe Feb 4, 2021

shomron Feb 6, 2021

maxsmythe Feb 6, 2021

codecov-io commented Feb 6, 2021 •

edited

shomron left a comment

maxsmythe commented Feb 9, 2021

Add pathtest functionality to assign mutators #1101

Add pathtest functionality to assign mutators #1101

Conversation

maxsmythe commented Jan 29, 2021

shomron 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

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

maxsmythe Feb 6, 2021 • edited

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 Feb 6, 2021 • edited

Codecov Report

shomron left a comment

Choose a reason for hiding this comment

maxsmythe commented Feb 9, 2021

maxsmythe Feb 6, 2021 •

edited

codecov-io commented Feb 6, 2021 •

edited