Don't create a hardlink to a symlink when handling file:// URLs

While os.link is supposed to follow symlinks, it's actually broken [1]
on Linux. As a result, Ironic may end up creating a hard link to
a symlink. If the symlink is relative, chances are high accessing the
resulting file will cause a FileNotFoundError.

[1] python/cpython#81793

Change-Id: Ic52f0ddb0c94410dd854ee525e3c57b2e78ea84d

Author: dtantsur

ironic/common/image_service.py

@@ -322,13 +322,21 @@ def download(self, image_href, image_file):
             image_file.close()
             os.remove(dest_image_path)
 
+            # NOTE(dtantsur): os.link is supposed to follow symlinks, but it
+            # does not: https://github.com/python/cpython/issues/81793
+            real_image_path = os.path.realpath(source_image_path)
             try:
-                os.link(source_image_path, dest_image_path)
+                os.link(real_image_path, dest_image_path)
             except OSError as exc:
-                LOG.debug('Could not create a link from %(src)s to %(dest)s, '
-                          'will copy the content instead. Error: %(exc)s.',
+                orig = (f' (real path {real_image_path})'
+                        if real_image_path != source_image_path
+                        else '')
+
+                LOG.debug('Could not create a link from %(src)s%(orig)s to '
+                          '%(dest)s, will copy the content instead. '
+                          'Error: %(exc)s.',
                           {'src': source_image_path, 'dest': dest_image_path,
-                           'exc': exc})
+                           'orig': orig, 'exc': exc})
             else:
                 return
 

ironic/tests/unit/common/test_image_service.py

@@ -612,6 +612,7 @@ def test_show(self, _validate_mock, getsize_mock, getmtime_mock):
 
     @mock.patch.object(shutil, 'copyfile', autospec=True)
     @mock.patch.object(os, 'link', autospec=True)
+    @mock.patch.object(os.path, 'realpath', lambda p: p)
     @mock.patch.object(os, 'remove', autospec=True)
     @mock.patch.object(image_service.FileImageService, 'validate_href',
                        autospec=True)
@@ -642,6 +643,24 @@ def test_download_copy(self, _validate_mock, remove_mock, link_mock,
         link_mock.assert_called_once_with(self.href_path, 'file')
         copy_mock.assert_called_once_with(self.href_path, 'file')
 
+    @mock.patch.object(shutil, 'copyfile', autospec=True)
+    @mock.patch.object(os, 'link', autospec=True)
+    @mock.patch.object(os.path, 'realpath', autospec=True)
+    @mock.patch.object(os, 'remove', autospec=True)
+    @mock.patch.object(image_service.FileImageService, 'validate_href',
+                       autospec=True)
+    def test_download_symlink(self, _validate_mock, remove_mock,
+                              realpath_mock, link_mock, copy_mock):
+        _validate_mock.return_value = self.href_path
+        realpath_mock.side_effect = lambda p: p + '.real'
+        file_mock = mock.MagicMock(spec=io.BytesIO)
+        file_mock.name = 'file'
+        self.service.download(self.href, file_mock)
+        _validate_mock.assert_called_once_with(mock.ANY, self.href)
+        realpath_mock.assert_called_once_with(self.href_path)
+        link_mock.assert_called_once_with(self.href_path + '.real', 'file')
+        copy_mock.assert_not_called()
+
     @mock.patch.object(shutil, 'copyfile', autospec=True)
     @mock.patch.object(os, 'link', autospec=True)
     @mock.patch.object(os, 'remove', autospec=True)

releasenotes/notes/file-symlink-b65bd6b407bd1683.yaml

@@ -0,0 +1,6 @@
+---
+fixes:
+  - |
+    Fixes the behavior of ``file:///`` image URLs pointing at a symlink.
+    Ironic no longer creates a hard link to the symlink, which could cause
+    confusing FileNotFoundError to happen if the symlink is relative.