NPE from OkHttpChannelBuilder with ConnectionSpec.CLEARTEXT

[justinsb]

I'm getting a NPE when I use OkHttpChannelBuilder with ConnectionSpec.CLEARTEXT. I might be misusing the API, but I think it's actually an issue.

    ConnectionSpec connectionSpec = new ConnectionSpec.Builder(ConnectionSpec.CLEARTEXT).build();

    ManagedChannelImpl channel = OkHttpChannelBuilder.forAddress(host, port).connectionSpec(connectionSpec).build();

The problem is that Utils.convertSpec is calling spec.tlsVersions:

static ConnectionSpec convertSpec(com.squareup.okhttp.ConnectionSpec spec) {
    List<com.squareup.okhttp.TlsVersion> tlsVersionList = spec.tlsVersions();

And spec.tlsVersions assumes tlsVersions is non-null:

  public List<TlsVersion> tlsVersions() {
    TlsVersion[] result = new TlsVersion[tlsVersions.length];

But tlsVersions is always null (and enforced to be so) if not using TLS.

If this isn't me doing something wrong, I'm happy to send a patch. I guess the patch should be to check whether TLS is enabled before calling tlsVersions, in Utils.

[ejona86]

We are generally expecting you to call usePlaintext(true) (true doesn't do Upgrade request, which is all we support for the moment on OkHttp).

It looks like we could call ConnectionSpec.isTls() and automatically swap to the plaintext Negotiator in that case. It's unclear whether that would imply doing the Upgrade request.

But I'm not sure if we should. We've tried to make it very obvious when you are disabling security. Even if we don't support it we should throw a better exception.

[justinsb]

This workaround works great, and I think it's reasonable to make people very aware when they are not using TLS.

For the benefit of others, the call looks like ManagedChannelImpl channel = OkHttpChannelBuilder.forAddress(host, port).usePlaintext(true).build();

(and for netty it is NettyChannelBuilder.forAddress(host,port).negotiationType(NegotiationType.PLAINTEXT).build();)

I think we can close this, unless you want to do more here.

[ejona86]

Okay. Let's leave things the way they are except improve the exception. A NPE isn't great. Let's have this issue be to have a clear error message.