Repository: ansible/ansible
Pull request: Filter BLACKLIST_EXTS in PluginLoader
##### SUMMARY

(Note this revives https://github.com/ansible/ansible/pull/27235)

Change:
Rather than hardcoding .pyo and .pyc, filter on all BLACKLIST_EXTS in
the non-legacy logic of PluginLoader (_find_fq_plugin). The two harcoded
extensions are part of BLACKLIST_EXTS already and this simply adds the
rest of the blacklisted extensions to the check.

In addition, check .endswith() instead of an exact match of the suffix,
like everywhere else that uses BLACKLIST_EXTS. This allows for
blacklisting, for example, emacs's backup files which can appear after
any extension, leading to things like `foo.py~`.

Test Plan:
Ran `ansible-playbook` against a collection where a `foo.py~` module was
getting executed instead of `foo.py` which also appeared in the same
directory. `foo.py~` is no longer executed.

Tickets:
Fixes #22268
Refs #27235

Signed-off-by: Rick Elrod <rick@elrod.me>

<!--- Describe the change below, including rationale and design decisions -->

<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->

##### ISSUE TYPE
<!--- Pick one below and delete the rest -->
- Bugfix Pull Request

##### COMPONENT NAME

plugin loader

Problem statement:
Wrong file used as module
<!---
Verify first that your issue/request is not already reported on GitHub.
Also test if the latest release, and master branch are affected too.
-->

##### ISSUE TYPE
<!--- Pick one below and delete the rest: -->
 - Feature Idea

##### COMPONENT NAME
<!--- Name of the module/plugin/task/feature -->

##### ANSIBLE VERSION
<!--- Paste verbatim output from “ansible --version” between quotes below -->
```
ansible 2.3.0 (devel 7018aa3316) last updated 2017/03/03 14:56:50 (GMT -400)
```

##### CONFIGURATION
<!---
Mention any settings you have changed/added/removed in ansible.cfg
(or using the ANSIBLE_* environment variables).
-->

##### OS / ENVIRONMENT
<!---
Mention the OS you are running Ansible from, and the OS you are
managing, or say “N/A” for anything that is not platform-specific.
-->
linux

##### SUMMARY
<!--- Explain the problem briefly -->
An emacs backup file `aws.py~` in the module directory is used in preference to a sibling file called `aws`.

##### STEPS TO REPRODUCE
<!---
For bugs, show exactly how to reproduce the problem, using a minimal test-case.
For new features, show how the feature would be used.
-->
```
mkdir ansible-bug
cd ansible-bug
cat > playbook.yaml <<EOF
---
- hosts: localhost
  connection: local

  tasks:
    - aws: {}
EOF
cat > aws <<EOF
#!/usr/bin/python
from ansible.module_utils.basic import AnsibleModule
module = AnsibleModule({})
module.exit_json(changed=False)
EOF
cat > aws.py~ <<EOF
#!/usr/bin/python
from ansible.module_utils.basic import AnsibleModule
module = AnsibleModule({})
module.exit_json(changed=True)
EOF
ansible-playbook -vvvv -i localhost, -M . playbook.yaml
```

##### EXPECTED RESULTS
<!--- What did you expect to happen when running the steps above? -->
`changed=0`

##### ACTUAL RESULTS
<!--- What actually happened? If possible run with extra verbosity (-vvvv) -->
`changed=1`
<!--- Paste verbatim command output between quotes below -->
```
(conf-mgmt) aaron:ansible-bug> ansible-playbook -vvvv -i localhost, -M . playbook.yaml
No config file found; using defaults
Set default localhost to localhost
Loading callback plugin default of type stdout, v2.0 from /home/aaron/Dropbox/newscred-src/ansible/lib/ansible/plugins/callback/__init__.pyc

PLAYBOOK: playbook.yaml *************************************************************************************************************
1 plays in playbook.yaml

PLAY [localhost] ********************************************************************************************************************

TASK [Gathering Facts] **************************************************************************************************************
Using module file /home/aaron/Dropbox/newscred-src/ansible/lib/ansible/modules/system/setup.py
<localhost> ESTABLISH LOCAL CONNECTION FOR USER: aaron
<localhost> EXEC /bin/sh -c 'echo ~ && sleep 0'
<localhost> EXEC /bin/sh -c '( umask 77 && mkdir -p "` echo /home/aaron/.ansible/tmp/ansible-tmp-1488571336.12-276444930035913 `" && echo ansible-tmp-1488571336.12-276444930035913="` echo /home/aaron/.ansible/tmp/ansible-tmp-1488571336.12-276444930035913 `" ) && sleep 0'
<localhost> PUT /tmp/tmpb2iKjX TO /home/aaron/.ansible/tmp/ansible-tmp-1488571336.12-276444930035913/setup.py
<localhost> EXEC /bin/sh -c 'chmod u+x /home/aaron/.ansible/tmp/ansible-tmp-1488571336.12-276444930035913/ /home/aaron/.ansible/tmp/ansible-tmp-1488571336.12-276444930035913/setup.py && sleep 0'
<localhost> EXEC /bin/sh -c '/usr/bin/python /home/aaron/.ansible/tmp/ansible-tmp-1488571336.12-276444930035913/setup.py; rm -rf "/home/aaron/.ansible/tmp/ansible-tmp-1488571336.12-276444930035913/" > /dev/null 2>&1 && sleep 0'
ok: [localhost]
META: ran handlers

TASK [aws] **************************************************************************************************************************
task path: /home/aaron/scratch/ansible-bug/ansible-bug/playbook.yaml:6
Using module file /home/aaron/scratch/ansible-bug/ansible-bug/aws.py~
<localhost> ESTABLISH LOCAL CONNECTION FOR USER: aaron
<localhost> EXEC /bin/sh -c 'echo ~ && sleep 0'
<localhost> EXEC /bin/sh -c '( umask 77 && mkdir -p "` echo /home/aaron/.ansible/tmp/ansible-tmp-1488571336.5-232883384061572 `" && echo ansible-tmp-1488571336.5-232883384061572="` echo /home/aaron/.ansible/tmp/ansible-tmp-1488571336.5-232883384061572 `" ) && sleep 0'
<localhost> PUT /tmp/tmp5HzZiD TO /home/aaron/.ansible/tmp/ansible-tmp-1488571336.5-232883384061572/aws.py~
<localhost> EXEC /bin/sh -c 'chmod u+x /home/aaron/.ansible/tmp/ansible-tmp-1488571336.5-232883384061572/ '"'"'/home/aaron/.ansible/tmp/ansible-tmp-1488571336.5-232883384061572/aws.py~'"'"' && sleep 0'
<localhost> EXEC /bin/sh -c '/usr/bin/python '"'"'/home/aaron/.ansible/tmp/ansible-tmp-1488571336.5-232883384061572/aws.py~'"'"'; rm -rf "/home/aaron/.ansible/tmp/ansible-tmp-1488571336.5-232883384061572/" > /dev/null 2>&1 && sleep 0'
changed: [localhost] => {
    "changed": true, 
    "invocation": {
        "module_args": {}
    }
}
META: ran handlers
META: ran handlers

PLAY RECAP **************************************************************************************************************************
localhost                  : ok=2    changed=1    unreachable=0    failed=0   
```

Note that when I originally tried to build this test case, I called the module files `my_module` and `my_module.py~` instead of `aws` and `aws.py~`, and it failed to demonstrate the bug. My guess is that it has something to do with the way filenames are hashed by the filesystem -- ansible-playbook uses whichever one comes first in the directory listing. If that's the case then the test might work differently for someone using a different filesystem. As a variant that shouldn't depend on filesystem implementation, try deleting the file `aws`. Then ansible-playbook unambiguously uses `aws.py~` as a module, which I would argue is incorrect behavior.

Hints from issue discussion:
I did get unchanged result:

```root@ubuntu-xenial:~/ansible-bug# cat > aws <<EOF
> #!/usr/bin/python
> from ansible.module_utils.basic import AnsibleModule
> module = AnsibleModule({})
> module.exit_json(changed=False)
> EOF
root@ubuntu-xenial:~/ansible-bug# cat > aws.py~ <<EOF
> #!/usr/bin/python
> from ansible.module_utils.basic import AnsibleModule
> module = AnsibleModule({})
> module.exit_json(changed=True)
> EOF
root@ubuntu-xenial:~/ansible-bug# ansible-playbook -vvvv -i localhost, -M . playbook.yaml
No config file found; using defaults
Set default localhost to localhost
Loading callback plugin default of type stdout, v2.0 from /home/ubuntu/ansible-1/lib/ansible/plugins/callback/__init__.pyc

PLAYBOOK: playbook.yaml ***********************************************************************************************************************************************************************************
1 plays in playbook.yaml

PLAY [localhost] ******************************************************************************************************************************************************************************************

TASK [Gathering Facts] ************************************************************************************************************************************************************************************
Using module file /home/ubuntu/ansible-1/lib/ansible/modules/system/setup.py
<localhost> ESTABLISH LOCAL CONNECTION FOR USER: root
<localhost> EXEC /bin/sh -c 'echo ~ && sleep 0'
<localhost> EXEC /bin/sh -c '( umask 77 && mkdir -p "` echo /home/ubuntu/.ansible/tmp/ansible-tmp-1488574383.17-246183316484828 `" && echo ansible-tmp-1488574383.17-246183316484828="` echo /home/ubuntu/.ansible/tmp/ansible-tmp-1488574383.17-246183316484828 `" ) && sleep 0'
<localhost> PUT /tmp/tmpPQR6QF TO /home/ubuntu/.ansible/tmp/ansible-tmp-1488574383.17-246183316484828/setup.py
<localhost> EXEC /bin/sh -c 'chmod u+x /home/ubuntu/.ansible/tmp/ansible-tmp-1488574383.17-246183316484828/ /home/ubuntu/.ansible/tmp/ansible-tmp-1488574383.17-246183316484828/setup.py && sleep 0'
<localhost> EXEC /bin/sh -c '/usr/bin/python /home/ubuntu/.ansible/tmp/ansible-tmp-1488574383.17-246183316484828/setup.py; rm -rf "/home/ubuntu/.ansible/tmp/ansible-tmp-1488574383.17-246183316484828/" > /dev/null 2>&1 && sleep 0'
ok: [localhost]
META: ran handlers

TASK [aws] ************************************************************************************************************************************************************************************************
task path: /home/ubuntu/ansible-bug/playbook.yaml:6
Using module file /home/ubuntu/ansible-bug/aws
<localhost> ESTABLISH LOCAL CONNECTION FOR USER: root
<localhost> EXEC /bin/sh -c 'echo ~ && sleep 0'
<localhost> EXEC /bin/sh -c '( umask 77 && mkdir -p "` echo /home/ubuntu/.ansible/tmp/ansible-tmp-1488574383.59-280078461272337 `" && echo ansible-tmp-1488574383.59-280078461272337="` echo /home/ubuntu/.ansible/tmp/ansible-tmp-1488574383.59-280078461272337 `" ) && sleep 0'
<localhost> PUT /tmp/tmpEXvnkA TO /home/ubuntu/.ansible/tmp/ansible-tmp-1488574383.59-280078461272337/aws
<localhost> EXEC /bin/sh -c 'chmod u+x /home/ubuntu/.ansible/tmp/ansible-tmp-1488574383.59-280078461272337/ /home/ubuntu/.ansible/tmp/ansible-tmp-1488574383.59-280078461272337/aws && sleep 0'
<localhost> EXEC /bin/sh -c '/usr/bin/python /home/ubuntu/.ansible/tmp/ansible-tmp-1488574383.59-280078461272337/aws; rm -rf "/home/ubuntu/.ansible/tmp/ansible-tmp-1488574383.59-280078461272337/" > /dev/null 2>&1 && sleep 0'
ok: [localhost] => {
    "changed": false,
    "invocation": {
        "module_args": {}
    }
}
META: ran handlers
META: ran handlers

PLAY RECAP ************************************************************************************************************************************************************************************************
localhost                  : ok=2    changed=0    unreachable=0    failed=0
```
OK, so it's tricky to reproduce, but as I said, if you remove the file `aws` you should be able to see that `aws.py~` is used as a module.
Got it. I will look into it now.

Commit message: lint: use {0} instead of {}

Signed-off-by: Rick Elrod <rick@elrod.me>

Patch to review:
diff --git a/lib/ansible/config/base.yml b/lib/ansible/config/base.yml
index dfb5fd136db809..457d110b148e43 100644
--- a/lib/ansible/config/base.yml
+++ b/lib/ansible/config/base.yml
@@ -1588,7 +1588,7 @@ INVENTORY_EXPORT:
   type: bool
 INVENTORY_IGNORE_EXTS:
   name: Inventory ignore extensions
-  default: "{{(BLACKLIST_EXTS + ( '.orig', '.ini', '.cfg', '.retry'))}}"
+  default: "{{(BLACKLIST_EXTS + ('.orig', '.ini', '.cfg', '.retry'))}}"
   description: List of extensions to ignore when using a directory as an inventory source
   env: [{name: ANSIBLE_INVENTORY_IGNORE}]
   ini:
@@ -1648,6 +1648,14 @@ INJECT_FACTS_AS_VARS:
   - {key: inject_facts_as_vars, section: defaults}
   type: boolean
   version_added: "2.5"
+MODULE_IGNORE_EXTS:
+  name: Module ignore extensions
+  default: "{{(BLACKLIST_EXTS + ('.yaml', '.yml', '.ini'))}}"
+  description: List of extensions to ignore when looking for modules to load
+  env: [{name: ANSIBLE_MODULE_IGNORE_EXTS}]
+  ini:
+  - {key: module_ignore_extensions, section: defaults}
+  type: list
 OLD_PLUGIN_CACHE_CLEARING:
   description: Previouslly Ansible would only clear some of the plugin loading caches when loading new roles, this led to some behaviours in which a plugin loaded in prevoius plays would be unexpectedly 'sticky'. This setting allows to return to that behaviour.
   env: [{name: ANSIBLE_OLD_PLUGIN_CACHE_CLEAR}]
diff --git a/lib/ansible/plugins/loader.py b/lib/ansible/plugins/loader.py
index e6d697a62cd334..fb61fd2084cda7 100644
--- a/lib/ansible/plugins/loader.py
+++ b/lib/ansible/plugins/loader.py
@@ -347,8 +347,9 @@ def _find_fq_plugin(self, fq_name, extension):
             return full_name, to_text(n_resource_path)
 
         # look for any matching extension in the package location (sans filter)
-        ext_blacklist = ['.pyc', '.pyo']
-        found_files = [f for f in glob.iglob(os.path.join(pkg_path, n_resource) + '.*') if os.path.isfile(f) and os.path.splitext(f)[1] not in ext_blacklist]
+        found_files = [f
+                       for f in glob.iglob(os.path.join(pkg_path, n_resource) + '.*')
+                       if os.path.isfile(f) and not f.endswith(C.MODULE_IGNORE_EXTS)]
 
         if not found_files:
             return None, None
@@ -448,7 +449,7 @@ def _find_plugin_legacy(self, name, ignore_deprecated=False, check_aliases=False
                 # HACK: We have no way of executing python byte compiled files as ansible modules so specifically exclude them
                 # FIXME: I believe this is only correct for modules and module_utils.
                 # For all other plugins we want .pyc and .pyo should be valid
-                if any(full_path.endswith(x) for x in C.BLACKLIST_EXTS):
+                if any(full_path.endswith(x) for x in C.MODULE_IGNORE_EXTS):
                     continue
 
                 splitname = os.path.splitext(full_name)
diff --git a/test/integration/targets/plugin_loader/ignore_exts/ignore_exts.yml b/test/integration/targets/plugin_loader/ignore_exts/ignore_exts.yml
new file mode 100644
index 00000000000000..27c56fa46c5191
--- /dev/null
+++ b/test/integration/targets/plugin_loader/ignore_exts/ignore_exts.yml
@@ -0,0 +1,18 @@
+- hosts: testhost
+  gather_facts: false
+  tasks:
+    - action: demoignore
+      args:
+        name: foo
+      ignore_errors: yes
+      register: ignored
+
+    - demo:
+        name: foo
+      register: not_ignored
+
+    - assert:
+        that:
+          - ignored is failed
+          - ignored.msg == 'The module demoignore was not found in configured module paths'
+          - not_ignored is successful
diff --git a/test/integration/targets/plugin_loader/ignore_exts/library/demo.py b/test/integration/targets/plugin_loader/ignore_exts/library/demo.py
new file mode 100644
index 00000000000000..54764a4d9c1572
--- /dev/null
+++ b/test/integration/targets/plugin_loader/ignore_exts/library/demo.py
@@ -0,0 +1,24 @@
+#!/usr/bin/python
+
+from ansible.module_utils.basic import AnsibleModule
+
+
+def main():
+    module_args = dict(
+        name=dict(type='str', required=True),
+    )
+
+    module = AnsibleModule(
+        argument_spec=module_args,
+        supports_check_mode=False
+    )
+
+    result = {}
+    result['message'] = 'ping, {0}'.format(module.params['name'])
+    result['changed'] = False
+
+    module.exit_json(**result)
+
+
+if __name__ == '__main__':
+    main()
diff --git a/test/integration/targets/plugin_loader/ignore_exts/library/demoignore.ini b/test/integration/targets/plugin_loader/ignore_exts/library/demoignore.ini
new file mode 100644
index 00000000000000..fed3f7f4d91a4c
--- /dev/null
+++ b/test/integration/targets/plugin_loader/ignore_exts/library/demoignore.ini
@@ -0,0 +1,22 @@
+#!/usr/bin/python
+
+from ansible.module_utils.basic import AnsibleModule
+
+def main():
+    module_args = dict(
+        name=dict(type='str', required=True),
+    )
+
+    module = AnsibleModule(
+        argument_spec=module_args,
+        supports_check_mode=False
+    )
+
+    result = {}
+    result['message'] = 'i should fail, {}'.format(module.params['name'])
+    result['changed'] = False
+
+    module.exit_json(**result)
+
+if __name__ == '__main__':
+    main()
diff --git a/test/integration/targets/plugin_loader/runme.sh b/test/integration/targets/plugin_loader/runme.sh
index 2a1bdedaca3b54..afc0e05e28613e 100755
--- a/test/integration/targets/plugin_loader/runme.sh
+++ b/test/integration/targets/plugin_loader/runme.sh
@@ -22,3 +22,6 @@ do
 		exit 1
 	fi
 done
+
+
+ansible-playbook ignore_exts/ignore_exts.yml -i ../../inventory -vvv "$@"
diff --git a/test/sanity/ignore.txt b/test/sanity/ignore.txt
index 7ab3628665cd37..f91fa4279d15d8 100644
--- a/test/sanity/ignore.txt
+++ b/test/sanity/ignore.txt
@@ -315,6 +315,9 @@ test/integration/targets/pip/files/ansible_test_pip_chdir/__init__.py future-imp
 test/integration/targets/pip/files/ansible_test_pip_chdir/__init__.py metaclass-boilerplate
 test/integration/targets/pip/files/setup.py future-import-boilerplate
 test/integration/targets/pip/files/setup.py metaclass-boilerplate
+test/integration/targets/plugin_loader/ignore_exts/library/demo.py future-import-boilerplate
+test/integration/targets/plugin_loader/ignore_exts/library/demo.py metaclass-boilerplate
+test/integration/targets/plugin_loader/ignore_exts/library/demoignore.ini shebang
 test/integration/targets/run_modules/library/test.py future-import-boilerplate
 test/integration/targets/run_modules/library/test.py metaclass-boilerplate
 test/integration/targets/script/files/no_shebang.py future-import-boilerplate


Code review comments:
diff_hunk: @@ -1648,6 +1648,14 @@ INJECT_FACTS_AS_VARS:
   - {key: inject_facts_as_vars, section: defaults}
   type: boolean
   version_added: "2.5"
+MODULE_IGNORE_EXTS:
+  name: Module ignore extensions
+  default: "{{(BLACKLIST_EXTS + ('.yaml', '.yml', '.ini'))}}"
+  description: List of extensions to ignore when looking for modules to load
+  env: [{name: ANSIBLE_MODULE_IGNORE_EXTS}]
+  ini:
+  - {key: module_ignore_extensions, section: defaults}
path: lib/ansible/config/base.yml
text: @user1:
I'd go for consistency here with the envvar and actual config item name.

diff_hunk: @@ -315,6 +315,9 @@ test/integration/targets/pip/files/ansible_test_pip_chdir/__init__.py future-imp
 test/integration/targets/pip/files/ansible_test_pip_chdir/__init__.py metaclass-boilerplate
 test/integration/targets/pip/files/setup.py future-import-boilerplate
 test/integration/targets/pip/files/setup.py metaclass-boilerplate
+test/integration/targets/plugin_loader/ignore_exts/library/demo.py future-import-boilerplate
path: test/sanity/ignore.txt
text: @user1:
I think we're usually adding the boilerplate on new tests rather than adding new ignores

@author:
Going to just do this differently and make it part of `module_precedence` tests instead.

diff_hunk: @@ -1648,6 +1648,14 @@ INJECT_FACTS_AS_VARS:
   - {key: inject_facts_as_vars, section: defaults}
   type: boolean
   version_added: "2.5"
+MODULE_IGNORE_EXTS:
+  name: Module ignore extensions
+  default: "{{(BLACKLIST_EXTS + ('.yaml', '.yml', '.ini'))}}"
+  description: List of extensions to ignore when looking for modules to load
path: lib/ansible/config/base.yml
text: @user1:
Maybe just clarify in the description that this is for blacklisting script/binary module fallback extensions. I'd say change the name of the item to be clear that it's for script/binary module extensions to ignore, but I can't think of a reasonably concise name... `SCRIPT_BINARY_MODULE_IGNORE_EXTS`, while accurate and self-describing, feels too verbose.

diff_hunk: @@ -315,6 +315,9 @@ test/integration/targets/pip/files/ansible_test_pip_chdir/__init__.py future-imp
 test/integration/targets/pip/files/ansible_test_pip_chdir/__init__.py metaclass-boilerplate
 test/integration/targets/pip/files/setup.py future-import-boilerplate
 test/integration/targets/pip/files/setup.py metaclass-boilerplate
+test/integration/targets/plugin_loader/ignore_exts/library/demo.py future-import-boilerplate
+test/integration/targets/plugin_loader/ignore_exts/library/demo.py metaclass-boilerplate
+test/integration/targets/plugin_loader/ignore_exts/library/demoignore.ini shebang
path: test/sanity/ignore.txt
text: @user1:
You could just put a bogus shebang in there to keep the test happy