fix: Create dynamic CEL object variable #432
Conversation
Signed-off-by: Max Smythe <smythe@google.com>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #432 +/- ##
==========================================
- Coverage 54.68% 53.19% -1.50%
==========================================
Files 71 104 +33
Lines 5241 6550 +1309
==========================================
+ Hits 2866 3484 +618
- Misses 2073 2704 +631
- Partials 302 362 +60
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| func BindObjectV1Beta1() admissionregistrationv1beta1.Variable { | ||
| return admissionregistrationv1beta1.Variable{ | ||
| Name: schema.ObjectName, | ||
| Expression: `has(request.operation) && request.operation == "DELETE" && object == null ? oldObject : object`, |
There was a problem hiding this comment.
This logic for rego is actually in GK not framework: https://github.com/open-policy-agent/gatekeeper/blob/f81f0acacb00802108abc81ba5d862bec6c2d633/pkg/util/request_validation.go#L29, should it be moved to framework to be consistent? not a PR blocker.
There was a problem hiding this comment.
Framework should not have any coupling with any particular platform.
We should probably move the logic to the TargetHandler.
In addition, we should update the engine logic to undo the setting of obj == oldObject on DELETE, otherwise CEL code will behave differently in on-k8s VAP vs in-g8r VAP.
There was a problem hiding this comment.
We should probably move the logic to the TargetHandler.
Move the obj == oldObject on DELETE logic to the TargetHandler instead of the webhook validation handler right? Could we move that logic to the rego driver similar to how it's part of the cel driver here?
There was a problem hiding this comment.
WRT moving it to target handler -- right.
The problem with moving it to the Rego driver is that the Rego driver is more generic than the VAP driver.
The VAP driver is highly coupled with K8s/KRM -- it is strongly typed, assuming an inbound admission review object. Because of this, and because the VAP driver is explicitly intended to support VAP (as opposed to a more generic application of CEL) I'm okay with adding "undoing" logic to the driver (we're not increasing coupling).
For something like the Rego driver, the only real assumption is that the inbound object is JSON-structured.
| @@ -22,6 +22,8 @@ const ( | |||
| ReservedPrefix = "gatekeeper_internal_" | |||
| // ParamsName is the VAP variable constraint parameters will be bound to. | |||
| ParamsName = "params" | |||
| // ObjectName is the VAP variable that describes either an object or (on DELETE requests) oldObject | |||
| ObjectName = "anyObject" | |||
There was a problem hiding this comment.
With this change, do we plan to update all cel expressions for gatekeeper-libraries policies to use this before merging those PRs? if so, then we need to cut a patch release for GK and update GK-lib CI and docs to ensure the latest framework/GK is used.
There was a problem hiding this comment.
yea, we'll need to merge this, vendor it in GK, cherry pick in release-3.16, cut a GK release (3.16.1), and update library
|
LGTY @ritazh ? |
There was a problem hiding this comment.
LGTM
Looks like there's a race condition failure in the readiness tracker test https://github.com/open-policy-agent/frameworks/actions/runs/9134818421/job/25121118481?pr=432
|
WRT the race condition, I'm hoping @David-Jaeyoon-Lee's PR will address this once we solve the shared-fate issue across goroutines: |
Defines a variable,
anyObjectthat behaves in the same wayrequest.Objectdoes in Rego, allowing for a 1:1 behavioral analogue for Rego'srequest.Objectcommand when running a template in VAP.This also fixes a bug in generated CEL code where user-defined variables are placed before
variables.paramsis defined, making users unable to referencevariables.paramsin the variables they create.