Installing the stack using helm fails: ’,’ should be escaped #29

mgrzybek · 2023-01-18T10:39:30Z

Expected Behaviour

The "TL;DR" provided there https://github.com/tinkerbell/charts/blob/main/tinkerbell/stack/README.md#tldr does not work.

Current Behaviour

The given shell command to create the "trusted_proxies" string does not escape the commas.

Possible Solution

The last tr command used in kubectl get nodes -o jsonpath='{.items[*].spec.podCIDR}' | tr ' ' ',' should escape the commas.

trusted_proxies=$(kubectl get nodes -o jsonpath='{.items[*].spec.podCIDR}' | tr ' ' '\\,')

Steps to Reproduce (for bugs)

helm dependency build stack/
trusted_proxies=$(kubectl get nodes -o jsonpath='{.items[*].spec.podCIDR}' | tr ' ' ',')
helm install stack-release stack/ --create-namespace --namespace tink-system --wait --set "boots.trustedProxies=${trusted_proxies}" --set "hegel.trustedProxies=${trusted_proxies}"

The result:

Error: INSTALLATION FAILED: failed parsing --set data: key "0/24" has no value (cannot end with ,)

Context

Your Environment

$ helm version
helm versionversion.BuildInfo{Version:"v3.7.0", GitCommit:"eeac83883cb4014fe60267ec6373570374ce770b", GitTreeState:"clean", GoVersion:"go1.16.8"}

$ kubectl version --short
Flag --short has been deprecated, and will be removed in the future. The --short output will become the default.
Client Version: v1.26.0
Kustomize Version: v4.5.7
Server Version: v1.26.0

The text was updated successfully, but these errors were encountered:

chrisdoherty4 · 2023-01-18T13:16:30Z

Hi @mgrzybek, thanks for raising the issue. Could you share the output of the kubectl command to get podCIDR?

mgrzybek · 2023-01-18T13:37:35Z

Hi,

The output is:

$ kubectl get nodes -o jsonpath='{.items[*].spec.podCIDR}' | tr ' ' ','
10.244.3.0/24,10.244.2.0/24,10.244.5.0/24,10.244.0.0/24,10.244.1.0/24,10.244.4.0/24

Output of my script:

$ make deploy 
kubectl apply -f tink-ns.yml
namespace/tink-system unchanged
cd charts/tinkerbell && helm dependency build stack/
Saving 4 charts
Deleting outdated charts
cd charts/tinkerbell && helm install stack-release stack/ \
        --namespace tink-system \
        --wait \
        --set "boots.trustedProxies=10.244.3.0/24,10.244.2.0/24,10.244.5.0/24,10.244.0.0/24,10.244.1.0/24,10.244.4.0/24" \
        --set "hegel.trustedProxies=10.244.3.0/24,10.244.2.0/24,10.244.5.0/24,10.244.0.0/24,10.244.1.0/24,10.244.4.0/24"
Error: INSTALLATION FAILED: failed parsing --set data: key "0/24" has no value (cannot end with ,)
make: *** [Makefile:16: deploy] Error 1
$

chrisdoherty4 · 2023-01-18T13:51:48Z

TIL you can set multiple values with --set.

chrisdoherty4 · 2023-01-18T13:53:28Z

@mgrzybek the fix you raised looks good if you'd like to re-open.

mgrzybek · 2023-01-18T14:47:44Z

I think my patch was wrong. The commas were not escaped by Helm interpretor.

According to https://stackoverflow.com/questions/48316330/how-to-set-multiple-values-with-helm the right way to write comma-separated lists should look like:

helm install stack-release stack/ --namespace tink-system --wait \
--set "boots.trustedProxies={10.244.3.0/24,10.244.2.0/24}" \
--set "hegel.trustedProxies={10.244.3.0/24,10.244.2.0/24}"

mgrzybek · 2023-01-18T15:58:26Z

I managed to make it work using "join" in deployment.yaml

$ git diff
diff --git a/tinkerbell/boots/templates/deployment.yaml b/tinkerbell/boots/templates/deployment.yaml
index 07ba8e2..8b046de 100644
--- a/tinkerbell/boots/templates/deployment.yaml
+++ b/tinkerbell/boots/templates/deployment.yaml
@@ -37,7 +37,7 @@ spec:
           {{- end }}
           env:
             - name: TRUSTED_PROXIES
-              value: {{ required "missing trustedProxies" .Values.trustedProxies | quote }}
+              value: {{ join "," .Values.trustedProxies }}
             {{- range $i, $env := .Values.env }}
             - name: {{ $env.name | quote }}
               value: {{ $env.value | quote }}
diff --git a/tinkerbell/hegel/templates/deployment.yaml b/tinkerbell/hegel/templates/deployment.yaml
index 81432b6..eb69e4e 100644
--- a/tinkerbell/hegel/templates/deployment.yaml
+++ b/tinkerbell/hegel/templates/deployment.yaml
@@ -33,7 +33,7 @@ spec:
           {{- end }}
           env:
             - name: HEGEL_TRUSTED_PROXIES
-              value: {{ required "missing trustedProxies" .Values.trustedProxies | quote }}
+              value: {{ join "," .Values.trustedProxies }}
             {{- range $i, $env := .Values.env }}
             - name: {{ $env.name | quote }}
               value: {{ $env.value | quote }}
diff --git a/tinkerbell/hegel/values.yaml b/tinkerbell/hegel/values.yaml
index ae6d2b6..1af08fa 100644
--- a/tinkerbell/hegel/values.yaml
+++ b/tinkerbell/hegel/values.yaml
@@ -1,5 +1,5 @@
 deploy: true
-trustedProxies: ""
+trustedProxies: []
 name: hegel
 image: quay.io/tinkerbell/hegel:v0.10.1
 imagePullPolicy: IfNotPresent
$

What do you think?

chrisdoherty4 · 2023-01-18T16:21:52Z

I think we can change the proxies to arrays, that aligns with the direction we want to take the charts anyway.

Could you also update Hegel values and Boots values to reflect them being arrays (just swap out the "" for []).

I think we also still want the required piece. If you can't use something like require ... | join "," .Values.trustedProxies you can add {{- $_ := required ... }} above the TRUSTED_PROXIES env definition.

jacobweinstock · 2023-01-23T23:49:13Z

Hey @mgrzybek, thanks for catching and reporting this! I'm able to get everything deploy successfully without any code changes by using this trusted_proxies=$(kubectl get nodes -o jsonpath='{.items[*].spec.podCIDR}' | sed 's/ /\\,/g') instead of this trusted_proxies=$(kubectl get nodes -o jsonpath='{.items[*].spec.podCIDR}' | tr ' ' ',').

It seems like we could just update the docs to resolve this?
CC @chrisdoherty4

chrisdoherty4 · 2023-01-24T00:42:00Z

@jacobweinstock We can update the docs instead. I still like the idea of transitioning to an array though. It removes the need for the consumer to care about argument formatting and instead focus on formatting YAML, the document they're adjusting. I think this might go hand in hand with some of the other changes we spoke about regarding exposure of configuration in values YAML. What do you think?

jacobweinstock · 2023-01-24T00:52:27Z

Ah yeah, true. An array does feel like it improves the understand-ability.

mgrzybek · 2023-01-24T15:24:13Z

The PR has been closed. Another solution is on the way.
I guess I can wait for #32 to be merged to close the issue.

chrisdoherty4 · 2023-02-07T19:33:24Z

@mgrzybek With #34 and #32 this should be patched. Let us know if you have any issues.

We found a pattern that works well and is proven out in Boots. This aligns trusted proxy configuration with the approach in Hegel. Closes #29

displague · 2023-04-30T13:59:30Z

Following the docs, I ran into this again:

$ helm version
version.BuildInfo{Version:"v3.10.1", GitCommit:"9f88ccb6aee40b9a0535fcc7efea6055e1ef72c9", GitTreeState:"clean", GoVersion:"go1.18.7"}

$ trusted_proxies=$(kubectl get nodes -o jsonpath='{.items[*].spec.podCIDR}' | tr ' ' ',')
$ ~/src/tinkerbell/charts/tinkerbell/main$ echo xxx${trusted_proxies}xxx 
xxx10.42.0.0/24,10.42.3.0/24,10.42.1.0/24xxx
$ helm install stack-release stack/ --create-namespace --namespace tink-system --wait --set "boots.trustedProxies=${trusted_proxies}" --set "hegel.trustedProxies=${trusted_proxies}"
Error: INSTALLATION FAILED: failed parsing --set data: key "0/24" has no value (cannot end with ,)

