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

=htc Support `UseHttp2.Negotiated` for http2 connections over plain http #2543

Merged
merged 10 commits into from Jun 5, 2019

Conversation

@viktorklang
Copy link
Member

commented May 24, 2019

Purpose

If we want to support things like serving unencrypted gRPC traffic from the same server instance as unencrypted REST traffic—we already support serving encrypted gRPC traffic and REST traffic from the same server as is with UseHttp2.ALWAYS—then the server logic needs to be changed.

This draft PR adds support for a new UseHttp2 setting called PriorKnowledge,
this allows for serving non-tls http requests for both HTTP1.1 and HTTP2.0 on the same server/port.
If an incoming request sends the HTTP2.0 PRI preamble we service that request through an HTTP2 pipeline, otherwise we serve it though the HTTP1.1 pipeline.

References

References #2523

Changes

Adds a PriorKnowledgeSwitch in the same vein as AlpnSwitch, and then sniffs the initial bytes of each request to match against the HTTP2 Prior Knowledge preamble.

Background Context

@raboof and @jrudolph pointed me in this direction.

This feature, assuming approval by the maintainers, will be getting a test suite to verify the implementation. Current testing has been done with curl to validate that it works with existing, external, clients.

use the curl flag --http2-prior-knowledge to have curl send the HTTP2 Prior Knowledge preamble with the request.

@viktorklang viktorklang requested review from jrudolph and raboof May 24, 2019

@akka-ci akka-ci added validating tested and removed validating labels May 24, 2019

@akka-ci

This comment has been minimized.

Copy link
Collaborator

commented May 24, 2019

Test PASSed.

@raboof

This comment has been minimized.

Copy link
Member

commented May 24, 2019

Related to #2085 (a more common but arguably less elegant way to support both http 1 and 2 on the same unencrypted port)

@raboof
Copy link
Member

left a comment

First look: looks good, couple of comments here and there. Haven't looked closely at the changes to the main Http2 class yet, but at first glance it seems like a pretty neat cut!

