[BUG] separateGrpcIngress flag not working in flyte-binary helm chart

[spacepirate0001]

Describe the bug

separateGrpcIngress: false flag should force flyte-binary chart to create one ingress service for http and grpc but the condition is included in flyte-core only. Also flyte-flyte-http and flyte-flyte-grpc services should follow the same behavior to simplify http/grpc connections.

Expected behavior

separateGrpcIngress: false flag should be included in flyte-binary chart and condition reflected on yaml files.

Additional context to reproduce

No response

Screenshots

No response

Are you sure this issue hasn't been raised already?

• [X] Yes

Have you read the Code of Conduct?

• [X] Yes

[welcome[bot]]

Thank you for opening your first issue here! 🛠

[dosubot[bot]]

🤖

Hey there @spacepirate0001! Great to see you navigating the cosmic waves of Flyte again. 🚀

Thank you for bringing this to our attention. It seems like the separateGrpcIngress: false flag is not currently included in the flyte-binary helm chart. This flag should indeed allow the creation of a single ingress service for both http and grpc when set to false.

To achieve this, the service specification in the yaml file can be modified to include conditions that check the value of separateGrpcIngress. Here is an example of how you can modify the yaml file:

apiVersion: v1
kind: Service
metadata:
  name: {{ include "flyte-binary.service.grpc.name" . }}
  namespace: {{ .Release.Namespace | quote }}
  labels: {{- include "flyte-binary.labels" . | nindent 4 }}
    {{- if .Values.commonLabels }}
    {{- tpl ( .Values.commonLabels | toYaml ) . | nindent 4 }}
    {{- end }}
    {{- if .Values.service.labels }}
    {{- tpl ( .Values.service.labels | toYaml ) . | nindent 4 }}
    {{- end }}
  annotations:
    {{- if .Values.commonAnnotations }}
    {{- tpl ( .Values.commonAnnotations | toYaml ) . | nindent 4 }}
    {{- end }}
    {{- if .Values.service.annotations }}
    {{- tpl ( .Values.service.annotations | toYaml ) . | nindent 4 }}
    {{- end }}
    {{- if .Values.service.commonAnnotations }}
    {{- tpl ( .Values.service.commonAnnotations | toYaml ) . | nindent 4 }}
    {{- end }}
    {{- if .Values.service.grpcAnnotations }}
    {{- tpl ( .Values.service.grpcAnnotations | toYaml ) . | nindent 4 }}
    {{- end }}
spec:
  type: {{ .Values.service.type }}
  {{- if or (eq .Values.service.type "LoadBalancer") (eq .Values.service.type "NodePort") }}
  externalTrafficPolicy: {{ .Values.service.externalTrafficPolicy | quote }}
  {{- end }}
  {{- if and (eq .Values.service.type "LoadBalancer") (not (empty .Values.service.loadBalancerSourceRanges)) }}
  loadBalancerSourceRanges: {{ .Values.service.loadBalancerSourceRanges }}
  {{- end }}
  {{- if and (eq .Values.service.type "LoadBalancer") (not (empty .Values.service.loadBalancerIP)) }}
  loadBalancerIP: {{ .Values.service.loadBalancerIP }}
  {{- end }}
  {{- if and .Values.service.clusterIP (eq .Values.service.type "ClusterIP") }}
  clusterIP: {{ .Values.service.clusterIP }}
  {{- end }}
  ports:
    {{- if eq .Values.separateGrpcIngress false }}
    - name: http
      port: {{ include "flyte-binary.service.http.port" . }}
      targetPort: http
    {{- end }}
    - name: grpc
      port: {{ include "flyte-binary.service.grpc.port" . }}
      targetPort: grpc
      {{- if and (or (eq .Values.service.type "NodePort") (eq .Values.service.type "LoadBalancer")) (not (empty .Values.service.nodePorts.grpc)) }}
      nodePort: {{ .Values.service.nodePorts.grpc }}
{{- else if eq .Values.service.type "ClusterIP" }}
      nodePort: null
      {{- end }}
  selector: {{- include "flyte-binary.selectorLabels" . | nindent 4 }}

In this modified yaml file, the service will include an http port if separateGrpcIngress is set to false. This allows the service to handle both http and grpc connections through a single ingress.

Please note that this is a suggested modification and might need further testing to ensure it works as expected in your specific setup.

Sources

• charts/flyte-binary/templates/service/grpc.yaml
• charts/flyte-binary/templates/service/http.yaml

About Dosu

This response is meant to be useful and save you time. It is not meant to be a precise solution, but rather a starting point for your own research.

Help me be more useful! Please leave a 👍 if this is helpful and 👎 if it is irrelevant.

If you want to continue the conversation, start your reply with @dosu-bot.

[spacepirate0001]

The following two services should also be combined into one. flyte-flyte-binary-grpc Node Port <IP_ADDRESS>:8089 TCP flyte-flyte-binary-http Node Port <IP_ADDRESS>:8088 TCP

[eapolinario]

@spacepirate0001 , would you be willing to contribute this change?

[lowc1012]

Hi @eapolinario, I'd like to take this one, can you assign it to me if the reporter doesn't respond

[lowc1012]

#take