-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
🐛 Fix webhook write response error for broken HTTP connection #1930
🐛 Fix webhook write response error for broken HTTP connection #1930
Conversation
// 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
endless circular calling, why would that happen?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alvaroaleman WDYT :)
User issues can be found on the top of this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Emm.. Sure, let me see how to reproduce or mock the broken error in unit-test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test added.
Signed-off-by: FillZpp <FillZpp.pub@gmail.com>
57eecbc
to
efcc115
Compare
/retest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
/lgtm
/approve
/cherrypick release-0.12
[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 |
/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