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 #1930

Merged
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.
Copy link
Member

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?

Copy link
Contributor Author

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:

  1. json.NewEncoder(w).Encode(ar) failed because of broken pipe
  2. call wh.writeResponse(w, Errored(http.StatusInternalServerError, err))
  3. call wh.writeAdmissionResponse(w, v1.AdmissionReview{Response: &response.AdmissionResponse})
  4. again json.NewEncoder(w).Encode(ar) failed because of broken pipe
  5. ...

Copy link
Contributor Author

@FillZpp FillZpp Jun 14, 2022

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.

Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test added.

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")
}