Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Preserve conflicting schemas and report conflicts #1364

Merged
merged 1 commit into from Jul 13, 2021

Conversation

willbeason
Copy link
Member

@willbeason willbeason commented Jun 15, 2021

Pre-warning: Some of the changes in this PR can be split off into
other PRs and others cannot. Many of the changes discussed
which can be split off won't make sense without this PR (e.g. the
changes to parser.Node), but I can put those in their own PR.

When multiple Pods are running mutators, it is possible that they will
ingest mutators in different orders. Before this PR, we would
essentially randomly pick which mutator to honor and discard future
mutators which conflicted.

AssignMutators now directly store []schema.GroupVersionKind
representing the types they match for schema bookkeeping. In the
future we may replace this with a map if we decide to use that
instead of iterating over the ApplyTos. ApplyTos thus have a
convenience method for flattening to the list of GVKs they match.

Various uses of meta.Accessor() have been removed as they are
no longer needed with the relatively new client.Client interface
update. Respective interfaces have been changed to accept
client.Object instead of runtime.Object as in all cases where
this change has been made, we have a client.Object.

Added degenerate cases to unit testing for Match to document
that it is intentional that empty Match fields implicitly match
everything.

Remove Bindings type (which just replicated the ApplyTo struct)
as we just use []schema.GroupVersionKind instead.

Replace combined cmp.Equal+cmp.Diff calls with single call to
cmp.Diff since this is more idiomatic.

Add Key() to Node interface so callers can access the key field
without needing to do type introspection.

Make Path no longer match the Node interface. We never use
a Path as a Node, so this has no functional impact. It does
make code checking for Nodes cleaner as we don't need to
check to see if a Node is of type Path (which in all cases is an error).

Split implicit schema conflict checking into its own type, schema.node.
Large changes to schema.DB to account for that we allow the implicit
schema to be in an inconsistent state, but can be healed by removing
the offending mutators.

As a side effect, this greatly reduced the complexity of DB and makes
testing simpler than it was before. I've completely rewritten the test
cases. It's possible I've missed edge cases - please suggest more.

Modify mutation.System to properly account for that schemas may
enter inconsistent states. Before calling mutate, System now
checks the DB to see if the mutator has conflicts in any of the
types it declares. Note that this means that if mutators Foo and Bar
have a conflict on the v1/Role type, but only Foo modifies the
v1beta1/Role type, then Foo will not perform mutations on
v1beta1/Role objects.

Define Error string type alias in util/ for use throughout the repository
per comment on previous PR.

Fixes: #1216

Signed-off-by: Will Beason willbeason@google.com

@willbeason willbeason marked this pull request as draft June 15, 2021 21:51
@willbeason willbeason marked this pull request as ready for review June 15, 2021 22:08
pkg/mutation/path/parser/node.go Outdated Show resolved Hide resolved
pkg/mutation/schema/node.go Outdated Show resolved Hide resolved
pkg/mutation/schema/node.go Outdated Show resolved Hide resolved
pkg/mutation/schema/node.go Outdated Show resolved Hide resolved
pkg/mutation/schema/node.go Outdated Show resolved Hide resolved
pkg/mutation/schema/schema.go Show resolved Hide resolved
pkg/mutation/schema/schema.go Show resolved Hide resolved
pkg/mutation/system.go Outdated Show resolved Hide resolved
pkg/mutation/types/mutator.go Show resolved Hide resolved
pkg/mutation/system_test.go Outdated Show resolved Hide resolved
@willbeason willbeason force-pushed the conflicting-schema branch 6 times, most recently from 937a78e to 7a5c82f Compare June 16, 2021 19:28
Copy link
Contributor

@maxsmythe maxsmythe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got to schema/node.go before I ran out of time, submitting the feedback I have to-date.

pkg/mutation/path/parser/node.go Outdated Show resolved Hide resolved
pkg/mutation/schema/node.go Show resolved Hide resolved
pkg/mutation/schema/node.go Show resolved Hide resolved

childKey := path[0].Key()
if _, found := n.children[childKey]; !found {
// The child does not exist.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case an unexpected missing field indicates that the database has been corrupted, leaving the process in an unknown state. The only way to recover from that is to panic.

We rely on mutex locks to prevent duplicated calls, and the fact that we bypass schema deletion if the mutator is not registered.

pkg/mutation/schema/node.go Show resolved Hide resolved
@willbeason willbeason force-pushed the conflicting-schema branch 2 times, most recently from 0b0a14c to 26265d1 Compare June 16, 2021 20:52
pkg/mutation/schema/node.go Show resolved Hide resolved
pkg/mutation/schema/node.go Show resolved Hide resolved
pkg/mutation/schema/node.go Show resolved Hide resolved
pkg/mutation/system.go Outdated Show resolved Hide resolved
pkg/mutation/system.go Outdated Show resolved Hide resolved
@willbeason
Copy link
Member Author

Setting up a meeting to figure out the desired behavior of pkg/mutation/system.go.

@willbeason willbeason force-pushed the conflicting-schema branch 3 times, most recently from 860fa84 to 7d9886f Compare June 17, 2021 16:58
pkg/mutation/schema/schema.go Outdated Show resolved Hide resolved
pkg/mutation/schema/schema.go Show resolved Hide resolved
pkg/mutation/schema/schema.go Show resolved Hide resolved
pkg/mutation/system_test.go Outdated Show resolved Hide resolved
pkg/mutation/system_test.go Show resolved Hide resolved
pkg/mutation/system_test.go Show resolved Hide resolved
@@ -0,0 +1,7 @@
package util

type Error string
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we adding this? Is errors.New() insufficient?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oren recommended this pattern so that errors could be compile-time constants. I have absolutely no opinion on whether we do this and will concede to reviewers. If you want me to remove it, I will.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's figure out the "why" to get you out of the middle of conflicting feedback.

@shomron Is there benefit in creating our own error class instead of using errors.New()? I get the appeal of a constant.

Copy link
Contributor

@maxsmythe maxsmythe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with one test nit and pending the outcome of the discussion with @shomron

@ritazh @shomron @sozercan LGTY?

pkg/mutation/system_test.go Show resolved Hide resolved
@maxsmythe
Copy link
Contributor

@shomron @sozercan poke!

@willbeason
Copy link
Member Author

@shomron @sozercan Poke again!

@willbeason willbeason force-pushed the conflicting-schema branch 2 times, most recently from 48eea59 to e90c987 Compare July 13, 2021 14:49
Copy link
Member

@sozercan sozercan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry about the delay. LGTM

@sozercan
Copy link
Member

minor gofmt issue in linter

When multiple Pods are running mutators, it is possible that they will
ingest mutators in different orders. Before this PR, we would
essentially randomly pick which mutator to honor and discard future
mutators which conflicted.

Fixes: open-policy-agent#1216

Signed-off-by: Will Beason <willbeason@google.com>
@willbeason willbeason merged commit a6e24e7 into open-policy-agent:master Jul 13, 2021
@willbeason willbeason deleted the conflicting-schema branch July 13, 2021 16:49
julianKatz pushed a commit to julianKatz/gatekeeper that referenced this pull request Jul 27, 2021
…1364)

When multiple Pods are running mutators, it is possible that they will
ingest mutators in different orders. Before this PR, we would
essentially randomly pick which mutator to honor and discard future
mutators which conflicted.

Fixes: open-policy-agent#1216

Signed-off-by: Will Beason <willbeason@google.com>
Signed-off-by: juliankatz <juliankatz@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Systems with conflicting mutators may not converge
3 participants