Join GitHub today
GitHub is home to over 36 million developers working together to host and review code, manage projects, and build software together.
Sign upEnsure 4xx+ response codes from webhook rejections #77022
Conversation
k8s-ci-robot
added
kind/bug
release-note
size/M
needs-sig
needs-priority
cncf-cla: yes
labels
Apr 24, 2019
liggitt
referenced this pull request
Apr 24, 2019
Closed
Fix a condition a Failure Status is returned but treated as success. #76984
This comment has been minimized.
This comment has been minimized.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: liggitt 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
label
Apr 24, 2019
k8s-ci-robot
requested review from
jennybuckley and
ncdc
Apr 24, 2019
k8s-ci-robot
added
area/apiserver
sig/api-machinery
and removed
needs-sig
labels
Apr 24, 2019
This comment has been minimized.
This comment has been minimized.
|
/cc @brendandburns |
k8s-ci-robot
requested a review
from
brendandburns
Apr 24, 2019
liggitt
added
the
priority/important-soon
label
Apr 24, 2019
k8s-ci-robot
removed
the
needs-priority
label
Apr 24, 2019
liggitt
added
the
area/admission-control
label
Apr 24, 2019
This comment has been minimized.
This comment has been minimized.
|
/cc @caesarxuchao |
k8s-ci-robot
requested a review
from
caesarxuchao
Apr 25, 2019
liggitt
force-pushed the
liggitt:webhook-error-success
branch
from
9978a34
to
5007643
Apr 25, 2019
yue9944882
approved these changes
Apr 25, 2019
liggitt
reviewed
Apr 26, 2019
| @@ -32,6 +33,15 @@ func ToStatusErr(webhookName string, result *metav1.Status) *apierrors.StatusErr | |||
| result = &metav1.Status{Status: metav1.StatusFailure} | |||
| } | |||
|
|
|||
| // Make sure we don't return < 400 status codes along with a rejection | |||
| if result.Code < http.StatusBadRequest { | |||
| result.Code = http.StatusBadRequest | |||
This comment has been minimized.
This comment has been minimized.
liggitt
Apr 26, 2019
Author
Member
We talked about switching this to a 500 in the api-machinery call today, but the more I consider this, I think that would be misleading to the end user. The reason their request was rejected was because their request was judged to be bad, not because the server had some error.
I see this as akin to what we do here:
if we encounter a negotiation error in the process of trying to write an error response, we don't switch to a 500, we preserve the original error code
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
/lgtm |
liggitt commentedApr 24, 2019
What type of PR is this?
/kind bug
What this PR does / why we need it:
Admission webhooks can unintentionally reject requests while still returning a 200 status to the user.
This causes things like
kubectl createto return a 0 status when their request was actually rejected.Webhook dispatch should force rejections to have >= 400 http status codes.
Special notes for your reviewer:
xref #76984
Does this PR introduce a user-facing change?: