Repository: ansible/ansible
Pull request: ansible-galaxy - fix collection installation with trailing slashes

##### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->

If we fail to find a member when extracting a directory, try adding a trailing slash to the member name. In certain cases, the member in the tarfile will contain a trailing slash but the file name in FILES.json will never contain the trailing slash.

If unable to find the member, handle the KeyError and print a nicer error.

Also check if a directory exists before creating it since it may have been extracted from the archive.

Fixes #70009
<!--- 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
<!--- Write the short name of the module, plugin, task or feature below -->
`lib/ansible/galaxy/collection.py`
##### ADDITIONAL INFORMATION

Not sure we need a changelog since this was just added yesterday.

I'm open to suggestions for tests on this. It would be much easier to write unit tests rather than integration tests since I'm not sure how to create tar file where some members contain a trailing slash. If unit tests are ok, I'll work on adding those.


Problem statement:
`ansible-galaxy collection install` explodes for some collections on `devel`
#### SUMMARY
<!--- Explain the problem briefly below -->
Installing `amazon.aws` collection fails on ansible/ansible@devel, supposedly because of the embedded symlinks:
```console
ERROR! Unexpected Exception, this is probably a bug: "filename 'tests/unit/mod
ules/placebo_recordings/aws_direct_connect_link_aggregation_group/nonexistent_
lag_status' not found"
```

##### ISSUE TYPE
- Bug Report

##### COMPONENT NAME
<!--- Write the short name of the module, plugin, task or feature below, use your best guess if unsure -->
ansible-galaxy CLI

##### ANSIBLE VERSION
<!--- Paste verbatim output from "ansible --version" between quotes -->
devel

##### CONFIGURATION
<!--- Paste verbatim output from "ansible-config dump --only-changed" between quotes -->
N/A

##### OS / ENVIRONMENT
<!--- Provide all relevant information below, e.g. target OS versions, network device firmware, etc. -->
N/A

##### STEPS TO REPRODUCE
<!--- Describe exactly how to reproduce the problem, using a minimal test-case -->
Running the install should be enough:
<!--- Paste example playbooks or commands between quotes below -->
```console
$ ANSIBLE_DEBUG=1 ansible-galaxy -vvvv collection install amazon.aws
```

##### EXPECTED RESULTS
<!--- Describe what you expected to happen when running the steps above -->
The collection install should succeed.

##### ACTUAL RESULTS
<!--- Describe what actually happened. If possible run with extra verbosity (-vvvv) -->

<!--- Paste verbatim command output between quotes -->
```python-traceback
Exception, this is probably a bug: "filename 'tests/unit/modules/placebo_recordings/aws_direct_connect_link_aggregation_group/nonexistent_lag_status' not found"
the full traceback was:
Traceback (most recent call last):
  File "/home/bcoca/work/ansible/bin/ansible-galaxy", line 123, in <module>
    exit_code = cli.run()
  File "/home/bcoca/work/ansible/lib/ansible/cli/galaxy.py", line 490, in run
    context.CLIARGS['func']()
  File "/home/bcoca/work/ansible/lib/ansible/cli/galaxy.py", line 1057, in execute_install
    self._execute_install_collection(collection_requirements, collection_path)
  File "/home/bcoca/work/ansible/lib/ansible/cli/galaxy.py", line 1079, in _execute_install_collection
    no_deps, force, force_with_deps, allow_pre_release=allow_pre_release)
  File "/home/bcoca/work/ansible/lib/ansible/galaxy/collection.py", line 675, in install_collections
    collection.install(output_path, b_temp_path)
  File "/home/bcoca/work/ansible/lib/ansible/galaxy/collection.py", line 250, in install
    self.install_artifact(b_collection_path, b_temp_path)
  File "/home/bcoca/work/ansible/lib/ansible/galaxy/collection.py", line 275, in install_artifact
    _extract_tar_dir(collection_tar, file_name, b_collection_path)
  File "/home/bcoca/work/ansible/lib/ansible/galaxy/collection.py", line 1387, in _extract_tar_dir
    tar_member = tar.getmember(to_native(dirname, errors='surrogate_or_strict'))
  File "/usr/lib/python3.7/tarfile.py", line 1752, in getmember
    raise KeyError("filename %r not found" % name)
KeyError: "filename 'tests/unit/modules/placebo_recordings/aws_direct_connect_link_aggregation_group/nonexistent_lag_status' not found"
```

Hints from issue discussion:
bisect: d30fc6c0b359f631130b0e979d9a78a7b3747d48 is the first bad commit
So we should probably cc @jborean93.
As @sivel pointed out, the issue is the actual member in the tar file contains a trailing slash while the call does not:

```
In [18]: t.getmember('tests/unit/modules/placebo_recordings/aws_direct_connect_link_aggregation_group/nonexistent_lag_status/')
Out[18]: <TarInfo 'tests/unit/modules/placebo_recordings/aws_direct_connect_link_aggregation_group/nonexistent_lag_status/' at 0x10c7b4400>
```

