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

Convert to using OPA Constraint Framework #80

Merged
merged 12 commits into from Mar 29, 2019

Conversation

Projects
None yet
3 participants
@maxsmythe
Copy link
Contributor

commented Mar 26, 2019

This implements the ConstraintTemplate/Constraint functionality for Gatekeeper.

There is a lot more to do including:

  • ValidatingRego
  • MutatingRego
  • Audit
  • More useful status fields
  • More robust logging/debug capabilities
  • Stricter/better schema validation

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

maxsmythe added some commits Mar 26, 2019

Convert to using OPA Constraint Framework
Signed-off-by: Max Smythe <smythe@google.com>
Update OPA framework version
Signed-off-by: Max Smythe <smythe@google.com>
Add hermetic-test target
Signed-off-by: Max Smythe <smythe@google.com>

@maxsmythe maxsmythe force-pushed the maxsmythe:use_constraint_framework branch from 6f9e42b to 91f08fe Mar 26, 2019

func AddPolicyWebhook(mgr manager.Manager, opa opa.Client) error {
validatingWh, err := builder.NewWebhookBuilder().
Validating().
Name("mutation.styra.com").

This comment has been minimized.

Copy link
@ritazh

ritazh Mar 26, 2019

Contributor
Suggested change
Name("mutation.styra.com").
Name(*webhookName).
Show resolved Hide resolved pkg/webhook/policy.go Outdated
Fix webhook name
Signed-off-by: Max Smythe <smythe@google.com>
Show resolved Hide resolved pkg/webhook/policy.go Outdated

ritazh and others added some commits Mar 27, 2019

Update pkg/webhook/policy.go
Co-Authored-By: maxsmythe <max.smythe@gmail.com>
Signed-off-by: Max Smythe <smythe@google.com>
Use webhookName flag to set the webhook name
Signed-off-by: Max Smythe <smythe@google.com>

@maxsmythe maxsmythe force-pushed the maxsmythe:use_constraint_framework branch from a74a942 to afa2776 Mar 27, 2019

Implement namespace selector logic. Improve match schema validation
Signed-off-by: Max Smythe <smythe@google.com>
@tsandall
Copy link
Member

left a comment

@maxsmythe this looks like a great start. I reviewed the watch manager implementation and nothing jumped out at me immediately. One thing I was unsure about was why the manager polls every 5 seconds (as opposed to being signaled asynchronously when there's an update to process.) It would be good to include a comment in the manager code explaining the design.

A few other notes:

  • We're using glog and zap for logging throughout the codebase. It would be good if we could standardize on a single logging framework.

  • There's a lot of framework code wiring up different parts of the controller using global variables. At the same time, other parts of the codebase rely on dependency injection. Is there a reason why we didn't use dependency injection throughout so that the globals are unnecessary? Forgive my lack of experience with kubebuilder 😅

  • Test coverage is quite good. Thanks for putting the time in to make sure the watch manager was well covered. Those tests helped me understand how it was intended to work.

  • The CI runner is currently failing because kubebuilder dependencies are not installed. What about making the hermetic-test target the default test target (renaming the current test target to something like quick-test or the like.)

I've also reviewed the target Rego and noticed one small issue in the kind selector.

count(remaining_groups) < 2

kinds := {input.review.kind.kind, "*"}
selected_kinds := {k | k = kindSelector[r].kinds[_]}

This comment has been minimized.

Copy link
@tsandall

tsandall Mar 28, 2019

Member

@maxsmythe I think you intended r to bind to the same value from the comprehension above but not that's going to happen. Here's an alternative implementation of the kind selector matching that gives us the semantics that (I think) we want: https://play.openpolicyagent.org/p/DpckwavJZE. LMKWYT.

This comment has been minimized.

Copy link
@tsandall

tsandall Mar 28, 2019

Member

As a follow-up. It would be good to have some tests in place for the kind selector matching.

This comment has been minimized.

Copy link
@maxsmythe

maxsmythe Mar 29, 2019

Author Contributor

Thank you for the alternative kind selector implementation!

I folded a slightly modified version of your code into the latest patch.

maxsmythe added some commits Mar 29, 2019

Switch all logging to zapf
Signed-off-by: Max Smythe <smythe@google.com>
Shift test make target to run in docker container
Signed-off-by: Max Smythe <smythe@google.com>
Add tests for kind selector
Kind selector and tests adapted from Rego code by torin@styra.com

Signed-off-by: Max Smythe <smythe@google.com>
Add comment to watch manager explaining polling update model
Signed-off-by: Max Smythe <smythe@google.com>
@maxsmythe

This comment has been minimized.

Copy link
Contributor Author

commented Mar 29, 2019

@tsandall

Thanks for the review!

I added a comment to the watch manager, but the gist is that changes to the underlying CRD resource version will break the watch, so it's a good idea to make sure the roster stays up-to-date independent of actions from a controller.

Re: other comments:

  • Switched to using zap across the board

  • Mostly it seems that the global variables are used on initialization to keep track of what things need to get built, which enables dependency injection on the built object via type inspection. This is the purpose of the AddToManager(). The other global I'm aware of is logging, which is a convenience because passing logs from function to function gets old quick and muddies the API. LMK if I'm missing a use of global variables you're concerned about.

  • Thanks! I'm glad the tests were recognized.

  • Switched the test targets up

Also, signal boost that I copied your modified kind selector Rego code into the target. LMK if this is not okay to do.

Remove boilerplate comment
Signed-off-by: Max Smythe <smythe@google.com>
@tsandall

This comment has been minimized.

Copy link
Member

commented Mar 29, 2019

LMK if I'm missing a use of global variables you're concerned about.

No, it was just the initialization you described. It just took me a while to find my way around the code because of the indirection but I think once you spend some time with it, it's not so bad.

The rest of the changes look good to me. Feel free to merge whenever you want.

@maxsmythe maxsmythe merged commit 3a3a3e3 into open-policy-agent:master Mar 29, 2019

3 checks passed

DCO DCO
Details
cla/linuxfoundation maxsmythe authorized
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@maxsmythe

This comment has been minimized.

Copy link
Contributor Author

commented Mar 29, 2019

Awesome, thanks!

@maxsmythe maxsmythe deleted the maxsmythe:use_constraint_framework branch Jul 9, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.