feat(chart): Add default annotations for ingress nginx controller (#2047

)

Signed-off-by: Viet Nguyen Duc <nguyenducviet4496@gmail.com>

Video/Dockerfile

@@ -44,7 +44,7 @@ ENV DISPLAY_CONTAINER_NAME selenium
 ENV SE_SCREEN_WIDTH 1360
 ENV SE_SCREEN_HEIGHT 1020
 ENV SE_FRAME_RATE 15
-ENV SE_CODEC libx265
+ENV SE_CODEC libx264
 ENV SE_PRESET "-preset ultrafast"
 ENV FILE_NAME video.mp4
 

charts/selenium-grid/README.md

@@ -79,6 +79,54 @@ To uninstall:
 helm uninstall selenium-grid
 ```
 
+## Ingress Configuration
+
+By default, ingress is enabled without annotations set. If NGINX ingress controller is used, you need to set few annotations to override the default timeout values to avoid 504 errors (see #1808). Since in Selenium Grid the default of `SE_NODE_SESSION_TIMEOUT` and `SE_SESSION_REQUEST_TIMEOUT` is `300` seconds.
+
+In order to make user experience better, there are few annotations will be set by default if NGINX ingress controller is used. Mostly relates to timeouts and buffer sizes.
+
+If you are not using NGINX ingress controller, you can disable these default annotations by setting `ingress.nginx` to `nil` (aka null) via Helm CLI `--set ingress.nginx=null`) or via an override-values.yaml as below:
+
+```yaml
+ingress:
+  nginx:
+  # nginx: null (alternative way)
+```
+
+Similarly, if you want to disable a sub-config of `ingress.nginx`. For example: `--set ingress.nginx.proxyBuffer=null`)
+
+You are also able to combine using both default annotations and your own annotations in `ingress.annotations`. Duplicated keys will be merged strategy overwrite with your own annotations in `ingress.annotations` take precedence.
+
+```yaml
+ingress:
+  nginx:
+    proxyTimeout: 3600
+  annotations:
+    nginx.ingress.kubernetes.io/proxy-connect-timeout: "7200" # This key will take 7200 instead of 3600
+```
+
+List mapping of chart values and default annotation(s)
+
+```markdown
+# `ingress.nginx.proxyTimeout` pass value to annotation(s)
+nginx.ingress.kubernetes.io/proxy-connect-timeout
+nginx.ingress.kubernetes.io/proxy-send-timeout
+nginx.ingress.kubernetes.io/proxy-read-timeout
+nginx.ingress.kubernetes.io/proxy-next-upstream-timeout
+nginx.ingress.kubernetes.io/auth-keepalive-timeout
+
+# `ingress.nginx.proxyBuffer` pass value to to annotation(s)
+nginx.ingress.kubernetes.io/proxy-request-buffering: "on"
+nginx.ingress.kubernetes.io/proxy-buffering: "on"
+
+# `ingress.nginx.proxyBuffer.size` pass value to to annotation(s)
+nginx.ingress.kubernetes.io/proxy-buffer-size
+nginx.ingress.kubernetes.io/client-body-buffer-size
+
+# `ingress.nginx.proxyBuffer.number` pass value to annotation(s)
+nginx.ingress.kubernetes.io/proxy-buffers-number
+```
+
 ## Configuration
 
 For now, global configuration supported is:
@@ -111,6 +159,9 @@ This table contains the configuration parameters of the chart and their default
 | `ingress.enabled`                             | `true`                                      | Enable or disable ingress resource                                                                                         |
 | `ingress.className`                           | `""`                                        | Name of ingress class to select which controller will implement ingress resource                                           |
 | `ingress.annotations`                         | `{}`                                        | Custom annotations for ingress resource                                                                                    |
+| `ingress.nginx.proxyTimeout`                  | `3600`                                      | Value is used to set for NGINX ingress annotations related to proxy timeout                                                |
+| `ingress.nginx.proxyBuffer.size`              | `512M`                                      | Value is used to set for NGINX ingress annotations on size of the buffer proxy_buffer_size used for reading                |
+| `ingress.nginx.proxyBuffer.number`            | `4`                                         | Value is used to set for NGINX ingress annotations on number of the buffers in proxy_buffers used for reading              |
 | `ingress.hostname`                            | ``                                          | Default host for the ingress resource                                                                                      |
 | `ingress.path`                                | `/`                                         | Default host path for the ingress resource                                                                                 |
 | `ingress.pathType`                            | `Prefix`                                    | Default path type for the ingress resource                                                                                 |

charts/selenium-grid/TESTING.md

@@ -16,6 +16,7 @@ All related testing to this helm chart will be documented in this file.
 | Ingress                | Ingress is enabled without `hostname`                                | ✓  | Cluster  |
 |                        | Ingress is enabled with `hostname` is set                            | ✗  |          |
 |                        | Hub `sub-path` is set with Ingress `ImplementationSpecific` paths    | ✓  | Cluster  |
+|                        | `ingress.nginx` configs for NGINX ingress controller annotations     | ✓  | Template |
 | Distributed components | `isolateComponents` is enabled                                       | ✓  | Cluster  |
 |                        | `isolateComponents` is disabled                                      | ✓  | Cluster  |
 | Browser Nodes          | Node `nameOverride` is set                                           | ✓  | Cluster  |

charts/selenium-grid/templates/_helpers.tpl

@@ -80,6 +80,29 @@ Ingress fullname
 {{- default "selenium-ingress" .Values.ingress.nameOverride | trunc 63 | trimSuffix "-" -}}
 {{- end -}}
 
+{{- define "seleniumGrid.ingress.nginx.annotations.default" -}}
+{{- with .Values.ingress.nginx }}
+  {{- with .proxyTimeout }}
+nginx.ingress.kubernetes.io/proxy-connect-timeout: {{ . | quote }}
+nginx.ingress.kubernetes.io/proxy-send-timeout: {{ . | quote }}
+nginx.ingress.kubernetes.io/proxy-read-timeout: {{ . | quote }}
+nginx.ingress.kubernetes.io/proxy-next-upstream-timeout: {{ . | quote }}
+nginx.ingress.kubernetes.io/auth-keepalive-timeout: {{ . | quote }}
+  {{- end }}
+  {{- with .proxyBuffer }}
+nginx.ingress.kubernetes.io/proxy-request-buffering: "on"
+nginx.ingress.kubernetes.io/proxy-buffering: "on"
+    {{- with .size }}
+nginx.ingress.kubernetes.io/proxy-buffer-size: {{ . | quote }}
+nginx.ingress.kubernetes.io/client-body-buffer-size: {{ . | quote }}
+    {{- end }}
+    {{- with .number }}
+nginx.ingress.kubernetes.io/proxy-buffers-number: {{ . | quote }}
+    {{- end }}
+  {{- end }}
+{{- end }}
+{{- end -}}
+
 {{/*
 Service Account fullname
 */}}

charts/selenium-grid/templates/ingress.yaml

@@ -20,9 +20,13 @@ metadata:
     {{- with .Values.customLabels }}
       {{- toYaml . | nindent 4 }}
     {{- end }}
-  {{- with .Values.ingress.annotations }}
+  {{- $ingressAnnotations := (include "seleniumGrid.ingress.nginx.annotations.default" . | toString | fromYaml ) }}
+  {{- with .Values.ingress.annotations -}}
+    {{- $ingressAnnotations = mergeOverwrite $ingressAnnotations . }}
+  {{- end }}
+  {{- if not (empty $ingressAnnotations) }}
   annotations:
-    {{- toYaml . | nindent 4 }}
+    {{- $ingressAnnotations | toYaml | nindent 4 }}
   {{- end }}
 spec:
   {{- if and .Values.ingress.className (semverCompare ">=1.18-0" .Capabilities.KubeVersion.GitVersion) }}

charts/selenium-grid/values.yaml

@@ -36,6 +36,12 @@ ingress:
   enabled: true
   # Name of ingress class to select which controller will implement ingress resource
   className: ""
+  # Refer to list nginx annotations: https://github.com/kubernetes/ingress-nginx/blob/main/docs/user-guide/nginx-configuration/annotations.md#annotations
+  nginx:
+    proxyTimeout: 3600
+    proxyBuffer:
+      size: 512M
+      number: 4
   # Custom annotations for ingress resource
   annotations: {}
   # Default host for the ingress resource

tests/charts/templates/render/dummy.yaml

@@ -11,6 +11,34 @@ global:
                   values:
                     - selenium
             topologyKey: "kubernetes.io/hostname"
+ingress:
+  nginx:
+    proxyTimeout: 360 # Set different proxy timout
+    proxyBuffer:
+      # size: 512M # Keep using sub-config default
+      number: # Disable sub-config
+  annotations: # Add you own annotations
+    nginx.ingress.kubernetes.io/use-regex: "true" # Add new key
+    nginx.ingress.kubernetes.io/rewrite-target: /$2
+    nginx.ingress.kubernetes.io/app-root: &gridAppRoot "/selenium"
+    nginx.ingress.kubernetes.io/proxy-connect-timeout: "3600" # Override default key
+    nginx.ingress.kubernetes.io/proxy-send-timeout: "3600" # Override default key
+  hostname: ""
+  paths:
+    - path: /selenium(/|$)(.*)
+      pathType: ImplementationSpecific
+      backend:
+        service:
+          name: '{{ template "seleniumGrid.router.fullname" $ }}'
+          port:
+            number: 4444
+    - path: /(/?)(session/.*/se/vnc)
+      pathType: ImplementationSpecific
+      backend:
+        service:
+          name: '{{ template "seleniumGrid.router.fullname" $ }}'
+          port:
+            number: 4444
 
 isolateComponents: true
 

tests/charts/templates/test.py

@@ -20,12 +20,31 @@ def test_set_affinity(self):
         resources_name = ['selenium-chrome-node', 'selenium-distributor', 'selenium-edge-node', 'selenium-firefox-node',
                 'selenium-event-bus', 'selenium-router', 'selenium-session-map', 'selenium-session-queue']
         count = 0
+        logger.info(f"Assert affinity is set in global and nodes")
         for doc in LIST_OF_DOCUMENTS:
             if doc['metadata']['name'] in resources_name and doc['kind'] == 'Deployment':
+                logger.info(f"Assert affinity is set in resource {doc['metadata']['name']}")
                 self.assertTrue(doc['spec']['template']['spec']['affinity']['podAffinity']['requiredDuringSchedulingIgnoredDuringExecution'][0]['labelSelector']['matchExpressions'] is not None)
                 count += 1
         self.assertEqual(count, len(resources_name), "Not all resources have affinity set")
 
+    def test_ingress_nginx_annotations(self):
+        resources_name = ['selenium-ingress']
+        count = 0
+        for doc in LIST_OF_DOCUMENTS:
+            if doc['metadata']['name'] in resources_name and doc['kind'] == 'Ingress':
+                logger.info(f"Assert ingress ingress annotations")
+                logger.info(f"Config `ingress.nginx.proxyTimeout` is able to be set a different value")
+                self.assertTrue(doc['metadata']['annotations']['nginx.ingress.kubernetes.io/proxy-read-timeout'] == '360')
+                logger.info(f"Duplicated in `ingress.annotations` take precedence to overwrite the default value")
+                self.assertTrue(doc['metadata']['annotations']['nginx.ingress.kubernetes.io/proxy-connect-timeout'] == '3600')
+                logger.info(f"Default annotation is able to be disabled by setting it to null")
+                self.assertTrue(doc['metadata']['annotations'].get('nginx.ingress.kubernetes.io/proxy-buffers-number')  is None)
+                logger.info(f"Default annotation is added if no override value")
+                self.assertTrue(doc['metadata']['annotations']['nginx.ingress.kubernetes.io/client-body-buffer-size']  == '512M')
+                count += 1
+        self.assertEqual(count, len(resources_name), "No ingress resources found")
+
 if __name__ == '__main__':
     failed = False
     try: