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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Explicitly flush headers when proxying #75887

Merged
merged 1 commit into from Apr 2, 2019

Conversation

@liggitt
Copy link
Member

commented Mar 29, 2019

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?:

Fixes a regression proxying responses from aggregated API servers which could cause watch requests to hang until the first event was received

/sig api-machinery

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Mar 29, 2019

[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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@simonferquel
Copy link
Contributor

left a 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).

@simonferquel

This comment has been minimized.

Copy link
Contributor

commented Mar 29, 2019

Anyway, thanks a lot for the responsiveness on this issue, it is much appreciated !

@liggitt

This comment has been minimized.

Copy link
Member Author

commented Mar 29, 2019

updated with clearer comments

@liggitt liggitt force-pushed the liggitt:flush-headers branch 2 times, most recently from 650d523 to 2d6c548 Mar 29, 2019

@liggitt liggitt force-pushed the liggitt:flush-headers branch from 2d6c548 to aa8e75c Mar 29, 2019

@simonferquel
Copy link
Contributor

left a comment

lgtm 👍

@liggitt

This comment has been minimized.

Copy link
Member Author

commented Mar 29, 2019

/retest

@fedebongio

This comment has been minimized.

Copy link
Contributor

commented Apr 1, 2019

/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

This comment has been minimized.

Copy link
@cheftako

cheftako Apr 2, 2019

Member

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?

This comment has been minimized.

Copy link
@cheftako

cheftako Apr 2, 2019

Member

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

This comment has been minimized.

Copy link
@cheftako

cheftako Apr 2, 2019

Member

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().

This comment has been minimized.

Copy link
@simonferquel

simonferquel Apr 2, 2019

Contributor

Httputil.ReverseProxy requires it. (Note that all these interfaces are embedded in the structure, to make it implement all of them)

This comment has been minimized.

Copy link
@cheftako

cheftako Apr 2, 2019

Member

Thanks, I think that cleared up both my concerns.

@dims

This comment has been minimized.

Copy link
Member

commented Apr 2, 2019

/lgtm

@cheftako
Copy link
Member

left a comment

/lgtm

@k8s-ci-robot k8s-ci-robot merged commit 6681c12 into kubernetes:master Apr 2, 2019

17 checks passed

cla/linuxfoundation liggitt authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-conformance-image-test Skipped.
pull-kubernetes-cross Skipped.
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-godeps Job succeeded.
Details
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped.
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
pull-publishing-bot-validate Skipped.
tide In merge pool.
Details
@liggitt

This comment has been minimized.

Copy link
Member Author

commented Apr 2, 2019

picked to 1.14.1 in #76046

@liggitt liggitt deleted the liggitt:flush-headers branch Apr 3, 2019

@cheftako

This comment has been minimized.

Copy link
Member

commented Apr 4, 2019

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?

@liggitt

This comment has been minimized.

Copy link
Member Author

commented Apr 4, 2019

opened #76154 to track

k8s-ci-robot added a commit that referenced this pull request Apr 4, 2019

Merge pull request #76046 from liggitt/automated-cherry-pick-of-#7588…
…7-upstream-release-1.14

Automated cherry pick of #75887: Explicitly flush headers when proxying
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.