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

literal-compare: false positive inside strings - conflict with bash code #470

Closed
ssbarnea opened this issue Jan 23, 2019 · 4 comments · Fixed by #1315
Closed

literal-compare: false positive inside strings - conflict with bash code #470

ssbarnea opened this issue Jan 23, 2019 · 4 comments · Fixed by #1315
Assignees
Labels
Milestone

Comments

@ssbarnea
Copy link
Member

Simple example that replicates the bug

    - shell:
        cmd: |
              if [ $FOO != false ]; then

It seems that the linter is looking for this piece of code without even knowing if that is a string or a value evaluated by ansible.

This bug makes the entire E601 rule something to add to the skip_list.

@awcrosby
Copy link
Contributor

Since the rule description specifically calls out the use of True/False in when, it might be best to limit this rule 601 (and 602) to check only the when clause instead of all lines as it currently does.

@ssbarnea
Copy link
Member Author

ssbarnea commented Feb 3, 2019

The issue seems to become even worse, another false positive found at https://github.com/kubernetes-sigs/kubespray/blob/a5edd0d709f28507635771cefae43790f4265f20/roles/kubernetes-apps/network_plugin/kube-router/tasks/main.yml#L15 and for this one I have no idea about how to avoid it.

@direvus
Copy link

direvus commented Mar 2, 2020

Agreed @awcrosby, the rule should only be evaluated against Jinja2 expressions. I just got hit by this bug on the following task:

- name: "PRELIM | Gather accounts with empty password fields"
  shell: |
    set -o pipefail
    cat /etc/shadow | awk -F: '($2 == "") {j++;print $1;} END {exit j}'
  args:
    executable: bash
  register: empty_password_accounts
  changed_when: false
  check_mode: false

ansible-lint, suddenly developing strong opinions about AWK programs, had this to say:

[602] Don't compare to empty string
roles/RHEL7-CIS/tasks/prelim.yml:13
    cat /etc/shadow | awk -F: '($2 == "") {j++;print $1;} END {exit j}'

Go home, ansible-lint, you're drunk.

There's no way rules 601 or 602 should be inspecting the interior of shell or command free-form argument, unless they are able to correctly distinguish Jinja2 expressions from shell code. I can appreciate there is probably a lot of overhead in parsing out the Jinja2 to check this, so I would be supportive of limiting these rules to examine the when clause only.

@ssbarnea are you able to add E602 to the issue title?

@ssbarnea ssbarnea added bug and removed type/bug labels Jul 24, 2020
@isuftin
Copy link

isuftin commented Dec 10, 2020

Seeing similar:

- name: Find empty password fields in /etc/shadow
  shell: 'awk -F: ''($2 == "" ) { print $1 }'' /etc/shadow'
  register: empty_password_fields
  changed_when: empty_password_fields.stdout_lines | length > 0

@ssbarnea ssbarnea changed the title E601 false positive inside strings - conflict with bash code literal-compare: false positive inside strings - conflict with bash code Feb 7, 2021
@ssbarnea ssbarnea self-assigned this Feb 7, 2021
@ssbarnea ssbarnea added this to the 5.0.0 milestone Feb 7, 2021
ssbarnea added a commit that referenced this issue Feb 7, 2021
Refactor rule to only look inside when conditions and avoid
using line-parser.

Fixes: #470
ssbarnea added a commit that referenced this issue Feb 7, 2021
Refactor rule to only look inside when conditions and avoid
using line-parser.

Fixes: #470
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants