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

Support HTTP2 in cleartext (h2c) via Upgrade from HTTP1 #2464

Merged
merged 3 commits into from Sep 13, 2019

Conversation

@raboof
Copy link
Member

commented Mar 13, 2019

Continuation of #2085, refs #1966

This allows HTTP/1.1 and HTTP/2 to coexist on the same insecure port.
This feature is not widely supported among clients: browsers
generally do not support h2c at all, curl and nghttp somewhat support
it, grpc-java only since fairly recently

@akka-ci

This comment has been minimized.

Copy link
Collaborator

commented Mar 13, 2019

Test FAILed.

@akka-ci

This comment has been minimized.

Copy link
Collaborator

commented Mar 13, 2019

Test PASSed.

@akka-ci

This comment has been minimized.

Copy link
Collaborator

commented Mar 15, 2019

Test PASSed.

@raboof raboof force-pushed the h2c branch 2 times, most recently from 0990ea6 to 6af040b Jun 18, 2019
@akka-ci akka-ci added validating and removed tested labels Jun 18, 2019
@raboof

This comment has been minimized.

Copy link
Member Author

commented Jun 18, 2019

Rebased on top of master. Needs work: the test fails because it seems somehow the demand from the sink does not properly propagate to the protocol switch.

@akka-ci akka-ci added needs-attention and removed validating labels Jun 18, 2019
@akka-ci

This comment has been minimized.

Copy link
Collaborator

commented Jun 18, 2019

Test FAILed.

@akka-ci

This comment has been minimized.

Copy link
Collaborator

commented Sep 9, 2019

Test FAILed.

@akka-ci

This comment has been minimized.

Copy link
Collaborator

commented Sep 10, 2019

Test PASSed.

Copy link
Member

left a comment

LGTM, with a few nitpicks and comments

/*
* WebSocket support
*/
def switchToWebSocket(handlerFlow: Either[Graph[FlowShape[FrameEvent, FrameEvent], Any], Graph[FlowShape[Message, Message], Any]]): Unit = {

This comment has been minimized.

Copy link
@jrudolph

jrudolph Sep 10, 2019

Member

Good cleanup here. It was a bit weird anyway that part of the WebSocket flow creation was done inside here.

@@ -144,15 +147,22 @@ private[http] object Handshake {
concatenated value to obtain a 20-byte value and base64-
encoding (see Section 4 of [RFC4648]) this 20-byte hash.
*/
def buildResponse(key: `Sec-WebSocket-Key`, handler: Either[Graph[FlowShape[FrameEvent, FrameEvent], Any], Graph[FlowShape[Message, Message], Any]], subprotocol: Option[String]): HttpResponse =
def buildResponse(key: `Sec-WebSocket-Key`, handler: Either[Graph[FlowShape[FrameEvent, FrameEvent], Any], Graph[FlowShape[Message, Message], Any]], subprotocol: Option[String], settings: WebSocketSettings, log: LoggingAdapter): HttpResponse = {
val frameHandler = handler match {

This comment has been minimized.

Copy link
@jrudolph

jrudolph Sep 10, 2019

Member

That's good for now. I guess we should move those flow creation things to WebSocket at some point.

@@ -81,6 +81,10 @@ private[http] object Base64Parsing {
val rfc2045BlockDecoder: Decoder = decodeBlock(Base64.rfc2045())
val customBlockDecoder: Decoder = decodeBlock(Base64.custom())

private val base64url = new Base64("ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789-_=")

This comment has been minimized.

Copy link
@jrudolph

jrudolph Sep 10, 2019

Member

Great, yet another base64 variant ;)

@@ -198,6 +205,29 @@ private[http2] class Http2ServerDemux(http2Settings: Http2ServerSettings) extend
multiplexer.registerSubStream(sub)
}
})

private def applySettings(settings: immutable.Seq[Setting]): Boolean = {

This comment has been minimized.

Copy link
@jrudolph

jrudolph Sep 10, 2019

Member

Ok 👍


Scala
: @@snip[Http2Spec.scala]($test$/scala/docs/http/scaladsl/Http2Spec.scala) { #bindAndHandlePlain }

Java
: @@snip[Http2Test.java]($test$/java/docs/http/javadsl/Http2Test.java) { #bindAndHandlePlain }

#### h2c Upgrade

This comment has been minimized.

Copy link
@jrudolph
@@ -24,6 +24,11 @@ import akka.http.scaladsl.HttpConnectionContext

//#bindAndHandlePlain

//#bindAndHandleNegotiateUpgrade
import akka.http.scaladsl.UseHttp2.Negotiated

This comment has been minimized.

Copy link
@jrudolph

jrudolph Sep 10, 2019

Member

Still needed?

handler(req)
}

case _ =>

This comment has been minimized.

Copy link
@jrudolph

jrudolph Sep 10, 2019

Member

Ok, so the extra overhead for HTTP/1.1 when HTTP/2 is enabled is now checking that header here. I guess that's ok.

@akka-ci

This comment has been minimized.

Copy link
Collaborator

commented Sep 10, 2019

Test PASSed.

Copy link
Member

left a comment

LGTM with one small suggestion

log: LoggingAdapter
): HttpRequest => Future[HttpResponse] = { req =>
req.header[Upgrade] match {
case Some(upgrade) if upgrade.protocols.exists(_.name equalsIgnoreCase "h2c") =>

This comment has been minimized.

Copy link
@johanandren

johanandren Sep 12, 2019

Member

Feels like an if-else rather than a patmatch with catchall to me?

if (req.header[Upgrade].exists(_.name.equalsIgnoreCase("h2x"))) {} else {}

Maybe worth also caching that lambda since it is evaluated for each request?

This comment has been minimized.

Copy link
@raboof

raboof Sep 13, 2019

Author Member

it's a little more tricky because the exists is on the list of protocols, not on the option... good point about the lambda though, will try to rewrite

This comment has been minimized.

Copy link
@raboof

raboof Sep 13, 2019

Author Member

Hmm actually I struggle coming up with a solution that is meaningfully better, could you suggest something?

This comment has been minimized.

Copy link
@johanandren

johanandren Sep 13, 2019

Member

Nah, leave it be in that case.

@raboof raboof self-assigned this Sep 13, 2019
raboof added 3 commits May 3, 2018
@akka-ci

This comment has been minimized.

Copy link
Collaborator

commented Sep 13, 2019

Test PASSed.

Pull request validation report

@raboof raboof merged commit 1d91784 into master Sep 13, 2019
4 of 5 checks passed
4 of 5 checks passed
Jenkins PR Auto-Formatter Failed
Details
Jenkins PR Validation Test PASSed. 4172 tests run, 1074 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
@raboof raboof deleted the h2c branch Sep 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.