disabling TLS support in the chart is tricky

[bergerx]

We previously discussed about using the tls field as a toggle to turn on/off the tls support in #59 (comment)

Now while we're moving from our internal chart to this one, we realised disabling TLS is quite tricky. Given that helm will merge dictionaries, that actually makes it hard to simply remove a field inside an override file:
805b0ab#diff-f18d9ef27be2f36a73a018900706456eR42

Deploying the chart with a values file with tls: {} content wont overwrite the field in the default values file since helm uses merge strategy for dicts.

So I can see several options here:

1. add an explicit flag to enable/disable the TLS feature
2. set the default value of the tls key in the chart's default values.yaml to {} and comment out anything under the key.
3. update the comment in charts default values.yaml file about how to disable tls by assigning en empty string.

I personally prefer to go with the 2nd option since it's a well known pattern in most charts, and also chart will work as is without any explicit parameters, for now people have to generate certs and create secrets before being able to use the chart. But i don't have a strong opinion for either way.

Reproduce issue

Here is an example about how it can impact the non-tls scenario:

$ cat etcd-values.yaml
tls: {}

$ helm install --values etcd-values.yaml ./chart
NAME:   vocal-tortoise

...cropped...

$ helm status vocal-tortoise
LAST DEPLOYED: Thu Dec 20 13:39:43 2018
NAMESPACE: default
STATUS: DEPLOYED

RESOURCES:
==> v1/ConfigMap
NAME                     AGE
etcd-bootstrap-for-test  34s

==> v1/Service
etcd-for-test-client  34s

==> v1/StatefulSet
etcd-for-test  34s

==> v1/Pod(related)

NAME             READY  STATUS             RESTARTS  AGE
etcd-for-test-0  0/2    ContainerCreating  0         34s

$ kubectl describe pod/etcd-for-test-0
...cropped...
Events:
  Type     Reason       Age                 From               Message
  ----     ------       ----                ----               -------
  Normal   Scheduled    101s                default-scheduler  Successfully assigned default/etcd-for-test-0 to minikube
  Warning  FailedMount  37s (x8 over 101s)  kubelet, minikube  MountVolume.SetUp failed for volume "ca-etcd" : secrets "ca-etcd" not found
  Warning  FailedMount  37s (x8 over 101s)  kubelet, minikube  MountVolume.SetUp failed for volume "etcd-server-tls" : secrets "etcd-server-tls" not found
  Warning  FailedMount  37s (x8 over 101s)  kubelet, minikube  MountVolume.SetUp failed for volume "etcd-client-tls" : secrets "etcd-client-tls" not found

$ helm get values  vocal-tortoise
tls: {}

$ helm get values -a vocal-tortoise
backup:
  backupSecret: etcd-backup
  env: []
  garbageCollectionPolicy: Exponential
  maxBackups: 7
  schedule: 0 */24 * * *
  storageContainer: ""
  storageProvider: Local
  volumeMounts: []
images:
  etcd: quay.io/coreos/etcd:v3.3.10
  etcd-backup-restore: eu.gcr.io/gardener-project/gardener/etcdbrctl:0.4.0
podAnnotations: {}
replicas: 1
role: for-test
tls:
  caSecret: ca-etcd
  clientSecret: etcd-client-tls
  serverSecret: etcd-server-tls

Workaround

For now we applied the workaround to assign an empty string to the tls key in our override (so we use tls: "" in our values file). Here is our workaround:

$ cat etcd-values.yaml
tls: ""

$ helm install --values etcd-values.yaml ./chart
NAME:   ugly-marsupial

...cropped...

$ helm status ugly-marsupial
LAST DEPLOYED: Thu Dec 20 13:29:08 2018
NAMESPACE: default
STATUS: DEPLOYED

RESOURCES:
==> v1/ConfigMap
NAME                     AGE
etcd-bootstrap-for-test  32s

==> v1/Service
etcd-for-test-client  32s

==> v1/StatefulSet
etcd-for-test  32s

==> v1/Pod(related)

NAME             READY  STATUS   RESTARTS  AGE
etcd-for-test-0  2/2    Running  0         32s

$ helm get values  ugly-marsupial
tls: ""

$ helm get values -a ugly-marsupial
backup:
  backupSecret: etcd-backup
  env: []
  garbageCollectionPolicy: Exponential
  maxBackups: 7
  schedule: 0 */24 * * *
  storageContainer: ""
  storageProvider: Local
  volumeMounts: []
images:
  etcd: quay.io/coreos/etcd:v3.3.10
  etcd-backup-restore: eu.gcr.io/gardener-project/gardener/etcdbrctl:0.4.0
podAnnotations: {}
replicas: 1
role: for-test
tls: ""

[bergerx]

I actually missed this fact when i was working with the chart because i was testing the chart with this command:

helm upgrade --install etcd ./chart --set tls=

This command worked since its assigning an explicit empty string to the tls key, the problem happens when we assign a dict.

[swapnilgm]

Same here, tried similar command. Agree i would also prefer 2nd options.

In addition, I was again going through the helm chart standards as mentioned here. And hence i was also thinking of removing dependancy on external action for creating those secret. So, add tls secret as a part of template and have tls section in values.yaml, something like

tls:
#   caBundle: |
#     -----BEGIN CERTIFICATE-----
#     ---
#     -----END CERTIFICATE-----
#   crt: |
#     -----BEGIN CERTIFICATE-----
# .   ---
#     -----END CERTIFICATE-----
#   key: |
#     -----BEGIN PRIVATE KEY-----
#     ---
#     -----END PRIVATE KEY-----`

@georgekuruvillak @shreyas-s-rao WDYS?

[bergerx]

Agreed, i also started looking into this and eventually found myself replacing the labels with the ones that sig-app decided, which are covered here: https://docs.helm.sh/chart_best_practices/#standard-labels

I think better if we go over the whole chart_best_practices as described in the helm docs and update the chart.