The path in `FILES.json` does not contain the trailing slash, which is where the call is coming from.

```json
  {
   "ftype": "dir", 
   "chksum_sha256": null, 
   "name": "tests/unit/modules/placebo_recordings/aws_direct_connect_link_aggregation_group/nonexistent_lag_status", 
   "chksum_type": null, 
   "format": 1
  }, 
```
What's curious is there are no paths in the `FILES.json` that end with `/` but the archive itself contains some members with trailing slashes:

```
tests/unit/modules/placebo_recordings/aws_direct_connect_link_aggregation_group/nonexistent_lag_status/
tests/unit/modules/placebo_recordings/aws_direct_connect_link_aggregation_group/lag_exists_using_name/
tests/unit/modules/placebo_recordings/aws_direct_connect_link_aggregation_group/delete_lag_with_connections/
tests/unit/modules/placebo_recordings/aws_direct_connect_link_aggregation_group/delete_nonexistent_lag/
tests/unit/modules/placebo_recordings/aws_direct_connect_link_aggregation_group/delete_lag_with_connections_without_force_delete/
tests/unit/modules/placebo_recordings/aws_direct_connect_link_aggregation_group/nonexistent_lag_does_not_exist/
```

I wonder why only some dirs in the archive contain the trailing slash.
I know this tarfile is a bit old. I'd be curious to know the Ansible version used to generate it since I generated a new tarfile with `ansible-galaxy collection build ansible.aws` using `devel` and the commit before https://github.com/ansible/ansible/commit/d30fc6c0b359f631130b0e979d9a78a7b3747d48 and didn't get any tarfile members with trailing slashes. 😕 
> I wonder why only some dirs in the archive contain the trailing slash.

Maybe it's symlinks vs real dirs?
Could be. I would like to look at the code at the time the tarfile was created. I didn't see any tags in `ansible-collections/ansible.aws`.
> I didn't see any tags in `ansible-collections/ansible.aws`.

Yep, that's what confused me.
I just built the archive with 2.9.9 and didn't get and dirs with trailing slashes. I wonder if it could be symlinks that pointed to references with a trailing slash.

```
lrwxr-xr-x   1 sdoran  staff    13B Jun 11 12:20 test@ -> data_pipeline
lrwxr-xr-x   1 sdoran  staff    12B Jun 11 12:20 test2@ -> ec2_vpc_vpn/
```

I'll test.
No, still don't get members with trailing slashes (created archive with `ansible-galaxy collection build amazon.aws` on Ansible 2.9.9).

```
In [91]: t_with_links.getmember('tests/unit/modules/placebo_recordings/test')
Out[91]: <TarInfo 'tests/unit/modules/placebo_recordings/test' at 0x10dab0ac0>

In [92]: t_with_links.getmember('tests/unit/modules/placebo_recordings/test2')
Out[92]: <TarInfo 'tests/unit/modules/placebo_recordings/test2' at 0x10dadadc0>
```

Commit message: Add unit tests

Patch to review:
diff --git a/lib/ansible/galaxy/collection.py b/lib/ansible/galaxy/collection.py
index 72e4d9a42383ca..7b54b2c28581f8 100644
--- a/lib/ansible/galaxy/collection.py
+++ b/lib/ansible/galaxy/collection.py
@@ -1384,7 +1384,19 @@ def _download_file(url, b_path, expected_hash, validate_certs, headers=None):
 
 def _extract_tar_dir(tar, dirname, b_dest):
     """ Extracts a directory from a collection tar. """
-    tar_member = tar.getmember(to_native(dirname, errors='surrogate_or_strict'))
+    try:
+        tar_member = tar.getmember(to_native(dirname, errors='surrogate_or_strict'))
+    except KeyError:
+        # Some archives contain directories that end with '/'
+        if not dirname.endswith(os.path.sep):
+            dirname += os.path.sep
+
+        try:
+            tar_member = tar.getmember(to_native(dirname, errors='surrogate_or_strict'))
+        except KeyError:
+            # If we still can't find the member to extract, raise a nice error.
+            raise AnsibleError("Unable to extract '%s' from collection" % to_native(dirname, errors='surrogate_or_strict'))
+
     b_dir_path = os.path.join(b_dest, to_bytes(dirname, errors='surrogate_or_strict'))
 
     b_parent_path = os.path.dirname(b_dir_path)
@@ -1403,7 +1415,8 @@ def _extract_tar_dir(tar, dirname, b_dest):
         os.symlink(b_link_path, b_dir_path)
 
     else:
-        os.mkdir(b_dir_path, 0o0755)
+        if not os.path.isdir(b_dir_path):
+            os.mkdir(b_dir_path, 0o0755)
 
 
 def _extract_tar_file(tar, filename, b_dest, b_temp_path, expected_hash=None):
diff --git a/test/units/cli/galaxy/test_collection_extract_tar.py b/test/units/cli/galaxy/test_collection_extract_tar.py
new file mode 100644
index 00000000000000..da903159b33f4b
--- /dev/null
+++ b/test/units/cli/galaxy/test_collection_extract_tar.py
@@ -0,0 +1,52 @@
+#!/usr/bin/python
+# -*- coding: utf-8 -*-
+# Copyright (c) 2020 Ansible Project
+# GNU General Public License v3.0+ (see COPYING or https://www.gnu.org/licenses/gpl-3.0.txt)
+
+from __future__ import absolute_import, division, print_function
+__metaclass__ = type
+
+import pytest
+
+from ansible.errors import AnsibleError
+from ansible.galaxy.collection import _extract_tar_dir
+
+
+@pytest.fixture
+def fake_tar_obj(mocker):
+    m_tarfile = mocker.Mock()
+    m_tarfile.type = mocker.Mock(return_value=b'99')
+    m_tarfile.SYMTYPE = mocker.Mock(return_value=b'22')
+
+    return m_tarfile
+
+
+def test_extract_tar_bad_member(mocker):
+    m_tarfile = mocker.Mock()
+    m_tarfile.getmember = mocker.Mock(side_effect=KeyError)
+
+    with pytest.raises(AnsibleError, match='Unable to extract'):
+        _extract_tar_dir(m_tarfile, '/some/dir', b'/some/dest')
+
+    assert m_tarfile.getmember.call_count == 2
+
+
+def test_extract_tar_dir_exists(mocker, fake_tar_obj):
+    mocker.patch('os.makedirs', return_value=None)
+    m_makedir = mocker.patch('os.mkdir', return_value=None)
+    mocker.patch('os.path.isdir', return_value=True)
+
+    _extract_tar_dir(fake_tar_obj, '/some/dir', b'/some/dest')
+
+    assert not m_makedir.called
+
+
+def test_extract_tar_dir_does_not_exist(mocker, fake_tar_obj):
+    mocker.patch('os.makedirs', return_value=None)
+    m_makedir = mocker.patch('os.mkdir', return_value=None)
+    mocker.patch('os.path.isdir', return_value=False)
+
+    _extract_tar_dir(fake_tar_obj, '/some/dir', b'/some/dest')
+
+    assert m_makedir.called
+    assert m_makedir.call_args[0] == (b'/some/dir', 0o0755)


Code review comments:
diff_hunk: @@ -1384,7 +1384,19 @@ def _download_file(url, b_path, expected_hash, validate_certs, headers=None):
 
 def _extract_tar_dir(tar, dirname, b_dest):
     """ Extracts a directory from a collection tar. """
-    tar_member = tar.getmember(to_native(dirname, errors='surrogate_or_strict'))
+    try:
+        tar_member = tar.getmember(to_native(dirname, errors='surrogate_or_strict'))
+    except KeyError:
+        # Some archives contain directories that end with '/'
+        if not dirname.endswith(os.path.sep):
+            dirname += os.path.sep
+
+        try:
+            tar_member = tar.getmember(to_native(dirname, errors='surrogate_or_strict'))
+        except KeyError:
+            # If we still can't find the member to extract, raise a nice error.
+            raise AnsibleError("Unable to extract '%s' from collection" % to_native(dirname, errors='surrogate_or_strict'))
+
path: lib/ansible/galaxy/collection.py
text: @user1:
I don't want to hold up the PR but you could use a loop instead of a nested KeyError like

```suggestion
    member_names = [to_native(dirname, errors='surrogate_or_strict')]
    if not member_names[-1].endswith(os.path.sep):
        member_names.append(member_names[-1] + os.path.sep)
        
    for name in member_names:
        try:
            tar_member = tar.getmember(name)
        except KeyError:
            continue
        break
    else:
        raise AnsibleError("Unable to extract '%s' from collection" % member_names[0])
```

Potentially allows us to try more options in the future if need be.

@author:
I need to head out for today. I will try to poke at this a bit tomorrow even though it's technically a holiday. If not, I will get to it on Monday.

@author:
I ended up going with this solution. I'm trying to work out why it's not breaking out of the loop and adjusting the tests.

@author:
Found my bug. 😊

diff_hunk: @@ -1384,7 +1384,19 @@ def _download_file(url, b_path, expected_hash, validate_certs, headers=None):
 
 def _extract_tar_dir(tar, dirname, b_dest):
     """ Extracts a directory from a collection tar. """
-    tar_member = tar.getmember(to_native(dirname, errors='surrogate_or_strict'))
+    try:
+        tar_member = tar.getmember(to_native(dirname, errors='surrogate_or_strict'))
+    except KeyError:
+        # Some archives contain directories that end with '/'
+        if not dirname.endswith(os.path.sep):
+            dirname += os.path.sep
path: lib/ansible/galaxy/collection.py
text: @author:
Good point. Most of the items do not have a trailing slash. There's room for optimization here. I may end up doing a loop as @user1 suggested below.