Skip to content

Make HTTP/1.0 responses without Connection: keep-alive mean close.#7124

Merged
mattklein123 merged 9 commits intoenvoyproxy:masterfrom
jplevyak:HTTP10-no-keepalive-means-close
Jun 5, 2019
Merged

Make HTTP/1.0 responses without Connection: keep-alive mean close.#7124
mattklein123 merged 9 commits intoenvoyproxy:masterfrom
jplevyak:HTTP10-no-keepalive-means-close

Conversation

@jplevyak
Copy link
Contributor

Signed-off-by: John Plevyak jplevyak@gmail.com

For an explanation of how to fill out the fields, please see the relevant section
in PULL_REQUESTS.md

Description: Currently HTTP/1.0 requests without a Connection: close cause the connection to remain open. This does not conform to the HTTP/1.0 specification. Connections without Connection: keep-alive should be expected to close. Failing to do so can result in 503 errors if a new request is sent on that connection before the close is detected.
Risk Level: Medium
Testing: 24 hours with 5 applications
Docs Changes:
Release Notes: HTTP/1.0 responses without Connection: keep-alive now means close.
[Optional Fixes #Issue] #7123
[Optional Deprecated:]

Signed-off-by: John Plevyak <jplevyak@gmail.com>
@lizan lizan requested a review from alyssawilk June 2, 2019 21:01
Copy link
Contributor

@alyssawilk alyssawilk left a comment

Choose a reason for hiding this comment

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

Thanks for tackling this! Would you be up for rephrasing the release notes to make it clear this is an Envoy-upstream issue? I believe the Envoy-downstream behavior is currently correct and I was confused about what was going wrong until I started reading your fix :-)

jplevyak added 2 commits June 3, 2019 09:34
Signed-off-by: John Plevyak <jplevyak@gmail.com>
Signed-off-by: John Plevyak <jplevyak@gmail.com>
Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM with some comments.

/wait

Signed-off-by: John Plevyak <jplevyak@gmail.com>
Signed-off-by: John Plevyak <jplevyak@gmail.com>
Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

LGTM with a few comments. Thanks for iterating.

/wait

Signed-off-by: John Plevyak <jplevyak@gmail.com>
Signed-off-by: John Plevyak <jplevyak@gmail.com>
alyssawilk
alyssawilk previously approved these changes Jun 5, 2019
Copy link
Contributor

@alyssawilk alyssawilk left a comment

Choose a reason for hiding this comment

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

Excellent, thanks for the fix! LGTM pending Matt's sign-off

Signed-off-by: John Plevyak <jplevyak@gmail.com>
Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Awesome, thank you.

…ve-means-close

Signed-off-by: John Plevyak <jplevyak@gmail.com>
@mattklein123
Copy link
Member

/retest

@repokitteh-read-only
Copy link

🔨 rebuilding ci/circleci: tsan (failed build)

🐱

Caused by: a #7124 (comment) was created by @mattklein123.

see: more, trace.

@mattklein123 mattklein123 merged commit 0bafb9e into envoyproxy:master Jun 5, 2019
duderino pushed a commit to istio/envoy that referenced this pull request Jun 12, 2019
1.2 cherry-pick:  Make HTTP/1.0 responses without Connection: keep-alive mean close. (envoyproxy#7124)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants