fix: preserve admission operation for expanded reviews#4560
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes a regression where expanded/generated resource reviews were losing input.review.operation, ensuring constraints can reliably reason about CREATE/UPDATE/DELETE during expansion flows.
Changes:
- Thread the admission
Operationthrough expanded (resultant) review creation in the webhook handler. - Extend
target.AugmentedUnstructuredand its conversion to an admission-like review to preserveOperation, including DELETE semantics. - Add regression tests covering CREATE/UPDATE/DELETE to ensure expanded reviews keep the original operation.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| pkg/webhook/policy.go | Passes the admission operation into generated/resultant reviews so expanded evaluations preserve input.review.operation. |
| pkg/webhook/policy_test.go | Adds regression coverage validating expanded resources preserve CREATE/UPDATE/DELETE operation in review input. |
| pkg/target/target.go | Propagates Operation from AugmentedUnstructured into the internal review object; maps DELETE to OldObject as needed. |
| pkg/target/data.go | Adds Operation to AugmentedUnstructured so non-admission review wrappers can carry operation when applicable. |
| @@ -24,5 +25,6 @@ func IsWipeData(o interface{}) bool { | |||
| type AugmentedUnstructured struct { | |||
| Object unstructured.Unstructured | |||
| Namespace *corev1.Namespace | |||
| Operation admissionv1.Operation | |||
There was a problem hiding this comment.
Fixed in the latest push: the AugmentedUnstructured doc comment now mentions the optional admission operation and when it is populated.
Zakharden
force-pushed
the
fix/expanded-review-operation
branch
from
4d0a19b to
1f36ee9
Compare
Zakharden
changed the title
Zakharden
force-pushed
the
fix/expanded-review-operation
branch
from
1f36ee9 to
8886b16
Compare
| review.namespace = obj.Namespace | ||
| review.Operation = obj.Operation | ||
| if obj.Operation == admissionv1.Delete { | ||
| review.OldObject = review.Object |
There was a problem hiding this comment.
Fixed in the latest push: DELETE reviews now move the generated object into OldObject and clear Object with the zero runtime.RawExtension{} value. I also added TestAugmentedUnstructuredDeleteUsesOldObject to cover this path before the DELETE review normalization step.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4560 +/- ##
==========================================
- Coverage 54.49% 44.52% -9.97%
==========================================
Files 134 282 +148
Lines 12329 20721 +8392
==========================================
+ Hits 6719 9227 +2508
- Misses 5116 10699 +5583
- Partials 494 795 +301
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: Zakhar Dvurechensky <72825626+Zakharden@users.noreply.github.com>
Zakharden
force-pushed
the
fix/expanded-review-operation
branch
from
8886b16 to
45c4d4d
Compare
Fixes generated/expanded resource reviews losing input.review.operation, with CREATE/UPDATE/DELETE regression coverage. Validation: targeted go test ./pkg/webhook, go test ./pkg/target, git diff --check.