Change the copy_archives method to use copy2

Uses copy2 with follow_symlinks set to False, which will copy
symlinks as symlinks, rather than following their path. Since glob
does not have an option to not follow symlinks, we instead just check
for file existence to ignore issues with globbing symlinked paths
that "open" fails to find.

Issue: RELENG-2576
Change-Id: Ie5a6ffba5c1cb79cff7dd9c691d9b9bd2eaeeba3
Signed-off-by: Eric Ball <eball@linuxfoundation.org>

diff --git a/lftools/deploy.py b/lftools/deploy.py
index 6537e13..a7bcbe5 100755 (executable)
--- a/lftools/deploy.py
+++ b/lftools/deploy.py
@@ -50,9 +50,14 @@ def _compress_text(dir):
         paths.extend(glob.glob(search, recursive=True))
 
     for _file in paths:
-        with open(_file, 'rb') as src, gzip.open('{}.gz'.format(_file), 'wb') as dest:
-            shutil.copyfileobj(src, dest)
-            os.remove(_file)
+        # glob may follow symlink paths that open can't find
+        if os.path.exists(_file):
+            log.debug("Compressing file {}".format(_file))
+            with open(_file, 'rb') as src, gzip.open('{}.gz'.format(_file), 'wb') as dest:
+                shutil.copyfileobj(src, dest)
+                os.remove(_file)
+        else:
+            log.info("Could not open path from glob {}".format(_file))
 
     os.chdir(save_dir)
 
@@ -217,7 +222,7 @@ def copy_archives(workspace, pattern=None):
         :arg str pattern: Space-separated list of Unix style glob patterns.
             (default: None)
     """
-    archives_dir = os.path.join(workspace, 'archives')
+    archives_dir = os.path.join(workspace, "archives")
     dest_dir = os.getcwd()
 
     log.debug('Copying files from {} with pattern \'{}\' to {}.'.format(
@@ -233,11 +238,14 @@ def copy_archives(workspace, pattern=None):
             for file_or_dir in os.listdir(archives_dir):
                 f = os.path.join(archives_dir, file_or_dir)
                 try:
-                    log.debug('Moving {}'.format(f))
-                    shutil.move(f, dest_dir)
+                    log.debug('Copying {}'.format(f))
+                    shutil.copy2(f, dest_dir, follow_symlinks=False)
+                except IsADirectoryError:
+                    os.makedirs(os.path.join(dest_dir, file_or_dir),
+                                exist_ok=True)
                 except shutil.Error as e:
                     log.error(e)
-                    raise OSError(errno.EPERM, 'Could not move to', archives_dir)
+                    raise OSError(errno.EPERM, 'Could not copy to', archives_dir)
     else:
         log.error('Archives dir {} does not exist.'.format(archives_dir))
         raise OSError(errno.ENOENT, 'Missing directory', archives_dir)
@@ -262,6 +270,7 @@ def copy_archives(workspace, pattern=None):
         if len(os.path.basename(src)) > 255:
             log.warn('Filename {} is over 255 characters. Skipping...'.format(
                 os.path.basename(src)))
+            continue
 
         dest = os.path.join(dest_dir, src[len(workspace)+1:])
         log.debug('{} -> {}'.format(src, dest))
@@ -269,9 +278,9 @@ def copy_archives(workspace, pattern=None):
         if os.path.isfile(src):
             try:
                 shutil.move(src, dest)
-            except IOError as e:  # Switch to FileNotFoundError when Python 2 support is dropped.
+            except FileNotFoundError as e:
                 log.debug("Missing path, will create it {}.\n{}".format(os.path.dirname(dest), e))
-                os.makedirs(os.path.dirname(dest))
+                os.makedirs(os.path.dirname(dest), exist_ok=True)
                 shutil.move(src, dest)
         else:
             log.info('Not copying directories: {}.'.format(src))

diff --git a/releasenotes/notes/fix-archive-symlink-loops-0e754059d8db33ba.yaml b/releasenotes/notes/fix-archive-symlink-loops-0e754059d8db33ba.yaml
new file mode 100644 (file)
index 0000000..ae78b11
--- /dev/null
+++ b/releasenotes/notes/fix-archive-symlink-loops-0e754059d8db33ba.yaml
@@ -0,0 +1,8 @@
+---
+fixes:
+  - |
+    Change the copy_archives method used by deploy_archives to use copy2 with
+    follow_symlinks set to False, which will copy symlinks as symlinks, rather
+    than following their path. Since glob does not have an option to not follow
+    symlinks, we instead just check for file existence to ignore issues with
+    globbing symlinked paths that "open" fails to find.

diff --git a/tests/fixtures/deploy/workspace-symlinks/archives/child/parent b/tests/fixtures/deploy/workspace-symlinks/archives/child/parent
new file mode 120000 (symlink)
index 0000000..a96aa0e
--- /dev/null
+++ b/tests/fixtures/deploy/workspace-symlinks/archives/child/parent
@@ -0,0 +1 @@
+..
\ No newline at end of file

diff --git a/tests/fixtures/deploy/workspace-symlinks/archives/non-existent-file b/tests/fixtures/deploy/workspace-symlinks/archives/non-existent-file
new file mode 120000 (symlink)
index 0000000..ef70665
--- /dev/null
+++ b/tests/fixtures/deploy/workspace-symlinks/archives/non-existent-file
@@ -0,0 +1 @@
+non-existent-file
\ No newline at end of file

diff --git a/tests/fixtures/deploy/workspace-symlinks/archives/test.log b/tests/fixtures/deploy/workspace-symlinks/archives/test.log
new file mode 100644 (file)
index 0000000..b3589bf
--- /dev/null
+++ b/tests/fixtures/deploy/workspace-symlinks/archives/test.log
@@ -0,0 +1 @@
+This is a test log.

diff --git a/tests/test_deploy.py b/tests/test_deploy.py
index 77d0a1c..e33ac12 100644 (file)
--- a/tests/test_deploy.py
+++ b/tests/test_deploy.py
@@ -11,6 +11,7 @@
 
 import os
 import sys
+import tempfile
 
 import pytest
 import requests
@@ -74,6 +75,26 @@ def test_copy_archive_dir(cli_runner, datafiles):
     assert os.path.exists(os.path.join(stage_dir, 'test.log'))
 
 
+@pytest.mark.datafiles(
+    os.path.join(FIXTURE_DIR, 'deploy'),
+    )
+def test_copy_archive_dir_symlinks(cli_runner, datafiles):
+    """Test copy_archives() command with symlink loop and a broken symlink, in
+    order to test that these conditions are handled properly."""
+    os.chdir(str(datafiles))
+    workspace_dir = os.path.join(str(datafiles), 'workspace-symlinks')
+    stage_dir = str(datafiles.mkdir("stage_archive"))
+
+    os.chdir(stage_dir)
+    result = cli_runner.invoke(
+        cli.cli,
+        ['--debug', 'deploy', 'copy-archives', workspace_dir],
+        obj={})
+    assert result.exit_code == 0
+
+    assert os.path.exists(os.path.join(stage_dir, 'test.log'))
+
+
 @pytest.mark.datafiles(
     os.path.join(FIXTURE_DIR, 'deploy'),
     )
@@ -132,7 +153,7 @@ def test_deploy_archive(cli_runner, datafiles, responses):
     )
 def test_deploy_archive2(datafiles):
     """Test deploy_archives() command when archives dir is missing."""
-    os.chdir(str(datafiles))
+    os.chdir(tempfile.mkdtemp(prefix='lftools-test.'))
     workspace_dir = os.path.join(str(datafiles), 'workspace-noarchives')
 
     with pytest.raises(OSError) as excinfo:
@@ -145,7 +166,7 @@ def test_deploy_archive2(datafiles):
     )
 def test_deploy_archive3(datafiles):
     """Test deploy_archives() command when archives dir is a file instead of a dir."""
-    os.chdir(str(datafiles))
+    os.chdir(tempfile.mkdtemp(prefix='lftools-test.'))
     workspace_dir = os.path.join(str(datafiles), 'workspace-archivesfile')
 
     with pytest.raises(OSError) as excinfo:
@@ -158,7 +179,7 @@ def test_deploy_archive3(datafiles):
     )
 def test_deploy_archive4(cli_runner, datafiles, responses):
     """Test deploy_archives() command when using duplicated patterns."""
-    os.chdir(str(datafiles))
+    os.chdir(tempfile.mkdtemp(prefix='lftools-test.'))
     workspace_dir = os.path.join(str(datafiles), 'workspace-patternfile')
     pattern=["**/*.log", "**/hs_err_*.log", "**/target/**/feature.xml", "**/target/failsafe-reports/failsafe-summary.xml", "**/target/surefire-reports/*-output.txt", "**/target/surefire-reports/*-output.txt", "**/target/failsafe-reports/failsafe-summary.xml", "**/*"]
     result = deploy_sys.copy_archives(workspace_dir, pattern)