Feature Request: integrate salint rules

[max-arnold]

What do you think about integrating all rules from the (probably unmaintained) salint package into salt-lint?

It is able to catch things like this one:

test:
  file.touch:
    - name: /tmp/test
    - require:
      - test: test2
    - wathc_in:     # <-- typo here, should be watch_in
      - service: some-service

Also, it would be nice to be able to inject custom config values here https://github.com/gtmanfred/salint/blob/master/salint/script.py#L17 and specify pillar/grain fixtures.

[roaldnefs]

Thanks @max-arnold for your feature request!

I'm going to take a stab at a PR in the upcoming days, I've been thinking about a feature like this since the beginning of salt-lint.

[roaldnefs]

I'm now able to locally render the state which would be able to catch errors in simple states. There are however a few disadvantages to rendering the states:

• Rendering the states would cost a lot of time (couple of seconds per state).
• salt-lint needs to be executed from the location where the top.sls lives (would require a workaround to work on formulas, which don't contain a top.sls).
• States wrapped within a Jinja statement (see example below) would require the grain and pillar data to be available for salt-lint, because I see a lot of projects using a pillar.example we might want to automatically load any existing pillar.example file and figure out how to can supply the required grains data.

{% if salt['pillar.get']('test') %}
test:
  file.touch:
    - name: /tmp/test
    - require:
      - test: test2
    - wathc_in:     # <-- typo here, should be watch_in
      - service: some-service
{% endif %}

While I still would like to implement this feature, I don't think all salt-lint users would like to wait for a state to be rendered each time. Therefore I'm thinking about making the rendering part opt-in (requiring a flag to be set before it runs).

To allow somewhat more complex rules, I'm also working om a YAML renderer in #37. Using this feature we can easily add rules to check for deprecated options and for things like cmd.wait which could be found for both the standard declaration and inline function declaration, e.g.:

{% if salt['pillar.get']('test') %}
test:
  cmd:          # standard declaration
    - wait
    - name: mycommand
    ...

test:
  cmd.wait:   # inline function declaration
    - name: mycommand
    ...
{% endif %}

[max-arnold]

Rendering the states would cost a lot of time (couple of seconds per state).
I don't think all salt-lint users would like to wait for a state to be rendered each time. Therefore I'm thinking about making the rendering part opt-in (requiring a flag to be set before it runs).

Making this feature opt-in is a completely reasonable thing to do. This is also good from a security perspective (the state rendering process could have side effects).

salt-lint needs to be executed from the location where the top.sls lives (would require a workaround to work on formulas, which don't contain a top.sls)

Could you please elaborate on why it needs to work this way?

States wrapped within a Jinja statement (see example below) would require the grain and pillar data to be available for salt-lint, because I see a lot of projects using a pillar.example we might want to automatically load any existing pillar.example file and figure out how to can supply the required grains data.

1. This is not a problem for well-written community formulas, because they should always provide some default pillar values: https://docs.saltstack.com/en/latest/topics/development/conventions/formulas.html#pillar-overrides
2. In other cases, a way to provide pillar/grain fixtures (both global and per-state) would be useful. I'm thinking about having multiple sets of global user-provided fixtures to simulate different operating systems (grains) and pillar combinations.

Using this feature we can easily add rules to check for deprecated options

This is super cool!

When I asked for a possibility to inject custom config values, one of the use-cases I had in mind is to be able to flip deprecation settings like the use_superseded: [module.run] and somehow catch the warnings logged by Salt (maybe by overriding the logger and providing custom matching rules).

[max-arnold]

Re: Twitter
Please let me know what additional checks you would like to see.

This issue saltstack/salt#5378 (and the ones referenced inside) could provide more hints of what other people want from a Salt linter.

[max-arnold]

I also feel obligated to repost a discussion from Slack (that happened recently) while it is still available, just for a future reference:

Oloremo:

I'm once again puzzled by the fact Salt has no any linter for SLS. It have at least an issue about it dating from 2013... saltstack/salt#5378
I tried to google it and only found some alpha state abandoned projects. How do you guys manage without it?

gtmanfred:

Some people use kitchen-salt and write tests with inspec or testinfra to verify changes.
There is no good way to lint, I started one that checks for invalid syntax, it is called salint
But it would not have caught the bug mentioned in that issue, because file managed accepts **kwargs

Oloremo:

We do use test for our formulas and that's covers the syntax checks but I wish I could check for the style

Matt:

Oloremo define style?
sls just means state data
its not necessarily yaml with jinja
though generally more often than not it is

Oloremo:

define style?
"All file.manage tasks should have mode"
"state ids should be in the specific format"
"do not use _any requisites"
etc. Style.

--- subthread ---

Wayne Werner:

You could certainly create some tooling using salt to apply here.
My current experience says that if your states are so complicated that a linter would be helpful maybe you should simplify.
But I’ve not managed several tens/hundreds of thousands of nodes, so it’s possible that there are different scales for best practices…

Oloremo:

even creating 2 states which is kinda do the same thing but were written completely differently is the problem. And yes we do catch that during review but in ansible we don't even have to since linter catch it first.

Wayne Werner:

When you say “kinda the same thing” what does that mean?

Oloremo:

extract archive
template a file
start a service
yet it's possible to write that in a many different ways and we want to enforce at least some rules

Wayne Werner:

That makes sense.
You could pretty reasonably write one I think.

Oloremo:

I have no idea how. 🙂 This tool should have a good foundation to be used by most of salt users like ansible-lint and I think the only way to archive this is to be a damn good programmer with understanding how salt works or it should be written by salt folks 🙂
I mean I kinda think it's more or less easy after the rendering part which is confusing

Wayne Werner:

Well, the royal, “You”
In order to write something that’s generally useful, you’d have to know, generally, how people use Salt, and what practices work well
I’m certain there are some practices that are painful - but I’m not sure if there are any obviously good choices
Linting is easy if there are some clear-cut, well defined rules. But I’m not sure that our user base actually agrees on what those rules would be
some people don’t care whether their file.managed state provides mode
others are much more lax about what their state IDs should look like - or, their standards are vastly different than yours

Oloremo:

But I’m not sure that our user base actually agrees on what those rules would be
They don't have to, you should have be able to write your own rules. And enforce only the basic ones like "all tasks* should have name"

• how does it's even called in salt? state declaration?..

Wayne Werner:

Well, if we expect anyone to use it we would 🙂

Oloremo:

well ansible community somehow set the number of rules for the lint. I think salt community will do the same eventually

marnold:

There is a reason why some tools enforce styles automatically (think of black, gofmt, etc). This saves so many pointless efforts!
However, having a linter for Salt would be almost impossible, unless you significantly restrict its syntax and feature set. In other words, Salt is too dynamic/flexible. A state can:

• Use yaml or json or any other serialization format
• Include other states (the linter must understand the include directives)
• Contain dynamic templating tags and module calls, that break yaml/json/whatever syntax
• Depend on host-specific pillar values (including Jinja conditionals)
• Depend on host-specific grains or mine data (is it even possible to inject that kind of data into a linter?)
• Contain values that are expanded in runtime (slots)
• State arguments aren't always explicit, because some functions just accept *args or **kwargs (sometimes for valid reasons, for example to pass them as is to the underlying always-changing and complex library)
• Run on a master (orchestration states)
  Even if you use the py renderer and want to rely on your Python IDE to do the linting, it will fail to understand magically injected dunders.

msmith:

the (perhaps complicated) argument is more a case of picking up common cases, for example, one pass could blank values and check yaml, another could check the argument names against a "known" contextual list that can be expanded on as time goes by, another pass could work against specific args with explicit values. another pass might even try to render the content to catch rendering errors, and then check the result. but right now i don't think anything exists to do these things, sadly
there are lots of distinct things that could be checked, and would pick up a majority of issues. as with all things there's going to be diminishing returns on the uncommon things, but over time even those could be added in

Oloremo:

in a true nature of salt it could be more of a framework than a solution. That'd be fine by me. Community will build their own set of rules and after sometime one of them will became a popular enough to be considered a norm.

msmith:

if such a solution were mature enough then salt might accept it, but other than that i would expect this to remain a community effort. i don't have the ability to write such a linter myself, but perhaps my suggestions could be useful to someone else

Oloremo:

I still kinda think the framework should come from salt folks itself, it's indeed a bit too complex to pass throught the render stage (edited)

msmith:

feel free to suggest this as a wishlist item, but i suspect it won't get any attention unless someone from the community does it

---

Matt:

those you could do pretty easy
doing the equivalent of mocking the dunders, then doing the check

Oloremo:

pretty easy - is to write something like "use salt render to render all sls and then parse yaml with some parser which you need to write too"?
Ansible has a https://github.com/ansible/ansible-lint which works great. And I understand that it's a way easier to implement for ansible
having the same thing for salt would be amazing

pier:

My 2 cents about state linting: I did setup a CI job (gitlab) to lint and more using the following shell command used as a post-commit hook :
salt-call --retcode-passthrough --local --file-root=. --pillar-root=/home/gitlab-runner/salt_pillars/lab_rms_loc/ state.show_sls ${commit%.*}
This is not perfect nor classy but at least it works, it prevents teams from commiting wrongly formatted states files...

[dawidmalina]

Having ability to render state files will also (correct me if I'm wrong) allow us to discover all type of syntax errors - so I like this direction even if it will slow check a bit 👍

[roaldnefs]

Thanks @dawidmalina for your comment! I've updated the priority accordingly. Hope to get some work done on the feature request in the next couple of weeks.

[dawidmalina]

@roaldnefs i have very simple rule to validate (render) sls file: https://github.com/dawidmalina/salt-lint/blob/sls/saltlint/rules/StateCanBeRendered.py

If you are interested in I can drop a pull request with this.