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

Allow Http2 without negotiation/TLS #1934

Merged
merged 20 commits into from Apr 13, 2018
Merged

Conversation

raboof
Copy link
Member

@raboof raboof commented Mar 14, 2018

Refs #1849

@akka-ci akka-ci added validating PR that is currently being validated by Jenkins tested PR that was successfully built and tested by Jenkins and removed validating PR that is currently being validated by Jenkins labels Mar 14, 2018
@akka-ci
Copy link

akka-ci commented Mar 14, 2018

Test PASSed.

@akka-ci akka-ci added validating PR that is currently being validated by Jenkins needs-attention Indicates a PR validation failure (set by CI infrastructure) and removed tested PR that was successfully built and tested by Jenkins validating PR that is currently being validated by Jenkins labels Mar 14, 2018
@akka-ci
Copy link

akka-ci commented Mar 14, 2018

Test FAILed.

@raboof raboof force-pushed the akkaHttp2withoutNegotiation branch from 5b00b24 to fd6c5db Compare March 14, 2018 16:48
@akka-ci akka-ci added validating PR that is currently being validated by Jenkins needs-attention Indicates a PR validation failure (set by CI infrastructure) and removed needs-attention Indicates a PR validation failure (set by CI infrastructure) validating PR that is currently being validated by Jenkins labels Mar 14, 2018
@akka-ci
Copy link

akka-ci commented Mar 14, 2018

Test FAILed.

@raboof raboof force-pushed the akkaHttp2withoutNegotiation branch from fd6c5db to 46c193f Compare March 14, 2018 17:26
@akka-ci akka-ci added validating PR that is currently being validated by Jenkins needs-attention Indicates a PR validation failure (set by CI infrastructure) and removed needs-attention Indicates a PR validation failure (set by CI infrastructure) validating PR that is currently being validated by Jenkins labels Mar 14, 2018
@akka-ci
Copy link

akka-ci commented Mar 14, 2018

Test FAILed.

@akka-ci akka-ci added validating PR that is currently being validated by Jenkins tested PR that was successfully built and tested by Jenkins and removed needs-attention Indicates a PR validation failure (set by CI infrastructure) validating PR that is currently being validated by Jenkins labels Mar 14, 2018
@akka-ci
Copy link

akka-ci commented Mar 14, 2018

Test PASSed.

@ktoso
Copy link
Member

ktoso commented Mar 15, 2018

Some comments, looks like a good start though. We can merge this as we continue working on these things and polish remaining API / deduplicate later

}

