Skip to content

config: enable HTTP/2#417

Merged
rebello95 merged 10 commits intomasterfrom
h2
Oct 14, 2019
Merged

config: enable HTTP/2#417
rebello95 merged 10 commits intomasterfrom
h2

Conversation

@rebello95
Copy link
Copy Markdown
Contributor

@rebello95 rebello95 commented Sep 6, 2019 •
edited
Loading

Updates the standard configuration to send requests over HTTP/2 by default.

Note that there are future improvements that should be made here to support negotiating between HTTP/1.1 and HTTP/2, being tracked in #502.

More info on the config change: https://www.envoyproxy.io/docs/envoy/latest/api-v2/api/v2/cds.proto#envoy-api-msg-cluster

Signed-off-by: Michael Rebello me@michaelrebello.com

@rebello95
config: enable HTTP2
180a89c 
Allows us to execute HTTP2 requests.

More info: https://www.envoyproxy.io/docs/envoy/latest/api-v2/api/v2/cds.proto#envoy-api-msg-cluster

Signed-off-by: Michael Rebello <me@michaelrebello.com>
@rebello95
Merge remote-tracking branch 'origin/master' into h2
1320a28 
Signed-off-by: Michael Rebello <me@michaelrebello.com>
@rebello95
Merge remote-tracking branch 'origin/master' into h2
cc14faf 
Signed-off-by: Michael Rebello <me@michaelrebello.com>
@rebello95
Merge remote-tracking branch 'origin/master' into h2
b0819e8 
Signed-off-by: Michael Rebello <me@michaelrebello.com>
@rebello95
Copy link
Copy Markdown
Contributor Author

rebello95 commented Sep 12, 2019

Android CI failures seem to be legit - getting 503s when attempting to make requests from the demo app

@rebello95
Copy link
Copy Markdown
Contributor Author

rebello95 commented Sep 12, 2019

iOS is actually failing as well - seems like this might have to do with the domain we're hitting

@rebello95
use https
edc27c6 
Signed-off-by: Michael Rebello <me@michaelrebello.com>
@rebello95
Merge remote-tracking branch 'origin/master' into h2
b5e9d18 
Signed-off-by: Michael Rebello <me@michaelrebello.com>
@rebello95
Merge remote-tracking branch 'origin/master' into h2
84b7c87 
Signed-off-by: Michael Rebello <me@michaelrebello.com>
@stale
Copy link
Copy Markdown

stale bot commented Sep 27, 2019

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@stale stale bot added the stale label Sep 27, 2019
@stale
Copy link
Copy Markdown

stale bot commented Oct 4, 2019

This pull request has been automatically closed because it has not had activity in the last 14 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@stale stale bot closed this Oct 4, 2019
@rebello95 rebello95 reopened this Oct 4, 2019
@stale stale bot removed the stale label Oct 4, 2019
@rebello95
Copy link
Copy Markdown
Contributor Author

rebello95 commented Oct 4, 2019

@goaway is investigating this

@junr03
Copy link
Copy Markdown
Member

junr03 commented Oct 4, 2019

Drive by to update with the context I had that I missed to post here.

S3 does not support h2. So when envoy sends h2 frames to S3, S3 sends back an http/1.1 frames and nghttp2 fails and errors in callbacks to the router which gets charged to the client as a 503.

@rebello95
Copy link
Copy Markdown
Contributor Author

rebello95 commented Oct 4, 2019

Thanks @junr03. Additional details from what we've seen recently:

  • Sending non-bodied requests (i.e., GET) over HTTP/2 works fine
  • Sending requests with bodies (i.e., POST) fail, and it seems like upstream receives invalid bodies

@rebello95
Merge remote-tracking branch 'origin/master' into h2
d5db506 
Signed-off-by: Michael Rebello <me@michaelrebello.com>
@rebello95
updates
0f8415e 
Signed-off-by: Michael Rebello <me@michaelrebello.com>
@rebello95
fixup
1b30d73 
Signed-off-by: Michael Rebello <me@michaelrebello.com>
@rebello95
Copy link
Copy Markdown
Contributor Author

rebello95 commented Oct 14, 2019

PR is green now. To summarize findings from related investigations:

  • S3 does not support HTTP/2, which is why our demo apps were failing. Fix: Make a request to a sample ping endpoint which supports HTTP/2
  • gRPC expects client-side closes to be done using an empty data frame, not trailers. Fix: grpc: close stream with data frame #500

@rebello95 rebello95 assigned junr03 and unassigned goaway Oct 14, 2019
@rebello95 rebello95 mentioned this pull request Oct 14, 2019
Open
@rebello95 rebello95 changed the title config: enable HTTP2 config: enable HTTP/2 Oct 14, 2019
junr03
junr03 approved these changes Oct 14, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@junr03 junr03 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 adding the context, and for carrying this through.

@rebello95 rebello95 merged commit 604dbbc into master Oct 14, 2019
@rebello95 rebello95 deleted the h2 branch October 14, 2019 20:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@goaway goaway Awaiting requested review from goaway

@buildbreaker buildbreaker Awaiting requested review from buildbreaker

1 more reviewer

@junr03 junr03 junr03 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@junr03 junr03

Labels

no stalebot

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@rebello95 @junr03 @goaway