sbaudoin / yamllint

YAML Linter written in Java
Apache License 2.0
18 stars 12 forks source link

Support for common YAML templates (specifically Helm) #16

Closed jackwhelpton closed 3 years ago

jackwhelpton commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

Also running into this issue here -> +1

H3rby7 commented 4 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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:

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 commented 3 years ago

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 commented 3 years ago

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).

rufreakde commented 2 years ago

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 commented 2 years ago

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

rufreakde commented 2 years ago

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 commented 2 years ago

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 commented 8 months ago

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