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 mutators #1569

Merged
merged 27 commits into from
Sep 28, 2021

Conversation

willbeason
Copy link
Member

@willbeason willbeason commented Sep 21, 2021

Preserve conflicting mutator paths, making behavior independent of the order in which conflicting mutators are added.

Increase test coverage from 70% to 89%. Added tests are unit tests that fake out the dependencies of Reconciler, so they don't require the same setup as the existing integration tests in the same package.

Minor changes included in this PR:

  • Make pod ownership mode a member of Reconciler rather than a global variable. As on master makes testing both paths consistently impossible in unit tests, but this way makes the tests independent of which order they were run (or whether other unit tests were run). It is still a global variable, but it is only read when creating a Reconciler via newReconciler().
  • Add Type to MutatorError for machine consumption. This allows for selectively replacing errors or marking "enforced" in the case that a conflict between mutators is resolved, but one of the mutators has a separate error as well.
  • Cleanup POD_NAMESPACE environment variable after unit tests which set it to make unit tests independent.
  • OPA clients named opa renamed to opaClient to prevent conflicts between the package name and variables with the name.
  • Split Reconciler and Adder code into their own files (respectively named adder.go and reconciler.go). This reduces the file size and makes finding relevant code easier.
  • Delete SetupTestReconciler since it isn't used by anything.
  • Implement statusUpdate which is a functor that updates MutatorPodStatus
  • Make MutatorIngestionStatus enum values into constants to prevent them from being mistakenly reassigned.
  • Make ErrConflictingSchema a structured error type which records the IDs of the Mutators which conflict. This allows just accessing the set of conflicting mutators via erorrs.As elsewhere.
  • Add GetConflicts() to schema.node to dynamically fetch the conflicts a given mutator has with all other mutators.
  • Update system_test tests which validate that mutator conflicts are now handled as we expect.

Fixes: #1216

@willbeason willbeason self-assigned this Sep 21, 2021
@willbeason willbeason force-pushed the preserve-conflicts-2c branch 2 times, most recently from bdfeed8 to 3185f04 Compare September 21, 2021 16:06
@codecov-commenter
Copy link

codecov-commenter commented Sep 21, 2021

Codecov Report

Merging #1569 (82f40fa) into master (921869d) will increase coverage by 0.68%.
The diff coverage is 76.54%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1569      +/-   ##
==========================================
+ Coverage   52.83%   53.52%   +0.68%     
==========================================
  Files          89       93       +4     
  Lines        7849     7995     +146     
==========================================
+ Hits         4147     4279     +132     
- Misses       3356     3372      +16     
+ Partials      346      344       -2     
Flag Coverage Δ
unittests 53.52% <76.54%> (+0.68%) ⬆️

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

Impacted Files Coverage Δ
pkg/controller/constraint/constraint_controller.go 5.68% <0.00%> (-0.04%) ⬇️
pkg/controller/mutators/stats_reporter.go 70.21% <ø> (ø)
pkg/readiness/ready_tracker.go 69.82% <0.00%> (-0.40%) ⬇️
pkg/mutation/schema/errors.go 33.33% <33.33%> (ø)
pkg/mutation/schema/schema.go 80.76% <33.33%> (-10.73%) ⬇️
pkg/mutation/system.go 83.44% <50.00%> (-7.40%) ⬇️
pkg/controller/mutators/core/adder.go 53.12% <53.12%> (ø)
pkg/mutation/schema/id_set.go 63.15% <63.15%> (ø)
pkg/mutation/schema/node.go 89.80% <92.59%> (+0.33%) ⬆️
pkg/controller/mutators/core/reconciler.go 85.71% <93.44%> (ø)
... and 15 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 921869d...82f40fa. Read the comment docs.

@willbeason willbeason force-pushed the preserve-conflicts-2c branch 2 times, most recently from 582e199 to 42303ae Compare September 21, 2021 16:50
@willbeason willbeason force-pushed the preserve-conflicts-2c branch from 2a33674 to 7739603 Compare September 22, 2021 22:23
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, couple nits

Will Beason added 8 commits September 28, 2021 08:18
Signed-off-by: Will Beason <willbeason@google.com>
Signed-off-by: Will Beason <willbeason@google.com>
Signed-off-by: Will Beason <willbeason@google.com>
We don't need two copies of the same information, and it makes testing
easier this way.

Signed-off-by: Will Beason <willbeason@google.com>
Since it isn't holding mutators any more.

Signed-off-by: Will Beason <willbeason@google.com>
Signed-off-by: Will Beason <willbeason@google.com>
Signed-off-by: Will Beason <willbeason@google.com>
Signed-off-by: Will Beason <willbeason@google.com>
Will Beason added 19 commits September 28, 2021 08:18
Signed-off-by: Will Beason <willbeason@google.com>
As a global variable, with multiple tests manipulating the value, it
makes unit tests dependenton each other.

Signed-off-by: Will Beason <willbeason@google.com>
We properly preserve errors now

Signed-off-by: Will Beason <willbeason@google.com>
The problem was my test code. Now the reconciler edge cases work.

Signed-off-by: Will Beason <willbeason@google.com>
The logic already works, so no change to non-test code required

Signed-off-by: Will Beason <willbeason@google.com>
This isn't expected to change dynamically through the runtime of the
program, so just set it at Reconciler creation

Signed-off-by: Will Beason <willbeason@google.com>
Signed-off-by: Will Beason <willbeason@google.com>
Signed-off-by: Will Beason <willbeason@google.com>
Signed-off-by: Will Beason <willbeason@google.com>
This ensures we properly update PodStatus in the case we successfully
modify the Schema to include the conflict, but then fail to update the
PodStatus. In the second Reconcile of the conflicting Mutator, we must
return the schema conflict error rather than nil to indicate that the
PodStatus still needs to be updated.

Signed-off-by: Will Beason <willbeason@google.com>
Signed-off-by: Will Beason <willbeason@google.com>
Deduplicate logic for reporting conflicts

Signed-off-by: Will Beason <willbeason@google.com>
Signed-off-by: Will Beason <willbeason@google.com>
Fix controller tests which were logging noisy errors by specifying
metadata.namespace for the Pod.

Signed-off-by: Will Beason <willbeason@google.com>
Signed-off-by: Will Beason <willbeason@google.com>
Signed-off-by: Will Beason <willbeason@google.com>
Cover a few more edge cases in reconciler unit tests

Simplify redundant reconciler logic.

Signed-off-by: Will Beason <willbeason@google.com>
Signed-off-by: Will Beason <willbeason@google.com>
Signed-off-by: Will Beason <willbeason@google.com>
@willbeason willbeason force-pushed the preserve-conflicts-2c branch from 82f40fa to 00c1b55 Compare September 28, 2021 15:20
@willbeason willbeason merged commit 7ea3783 into open-policy-agent:master Sep 28, 2021
@willbeason willbeason deleted the preserve-conflicts-2c branch September 28, 2021 15:48
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
5 participants