refactor: Move setting up Obj to old obj on Delete logic to target handler #3511
Conversation
abhipatnala
force-pushed
the
objToOldObj_in_target_handler
branch
from
b26531c
to
41e70be
Compare
pkg/webhook/policy.go
Outdated
| } | ||
|
|
||
| trace, dump := h.tracingLevel(ctx, req) | ||
| resp, err := h.review(ctx, review, trace, dump) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("error reviewing resource %s: %w", req.Name, err) | ||
| } | ||
|
|
||
| // resultants slice will be empty if expand is skipped. |
There was a problem hiding this comment.
no need to comment about this, it can be inferred from the code without extra context.
There was a problem hiding this comment.
Deleted that comment.
| return nil, fmt.Errorf("unable to expand object: %w", err) | ||
| resultants := []*expansion.Resultant{} | ||
| // Skip the expansion if admissionRequest.Obj is nil. | ||
| if req.AdmissionRequest.Object.Raw != nil { |
There was a problem hiding this comment.
Probably out of scope for this PR, but once deletion expansion is fixed. This should probably follow the behavior of use oldObject.Raw for deletion requests (when object is nil) and object.Raw otherwise. Correct me if Im wrong @maxsmythe.
There was a problem hiding this comment.
discussed offline with max. You can ignore this for this PR.
| @@ -18,6 +18,9 @@ import ( | |||
| "k8s.io/apimachinery/pkg/util/validation/field" | |||
| ) | |||
|
|
|||
| // nolint: revive // Moved error out of pkg/webhook/admission; needs capitalization for backwards compat. | |||
| var ErrOldObjectIsNil = errors.New("oldObject cannot be nil for DELETE operations") | |||
There was a problem hiding this comment.
No good place to suggest this change -
Update runner_test.go#1160 to {Name: "no oldObject on delete", Error: &clienterrors.ErrorMap{target.Name: constraintclient.ErrReview}},
And import this in the same test file -
"github.com/open-policy-agent/gatekeeper/v3/pkg/target"
clienterrors "github.com/open-policy-agent/frameworks/constraint/pkg/client/errors"
This should fix the CI error.
There was a problem hiding this comment.
Thank You. I have included those changes.
abhipatnala
requested review from
JaydipGabani,
maxsmythe and
David-Jaeyoon-Lee
2.Add unit tests 3.Update runner and webhook Signed-off-by: Avinash Patnala <[email protected]>
Signed-off-by: Avinash Patnala <[email protected]>
Signed-off-by: Avinash Patnala <[email protected]>
Signed-off-by: Avinash Patnala <[email protected]>
abhipatnala
force-pushed
the
objToOldObj_in_target_handler
branch
from
903cea5
to
71af237
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3511 +/- ##
==========================================
- Coverage 54.49% 48.05% -6.44%
==========================================
Files 134 218 +84
Lines 12329 15167 +2838
==========================================
+ Hits 6719 7289 +570
- Misses 5116 7064 +1948
- Partials 494 814 +320
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
What this PR does / why we need it:
Which issue(s) this PR fixes (optional, using
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when the PR gets merged):Fixes #3441
Special notes for your reviewer: