From 75ec01365f80a81c6f4d5a910ab853d634e4fbc6 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Filip=20Zr=C5=AFst?=
 <128540+f4z4on@users.noreply.github.com>
Date: Fri, 16 Jun 2023 18:06:47 +0200
Subject: [PATCH 1/2] Fix validation of source for spec.router.instances
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

This field is sourced from value “routerInstances” or
“router.instances”. If “router.instances” is set, we check if it is the
same as “routerInstances”.

We do this with “ne” Helm function. This function cannot compare
different types. This is is quite typical when mixing values from
different sources (for example default values.yaml file and command-line
arguments).

Perhaps more strangely, when values YAML file contains an integer value,
it is treated as float64 by Helm, so even when we use default
values.yaml and set “router.instances” on command line, we get a
strangely looking error message as a result of “printf” trying to print
float64 as int64 (on the lastest version Helm as of this
writing—3.12.1).

We solve both issues by converting both values to strings.
---
 helm/mysql-innodbcluster/templates/deployment_cluster.yaml | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/helm/mysql-innodbcluster/templates/deployment_cluster.yaml b/helm/mysql-innodbcluster/templates/deployment_cluster.yaml
index bcf150e..36289af 100644
--- a/helm/mysql-innodbcluster/templates/deployment_cluster.yaml
+++ b/helm/mysql-innodbcluster/templates/deployment_cluster.yaml
@@ -6,8 +6,8 @@
 {{- $imagePullPolicies := list "ifnotpresent" "always" "never" }}
 {{- $serverVersion := .Values.serverVersion | default .Chart.AppVersion }}
 {{- if and ((.Values).routerInstances) (((.Values).router).instances) }}
-  {{- if ne ((.Values).routerInstances) (((.Values).router).instances) }}
-    {{- $err := printf "routerInstances and router.instances both are specified and have different values %d and %d. Use only one" ((.Values).routerInstances) (((.Values).router).instances) }}
+  {{- if ne (toString ((.Values).routerInstances)) (toString (((.Values).router).instances)) }}
+    {{- $err := printf "routerInstances and router.instances both are specified and have different values %s and %s. Use only one" (toString ((.Values).routerInstances)) (toString (((.Values).router).instances)) }}
     {{- fail $err }}
   {{- end }}
 {{- end }}

From ebd3b80ee47ef98dd2d6ef787d3fa999b1cfb5ae Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Filip=20Zr=C5=AFst?=
 <128540+f4z4on@users.noreply.github.com>
Date: Fri, 16 Jun 2023 18:06:52 +0200
Subject: [PATCH 2/2] Allow setting spec.router.instances to zero
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

InnoDBCluster resource can have zero router instances. However, Helm
considers zero to be an empty value (see “default” function†), so we
cannot rely on “coalesce” Helm function to source spec.router.instances
from one of the following values:

1. routerInstances
2. router.instances

We change implementation which relies on Helm functions which use
“emptiness” to the implementation which checks if a value is defined in
parent dict. We do not introduce any additional validation like checking
if the value is an integer zero or any other value or type. This has
been validated by Kubernetes using CRD so far, Helm properly reports
this back, and we do not change this.

† https://helm.sh/docs/chart_template_guide/function_list/#default
---
 .../templates/deployment_cluster.yaml                 | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/helm/mysql-innodbcluster/templates/deployment_cluster.yaml b/helm/mysql-innodbcluster/templates/deployment_cluster.yaml
index 36289af..d5314ce 100644
--- a/helm/mysql-innodbcluster/templates/deployment_cluster.yaml
+++ b/helm/mysql-innodbcluster/templates/deployment_cluster.yaml
@@ -5,13 +5,12 @@
 {{- $forbiddenVersions := list "8.0.29" }}
 {{- $imagePullPolicies := list "ifnotpresent" "always" "never" }}
 {{- $serverVersion := .Values.serverVersion | default .Chart.AppVersion }}
-{{- if and ((.Values).routerInstances) (((.Values).router).instances) }}
+{{- if and (hasKey (.Values) "routerInstances") (hasKey ((.Values).router) "instances") }}
   {{- if ne (toString ((.Values).routerInstances)) (toString (((.Values).router).instances)) }}
     {{- $err := printf "routerInstances and router.instances both are specified and have different values %s and %s. Use only one" (toString ((.Values).routerInstances)) (toString (((.Values).router).instances)) }}
     {{- fail $err }}
   {{- end }}
 {{- end }}
-{{- $routerInstances := coalesce ((.Values).routerInstances) (((.Values).router).instances) }}
 {{- if lt $serverVersion $minimalVersion }}
   {{- $err := printf "It is not possible to use MySQL version %s . Please, use %s or above" $serverVersion $minimalVersion }}
   {{- fail $err }}
@@ -37,7 +36,13 @@ spec:
   instances: {{ required "serverInstances is required" .Values.serverInstances }}
   tlsUseSelfSigned: {{ $use_self_signed }}
   router:
-    instances: {{ required "router.instances is required" $routerInstances }}
+{{- if hasKey (.Values) "routerInstances" }}
+    instances: {{ .Values.routerInstances }}
+{{- else if hasKey ((.Values).router) "instances" }}
+    instances: {{ .Values.router.instances }}
+{{- else }}
+  {{- fail "router.instances is required" }}
+{{- end }}
 {{- if (((.Values).router).podSpec) }}
     podSpec:  {{ toYaml (((.Values).router).podSpec) | nindent 6 }}
 {{- end }}