seldon-core icon indicating copy to clipboard operation
seldon-core copied to clipboard

Seldon executor gRPC server with TLS doesn't support ALPN negotiation

Open phrabec opened this issue 3 years ago • 1 comments

Describe the bug

When gRPC protocol is used for the invocation of Seldon executor together with TLS, it doesn't work as expected due to missing support for ALPN during TLS handshake. The gRPC specification requires the use of HTTP/2 as a transport protocol. Protocol negotiation is normally performed using the ALPN extension of the TLS protocol. The issue is that the Seldon executor doesn't offer HTTP/2 (h2) during the TLS handshake. This prevents standard-compliant HTTP clients to be used for gRPC transport. One example of such a library is the official gRPC library for Java.

To reproduce

  1. Create a Seldon deployment with TLS enabled gRPC server
  2. Try to connect with a gRPC library or OpenSSL's s_client
apiVersion: machinelearning.seldon.io/v1alpha2
kind: SeldonDeployment
metadata:
  name: model
  namespace: test
spec:
  protocol: kfserving
  predictors:
    - graph:
      ...
      name: default
      ssl:
        certSecretName: model-cert
      ...
  1. Observe failure during HTTP/2 protocol negotiation
openssl s_client -connect localhost:5000 -alpn h2

with result

...
No ALPN negotiated
SSL-Session:
    Protocol  : TLSv1.2
...

Expected behavior

TLS ALPN negotiation with Seldon executor successfully finished with HTTP/2 (h2)

openssl s_client -connect localhost:5000 -alpn h2

with result

...
ALPN protocol: h2
SSL-Session:
    Protocol  : TLSv1.2
...

Environment

  • Cloud Provider: Own K8s cluster
  • Kubernetes Cluster Version: v1.19.16
  • Deployed Seldon System Images:
value: docker.io/seldonio/engine:1.10.0
value: docker.io/seldonio/seldon-core-executor:1.10.0
image: docker.io/seldonio/seldon-core-operator:1.10.0

Other Details

The issue seems to be caused by the missing NextProtos attribute when initializing the TLS listener in the Seldon executor. The field should be initialized with at least h2 (HTTP/2). From the observation of the source code, it seems the issue also manifests in the latest version in master. Link.

phrabec avatar Apr 08 '22 12:04 phrabec

Thanks @phrabec for this. Can to provide a suggested update and a possible PR to fix this?

ukclivecox avatar Apr 08 '22 13:04 ukclivecox

We would support this in v2 where we use envoy directly. Can you open a new issue for v2 if still interested.

ukclivecox avatar Dec 19 '22 11:12 ukclivecox