Fix separateGrpcIngress flag not working in flyte-binary helm chart (#…

…4946) * [BUG] separateGrpcIngress flag not working in flyte-binary helm chart Signed-off-by: Ryan Lo <[email protected]> * make helm Signed-off-by: Ryan Lo <[email protected]> * revise Signed-off-by: Ryan Lo <[email protected]> * separate service Signed-off-by: Ryan Lo <[email protected]> * make helm Signed-off-by: Ryan Lo <[email protected]> * update README.md Signed-off-by: Ryan Lo <[email protected]> * move paths in values.yaml to _helpers.tpl Signed-off-by: Ryan Lo <[email protected]> * remove anno Signed-off-by: Ryan Lo <[email protected]> * fix doc Signed-off-by: Ryan Lo <[email protected]> * set ingress.separateGrpcIngress true by default Signed-off-by: Ryan Lo <[email protected]> --------- Signed-off-by: Ryan Lo <[email protected]> Signed-off-by: Eduardo Apolinario <[email protected]> Co-authored-by: Eduardo Apolinario <[email protected]>
flyteorg · Mar 27, 2024 · c81133b · c81133b
1 parent 5de1a02
commit c81133b
Show file tree

Hide file tree

Showing 7 changed files with 70 additions and 85 deletions.
diff --git a/charts/flyte-binary/README.md b/charts/flyte-binary/README.md
@@ -138,6 +138,7 @@ Chart for basic single Flyte executable deployment
 | ingress.httpTls | list | `[]` |  |
 | ingress.ingressClassName | string | `""` |  |
 | ingress.labels | object | `{}` |  |
+| ingress.separateGrpcIngress | bool | `true` |  |
 | ingress.tls | list | `[]` |  |
 | nameOverride | string | `""` |  |
 | rbac.annotations | object | `{}` |  |

diff --git a/charts/flyte-binary/templates/_helpers.tpl b/charts/flyte-binary/templates/_helpers.tpl
@@ -176,6 +176,24 @@ Get the Flyte service GRPC port.
 {{- default 8089 .Values.service.ports.grpc -}}
 {{- end -}}
 
+{{/*
+Get the Flyte service GRPC paths.
+*/}}
+{{- define "flyte-binary.ingress.grpcPaths" -}}
+- /flyteidl.service.AdminService
+- /flyteidl.service.AdminService/*
+- /flyteidl.service.AuthMetadataService
+- /flyteidl.service.AuthMetadataService/
+- /flyteidl.service.DataProxyService
+- /flyteidl.service.DataProxyService/*
+- /flyteidl.service.IdentityService
+- /flyteidl.service.IdentityService/*
+- /flyteidl.service.SignalService
+- /flyteidl.service.SignalService/*
+- /grpc.health.v1.Health
+- /grpc.health.v1.Health/*
+{{- end -}}
+
 {{/*
 Get the Flyte agent service GRPC port.
 */}}

diff --git a/charts/flyte-binary/templates/ingress/grpc.yaml b/charts/flyte-binary/templates/ingress/grpc.yaml
@@ -1,4 +1,5 @@
-{{- if .Values.ingress.create }}
+{{- if and .Values.ingress.create .Values.ingress.separateGrpcIngress }}
+{{- $paths := (include "flyte-binary.ingress.grpcPaths" .) | fromYamlArray }}
 apiVersion: networking.k8s.io/v1
 kind: Ingress
 metadata:
@@ -38,90 +39,22 @@ spec:
       {{- if .Values.ingress.grpcExtraPaths.prepend }}
       {{- tpl ( .Values.ingress.grpcExtraPaths.prepend | toYaml ) . | nindent 6 }}
       {{- end }}
-      - backend:
-          service:
-            name: {{ include "flyte-binary.service.grpc.name" . }}
-            port:
-              number: {{ include "flyte-binary.service.grpc.port" . }}
-        path: /flyteidl.service.AdminService
-        pathType: ImplementationSpecific
-      - backend:
-          service:
-            name: {{ include "flyte-binary.service.grpc.name" . }}
-            port:
-              number: {{ include "flyte-binary.service.grpc.port" . }}
-        path: /flyteidl.service.AdminService/*
-        pathType: ImplementationSpecific
-      - backend:
-          service:
-            name: {{ include "flyte-binary.service.grpc.name" . }}
-            port:
-              number: {{ include "flyte-binary.service.grpc.port" . }}
-        path: /flyteidl.service.DataProxyService
-        pathType: ImplementationSpecific
-      - backend:
-          service:
-            name: {{ include "flyte-binary.service.grpc.name" . }}
-            port:
-              number: {{ include "flyte-binary.service.grpc.port" . }}
-        path: /flyteidl.service.DataProxyService/*
-        pathType: ImplementationSpecific
-      - backend:
-          service:
-            name: {{ include "flyte-binary.service.grpc.name" . }}
-            port:
-              number: {{ include "flyte-binary.service.grpc.port" . }}
-        path: /flyteidl.service.AuthMetadataService
-        pathType: ImplementationSpecific
-      - backend:
-          service:
-            name: {{ include "flyte-binary.service.grpc.name" . }}
-            port:
-              number: {{ include "flyte-binary.service.grpc.port" . }}
-        path: /flyteidl.service.AuthMetadataService/*
-        pathType: ImplementationSpecific
-      - backend:
-          service:
-            name: {{ include "flyte-binary.service.grpc.name" . }}
-            port:
-              number: {{ include "flyte-binary.service.grpc.port" . }}
-        path: /flyteidl.service.IdentityService
-        pathType: ImplementationSpecific
-      - backend:
-          service:
-            name: {{ include "flyte-binary.service.grpc.name" . }}
-            port:
-              number: {{ include "flyte-binary.service.grpc.port" . }}
-        path: /flyteidl.service.IdentityService/*
-        pathType: ImplementationSpecific
-      - backend:
-          service:
-            name: {{ include "flyte-binary.service.grpc.name" . }}
-            port:
-              number: {{ include "flyte-binary.service.grpc.port" . }}
-        path: /grpc.health.v1.Health
-        pathType: ImplementationSpecific
-      - backend:
-          service:
-            name: {{ include "flyte-binary.service.grpc.name" . }}
-            port:
-              number: {{ include "flyte-binary.service.grpc.port" . }}
-        path: /grpc.health.v1.Health/*
-        pathType: ImplementationSpecific
-      - backend:
-          service:
-            name: {{ include "flyte-binary.service.grpc.name" . }}
-            port:
-              number: {{ include "flyte-binary.service.grpc.port" . }}
-        path: /flyteidl.service.SignalService
-        pathType: ImplementationSpecific
-      - backend:
-          service:
-            name: {{ include "flyte-binary.service.grpc.name" . }}
-            port:
-              number: {{ include "flyte-binary.service.grpc.port" . }}
-        path: /flyteidl.service.SignalService/*
-        pathType: ImplementationSpecific
+      {{- range $path := $paths }}
+        - path: {{ $path }}
+          {{- if semverCompare ">=1.18-0" $.Capabilities.KubeVersion.GitVersion }}
+          pathType: ImplementationSpecific
+          {{- end }}
+          backend:
+            {{- if semverCompare ">=1.19-0" $.Capabilities.KubeVersion.GitVersion }}
+            service:
+              name: {{ include "flyte-binary.service.grpc.name" $ }}
+              port:
+                number: {{ include "flyte-binary.service.grpc.port" $ }}
+            {{- else }}
+            serviceName: {{ include "flyte-binary.service.grpc.name" $ }}
+            servicePort: {{ include "flyte-binary.service.grpc.port" $ }}
+            {{- end }}
+      {{- end }}
       {{- if .Values.ingress.grpcExtraPaths.append }}
       {{- tpl ( .Values.ingress.grpcExtraPaths.append | toYaml ) . | nindent 6 }}
       {{- end }}

diff --git a/charts/flyte-binary/templates/ingress/http.yaml b/charts/flyte-binary/templates/ingress/http.yaml
@@ -171,6 +171,25 @@ spec:
               number: {{ include "flyte-binary.service.http.port" . }}
         path: /oauth2/*
         pathType: ImplementationSpecific
+      {{- if not .Values.ingress.separateGrpcIngress }}
+      {{- $paths := (include "flyte-binary.ingress.grpcPaths" .) | fromYamlArray }}
+      {{- range $path := $paths }}
+      - path: {{ $path }}
+        {{- if semverCompare ">=1.18-0" $.Capabilities.KubeVersion.GitVersion }}
+        pathType: ImplementationSpecific
+        {{- end }}
+        backend:
+          {{- if semverCompare ">=1.19-0" $.Capabilities.KubeVersion.GitVersion }}
+          service:
+            name: {{ include "flyte-binary.service.http.name" $ }}
+            port:
+              number: {{ include "flyte-binary.service.grpc.port" $ }}
+          {{- else }}
+          serviceName: {{ include "flyte-binary.service.http.name" $ }}
+          servicePort: {{ include "flyte-binary.service.grpc.port" $ }}
+          {{- end }}
+      {{- end }}
+      {{- end }}
       {{- if .Values.ingress.httpExtraPaths.append }}
       {{- tpl ( .Values.ingress.httpExtraPaths.append | toYaml ) . | nindent 6 }}
       {{- end }}

diff --git a/charts/flyte-binary/templates/service/grpc.yaml b/charts/flyte-binary/templates/service/grpc.yaml
@@ -1,3 +1,4 @@
+{{- if .Values.ingress.separateGrpcIngress }}
 apiVersion: v1
 kind: Service
 metadata:
@@ -47,3 +48,4 @@ spec:
       nodePort: null
       {{- end }}
   selector: {{- include "flyte-binary.selectorLabels" . | nindent 4 }}
+{{- end }}
diff --git a/charts/flyte-binary/templates/service/http.yaml b/charts/flyte-binary/templates/service/http.yaml
@@ -46,6 +46,16 @@ spec:
       {{- else if eq .Values.service.type "ClusterIP" }}
       nodePort: null
       {{- end }}
+    {{- if not .Values.ingress.separateGrpcIngress }}
+    - 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 }}
+    {{- end }}
     {{- if .Values.service.extraPorts }}
     {{- tpl ( .Values.service.extraPorts | toYaml ) . | nindent 4 }}
     {{- end }}

diff --git a/charts/flyte-binary/values.yaml b/charts/flyte-binary/values.yaml
@@ -329,6 +329,8 @@ ingress:
   labels: {}
   # host Hostname to bind to ingress resources
   host: ""
+  # separateGrpcIngress Create a separate ingress resource for GRPC if true. Required for certain ingress controllers like nginx.
+  separateGrpcIngress: true
   # commonAnnotations Add common annotations to all ingress resources
   commonAnnotations: {}
   # httpAnnotations Add annotations to http ingress resource