馃悰 Fix webhook write response error for broken HTTP connection #1930
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
the
size/S
| // it should not have problem to be marshalled into bytes. | ||
| // The error here is probably caused by the abnormal HTTP connection, | ||
| // e.g., broken pipe, so we can only write the error response once, | ||
| // to avoid endless circular calling. |
There was a problem hiding this comment.
endless circular calling, why would that happen?
There was a problem hiding this comment.
If the connection has broken, it will run into a dead loop:
json.NewEncoder(w).Encode(ar)failed because ofbroken pipe- call
wh.writeResponse(w, Errored(http.StatusInternalServerError, err)) - call
wh.writeAdmissionResponse(w, v1.AdmissionReview{Response: &response.AdmissionResponse}) - again
json.NewEncoder(w).Encode(ar)failed because ofbroken pipe - ...
There was a problem hiding this comment.
@alvaroaleman WDYT :)
User issues can be found on the top of this PR.
There was a problem hiding this comment.
Ah, thanks for the explanation. Looks good to me, but can we add a test with a broken io.Writer to test this scenario we are fixing?
There was a problem hiding this comment.
Emm.. Sure, let me see how to reproduce or mock the broken error in unit-test.
Signed-off-by: FillZpp <FillZpp.pub@gmail.com>
FillZpp
force-pushed
the
fix-webhook-write-response
branch
from
57eecbc
to
efcc115
Compare
k8s-ci-robot
added
size/M
|
/retest |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alvaroaleman, FillZpp 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
|
/cherrypick release-0.12 |
|
@alvaroaleman: new pull request created: #1931 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. |
|
@FillZpp awesome information. Do you happen to know why api server drop http connections? I start to observe this issue in clusters with Kubernetes 1.20 deployed and very curious to know what caused that. |
I believe that if the apiserver container/pod has been restarted or recreated during the request to webhook, then webhook will get the broken pipe error when it tries to write response back to the connection. But I'm not sure in which cases apiserver will close or lose the connection if it remains running. |
Signed-off-by: FillZpp FillZpp.pub@gmail.com
Fix webhook write response error for broken HTTP connection
fixes #1829
fixes issue from slack
fixes issue from slack