vcluster

vcluster copied to clipboard

https://github.com/loft-sh/vcluster/blob/main/chart/templates/statefulset.yaml#L32

spec.volumeClaimTemplates.0.spec.storageClassName by default renders as null instead of a value or empty string. This breaks kustomize validation.

  {{- if .Values.storage.persistence }}  <== default renders as to IF TRUE
  {{- if not .Values.storage.volumeClaimTemplates }}  <== defaults renders to IF NOT FALSE
  volumeClaimTemplates:
    - metadata:
        name: data
      spec:
        accessModes: [ "ReadWriteOnce" ]
        storageClassName: {{ .Values.storage.className }} <== undefined in values file
        resources:
          requests:
            storage: {{ .Values.storage.size }}
  {{- else }}
  volumeClaimTemplates:
{{ toYaml .Values.volumeClaimTemplates | indent 4 }}
  {{- end }}
  {{- end }}

I'm actually surprised Helm doesn't complain about a null value to the key, seems like a break of yaml spec.

K8s doesn't care if the field is defined or not, and the docs simply state to use a string. storageClassName: "" # Empty string must be explicitly set otherwise default StorageClass will be set https://kubernetes.io/docs/concepts/storage/persistent-volumes/#class-1 https://v1-21.docs.kubernetes.io/docs/reference/generated/kubernetes-api/v1.21/#persistentvolumeclaimspec-v1-core

Proposed:

• Define the key 'storageClassName' only if a name is defined in values OR
• Define a default in values "Filesystem" is the inferred value when undefined per: https://github.com/kubernetes/api/blob/release-1.21/core/v1/types.go#L489

@jburnitz thanks for creating this issue! Yeah I guess the best solution is that we only define storageClassName if a name was defined in the values

This is the same in the chart, so we should fix it.

As for this sentence from the description:

Define a default in values "Filesystem" is the inferred value when undefined per: https://github.com/kubernetes/api/blob/release-1.21/core/v1/types.go#L489

The default value of "Filesystem" is for the volumeMode field, which we don't set.

@matskiv I can try to fix this minor issue if that's okay (especially if I get a few days to work on it). I didn't find the mentioned main/chart/templates/statefulset.yaml#L32 repo but only the main/charts/**/templates one.

This is an excellent issue to be worked on before diving into the actual vcluster's logic.

@vsantos Yeah, absolutely! I'll assign this to you then. You can take your time, there is no rush with this issue.

---

I didn't find the mentioned main/chart/templates/statefulset.yaml#L32 repo but only the main/charts/**/templates one.

Yeah, we added more charts since this was reported :smile: I would say that the issue applies to all storageClassName references in all charts/**.

---

btw: a tip for quick validation. First time I was testing some chart changes I did in a way too complicated way, so sharing just in case.

helm template vcluster ./charts/k3s/ --set storage.className=test | grep storageClassName
helm template vcluster ./charts/k8s/ | grep storageClassName

I used a very similar command to this just the other day, it's very helpful for quickly checking the output of the chart. The first one tests k3s chart with .Values.storage.className set. Second tests k8s chart without the .Values.storage.className value. And then I grep all outputted manifests... you get the idea :)

@matskiv Do you guys think is a good idea to add some kind of "automated" mechanism to make it easier to test static YAML files as part of the scope of this PR? We could use bats, it's a very old tool but good in make assertions in bash scripts. Your suggested logic to test locally could be automated to ensure that a given values.yaml will return specific structures.

Or maybe we should just fix the broken logic and in the future start a new thread about automation alternatives?

@vsantos Oh, I would love to have automated tests for the charts. This has been on my todo list for a while, but it always gets pushed back :joy: I would try to first look for some tools tailored to Helm, to have sort of a "unit tests" for the Helm charts. But this should be a separate issue for sure :)

@matskiv I would like to contribute to this issue, could you please assign it to me?

@hiteshwani29 Thank you for your interest. It's assigned to you now.