[Feature] Set appProtocol correctly for head service

[neggert]

Search before asking

• [X] I had searched in the issues and found no similar feature requirement.

Description

#668 set the appProtocol to all ports in the head service to tcp. I believe the goal was to provide compatibility with OpenServiceMesh (see #625). While tcp basically works, we lose features that service meshes like Istio and (I assume) OSM can provide by not providing a more specific protocol. Aside from possibly the GCS port, these ports all use HTTP or gRPC. Is there any reason not to provide the correct protocol?

Background:

• Istio uses appProtocol to detect service protocol. It supports, http, tcp, grpc and a few other values.
  • TCP services cannot be routed on anything more complicated than port number
  • HTTP/HTTP2/GRPC services provide wealth of routing options
• I'm not familiar with OSM, but it looks like it also supports http, tcp, and grpc

Use case

I'm deploying Kuberay into a cluster that runs Istio. I'd like to be able to expose head services outside the cluster, ideally with routing based on hostname. For example, I'd like cluster1.ray.example.com to go to cluster1 and cluster2.ray.example.target.com to go to cluster2.

Related issues

#1025 notes that JWT RequestAuthentication in istio can only be applied to ports with appProtocol: http.

Are you willing to submit a PR?

• [X] Yes I am willing to submit a PR!

[neggert]

Ideally, the service should look like this (unsure about the gcs port):

  ports:
  - appProtocol: grpc
    name: client
    port: 10001
    protocol: TCP
    targetPort: 10001
  - appProtocol: http
    name: dashboard
    port: 8265
    protocol: TCP
    targetPort: 8265
  - appProtocol: tcp
    name: gcs
    port: 6379
    protocol: TCP
    targetPort: 6379
  - appProtocol: http
    name: metrics
    port: 8080
    protocol: TCP
    targetPort: 8080
  - appProtocol: http
    name: serve
    port: 8000
    protocol: TCP
    targetPort: 8000

If I manually patch the service after creation by the operator, Istio works as expected.

[neggert]

I was able to figure out a hacky workaround for this. We can add port definitions using the headService field. However, these ports get appended to what's in headGroupSpec.template.spec.containers[0].ports. If there's nothing in that list, default ports are populated (with the incorrect appProtocol), and will produce name conflicts if we try to override any of the default ports using headService. Furthermore, if headGroupSpec.template.spec.containers[0].ports doesn't contain a port named metrics, it's added by the operator.

So the solution is to set up headGroupSpec.template.spec.containers[0].ports to contain only the metrics port. This will prevent the operator from adding the default port definitions. Then we're free to redefine everything except the metrics port in headService. metrics is, unfortunately, stuck with the wrong appProtocol, but for my use case at least, I can live with that. You end up with somethign like this:

apiVersion: ray.io/v1alpha1
kind: RayCluster
metadata:
  name: ray-test
spec:
  ...
  headGroupSpec:
    ...
    headService:
      spec:
        ports:
          - appProtocol: grpc
            name: client
            port: 10001
            protocol: TCP
            targetPort: 10001
          - appProtocol: http
            name: dashboard
            port: 8265
            protocol: TCP
            targetPort: 8265
          - appProtocol: tcp
            name: gcs
            port: 6379
            protocol: TCP
            targetPort: 6379
          - appProtocol: http
            name: serve
            port: 8000
            protocol: TCP
            targetPort: 8000
    ...
    template:
      spec:
        containers:
          - name: ray-head
            ports:
              - name: metrics
                containerPort: 8080
            ...