Skip to content

Commit

Permalink
okhttp: Convert to internal ConnectionSpec eagerly
Browse files Browse the repository at this point in the history
This allows ProGuard to remove OkHttp's ConnectionSpec in most cases,
saving about 40 methods. The savings won't be realized until
DEFAULT_CONNECTION_SPEC is removed.
  • Loading branch information
ejona86 authored Mar 26, 2018
1 parent 6b753bd commit 25f3576
Show file tree
Hide file tree
Showing 3 changed files with 44 additions and 13 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@
import com.google.common.base.Throwables;
import com.google.protobuf.EmptyProtos.Empty;
import com.squareup.okhttp.ConnectionSpec;
import com.squareup.okhttp.TlsVersion;
import io.grpc.ManagedChannel;
import io.grpc.internal.AbstractServerImplBuilder;
import io.grpc.internal.GrpcUtil;
Expand Down Expand Up @@ -94,9 +93,8 @@ protected ManagedChannel createChannel() {
private OkHttpChannelBuilder createChannelBuilder() {
OkHttpChannelBuilder builder = OkHttpChannelBuilder.forAddress("localhost", getPort())
.maxInboundMessageSize(AbstractInteropTest.MAX_MESSAGE_SIZE)
.connectionSpec(new ConnectionSpec.Builder(OkHttpChannelBuilder.DEFAULT_CONNECTION_SPEC)
.connectionSpec(new ConnectionSpec.Builder(ConnectionSpec.MODERN_TLS)
.cipherSuites(TestUtils.preferredTestCiphers().toArray(new String[0]))
.tlsVersions(ConnectionSpec.MODERN_TLS.tlsVersions().toArray(new TlsVersion[0]))
.build())
.overrideAuthority(GrpcUtil.authorityFromHostAndPort(
TestUtils.TEST_SERVER_HOST, getPort()));
Expand Down
51 changes: 42 additions & 9 deletions okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,6 @@

import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Preconditions;
import com.squareup.okhttp.CipherSuite;
import com.squareup.okhttp.ConnectionSpec;
import com.squareup.okhttp.TlsVersion;
import io.grpc.Attributes;
import io.grpc.ExperimentalApi;
import io.grpc.Internal;
Expand All @@ -40,7 +37,10 @@
import io.grpc.internal.SharedResourceHolder;
import io.grpc.internal.SharedResourceHolder.Resource;
import io.grpc.internal.TransportTracer;
import io.grpc.okhttp.internal.CipherSuite;
import io.grpc.okhttp.internal.ConnectionSpec;
import io.grpc.okhttp.internal.Platform;
import io.grpc.okhttp.internal.TlsVersion;
import java.net.InetSocketAddress;
import java.net.SocketAddress;
import java.security.GeneralSecurityException;
Expand All @@ -62,7 +62,39 @@
public class OkHttpChannelBuilder extends
AbstractManagedChannelImplBuilder<OkHttpChannelBuilder> {

public static final ConnectionSpec DEFAULT_CONNECTION_SPEC =
/**
* ConnectionSpec closely matching the default configuration that could be used as a basis for
* modification.
*
* <p>Since this field is the only reference in gRPC to ConnectionSpec that may not be ProGuarded,
* we are removing the field to reduce method count. We've been unable to find any existing users
* of the field, and any such user would highly likely at least be changing the cipher suites,
* which is sort of the only part that's non-obvious. Any existing user should instead create
* their own spec from scratch or base it off ConnectionSpec.MODERN_TLS if believed to be
* necessary. If this was providing you with value and don't want to see it removed, open a GitHub
* issue to discuss keeping it.
*
* @deprecated Deemed of little benefit and users weren't using it. Just define one yourself
*/
@Deprecated
public static final com.squareup.okhttp.ConnectionSpec DEFAULT_CONNECTION_SPEC =
new com.squareup.okhttp.ConnectionSpec.Builder(com.squareup.okhttp.ConnectionSpec.MODERN_TLS)
.cipherSuites(
// The following items should be sync with Netty's Http2SecurityUtil.CIPHERS.
com.squareup.okhttp.CipherSuite.TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384,
com.squareup.okhttp.CipherSuite.TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256,
com.squareup.okhttp.CipherSuite.TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384,
com.squareup.okhttp.CipherSuite.TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256,
com.squareup.okhttp.CipherSuite.TLS_DHE_RSA_WITH_AES_128_GCM_SHA256,
com.squareup.okhttp.CipherSuite.TLS_DHE_DSS_WITH_AES_128_GCM_SHA256,
com.squareup.okhttp.CipherSuite.TLS_DHE_RSA_WITH_AES_256_GCM_SHA384,
com.squareup.okhttp.CipherSuite.TLS_DHE_DSS_WITH_AES_256_GCM_SHA384)
.tlsVersions(com.squareup.okhttp.TlsVersion.TLS_1_2)
.supportsTlsExtensions(true)
.build();

@VisibleForTesting
static final ConnectionSpec INTERNAL_DEFAULT_CONNECTION_SPEC =
new ConnectionSpec.Builder(ConnectionSpec.MODERN_TLS)
.cipherSuites(
// The following items should be sync with Netty's Http2SecurityUtil.CIPHERS.
Expand Down Expand Up @@ -110,7 +142,7 @@ public static OkHttpChannelBuilder forTarget(String target) {

private SSLSocketFactory sslSocketFactory;
private HostnameVerifier hostnameVerifier;
private ConnectionSpec connectionSpec = DEFAULT_CONNECTION_SPEC;
private ConnectionSpec connectionSpec = INTERNAL_DEFAULT_CONNECTION_SPEC;
private NegotiationType negotiationType = NegotiationType.TLS;
private long keepAliveTimeNanos = KEEPALIVE_TIME_NANOS_DISABLED;
private long keepAliveTimeoutNanos = DEFAULT_KEEPALIVE_TIMEOUT_NANOS;
Expand Down Expand Up @@ -272,17 +304,18 @@ public final OkHttpChannelBuilder hostnameVerifier(@Nullable HostnameVerifier ho
* For secure connection, provides a ConnectionSpec to specify Cipher suite and
* TLS versions.
*
* <p>By default {@link #DEFAULT_CONNECTION_SPEC} will be used.
* <p>By default a modern, HTTP/2-compatible spec will be used.
*
* <p>This method is only used when building a secure connection. For plaintext
* connection, use {@link #usePlaintext()} instead.
*
* @throws IllegalArgumentException
* If {@code connectionSpec} is not with TLS
*/
public final OkHttpChannelBuilder connectionSpec(ConnectionSpec connectionSpec) {
public final OkHttpChannelBuilder connectionSpec(
com.squareup.okhttp.ConnectionSpec connectionSpec) {
Preconditions.checkArgument(connectionSpec.isTls(), "plaintext ConnectionSpec is not accepted");
this.connectionSpec = connectionSpec;
this.connectionSpec = Utils.convertSpec(connectionSpec);
return this;
}

Expand Down Expand Up @@ -481,7 +514,7 @@ public void run() {
executor,
socketFactory,
hostnameVerifier,
Utils.convertSpec(connectionSpec),
connectionSpec,
maxMessageSize,
proxy,
tooManyPingsRunnable,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -228,7 +228,7 @@ public void testToString() throws Exception {
executor,
sslSocketFactory,
hostnameVerifier,
Utils.convertSpec(OkHttpChannelBuilder.DEFAULT_CONNECTION_SPEC),
OkHttpChannelBuilder.INTERNAL_DEFAULT_CONNECTION_SPEC,
DEFAULT_MAX_MESSAGE_SIZE,
NO_PROXY,
tooManyPingsRunnable,
Expand Down

0 comments on commit 25f3576

Please sign in to comment.