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

False positive load-failure when using import_tasks / include_tasks #1446

Closed
europ opened this issue Mar 8, 2021 · 20 comments · Fixed by #2202
Closed

False positive load-failure when using import_tasks / include_tasks #1446

europ opened this issue Mar 8, 2021 · 20 comments · Fixed by #2202
Assignees
Labels
bug help wanted reproducer: present This PR or issue contains code, which reproduce the problem described or clearly understandable STR

Comments

@europ
Copy link
Contributor

europ commented Mar 8, 2021

Summary

Internal logic for file path evaluation seems to be broken in ansible-lint.

Related: #540
Reproducer: #1445

Issue Type
  • Bug Report
Ansible and Ansible Lint details
# installed as pip package within pyenv-virtualenv 
ansible 2.10.5
ansible-lint 5.0.2
OS / ENVIRONMENT
# OS
Linux

# ENV
pyenv 1.2.23
pyenv-virtualenv 1.1.5

# Python
python 3.8.3
pip 21.0.1
STEPS TO REPRODUCE

The example below includes only import_tasks while the same bug applies to the include_tasks as well.

Project layout example: ansible-lint-bug.zip

Project files and content:

./main.yml:1 ---
./main.yml:2 - hosts: target
./main.yml:3   gather_facts: false
./main.yml:4   tasks:
./main.yml:5     - name: from main import task 1
./main.yml:6       import_tasks: tasks/task_1.yml

./tasks/task_1.yml:1 ---
./tasks/task_1.yml:2 - name: from task 1 import task 2
./tasks/task_1.yml:3   import_tasks: tasks/task_2.yml

./tasks/task_2.yml:1 ---
./tasks/task_2.yml:2 - name: from task 2 import subtask 1
./tasks/task_2.yml:3   import_tasks: tasks/subtasks/subtask_1.yml

./tasks/subtasks/subtask_1.yml:1 ---
./tasks/subtasks/subtask_1.yml:2 - name: from subtask 1 import subtask 2
./tasks/subtasks/subtask_1.yml:3   import_tasks: tasks/subtasks/subtask_2.yml

./tasks/subtasks/subtask_2.yml:1 ---
./tasks/subtasks/subtask_2.yml:2 - name: from subtask 2 do something
./tasks/subtasks/subtask_2.yml:3   debug:
./tasks/subtasks/subtask_2.yml:4     msg: |
./tasks/subtasks/subtask_2.yml:5       Something...

./ansible.cfg:1 [defaults]
./ansible.cfg:2 stdout_callback = debug

./inventory/hosts:1 [target]
./inventory/hosts:2 localhost

./inventory/host_vars/localhost.yml:1 ---
./inventory/host_vars/localhost.yml:2 ansible_connection: local

Command (ansible):

$ ansible-playbook -i inventory main.yml

PLAY [target] *************************************************************************************************************

TASK [from subtask 2 do something] ****************************************************************************************
ok: [localhost] => {}

MSG:

Something...


PLAY RECAP ****************************************************************************************************************
localhost                  : ok=1    changed=0    unreachable=0    failed=0    skipped=0    rescued=0    ignored=0

Command (ansible-lint):

$ ansible-lint .
WARNING  Listing 1 violation(s) that are fatal
load-failure: [Errno 2] No such file or directory: 'tasks/tasks/subtasks/subtask_2.yml' (filenotfounderror)
tasks/tasks/subtasks/subtask_2.yml:0

You can skip specific rules or tags by adding them to your configuration file:
# .ansible-lint
warn_list:  # or 'skip_list' to silence them completely
  - load-failure  # Failed to load or parse file
Finished with 1 failure(s), 0 warning(s) on 6 files.
Desired Behaviour

Ansible-lint should not report any failure at all for the above defined project, which includes valid import_tasks or include_tasks.

Actual Behaviour

Ansible-lint reports a false positive failure - load-failure due to incorrectly evaluated file path(s).

@europ europ added the bug label Mar 8, 2021
@webknjaz
Copy link
Member

webknjaz commented Mar 8, 2021

Could you please just include the important bits in one place instead of referring to explanations in other places that have too much unrelated context that people would just get lost in what you're trying to communicate? We need to know what's happening, what's expected, why this is expected, separately from some other references to similar issues that may have been fixed by now. Sending people to follow a chain of redirects is not helpful because you put the cognitive load of trying to understand what's going on on every reader who tries to follow what you're talking about.

