⚠️ admission responses with raw Status #1129
Conversation
If a custom webhook validator returns a metav1.Status object as the error, the validation handler will build an admission response with this Status object as is, instead of building a new Status object based on err.Error() This might be considered as a breaking change since the behaviour of the validation handler is going to be slightly different from what the users might expect based on the previous version of the code
|
Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA. It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
k8s-ci-robot
added
cncf-cla: no
|
Welcome @Shpectator! |
|
Hi @Shpectator. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
k8s-ci-robot
added
the
size/M
pkg/webhook/admission/response.go
Outdated
| @@ -80,6 +80,17 @@ func ValidationResponse(allowed bool, reason string) Response { | |||
| return resp | |||
| } | |||
|
|
|||
| // ValidationResponseFromStatus returns a response for admitting a request with provided Status object. | |||
| func ValidationResponseFromStatus(allowed bool, status *metav1.Status) Response { | |||
There was a problem hiding this comment.
I don't know that this needs to be a public helper method at the moment, especially since it's basically just constructing an object from the listed fields.
pkg/webhook/admission/validator.go
Outdated
| return Denied(err.Error()) | ||
| } | ||
| } | ||
|
|
||
| return Allowed("") | ||
| } | ||
|
|
||
| func isStatusError(err *error) (bool, *metav1.Status) { |
There was a problem hiding this comment.
this method is a bit weirdly named -- I'd just expect it to return a bool.
The entire thing should be replacable with errors.As (see above)
pkg/webhook/admission/validator.go
Outdated
| isStatusError, status := isStatusError(&err) | ||
| if isStatusError { | ||
| return ValidationResponseFromStatus(false, status) |
There was a problem hiding this comment.
| isStatusError, status := isStatusError(&err) | |
| if isStatusError { | |
| return ValidationResponseFromStatus(false, status) | |
| var statusError errors.StatusError | |
| if goerrors.As(&statusError) { | |
| return ... | |
| } |
There was a problem hiding this comment.
(the other nice thing about this is that if the status error is wrapped in such a way that it's intended to be unwrapped, we'll still pull the status)
There was a problem hiding this comment.
is there any corresponding interface for status-returning errors in the k8s api errors package that we should be asserting on? (basically, is there a pattern for "my custom error returns status information)
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
|
/retest |
|
I signed it |
|
ping cla bot |
|
I signed it |
k8s-ci-robot
added
cncf-cla: yes
If a custom webhook validator returns a metav1.Status object as the error, the validation handler will build an admission response with this Status object as is, instead of building a new Status object based on err.Error() This might be considered as a breaking change since the behaviour of the validation handler is going to be slightly different from what the users might expect based on the previous version of the code
If a custom webhook validator returns a metav1.Status object as the error, the validation handler will build an admission response with this Status object as is, instead of building a new Status object based on err.Error() This might be considered as a breaking change since the behaviour of the validation handler is going to be slightly different from what the users might expect based on the previous version of the code
k8s-ci-robot
added
size/S
|
/assign @DirectXMan12 |
|
@DirectXMan12 I've addressed all your comments |
|
/retest |
|
looks good, just need tests :-) |
k8s-ci-robot
added
size/L
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: DirectXMan12, Shpectator The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
|
awesome, looks great now |
|
o_O I'll file a bug about this flake. /retest for the moment |
|
@DirectXMan12: The
Use In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/retest |
If a custom webhook validator returns a metav1.Status object as the error, the validation handler will build an admission response with this Status object as is, instead of building a new Status object based on err.Error()
This might be considered as a breaking change since the behaviour of the
validation handler is going to be slightly different from what the users
might expect based on the previous version of the code