-
Notifications
You must be signed in to change notification settings - Fork 744
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
Add an owner reference and a watch to Constraint Template CRDs, remove finalizers #459
Conversation
@shomron LMK if you see something missing here |
pkg/controller/constrainttemplate/constrainttemplate_controller.go
Outdated
Show resolved
Hide resolved
@maxsmythe one issue noted above, and could use some new test cases, but otherwise 👍🏻 |
6df1f3a
to
99103cc
Compare
pkg/controller/constrainttemplate/constrainttemplate_controller.go
Outdated
Show resolved
Hide resolved
pkg/controller/constrainttemplate/constrainttemplate_controller.go
Outdated
Show resolved
Hide resolved
pkg/controller/constrainttemplate/constrainttemplate_controller.go
Outdated
Show resolved
Hide resolved
pkg/controller/constrainttemplate/constrainttemplate_controller.go
Outdated
Show resolved
Hide resolved
pkg/controller/constrainttemplate/constrainttemplate_controller_test.go
Outdated
Show resolved
Hide resolved
pkg/controller/constrainttemplate/constrainttemplate_controller_test.go
Outdated
Show resolved
Hide resolved
Ping. @maxsmythe what should be the sequencing between this PR and #490? |
@maxsmythe No pressure, I just wanted to understand what order we'd like to stagger them in. I pushed a fix for #506 which #490 will now depend on. Aside from that, I have a few more tests I can add until next week, and I'm seeking your input on config set changes and transactionality. But I think we're in good shape and close to rounding this out. |
@shomron Cool, we can hash out ordering tomorrow. I don't think I'm going to get to this tonight, unfortunately. |
Rebased on top of the watch manager changes and threw in the removal of finalizers on constraints and templates. Note that I had to update the version of Kubebuilder our test harness installs to avoid tests hanging while waiting for watch reschedules. Also note that this change shouldn't yet be committed as there is a blocking change in the constraint framework that allows lookup/removal of templates just using the template name: |
Fixes open-policy-agent#448 Signed-off-by: Max Smythe <smythe@google.com>
Signed-off-by: Max Smythe <smythe@google.com>
Signed-off-by: Max Smythe <smythe@google.com>
Signed-off-by: Max Smythe <smythe@google.com>
Signed-off-by: Max Smythe <smythe@google.com>
Signed-off-by: Max Smythe <smythe@google.com>
This should now be ready for review. I also got rid of the vendoring hack now that we are able. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@maxsmythe this looks good! I have a few questions and a few non-blocking suggestions. Let me know what you think.
defer r.metrics.registry.report(r.metrics) | ||
if containsString(finalizerName, ct.GetFinalizers()) { | ||
// preserve original status as otherwise it will get wiped in the update | ||
origStatus := ct.Status.DeepCopy() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question regarding this: is the status simply not returned in the response when updating the primary resource (but is still intact in etcd), or has it actually been wiped by the apiserver? Also I'm assuming we have status subresource enabled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, status resource is enabled.
IIRC the status resource is not returned in the response. I did an experiment ~1 month ago to verify this.
pkg/controller/constrainttemplate/constrainttemplate_controller.go
Outdated
Show resolved
Hide resolved
pkg/controller/constrainttemplate/constrainttemplate_controller.go
Outdated
Show resolved
Hide resolved
pkg/controller/constrainttemplate/constrainttemplate_controller.go
Outdated
Show resolved
Hide resolved
pkg/controller/constrainttemplate/constrainttemplate_controller_test.go
Outdated
Show resolved
Hide resolved
Signed-off-by: Max Smythe <smythe@google.com>
Comments should be addressed by latest push |
log.Info("missing constraint template in OPA cache, no deletion necessary") | ||
ct.SetName(request.Namespace) | ||
ct.SetName(request.Name) | ||
logAction(ct, deletedAction) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: you can pass ctRef
for logging now and remove the above lines.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Signed-off-by: Max Smythe <smythe@google.com>
Sweet! Addressed nit and lint failure. If tests pass, I'll assume this is sufficient. |
@sozercan LMK if you wanted a chance for a review pass |
manifest_staging/chart/gatekeeper-operator/templates/gatekeeper.yaml
Outdated
Show resolved
Hide resolved
Signed-off-by: Max Smythe <smythe@google.com>
@sozercan addressed comments |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Fixes #448
Signed-off-by: Max Smythe smythe@google.com