@europ europ changed the title false positive load-failure false positive load-failure when using import_tasks/include_tasks Mar 8, 2021
@europ europ changed the title false positive load-failure when using import_tasks/include_tasks false positive load-failure when using import_tasks / include_tasks Mar 8, 2021
@europ
Copy link
Contributor Author

europ commented Mar 8, 2021

fixed

@webknjaz webknjaz changed the title false positive load-failure when using import_tasks / include_tasks False positive load-failure when using import_tasks / include_tasks Mar 8, 2021
@webknjaz
Copy link
Member

webknjaz commented Mar 8, 2021

ansible 2.10.5

is that ansible-base or the one with a bunch collections bundled?

@webknjaz
Copy link
Member

webknjaz commented Mar 8, 2021

OS / ENVIRONMENT

Any.

You may feel like this is not important but it is. You couldn't possibly have tested this on every possible OS+env combo that exists in the world.
The questionnaire is there not to annoy the issue creators but to make the process more efficient. Also, you may have assumptions in your mind but you don't always communicate them verbally because you think that "everyone understands this thing already" (I know this feeling!) but IRL there's always something people don't mention that could be of crucial significance. This info can hint the maintainers about what code paths to look for or how to attempt reproducing it. It's also useful for history. So please try to fill out the form as much as possible because even if you don't have visibility into how this could be useful, this could surface later.

I've seen a lot of cases of people saying "reproducible in any env, there's nothing special about mine" and being surprised to hear "works for me" from the maintainers just because they didn't think to tell about the quirks their env has or some special setting in .bashrc that literally nobody else uses in the whole world.

Urgh... This is getting long but it's something I wanted to get out there because humans seem to operate on assumptions.

europ added a commit to europ/ansible-lint that referenced this issue Mar 8, 2021
europ added a commit to europ/ansible-lint that referenced this issue Mar 8, 2021
@webknjaz
Copy link
Member

webknjaz commented Mar 9, 2021

@ssbarnea could you help Adrián figure out where to stick the breakpoints?

@kimmoahola
Copy link

I also have this issue. And it seems that adding load-failure to the skip_list has no effect. The load-failures are still reported as failures.

@phospi
Copy link

phospi commented May 11, 2021

can confirm that this issue still exists in ansible-lint 5.0.8

@ssbarnea
Copy link
Member

Considering that the 2 tests test_file_path_evaluation are still marked with xfail and failing I think that the issue is still valid. I would really appreciate if someone can address these. You can start by removing the @pytest.mark.xfail line and running the tests.

@webknjaz
Copy link
Member

webknjaz commented Sep 7, 2021

