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

HTTP/2 and server push support #1349

Merged
merged 21 commits into from Jan 2, 2016

Conversation

Projects
None yet
4 participants
@arteam
Member

arteam commented Nov 21, 2015

This PR adds support for HTTP/2 over TLS, HTTP/2 plain text connections and server push to Dropwizard. This is based on the work in Jetty 9.3 branch.

We use to have SPDY support, but this protocol is completelty deprecated and supressed by
HTTP/2. Jetty 9.3.* now provides only HTTP/2 support. One important distinction from SPDY is that HTTP/2 can work over TLS, as well as over a plain connection. The latter connection type is not supported by major browsers, but could be useful for internal network installations or for handling non-enctypted traffic from load balancers.

This PR provides two connection factories for each connection type. HTTP/2 over TLS has http2 key, HTTP/2 over plain text - http2c.

Other important feature is support for the server push techonology. The server can send additional
resources along with the requested resource to clients on a single response. This could be a big
perfomance optimization for web pages that contain many static assets.

Jetty has ServerPushFilter which provides a good heuristic for determining primary and additinal resources. It uses URI from the Referer header and request timestamps to guess that resources are linked with each other. This change adds a simple factory for enabling this filter from the configuration. By default it's disabled.

@arteam arteam added the feature label Nov 21, 2015

@arteam arteam added this to the 1.0.0 milestone Nov 21, 2015

@jplock

This comment has been minimized.

Member

jplock commented Nov 22, 2015

What needs to be done to have JerseyClient support HTTP/2? Is it just a matter of waiting until its been upgraded?

This is great work by the way. I've been following https://github.com/grpc/grpc-java for awhile and I'm excited about having HTTP/2 support in Dropwizard.

@arteam

This comment has been minimized.

Member

arteam commented Nov 22, 2015

Yes, JerseyClient and Apache HttpClient don't support HTTP/2 currently.

I know 3 Java client HTTP/2 libraries:

I like the first one from Jetty, because:

  • It provides an asynchronous API;
  • h2 and h2c support;
  • Has the same core as jetty-http2-server. It provides a good interopability with the server implementation. Despite that HTTP/2 is a standart, there could be some idiosyncrasy .
@jplock

This comment has been minimized.

Member

jplock commented Nov 22, 2015

Yea, I agree using the Jetty Http2Client is the best choice for now. Thanks!

@brentryan

This comment has been minimized.

brentryan commented Nov 23, 2015

Have you guys looked into switching to retrofit for clients instead? It uses OkHttp under the hood and has nice async features.

@arteam

This comment has been minimized.

Member

arteam commented Nov 24, 2015

Have you guys looked into switching to retrofit for clients instead? It uses OkHttp under the hood and has nice async features.

Personally, I don't like OkHttp and Retrofit much, because they are developed for Android in the first place. Currently they run on OpenJDK/HotSpot, but maintainers could drop its support in any time.

Yes, the library has a fluent API and code quality is really good. Nevertheless, I wouldn't pull it into the Dropwizard core. But we open to add any integration to the list of 3rd party modules, if anyone develop it.