class Http2(system: ExtendedActorSystem) extends Extension {
import akka.dispatch.ExecutionContexts.{ sameThreadExecutionContext ⇒ ec }
Copy link
Member

Choose a reason for hiding this comment

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

please pass it explicitly rather than import like this; we want to avoid using it accidentally


object Http2 extends ExtensionId[Http2] with ExtensionIdProvider {
override def get(system: ActorSystem): Http2 = super.get(system)
def lookup() = Http
Copy link
Member

Choose a reason for hiding this comment

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

Http2

def createExtension(system: ExtendedActorSystem): Http2 = new Http2(system)
}

class Http2(system: ExtendedActorSystem) extends Extension {
Copy link
Member

Choose a reason for hiding this comment

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

final

import akka.dispatch.ExecutionContexts.{ sameThreadExecutionContext ⇒ ec }

import language.implicitConversions
private implicit def javaModelIsScalaModel[J <: AnyRef, S <: J](in: Future[J]): Future[S] = in.asInstanceOf[Future[S]]
Copy link
Member

Choose a reason for hiding this comment

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

fun one ;) ok

import scala.compat.java8.FutureConverters._
import scala.concurrent.Future

object Http2 extends ExtensionId[Http2] with ExtensionIdProvider {
Copy link
Member

Choose a reason for hiding this comment

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

If we're introducing it, then please mirror methods from scaladsl's Http2 or mark with huge FIXME's that it's not mirroring all methods yet

/**
* Handle requests using HTTP/2 immediately, without any TLS or negotiation layer.
*/
def bindAndHandleRaw(
Copy link
Member

Choose a reason for hiding this comment

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

Raw sounds a bit weird; bindAndHandlePlainText? AFAIR that's also wording used in GRPC

Copy link
Member Author

Choose a reason for hiding this comment

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

That's indeed what io.grpc.netty uses, but I found PlainText a bit weird since even though there's no encryption, it's still a binary protocol.

This approach is described in the spec as starting 'with prior knowledge', but bindAndHandleWithPriorKnowledge is weird because it's the client that has prior knowledge, not the server.

Copy link
Member

Choose a reason for hiding this comment

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

would it be possible to make it a configuration property or a special connectionContext instead?

I'd image that you might want to easily switch between this and the real thing, perhaps running (some) tests with this and full integration tests and production with the real thing.

Copy link
Member

Choose a reason for hiding this comment

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

That's indeed what io.grpc.netty uses, but I found PlainText a bit weird since even though there's no encryption, it's still a binary protocol.

Yeah, I've heard this argument but the general feel it conveys seems right "though shalt not use paintext in prod" ;-) Crypto wise I think plaintext is referred to non encrypted, regardless if binary or not?

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I meant this comment; Yeah trying to achieve this with a connection context would be preferable indeed, could you give it a look @raboof ?

Copy link
Member

Choose a reason for hiding this comment

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

Basically it's the same as we have in Akka HTTP/1, where we have the plain and https one; the same could work here basically, even if we strongly recommend the https one

@@ -91,7 +134,7 @@ class Http2Ext(private val config: Config)(implicit val system: ActorSystem)
val connections = Tcp().bind(interface, effectivePort, settings.backlog, settings.socketOptions, halfClose = false, settings.timeouts.idleTimeout)

connections.mapAsyncUnordered(settings.maxConnections) {
case incoming: Tcp.IncomingConnection ⇒
incoming: Tcp.IncomingConnection ⇒
Copy link
Member

Choose a reason for hiding this comment

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

good

log: LoggingAdapter = system.log)(implicit fm: Materializer): Future[ServerBinding] = {
val effectivePort = if (port >= 0) port else 80

val serverLayer: Flow[ByteString, ByteString, Future[Done]] = Flow.fromGraph(StreamUtils.fuseAggressive(
Copy link
Member

Choose a reason for hiding this comment

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

we can remove StreamUtils.fuseAggressive now since Akka HTTP 10.1 requires 2.5 and this is a No-Op on 2.5+

@akka-ci akka-ci added validating PR that is currently being validated by Jenkins and removed tested PR that was successfully built and tested by Jenkins labels Mar 15, 2018
@raboof raboof force-pushed the akkaHttp2withoutNegotiation branch from 9f4447d to d6f6084 Compare April 13, 2018 08:17
@akka-ci akka-ci added validating PR that is currently being validated by Jenkins needs-attention Indicates a PR validation failure (set by CI infrastructure) and removed tested PR that was successfully built and tested by Jenkins validating PR that is currently being validated by Jenkins labels Apr 13, 2018
@akka-ci
Copy link

akka-ci commented Apr 13, 2018

Test FAILed.

@akka-ci akka-ci added validating PR that is currently being validated by Jenkins tested PR that was successfully built and tested by Jenkins and removed needs-attention Indicates a PR validation failure (set by CI infrastructure) validating PR that is currently being validated by Jenkins labels Apr 13, 2018
@akka-ci
Copy link

akka-ci commented Apr 13, 2018

Test PASSed.

@raboof raboof merged commit 174a694 into master Apr 13, 2018
@raboof raboof deleted the akkaHttp2withoutNegotiation branch April 13, 2018 10:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2 - pick next Used to mark issues which are next up in the queue to be worked on. The tag is non-binding tested PR that was successfully built and tested by Jenkins
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants