Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support for common YAML templates (specifically Helm) #16

Closed
jackwhelpton opened this issue Jun 27, 2020 · 13 comments
Closed

Support for common YAML templates (specifically Helm) #16

jackwhelpton opened this issue Jun 27, 2020 · 13 comments

Comments

@jackwhelpton
Copy link

jackwhelpton commented Jun 27, 2020

One common form of YAML file found in applications is the Helm chart, which utilizes a template language that commonly results in two linting errors, even though the YAML is valid.

Firstly, I tend to set up the linter to enforce a single space after opening braces and before closing ones. Because {{ is used as a templating expression, code such as this:

labels:
  app: {{ template "my-application.name" . }}

results in "too few spaces inside braces (braces)". I could allow 0 or 1 space, which I may be forced to do, but it'd be great if there was a way to override this check if the next character is also a brace, allowing {{.

In a related fashion, this line is flagged with "Parse error: syntax error: expected the node content, but found '-'":

{{- $context := index .Values "current-context" | default "dev" }}

{{- is used in Helm to trim whitespace. Is there a way to tweak the rules to avoid flagging this construct as an error?

@jackwhelpton
Copy link
Author

What would be great here would be a mode (i.e. perhaps enabled by a boolean, if we don't want it as default behavior) that is capable of handling Jinja expressions:

{{ content }}
{% content %}
{# content #}

by checking space around the 2-character tokens, together with allowing Helm's use of -, which can be added to the opener, closer or both:

{{- content }}
{{ content -}}
{{- content -}}

yamllint in template mode would check the spacing after {{- or before -}}, and not treat - as node content in those scenarios.

@H3rby7
Copy link

H3rby7 commented Aug 4, 2020

Also running into this issue here -> +1

@H3rby7
Copy link

H3rby7 commented Aug 4, 2020

Actually this would make a bigger issue. I built a little script to "fix" the error we both had - see below

The issue is - it just moves on to another syntax error right after as this linter is not designed to lint helm templates. This would require more logic to be implemented. Basically "the whole helm templating".

Script

Run for Example in this container: quay.io/helmpack/chart-testing:v3.0.0 (must run apk add bash once for the bash script to work).

#!/bin/bash

echo "Stashing workDirectory"
git stash

yamlFiles=(`find | grep -e ".ya\?ml"`)

function replaceHelmSyntax() {
    sed -i "s!{{-!{{!g" ${1}
}

s="charts/my-service/templates/deployment.yaml"

#for s in ${yamlFiles[@]}; do
    replaceHelmSyntax ${s}
    cat -n ${s}
    yamllint ${s}
#done

echo "Resetting workDirectory"
git checkout .
git stash pop

@sbaudoin
Copy link
Owner

sbaudoin commented Nov 3, 2020

Hi,

I don't know how to tackle this need. It's a kind of chicken and egg thing: what is true YAML and that must be linted? The not yet processed, templated YAML or the YAML document that is the result of the template resolution process? My first reflex was to have a kind of "escaper" just like @H3rby7 did in Bash but this may not be we expect from such a feature: we can imagine "ugly" Jinja templates that would put together odd parts of YAML fragments to produce a valid YAML document: is fragment, if linted, would be full of errors but once processed, the complete YAML document could be syntactically correct. The problem is that embedding all templating engines is not possible, so what to do?

@sbaudoin
Copy link
Owner

sbaudoin commented Nov 3, 2020

Having the linter to sort of ignore (or recognize) the template strings is feasible but at an important cost: it will require hacking (not to say reimplementing) SnakeYAML, which is something I'm not very likely to do. So help is wanted!

@jackwhelpton
Copy link
Author

jackwhelpton commented Nov 3, 2020

Perhaps ignoring templated YAML files altogether is the most feasible thing here? My main concern when I raised the ticket was that we have lots of repos that contain a large number of Helm charts, and the number of false positives resulting from these renders enabling yamllint in SonarQube impractical.

I also do occasionally see bugs that result from incorrect indentation within these templates, but without running the templating engine I suspect finding these would be next to impossible.

For Helm charts specifically, we could potentially identify a chart via the directory structure, to save complex parsing changes:

  • file is contained underneath a templates directory (possibly several levels deep)
  • directory containing that templates directory also contains a file called Chart.yaml

I know this isn't a general solution to the templated YAML problem, but it may be sufficient for what I imagine to be a common use case. I wonder if there are other widely-used templates that still bear the .yaml extension: I suspect this is the most prevalent.

Dropping the second requirement (so just adding the means to say "if the path matches *\templates\*, skip) may actually make this approach more generally applicable.

@sbaudoin
Copy link
Owner

sbaudoin commented Nov 3, 2020

Thanks for the feedback. So yes, you would need to call yamllint after the template engines. However I can work on an exclusion mechanism to counterbalance the recursive analysis. BTW if you use the sonar-yaml plugin, you have the possibility to exclude paths from the analysis if I remember well. So this raises the question: how do you actually call yamllint?

@sbaudoin
Copy link
Owner

sbaudoin commented Nov 7, 2020

The tool already allows you to ignore some files with the ignore configuration parameter. I'm going to document it better and also bring into the configuration the list of files considered as YAML files (currently, this is hardcoded to .yml and .yaml files only).

sbaudoin added a commit that referenced this issue Nov 7, 2020
…env variables to give path to yamllint conf file
@rufreakde
Copy link

So as I saw several posts regarding this issue I wanted to share a simple fix for your pipeline scans:

echo "# Yamllint - Checking dev/"
helm template dev ./dev/helm --output-dir=./dev-rendered/
yamllint ./dev-rendered || exit 3

./dev/helm is where my helm chart resides with the respective subcharts and templates.
and then I execute yamllint on ./dev-rendered and if it has an error it exits with error code 3 so my run fails :)
helm template also keeps formatting and comments in place ;)

Hope this helps others coming here after me. Also could be a good addition to the general yamllint wiki as helm is a veeeeery wide spread use-case.

@orefalo
Copy link

orefalo commented May 31, 2022

the above doesn't work when you have a malformed yaml to begin with.

@rufreakde
Copy link

rufreakde commented Jun 18, 2022

the above doesn't work when you have a malformed yaml to begin with.

orefalo this will if you use the shell pipefail which you should try to use in your pipeline to fail early. Helm template will fail so either way you validate the validity of your yaml manifests.

@orefalo
Copy link

orefalo commented Jun 19, 2022

Hi, a bit confused by your answer. Apologies, I don't have your expertise (and may be missing context)

helm can be quite cryptic about yaml parsing errors, the other day (when I found this post), I spend 2h to find the source of the problem - no surprise k8s is a pain to learn.

Anyways, I might be out of context on this thread.
My goal was to find a tool which would save me 2h ;-)

@jshields
Copy link

jshields commented Jan 16, 2024

I ran across this thread trying to use yamllint on my Helm charts. I think this command directly from Helm might be better suited:
https://github.com/helm/chart-testing

And maybe:
https://helm.sh/docs/helm/helm_lint/

If you are using GitHub Actions, the chart testing tool has an Action directly from the Helm org, and appears to use yamllint under the hood:
https://github.com/helm/chart-testing-action

zasgar pushed a commit to gimbal-ai/gimbal that referenced this issue Jul 9, 2024
Summary: We will use a postgres deployment for dev deployments and
cloudSQL for testing/staging/prod.
Unfortunately, yamllint doesnt handle templated YAMLs:
sbaudoin/yamllint#16. Alternatives to
disabling any YAMLs in the templates directory would be to name the
files something other than .yaml (however that is the recommended for
helm).

Relevant Issues: N/A

Type of change: /kind infra

Test Plan: skaffold run with dev profile

Signed-off-by: Michelle Nguyen <[email protected]>
GitOrigin-RevId: 6c71b25abf82c6666cdc5cc626fafd12fbfa8581
zasgar pushed a commit to gimbal-ai/gimbal that referenced this issue Jul 9, 2024
Summary: We will use a postgres deployment for dev deployments and
cloudSQL for testing/staging/prod.
Unfortunately, yamllint doesnt handle templated YAMLs:
sbaudoin/yamllint#16. Alternatives to
disabling any YAMLs in the templates directory would be to name the
files something other than .yaml (however that is the recommended for
helm).

Relevant Issues: N/A

Type of change: /kind infra

Test Plan: skaffold run with dev profile

Signed-off-by: Michelle Nguyen <[email protected]>
GitOrigin-RevId: 6c71b25abf82c6666cdc5cc626fafd12fbfa8581
zasgar pushed a commit to gimbal-ai/gimbal that referenced this issue Jul 9, 2024
Summary: We will use a postgres deployment for dev deployments and
cloudSQL for testing/staging/prod.
Unfortunately, yamllint doesnt handle templated YAMLs:
sbaudoin/yamllint#16. Alternatives to
disabling any YAMLs in the templates directory would be to name the
files something other than .yaml (however that is the recommended for
helm).

Relevant Issues: N/A

Type of change: /kind infra

Test Plan: skaffold run with dev profile

Signed-off-by: Michelle Nguyen <[email protected]>
GitOrigin-RevId: 6c71b25abf82c6666cdc5cc626fafd12fbfa8581
zasgar pushed a commit to gimbal-ai/gimbal that referenced this issue Jul 10, 2024
Summary: We will use a postgres deployment for dev deployments and
cloudSQL for testing/staging/prod.
Unfortunately, yamllint doesnt handle templated YAMLs:
sbaudoin/yamllint#16. Alternatives to
disabling any YAMLs in the templates directory would be to name the
files something other than .yaml (however that is the recommended for
helm).

Relevant Issues: N/A

Type of change: /kind infra

Test Plan: skaffold run with dev profile

Signed-off-by: Michelle Nguyen <[email protected]>
GitOrigin-RevId: 6c71b25abf82c6666cdc5cc626fafd12fbfa8581
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants