Sarif: match.message vs match.details

[mr1chter]

Background

I just added ansible-lint to our jenkins pipeline in our ansible project. There were multiple issues I encountered and I think most of them can be broken down to a very specific discussion.

Maybe as a context: this ansible project is a bit bigger (~6k files) and already established. Our team decided on a few categories we will not follow and ignore, some categories we want to adapt to over time, but categorize as warnings for now and the rest are pipeline-breaking issues.

First, I quickly found the AnsibleLint parser for the WarningsNG plugin, implemented mostly via Regex. I thought "Great, I just need to add the -p flag and pipe the output somewhere warnings-ng can pick it up".

Sadly, it did not determine the severity and put all issues into the "normal" category which would be crucial when figuring out which issue broke the build. Also, it only parses the first line of the issue message and some can be multi-line. I'm not sure if/how the pep8 format even handles multiline-output.

I then looked for alternative formats and intersections of the warnings-ng parser list and the formats of ansible-lint. I found sarif, which conveniently wraps all issues in json, so multiline shouldn't be an issue. Also, severities are handled properly. But it's not all sunshine and roses either, because the sarif formatter (in the issues I tested) outputs more generic warnings than the AnsibleLint plugin. But conveniently we can output both formats with a single ansible-lint invocation and just use both parsers so we get all information.

Obviously this is not convenient at all (and I pulled my hair out a few times while having all these issues). I think I Identified a few core problems, which could make at least sarif work properly (since the pep8 format seems broken anyways, since severities are only tacked on and multiline is also not working).

The problem with formatting (in sarif)

It basically comes down to this line:

ansible-lint/src/ansiblelint/formatters/__init__.py

Lines 292 to 296 in 7849f27

	"message": {
	"text": (
	str(match.details) if str(match.details) else str(match.message)
	),
	},

This was mostly the result of (and with reasonable explanation) #3163. The result of that MR still has a few problems:

1. Since jenkins/WarningsNG does not show the rule description (afaict), I only get to see match.details now, even if match.message has far more interesting content. E.g.
2. match.message, for some rules, can be very specific for the one issue that was detected.

e.g. lets take this issue (normal output from ansible-lint):

jinja[invalid]: Syntax error in jinja2 template: {{ inventory_dir + '/../../files/docker-nginx/hosts/group_a/extra/index.html') }}
inventory/group_a/host_vars/HOST-3.yml:28 unexpected ')'

message is Syntax error in jinja2 template: {{ inventory_dir + '/../../files/docker-nginx/hosts/group_a/extra/index.html') }} and details is unexpected ')'

or e.g. from jinja[spacing] with multiline issues:

jinja[spacing]: Jinja2 spacing could be improved: {%- set a=[] %}
{%- for h in play_hosts %}
  {%- if a.append(xyz_protocol + '://' + internal_hostname + ':' + xyz_port | string) %}
  {%- endif %}
{%- endfor %}
{{ a | unique}}
 -> {%- set a = [] %}
{%- for h in play_hosts %}
{%- if a.append(xyz_protocol + '://' + internal_hostname + ':' + xyz_port | string) %}
{%- endif %}
{%- endfor %}
{{ a | unique }}
 (warning)
inventory/group_b/group_vars/xyz/xyz_instances.yml:43 Jinja2 template rewrite recommendation: `{%- set a = [] %}
{%- for h in play_hosts %}
{%- if a.append(xyz_protocol + '://' + internal_hostname + ':' + xyz_port | string) %}
{%- endif %}
{%- endfor %}
{{ a | unique }}
`.

Here, the details basically repeat part of the message (and the message would be a very bad descriptor for the rule as a whole)

Or another example where the details are not helping at all and the message is the interesting part:

schema[tasks]: $[0].with_items "[ {% for c in xxx_config %} {% for r in c.roles %} {% for p in r.token_policies %} {% for file in lookup('ansible.builtin.fileglob', xxx_dir + '/' + p + '.policy.hcl.j2', wantlist=True) %} {'name': '{{ c.name }}', 'path':'{{ file }}'} {% if not loop.last %},{% endif %} {% endfor %} {% if not loop.last %},{% endif %} {% endfor %} {% endfor %} {% if not loop.last %},{% endif %} {% endfor %} ]" does not match '^\\{[\\{%](.|[\r\n])*[\\}%]\\}$'
roles/xxx-config/tasks/configure-policies-xyz.yml:1  Returned errors will not include exact line numbers, but they will mention
the schema name being used as a tag, like ``schema[playbook]``,
``schema[tasks]``.

This rule is not skippable and stops further processing of the file.

If incorrect schema was picked, you might want to either:

* move the file to standard location, so its file is detected correctly.
* use ``kinds:`` option in linter config to help it pick correct file type.

If you keep in mind that jenkins' sarif visualization will only show the part with "Returned errors will not include..." it's not just unhelpful, it's infuriating, given the context that could be delivered in the message ("the issue is in a with_items block and looks like ")

Another example:

command-instead-of-shell: Use shell only when shell functionality is required.
roles/mydb/tasks/install-mydb.yml:52 Task/Handler: Execute rooting scripts

Here, the message is generic, but the details are not that helpful as well, since we already have the line in the file where the task is written.

Ways to a solution

Imho, the best way out of this would be the following, given we want to change existing rules and output as little as possible:
0. Keep the message and description fields

1. Output both message and description, always, in the sarif issue text.
2. Have a simple rule description that is definitely generic per tag and possibly also includes a link to the documentation. This should then be used in https://github.com/ansible/ansible-lint/blame/main/src/ansiblelint/formatters/__init__.py#L267. (e.g. something like https://github.com/ansible/ansible-lint/blob/main/src/ansiblelint/rules/fqcn.py#L106-L110)
3. Eliminate duplication in message and description (like in jinja[spacing] in the second example above)
4. Define what message and description should contain, in some kind of dev guideline. Because right now it seems to be completely mixed and every rule does its own thing (of course also because the rules differ wildly in what they're checking for)

Of course there are also different options, like only having one message going forward, which includes the details. This would require bigger changes and would also affect the output of other formatters.

I'm also interested in pushing this myself, with implementations and tests. But I want to hear your opinion first, to see if this is reasonable and/or wanted or has some issues. I would imagine that this can also be achieved in small steps, like first doing 1., progressively doing 2 and 3 and see what 4 should contain. Imho 4. is the most difficult part to figure out and should be up to other people to decide.

Also tagging @4ch1m and @atiterlea since they were involved in the last bigger changes of the sarif output and "fixing jenkins" shouldn't break other peoples pipelines.

[4ch1m]

Hi there,

The current implementation/SARIF-output always does provide both the match.message and the match.details.

The "message" is always attached to the rule-definition itself:

ansible-lint/src/ansiblelint/formatters/__init__.py

Lines 263 to 268 in 7849f27

	rule: dict[str, Any] = {
	"id": match.tag,
	"name": match.tag,
	"shortDescription": {
	"text": str(match.message),
	},

The "details" are present in the result-item (which refers to the rule):

ansible-lint/src/ansiblelint/formatters/__init__.py

Lines 289 to 295 in 7849f27

	result: dict[str, Any] = {
	"ruleId": match.tag,
	"level": self.get_sarif_result_severity_level(match),
	"message": {
	"text": (
	str(match.details) if str(match.details) else str(match.message)
	),

e.g.

{
    "$schema": "https://schemastore.azurewebsites.net/schemas/json/sarif-2.1.0-rtm.5.json",
    "version": "2.1.0",
    "runs": [
        {
            "tool": {
                "driver": {
                    "name": "ansible-lint",
                    "version": "6.22.1",
                    "informationUri": "https://github.com/ansible/ansible-lint",
                    "rules": [
                        {
                            "id": "name[casing]",
                            "name": "name[casing]",
                            "shortDescription": {
                                "text": "All names should start with an uppercase letter."
                            },
                            "defaultConfiguration": {
                                "level": "warning"
                            },
                            "help": {
                                "text": "All tasks and plays should have a distinct name for readability and for ``--start-at-task`` to work"
                            },
                            "helpUri": "https://ansible-lint.readthedocs.io/rules/name/",
                            "properties": {
                                "tags": [
                                    "idiom"
                                ]
                            }
                        },
...
                    ]
                }
            },
            "columnKind": "utf16CodeUnits",
            "results": [
                {
                    "ruleId": "name[casing]",
                    "level": "error",
                    "message": {
                        "text": "All names should start with an uppercase letter."
                    },
                    "locations": [
                        {
                            "physicalLocation": {
                                "artifactLocation": {
                                    "uri": "test-playbook.yml",
                                    "uriBaseId": "SRCROOT"
                                },
                                "region": {
                                    "startLine": 1
                                }
                            }
                        }
                    ]
                },
...

So... all the information is already available:

• rules -> shortDescription (= general description)
• rules -> help (= elaborate description)
• results -> message (= specific description, which may be the same as shortDescpription; but can differ)

It's up to you/the consumer of the SARIF-output to parse/compile this information.

I don't see the need or any value in further merging these texts (and produce redundant content).

[mr1chter]

As explained, the message in rules->shortDescription is only stored once per rule tag and the assumption that the message is the same for all rule violations just does not hold. So individual information gets lost or wrongfully assigned when a rule (with tag) is triggered more than once (see the examples, the rule description would be very specific and in some cases not "short" at all; the message is everything that comes before the <file>:<line> output).

Also, my specific implementation of the sarif parser does not use the rule descriptions at all so they are very much lost, at least when using jenkins.

[mr1chter]

And also: the proposal is not just to merge the texts.
Another solution could also be to move all content where "message" is not generic into "description". This would change the output of all other formats so I didn't propose it. But then why have individual "message"s at all, when we already have rule-based information like in rules->help

[4ch1m]

It's not my call, since I'm not responsible or the owner of this project.
But... make a PR.
Let people have a look at it; and decide if things improved.