Skip to content
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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

馃悰 Fix webhook write response error for broken HTTP connection #1931

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
12 changes: 10 additions & 2 deletions pkg/webhook/admission/http.go
Expand Up @@ -124,8 +124,16 @@ func (wh *Webhook) writeResponseTyped(w io.Writer, response Response, admRevGVK
// writeAdmissionResponse writes ar to w.
func (wh *Webhook) writeAdmissionResponse(w io.Writer, ar v1.AdmissionReview) {
if err := json.NewEncoder(w).Encode(ar); err != nil {
wh.log.Error(err, "unable to encode the response")
wh.writeResponse(w, Errored(http.StatusInternalServerError, err))
wh.log.Error(err, "unable to encode and write the response")
// Since the `ar v1.AdmissionReview` is a clear and legal object,
// 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.
serverError := Errored(http.StatusInternalServerError, err)
if err = json.NewEncoder(w).Encode(v1.AdmissionReview{Response: &serverError.AdmissionResponse}); err != nil {
wh.log.Error(err, "still unable to encode and write the InternalServerError response")
}
} else {
res := ar.Response
if log := wh.log; log.V(1).Enabled() {
Expand Down
27 changes: 27 additions & 0 deletions pkg/webhook/admission/http_test.go
Expand Up @@ -23,6 +23,7 @@ import (
"io"
"net/http"
"net/http/httptest"
"time"

. "github.com/onsi/ginkgo"
. "github.com/onsi/gomega"
Expand Down Expand Up @@ -190,6 +191,24 @@ var _ = Describe("Admission Webhooks", func() {
webhook.ServeHTTP(respRecorder, req.WithContext(ctx))
Expect(respRecorder.Body.String()).To(Equal(expected))
})

It("should never run into circular calling if the writer has broken", func() {
req := &http.Request{
Header: http.Header{"Content-Type": []string{"application/json"}},
Body: nopCloser{Reader: bytes.NewBufferString(fmt.Sprintf(`{%s,"request":{}}`, gvkJSONv1))},
}
webhook := &Webhook{
Handler: &fakeHandler{},
log: logf.RuntimeLog.WithName("webhook"),
}

bw := &brokenWriter{ResponseWriter: respRecorder}
Eventually(func() int {
// This should not be blocked by the circular calling of writeResponse and writeAdmissionResponse
webhook.ServeHTTP(bw, req)
return respRecorder.Body.Len()
}, time.Second*3).Should(Equal(0))
})
})
})

Expand Down Expand Up @@ -225,3 +244,11 @@ func (h *fakeHandler) Handle(ctx context.Context, req Request) Response {
Allowed: true,
}}
}

type brokenWriter struct {
http.ResponseWriter
}

func (bw *brokenWriter) Write(buf []byte) (int, error) {
return 0, fmt.Errorf("mock: write: broken pipe")
}