No need to comment with "me too", it's not productive. For as long as Sorin's comment (#1446 (comment)) is not addressed, we know that it's still an issue.
Somebody needs to debug it and unxfail the existing test cases.

@webknjaz webknjaz added reproducer: present This PR or issue contains code, which reproduce the problem described or clearly understandable STR help wanted labels Sep 7, 2021
@webknjaz
Copy link
Member

webknjaz commented Sep 7, 2021

FWIW I'm going to put this into the triage queue to see if it's something that we need to schedule for fixing sooner. But bear in mind that the triage calls are unlikely to get to this during the next month anyway.

@webknjaz webknjaz added the new Triage required label Sep 7, 2021
@relrod relrod assigned relrod and unassigned europ Sep 15, 2021
@kugashia
Copy link

I also have this issue. And it seems that adding load-failure to the skip_list has no effect. The load-failures are still reported as failures.

adding it to warn_list does the trick

warn_list:
- load-failure

@Nebucatnetzer
Copy link

I had troubles with this errors as well.
It turned out in order to get rid of them I had to replace the old include: with include_tasks: and ansible-lint was able to find the task files.

@tobijdc
Copy link

tobijdc commented Jan 10, 2022

Analysis
(only scripting experience with python, so significant chance that I am wrong here)
So I gave this a try on main be43b50 (basically 5.3.2).
It seems that ansible-lint does not track the original playbook from which it starts to follow the includes, it only remembers the parent of a Lintable.
When reading through some ansible docs, the-magic-of-local-paths says the playbookdir is being searched and special_variables > playbook_dir says that playbook_dir is the directory of the playbook provided to ansible-playbook command.

Possible Solution
So I come to the conclusion, that it might be necessary to track "root" playbook somehow and utils.py#_include_children should use this "root" playbook_dir (as fallback) to try to resolve paths.
(Or maybe utils.py#_set_collections_basedir should not be set to current dir, when we got there by include/import? 🤷‍♂️)
This however would be a quite substantial change (as far as I can see).

Workaround
Something that could make this bug much more bearable, would be to report it on the line of the include/import and not on line 0 of the missing file (we know in utils.py#_include_children by not os.path.exists, that we have an issue).
Why? Because then load-failure false positives could be ignored inline and not globally "ignored" by include it on the warn_list (which I am reluctant to do, because load-failure has saved me in the past 😄).

Why I might be wrong
My personal problem is with ../ path traversal (like this person), which I am not sure if it would be solved with the proposed possible solution, which makes me worry the root cause might be something else entirely.
For reference my error looks like this:
load-failure: [Errno 2] No such file or directory: "/<project_dir>/playbooks/{'file': '../roles/<role_name>/tasks/<partial_filename>.yml'}" (filenotfounderror) (which seems correct, if traversal would be done properly)

I hope this helps to narrow the issue down, for someone with more python/project experience.

@suukit
Copy link

suukit commented Feb 23, 2022

ansible-lint 5.4.0 using ansible 2.9.15

While checking roles I found another variant of this issue, when having a role like this:

.
├── tasks
│   ├── f1
│   │   ├── f1.yml
│   │   └── f3.yml
│   ├── f2
│   │   └── f2.yml
│   └── main.yml

with tasks/f1/f1.yaml like:

---
- name: f2f2
  include_tasks: f2/f2.yml
  
- name: f1f3
  include_tasks: f1/f3.yml

- block:
  - name: f1f3
    include_tasks: f1/f3.yml

This produces an error for the include_tasks within the block but not for the once above. The above mentioned problem, that the error is reported at line 1 of the "non existing" file also means inline disable via # noqa is not working. Even starting ansible-lint with ansible-lint -x load-failure . results in the load failure and and exit code > 0.

@javanaut-de
Copy link

Since not even -x load-failure works, is this the only way to get ansible-lint 5.4.0 to "work" with project scope taskbooks in some kind?

[[ -z $(ansible-lint --nocolor | sed '/load-failure/,+1 d') ]] && echo 'ok' || echo 'failed'

@ssbarnea
Copy link
Member

@relrod Did you had any chance to work on this one? Maybe someone else can attempt to make a pull request to fix it?

@ssbarnea ssbarnea removed the new Triage required label Mar 18, 2022
@gtirloni
Copy link

Confirmed with ansible-lint 5.4.0 and Ansible 2.9.27. None of the workarounds actually work here.

Please let me know if you need any help reproducing this issue.

@ssbarnea
Copy link
Member

Sorry but unless you can reproduce it with latest release 6.2.1, i will have to close it. It should also be noted that use of imports that use .. are almost always a sign of bad design.

@ryaner
Copy link
Contributor

ryaner commented May 20, 2022

Repo using the original zip file

# ansible-playbook -i inventory main.yml

PLAY [target] ****************************************************************************

TASK [from subtask 2 do something] *******************************************************
ok: [localhost] => {}

MSG:

Something...


PLAY RECAP *******************************************************************************
localhost                  : ok=1    changed=0    unreachable=0    failed=0    skipped=0    rescued=0    ignored=0

# ansible-lint .
WARNING  Listing 2 violation(s) that are fatal
fqcn-builtins: Use FQCN for builtin actions.
tasks/subtasks/subtask_2.yml:2 Task/Handler: from subtask 2 do something

load-failure: [Errno 2] No such file or directory: '/etc/lint_1446/ansible-lint-bug/tasks/tasks/subtasks/subtask_2.yml' (load-failure[filenotfounderror])
tasks/tasks/subtasks/subtask_2.yml:1

You can skip specific rules or tags by adding them to your configuration file:
# .config/ansible-lint.yml
warn_list:  # or 'skip_list' to silence them completely
  - fqcn-builtins  # Use FQCN for builtin actions.
  - load-failure  # Failed to load or parse file.

Finished with 2 failure(s), 0 warning(s) on 6 files.

This is on a Debian 11 vm

 # pip3 list | grep ansible
ansible            5.8.0
ansible-compat     2.0.4
ansible-core       2.12.5
ansible-lint       6.2.1

@gtirloni
Copy link

@ryaner thank you for reproducing issue with 6.2.1

CC @ssbarnea

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug help wanted reproducer: present This PR or issue contains code, which reproduce the problem described or clearly understandable STR
Projects
None yet
Development

Successfully merging a pull request may close this issue.