Explicitly flush headers when proxying #75887
Conversation
k8s-ci-robot
added
release-note
|
[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
liggitt
added
the
priority/critical-urgent
k8s-ci-robot
removed
the
needs-priority
There was a problem hiding this comment.
newFlushHeaderWriter should either return an error for writers not implementing http.Flusher, or be renamed something like tryWrapInFlushHeaderWriter (and potentially returning a (w http.ResonseWriter, ok bool)).
Also, even if the component only requires the flusher interface, it also checks for CloseNotifier and Hijacker interfaces. Is it relevant ? (If it is, a comment about that would be welcome).
|
Anyway, thanks a lot for the responsiveness on this issue, it is much appreciated ! |
|
updated with clearer comments |
liggitt
force-pushed
the
flush-headers
branch
2 times, most recently
from
650d523 to
2d6c548
Compare
|
/retest |
|
/assign @cheftako |
| // flusher, hijacker, and closeNotifier are all used by the ReverseProxy implementation. | ||
| // if the given writer can't support all three, return the original writer. | ||
| if !isFlusher || !isHijacker || !isCloseNotifier { | ||
| return w |
There was a problem hiding this comment.
Seems like it would be good to track that this happened. Maybe log that we may not flush the headers correctly along with the type of the writer not supporting this?
There was a problem hiding this comment.
NM, https://golang.org/src/net/http/httputil/reverseproxy.go?h=handleUpgradeResponse#L518 looks like it will give us the error message I was looking for.
| type flushHeadersWriter struct { | ||
| http.ResponseWriter | ||
| http.Flusher | ||
| http.Hijacker |
There was a problem hiding this comment.
Do we know why we need Hijacker (Other than it was in the sample code from golang/go#31125)? I don't see us actually calling Hijack().
There was a problem hiding this comment.
Httputil.ReverseProxy requires it. (Note that all these interfaces are embedded in the structure, to make it implement all of them)
There was a problem hiding this comment.
Thanks, I think that cleared up both my concerns.
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
picked to 1.14.1 in #76046 |
|
This is supposed to be temporary. Do we have a bug or anything similar to tell us to remove it once it is no longer necessary? |
|
opened #76154 to track |
…7-upstream-release-1.14 Automated cherry pick of #75887: Explicitly flush headers when proxying
What type of PR is this?
/kind bug
What this PR does / why we need it:
Works around a regression in the go reverse proxy, in which headers from a backend aren't flushed correctly until the backend also sends body content.
Which issue(s) this PR fixes:
Fixes #75837
Special notes for your reviewer:
This is a temporary workaround we can drop once we pick up a version of go that resolves the regression, but this should be picked to make 1.14.1 in the meantime
Does this PR introduce a user-facing change?:
/sig api-machinery