val http2Enabled = settings.previewServerSettings.enableHttp2 && connectionContext.http2 != Never
val http2Forced = connectionContext.http2 == Always
if (http2Enabled && (connectionContext.isSecure || http2Forced)) {
val http2Enabled = true || settings.previewServerSettings.enableHttp2 && connectionContext.http2 != Never

This comment has been minimized.

Copy link
@raboof

raboof May 24, 2019

Member

don't forget to remove this ;)

This comment has been minimized.

Copy link
@viktorklang

viktorklang May 24, 2019

Author Member

haha, good catch! :D

@@ -17,4 +17,5 @@ object UseHttp2 {
def always: UseHttp2 = Always
def negotiated: UseHttp2 = Negotiated
def never: UseHttp2 = Never
def priorKnowledge: UseHttp2 = PriorKnowledge

This comment has been minimized.

Copy link
@raboof

raboof May 24, 2019

Member

When this PR and #1966 are both merged, it would be useful to be able to support h2c via upgrade and h2c with prior knowledge on the same port.

Perhaps negotiated should enable h2c via 'upgrade' (since it doesn't add overhead), and priorKnowledge should enable both h2c mechanisms? That behavior seems reasonable, but then perhaps the naming needs some more thought?

This comment has been minimized.

Copy link
@viktorklang

viktorklang May 24, 2019

Author Member

Perhaps consolidate that behavior once both PRs are merged? (to make them independent)

This comment has been minimized.

Copy link
@raboof

raboof May 24, 2019

Member

sure, just throwing this out there since finding a better name now is nicer than changing it (or not ;) ) later ;)

This comment has been minimized.

Copy link
@viktorklang

viktorklang May 26, 2019

Author Member

@raboof I didn't find any preexisting good place to write tests for this feature, is there somewhere you'd recommend?

install(http2Stack, data)
else
install(http1Stack, data)
} else if (data.isEmpty || data.startsWith(PRIOR_KNOWLEDGE_PREFACE, 0)) { // Still unknown

This comment has been minimized.

Copy link
@raboof

raboof May 24, 2019

Member

data.startsWith(PRIOR_KNOWLEDGE_PREFACE, 0) will always be false here, since data.length >= PRIOR_KNOWLEDGE_PREFACE.length was already false, I think?

This comment has been minimized.

Copy link
@raboof

raboof May 24, 2019

Member

(looks like the intent was to check whether the data received so far matches the preface, so we can switch to http1 as soon as we see a mismatch - which makes sense)

This comment has been minimized.

Copy link
@viktorklang

viktorklang May 24, 2019

Author Member

@raboof Indeed!


/** INTERNAL API */
@InternalApi
private[http] object PriorKnowledgeSwitch {

This comment has been minimized.

Copy link
@raboof

raboof May 24, 2019

Member

Seems pretty verbose for what it does, but I can't really think of a more straightforward way either.

This comment has been minimized.

Copy link
@viktorklang

viktorklang May 24, 2019

Author Member

I'd be more than happy to cut down the noise here, open to all suggestions!

This comment has been minimized.

Copy link
@jrudolph

jrudolph Jun 4, 2019

Member

Since it is mostly copied and adapted from AlpnSwitch, it would be nice if we could abstract it somehow.

This comment has been minimized.

Copy link
@viktorklang

viktorklang Jun 5, 2019

Author Member

@jrudolph I thought about that, but then again, the AlpnSwitch deals with Bidi and this works with Flow, so I think the correct move in that case would be to abstract each separately so that you'd be reusable for other stages. That does seem orthogonal to this PR though?

@akka-ci

This comment has been minimized.

Copy link
Collaborator

commented May 24, 2019

Test PASSed.

@akka-ci

This comment has been minimized.

Copy link
Collaborator

commented May 28, 2019

Test FAILed.

@akka-ci

This comment has been minimized.

Copy link
Collaborator

commented May 28, 2019

Test PASSed.

@viktorklang viktorklang marked this pull request as ready for review May 28, 2019

@viktorklang

This comment has been minimized.

Copy link
Member Author

commented May 28, 2019

@raboof @jrudolph This is ready for final review!

@akka-ci akka-ci added tested and removed validating labels Jun 5, 2019

@akka-ci

This comment has been minimized.

Copy link
Collaborator

commented Jun 5, 2019

Test PASSed.

@jrudolph

This comment has been minimized.

Copy link
Member

commented Jun 5, 2019

How about merging this as is, and then create an Issue to collapse the UseHttp2 flags (ideally deprecating all of them and having things just work by inspecting the isSecure option).

It doesn't seem pressing to merge it right now, so I'd rather improve it first and then merge it, instead of adding a rarely-used feature to our pile of code we have no capacity to maintain. Especially, the public API part needs to be figured out, since UseHttp2 doesn't seem to be marked @ApiMayChange.

@viktorklang

This comment has been minimized.

Copy link
Member Author

commented Jun 5, 2019

@jrudolph Well, I need it for StateServ: cloudstateio/cloudstate@a6b6b70

raboof added a commit that referenced this pull request Jun 5, 2019

Roll together 'prior knowledge' and 'negotiated', take 1
Just for discussion, I don't think we should do this.
#2543 (comment)

I'm not sure we should do this, as `Negotiated` is the default, and adding the
`PriorKnowledgeSwitch` might have a performance penalty on top of 'plain'
HTTP/1.

For the same reason I'm not sure we should roll it into `Always`.

raboof added a commit that referenced this pull request Jun 5, 2019

Roll together 'prior knowledge' and 'negotiated'
Just for discussion, I'm not sure we should do this.
#2543 (comment)

I'm hesitant because `Negotiated` is the default, and adding the
`PriorKnowledgeSwitch` might have a performance penalty on top of 'plain'
HTTP/1.

For the same reason I'm not sure we should roll it into `Always`.
Roll together 'prior knowledge' and 'negotiated' (#2553)
Just for discussion, I'm not sure we should do this.
#2543 (comment)

I'm hesitant because `Negotiated` is the default, and adding the
`PriorKnowledgeSwitch` might have a performance penalty on top of 'plain'
HTTP/1.

For the same reason I'm not sure we should roll it into `Always`.
@akka-ci

This comment has been minimized.

Copy link
Collaborator

commented Jun 5, 2019

Test PASSed.

@viktorklang

This comment has been minimized.

Copy link
Member Author

commented Jun 5, 2019

@raboof @jrudolph Let me know if you want me to do the honors of merging!

@jrudolph

This comment has been minimized.

Copy link
Member

commented Jun 5, 2019

I'd like to have a try at removing the code duplication in the switches.

@viktorklang

This comment has been minimized.

Copy link
Member Author

commented Jun 5, 2019

@jrudolph Ok, cool, let me know if you want me to review it!

@jrudolph
Copy link
Member

left a comment

From my perspective we could merge this as is, I have a follow up PR ready to clean things up a bit.

@jrudolph jrudolph changed the title Wip prior knowledge √ =htc Support `UseHttp2.Negotiated` for http2 connections over plain http Jun 5, 2019

@jrudolph jrudolph merged commit 7eff56d into master Jun 5, 2019

5 checks passed

Jenkins PR Auto-Formatter Successful
Details
Jenkins PR Validation Test PASSed. 4209 tests run, 1145 skipped, 0 failed.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
typesafe-cla-validator All users have signed the CLA
Details
@jrudolph

This comment has been minimized.

Copy link
Member

commented Jun 5, 2019

Thanks, @viktorklang, that was some quick turnaround on that feature.

@viktorklang

This comment has been minimized.

Copy link
Member Author

commented Jun 5, 2019

Thanks @jrudolph, I couldn't have completed it in such short amount of time without the great advice and contributions from the Akka team! ^^

install(http2Stack, data)
else
install(http1Stack, data)
} else if (data.isEmpty || data.startsWith(HTTP2_CONNECTION_PREFACE, 0)) { // Still unknown

This comment has been minimized.

Copy link
@jroper

jroper Jun 6, 2019

Contributor

Shouldn't this be HTTP2_CONNECTION_PREFACE.startsWith(data, 0)? data.startsWith(HTTP2_CONNECTION_PREFACE, 0) will always evaluate to false, since we know that data.length < HTTP2_CONNECTION_PREFACE.length.

This comment has been minimized.

Copy link
@raboof

raboof Jun 6, 2019

Member

Jup, I'll PR

This comment has been minimized.

Copy link
@jrudolph

jrudolph Jun 6, 2019

Member

We can fix that in my PR directly.

This comment has been minimized.

Copy link
@raboof
@viktorklang

This comment has been minimized.

Copy link
Member Author

commented Jun 6, 2019

@jrudolph jrudolph deleted the wip-prior-knowledge-√ branch Jun 6, 2019

franktominc added a commit to franktominc/akka-http that referenced this pull request Jun 12, 2019

=htc Support `UseHttp2.Negotiated` for http2 connections over plain h…
…ttp (akka#2543)

By looking ahead and deciding on the prior knowledge preface a
potential HTTP/2 client sends whether to use HTTP/1.1 or HTTP/2.

Co-Authored-By: Arnout Engelen <github@bzzt.net>
Co-Authored-By: Johannes Rudolph <johannes.rudolph@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.