Changing to the alternate syntax @jacobweinstock suggested:

$ trusted_proxies=$(kubectl get nodes -o jsonpath='{.items[*].spec.podCIDR}' | sed 's/ /\\,/g')
$ echo xxx${trusted_proxies}xxx
xxx10.42.1.0/24\,10.42.0.0/24\,10.42.3.0/24xxx
$ helm install stack-release stack/ --create-namespace --namespace tink-system --wait --set "boots.trustedProxies=${trusted_proxies}" --set "hegel.trustedProxies=${trusted_proxies}"
Error: INSTALLATION FAILED: timed out waiting for the condition

Now that's better :-)

rufio failed with exec /manager: exec format error
tink-stack-relay with

  Normal   Started    45m (x4 over 46m)     kubelet            Started container macvlan-interface
  Normal   Pulling    44m (x5 over 46m)     kubelet            Pulling image "alpine"
  Normal   Pulled     44m                   kubelet            Successfully pulled image "alpine" in 724.920905ms (724.995317ms including waiting)
  Normal   Created    44m (x5 over 46m)     kubelet            Created container macvlan-interface
  Warning  BackOff    104s (x205 over 45m)  kubelet            Back-off restarting failed container macvlan-interface in pod tink-stack-relay-7f9b46b754-4pt6j_tink-system(b33e67bf-f6b1-4063-9678-7c311ef95b21)

mgrzybek mentioned this issue Jan 18, 2023

[readme] escape the commas using tr #30

Closed

3 tasks

mgrzybek mentioned this issue Jan 19, 2023

Use trustedProxies values as arrays #31

Closed

3 tasks

chrisdoherty4 mentioned this issue Feb 7, 2023

Use a list for Hegel trusted proxy configuration #34

Merged

mergify bot closed this as completed in #34 Feb 7, 2023

mergify bot added a commit that referenced this issue Feb 7, 2023

Use a list for Hegel trusted proxy configuration (#34)

f72bb4d

We found a pattern that works well and is proven out in Boots. This aligns trusted proxy configuration with the approach in Hegel. Closes #29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Installing the stack using helm fails: ’,’ should be escaped #29

Installing the stack using helm fails: ’,’ should be escaped #29

mgrzybek commented Jan 18, 2023

chrisdoherty4 commented Jan 18, 2023

mgrzybek commented Jan 18, 2023

chrisdoherty4 commented Jan 18, 2023

chrisdoherty4 commented Jan 18, 2023

mgrzybek commented Jan 18, 2023

mgrzybek commented Jan 18, 2023

chrisdoherty4 commented Jan 18, 2023 •

edited

Loading

jacobweinstock commented Jan 23, 2023 •

edited

Loading

chrisdoherty4 commented Jan 24, 2023 •

edited

Loading

jacobweinstock commented Jan 24, 2023

mgrzybek commented Jan 24, 2023

chrisdoherty4 commented Feb 7, 2023 •

edited

Loading

displague commented Apr 30, 2023 •

edited

Loading

Installing the stack using helm fails: ’,’ should be escaped #29

Installing the stack using helm fails: ’,’ should be escaped #29

Comments

mgrzybek commented Jan 18, 2023

Expected Behaviour

Current Behaviour

Possible Solution

Steps to Reproduce (for bugs)

Context

Your Environment

chrisdoherty4 commented Jan 18, 2023

mgrzybek commented Jan 18, 2023

chrisdoherty4 commented Jan 18, 2023

chrisdoherty4 commented Jan 18, 2023

mgrzybek commented Jan 18, 2023

mgrzybek commented Jan 18, 2023

chrisdoherty4 commented Jan 18, 2023 • edited Loading

jacobweinstock commented Jan 23, 2023 • edited Loading

chrisdoherty4 commented Jan 24, 2023 • edited Loading

jacobweinstock commented Jan 24, 2023

mgrzybek commented Jan 24, 2023

chrisdoherty4 commented Feb 7, 2023 • edited Loading

displague commented Apr 30, 2023 • edited Loading

chrisdoherty4 commented Jan 18, 2023 •

edited

Loading

jacobweinstock commented Jan 23, 2023 •

edited

Loading

chrisdoherty4 commented Jan 24, 2023 •

edited

Loading

chrisdoherty4 commented Feb 7, 2023 •

edited

Loading

displague commented Apr 30, 2023 •

edited

Loading