GoogleContainerTools / skaffold

Easy and Repeatable Kubernetes Development
https://skaffold.dev/
Apache License 2.0
15.02k stars 1.62k forks source link

Deploying Helm - Skaffold wants to parse quoted digits as numbers #8686

Open witchbutter opened 1 year ago

witchbutter commented 1 year ago

Expected behavior

To be able to enter strings containing digits in the setValues of a helm release

Actual behavior

Inescapable nuisance error:

Error: INSTALLATION FAILED: 1 error occurred:
        * StorageClass in version "v1" cannot be handled as a StorageClass: json: cannot unmarshal number into Go struct field StorageClass.parameters of type string

Information

apiVersion: skaffold/v3
kind: Config
metadata:
  name: base-app
deploy:
  helm:
    releases:
      - name: efs-csi
        remoteChart: aws-efs-csi-driver
        # https://docs.aws.amazon.com/eks/latest/userguide/efs-csi.html
        version: 2.4.1
        repo: https://kubernetes-sigs.github.io/aws-efs-csi-driver
        namespace: kube-system
        wait: true
        setValues:
          storageClasses:
              - name: efs-immediate
                volumeBindingMode: Immediate
                reclaimPolicy: Delete
                mountOptions:
                  - tls
                parameters:
                  provisioningMode: efs-ap
                  directoryPerms: "700"
                  gidRangeStart: "1000"
                  gidRangeEnd: "2000"
                  basePath: "/data"
        setValueTemplates:
          storageClasses:
            - name: efs-immediate
              parameters:
                #pay attention to camelCase, it will accept any parameter even if it's wrong
                fileSystemId: "{{.EFS_FILESYSTEM_ID}}"

Steps to reproduce the behavior

  1. Create an EKS cluster, to which this is relevant.
  2. Put the above code in skaffold.yaml
  3. export the variable EFS_FILESYSTEM_ID to something sane
  4. skaffold deploy

I'm believe this is related to the Helm variation on Go templating.

aran commented 1 year ago

I hit a variation on this with a helm chart that had a setValueTemplates including:

image.tag: '{{.IMAGE_TAG_my_service}}'

when git rev-parse --short HEAD happened to be all-numeric.

Resulting error was:

std out err: %!(extra *errors.errorstring=error: values don't meet the specifications of the schema(s) in the following chart(s): <snip>

aran commented 1 year ago

Implementation note, supporting https://yaml.org/type/ might be one way. In my case, I didn't have a chance to prioritize testing if !!str was already supported and would solve this. For the example above, this would look like fileSystemId: !!str "{{.EFS_FILESYSTEM_ID}}"

renzodavid9 commented 1 year ago

Hello there! Taking a look to this, I was able to force the quotes in the values set in setValues and setValueTemplates properties. It will look something like this:

...
deploy:
  helm:
    releases:
      - name: skaffold-helm
        chartPath: charts
        setValues:
          firstValue: '"111"'
        setValueTemplates:
          templateValue: '"{{.VAR}}"'

Calling VAR=222 skaffold render -v DEBUG will make Skaffold to invoke Helm like this:

...
DEBU[0000] Running command: [helm --kube-context local-cluster template skaffold-helm charts --post-renderer /usr/local/bin/skaffold --set firstValue="111" --set secondValue="222"]  subtask=0 task=Render
...

Which will produce a manifest with the double quotes included, something like this for the example I was using:

apiVersion: apps/v1
kind: Deployment
metadata:
  labels:
    app: skaffold-helm
    firstValue: "111"
    secondValue: "222"
...

From @aran comment, Skaffold does not support https://yaml.org/type/ for the setValues and setValueTemplates properties, it will cast everything as a string automatically: https://github.com/GoogleContainerTools/skaffold/blob/ed48957026d4775315ca497ed1a6c783139f9638/pkg/skaffold/schema/latest/config.go#L949-L957

FlatMap defined in: https://github.com/GoogleContainerTools/skaffold/blob/ed48957026d4775315ca497ed1a6c783139f9638/pkg/skaffold/schema/util/util.go#L40-L41

Will take a look on if/how we can support the https://yaml.org/type/ in these properties. Hopefully the workaround explained before will help. Thanks! 😄

dmavrommatis commented 12 months ago

hi @renzodavid9 , for me the workaround by escaping with quotes does not work.

The chart from pebble is like this

            {{- with .Values.pebble.env }}
            {{- . | toYaml | nindent 12 }}
            {{- end }}

and then I set the values like this

        setValues:
          pebble.env:
            - name: PEBBLE_VA_NOSLEEP
              value: '"1"'
            - name: PEBBLE_WFE_NONCEREJECT
              value: '"0"'
            - name: PEBBLE_AUTHZREUSE
              value: '"100"'
            - name: PEBBLE_VA_ALWAYS_VALID
              value: '"1"'

and the rendered manifest has both ' and ". If I do not add the quotes the render fails because the helm chart has a schema that it only allows string.