// request with an Upgrade header with "h2c" value. The server supports HTTP/2 clear text connections,
// so it will return the predefined HTTP/2 preamble and the client and the server will switch to the
// new protocol.
return buildConnector(server, new ScheduledExecutorScheduler(), buildBufferPool(), name, threadPool,

This comment has been minimized.

@jplock

jplock Nov 24, 2015

Member

Should this ScheduledExecutorScheduler() be Managed so its stopped and started alongside Jetty?

This comment has been minimized.

@arteam

arteam Nov 24, 2015

Member

It implements Scheduler which in turn extends LifeCycle. It will be closed along with the connector.

@brentryan

This comment has been minimized.

brentryan commented Nov 25, 2015

@arteam why does it matter what they developed the http client for if it works better, has clean code, is resilient, and has decent perf when compared to anything else out there. It runs fine on oracle jdk as well so I'm confused by your comment.

@jplock

This comment has been minimized.

Member

jplock commented Nov 25, 2015

@arteam was there anything else you'd like to add to this, otherwise I think its ready for merging? Any other feedback @dropwizard/committers? This is great. Glad to see this get into 1.0.

@arteam

This comment has been minimized.

Member

arteam commented Nov 25, 2015

I'm think that the PR is ready to merge. But I would wait for another feedback from @dropwizard/committers, because it's a quite a big change and I could miss something.

Also it would be great, if someone tested the HTTP/2 integration with a SSL cerftificate issued by CA. I tested it only on self-signed ceritificates.

@arteam

This comment has been minimized.

Member

arteam commented Dec 1, 2015

BTW, a very interesting talk about HTTP/2 and Java clients/servers by Fabian Stäber.

@nickbabcock

This comment has been minimized.

Contributor

nickbabcock commented Dec 7, 2015

Something is causing this pull request to fail consistently locally and on Travis.

testSqlExceptionIsHandled(io.dropwizard.hibernate.JerseyIntegrationTest)
javax.ws.rs.ProcessingException: Server-side request processing failed with an error.

@arteam

This comment has been minimized.

Member

arteam commented Dec 9, 2015

Looks like I mixed up this PR with another (#1361) in some time. Now the build should pass tests.

@nickbabcock

This comment has been minimized.

Contributor

nickbabcock commented Dec 9, 2015

I attempted to use this pull request with a CA cert, but there is a current problem on windows. Every test in Http2IntegrationTest fails with no valid keystore. I'm investigating, but debugging tls issues can sometimes be black magic 😛

Edit: The valid keystore error doesn't happen with just the CA cert, but with the test cert in the PR.

Additionally, HTTPCLIENT-1672 is tracking HTTP 2.0 status. Unfortunately, I doubt it will land anytime soon.

@nickbabcock

This comment has been minimized.

Contributor

nickbabcock commented Dec 9, 2015

The fix was to add another config override for the trust store path:

    @Rule
    public final DropwizardAppRule<Configuration> appRule = new DropwizardAppRule<>(
            FakeApplication.class, ResourceHelpers.resourceFilePath("test-http2.yml"),
            Optional.of("tls_http2"),
            ConfigOverride.config("tls_http2", "server.connector.keyStorePath",
                    ResourceHelpers.resourceFilePath("stores/http2_server.jks")),
            ConfigOverride.config("tls_http2", "server.connector.trustStorePath",
                ResourceHelpers.resourceFilePath("stores/http2_client.jts"))
    );
@nickbabcock

This comment has been minimized.

Contributor

nickbabcock commented Dec 9, 2015

Ok, I tested the branch (after I applied my small fix) with a CA cert and I was able to see in the request logs HTTP 2.0!

I'm not sure if you noticed this as well, but it looks like I had to supply at least one supportedCipherSuites in the yaml, else Chrome and Jetty couldn't negotiate. I'm curious if the HTTP 2.0 Jetty starts with zero supported cipher suites. If so, we'll have to document this and potentially provide a sane default.

Anyways, awesome work. As soon as I can compile/test on windows, this PR is good to merge 👍

@jplock

This comment has been minimized.

Member

jplock commented Dec 29, 2015

@arteam did you want to comment on @nickbabcock's comment above regarding supportedCipherSuites? Otherwise, we should get this merged in. 👍

@arteam

This comment has been minimized.

Member

arteam commented Dec 29, 2015

Yes, I wanted to look at this closely, but hadn't a chance. Was busy with work and holiday stuff. I am going to give my feedback today or tomorrow.

arteam added some commits May 8, 2015

Add dropwizard-http 2.0
The module provides the ability to configure access
to Dropwizard applications via HTTP/2 over TLS or
a plain text TCP connections.
Use instrumenting provider for Jetty 9.3
Dropwizard Metrics doesn't support Jetty 9.3.0, so we are forced
to use our own implementation of an insrumented connection factory.

Jetty Connection Factory public
Refer to HTTP/2 instead SPDY in dropwizard-example
SPDY is no longer relevant. We should encourage users to try HTTP/2.
Add HTTP/2 TLS Connection Factory
Add a connection factory for providing access to Dropwizard
applications via HTTP/2 with TLS. We should use ALPN for
connection negotiation and fallback to HTTPS, if the client
doesn't support HTTP/2.
Add HTTP/2 plain text connector factory
Add a connector that provides HTTP/2 connections
without SSL (h2c). A quite useful thing for development.
Add HTTP/2 with TLS integration test
Add a test for connecting to an HTTP/2 endpoint with configured TLS.
That's a bit tricky, because it requires providing an ALPN library in
bootpath, setting a keystore in the server side and a correct cipher
suite on the client side.
Add a test for plain HTTP 1.1 connection
We should test that we don't break old clients.
Add HTTPS via HTTP/2 endpoint integration test
Add a test that shows that old clients work via HTTP/1.1 over TLS.
Use ALPN version compatible with Travis CI
Also change the scope of alpn-boot to test.
We actually need to download the library to the local Maven repository,
because we use it in HTTP/2 integration tests.
Force using TLSv1.2 in tests
HTTP/2 works only with the latest version

arteam added some commits Nov 19, 2015

Support for HTTP/2 connector additional parameters
Add the ability to configure *maxConcurrentStreams* and
*initialStreamSendWindow*. Unfortunately, there is no way we can
reuse the code and docs  between connectors, so we just duplicate
them.
Add support for configuration of HTTP/2 server push.
HTTP/2 supports the server push technology. It affords to a server to
push to a client several additional resources along with a primary
resource. It's very beneficial for dynamic web pages that consists
of many resources apart from an HTML markup (JS libraries, CSS rules,
images, etc).

Jetty provides `ServerPushFilter`, which separates resources on
primary and secondary based on the `Referer` header. The referred
resource within the specified time frame are considered as secondary
to the referer. On the next request to the primary resource secondary
resources will be pushed on a client.
Set minimum and maximum limits on parameters
RFC 7540 advises to set `maxConcurrentStreams` no less then 100.
See https://tools.ietf.org/html/rfc7540#section-6.5.2
Add tests for many concurrent HTTP/2 requests
Check that server correctly and efficiently handles concurrent
requests over a single connection.
Force using supported cipher suites for HTTP/2
This is approved by Google the list of ciphers.
Use a high-level Jetty client for testing HTTP/2 API
Jetty 9 provides an HTTP client with a nice synchronous/asynchronous
API to which we could plug-in the HTTP/2 transport.

Using it makes tests more readable and convenient for maintaining.
Update HTTP/2 and server push documentation
Fix some spelling and linguistics problems.
Extract common code in HTTP/2 tests
Follow the DRY principle.
Force TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 cipher suite
HTTP/2 specification requires that deployments MUST support
TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 cipher suite.See
http://http2.github.io/http2-spec/index.html#rfc.section.9.2.2

We should follow this practice and forcing this suite by default
on the server side.
@arteam

This comment has been minimized.

Member

arteam commented Dec 30, 2015

Looks like that HTTP/2 spec requires the support of TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 cipher on the server side. I believe, this is the default cipher for modern browsers.

Therefore, I would argue that we should force it by default in the connector. Users, who want to allow other cipher suites, have the ability to set explicitly them in the configuration.

@arteam

This comment has been minimized.

Member

arteam commented Dec 30, 2015

@nickbabcock, could you try the new version? HTTP/2 support works for me with Firefox 43 on Linux, but I would glad to see how it's going for users in other operation systems.

@nickbabcock

This comment has been minimized.

Contributor

nickbabcock commented Dec 30, 2015

It doesn't look that you applied my suggested patch of:

The fix was to add another config override for the trust store path:

    @Rule
    public final DropwizardAppRule<Configuration> appRule = new DropwizardAppRule<>(
            FakeApplication.class, ResourceHelpers.resourceFilePath("test-http2.yml"),
            Optional.of("tls_http2"),
            ConfigOverride.config("tls_http2", "server.connector.keyStorePath",
                    ResourceHelpers.resourceFilePath("stores/http2_server.jks")),
            ConfigOverride.config("tls_http2", "server.connector.trustStorePath",
                ResourceHelpers.resourceFilePath("stores/http2_client.jts"))
    );

Any reason why not?

With the patch applied, HTTP/2 support works with Chrome on Windows.

I'll have to double check next week Monday that the cipher suite now works out of the box.

Use an OS independent truststore in HTTP/2 tests
`/etc/ssl/certs/java/cacerts` default Java truststore exists only
on Unix operation sustems. It doesn't exist on Windows. To run tests
on Windows correctly, we should explicitly point the server to a
valid truststore from the project's resources.
@arteam

This comment has been minimized.

Member

arteam commented Dec 31, 2015

Any reason why not?

Sorry, I misinterpeted your comment. I thought about it in the context: a HTTP/2 test requires providing a trust store (which had been already done), not in the context: the current HTTP/2 test provides an invalid truststore. You are absolutely right, /etc/ssl/certs/java/cacerts truststore exists only on Unix systems. I've applied your changes.

@nickbabcock

This comment has been minimized.

Contributor

nickbabcock commented Jan 2, 2016

Awesome, thank you 😎

nickbabcock added a commit that referenced this pull request Jan 2, 2016

@nickbabcock nickbabcock merged commit 3d78e02 into dropwizard:master Jan 2, 2016

@arteam

This comment has been minimized.

Member

arteam commented Jan 2, 2016

Great! Thanks for rigorous review and testing. Windows users will certainly appreciate it.

@arteam arteam deleted the arteam:dropwizard-http-2 branch Jan 24, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment