From 0b3ed093eafdf439dbeb092e72bea03747d314f7 Mon Sep 17 00:00:00 2001
From: Dmitry Tantsur <dtantsur@protonmail.com>
Date: Mon, 15 Jan 2024 19:13:18 +0100
Subject: [PATCH] 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] https://github.com/python/cpython/issues/81793

Change-Id: Ic52f0ddb0c94410dd854ee525e3c57b2e78ea84d
---
 ironic/common/image_service.py                | 16 ++++++++++++----
 .../tests/unit/common/test_image_service.py   | 19 +++++++++++++++++++
 .../notes/file-symlink-b65bd6b407bd1683.yaml  |  6 ++++++
 3 files changed, 37 insertions(+), 4 deletions(-)
 create mode 100644 releasenotes/notes/file-symlink-b65bd6b407bd1683.yaml

diff --git a/ironic/common/image_service.py b/ironic/common/image_service.py
index 453924b4e3..6197b1962e 100644
--- a/ironic/common/image_service.py
+++ b/ironic/common/image_service.py
@@ -322,13 +322,21 @@ class FileImageService(BaseImageService):
             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
 
diff --git a/ironic/tests/unit/common/test_image_service.py b/ironic/tests/unit/common/test_image_service.py
index fb9041e45d..a2451fedd9 100644
--- a/ironic/tests/unit/common/test_image_service.py
+++ b/ironic/tests/unit/common/test_image_service.py
@@ -612,6 +612,7 @@ class FileImageServiceTestCase(base.TestCase):
 
     @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 @@ class FileImageServiceTestCase(base.TestCase):
         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)
diff --git a/releasenotes/notes/file-symlink-b65bd6b407bd1683.yaml b/releasenotes/notes/file-symlink-b65bd6b407bd1683.yaml
new file mode 100644
index 0000000000..118c98efa4
--- /dev/null
+++ b/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.